Re: [PATCH] usb: gadget: f_mass_storage: Shut down mass storage device when USB connection is shut, down.

[Alan Stern <stern@rowland.harvard.edu>]

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