On 11/16/2015 3:14 PM, Doug Anderson wrote: > Alan, > > On Mon, Nov 16, 2015 at 12:31 PM, Alan Stern <stern at rowland.harvard.edu> wrote: >> On Mon, 16 Nov 2015, Doug Anderson wrote: >> >>> Alan, >>> >>> On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <stern at rowland.harvard.edu> wrote: >>>> On Mon, 16 Nov 2015, Doug Anderson wrote: >>>> >>>>> --- >>>>> >>>>> usb: dwc2: host: Fix missing device insertions >>>>> >>>>> If you've got your interrupt signals bouncing a bit as you insert your >>>>> USB device, you might end up in a state when the device is connected but >>>>> the driver doesn't know it. >>>>> >>>>> Specifically, the observed order is: >>>>> 1. hardware sees connect >>>>> 2. hardware sees disconnect >>>>> 3. hardware sees connect >>>>> 4. dwc2_port_intr() - clears connect interrupt >>>>> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() >>>>> >>>>> Now you'll be stuck with the cable plugged in and no further interrupts >>>>> coming in but the driver will think we're disconnected. >>>>> >>>>> We'll fix this by checking for the missing connect interrupt and >>>>> re-connecting after the disconnect is posted. We don't skip the >>>>> disconnect because if there is a transitory disconnect we really want to >>>>> de-enumerate and re-enumerate. >>>> >>>> Why do you need to do anything special here? Normally a driver's >>>> interrupt handler should query the hardware status after clearing the >>>> interrupt source. That way no transitions ever get missed. >>>> >>>> In your example, at step 5 the dwc2 driver would check the port status >>>> and see that it currently is connected. Therefore the driver would >>>> pass a "connect status changed" event to the USB core and set the port >>>> status to "connected". No extra checking is needed, and transitory >>>> connects or disconnects get handled correctly. >>> >>> Things are pretty ugly at the moment because the dwc2 interrupt >>> handler is split in two. There's dwc2_handle_hcd_intr() and >>> dwc2_handle_common_intr(). Both are registered for the same "shared" >>> IRQ. If I had to guess, I'd say that this is probably because someone >>> wanted to assign the ".irq" field in the "struct hc_driver" for the >>> host controller but that they also needed the other interrupt handler >>> to handle things shared between host and gadget mode. >>> >>> In any case, one of these two interrupt routines handles connect and >>> the other disconnect. That's pretty ugly but means that you can't >>> just rely on standard techniques for keeping things in sync. >> >> Okay, that is rather weird. Still, I don't see why it should matter. >> >> Fundamentally there's no difference between a "connect" interrupt and a >> "disconnect" interrupt. They should both do exactly the same things: >> clear the interrupt source, post a "connection change" event, and set >> the driver's connect status based on the hardware's current state. >> The second and third parts can be handled by a shared subroutine. > > Ah, sorry I misunderstood. OK, fair enough. So you're saying that > the problem is that dwc2_hcd_disconnect() has a line that looks like > this: > > hsotg->flags.b.port_connect_status = 0; > > ...and the dwc2_port_intr() has a line that looks like this: > > hsotg->flags.b.port_connect_status = 1; > > ...and both should just query the status. > > > Do you think we should to block landing this patch on cleaning up how > dwc2 handles port_connect_status? I'm not sure what side effects > changing port_connect_status will have, so I'll need to test and dig a > bit... > > I'm currently working on trying to fix the microframe scheduler and > was planning to post the next series of patches there pretty soon. > I'm also planning to keep digging to figure out how to overall > increase compatibility with devices (and compatibility with many > devices plugged in). > > If it were up to me, I'd prefer to land this patch in either 4.4 or > 4.5 since it does seem to work. ...then put seeing what we can do to > cleanup all of the port_connect_status on the todo list. That sound good to me. Though I'd prefer to see this series queued for 4.5 for more testing. > > Julius points out in his response that there are comments saying that > HPRT0 can't be read in device mode. John: since you're setup to test > device mode, can you check if my patch (which now adds a read of > HPRT0) will cause problems? Maybe holding this off and keeping it out > of the RC is a good idea after all... > Yup it's only available in host mode. The same with all the host-mode registers. You will get a ModeMis interrupt if you try to access them in device mode. I gave my test-by on the v2 patches earlier, no problems here. John
