From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752163AbbKPTqr (ORCPT ); Mon, 16 Nov 2015 14:46:47 -0500 Received: from mail-yk0-f170.google.com ([209.85.160.170]:33195 "EHLO mail-yk0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751884AbbKPTqp (ORCPT ); Mon, 16 Nov 2015 14:46:45 -0500 MIME-Version: 1.0 In-Reply-To: References: Date: Mon, 16 Nov 2015 11:46:44 -0800 X-Google-Sender-Auth: 432linMtmSkSZSQiUEKMNf_3xNU Message-ID: Subject: Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions From: Doug Anderson To: Alan Stern 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" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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. -Doug From mboxrd@z Thu Jan 1 00:00:00 1970 From: Doug Anderson Subject: Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions Date: Mon, 16 Nov 2015 11:46:44 -0800 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: Sender: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Alan Stern 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: linux-rockchip.vger.kernel.org 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. 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. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html