On Thu, 9 May 2019, EJ Hsu wrote: > > You forgot to change fsg_unbind() to use FSG_STATE_DISCONNECT. And when > > that's done, it will no longer need to set common->new_fsg to NULL either. > > > > Yes, we should change fsg_unbind() as well. > > > This is sort of a band-aid approach. The real problem is that the original design > > of the driver never intended for there to be more than one outstanding > > CONFIG_CHANGE event, so naturally there are scenarios where it doesn't work > > right when that assumption is violated. This patch addresses one of those > > scenarios, but there may be others remaining. > > > > Ultimately the problem boils down to synchronization with the composite core. > > Thanks for your reviewing, I agree with your point. > > > Some of the callbacks made by the core take time to fully process, so what > > should happen if the core makes a second callback before the first one is > > completely processed? > > > > On the other hand, I can't think of anything better at the moment. > > > > Alan Stern > > Actually, composite core have tried to deal with this case by using a spinlock. > (please refer to the cdev->lock) > The problem here is that the callback of fsg will delegate the request to > fsg_main_thread. That is, the handling of fsg request will run in parallel with > composite core. > In my opinion, this issue can be avoided if we handle these request directly > in the callback of fsg instead of handing it over to fsg_main_thread. But this > might take too much time in the interrupt context, which is not good for > system performance. > > Please correct me if there is anything wrong. Thanks In theory, the mass-storage function could also be fixed to be more careful about locking and synchronization. For example, never set or read common->next_fsg unless you're holding the fsg lock, and don't raise a CONFIG_CHANGE exception if another one is already pending. But I think your patch will be good enough for now, once you have fixed up the two issues mentioned earlier. Alan Stern
