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

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

[Florian Faber]

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(+)

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
  		 * submissions if DMA is enabled.
-- 
2.33.0

Flo
-- 
Machines can do the work, so people have time to think.

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

[Alan Stern]

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

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

[Florian Faber]

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
-- 
Machines can do the work, so people have time to think.

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

[Alan Stern]

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