Re: [PATCH 2/2] usb: dwc3: drd: Fix lock-up on ID change during system suspend/resume

[Manu Gautam]

Hi,


On 1/22/2018 6:31 PM, Roger Quadros wrote:
> Adding/removing host/gadget controller before .pm_complete()
> causes a lock-up. Let's prevent any dual-role state change
> between .pm_prepare() and .pm_complete() to fix this.

What kind of lock-up are you seeing? Some hardware lockup or software deadlock?
IMO using a freezable_wq for drd_work should address that?


>
> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
> ---
>  drivers/usb/dwc3/core.c | 31 +++++++++++++++++++++++++++++++
>  drivers/usb/dwc3/core.h |  5 +++++
>  drivers/usb/dwc3/drd.c  | 10 ++++++----
>  3 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 42379cc..85388dd 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1414,6 +1414,33 @@ static int dwc3_runtime_idle(struct device *dev)
>  #endif /* CONFIG_PM */
>  
>  #ifdef CONFIG_PM_SLEEP
> +static int dwc3_prepare(struct device *dev)
> +{
> +	struct dwc3	*dwc = dev_get_drvdata(dev);
> +	unsigned long	flags;
> +
> +	if (dwc->dr_mode == USB_DR_MODE_OTG) {
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		dwc->dr_keep_role = true;
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +	}
> +
> +	return 0;
> +}
> +
> +static void dwc3_complete(struct device *dev)
> +{
> +	struct dwc3	*dwc = dev_get_drvdata(dev);
> +	unsigned long	flags;
> +
> +	if (dwc->dr_mode == USB_DR_MODE_OTG) {
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		dwc->dr_keep_role = false;
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +		dwc3_drd_update(dwc);
> +	}
> +}
> +
>  static int dwc3_suspend(struct device *dev)
>  {
>  	struct dwc3	*dwc = dev_get_drvdata(dev);
> @@ -1451,6 +1478,10 @@ static const struct dev_pm_ops dwc3_dev_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
>  	SET_RUNTIME_PM_OPS(dwc3_runtime_suspend, dwc3_runtime_resume,
>  			dwc3_runtime_idle)
> +#ifdef CONFIG_PM_SLEEP
> +	.prepare = dwc3_prepare,
> +	.complete = dwc3_complete,
> +#endif
>  };
>  
>  #ifdef CONFIG_OF
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 4a4a4c9..f5eb474 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -786,6 +786,7 @@ struct dwc3_scratchpad_array {
>   * @dr_mode: requested mode of operation
>   * @current_dr_role: current role of operation when in dual-role mode
>   * @desired_dr_role: desired role of operation when in dual-role mode
> + * @dr_keep_role: keep the current dual-role irrespective of ID changes
>   * @edev: extcon handle
>   * @edev_nb: extcon notifier
>   * @hsphy_mode: UTMI phy mode, one of following:
> @@ -901,6 +902,7 @@ struct dwc3 {
>  	enum usb_dr_mode	dr_mode;
>  	u32			current_dr_role;
>  	u32			desired_dr_role;
> +	bool			dr_keep_role;
>  	struct extcon_dev	*edev;
>  	struct notifier_block	edev_nb;
>  	enum usb_phy_interface	hsphy_mode;
> @@ -1227,11 +1229,14 @@ static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
>  #if IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
>  int dwc3_drd_init(struct dwc3 *dwc);
>  void dwc3_drd_exit(struct dwc3 *dwc);
> +void dwc3_drd_update(struct dwc3 *dwc);
>  #else
>  static inline int dwc3_drd_init(struct dwc3 *dwc)
>  { return 0; }
>  static inline void dwc3_drd_exit(struct dwc3 *dwc)
>  { }
> +static inline void dwc3_drd_update(struct dwc3 *dwc);
> +{ }
>  #endif
>  
>  /* power management interface */
> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index cc8ab9a..177a8be 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -13,7 +13,7 @@
>  #include "core.h"
>  #include "gadget.h"
>  
> -static void dwc3_drd_update(struct dwc3 *dwc)
> +void dwc3_drd_update(struct dwc3 *dwc)
>  {
>  	int id;
>  
> @@ -31,9 +31,11 @@ static int dwc3_drd_notifier(struct notifier_block *nb,
>  {
>  	struct dwc3 *dwc = container_of(nb, struct dwc3, edev_nb);
>  
> -	dwc3_set_mode(dwc, event ?
> -		      DWC3_GCTL_PRTCAP_HOST :
> -		      DWC3_GCTL_PRTCAP_DEVICE);
> +	if (!dwc->dr_keep_role) {
> +		dwc3_set_mode(dwc, event ?
> +			      DWC3_GCTL_PRTCAP_HOST :
> +			      DWC3_GCTL_PRTCAP_DEVICE);
> +	}
>  
>  	return NOTIFY_DONE;
>  }

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project