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

From: Felipe Balbi
Date: Mon Nov 16 2015 - 14:10:34 EST


Doug Anderson <dianders@xxxxxxxxxxxx> writes:
> Felipe,
> On Mon, Nov 16, 2015 at 10:22 AM, Felipe Balbi <balbi@xxxxxx> 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). 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


Attachment: signature.asc
Description: PGP signature