From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751702AbbKPUb7 (ORCPT ); Mon, 16 Nov 2015 15:31:59 -0500 Received: from iolanthe.rowland.org ([192.131.102.54]:39550 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751041AbbKPUb4 (ORCPT ); Mon, 16 Nov 2015 15:31:56 -0500 Date: Mon, 16 Nov 2015 15:31:55 -0500 (EST) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Doug Anderson cc: Felipe Balbi , 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" Subject: Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions In-Reply-To: Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 16 Nov 2015, Doug Anderson wrote: > Alan, > > On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern wrote: > > On Mon, 16 Nov 2015, Doug Anderson wrote: > > > >> --- > >> > >> 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. > > > > Why do you need to do anything special here? Normally a driver's > > interrupt handler should query the hardware status after clearing the > > interrupt source. That way no transitions ever get missed. > > > > In your example, at step 5 the dwc2 driver would check the port status > > and see that it currently is connected. Therefore the driver would > > pass a "connect status changed" event to the USB core and set the port > > status to "connected". No extra checking is needed, and transitory > > connects or disconnects get handled correctly. > > Things are pretty ugly at the moment because the dwc2 interrupt > handler is split in two. There's dwc2_handle_hcd_intr() and > dwc2_handle_common_intr(). Both are registered for the same "shared" > IRQ. If I had to guess, I'd say that this is probably because someone > wanted to assign the ".irq" field in the "struct hc_driver" for the > host controller but that they also needed the other interrupt handler > to handle things shared between host and gadget mode. > > In any case, one of these two interrupt routines handles connect and > the other disconnect. That's pretty ugly but means that you can't > just rely on standard techniques for keeping things in sync. Okay, that is rather weird. Still, I don't see why it should matter. Fundamentally there's no difference between a "connect" interrupt and a "disconnect" interrupt. They should both do exactly the same things: clear the interrupt source, post a "connection change" event, and set the driver's connect status based on the hardware's current state. The second and third parts can be handled by a shared subroutine. If you think of these things as "connect changed" interrupts rather than as "connect" and "disconnect" interrupts, it makes a lot of sense. > It would probably be a pretty reasonable idea to try to clean that up > more, but that would be a very major and intrusive change. Maybe so -- I'll take your word for it since I'm not at all familiar with the dwc2 code. Alan Stern