| Message ID | 20240228111521.3864-1-WeitaoWang-oc@zhaoxin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] USB:UAS:return ENODEV when submit urbs fail with device not attached. | expand |
On 28.02.24 12:15, Weitao Wang wrote: Hi, sorry for going at this again, but there are a few technical issues left. Regards Oliver > --- > v1->v2 > - Modify the description of this patch. > > drivers/usb/storage/uas.c | 21 ++++++++++----------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c > index 9707f53cfda9..967f18db525a 100644 > --- a/drivers/usb/storage/uas.c > +++ b/drivers/usb/storage/uas.c > @@ -562,9 +561,9 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, > > lockdep_assert_held(&devinfo->lock); > if (cmdinfo->state & SUBMIT_STATUS_URB) { > - urb = uas_submit_sense_urb(cmnd, GFP_ATOMIC); > - if (!urb) > - return SCSI_MLQUEUE_DEVICE_BUSY; > + err = uas_submit_sense_urb(cmnd, GFP_ATOMIC); > + if (err) > + return (err == -ENODEV) ? -ENODEV : SCSI_MLQUEUE_DEVICE_BUSY; Either we ought to use SCSI error codes or generic error codes. There is no need to translate all but one error condition here. > cmdinfo->state &= ~SUBMIT_STATUS_URB; > } > > @@ -582,7 +581,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, > if (err) { > usb_unanchor_urb(cmdinfo->data_in_urb); > uas_log_cmd_state(cmnd, "data in submit err", err); > - return SCSI_MLQUEUE_DEVICE_BUSY; > + return (err == -ENODEV) ? -ENODEV : SCSI_MLQUEUE_DEVICE_BUSY; Same as above and below.
On 28.02.24 23:32, WeitaoWang-oc@zhaoxin.com wrote: > @@ -602,6 +606,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, > if (err) { > usb_unanchor_urb(cmdinfo->data_out_urb); > uas_log_cmd_state(cmnd, "data out submit err", err); > + if (err == -ENODEV) > + return -ENODEV; This is a generic error code from errno.h > return SCSI_MLQUEUE_DEVICE_BUSY; This is not. > } > cmdinfo->state &= ~SUBMIT_DATA_OUT_URB; > @@ -621,6 +627,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, > if (err) { > usb_unanchor_urb(cmdinfo->cmd_urb); > uas_log_cmd_state(cmnd, "cmd submit err", err); > + if (err == -ENODEV) > + return -ENODEV; > return SCSI_MLQUEUE_DEVICE_BUSY; > } > > I'm not sure I fully understand what your mean. > Whether the above code is more reasonable? If not,could you give me some > suggestion? Thanks for your help! You want to change uas_submit_urbs() to return the reason for errors, because -ENODEV needs to be handled differently. That is good. But why don't you just do return err; unconditionally? There is no point in using SCSI_MLQUEUE_DEVICE_BUSY Regards Oliver
On 2024/2/28 16:00, Oliver Neukum wrote: > >> @@ -562,9 +561,9 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, >> >> lockdep_assert_held(&devinfo->lock); >> if (cmdinfo->state & SUBMIT_STATUS_URB) { >> - urb = uas_submit_sense_urb(cmnd, GFP_ATOMIC); >> - if (!urb) >> - return SCSI_MLQUEUE_DEVICE_BUSY; >> + err = uas_submit_sense_urb(cmnd, GFP_ATOMIC); >> + if (err) >> + return (err == -ENODEV) ? -ENODEV : SCSI_MLQUEUE_DEVICE_BUSY; > > Either we ought to use SCSI error codes or generic error codes. > There is no need to translate all but one error condition here. > static int uas_submit_urbs(struct scsi_cmnd *cmnd, @@ -562,10 +561,13 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, lockdep_assert_held(&devinfo->lock); if (cmdinfo->state & SUBMIT_STATUS_URB) { - urb = uas_submit_sense_urb(cmnd, GFP_ATOMIC); - if (!urb) + err = uas_submit_sense_urb(cmnd, GFP_ATOMIC); + if (!err) + cmdinfo->state &= ~SUBMIT_STATUS_URB; + else if (err == -ENODEV) + return -ENODEV; + else return SCSI_MLQUEUE_DEVICE_BUSY; - cmdinfo->state &= ~SUBMIT_STATUS_URB; } if (cmdinfo->state & ALLOC_DATA_IN_URB) { @@ -582,6 +584,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, if (err) { usb_unanchor_urb(cmdinfo->data_in_urb); uas_log_cmd_state(cmnd, "data in submit err", err); + if (err == -ENODEV) + return -ENODEV; return SCSI_MLQUEUE_DEVICE_BUSY; } cmdinfo->state &= ~SUBMIT_DATA_IN_URB; @@ -602,6 +606,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, if (err) { usb_unanchor_urb(cmdinfo->data_out_urb); uas_log_cmd_state(cmnd, "data out submit err", err); + if (err == -ENODEV) + return -ENODEV; return SCSI_MLQUEUE_DEVICE_BUSY; } cmdinfo->state &= ~SUBMIT_DATA_OUT_URB; @@ -621,6 +627,8 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, if (err) { usb_unanchor_urb(cmdinfo->cmd_urb); uas_log_cmd_state(cmnd, "cmd submit err", err); + if (err == -ENODEV) + return -ENODEV; return SCSI_MLQUEUE_DEVICE_BUSY; } I'm not sure I fully understand what your mean. Whether the above code is more reasonable? If not,could you give me some suggestion? Thanks for your help! Weitao
On 29.02.24 12:19, WeitaoWang-oc@zhaoxin.com wrote: > When alloc urb fail in the same function uas_submit_urbs, > whether we should replace SCSI_MLQUEUE_DEVICE_BUSY with generic > error code -ENOMEM? Such like this: > > @@ -572,7 +571,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, > cmdinfo->data_in_urb = uas_alloc_data_urb(devinfo, GFP_ATOMIC, > cmnd, DMA_FROM_DEVICE); > if (!cmdinfo->data_in_urb) > - return SCSI_MLQUEUE_DEVICE_BUSY; > + return -ENOMEM; > cmdinfo->state &= ~ALLOC_DATA_IN_URB; > } Hi, yes, and then you translate in one central place for the SCSI layer into DID_ERROR or DID_NO_CONNECT. Regards Oliver
On 29.02.24 17:40, WeitaoWang-oc@zhaoxin.com wrote:
> OK, I'll submit a new version after you help to review the following patch.
Hi,
perfect.
Regards
Oliver
On 2024/2/28 22:47, Oliver Neukum wrote: >> I'm not sure I fully understand what your mean. >> Whether the above code is more reasonable? If not,could you give me some >> suggestion? Thanks for your help! > > You want to change uas_submit_urbs() to return the reason for > errors, because -ENODEV needs to be handled differently. That > is good. > But why don't you just do > > return err; > > unconditionally? There is no point in using SCSI_MLQUEUE_DEVICE_BUSY I got it, Thanks. New patch would like this sample: @@ -562,9 +561,9 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, lockdep_assert_held(&devinfo->lock); if (cmdinfo->state & SUBMIT_STATUS_URB) { - urb = uas_submit_sense_urb(cmnd, GFP_ATOMIC); - if (!urb) - return SCSI_MLQUEUE_DEVICE_BUSY; + err = uas_submit_sense_urb(cmnd, GFP_ATOMIC); + if (err) + return err; cmdinfo->state &= ~SUBMIT_STATUS_URB; } @@ -582,7 +581,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, if (err) { usb_unanchor_urb(cmdinfo->data_in_urb); uas_log_cmd_state(cmnd, "data in submit err", err); - return SCSI_MLQUEUE_DEVICE_BUSY; + return err; } When alloc urb fail in the same function uas_submit_urbs, whether we should replace SCSI_MLQUEUE_DEVICE_BUSY with generic error code -ENOMEM? Such like this: @@ -572,7 +571,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, cmdinfo->data_in_urb = uas_alloc_data_urb(devinfo, GFP_ATOMIC, cmnd, DMA_FROM_DEVICE); if (!cmdinfo->data_in_urb) - return SCSI_MLQUEUE_DEVICE_BUSY; + return -ENOMEM; cmdinfo->state &= ~ALLOC_DATA_IN_URB; } Thanks and Best regards, Weitao
On 2024/2/29 16:08, Oliver Neukum wrote: > > On 29.02.24 12:19, WeitaoWang-oc@zhaoxin.com wrote: > >> When alloc urb fail in the same function uas_submit_urbs, >> whether we should replace SCSI_MLQUEUE_DEVICE_BUSY with generic >> error code -ENOMEM? Such like this: >> >> @@ -572,7 +571,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, >> cmdinfo->data_in_urb = uas_alloc_data_urb(devinfo, GFP_ATOMIC, >> cmnd, DMA_FROM_DEVICE); >> if (!cmdinfo->data_in_urb) >> - return SCSI_MLQUEUE_DEVICE_BUSY; >> + return -ENOMEM; >> cmdinfo->state &= ~ALLOC_DATA_IN_URB; >> } > > Hi, > > yes, and then you translate in one central place for the SCSI layer > into DID_ERROR or DID_NO_CONNECT. > OK, I'll submit a new version after you help to review the following patch. diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 9707f53cfda9..689396777b6f 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -533,7 +533,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp, * daft to me. */ -static struct urb *uas_submit_sense_urb(struct scsi_cmnd *cmnd, gfp_t gfp) +static int uas_submit_sense_urb(struct scsi_cmnd *cmnd, gfp_t gfp) { struct uas_dev_info *devinfo = cmnd->device->hostdata; struct urb *urb; @@ -541,16 +541,15 @@ static struct urb *uas_submit_sense_urb(struct scsi_cmnd *cmnd, gfp_t gfp) urb = uas_alloc_sense_urb(devinfo, gfp, cmnd); if (!urb) - return NULL; + return -ENOMEM; usb_anchor_urb(urb, &devinfo->sense_urbs); err = usb_submit_urb(urb, gfp); if (err) { usb_unanchor_urb(urb); uas_log_cmd_state(cmnd, "sense submit err", err); usb_free_urb(urb); - return NULL; } - return urb; + return err; } static int uas_submit_urbs(struct scsi_cmnd *cmnd, @@ -562,9 +561,9 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, lockdep_assert_held(&devinfo->lock); if (cmdinfo->state & SUBMIT_STATUS_URB) { - urb = uas_submit_sense_urb(cmnd, GFP_ATOMIC); - if (!urb) - return SCSI_MLQUEUE_DEVICE_BUSY; + err = uas_submit_sense_urb(cmnd, GFP_ATOMIC); + if (err) + return err; cmdinfo->state &= ~SUBMIT_STATUS_URB; } @@ -572,7 +571,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, cmdinfo->data_in_urb = uas_alloc_data_urb(devinfo, GFP_ATOMIC, cmnd, DMA_FROM_DEVICE); if (!cmdinfo->data_in_urb) - return SCSI_MLQUEUE_DEVICE_BUSY; + return -ENOMEM; cmdinfo->state &= ~ALLOC_DATA_IN_URB; } @@ -582,7 +581,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, if (err) { usb_unanchor_urb(cmdinfo->data_in_urb); uas_log_cmd_state(cmnd, "data in submit err", err); - return SCSI_MLQUEUE_DEVICE_BUSY; + return err; } cmdinfo->state &= ~SUBMIT_DATA_IN_URB; cmdinfo->state |= DATA_IN_URB_INFLIGHT; @@ -592,7 +591,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, cmdinfo->data_out_urb = uas_alloc_data_urb(devinfo, GFP_ATOMIC, cmnd, DMA_TO_DEVICE); if (!cmdinfo->data_out_urb) - return SCSI_MLQUEUE_DEVICE_BUSY; + return -ENOMEM; cmdinfo->state &= ~ALLOC_DATA_OUT_URB; } @@ -602,7 +601,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, if (err) { usb_unanchor_urb(cmdinfo->data_out_urb); uas_log_cmd_state(cmnd, "data out submit err", err); - return SCSI_MLQUEUE_DEVICE_BUSY; + return err; } cmdinfo->state &= ~SUBMIT_DATA_OUT_URB; cmdinfo->state |= DATA_OUT_URB_INFLIGHT; @@ -611,7 +610,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, if (cmdinfo->state & ALLOC_CMD_URB) { cmdinfo->cmd_urb = uas_alloc_cmd_urb(devinfo, GFP_ATOMIC, cmnd); if (!cmdinfo->cmd_urb) - return SCSI_MLQUEUE_DEVICE_BUSY; + return -ENOMEM; cmdinfo->state &= ~ALLOC_CMD_URB; } @@ -621,7 +620,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, if (err) { usb_unanchor_urb(cmdinfo->cmd_urb); uas_log_cmd_state(cmnd, "cmd submit err", err); - return SCSI_MLQUEUE_DEVICE_BUSY; + return err; } cmdinfo->cmd_urb = NULL; cmdinfo->state &= ~SUBMIT_CMD_URB; @@ -698,7 +697,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd) * of queueing, no matter how fatal the error */ if (err == -ENODEV) { - set_host_byte(cmnd, DID_ERROR); + set_host_byte(cmnd, DID_NO_CONNECT); scsi_done(cmnd); goto zombie; } Thanks and best regards, weitao
diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c index 9707f53cfda9..967f18db525a 100644 --- a/drivers/usb/storage/uas.c +++ b/drivers/usb/storage/uas.c @@ -533,7 +533,7 @@ static struct urb *uas_alloc_cmd_urb(struct uas_dev_info *devinfo, gfp_t gfp, * daft to me. */ -static struct urb *uas_submit_sense_urb(struct scsi_cmnd *cmnd, gfp_t gfp) +static int uas_submit_sense_urb(struct scsi_cmnd *cmnd, gfp_t gfp) { struct uas_dev_info *devinfo = cmnd->device->hostdata; struct urb *urb; @@ -541,16 +541,15 @@ static struct urb *uas_submit_sense_urb(struct scsi_cmnd *cmnd, gfp_t gfp) urb = uas_alloc_sense_urb(devinfo, gfp, cmnd); if (!urb) - return NULL; + return -ENOMEM; usb_anchor_urb(urb, &devinfo->sense_urbs); err = usb_submit_urb(urb, gfp); if (err) { usb_unanchor_urb(urb); uas_log_cmd_state(cmnd, "sense submit err", err); usb_free_urb(urb); - return NULL; } - return urb; + return err; } static int uas_submit_urbs(struct scsi_cmnd *cmnd, @@ -562,9 +561,9 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, lockdep_assert_held(&devinfo->lock); if (cmdinfo->state & SUBMIT_STATUS_URB) { - urb = uas_submit_sense_urb(cmnd, GFP_ATOMIC); - if (!urb) - return SCSI_MLQUEUE_DEVICE_BUSY; + err = uas_submit_sense_urb(cmnd, GFP_ATOMIC); + if (err) + return (err == -ENODEV) ? -ENODEV : SCSI_MLQUEUE_DEVICE_BUSY; cmdinfo->state &= ~SUBMIT_STATUS_URB; } @@ -582,7 +581,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, if (err) { usb_unanchor_urb(cmdinfo->data_in_urb); uas_log_cmd_state(cmnd, "data in submit err", err); - return SCSI_MLQUEUE_DEVICE_BUSY; + return (err == -ENODEV) ? -ENODEV : SCSI_MLQUEUE_DEVICE_BUSY; } cmdinfo->state &= ~SUBMIT_DATA_IN_URB; cmdinfo->state |= DATA_IN_URB_INFLIGHT; @@ -602,7 +601,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, if (err) { usb_unanchor_urb(cmdinfo->data_out_urb); uas_log_cmd_state(cmnd, "data out submit err", err); - return SCSI_MLQUEUE_DEVICE_BUSY; + return (err == -ENODEV) ? -ENODEV : SCSI_MLQUEUE_DEVICE_BUSY; } cmdinfo->state &= ~SUBMIT_DATA_OUT_URB; cmdinfo->state |= DATA_OUT_URB_INFLIGHT; @@ -621,7 +620,7 @@ static int uas_submit_urbs(struct scsi_cmnd *cmnd, if (err) { usb_unanchor_urb(cmdinfo->cmd_urb); uas_log_cmd_state(cmnd, "cmd submit err", err); - return SCSI_MLQUEUE_DEVICE_BUSY; + return (err == -ENODEV) ? -ENODEV : SCSI_MLQUEUE_DEVICE_BUSY; } cmdinfo->cmd_urb = NULL; cmdinfo->state &= ~SUBMIT_CMD_URB; @@ -698,7 +697,7 @@ static int uas_queuecommand_lck(struct scsi_cmnd *cmnd) * of queueing, no matter how fatal the error */ if (err == -ENODEV) { - set_host_byte(cmnd, DID_ERROR); + set_host_byte(cmnd, DID_NO_CONNECT); scsi_done(cmnd); goto zombie; }
In the scenario of entering hibernation with udisk in the system, if the udisk was gone or resume fail in the thaw phase of hibernation. Its state will be set to NOTATTACHED. At this point, usb_hub_wq was already freezed and can't not handle disconnect event. Next, in the poweroff phase of hibernation, SYNCHRONIZE_CACHE SCSI command will be sent to this udisk when poweroff this scsi device, which will cause uas_submit_urbs to be called to submit URB for sense/data/cmd pipe. However, these URBs will submit fail as device was set to NOTATTACHED state. Then, uas_submit_urbs will return a value SCSI_MLQUEUE_DEVICE_BUSY to the caller. That will lead the SCSI layer go into an ugly loop and system fail to go into hibernation. On the other hand, when we specially check for -ENODEV in function uas_queuecommand_lck, returning DID_ERROR to SCSI layer will cause device poweroff fail and system shutdown instead of entering hibernation. To fix this issue, let uas_submit_urbs function to return a value -ENODEV when submit URB fail with device in NOTATTACHED state. At the same time, we need to translate -ENODEV to DID_NOT_CONNECT for the SCSI layer. Cc: stable@vger.kernel.org Signed-off-by: Weitao Wang <WeitaoWang-oc@zhaoxin.com> --- v1->v2 - Modify the description of this patch. drivers/usb/storage/uas.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)