Re: [PATCH 1/2] usb: dwc2: host: Fix missing device insertions

[Doug Anderson]

John,

On Fri, Oct 30, 2015 at 1:38 PM, John Youn <John.Youn@xxxxxxxxxxxx> wrote:
> On 10/16/2015 3:10 PM, Douglas Anderson wrote:
>> 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.
>>
>> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
>> ---
>>  drivers/usb/dwc2/core.h     |  2 ++
>>  drivers/usb/dwc2/hcd.c      | 33 +++++++++++++++++++++++++++++++++
>>  drivers/usb/dwc2/hcd_intr.c |  6 +-----
>>  3 files changed, 36 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
>> index ebf2504..30748be 100644
>> --- a/drivers/usb/dwc2/core.h
>> +++ b/drivers/usb/dwc2/core.h
>> @@ -1151,11 +1151,13 @@ static inline int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg,
>>
>>  #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
>>  extern int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg);
>> +extern void dwc2_hcd_connect(struct dwc2_hsotg *hsotg);
>>  extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg);
>>  extern void dwc2_hcd_start(struct dwc2_hsotg *hsotg);
>>  #else
>>  static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg)
>>  { return 0; }
>> +static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {}
>>  static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) {}
>>  static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
>>  static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index af4e4a2..4d2bc50 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -268,6 +268,22 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg)
>>  }
>>
>>  /**
>> + * dwc2_hcd_connect() - Handles connect of the HCD
>> + *
>> + * @hsotg: Pointer to struct dwc2_hsotg
>> + *
>> + * Must be called with interrupt disabled and spinlock held
>> + */
>> +void dwc2_hcd_connect(struct dwc2_hsotg *hsotg)
>> +{
>> +     if (hsotg->lx_state != DWC2_L0)
>> +             usb_hcd_resume_root_hub(hsotg->priv);
>> +
>> +     hsotg->flags.b.port_connect_status_change = 1;
>> +     hsotg->flags.b.port_connect_status = 1;
>> +}
>> +
>> +/**
>>   * dwc2_hcd_disconnect() - Handles disconnect of the HCD
>>   *
>>   * @hsotg: Pointer to struct dwc2_hsotg
>> @@ -277,6 +293,7 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg)
>>  void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
>>  {
>>       u32 intr;
>> +     u32 hprt0;
>>
>>       /* Set status flags for the hub driver */
>>       hsotg->flags.b.port_connect_status_change = 1;
>> @@ -315,6 +332,22 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
>>               dwc2_hcd_cleanup_channels(hsotg);
>>
>>       dwc2_host_disconnect(hsotg);
>> +
>> +     /*
>> +      * Add an extra check here to see if we're actually connected but
>> +      * we don't have a detection interrupt pending.  This can happen if:
>> +      *   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 here
>> +      *
>> +      * Without the extra check here we will end calling disconnect
>> +      * and won't get any future interrupts to handle the connect.
>> +      */
>> +     hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>> +     if (!(hprt0 & HPRT0_CONNDET) && (hprt0 & HPRT0_CONNSTS))
>> +             dwc2_hcd_connect(hsotg);
>
> I'm a little bit wary of this as it will also run when you unload
> the module with a device connected. However I can't produce any
> negative results from that. Will it work if you handle it just
> for the disconnect interrupt?

Good call.  I think it might actually be cleaner to just add a "force"
parameter to dwc2_hcd_disconnect().  That way the 3 callers other than
_dwc2_hcd_stop() all get the behavior to check for reconnect.  I hope
that's OK.  New patch forthcoming.

-Doug
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/