[PATCH 0/2] scsi: ufs: Block user-space access if eh-in-progress

[PATCH 0/2] scsi: ufs: Block user-space access if eh-in-progress

[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

[PATCH 1/2] scsi: ufs: Inline eh-in-progress states

[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

[PATCH 2/2] scsi: ufs: Return a bsg request immediatley if eh-in-progress

[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

Re: [PATCH 2/2] scsi: ufs: Return a bsg request immediatley if eh-in-progress

[Bart Van Assche]

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.

RE: [PATCH 2/2] scsi: ufs: Return a bsg request immediatley if eh-in-progress

[Avri Altman]

> 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.
> 

Re: [PATCH 2/2] scsi: ufs: Return a bsg request immediatley if eh-in-progress

[Bart Van Assche]

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.

RE: [PATCH 2/2] scsi: ufs: Return a bsg request immediatley if eh-in-progress

[Avri Altman]

 
> 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.