Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753204AbbKPTKb (ORCPT ); Mon, 16 Nov 2015 14:10:31 -0500 Received: from devils.ext.ti.com ([198.47.26.153]:36395 "EHLO devils.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751978AbbKPTKO (ORCPT ); Mon, 16 Nov 2015 14:10:14 -0500 From: Felipe Balbi To: Doug Anderson CC: John Youn , Yunzhi Li , Heiko =?utf-8?Q?St=C3=BCbner?= , "open list:ARM/Rockchip SoC..." , Julius Werner , "Herrero, Gregory" , "Kaukab, Yousaf" , Dinh Nguyen , John Youn , "Greg Kroah-Hartman" , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions In-Reply-To: References: <1446582629-30091-1-git-send-email-dianders@chromium.org> <87y4dxltzi.fsf@saruman.tx.rr.com> <87vb91ls89.fsf@saruman.tx.rr.com> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Mon, 16 Nov 2015 13:09:30 -0600 Message-ID: <87poz9lq1h.fsf@saruman.tx.rr.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6110 Lines: 159 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable hi, Doug Anderson writes: > Felipe, > > On Mon, Nov 16, 2015 at 10:22 AM, Felipe Balbi 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 unlo= ad. > > * 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 refacto= r, >>>> 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 =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWSinrAAoJEIaOsuA1yqRE99EP/0ZlItHpOxI+Zm06E/MSxH+/ vL3cZISuTfj/OBo9sWnEj1h23SE7i6nOzQraAjNAQKrNPKgzftMlg52mLu4wVeil OzkPO05yCFXg0uqEd4I6pa9QJWLdZk5EEFG8YsG7hxA8vHUYwB+Ko6kGFgCWPOcx /i8qRHMAtXEVSESmCcMHcrKLZy8EZ5ozCYUStyYupGa/7wtJiOF23jR5IhzbUj54 3/uOv0blaQFHB1r2n7oMAmCZx+Z7+m9gNWt89KvkolIjvTgxyt9q4e1JcNGWUrde es9/recvh6ZV0m/h7d5Dd4wRiGlec0AMmmFvPeGawm2HvA9Bu9LyuLxa45anTDdJ QVBE4g+geAF+8XxxZCBRFG9VXolJZX7ExPjhib+ziHRv8hHeLhvIB6ca806oMhKG ANdNWnEJz6zuLBwfwqk+IKJli7bMNN1Atbgx9tP8FdUzOqBHQhiTpQvkJveAf29u HKLzEyELR+Dps28zvFSAoYiV0pKGEY1q3N/6DJBs1DdtRRHmgc4MgRMHf6BnCoaU 2JbJyqceoo9iNfCwDG5uDGS7okXALCtVeHMZH4Qd/88xshC3w/d6yTu/UmKYY/Jm JhF79M5i5s+INRuL6SYapCmjqZz0aS4Et6AKCYEAPIURmifaLpdtVtQBTDnKs0h6 PgWSeCZeFrmGvkR+EwjD =hz4d -----END PGP SIGNATURE----- --=-=-=-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/