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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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.
> 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 <[email protected]>
> > ---
> > 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.
>
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.
> 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.