[PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[Zixun LI]

usb_gadget_register_driver() called by probe will initialize the USB
controller which enters ready to connect state with pull-up resistor
enabled.

From the host's point of view, a device is ready to be enumerated.
However, since dm_usb_gadget_handle_interrupts() is only called when
ethernet function is started, at this stage USB events are not managed,
host's enumeration attempts will fail and resulting error like:
    usb 1-1: new high-speed USB device number 50 using xhci_hcd
    usb 1-1: device descriptor read/64, error -110
    usb 1-1: device descriptor read/64, error -110
    usb usb1-port1: attempt power cycle

Disable USB pullup to prevent unwanted enumeration attempt.

Signed-off-by: Zixun LI <admin@hifiphile.com>
---
 drivers/usb/gadget/ether.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
index 7973927e8a..c864beeaa7 100644
--- a/drivers/usb/gadget/ether.c
+++ b/drivers/usb/gadget/ether.c
@@ -2459,6 +2459,8 @@ static int usb_eth_probe(struct udevice *dev)
 {
 	struct ether_priv *priv = dev_get_priv(dev);
 	struct eth_pdata *pdata = dev_get_plat(dev);
+	struct eth_dev *ethdev = &priv->ethdev;
+	int ret;

 	priv->netdev = dev;
 	l_priv = priv;
@@ -2499,7 +2501,14 @@ static int usb_eth_probe(struct udevice *dev)
 	priv->eth_driver.disconnect	= eth_disconnect;
 	priv->eth_driver.suspend	= eth_suspend;
 	priv->eth_driver.resume		= eth_resume;
-	return usb_gadget_register_driver(&priv->eth_driver);
+
+	ret = usb_gadget_register_driver(&priv->eth_driver);
+	if (ret)
+		return ret;
+
+	/* Keep pullup disabled until interrupt is available */
+	usb_gadget_disconnect(ethdev->gadget);
+	return 0;
 }

 static int usb_eth_remove(struct udevice *dev)
--
2.45.2

Re: [PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[Marek Vasut]

On 8/21/24 11:22 PM, Zixun LI wrote:
> usb_gadget_register_driver() called by probe will initialize the USB
> controller which enters ready to connect state with pull-up resistor
> enabled.
> 
>>From the host's point of view, a device is ready to be enumerated.
> However, since dm_usb_gadget_handle_interrupts() is only called when
> ethernet function is started, at this stage USB events are not managed,
> host's enumeration attempts will fail and resulting error like:
>      usb 1-1: new high-speed USB device number 50 using xhci_hcd
>      usb 1-1: device descriptor read/64, error -110
>      usb 1-1: device descriptor read/64, error -110
>      usb usb1-port1: attempt power cycle
> 
> Disable USB pullup to prevent unwanted enumeration attempt.
> 
> Signed-off-by: Zixun LI <admin@hifiphile.com>
> ---
>   drivers/usb/gadget/ether.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
> index 7973927e8a..c864beeaa7 100644
> --- a/drivers/usb/gadget/ether.c
> +++ b/drivers/usb/gadget/ether.c
> @@ -2459,6 +2459,8 @@ static int usb_eth_probe(struct udevice *dev)
>   {
>   	struct ether_priv *priv = dev_get_priv(dev);
>   	struct eth_pdata *pdata = dev_get_plat(dev);
> +	struct eth_dev *ethdev = &priv->ethdev;
> +	int ret;
> 
>   	priv->netdev = dev;
>   	l_priv = priv;
> @@ -2499,7 +2501,14 @@ static int usb_eth_probe(struct udevice *dev)
>   	priv->eth_driver.disconnect	= eth_disconnect;
>   	priv->eth_driver.suspend	= eth_suspend;
>   	priv->eth_driver.resume		= eth_resume;
> -	return usb_gadget_register_driver(&priv->eth_driver);
> +
> +	ret = usb_gadget_register_driver(&priv->eth_driver);

Wouldn't it be better if usb_gadget_register_driver() started in 
disconnected state right away ?

Re: [PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[Zixun LI]

Hi Marek,

On Wed, Aug 21, 2024 at 11:43 PM Marek Vasut <marex@denx.de> wrote:
>
> Wouldn't it be better if usb_gadget_register_driver() started in
> disconnected state right away ?

That's what I did initially. But since g_dnl doesn't do connection after
register (I explained in cover letter) doing so will keep g_dnl always in
disconnected state.

Re: [PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[Marek Vasut]

On 8/21/24 11:58 PM, Zixun LI wrote:
> Hi Marek,

Hi,

> On Wed, Aug 21, 2024 at 11:43 PM Marek Vasut <marex@denx.de> wrote:
>>
>> Wouldn't it be better if usb_gadget_register_driver() started in
>> disconnected state right away ?
> 
> That's what I did initially. But since g_dnl doesn't do connection after
> register (I explained in cover letter) doing so will keep g_dnl always in
> disconnected state.

Would it be possible to fix up the g_dnl ?

Re: [PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[Zixun LI]

Hi,

On Thu, Aug 22, 2024 at 1:26 AM Marek Vasut <marex@denx.de> wrote:
>
> Would it be possible to fix up the g_dnl ?

g_dnl's interface is pretty different from usb_ether, I can't find a
way to access
usb_gadget struct in g_dnl_register().

Meanwhile it can be fixed in udc core, by doing gadget_driver bind
after udc start,
in addition to keeping the controller in disconnected state.

diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index 6bb419ae2a..b917a79892 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -300,15 +300,17 @@ static int udc_bind_to_driver(struct usb_udc
*udc, struct usb_gadget_driver *dri

     usb_gadget_udc_set_speed(udc, driver->speed);

-    ret = driver->bind(udc->gadget);
-    if (ret)
-        goto err1;
     ret = usb_gadget_udc_start(udc);
     if (ret) {
-        driver->unbind(udc->gadget);
         goto err1;
     }
-    usb_gadget_connect(udc->gadget);
+
+    /* Keep pullup disabled until interrupt is available */
+    usb_gadget_disconnect(udc->gadget);
+
+    ret = driver->bind(udc->gadget);
+    if (ret)
+        goto err1;

     return 0;
 err1:

Re: [PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[Marek Vasut]

On 8/22/24 11:11 AM, Zixun LI wrote:
> Hi,

Hi,

> On Thu, Aug 22, 2024 at 1:26 AM Marek Vasut <marex@denx.de> wrote:
>>
>> Would it be possible to fix up the g_dnl ?
> 
> g_dnl's interface is pretty different from usb_ether, I can't find a
> way to access
> usb_gadget struct in g_dnl_register().

Can you change the g_dnl_register() prototype and pass he structure in ? 
I suspect it should be available in the commands which register g_dnl ?

> Meanwhile it can be fixed in udc core, by doing gadget_driver bind
> after udc start,
> in addition to keeping the controller in disconnected state.
> 
> diff --git a/drivers/usb/gadget/udc/udc-core.c
> b/drivers/usb/gadget/udc/udc-core.c
> index 6bb419ae2a..b917a79892 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -300,15 +300,17 @@ static int udc_bind_to_driver(struct usb_udc
> *udc, struct usb_gadget_driver *dri
> 
>       usb_gadget_udc_set_speed(udc, driver->speed);
> 
> -    ret = driver->bind(udc->gadget);
> -    if (ret)
> -        goto err1;
>       ret = usb_gadget_udc_start(udc);
>       if (ret) {
> -        driver->unbind(udc->gadget);
>           goto err1;
>       }
> -    usb_gadget_connect(udc->gadget);
> +
> +    /* Keep pullup disabled until interrupt is available */
> +    usb_gadget_disconnect(udc->gadget);

This is no good, this is a workaround, let's not do this.

> +    ret = driver->bind(udc->gadget);
> +    if (ret)
> +        goto err1;
> 
>       return 0;
>   err1:

[...]

Re: [PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[Zixun LI]

On Thu, Aug 22, 2024 at 5:24 PM Marek Vasut <marex@denx.de> wrote:
>
> Can you change the g_dnl_register() prototype and pass he structure in ?
> I suspect it should be available in the commands which register g_dnl ?
>

There are many places where g_dnl_register() is called, like fastboot, dfu, ums,
etc, need to modify all of them ?

>
> This is no good, this is a workaround, let's not do this.
>

You mean which part, order change or disconnect call ?

Is there a fixed order of udc_start (device init) and driver->bind ?
In non-DM mode most device drivers do init first in their
usb_gadget_register_driver(), except udc-core and DWC2 who does
driver->bind first.

Call usb_gadget_disconnect here could avoid modifying all device driver's init.

Re: [PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[Marek Vasut]

On 8/22/24 5:53 PM, Zixun LI wrote:
> On Thu, Aug 22, 2024 at 5:24 PM Marek Vasut <marex@denx.de> wrote:
>>
>> Can you change the g_dnl_register() prototype and pass he structure in ?
>> I suspect it should be available in the commands which register g_dnl ?
>>
> 
> There are many places where g_dnl_register() is called, like fastboot, dfu, ums,
> etc, need to modify all of them ?

I think that should be possible with 'git grep' and 'sed' mechanically.

>> This is no good, this is a workaround, let's not do this.
>>
> 
> You mean which part, order change or disconnect call ?

Yes

> Is there a fixed order of udc_start (device init) and driver->bind ?
> In non-DM mode most device drivers do init first in their
> usb_gadget_register_driver(), except udc-core and DWC2 who does
> driver->bind first.

The driver .bind callback is called when the driver is instantiated 
(e.g. matching compatible string is found in DT, or some board file 
instantiates a driver and binds it to a DT node). This can happen very 
early.

The driver .probe callback initializes the controller hardware and is 
called only the first time the controller is used. U-Boot uses lazy 
hardware initialization to speed up the boot process, i.e. hardware is 
initialized the first time it is used, using the .probe callback.

The .udc_start callback in usb_gadget_ops must be called after the 
controller driver .bind and .probe were called.

The usb_composite_driver .bind has to be called after controller driver 
.bind (but it may be called before controller driver .probe).

I think udc_bind_to_driver() should not call usb_gadget_connect() , that 
connect should likely be called by at some later point. I also think 
that drivers/usb/gadget/udc/udc-core.c might have to be extended to 
provide some way to convert controller struct udevice to struct 
usb_gadget , so when e.g. run_usb_dnl_gadget() calls 
udc_device_get_by_index() and obtains struct udevice * , it can also get 
matching struct usb_gadget * , and call the usb_gadget_connect().

What do you think ?

> Call usb_gadget_disconnect here could avoid modifying all device driver's init.

[...]

Re: [PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[Zixun LI]

On Fri, Aug 23, 2024 at 4:54 AM Marek Vasut <marex@denx.de> wrote:
>
> I think udc_bind_to_driver() should not call usb_gadget_connect() , that
> connect should likely be called by at some later point.

Yes it's more logical, to keep it in disconnected state you prefer modify
controllers drivers init (I can do usba_udc and dwc2) or add a disconnect()
call ?

> I also think
> that drivers/usb/gadget/udc/udc-core.c might have to be extended to
> provide some way to convert controller struct udevice to struct
> usb_gadget , so when e.g. run_usb_dnl_gadget() calls
> udc_device_get_by_index() and obtains struct udevice * , it can also get
> matching struct usb_gadget * , and call the usb_gadget_connect().
>
> What do you think ?

I think it's a good idea.

If there are multiple gadget controllers (very rare), since
usb_gadget_probe_driver() simply takes the first one from udc list,
is there a chance that the one to be started is different from the one
used by udc_device_get_by_index() ?

Re: [PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[Marek Vasut]

On 8/23/24 11:22 AM, Zixun LI wrote:
> On Fri, Aug 23, 2024 at 4:54 AM Marek Vasut <marex@denx.de> wrote:
>>
>> I think udc_bind_to_driver() should not call usb_gadget_connect() , that
>> connect should likely be called by at some later point.
> 
> Yes it's more logical, to keep it in disconnected state you prefer modify
> controllers drivers init (I can do usba_udc and dwc2) or add a disconnect()
> call ?

I think modify the init is the best option.

>> I also think
>> that drivers/usb/gadget/udc/udc-core.c might have to be extended to
>> provide some way to convert controller struct udevice to struct
>> usb_gadget , so when e.g. run_usb_dnl_gadget() calls
>> udc_device_get_by_index() and obtains struct udevice * , it can also get
>> matching struct usb_gadget * , and call the usb_gadget_connect().
>>
>> What do you think ?
> 
> I think it's a good idea.
> 
> If there are multiple gadget controllers (very rare), since
> usb_gadget_probe_driver() simply takes the first one from udc list,
> is there a chance that the one to be started is different from the one
> used by udc_device_get_by_index() ?

Yes, I think that chance exists.

I am looking at the usb_add_gadget_udc_release() and 
usb_gadget_probe_driver() and I am thinking, sigh, all that code is 
ready to be replaced by something cleaner which does not use this 
special struct usb_udc and udc_list, but instead only uses U-Boot DM 
udevices . But I am reluctant to ask you to do all that rework, because 
that is WAY out of scope of the problem you are trying to fix.

Re: [PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[Marek Vasut]

On 8/25/24 1:23 AM, Marek Vasut wrote:
> On 8/23/24 11:22 AM, Zixun LI wrote:
>> On Fri, Aug 23, 2024 at 4:54 AM Marek Vasut <marex@denx.de> wrote:
>>>
>>> I think udc_bind_to_driver() should not call usb_gadget_connect() , that
>>> connect should likely be called by at some later point.
>>
>> Yes it's more logical, to keep it in disconnected state you prefer modify
>> controllers drivers init (I can do usba_udc and dwc2) or add a 
>> disconnect()
>> call ?
> 
> I think modify the init is the best option.
> 
>>> I also think
>>> that drivers/usb/gadget/udc/udc-core.c might have to be extended to
>>> provide some way to convert controller struct udevice to struct
>>> usb_gadget , so when e.g. run_usb_dnl_gadget() calls
>>> udc_device_get_by_index() and obtains struct udevice * , it can also get
>>> matching struct usb_gadget * , and call the usb_gadget_connect().
>>>
>>> What do you think ?
>>
>> I think it's a good idea.
>>
>> If there are multiple gadget controllers (very rare), since
>> usb_gadget_probe_driver() simply takes the first one from udc list,
>> is there a chance that the one to be started is different from the one
>> used by udc_device_get_by_index() ?
> 
> Yes, I think that chance exists.
> 
> I am looking at the usb_add_gadget_udc_release() and 
> usb_gadget_probe_driver() and I am thinking, sigh, all that code is 
> ready to be replaced by something cleaner which does not use this 
> special struct usb_udc and udc_list, but instead only uses U-Boot DM 
> udevices . But I am reluctant to ask you to do all that rework, because 
> that is WAY out of scope of the problem you are trying to fix.

Maybe this could be a start (compile tested only):

https://source.denx.de/u-boot/custodians/u-boot-sh/-/commits/usb-udc-udevice

Re: [PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[Zixun LI]

On Sun, Aug 25, 2024 at 2:13 AM Marek Vasut <marex@denx.de> wrote:
>
> >
> > I am looking at the usb_add_gadget_udc_release() and
> > usb_gadget_probe_driver() and I am thinking, sigh, all that code is
> > ready to be replaced by something cleaner which does not use this
> > special struct usb_udc and udc_list, but instead only uses U-Boot DM
> > udevices . But I am reluctant to ask you to do all that rework, because
> > that is WAY out of scope of the problem you are trying to fix.
>

Thank you for looking into this, refactor udc core could make things clear.

> Maybe this could be a start (compile tested only):
>
> https://source.denx.de/u-boot/custodians/u-boot-sh/-/commits/usb-udc-udevice

Tested no regression with usba_udc.

Re: [PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[Marek Vasut]

On 8/26/24 11:12 AM, Zixun LI wrote:
> On Sun, Aug 25, 2024 at 2:13 AM Marek Vasut <marex@denx.de> wrote:
>>
>>>
>>> I am looking at the usb_add_gadget_udc_release() and
>>> usb_gadget_probe_driver() and I am thinking, sigh, all that code is
>>> ready to be replaced by something cleaner which does not use this
>>> special struct usb_udc and udc_list, but instead only uses U-Boot DM
>>> udevices . But I am reluctant to ask you to do all that rework, because
>>> that is WAY out of scope of the problem you are trying to fix.
>>
> 
> Thank you for looking into this, refactor udc core could make things clear.

Sigh, yeah, I think you ran into part of the UDC code which is just ... 
not great ... sorry about that.

>> Maybe this could be a start (compile tested only):
>>
>> https://source.denx.de/u-boot/custodians/u-boot-sh/-/commits/usb-udc-udevice
> 
> Tested no regression with usba_udc.

Thanks for testing, patches posted.

Re: [PATCH 1/1] usb: gadget: ether: Disable USB pullup in eth probe

[Mattijs Korpershoek]

On lun., août 26, 2024 at 16:40, Marek Vasut <marex@denx.de> wrote:

> On 8/26/24 11:12 AM, Zixun LI wrote:
>> On Sun, Aug 25, 2024 at 2:13 AM Marek Vasut <marex@denx.de> wrote:
>>>
>>>>
>>>> I am looking at the usb_add_gadget_udc_release() and
>>>> usb_gadget_probe_driver() and I am thinking, sigh, all that code is
>>>> ready to be replaced by something cleaner which does not use this
>>>> special struct usb_udc and udc_list, but instead only uses U-Boot DM
>>>> udevices . But I am reluctant to ask you to do all that rework, because
>>>> that is WAY out of scope of the problem you are trying to fix.
>>>
>> 
>> Thank you for looking into this, refactor udc core could make things clear.
>
> Sigh, yeah, I think you ran into part of the UDC code which is just ... 
> not great ... sorry about that.
>
>>> Maybe this could be a start (compile tested only):
>>>
>>> https://source.denx.de/u-boot/custodians/u-boot-sh/-/commits/usb-udc-udevice
>> 
>> Tested no regression with usba_udc.
>
> Thanks for testing, patches posted.

For reference, the series was posted here:

https://lore.kernel.org/r/20240826143851.8020-1-marek.vasut+renesas@mailbox.org