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

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 5004 bytes --]


hi,

Doug Anderson <dianders@chromium.org> writes:
> Felipe,
>
> On Mon, Nov 16, 2015 at 10:22 AM, Felipe Balbi <balbi@ti.com> wrote:
>>> I added "force" in v2 of the patch in response to John's feedback to
>>> v1.  He pointed out that when you unload the module when you have a
>>> device connected that my v1 patch would not properly disconnect the
>>> device (or, rather, it would disconnect it and then reconnect it).
>>>
>>> That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force
>>> of "true".
>>
>> There's no mention of this in commit log. It would be great to add a:
>>
>> "while at that, also make sure that we don't try to detect a device on
>> module unload because of foo bar baz as pointed out by John Youn".
>>
>> Or something along these lines.
>
> ...well, except that it's not new behavior.  In other words:
>
> * Without my patch at all: we don't try to detect a device on module unload.
>
> * With my v1 patch: we (incorrectly) did try to detect a device on
> module unload.
>
> * With my v2 patch: we're back to not trying to detect a device on
> module unload.
>
> In other words: my v2 patch (correctly) doesn't change the behavior on
> module unload.  That's why I didn't mention it in the commit message.
> It's in the "v2" changelog though.
>
>
> I'll try to come up with something for the commit message though.  See
> below for new proposed commit message.
>
>
>>>> you make no mention of why this is needed. This is basically a refactor,
>>>> not a fix.
>>>
>>> This new function is called from two places now, isn't it?
>>>
>>> Basically this is a little bit of code that used to be directly in
>>> dwc2_port_intr().  I still want it there, but I also want to call the
>>> same bit of code after a disconnect if I detect that the device has
>>> been inserted again.
>>
>> I got that :-) But it's not mentioned in commit and it's apparently
>> unnecessary for fixing the bug :-) Another "we're also adding a new
>> hsotg_disconnect() function by means of refactoring to avoid code
>> duplication" would've been enough.
>
> OK, sure.
>
>
>>> I'd really rather not add the duplication unless you insist.  To me it
>>> makes it clearer to include the (small) refactor in the same patch.
>>>
>>> If the refactor makes this change too big for an RC, then it's OK with
>>> me to just skip this for the RC.  It's not fixing a regression or
>>> anything.  I have no requirements to have this land in 4.4.  It fixes
>>> a bug and I thought that the fix was pretty small and safe (despite
>>> having a diffstat that's bigger than the bare minimum).  I will leave
>>> it to your judgement.
>>
>> let's at least modify commit log to make all these extra changes clear
>> that they are needed because of reason (a) or (b) or whatever. If you
>> just send a patch doing much more than it apparently should without no
>> mention as to why it was done this way, I can't know for sure those
>> changes are needed; next thing you know, Greg refuses to apply my pull
>> request because the change is too large :-)
>>
>> We don't want that to happen :-)
>
> Totally understand.  It's your butt on the line for the pull request
> to Greg, so definitely want to make sure you're comfortable with
> anything you pass on.  As always I definitely appreciate your reviews
> and your time.
>
>
> How about if we just add a "Notes" to the end of the patch
> description.  I can repost a patch or you can feel free to change the
> description as per below (just let me know).  ...so in total:
>
> ---
>
> 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.
>
> Notes:
>
> 1. As part of this change we add a "force" parameter to
>    dwc2_hcd_disconnect() so that when we're unloading the module we
>    avoid the new behavior.  The need for this was pointed out by John
>    Youn.
> 2. The bit of code needed at the end of dwc2_hcd_disconnect() is
>    exactly the same bit of code from dwc2_port_intr().  To avoid
>    duplication, we refactor that code out into a new function
>    dwc2_hcd_connect().

this should be enough, thanks for being so responsive

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]