Re: [PATCH] usb: gadget: lpc32xx_udc: drain the pullup work on removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: gadget: lpc32xx_udc: cancel_work_sync() skips
  pullup_work() on remove
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] usb: gadget: lpc32xx_udc: incorrect work struct scheduled in
  udc_handle_dev() [1]
--

commit 4049fdca943c63a00b8c69476b8030baf97607c6
Author: Hyunjung Ko <hj351016@gmail.com>

usb: gadget: lpc32xx_udc: drain the pullup work on removal

This commit cancels udc->pullup_job in lpc32xx_udc_remove() before releasing
the devm-allocated udc and its isp1301 i2c client.

> diff --git a/drivers/usb/gadget/udc/lpc32xx_udc.c b/drivers/usb/gadget/udc/lpc32xx_udc.c
> index 94355d3d930fd..210fa1f886c8a 100644
> --- a/drivers/usb/gadget/udc/lpc32xx_udc.c
> +++ b/drivers/usb/gadget/udc/lpc32xx_udc.c
> @@ -3187,6 +3187,13 @@ static void lpc32xx_udc_remove(struct platform_device *pdev)
>  	udc_disable(udc);
>  	pullup(udc, 0);
>  
> +	/*
> +	 * pullup() schedules pullup_job to do the slow isp1301 i2c access; the
> +	 * work dereferences udc, so drain it before the devm-allocated udc (and
> +	 * the isp1301 i2c client it uses) are released.
> +	 */
> +	cancel_work_sync(&udc->pullup_job);

[Severity: Medium]
Does cancel_work_sync(&udc->pullup_job) skip disabling the ISP1301 D+
pull-up on removal?

pullup(udc, 0) calls isp1301_pullup_enable(udc, 0, 0) with block = 0, which
sets udc->pullup = 0 and queues udc->pullup_job so pullup_work() ->
isp1301_pullup_set() clears OTG1_DP_PULLUP over I2C:

drivers/usb/gadget/udc/lpc32xx_udc.c:isp1301_pullup_enable() {
	if (en_pullup == udc->pullup)
		return;

	udc->pullup = en_pullup;
	if (block)
		isp1301_pullup_set(udc);
	else
		/* defer slow i2c pull up setting */
		schedule_work(&udc->pullup_job);
}

Canceling udc->pullup_job right after scheduling it prevents pullup_work()
from running if it has not started yet, leaving OTG1_DP_PULLUP asserted in
the transceiver after the UDC is disabled and unbound.  Using flush_work()
here would let the queued pull-up disable complete before tearing down udc.

[Severity: Medium]
This problem wasn't introduced by this patch, but udc_handle_dev() schedules
udc->pullup_job instead of udc->power_job when handling suspend and resume
events [1]:

drivers/usb/gadget/udc/lpc32xx_udc.c:udc_handle_dev() {
    ...
				/* Power down transceiver */
				udc->poweron = 0;
				schedule_work(&udc->pullup_job);
				uda_resm_susp_event(udc, 1);
			}
		} else if ((udc->gadget.speed != USB_SPEED_UNKNOWN) &&
			   udc->driver && udc->vbus) {
			uda_resm_susp_event(udc, 0);
			/* Power up transceiver */
			udc->poweron = 1;
			schedule_work(&udc->pullup_job);
		}
}

Because pullup_work() ignores udc->poweron and udc->power_job is never
scheduled anywhere else, power_work() never runs and the ISP1301 transceiver
never transitions power states across suspend and resume.

[1] https://sashiko.dev/#/bug/linux-931b6f24-8f05-4e96-9acb-90ce7f88f38f

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110114351.2524921-1-hj351016@gmail.com?part=1