| Message ID | 5f9fbabd-2e2c-9389-446d-3bd1dd954a82@faberman.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: f_mass_storage: Shut down mass storage device when USB connection is shut, down. | expand |
On Sat, Sep 04, 2021 at 05:32:58PM +0200, Florian Faber wrote: > f_mass_storage continues to send out packets after the connection to the USB > host has been terminated, ignoring the error status. The driver doesn't send packets after the connection has been terminated -- that would be impossible. It may _try_ to send packets, but it can't actually send anything unless the hosts asks for the data. There's nothing wrong with trying and failing. > Signed-off-by: Florian Faber <faber@faberman.de> > > --- > drivers/usb/gadget/function/f_mass_storage.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c > b/drivers/usb/gadget/function/f_mass_storage.c > index 6ad669dde41c..1e73ba629e43 100644 > --- a/drivers/usb/gadget/function/f_mass_storage.c > +++ b/drivers/usb/gadget/function/f_mass_storage.c > @@ -529,6 +529,10 @@ static int start_transfer(struct fsg_dev *fsg, struct > usb_ep *ep, > /* We can't do much more than wait for a reset */ > req->status = rc; > > + if (rc==-ESHUTDOWN) { > + fsg->common->running = 0; > + } > + This is wrong because it isn't what common->running means. common->running means that the mass-storage configuration has been set by the host; it has nothing to do with whether or not the USB cable is connected. Also the new code doesn't conform to the kernel's programming style (missing spaces around the "==" sign and braces not needed for a single-line conditional statement). And if it were a serious change, it should be merged with the conditional statement that follows, which also tests whether rc is -ESHUTDOWN. Alan Stern
On 9/4/21 6:09 PM, Alan Stern wrote: > On Sat, Sep 04, 2021 at 05:32:58PM +0200, Florian Faber wrote: >> f_mass_storage continues to send out packets after the connection to the USB >> host has been terminated, ignoring the error status. > > The driver doesn't send packets after the connection has been terminated -- > that would be impossible. It may _try_ to send packets, but it can't > actually send anything unless the hosts asks for the data. There's nothing > wrong with trying and failing. Correct, that could have been expressed more clearly. > >> Signed-off-by: Florian Faber <faber@faberman.de> >> >> --- >> drivers/usb/gadget/function/f_mass_storage.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/usb/gadget/function/f_mass_storage.c >> b/drivers/usb/gadget/function/f_mass_storage.c >> index 6ad669dde41c..1e73ba629e43 100644 >> --- a/drivers/usb/gadget/function/f_mass_storage.c >> +++ b/drivers/usb/gadget/function/f_mass_storage.c >> @@ -529,6 +529,10 @@ static int start_transfer(struct fsg_dev *fsg, struct >> usb_ep *ep, >> /* We can't do much more than wait for a reset */ >> req->status = rc; >> >> + if (rc==-ESHUTDOWN) { >> + fsg->common->running = 0; >> + } >> + > > This is wrong because it isn't what common->running means. common->running > means that the mass-storage configuration has been set by the host; it has > nothing to do with whether or not the USB cable is connected. So it is OK to keep the thread running and _trying_ to send out packets all the time until another USB host is plugged in, wasting resources? I would argue that the removal of a USB host is an implicit configuration change. Flo
On Sat, Sep 04, 2021 at 06:17:36PM +0200, Florian Faber wrote: > > On 9/4/21 6:09 PM, Alan Stern wrote: > > On Sat, Sep 04, 2021 at 05:32:58PM +0200, Florian Faber wrote: > > > f_mass_storage continues to send out packets after the connection to the USB > > > host has been terminated, ignoring the error status. > > > > The driver doesn't send packets after the connection has been terminated -- > > that would be impossible. It may _try_ to send packets, but it can't > > actually send anything unless the hosts asks for the data. There's nothing > > wrong with trying and failing. > > Correct, that could have been expressed more clearly. > > > > > > Signed-off-by: Florian Faber <faber@faberman.de> > > > > > > --- > > > drivers/usb/gadget/function/f_mass_storage.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/drivers/usb/gadget/function/f_mass_storage.c > > > b/drivers/usb/gadget/function/f_mass_storage.c > > > index 6ad669dde41c..1e73ba629e43 100644 > > > --- a/drivers/usb/gadget/function/f_mass_storage.c > > > +++ b/drivers/usb/gadget/function/f_mass_storage.c > > > @@ -529,6 +529,10 @@ static int start_transfer(struct fsg_dev *fsg, struct > > > usb_ep *ep, > > > /* We can't do much more than wait for a reset */ > > > req->status = rc; > > > > > > + if (rc==-ESHUTDOWN) { > > > + fsg->common->running = 0; > > > + } > > > + > > > > This is wrong because it isn't what common->running means. common->running > > means that the mass-storage configuration has been set by the host; it has > > nothing to do with whether or not the USB cable is connected. > > So it is OK to keep the thread running and _trying_ to send out packets all > the time until another USB host is plugged in, wasting resources? Not all the time. When the disconnect occurs, the composite core's disconnect handler will reset the current configuration, thereby calling the driver's do_set_interface() routine, which will set common->running to 0. > I would argue that the removal of a USB host is an implicit configuration > change. Indeed it is. And it is treated as such by the existing code. Alan Stern
diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c index 6ad669dde41c..1e73ba629e43 100644 --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -529,6 +529,10 @@ static int start_transfer(struct fsg_dev *fsg, struct usb_ep *ep, /* We can't do much more than wait for a reset */ req->status = rc; + if (rc==-ESHUTDOWN) { + fsg->common->running = 0; + } + /* * Note: currently the net2280 driver fails zero-length
f_mass_storage continues to send out packets after the connection to the USB host has been terminated, ignoring the error status. Signed-off-by: Florian Faber <faber@faberman.de> --- drivers/usb/gadget/function/f_mass_storage.c | 4 ++++ 1 file changed, 4 insertions(+) * submissions if DMA is enabled.