[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 at 0/usb at 32f10100/usb at 38100000 dwc3-generic-peripheral
u-boot=> bind /soc at 0/usb at 32f10100/usb at 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 at denx.de