On Thu, 2019-07-04 at 21:34 -0400, Alan Stern wrote:
> > It is the right HW behaviour. But with our gadget stack, it doesn't
> > reset or cleanup anything. Though since the port gets disabled, I
> > suppose re-enabling it will cause a reset and will sort that out.
>
> That's right. (Except that you got the cause and effect reversed;
> resetting the port is what causes it to be re-enabled. :-)
Right.
> > > > Now, a few things i noticed while at it:
> > > >
> > > > - At some point I had code to reject EP queue() if the device is
> > > > suspended with -ESHUTDOWN. The end result was bad ... f_mass_storage
> > > > goes into an infinite loop of trying to queue the same stuff in
> > > > start_out_transfer() when that happens. It looks like it's not really
> > > > handling errors from queue() in a particularily useful way.
> > >
> > > Don't reject EP queue requests. Accept them as you would at any time;
> > > they will complete after the port is resumed.
> >
> > Except the suspend on a bus reset clears the port enable. You can't
> > resume from that, only reset the port no ? Or am I missing something ?
>
> You're right. Nevertheless, the driver should accept queue requests,
> even if they will eventually fail. It's what the gadget drivers
> expect.
Ok. Things seem to work. I went back to suspend on bus reset, and with
some other fixes I did to my enable/reset logic and the mass storage
fix I posted yesterday it seems to be solid. Yay !
> > > As for f_mass_storage, repeatedly attempting to queue an OUT transfer
> > > is normal behavior. The fact that one attempt gets an error doesn't
> > > stop the driver from making more attempts; the only thing that would
> > > stop it is being disabled by a config change, a suspend, a disconnect,
> > > or an unbind.
> >
> > Except it does that in a tight loop and locks up the machine...
>
> Well, that wouldn't happen if your UDC accepted the requests, right?
Sure but it would be nice if the mass storage dealt with -ESHUTDOWN
properly and stopped :-) Or other errors... if the UDC HW for example
dies for some reason, mass storage will lockup.
> Besides, f_mass_storage won't repeatedly try to queue an OUT transfer
> once it knows that it is suspended.
Not afaik. But I might have missed something. I didn't see any suspend
callback in f_mass_storage.c...
Cheers,
Ben.
> Alan Stern
>
> > > > - With my current code doing suspend/resume on bus resets, when I
> > > > reboot some hosts, and they re-enumerate, I tend to hit the WARN_ON
> > > > drivers/usb/gadget/function/f_mass_storage.c:341
> > > >
> > > > static inline int __fsg_is_set(struct fsg_common *common,
> > > > const char *func, unsigned line)
> > > > {
> > > > if (common->fsg)
> > > > return 1;
> > > > ERROR(common, "common->fsg is NULL in %s at %u\n", func, line);
> > > > WARN_ON(1);
> > > > return 0;
> > > > }
> > > >
> > > > This happens a little while after a successul set_configuration. Here's
> > > > a trace:
> > >
> > > ...
> > >
> > > > I have to get my head around that code, but if one of you have a clue, I
> > > > would welcome it :-)
> > > >
> > > > Interestingly it recovers. The host seems to then reset the prot, then
> > > > reconfigure and
> > > > the second time around it all works fine.
> > >
> > > I suspect this is related to the race you found. EJ Hsu has been
> > > working on much the same thing (see the mailing list archive).
> >
> > Right. I debugged the race and produced the fix I posted *after* I had
> > change my code to do a reset rather than a suspend on the hub receiving
> > an upstream bus reset.
> >
> > I will switch back to doing suspend instead and see whether that stays
> > fixed.
> >
> > Cheers,
> > Ben.
