[PATCH v3] USB: pxa27x_udc: check return value of clk_enable

[PATCH v3] USB: pxa27x_udc: check return value of clk_enable

[Zhaoyang Yu]

clk_enable() may fail according to the API contract.
Previously, udc_enable() ignored its return value and returned void.

Modify udc_enable() to return the error code. Additionally, update
all of its callers (pxa_udc_pullup, pxa_udc_vbus_session,
pxa27x_udc_start, pxa_udc_probe, and pxa_udc_resume) to check
this return value and handle the failure properly with necessary
cleanups or rollbacks.

Signed-off-by: Zhaoyang Yu <2426767509@qq.com>
---
Changes in v3:
- Changed udc_enable() return type from void to int.
- Propagated the error to all caller functions and added proper
  error handling/rollback per Greg KH's review.

Changes in v2:
- Fixed a formatting issue by moving the 'int ret' declaration to
  the beginning of the function block.

 drivers/usb/gadget/udc/pxa27x_udc.c | 60 ++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c b/drivers/usb/gadget/udc/pxa27x_udc.c
index 897f53601b5b..0a5f05f8e73c 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -1462,7 +1462,7 @@ static int pxa_udc_wakeup(struct usb_gadget *_gadget)
 	return 0;
 }
 
-static void udc_enable(struct pxa_udc *udc);
+static int udc_enable(struct pxa_udc *udc);
 static void udc_disable(struct pxa_udc *udc);
 
 /**
@@ -1519,14 +1519,18 @@ static int should_disable_udc(struct pxa_udc *udc)
 static int pxa_udc_pullup(struct usb_gadget *_gadget, int is_active)
 {
 	struct pxa_udc *udc = to_gadget_udc(_gadget);
+	int ret;
 
 	if (!udc->gpiod && !udc->udc_command)
 		return -EOPNOTSUPP;
 
 	dplus_pullup(udc, is_active);
 
-	if (should_enable_udc(udc))
-		udc_enable(udc);
+	if (should_enable_udc(udc)) {
+		ret = udc_enable(udc);
+		if (ret)
+			return ret;
+	}
 	if (should_disable_udc(udc))
 		udc_disable(udc);
 	return 0;
@@ -1545,10 +1549,14 @@ static int pxa_udc_pullup(struct usb_gadget *_gadget, int is_active)
 static int pxa_udc_vbus_session(struct usb_gadget *_gadget, int is_active)
 {
 	struct pxa_udc *udc = to_gadget_udc(_gadget);
+	int ret;
 
 	udc->vbus_sensed = is_active;
-	if (should_enable_udc(udc))
-		udc_enable(udc);
+	if (should_enable_udc(udc)) {
+		ret = udc_enable(udc);
+		if (ret)
+			return ret;
+	}
 	if (should_disable_udc(udc))
 		udc_disable(udc);
 
@@ -1691,12 +1699,18 @@ static void udc_init_data(struct pxa_udc *dev)
  * Enables the udc device : enables clocks, udc interrupts, control endpoint
  * interrupts, sets usb as UDC client and setups endpoints.
  */
