LKML Archive on lore.kernel.org help / color / mirror / Atom feed
* [PATCH 0/2] scsi: ufs: Block user-space access if eh-in-progress @ 2021-11-08 12:08 Avri Altman 2021-11-08 12:08 ` [PATCH 1/2] scsi: ufs: Inline eh-in-progress states Avri Altman 2021-11-08 12:08 ` [PATCH 2/2] scsi: ufs: Return a bsg request immediatley if eh-in-progress Avri Altman 0 siblings, 2 replies; 7+ messages in thread From: Avri Altman @ 2021-11-08 12:08 UTC (permalink / raw) To: James E . J . Bottomley, Martin K . Petersen Cc: linux-scsi, linux-kernel, Adrian Hunter, Avri Altman If error handling is in-progress, keep things simple and do not allow user-space access until we are operational again. Avri Altman (2): scsi: ufs: Inline eh-in-progress states scsi: ufs: Return a bsg request immediatley if eh-in-progress drivers/scsi/ufs/ufshcd.c | 15 +++------------ drivers/scsi/ufs/ufshcd.h | 26 +++++++++++++++++++++++++- 2 files changed, 28 insertions(+), 13 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] scsi: ufs: Inline eh-in-progress states 2021-11-08 12:08 [PATCH 0/2] scsi: ufs: Block user-space access if eh-in-progress Avri Altman @ 2021-11-08 12:08 ` Avri Altman 2021-11-08 12:08 ` [PATCH 2/2] scsi: ufs: Return a bsg request immediatley if eh-in-progress Avri Altman 1 sibling, 0 replies; 7+ messages in thread From: Avri Altman @ 2021-11-08 12:08 UTC (permalink / raw) To: James E . J . Bottomley, Martin K . Petersen Cc: linux-scsi, linux-kernel, Adrian Hunter, Avri Altman Improve the static type checking. while at it, do not allow user-space access if eh-in-progress. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/scsi/ufs/ufshcd.c | 12 ------------ drivers/scsi/ufs/ufshcd.h | 26 +++++++++++++++++++++++++- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 470affdec426..3869bb57769b 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -140,11 +140,6 @@ static const char *const ufshcd_state_name[] = { [UFSHCD_STATE_EH_SCHEDULED_NON_FATAL] = "eh_non_fatal", }; -/* UFSHCD error handling flags */ -enum { - UFSHCD_EH_IN_PROGRESS = (1 << 0), -}; - /* UFSHCD UIC layer error flags */ enum { UFSHCD_UIC_DL_PA_INIT_ERROR = (1 << 0), /* Data link layer error */ @@ -156,13 +151,6 @@ enum { UFSHCD_UIC_PA_GENERIC_ERROR = (1 << 6), /* Generic PA error */ }; -#define ufshcd_set_eh_in_progress(h) \ - ((h)->eh_flags |= UFSHCD_EH_IN_PROGRESS) -#define ufshcd_eh_in_progress(h) \ - ((h)->eh_flags & UFSHCD_EH_IN_PROGRESS) -#define ufshcd_clear_eh_in_progress(h) \ - ((h)->eh_flags &= ~UFSHCD_EH_IN_PROGRESS) - struct ufs_pm_lvl_states ufs_pm_lvl_states[] = { [UFS_PM_LVL_0] = {UFS_ACTIVE_PWR_MODE, UIC_LINK_ACTIVE_STATE}, [UFS_PM_LVL_1] = {UFS_ACTIVE_PWR_MODE, UIC_LINK_HIBERN8_STATE}, diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 4ceb3c7e65b4..c5d98052a20a 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -992,9 +992,33 @@ static inline bool ufshcd_is_wb_allowed(struct ufs_hba *hba) return hba->caps & UFSHCD_CAP_WB_EN; } +/* UFSHCD error handling flags */ +enum { + UFSHCD_EH_IN_PROGRESS = (1 << 0), +}; + +static inline void ufshcd_set_eh_in_progress(struct ufs_hba *hba) +{ + lockdep_assert_held(hba->host->host_lock); + + hba->eh_flags |= UFSHCD_EH_IN_PROGRESS; +} + +static inline void ufshcd_clear_eh_in_progress(struct ufs_hba *hba) +{ + lockdep_assert_held(hba->host->host_lock); + + hba->eh_flags &= ~UFSHCD_EH_IN_PROGRESS; +} + +static inline bool ufshcd_eh_in_progress(struct ufs_hba *hba) +{ + return hba->eh_flags & UFSHCD_EH_IN_PROGRESS; +} + static inline bool ufshcd_is_user_access_allowed(struct ufs_hba *hba) { - return !hba->shutting_down; + return !hba->shutting_down && !ufshcd_eh_in_progress(hba); } #define ufshcd_writel(hba, val, reg) \ -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] scsi: ufs: Return a bsg request immediatley if eh-in-progress 2021-11-08 12:08 [PATCH 0/2] scsi: ufs: Block user-space access if eh-in-progress Avri Altman 2021-11-08 12:08 ` [PATCH 1/2] scsi: ufs: Inline eh-in-progress states Avri Altman @ 2021-11-08 12:08 ` Avri Altman 2021-11-08 17:16 ` Bart Van Assche 1 sibling, 1 reply; 7+ messages in thread From: Avri Altman @ 2021-11-08 12:08 UTC (permalink / raw) To: James E . J . Bottomley, Martin K . Petersen Cc: linux-scsi, linux-kernel, Adrian Hunter, Avri Altman ufs-bsg is attempting to access the device from user-space, and it is unaware of the internal driver flows, specifically if error handling is currently ongoing. Fixes: 5e0a86eed846 (scsi: ufs: Add API to execute raw upiu commands) Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/scsi/ufs/ufshcd.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 3869bb57769b..828061c05909 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -6830,6 +6830,9 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, enum utp_ocs ocs_value; u8 tm_f = be32_to_cpu(req_upiu->header.dword_1) >> 16 & MASK_TM_FUNC; + if (!ufshcd_is_user_access_allowed(hba)) + return -EBUSY; + switch (msgcode) { case UPIU_TRANSACTION_NOP_OUT: cmd_type = DEV_CMD_TYPE_NOP; -- 2.17.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] scsi: ufs: Return a bsg request immediatley if eh-in-progress 2021-11-08 12:08 ` [PATCH 2/2] scsi: ufs: Return a bsg request immediatley if eh-in-progress Avri Altman @ 2021-11-08 17:16 ` Bart Van Assche 2021-11-08 17:24 ` Avri Altman 0 siblings, 1 reply; 7+ messages in thread From: Bart Van Assche @ 2021-11-08 17:16 UTC (permalink / raw) To: Avri Altman, James E . J . Bottomley, Martin K . Petersen Cc: linux-scsi, linux-kernel, Adrian Hunter On 11/8/21 4:08 AM, Avri Altman wrote: > ufs-bsg is attempting to access the device from user-space, and it is > unaware of the internal driver flows, specifically if error handling is > currently ongoing. > > Fixes: 5e0a86eed846 (scsi: ufs: Add API to execute raw upiu commands) > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > --- > drivers/scsi/ufs/ufshcd.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 3869bb57769b..828061c05909 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -6830,6 +6830,9 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, > enum utp_ocs ocs_value; > u8 tm_f = be32_to_cpu(req_upiu->header.dword_1) >> 16 & MASK_TM_FUNC; > > + if (!ufshcd_is_user_access_allowed(hba)) > + return -EBUSY; > + > switch (msgcode) { > case UPIU_TRANSACTION_NOP_OUT: > cmd_type = DEV_CMD_TYPE_NOP; Making operations fail if error handling is in progress makes it harder than necessary to write user space software that uses the BSG interface. Has it been considered to wait inside ufshcd_exec_raw_upiu_cmd() until error handling has finished? Thanks, Bart. ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 2/2] scsi: ufs: Return a bsg request immediatley if eh-in-progress 2021-11-08 17:16 ` Bart Van Assche @ 2021-11-08 17:24 ` Avri Altman 2021-11-08 17:32 ` Bart Van Assche 0 siblings, 1 reply; 7+ messages in thread From: Avri Altman @ 2021-11-08 17:24 UTC (permalink / raw) To: Bart Van Assche, James E . J . Bottomley, Martin K . Petersen Cc: linux-scsi, linux-kernel, Adrian Hunter > On 11/8/21 4:08 AM, Avri Altman wrote: > > ufs-bsg is attempting to access the device from user-space, and it is > > unaware of the internal driver flows, specifically if error handling is > > currently ongoing. > > > > Fixes: 5e0a86eed846 (scsi: ufs: Add API to execute raw upiu commands) > > > > Signed-off-by: Avri Altman <avri.altman@wdc.com> > > --- > > drivers/scsi/ufs/ufshcd.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > > index 3869bb57769b..828061c05909 100644 > > --- a/drivers/scsi/ufs/ufshcd.c > > +++ b/drivers/scsi/ufs/ufshcd.c > > @@ -6830,6 +6830,9 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba > *hba, > > enum utp_ocs ocs_value; > > u8 tm_f = be32_to_cpu(req_upiu->header.dword_1) >> 16 & > MASK_TM_FUNC; > > > > + if (!ufshcd_is_user_access_allowed(hba)) > > + return -EBUSY; > > + > > switch (msgcode) { > > case UPIU_TRANSACTION_NOP_OUT: > > cmd_type = DEV_CMD_TYPE_NOP; > > Making operations fail if error handling is in progress makes it harder than > necessary to write user space software that uses the BSG interface. Has it > been considered to wait inside ufshcd_exec_raw_upiu_cmd() until error > handling > has finished? I am not sure. I would expect a retry / polling / other, if any, to be done in user-space and not in the kernel. e.g. a common practice in the code that send SG_IO or other ioctls is to retry on EBUSY. Not sure that this is the case in ufs-utils though. Thanks, Avri Thanks, Avri > > Thanks, > > Bart. > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] scsi: ufs: Return a bsg request immediatley if eh-in-progress 2021-11-08 17:24 ` Avri Altman @ 2021-11-08 17:32 ` Bart Van Assche 2021-11-08 18:00 ` Avri Altman 0 siblings, 1 reply; 7+ messages in thread From: Bart Van Assche @ 2021-11-08 17:32 UTC (permalink / raw) To: Avri Altman, James E . J . Bottomley, Martin K . Petersen Cc: linux-scsi, linux-kernel, Adrian Hunter On 11/8/21 9:24 AM, Avri Altman wrote: > I am not sure. I would expect a retry / polling / other, if any, to > be done in user-space and not in the kernel. e.g. a common practice > in the code that send SG_IO or other ioctls is to retry on EBUSY. Not > sure that this is the case in ufs-utils though. Shouldn't we aim to make sure that user space code does not have to use busy waiting? Thanks, Bart. ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 2/2] scsi: ufs: Return a bsg request immediatley if eh-in-progress 2021-11-08 17:32 ` Bart Van Assche @ 2021-11-08 18:00 ` Avri Altman 0 siblings, 0 replies; 7+ messages in thread From: Avri Altman @ 2021-11-08 18:00 UTC (permalink / raw) To: Bart Van Assche, James E . J . Bottomley, Martin K . Petersen Cc: linux-scsi, linux-kernel, Adrian Hunter > On 11/8/21 9:24 AM, Avri Altman wrote: > > I am not sure. I would expect a retry / polling / other, if any, to be > > done in user-space and not in the kernel. e.g. a common practice in > > the code that send SG_IO or other ioctls is to retry on EBUSY. Not > > sure that this is the case in ufs-utils though. > Shouldn't we aim to make sure that user space code does not have to use > busy waiting? I don't know. Waiting in the kernel seems like an unnecessary complication. If you find it useless, better to just drop it. I looked it up in ufs-utils public repository (https://github.com/westerndigitalcorporation/ufs-utils), and it looks like that: while (((ret = ioctl(fd, SG_IO, &io_hdr_v4)) < 0) && ((errno == EINTR) || (errno == EAGAIN))) ; Thanks, Avri > > Thanks, > > Bart. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-08 18:00 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-08 12:08 [PATCH 0/2] scsi: ufs: Block user-space access if eh-in-progress Avri Altman 2021-11-08 12:08 ` [PATCH 1/2] scsi: ufs: Inline eh-in-progress states Avri Altman 2021-11-08 12:08 ` [PATCH 2/2] scsi: ufs: Return a bsg request immediatley if eh-in-progress Avri Altman 2021-11-08 17:16 ` Bart Van Assche 2021-11-08 17:24 ` Avri Altman 2021-11-08 17:32 ` Bart Van Assche 2021-11-08 18:00 ` Avri Altman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).