Hi Greg,

On Wed, Mar 11, 2026 at 01:56:14PM +0000, Greg Kroah-Hartman wrote:

quoted

 	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.

quoted

 	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.

quoted

+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