Re: [PATCH v2 2/3] usb: dwc3: xilinx: re-assert resets on ZynqMP init error paths

[Philipp Zabel <p.zabel@pengutronix.de>]

On Di, 2026-09-22 at 23:51 +0530, Radhey Shyam Pandey wrote:
> If reset deassert or PHY setup fails partway through
> dwc3_xlnx_init_zynqmp(), re-assert any resets that were already
> released before unwinding the PHY. Use fall-through error labels so
> unwind matches how far init progressed, for both USB2 and USB3 paths.
> 
> Save the ZynqMP reset handles in driver private data so later probe
> teardown can re-assert released resets.
> 
> Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> Changes in v2:
> - Split out of the combined five patch series; see patch 1.
> - Reordered ahead of the platform-data cleanups.
> - Added Cc: stable.
> - No functional change to the patch itself.
> 
>  drivers/usb/dwc3/dwc3-xilinx.c | 49 +++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> index 8c63e02575f1..d18d3e364381 100644
> --- a/drivers/usb/dwc3/dwc3-xilinx.c
> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> @@ -48,6 +48,10 @@ struct dwc3_xlnx {
>  	void __iomem			*regs;
>  	int				(*pltfm_init)(struct dwc3_xlnx *data);
>  	struct phy			*usb3_phy;
> +	struct reset_control		*usb_crst;
> +	struct reset_control		*usb_hibrst;
> +	struct reset_control		*usb_apbrst;
> +	bool				usb_resets_released;
>  };
>  
>  static void dwc3_xlnx_mask_phy_rst(struct dwc3_xlnx *priv_data, bool mask)
> @@ -112,7 +116,6 @@ static int dwc3_xlnx_init_versal(struct dwc3_xlnx *priv_data)
>  static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  {
>  	struct device		*dev = priv_data->dev;
> -	struct reset_control	*crst, *hibrst, *apbrst;
>  	struct gpio_desc	*reset_gpio;
>  	int			ret = 0;
>  
> @@ -124,25 +127,25 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  		goto err;
>  	}
>  
> -	crst = devm_reset_control_get_exclusive(dev, "usb_crst");
> -	if (IS_ERR(crst)) {
> -		ret = PTR_ERR(crst);
> +	priv_data->usb_crst = devm_reset_control_get_exclusive(dev, "usb_crst");
> +	if (IS_ERR(priv_data->usb_crst)) {
> +		ret = PTR_ERR(priv_data->usb_crst);
>  		dev_err_probe(dev, ret,
>  			      "failed to get core reset signal\n");
>  		goto err;
>  	}
>  
> -	hibrst = devm_reset_control_get_exclusive(dev, "usb_hibrst");
> -	if (IS_ERR(hibrst)) {
> -		ret = PTR_ERR(hibrst);
> +	priv_data->usb_hibrst = devm_reset_control_get_exclusive(dev, "usb_hibrst");
> +	if (IS_ERR(priv_data->usb_hibrst)) {
> +		ret = PTR_ERR(priv_data->usb_hibrst);
>  		dev_err_probe(dev, ret,
>  			      "failed to get hibernation reset signal\n");
>  		goto err;
>  	}
>  
> -	apbrst = devm_reset_control_get_exclusive(dev, "usb_apbrst");
> -	if (IS_ERR(apbrst)) {
> -		ret = PTR_ERR(apbrst);
> +	priv_data->usb_apbrst = devm_reset_control_get_exclusive(dev, "usb_apbrst");
> +	if (IS_ERR(priv_data->usb_apbrst)) {
> +		ret = PTR_ERR(priv_data->usb_apbrst);
>  		dev_err_probe(dev, ret,
>  			      "failed to get APB reset signal\n");
>  		goto err;
> @@ -156,19 +159,19 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  	 * absent.
>  	 */
>  	if (priv_data->usb3_phy) {
> -		ret = reset_control_assert(crst);
> +		ret = reset_control_assert(priv_data->usb_crst);
>  		if (ret < 0) {
>  			dev_err(dev, "Failed to assert core reset\n");
>  			goto err;
>  		}
>  
> -		ret = reset_control_assert(hibrst);
> +		ret = reset_control_assert(priv_data->usb_hibrst);
>  		if (ret < 0) {
>  			dev_err(dev, "Failed to assert hibernation reset\n");
>  			goto err;
>  		}
>  
> -		ret = reset_control_assert(apbrst);
> +		ret = reset_control_assert(priv_data->usb_apbrst);
>  		if (ret < 0) {
>  			dev_err(dev, "Failed to assert APB reset\n");
>  			goto err;
> @@ -179,7 +182,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  	if (ret < 0)
>  		goto err;
>  
> -	ret = reset_control_deassert(apbrst);
> +	ret = reset_control_deassert(priv_data->usb_apbrst);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to release APB reset\n");
>  		goto err_phy_exit;
> @@ -195,21 +198,21 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  		writel(PIPE_CLK_DESELECT, priv_data->regs + XLNX_USB_FPD_PIPE_CLK);
>  	}
>  
> -	ret = reset_control_deassert(crst);
> +	ret = reset_control_deassert(priv_data->usb_crst);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to release core reset\n");
> -		goto err_phy_exit;
> +		goto err_apbrst_assert;
>  	}
>  
> -	ret = reset_control_deassert(hibrst);
> +	ret = reset_control_deassert(priv_data->usb_hibrst);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to release hibernation reset\n");
> -		goto err_phy_exit;
> +		goto err_crst_assert;
>  	}
>  
>  	ret = phy_power_on(priv_data->usb3_phy);
>  	if (ret < 0)
> -		goto err_phy_exit;
> +		goto err_hibrst_assert;
>  
>  	/* ulpi reset via gpio-modepin or gpio-framework driver */
>  	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);

Are the resets asserted if this fails?

Same question about if probe fails after pltfm_init() succeeded.

> @@ -226,10 +229,18 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  
>  	dwc3_xlnx_set_coherency(priv_data, XLNX_USB_TRAFFIC_ROUTE_CONFIG);
>  
> +	priv_data->usb_resets_released = true;
> +
>  	return 0;
>  
>  err_phy_power_off:
>  	phy_power_off(priv_data->usb3_phy);
> +err_hibrst_assert:
> +	reset_control_assert(priv_data->usb_hibrst);
> +err_crst_assert:
> +	reset_control_assert(priv_data->usb_crst);
> +err_apbrst_assert:
> +	reset_control_assert(priv_data->usb_apbrst);
>  err_phy_exit:
>  	phy_exit(priv_data->usb3_phy);
>  err:

Rather than storing usb_resets_released state, and then having
conditional teardown in the next patch, this could be handled via
devm_add_action_or_reset.

regards
Philipp