| Message ID | 20240122105138.3759477-1-yuanlinyu@hihonor.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1] usb: f_mass_storage: forbid async queue when shutdown happen | expand |
On Mon, Jan 22, 2024 at 06:51:38PM +0800, yuan linyu wrote: > When write UDC to empty and unbind gadget driver from gadget device, it is > possible that there are many queue failures for mass storage function. > > The root cause is mass storage main thread alaways try to queue request to > receive a command from host if running flag is on, on platform like dwc3, > if pull down called, it will not queue request again and return > -ESHUTDOWN, but it not affect running flag of mass storage function. > > Check return code from mass storage function and clear running flag if it > is -ESHUTDOWN, also indicate start in/out transfer failure to break loops. > > Signed-off-by: yuan linyu <yuanlinyu@hihonor.com> > --- > v1: follow Alan suggestion, only limit change in f_mass_storage > RFC: https://lore.kernel.org/linux-usb/5f4c9d8b6e0e4e73a5b3b1540a500b6a@hihonor.com/T/#t This is basically okay. I have a suggestion for improvement, see below. > drivers/usb/gadget/function/f_mass_storage.c | 28 +++++++++++++++++--- > 1 file changed, 24 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c > index 722a3ab2b337..d5174e066078 100644 > --- a/drivers/usb/gadget/function/f_mass_storage.c > +++ b/drivers/usb/gadget/function/f_mass_storage.c > @@ -545,21 +545,41 @@ static int start_transfer(struct fsg_dev *fsg, struct usb_ep *ep, > > static bool start_in_transfer(struct fsg_common *common, struct fsg_buffhd *bh) > { > + int rc; > + > if (!fsg_is_set(common)) > return false; > bh->state = BUF_STATE_SENDING; > - if (start_transfer(common->fsg, common->fsg->bulk_in, bh->inreq)) > - bh->state = BUF_STATE_EMPTY; > + rc = start_transfer(common->fsg, common->fsg->bulk_in, bh->inreq); > + if (!rc) > + return true; > + > + bh->state = BUF_STATE_EMPTY; > + if (rc == -ESHUTDOWN) { > + common->running = 0; > + return false; > + } > + > return true; > } This could be written more cleanly as: rc = start_transfer(common->fsg, common->fsg->bulk_in, bh->inreq); if (rc) { bh->state = BUF_STATE_EMPTY; if (rc == -ESHUTDOWN) { common->running = 0; return false; } } return true; And the same goes for start_out_transfer(). Have you tested this? Does it do what you want? Alan Stern
> From: Alan Stern <stern@rowland.harvard.edu> > Sent: Monday, January 22, 2024 11:08 PM > To: yuanlinyu <yuanlinyu@hihonor.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; > linux-usb@vger.kernel.org > Subject: Re: [PATCH v1] usb: f_mass_storage: forbid async queue when > shutdown happen > > > --- a/drivers/usb/gadget/function/f_mass_storage.c > > +++ b/drivers/usb/gadget/function/f_mass_storage.c > > @@ -545,21 +545,41 @@ static int start_transfer(struct fsg_dev *fsg, struct > usb_ep *ep, > > > > This could be written more cleanly as: > > rc = start_transfer(common->fsg, common->fsg->bulk_in, bh->inreq); > if (rc) { > bh->state = BUF_STATE_EMPTY; > if (rc == -ESHUTDOWN) { > common->running = 0; > return false; > } > } > return true; Sure, will send v2. > > And the same goes for start_out_transfer(). > > Have you tested this? Does it do what you want? Yes, test the change, before the change, there are many logs, now only one time. > > Alan Stern
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c index 722a3ab2b337..d5174e066078 100644 --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -545,21 +545,41 @@ static int start_transfer(struct fsg_dev *fsg, struct usb_ep *ep, static bool start_in_transfer(struct fsg_common *common, struct fsg_buffhd *bh) { + int rc; + if (!fsg_is_set(common)) return false; bh->state = BUF_STATE_SENDING; - if (start_transfer(common->fsg, common->fsg->bulk_in, bh->inreq)) - bh->state = BUF_STATE_EMPTY; + rc = start_transfer(common->fsg, common->fsg->bulk_in, bh->inreq); + if (!rc) + return true; + + bh->state = BUF_STATE_EMPTY; + if (rc == -ESHUTDOWN) { + common->running = 0; + return false; + } + return true; } static bool start_out_transfer(struct fsg_common *common, struct fsg_buffhd *bh) { + int rc; + if (!fsg_is_set(common)) return false; bh->state = BUF_STATE_RECEIVING; - if (start_transfer(common->fsg, common->fsg->bulk_out, bh->outreq)) - bh->state = BUF_STATE_FULL; + rc = start_transfer(common->fsg, common->fsg->bulk_out, bh->outreq); + if (!rc) + return true; + + bh->state = BUF_STATE_FULL; + if (rc == -ESHUTDOWN) { + common->running = 0; + return false; + } + return true; }
When write UDC to empty and unbind gadget driver from gadget device, it is possible that there are many queue failures for mass storage function. The root cause is mass storage main thread alaways try to queue request to receive a command from host if running flag is on, on platform like dwc3, if pull down called, it will not queue request again and return -ESHUTDOWN, but it not affect running flag of mass storage function. Check return code from mass storage function and clear running flag if it is -ESHUTDOWN, also indicate start in/out transfer failure to break loops. Signed-off-by: yuan linyu <yuanlinyu@hihonor.com> --- v1: follow Alan suggestion, only limit change in f_mass_storage RFC: https://lore.kernel.org/linux-usb/5f4c9d8b6e0e4e73a5b3b1540a500b6a@hihonor.com/T/#t drivers/usb/gadget/function/f_mass_storage.c | 28 +++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-)