hi, 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). ...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:
signature.asc
Description: PGP signature
