Re: [PATCH] usb: gadget: ether: Handle gadget driver registration in start and stop

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Zixun,

Thank you for the patch.

On ven., juil. 26, 2024 at 10:31, Zixun LI <admin@hifiphile.com> wrote:

> Revert part of 718f1d41 to move
>  usb_gadget_register_driver()/usb_gadget_unregister_driver()
> back to usb_eth_start()/usb_eth_stop().
>
> usb_gadget_register_driver() 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
>
> With this patch the USB controller will only be initialized when ethernet
> function is used, in which case USB controller events are handled, so the
> host won't see an unresponsive device.
>
> Signed-off-by: Zixun LI <admin@hifiphile.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

I'd like to test this on my end as well. Could you please give some
details on how this has been tested?

A sequence of U-Boot commands would be helpful, for example.

> ---
>  drivers/usb/gadget/ether.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
> index b7b7bacb00..ed55f12662 100644
> --- a/drivers/usb/gadget/ether.c
> +++ b/drivers/usb/gadget/ether.c
> @@ -2277,6 +2277,9 @@ static int usb_eth_start(struct udevice *udev)
>  	packet_received = 0;
>  	packet_sent = 0;
>
> +	if (usb_gadget_register_driver(&priv->eth_driver) < 0)
> +		goto fail;
> +
>  	gadget = dev->gadget;
>  	usb_gadget_connect(gadget);
>
> @@ -2398,6 +2401,8 @@ static void usb_eth_stop(struct udevice *udev)
>  		dm_usb_gadget_handle_interrupts(udev->parent);
>  		dev->network_started = 0;
>  	}
> +
> +	usb_gadget_unregister_driver(&priv->eth_driver);
>  }
>
>  static int usb_eth_recv(struct udevice *dev, int flags, uchar **packetp)
> @@ -2503,15 +2508,6 @@ 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);
> -}
> -
> -static int usb_eth_remove(struct udevice *dev)
> -{
> -	struct ether_priv *priv = dev_get_priv(dev);
> -
> -	usb_gadget_unregister_driver(&priv->eth_driver);
> -
>  	return 0;
>  }
>
> @@ -2526,7 +2522,6 @@ U_BOOT_DRIVER(eth_usb) = {
>  	.name	= "usb_ether",
>  	.id	= UCLASS_ETH,
>  	.probe	= usb_eth_probe,
> -	.remove	= usb_eth_remove,
>  	.unbind	= usb_eth_unbind,
>  	.ops	= &usb_eth_ops,
>  	.priv_auto	= sizeof(struct ether_priv),
> --
> 2.45.2