-static void udc_enable(struct pxa_udc *udc)
+static int udc_enable(struct pxa_udc *udc)
 {
+	int ret;
+
 	if (udc->enabled)
-		return;
+		return 0;
 
-	clk_enable(udc->clk);
+	ret = clk_enable(udc->clk);
+	if (ret) {
+		dev_err(udc->dev, "clk_enable failed: %d\n", ret);
+		return ret;
+	}
 	udc_writel(udc, UDCICR0, 0);
 	udc_writel(udc, UDCICR1, 0);
 	udc_clear_mask_UDCCR(udc, UDCCR_UDE);
@@ -1726,6 +1740,8 @@ static void udc_enable(struct pxa_udc *udc)
 	pio_irq_enable(&udc->pxa_ep[0]);
 
 	udc->enabled = 1;
+
+	return 0;
 }
 
 /**
@@ -1761,10 +1777,16 @@ static int pxa27x_udc_start(struct usb_gadget *g,
 		}
 	}
 
-	if (should_enable_udc(udc))
-		udc_enable(udc);
+	if (should_enable_udc(udc)) {
+		retval = udc_enable(udc);
+		if (retval)
+			goto fail_enable;
+	}
 	return 0;
 
+fail_enable:
+	if (!IS_ERR_OR_NULL(udc->transceiver))
+		otg_set_peripheral(udc->transceiver->otg, NULL);
 fail:
 	udc->driver = NULL;
 	return retval;
@@ -2430,10 +2452,16 @@ static int pxa_udc_probe(struct platform_device *pdev)
 		goto err_add_gadget;
 
 	pxa_init_debugfs(udc);
-	if (should_enable_udc(udc))
-		udc_enable(udc);
+	if (should_enable_udc(udc)) {
+		retval = udc_enable(udc);
+		if (retval)
+			goto err_enable;
+	}
 	return 0;
 
+err_enable:
+	usb_del_gadget_udc(&udc->gadget);
+	pxa_cleanup_debugfs(udc);
 err_add_gadget:
 	if (!IS_ERR_OR_NULL(udc->transceiver))
 		usb_unregister_notifier(udc->transceiver, &pxa27x_udc_phy);
@@ -2509,13 +2537,17 @@ static int pxa_udc_resume(struct platform_device *_dev)
 {
 	struct pxa_udc *udc = platform_get_drvdata(_dev);
 	struct pxa_ep *ep;
+	int ret;
 
 	ep = &udc->pxa_ep[0];
 	udc_ep_writel(ep, UDCCSR, udc->udccsr0 & (UDCCSR0_FST | UDCCSR0_DME));
 
 	dplus_pullup(udc, udc->pullup_resume);
-	if (should_enable_udc(udc))
-		udc_enable(udc);
+	if (should_enable_udc(udc)) {
+		ret = udc_enable(udc);
+		if (ret)
+			return ret;
+	}
 	/*
 	 * We do not handle OTG yet.
 	 *
-- 
2.34.1

Re: [PATCH v3] USB: pxa27x_udc: check return value of clk_enable

[Greg KH]

On Wed, Mar 11, 2026 at 01:56:14PM +0000, Zhaoyang Yu wrote:
> clk_enable() may fail according to the API contract.
> Previously, udc_enable() ignored its return value and returned void.
> 
> Modify udc_enable() to return the error code. Additionally, update
> all of its callers (pxa_udc_pullup, pxa_udc_vbus_session,
> pxa27x_udc_start, pxa_udc_probe, and pxa_udc_resume) to check
> this return value and handle the failure properly with necessary
> cleanups or rollbacks.
> 
> Signed-off-by: Zhaoyang Yu <2426767509@qq.com>
> ---
> Changes in v3:
> - Changed udc_enable() return type from void to int.
> - Propagated the error to all caller functions and added proper
>   error handling/rollback per Greg KH's review.
> 
> Changes in v2:
> - Fixed a formatting issue by moving the 'int ret' declaration to
>   the beginning of the function block.
> 
>  drivers/usb/gadget/udc/pxa27x_udc.c | 60 ++++++++++++++++++++++-------
>  1 file changed, 46 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c b/drivers/usb/gadget/udc/pxa27x_udc.c
> index 897f53601b5b..0a5f05f8e73c 100644
> --- a/drivers/usb/gadget/udc/pxa27x_udc.c
> +++ b/drivers/usb/gadget/udc/pxa27x_udc.c
> @@ -1462,7 +1462,7 @@ static int pxa_udc_wakeup(struct usb_gadget *_gadget)
>  	return 0;
>  }
>  
> -static void udc_enable(struct pxa_udc *udc);
> +static int udc_enable(struct pxa_udc *udc);
>  static void udc_disable(struct pxa_udc *udc);
>  
>  /**
> @@ -1519,14 +1519,18 @@ static int should_disable_udc(struct pxa_udc *udc)
>  static int pxa_udc_pullup(struct usb_gadget *_gadget, int is_active)
>  {
>  	struct pxa_udc *udc = to_gadget_udc(_gadget);
> +	int ret;
>  
>  	if (!udc->gpiod && !udc->udc_command)
>  		return -EOPNOTSUPP;
>  
>  	dplus_pullup(udc, is_active);
>  
> -	if (should_enable_udc(udc))
> -		udc_enable(udc);
> +	if (should_enable_udc(udc)) {
> +		ret = udc_enable(udc);
> +		if (ret)
> +			return ret;
> +	}

DOn't you need to change the pullup?

>  	if (should_disable_udc(udc))
>  		udc_disable(udc);
>  	return 0;
> @@ -1545,10 +1549,14 @@ static int pxa_udc_pullup(struct usb_gadget *_gadget, int is_active)
>  static int pxa_udc_vbus_session(struct usb_gadget *_gadget, int is_active)
>  {
>  	struct pxa_udc *udc = to_gadget_udc(_gadget);
> +	int ret;
>  
>  	udc->vbus_sensed = is_active;
> -	if (should_enable_udc(udc))
> -		udc_enable(udc);
> +	if (should_enable_udc(udc)) {
> +		ret = udc_enable(udc);
> +		if (ret)
> +			return ret;
> +	}

Shouldn't you change vbus_sensed?


>  	if (should_disable_udc(udc))
>  		udc_disable(udc);
>  
> @@ -1691,12 +1699,18 @@ static void udc_init_data(struct pxa_udc *dev)
>   * Enables the udc device : enables clocks, udc interrupts, control endpoint
>   * interrupts, sets usb as UDC client and setups endpoints.
>   */
> -static void udc_enable(struct pxa_udc *udc)
> +static int udc_enable(struct pxa_udc *udc)
>  {
> +	int ret;
> +
>  	if (udc->enabled)
> -		return;
> +		return 0;
>  
> -	clk_enable(udc->clk);
> +	ret = clk_enable(udc->clk);
> +	if (ret) {
> +		dev_err(udc->dev, "clk_enable failed: %d\n", ret);
> +		return ret;
> +	}
>  	udc_writel(udc, UDCICR0, 0);
>  	udc_writel(udc, UDCICR1, 0);
>  	udc_clear_mask_UDCCR(udc, UDCCR_UDE);
> @@ -1726,6 +1740,8 @@ static void udc_enable(struct pxa_udc *udc)
>  	pio_irq_enable(&udc->pxa_ep[0]);
>  
>  	udc->enabled = 1;
> +
> +	return 0;
>  }
>  
>  /**
> @@ -1761,10 +1777,16 @@ static int pxa27x_udc_start(struct usb_gadget *g,
>  		}
>  	}
>  
> -	if (should_enable_udc(udc))
> -		udc_enable(udc);
> +	if (should_enable_udc(udc)) {
> +		retval = udc_enable(udc);
> +		if (retval)
> +			goto fail_enable;
> +	}

No other unwinding is needed?

thanks,

greg k-h

Re: [PATCH v3] USB: pxa27x_udc: check return value of clk_enable

[俞朝阳]

Hi Greg,

On Wed, Mar 11, 2026 at 01:56:14PM +0000, Greg Kroah-Hartman wrote:
> >  	dplus_pullup(udc, is_active);
> >  
> > -	if (should_enable_udc(udc))
> > -		udc_enable(udc);
> > +	if (should_enable_udc(udc)) {
> > +		ret = udc_enable(udc);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> DOn't you need to change the pullup?

Yes, you are absolutely right. If udc_enable() fails, the hardware pullup state should be reverted to !is_active. I missed this in v3.

> >  	udc->vbus_sensed = is_active;
> > -	if (should_enable_udc(udc))
> > -		udc_enable(udc);
> > +	if (should_enable_udc(udc)) {
> > +		ret = udc_enable(udc);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> Shouldn't you change vbus_sensed?

Indeed, I should also roll back udc->vbus_sensed = !is_active if the enable fails.

> > +fail_enable:
> > +	if (!IS_ERR_OR_NULL(udc->transceiver))
> > +		otg_set_peripheral(udc->transceiver->otg, NULL);
> >  fail:
> >  	udc->driver = NULL;
> >  	return retval;
> 
> No other unwinding is needed?

I double-checked `pxa27x_udc_start`. The only setup steps before `udc_enable()` are assigning `udc->driver` and calling `otg_set_peripheral()`. The error path undoes both, so there shouldn't be any other unwinding needed here.

However, your question prompted me to check the other functions, and I realized that I also missed rolling back `dplus_pullup` in `pxa_udc_resume()` on failure. I will fix all of these missing rollbacks.

Thank you for the rigorous review! I will send out the v4 patch shortly.

Best regards,
Zhaoyang Yu