| Message ID | 20221212152945.1870385-2-lusus@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | usb: gadget: ether: split start/stop from init/halt | expand |
On 12/12/22 16:29, Niel Fourie wrote: > 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. For non-DM gadget drivers the old behaviour has been > retained. Does this mean the udevice pointer and associated private date are retained during the entire operation of the USB gadget , i.e. even between stop/start cycles ?
Hi Marek, On 12/12/2022 17:46, Marek Vasut wrote: > On 12/12/22 16:29, Niel Fourie wrote: >> 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. For non-DM gadget drivers the old behaviour has been >> retained. > > Does this mean the udevice pointer and associated private date are > retained during the entire operation of the USB gadget , i.e. even > between stop/start cycles ? In the DM_ETH case, yes. The drivers/devices remain registered the whole time between _usb_eth_init() and _usb_eth_halt(), and the need for revalidating the private data pointer falls away as was done in my previous patch. I tested this on imx8mp with the dwc3 gadget driver, and the data structures, specifically the private data, remained intact from probe() until remove(). I also tested that probing again after removal works as expected. I tested the non-DM_ETH case on an iMX6 Phytec Mira (pcm058) which uses the ci_udc.c driver, which appears to not support DM_ETH yet. In that case the gadget drivers get registered/unregistered in usb_eth_init()/usb_eth_halt(). The legacy non-DM_ETH implementation uses init()/halt() similarly to start()/stop() in the DM_ETH implementation, but had no further equivalent for probe()/remove() in struct eth_device. When the Ethernet gadget was ported to DM_ETH, this difference most likely lead to the existing implementation. I unfortunately did not have other devices on hand to test against, so I am hoping that there are no other surprises. Best regards, Niel Fourie
On 12/13/22 12:01, Niel Fourie wrote: > Hi Marek, Hi, > On 12/12/2022 17:46, Marek Vasut wrote: >> On 12/12/22 16:29, Niel Fourie wrote: >>> 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. For non-DM gadget drivers the old behaviour has been >>> retained. >> >> Does this mean the udevice pointer and associated private date are >> retained during the entire operation of the USB gadget , i.e. even >> between stop/start cycles ? > > In the DM_ETH case, yes. The drivers/devices remain registered the whole > time between _usb_eth_init() and _usb_eth_halt(), and the need for > revalidating the private data pointer falls away as was done in my > previous patch. Perfect. > I tested this on imx8mp with the dwc3 gadget driver, and the data > structures, specifically the private data, remained intact from probe() > until remove(). I also tested that probing again after removal works as > expected. Excellent. [...] Should "[PATCH v2] net: eth-uclass: revalidate priv after stop() in eth_halt()" be dropped ? It seems this patch fully replaces it.
Hi Marek, On 2022-12-15 06:58, Marek Vasut wrote: > On 12/13/22 12:01, Niel Fourie wrote: >> Hi Marek, > > Hi, > >> On 12/12/2022 17:46, Marek Vasut wrote: >>> On 12/12/22 16:29, Niel Fourie wrote: >>>> 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. For non-DM gadget drivers the old behaviour has been >>>> retained. >>> >>> Does this mean the udevice pointer and associated private date are >>> retained during the entire operation of the USB gadget , i.e. even >>> between stop/start cycles ? >> >> In the DM_ETH case, yes. The drivers/devices remain registered the >> whole time between _usb_eth_init() and _usb_eth_halt(), and the need >> for revalidating the private data pointer falls away as was done in my >> previous patch. > > Perfect. > >> I tested this on imx8mp with the dwc3 gadget driver, and the data >> structures, specifically the private data, remained intact from >> probe() until remove(). I also tested that probing again after removal >> works as expected. > > Excellent. > > [...] > > Should "[PATCH v2] net: eth-uclass: revalidate priv after stop() in > eth_halt()" be dropped ? It seems this patch fully replaces it. That old patch only exists for in case there were showstopper issues with the new patch which I missed, or as a stopgap if major changes were needed first. I did my best to test the new patch on the hardware I have, but if any other gadget drivers were to misbehave, I would not know about it, for example. But if you are happy with this new patch, that old patch could gladly be dropped. Best regards, Niel Fourie
On 12/12/22 16:29, Niel Fourie wrote: [...] > +static int _usb_eth_start(struct ether_priv *priv) > +{ > + unsigned long ts; > + unsigned long timeout = USB_CONNECT_TIMEOUT; > + struct eth_dev *dev = &priv->ethdev; > + > + 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()") > 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; [...] > @@ -2493,9 +2509,13 @@ static void _usb_eth_halt(struct ether_priv *priv) > #ifndef CONFIG_DM_ETH > static int usb_eth_init(struct eth_device *netdev, struct bd_info *bd) > { > + int ret; > struct ether_priv *priv = (struct ether_priv *)netdev->priv; Keep the vars sorted as reverse xmas tree please, i.e. int ret goes below the struct . [...]
On 12/16/22 17:35, lusus@denx.de wrote: > Hi Marek, Hi, [...] >> Should "[PATCH v2] net: eth-uclass: revalidate priv after stop() in >> eth_halt()" be dropped ? It seems this patch fully replaces it. > > That old patch only exists for in case there were showstopper issues > with the new patch which I missed, or as a stopgap if major changes were > needed first. I did my best to test the new patch on the hardware I > have, but if any other gadget drivers were to misbehave, I would not > know about it, for example. But if you are happy with this new patch, > that old patch could gladly be dropped. The new patch is by far preferable. If you can sort out the network_started comment, I would like to pick this via usb/next. Thanks
Hi Marek On 18/12/2022 02:51, Marek Vasut wrote: > On 12/16/22 17:35, lusus@denx.de wrote: >> Hi Marek, > > Hi, > > [...] > >>> Should "[PATCH v2] net: eth-uclass: revalidate priv after stop() in >>> eth_halt()" be dropped ? It seems this patch fully replaces it. >> >> That old patch only exists for in case there were showstopper issues >> with the new patch which I missed, or as a stopgap if major changes >> were needed first. I did my best to test the new patch on the hardware >> I have, but if any other gadget drivers were to misbehave, I would not >> know about it, for example. But if you are happy with this new patch, >> that old patch could gladly be dropped. > > The new patch is by far preferable. If you can sort out the > network_started comment, I would like to pick this via usb/next. I did some more testing and I found that keeping network_started as it is works best, so I did not change it for v2 of this patch. Unfortunately during local testing (on imx8mp), I uncovered an issue in the dwc3 gadget driver causing it to hang. I have a workaround, but I am still looking into fixing the root cause, and that would have to be fixed in a separate patch series. Similar issues might also still crop up with other gadget drivers. Best regards, Niel Fourie
On 1/20/23 18:26, Niel Fourie wrote: > Hi Marek Hi, > On 18/12/2022 02:51, Marek Vasut wrote: >> On 12/16/22 17:35, lusus@denx.de wrote: >>> Hi Marek, >> >> Hi, >> >> [...] >> >>>> Should "[PATCH v2] net: eth-uclass: revalidate priv after stop() in >>>> eth_halt()" be dropped ? It seems this patch fully replaces it. >>> >>> That old patch only exists for in case there were showstopper issues >>> with the new patch which I missed, or as a stopgap if major changes >>> were needed first. I did my best to test the new patch on the >>> hardware I have, but if any other gadget drivers were to misbehave, I >>> would not know about it, for example. But if you are happy with this >>> new patch, that old patch could gladly be dropped. >> >> The new patch is by far preferable. If you can sort out the >> network_started comment, I would like to pick this via usb/next. > > I did some more testing and I found that keeping network_started as it > is works best, so I did not change it for v2 of this patch. This doesn't answer the question whether netconsole connection gets broken or not by this change, it think the netconsole connection would get broken and that's not good. > Unfortunately during local testing (on imx8mp), I uncovered an issue in > the dwc3 gadget driver causing it to hang. I have a workaround, but I am > still looking into fixing the root cause, and that would have to be > fixed in a separate patch series. You can send the workaround as [RFC][PATCH] so it wouldn't get included accidentally and possibly CC Peng Fan from NXP. > Similar issues might also still crop up with other gadget drivers. [...]
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c index 43aec7ffa70..a75b4eeb5bb 100644 --- a/drivers/usb/gadget/ether.c +++ b/drivers/usb/gadget/ether.c @@ -2305,15 +2305,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; @@ -2353,14 +2349,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 ts; + unsigned long timeout = USB_CONNECT_TIMEOUT; + struct eth_dev *dev = &priv->ethdev; + + 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; @@ -2378,6 +2386,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; } @@ -2457,11 +2466,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; @@ -2485,6 +2493,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); @@ -2493,9 +2509,13 @@ static void _usb_eth_halt(struct ether_priv *priv) #ifndef CONFIG_DM_ETH static int usb_eth_init(struct eth_device *netdev, struct bd_info *bd) { + int ret; struct ether_priv *priv = (struct ether_priv *)netdev->priv; - return _usb_eth_init(priv); + ret = _usb_eth_init(priv); + if (!ret) + ret = _usb_eth_start(priv); + return ret; } static int usb_eth_send(struct eth_device *netdev, void *packet, int length) @@ -2536,6 +2556,7 @@ void usb_eth_halt(struct eth_device *netdev) { struct ether_priv *priv = (struct ether_priv *)netdev->priv; + _usb_eth_stop(priv); _usb_eth_halt(priv); } @@ -2559,7 +2580,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) @@ -2609,7 +2630,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) @@ -2623,6 +2644,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; } @@ -2658,6 +2689,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),
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. For non-DM gadget drivers the old behaviour has been retained. 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> --- drivers/usb/gadget/ether.c | 58 +++++++++++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 13 deletions(-)