Re: [PATCH v2] usb: dwc3: st: balance reset ownership on deassert failure

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Tue, Sep 22, 2026, Pengpeng Hou wrote:
> The ST DWC3 glue ignores reset deassert failures in probe and resume.
> Probe may publish the child device, and resume may access glue registers,
> without both reset operations having succeeded.
> 
> Check both results. Balance a failed shared softreset deassert through
> undo_softreset, since the reset core increments its shared count before
> calling the provider and does not drop it on a provider error.
> 
> Record whether this driver holds the completed deassert pair. A failed
> resume unwinds the pair and leaves that state clear. A later suspend or
> remove must not assert the shared reset a second time, and a subsequent
> resume can acquire the pair again. Unwind the resets on DRD setup errors
> as well. Exclusive powerdown and shared softreset remain distinct reset
> controls.
> 
> The issue was found by our static-analysis tool.
> 
> Fixes: f83fca0707c6 ("usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC")
> Assisted-by: gpt 5
> Signed-off-by: Pengpeng Hou <hppiscas@163.com>
> ---
> Changes since v1:
> - use undo_softreset for a failed shared deassert, as Thinh requested
> - guard later suspend/remove against a pair already unwound during resume
> - unwind DRD setup failure and preserve the original failure code
> - omit the earlier Reviewed-by because the lifecycle handling changed
> 
> Previous version:
> https://urldefense.com/v3/__https://lore.kernel.org/all/20260624055728.46078-1-pengpeng@iscas.ac.cn/__;!!A4F2R9G_pg!fLyuQUcLEdsSfSAVcTYR6KpFs-RnCNN6RGlDt1Z-G_XIHGMXAEEffsm-JlJ07OQhxy2W-W0JyHoHVeqCj0gknA$ 
> 
>  drivers/usb/dwc3/dwc3-st.c | 46 +++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c
> index 5d513decaacd..f954234d2b83 100644
> --- a/drivers/usb/dwc3/dwc3-st.c
> +++ b/drivers/usb/dwc3/dwc3-st.c
> @@ -84,6 +84,7 @@
>   * @dr_mode:		drd static host/device config
>   * @rstc_pwrdn:		rest controller for powerdown signal
>   * @rstc_rst:		reset controller for softreset signal
> + * @resets_active:	whether this driver holds its deasserted reset pair
>   */
>  
>  struct st_dwc3 {
> @@ -94,6 +95,7 @@ struct st_dwc3 {
>  	enum usb_dr_mode dr_mode;
>  	struct reset_control *rstc_pwrdn;
>  	struct reset_control *rstc_rst;
> +	bool resets_active;
>  };
>  
>  static inline u32 st_dwc3_readl(void __iomem *base, u32 offset)
> @@ -242,7 +244,9 @@ static int st_dwc3_probe(struct platform_device *pdev)
>  				     "could not get power controller\n");
>  
>  	/* Manage PowerDown */
> -	reset_control_deassert(dwc3_data->rstc_pwrdn);
> +	ret = reset_control_deassert(dwc3_data->rstc_pwrdn);
> +	if (ret)
> +		return ret;
>  
>  	dwc3_data->rstc_rst =
>  		devm_reset_control_get_shared(dev, "softreset");
> @@ -253,7 +257,10 @@ static int st_dwc3_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Manage SoftReset */
> -	reset_control_deassert(dwc3_data->rstc_rst);
> +	ret = reset_control_deassert(dwc3_data->rstc_rst);
> +	if (ret)
> +		goto undo_softreset;
> +	dwc3_data->resets_active = true;
>  
>  	/* Allocate and initialize the core */
>  	ret = of_platform_populate(node, NULL, NULL, dev);
> @@ -305,16 +312,22 @@ static void st_dwc3_remove(struct platform_device *pdev)
>  
>  	of_platform_depopulate(&pdev->dev);
>  
> -	reset_control_assert(dwc3_data->rstc_pwrdn);
> -	reset_control_assert(dwc3_data->rstc_rst);
> +	if (dwc3_data->resets_active) {
> +		reset_control_assert(dwc3_data->rstc_pwrdn);
> +		reset_control_assert(dwc3_data->rstc_rst);
> +		dwc3_data->resets_active = false;
> +	}
>  }
>  
>  static int st_dwc3_suspend(struct device *dev)
>  {
>  	struct st_dwc3 *dwc3_data = dev_get_drvdata(dev);
>  
> -	reset_control_assert(dwc3_data->rstc_pwrdn);
> -	reset_control_assert(dwc3_data->rstc_rst);
> +	if (dwc3_data->resets_active) {
> +		reset_control_assert(dwc3_data->rstc_pwrdn);
> +		reset_control_assert(dwc3_data->rstc_rst);
> +		dwc3_data->resets_active = false;
> +	}
>  
>  	pinctrl_pm_select_sleep_state(dev);
>  
> @@ -326,21 +339,36 @@ static int st_dwc3_resume(struct device *dev)
>  	struct st_dwc3 *dwc3_data = dev_get_drvdata(dev);
>  	int ret;
>  
> +	if (dwc3_data->resets_active)

Why? Can resets_active be true here?

Thanks,
Thinh

> +		return 0;
> +
>  	pinctrl_pm_select_default_state(dev);
>  
> -	reset_control_deassert(dwc3_data->rstc_pwrdn);
> -	reset_control_deassert(dwc3_data->rstc_rst);
> +	ret = reset_control_deassert(dwc3_data->rstc_pwrdn);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_deassert(dwc3_data->rstc_rst);
> +	if (ret)
> +		goto undo_softreset;
>  
>  	ret = st_dwc3_drd_init(dwc3_data);
>  	if (ret) {
>  		dev_err(dev, "drd initialisation failed\n");
> -		return ret;
> +		goto undo_softreset;
>  	}
>  
>  	/* ST glue logic init */
>  	st_dwc3_init(dwc3_data);
> +	dwc3_data->resets_active = true;
>  
>  	return 0;
> +
> +undo_softreset:
> +	/* A shared deassert holds its reference even if the provider fails. */
> +	reset_control_assert(dwc3_data->rstc_rst);
> +	reset_control_assert(dwc3_data->rstc_pwrdn);
> +	return ret;
>  }
>  
>  static DEFINE_SIMPLE_DEV_PM_OPS(st_dwc3_dev_pm_ops, st_dwc3_suspend, st_dwc3_resume);
> 
> base-commit: f0100363d8c374bd8e9ea7c9ba02744f0b802ca4
> -- 
> 2.50.1 (Apple Git-155)
>