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