The device controller may update vbus status via usb_udc_vbus_handler(),
which tries to connect the gadget even though gadget_bind_driver() has
already called usb_udc_connect_control_locked(). This causes pullup() to
be called twice. Avoid this by checking if gadget->connected is true.

Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
 drivers/usb/gadget/udc/core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index e8861eaad907..be29daa7ad3c 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -712,6 +712,9 @@ static int usb_gadget_connect_locked(struct usb_gadget *gadget)
 		goto out;
 	}
 
+	if (gadget->connected)
+		goto out;
+
 	if (gadget->deactivated || !gadget->udc->allow_connect || !gadget->udc->started) {
 		/*
 		 * If the gadget isn't usable (because it is deactivated,
-- 
2.34.1

On Tue, Apr 21, 2026 at 04:20:50PM +0800, Xu Yang wrote:
> The device controller may update vbus status via usb_udc_vbus_handler(),
> which tries to connect the gadget even though gadget_bind_driver() has
> already called usb_udc_connect_control_locked(). This causes pullup() to
> be called twice. Avoid this by checking if gadget->connected is true.

This patch is wrong.  To see why, read the comments just below the end 
of the patch and see also usb_gadget_activate().

(Also, is there really anything wrong with calling pullup() twice in a 
row?)

Alan Stern

> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> ---
>  drivers/usb/gadget/udc/core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index e8861eaad907..be29daa7ad3c 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -712,6 +712,9 @@ static int usb_gadget_connect_locked(struct usb_gadget *gadget)
>  		goto out;
>  	}
>  
> +	if (gadget->connected)
> +		goto out;
> +
>  	if (gadget->deactivated || !gadget->udc->allow_connect || !gadget->udc->started) {
>  		/*
>  		 * If the gadget isn't usable (because it is deactivated,
> -- 
> 2.34.1
> 
>

On Tue, Apr 21, 2026 at 10:37:53AM -0400, Alan Stern wrote:
> On Tue, Apr 21, 2026 at 04:20:50PM +0800, Xu Yang wrote:
> > The device controller may update vbus status via usb_udc_vbus_handler(),
> > which tries to connect the gadget even though gadget_bind_driver() has
> > already called usb_udc_connect_control_locked(). This causes pullup() to
> > be called twice. Avoid this by checking if gadget->connected is true.
> 
> This patch is wrong.  To see why, read the comments just below the end 
> of the patch and see also usb_gadget_activate().

OK. So it breaks usb_gadget_activate() as usb_udc_connect_control_locked()
return early.

I think usb_gadget_activate() needs to set gadget->connected as false before
running usb_gadget_connect_locked() just like usb_gadget_deactivate() sets
gadget->connected as true after running usb_gadget_disconnect_locked().

Then it will work correctly, do you agree?

> 
> (Also, is there really anything wrong with calling pullup() twice in a 
> row?)

It depends on the device controller driver. But currently only a few drivers
call usb_udc_vbus_handler(), so I think the issue hasn't shown up.

When you look at the pullup() implementation of some UDC driver, you may see
they do more complicated thing than just pulling the data line up. Such as
cdnsp_gadget_pullup(), dwc3_gadget_pullup(), etc.

Therefore, I think the UDC core need to handle this well to avoid calling
pullup() twice in a row.

Thanks,
Xu Yang

On Wed, Apr 22, 2026 at 07:31:16PM +0800, Xu Yang wrote:
> On Tue, Apr 21, 2026 at 10:37:53AM -0400, Alan Stern wrote:
> > On Tue, Apr 21, 2026 at 04:20:50PM +0800, Xu Yang wrote:
> > > The device controller may update vbus status via usb_udc_vbus_handler(),
> > > which tries to connect the gadget even though gadget_bind_driver() has
> > > already called usb_udc_connect_control_locked(). This causes pullup() to
> > > be called twice. Avoid this by checking if gadget->connected is true.
> > 
> > This patch is wrong.  To see why, read the comments just below the end 
> > of the patch and see also usb_gadget_activate().
> 
> OK. So it breaks usb_gadget_activate() as usb_udc_connect_control_locked()
> return early.
> 
> I think usb_gadget_activate() needs to set gadget->connected as false before
> running usb_gadget_connect_locked() just like usb_gadget_deactivate() sets
> gadget->connected as true after running usb_gadget_disconnect_locked().
> 
> Then it will work correctly, do you agree?

Yes, that should take care of it.  Don't forget to update the patch 
description as well.

> > (Also, is there really anything wrong with calling pullup() twice in a 
> > row?)
> 
> It depends on the device controller driver. But currently only a few drivers
> call usb_udc_vbus_handler(), so I think the issue hasn't shown up.
> 
> When you look at the pullup() implementation of some UDC driver, you may see
> they do more complicated thing than just pulling the data line up. Such as
> cdnsp_gadget_pullup(), dwc3_gadget_pullup(), etc.
> 
> Therefore, I think the UDC core need to handle this well to avoid calling
> pullup() twice in a row.

Okay.

Alan Stern

Alan Stern <stern@rowland.harvard.edu> writes:

> This patch is wrong.  To see why, read the comments just below the end 
> of the patch and see also usb_gadget_activate().


Made me look...

Must say. This strikes me as a nice way to filter out humans from the
rest of the submitters:

                gadget->connected = true;
                goto out;
        }
 
        ret = gadget->ops->pullup(gadget, 1);
        if (!ret)
                gadget->connected = 1;


The indecisiveness looks strange. There's a nice symmetry with
usb_gadget_disconnect_locked() though:

                gadget->connected = false;
                goto out;
        }
 
        ret = gadget->ops->pullup(gadget, 0);
        if (!ret)
                gadget->connected = 0;




What surprised me most was that the different variants were added by the
same commit
ccdf138fe3e2 ("usb: gadget: add usb_gadget_activate/deactivate functions").


Bjørn

On Tue, Apr 21, 2026 at 05:27:32PM +0200, Bjørn Mork wrote:
> Alan Stern <stern@rowland.harvard.edu> writes:
> 
> > This patch is wrong.  To see why, read the comments just below the end 
> > of the patch and see also usb_gadget_activate().
> 
> 
> Made me look...
> 
> Must say. This strikes me as a nice way to filter out humans from the
> rest of the submitters:
> 
>                 gadget->connected = true;
>                 goto out;
>         }
>  
>         ret = gadget->ops->pullup(gadget, 1);
>         if (!ret)
>                 gadget->connected = 1;
> 
> 
> The indecisiveness looks strange. There's a nice symmetry with
> usb_gadget_disconnect_locked() though:
> 
>                 gadget->connected = false;
>                 goto out;
>         }
>  
>         ret = gadget->ops->pullup(gadget, 0);
>         if (!ret)
>                 gadget->connected = 0;

Granted, it does look a little strange.  ->connected is actually a 1-bit 
bitfield, though, so treating it as a boolean instead of an integer is 
not completely beyond the bounds of reason.

> What surprised me most was that the different variants were added by the
> same commit
> ccdf138fe3e2 ("usb: gadget: add usb_gadget_activate/deactivate functions").

But this isn't what my rejection was complaining about.  The patch's 
problem was functional, not aesthetic.

Alan Stern