[PATCH v2] usb: gadget: ether: split start/stop from init/halt
Split out _usb_eth_start() from _usb_eth_init() and usb_eth_stop() from _usb_eth_halt(). Now _usb_eth_init() only initialises and registers the gadget device, which _usb_eth_halt() reverses, and together are used for probing and removing the device. The _usb_eth_start() and _usb_eth_stop() functions connect and disconnect the gadget as expected by the start()/stop() callbacks.
Previously the gadget device was probed on every start() and removed on every stop(), which is inconsistent with other DM_ETH drivers.
Signed-off-by: Niel Fourie lusus@denx.de Cc: Marek Vasut marex@denx.de Cc: Lukasz Majewski lukma@denx.de Cc: Ramon Fried rfried.dev@gmail.com --- Changes for v2: - fixed variable declaration order - removed non-DM_ETH code due to upstream removal during rebasing
drivers/usb/gadget/ether.c | 51 +++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index 85c971e4c4..914427eb77 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -2274,15 +2274,11 @@ fail:
/*-------------------------------------------------------------------------*/ static void _usb_eth_halt(struct ether_priv *priv); +static void _usb_eth_stop(struct ether_priv *priv);
static int _usb_eth_init(struct ether_priv *priv) { - struct eth_dev *dev = &priv->ethdev; - struct usb_gadget *gadget; - unsigned long ts; int ret; - unsigned long timeout = USB_CONNECT_TIMEOUT; - ret = usb_gadget_initialize(0); if (ret) return ret; @@ -2322,14 +2318,26 @@ static int _usb_eth_init(struct ether_priv *priv) priv->eth_driver.resume = eth_resume; if (usb_gadget_register_driver(&priv->eth_driver) < 0) goto fail; + return 0; +fail: + _usb_eth_halt(priv); + return -1; +}
- dev->network_started = 0; +static int _usb_eth_start(struct ether_priv *priv) +{ + unsigned long timeout = USB_CONNECT_TIMEOUT; + struct eth_dev *dev = &priv->ethdev; + unsigned long ts;
+ if (!dev->gadget) + return -1; + + dev->network_started = 0; packet_received = 0; packet_sent = 0;
- gadget = dev->gadget; - usb_gadget_connect(gadget); + usb_gadget_connect(dev->gadget);
if (env_get("cdc_connect_timeout")) timeout = dectoul(env_get("cdc_connect_timeout"), NULL) * CONFIG_SYS_HZ; @@ -2347,6 +2355,7 @@ static int _usb_eth_init(struct ether_priv *priv) rx_submit(dev, dev->rx_req, 0); return 0; fail: + _usb_eth_stop(priv); _usb_eth_halt(priv); return -1; } @@ -2426,11 +2435,10 @@ static int _usb_eth_recv(struct ether_priv *priv) return 0; }
-static void _usb_eth_halt(struct ether_priv *priv) +static void _usb_eth_stop(struct ether_priv *priv) { struct eth_dev *dev = &priv->ethdev;
- /* If the gadget not registered, simple return */ if (!dev->gadget) return;
@@ -2454,6 +2462,14 @@ static void _usb_eth_halt(struct ether_priv *priv) usb_gadget_handle_interrupts(0); dev->network_started = 0; } +} + +static void _usb_eth_halt(struct ether_priv *priv) +{ + struct eth_dev *dev = &priv->ethdev; + + if (!dev->gadget) + return;
usb_gadget_unregister_driver(&priv->eth_driver); usb_gadget_release(0); @@ -2463,7 +2479,7 @@ static int usb_eth_start(struct udevice *dev) { struct ether_priv *priv = dev_get_priv(dev);
- return _usb_eth_init(priv); + return _usb_eth_start(priv); }
static int usb_eth_send(struct udevice *dev, void *packet, int length) @@ -2513,7 +2529,7 @@ static void usb_eth_stop(struct udevice *dev) { struct ether_priv *priv = dev_get_priv(dev);
- _usb_eth_halt(priv); + _usb_eth_stop(priv); }
static int usb_eth_probe(struct udevice *dev) @@ -2527,6 +2543,16 @@ static int usb_eth_probe(struct udevice *dev) get_ether_addr(CONFIG_USBNET_DEV_ADDR, pdata->enetaddr); eth_env_set_enetaddr("usbnet_devaddr", pdata->enetaddr);
+ return _usb_eth_init(priv); + + return 0; +} + +static int usb_eth_remove(struct udevice *dev) +{ + struct ether_priv *priv = dev_get_priv(dev); + + _usb_eth_halt(priv); return 0; }
@@ -2562,6 +2588,7 @@ U_BOOT_DRIVER(eth_usb) = { .name = "usb_ether", .id = UCLASS_ETH, .probe = usb_eth_probe, + .remove = usb_eth_remove, .ops = &usb_eth_ops, .priv_auto = sizeof(struct ether_priv), .plat_auto = sizeof(struct eth_pdata),
On 1/20/23 18:15, Niel Fourie wrote:
[...]
Same question as in V1 below.
+static int _usb_eth_start(struct ether_priv *priv) +{
unsigned long timeout = USB_CONNECT_TIMEOUT;
struct eth_dev *dev = &priv->ethdev;
unsigned long ts;
if (!dev->gadget)
return -1;dev->network_started = 0;
Will this work on systems which already have netconsole active ? I think this would break the netconsole connection, since the network would be reinitialized, won't it ?
I would expect this assignment to be in _init and _stop , not in _start callback.
But I wonder whether the current ethernet uclass interface running code does not make the entire network_started mechanism obsolete. See the patch which you referenced previously in related patch:
fa795f45254 ("net: eth-uclass: avoid running start() twice without stop()")
Hi Marek
On 20/01/2023 19:42, Marek Vasut wrote:
On 1/20/23 18:15, Niel Fourie wrote:
[...]
Same question as in V1 below.
+static int _usb_eth_start(struct ether_priv *priv) +{ + unsigned long timeout = USB_CONNECT_TIMEOUT; + struct eth_dev *dev = &priv->ethdev; + unsigned long ts; + if (!dev->gadget) + return -1;
+ dev->network_started = 0;
Will this work on systems which already have netconsole active ? I think this would break the netconsole connection, since the network would be reinitialized, won't it ?
Good question, sorry for being unclear in my previous email. The upper layers appear to do a good job of taking the connection down and then bringing it up again when the network_started calls remain where they are. Here is an example run (copied from the serial console with console multiplexing enabled), where the last two dhcp calls were happily issued over the Netconsole:
u-boot=> bind /soc@0/usb@32f10100/usb@38100000 dwc3-generic-peripheral u-boot=> bind /soc@0/usb@32f10100/usb@38100000 usb_ether u-boot=> setenv autoload no u-boot=> setenv ethact eth2 u-boot=> setenv nc 'setenv stdout serial,nc;setenv stdin serial,nc' u-boot=> setenv ncip 10.42.0.1 u-boot=> dhcp using dwc3-gadget, OUT ep2out IN ep1in STATUS ep3in MAC de:ad:be:ef:00:01 HOST MAC de:ad:be:ef:00:00 high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet ep1in is already disabled ep2out is already disabled USB network up! BOOTP broadcast 1 DHCP client bound to address 10.42.0.65 (193 ms) u-boot=> run nc u-boot=> high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet ep1in is already disabled ep2out is already disabled USB network up!
u-boot=> dhcp BOOTP broadcast 1 DHCP client bound to address 10.42.0.65 (153 ms) u-boot=> high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet ep1in is already disabled ep2out is already disabled USB network up!
u-boot=> dhcp BOOTP broadcast 1 DHCP client bound to address 10.42.0.65 (144 ms) u-boot=> Stuck? Emptying request_list... head=bdf617c8, next=bdf64c88; high speed config #1: 2 mA, Ethernet Gadget, using CDC Ethernet ep1in is already disabled ep2out is already disabled USB network up!
u-boot=>
The "Stuck? Emptying request_list... head=bdf617c8, next=bdf64c88;" message comes from the dwc3 workaround I mentioned in the previous email. I will share it later as RFC, because I am a bit stumped at how the list_head ends up not being in the list when Netconsole is enabled.
I would expect this assignment to be in _init and _stop , not in _start callback.
I tried moving the dev->network_started as you requested, and it caused the driver to hang, on (say) the second "dhcp" command, even without Netconsole enabled. Unfortunately GDB was not particularly helpful in getting a backtrace for identifying exactly where/why.
But I wonder whether the current ethernet uclass interface running code does not make the entire network_started mechanism obsolete. See the patch which you referenced previously in related patch:
fa795f45254 ("net: eth-uclass: avoid running start() twice without stop()")
From what I could gather from my testing (shown above) it appears to working as expected, but I admit I am not sure how/why. The dev->network_started variable appears to be specific to the ether.c file, and does not directly interact with the other layers, like eth-uclass.
I wish I could give you a better explanation...
Best regards, Niel Fourie
Niel Fourie lusus@denx.de writes:
Split out _usb_eth_start() from _usb_eth_init() and usb_eth_stop() from _usb_eth_halt(). Now _usb_eth_init() only initialises and registers the gadget device, which _usb_eth_halt() reverses, and together are used for probing and removing the device. The _usb_eth_start() and _usb_eth_stop() functions connect and disconnect the gadget as expected by the start()/stop() callbacks.
Previously the gadget device was probed on every start() and removed on every stop(), which is inconsistent with other DM_ETH drivers.
By suggestion from Marek, I was testing this patch and discovered that it broke fastboot over USB support. With this patch applied on top of v2022.10, I'm seeing:
=> fastboot 0 couldn't find an available UDC g_dnl_register: failed!, error: -19
Kevin
Hi Kevin,
On 25/01/2023 23:50, Kevin Hilman wrote:
Niel Fourie lusus@denx.de writes:
Split out _usb_eth_start() from _usb_eth_init() and usb_eth_stop() from _usb_eth_halt(). Now _usb_eth_init() only initialises and registers the gadget device, which _usb_eth_halt() reverses, and together are used for probing and removing the device. The _usb_eth_start() and _usb_eth_stop() functions connect and disconnect the gadget as expected by the start()/stop() callbacks.
Previously the gadget device was probed on every start() and removed on every stop(), which is inconsistent with other DM_ETH drivers.
By suggestion from Marek, I was testing this patch and discovered that it broke fastboot over USB support. With this patch applied on top of v2022.10, I'm seeing:
=> fastboot 0 couldn't find an available UDC g_dnl_register: failed!, error: -19
Kevin
Thank you very much! That is another use case that I have not thought about and I will look into it. Unfortunately the side effects of the patch is not trivial, so I highly appreciate the feedback.
Best regards, Niel Fourie
participants (3)
-
Kevin Hilman -
Marek Vasut -
Niel Fourie