Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753437AbbKPSoc (ORCPT ); Mon, 16 Nov 2015 13:44:32 -0500 Received: from mail-yk0-f172.google.com ([209.85.160.172]:33631 "EHLO mail-yk0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751807AbbKPSo3 (ORCPT ); Mon, 16 Nov 2015 13:44:29 -0500 MIME-Version: 1.0 In-Reply-To: <87vb91ls89.fsf@saruman.tx.rr.com> References: <1446582629-30091-1-git-send-email-dianders@chromium.org> <87y4dxltzi.fsf@saruman.tx.rr.com> <87vb91ls89.fsf@saruman.tx.rr.com> Date: Mon, 16 Nov 2015 10:44:28 -0800 X-Google-Sender-Auth: ZQZG7EBM8HOtAM78IJGiEpkf-Q4 Message-ID: Subject: Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions From: Doug Anderson To: Felipe Balbi Cc: John Youn , Yunzhi Li , =?UTF-8?Q?Heiko_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" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4849 Lines: 125 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 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(). -Doug -- 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/