[PATCH v2] usb: gadget: ether: split start/stop from init/halt

[PATCH v2] usb: gadget: ether: split start/stop from init/halt

[Niel Fourie]

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),
-- 
2.39.0

Re: [PATCH v2] usb: gadget: ether: split start/stop from init/halt

[Marek Vasut]

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()")

Re: [PATCH v2] usb: gadget: ether: split start/stop from init/halt

[Niel Fourie]

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

-- 
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-21 Fax: +49-8142-66989-80  Email: lusus@denx.de

Re: [PATCH v2] usb: gadget: ether: split start/stop from init/halt

[Kevin Hilman]

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

Re: [PATCH v2] usb: gadget: ether: split start/stop from init/halt

[Niel Fourie]

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

-- 
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-21 Fax: +49-8142-66989-80  Email: lusus@denx.de