Re: [PATCH v2 4/4] usb: gadget: udc: Decide on connecting in one place

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: gadget: udc: stale gadget->connected if pullup disable
  fails on unbind
--

commit 8198ac49b976bc4f15b1f15c4abb55ae61ceb982
Author: Alexey Charkov <alchark@flipper.net>

usb: gadget: udc: Decide on connecting in one place

This commit centralizes the UDC connection rules in
usb_udc_update_connection_locked() so that gadget->connected only tracks
whether the pullup is enabled.

> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 84dc0820c2cb9..e592705de55e4 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
[ ... ]
> @@ -1236,16 +1238,8 @@ static inline int usb_gadget_udc_start_locked(struct usb_udc *udc)
>  	}
>  
>  	ret = udc->gadget->ops->udc_start(udc->gadget, udc->driver);
> -	if (!ret) {
> +	if (!ret)
>  		udc->started = true;
> -		/*
> -		 * The pullup is never enabled while the UDC is stopped, so any
> -		 * connection recorded meanwhile was only a request. Forget it,
> -		 * so that the next usb_gadget_connect_locked() enables the
> -		 * pullup rather than returning early.
> -		 */
> -		udc->gadget->connected = false;
> -	}
>  
>  	return ret;
>  }
[ ... ]
> @@ -1733,15 +1729,17 @@ static void gadget_unbind_driver(struct device *dev)
>  
>  	dev_dbg(&udc->dev, "unbinding gadget driver [%s]\n", driver->function);
>  
> -	udc->allow_connect = false;
> -	cancel_work_sync(&udc->vbus_work);
>  	mutex_lock(&udc->connect_lock);
> -	usb_gadget_disconnect_locked(gadget);
> +	udc->bound = false;
> +	usb_udc_update_connection_locked(udc);
>  	usb_gadget_disable_async_callbacks(udc);
>  	if (gadget->irq)
>  		synchronize_irq(gadget->irq);
>  	mutex_unlock(&udc->connect_lock);
>  
> +	/* A VBUS event can no longer connect the gadget, so just wait it out */
> +	cancel_work_sync(&udc->vbus_work);

[Severity: Medium]
Can gadget->connected stay set across unbind and prevent the next bound
gadget driver from enabling the pullup?

In usb_gadget_disconnect_locked(), gadget->connected is only cleared when
gadget->ops->pullup(gadget, 0) returns 0, and some UDC drivers fail that
call when VBUS is already inactive:

drivers/usb/gadget/udc/fsl_udc_core.c:fsl_pullup() {
	...
	if (!udc->vbus_active)
		return -EOPNOTSUPP;
	...
}

When that happens during gadget_unbind_driver(), neither
usb_gadget_udc_stop_locked() nor usb_udc_forget_driver_locked() clears
gadget->connected, and usb_gadget_udc_start_locked() no longer resets it
either.  On the next gadget_bind_driver(), usb_gadget_connect_locked() sees
gadget->connected == 1 and returns early without calling
gadget->ops->pullup(gadget, 1).

Also, if vbus_work is pending when cancel_work_sync() runs after
usb_gadget_disable_async_callbacks(), a lingering gadget->connected == 1
causes vbus_event_work() -> usb_gadget_disconnect_locked() to invoke
driver->disconnect() after async callbacks have been disabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-udc-connect-v2-0-f51b11f07eb8@flipper.net?part=4