Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions

[John Youn]

On 11/16/2015 3:14 PM, Doug Anderson wrote:
> Alan,
> 
> On Mon, Nov 16, 2015 at 12:31 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
>> On Mon, 16 Nov 2015, Doug Anderson wrote:
>>
>>> Alan,
>>>
>>> On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/