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@xxxxxx>
> ---
> 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