Re: [PATCH] usb: f_mass_storage: test whether thread is running before starting another

[Alan Stern]

On Tue, 5 Apr 2016, Michal Nazarewicz wrote:

> > On Tue, 5 Apr 2016, Michal Nazarewicz wrote:
> >> When binding the function to usb_configuration, check whether the thread
> >> is running before starting another one.  Without that, when function
> >> instance is added to multiple configurations, fsg_bing starts multiple
> >> threads with all but the latest one being forgotten by the driver.  This
> >> leads to obvious thread leaks, possible lockups when trying to halt the
> >> machine and possible more issues.
> >> 
> >> This fixes issues with legacy/multi gadget as well as configfs gadgets
> >> when mass_storage function is added to multiple configurations.
> >> 
> >> This change also simplifies API since the legacy gadgets no longer need
> >> to worry about starting the thread by themselves (which was where bug
> >> in legacy/multi was in the first place).
> >> 
> >> Â I have no example failure though.  Conclusion that legacy/multi has
> >>   a bug is based purely on me reading the code.
> >> 
> >> Signed-off-by: Michal Nazarewicz <mina86@xxxxxxxxxx>
> 
> On Tue, Apr 05 2016, Alan Stern wrote:
> > This doesn't address the problem I raised in a previous email.  
> > Sharing one thread among several function instances in the same config
> > will not work if one of them encounters an error.
> 
> Each usb_function_instance has its own fsg_common and its own thread.
> This was true in the past and is true with this patch as well.

We're getting confused by the stupid terminology again.  Yes, each
usb_function_instance has its own fsg_common and its own thread.  But
multiple usb_function structures (each one being a separate function
instance) can belong to the same usb_function_instance and they will
all share the same fsg_common.

That's what happened with the nokia driver.  It creates one 
usb_function_instance with two usb_function structures.  In this case 
they are in different configs, so sharing a thread doesn't matter.  But 
it would matter if they were in the same config.

> And unless Iâm missing something, sharing a thread among multiple
> usb_functionâs does not prevent the driver from working correctly.

Suppose one usb_function is carrying out an I/O operation while another
one in the same config gets a Set-Interface request from the host.  
The request causes the driver to raise an FSG_STATE_CONFIG_CHANGE
exception.  When the thread sees this exception, it will abort the I/O
that it is carrying out for the first usb_function.

In other words, exceptions raised by one instance will affect the 
shared thread even when it's doing work for a different instance.

> Having the thread run even when itâs not used may be considered wasteful
> but thatâs an orthogonal issue to the configfs failure.

Yes.  Having extra unused threads isn't terrible.

Alan Stern