[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87poz9lq1h.fsf@saruman.tx.rr.com>
Date: Mon, 16 Nov 2015 13:09:30 -0600
From: Felipe Balbi <balbi@...com>
To: Doug Anderson <dianders@...omium.org>
CC: John Youn <John.Youn@...opsys.com>, Yunzhi Li <lyz@...k-chips.com>,
Heiko Stübner <heiko@...ech.de>,
"open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
Julius Werner <jwerner@...omium.org>,
"Herrero, Gregory" <gregory.herrero@...el.com>,
"Kaukab, Yousaf" <yousaf.kaukab@...el.com>,
Dinh Nguyen <dinguyen@...nsource.altera.com>,
John Youn <johnyoun@...opsys.com>,
"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions
hi,
Doug Anderson <dianders@...omium.org> writes:
> Felipe,
>
> On Mon, Nov 16, 2015 at 10:22 AM, Felipe Balbi <balbi@...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
Download attachment "signature.asc" of type "application/pgp-signature" (819 bytes)
Powered by blists - more mailing lists