bind/unbind command not working for usb_ether

[Heiko Schocher]

Hello Tim, Patrice,

On 15.02.22 19:28, Tim Harvey wrote:
> On Tue, Feb 15, 2022 at 9:36 AM Patrice CHOTARD
> <patrice.chotard at foss.st.com> wrote:
>>
>> Hi Tim
>>
>> On 2/15/22 17:09, Tim Harvey wrote:
>>> On Tue, Feb 15, 2022 at 5:29 AM Patrice CHOTARD
>>> <patrice.chotard at foss.st.com> wrote:
>>>>
>>>> Hi Tim
>>>>
>>>> On 2/10/22 18:04, Tim Harvey wrote:
>>>>> Greetings,
>>>>>
>>>>> I'm trying to understand how to use the U-Boot bind command to bind
>>>>> the usb_ether driver to the usb class to register a USB ethernet
>>>>> gadget network device as referenced in:
>>>>> commit 02291d83fdaaf ("doc: add bind/unbind command documentation")
>>>>> commit 49c752c93a78 ("cmd: Add bind/unbind commands to bind a device
>>>>> to a driver from the command line")
>>>>>
>>>>
>>>> For example, i made some trial on STM32MP1 platform:
>>>>
>>>> At boot, we got :
>>>>
>>>> STM32MP> dm tree
>>>>  Class     Index  Probed  Driver                Name
>>>> -----------------------------------------------------------
>>>>  root          0  [ + ]   root_driver           root_driver
>>>>  firmware      0  [   ]   psci                  |-- psci
>>>>  sysreset      0  [   ]   psci-sysreset         |   `-- psci-sysreset
>>>>  .....
>>>>  blk           0  [ + ]   mmc_blk               |   |   `-- mmc at 58005000.blk
>>>>  ethernet      0  [ + ]   eth_eqos              |   |-- ethernet at 5800a000
>>>>  eth_phy_ge    0  [ + ]   eth_phy_generic_drv   |   |   `-- ethernet-phy at 0
>>>>  usb           0  [   ]   ehci_generic          |   |-- usb at 5800d000
>>>>  video         0  [   ]   stm32_display         |   |-- display-controller at 5a001000
>>>>  .....
>>>>
>>>>
>>>> As you can see, there is already an ethernet interface used.
>>>> We unbind the ethernet interface before binding the usb_ether gadget to the usb class.
>>>> First unbind the generic ethernet phy (eth_phy_generic_drv) and the ethernet driver
>>>> (eth_eqos).
>>>>
>>>>
>>>> STM32MP> unbind eth_phy_generic 0
>>>> STM32MP> unbind ethernet 0
>>>> STM32MP> dm tree
>>>>  Class     Index  Probed  Driver                Name
>>>> -----------------------------------------------------------
>>>>  root          0  [ + ]   root_driver           root_driver
>>>>  firmware      0  [   ]   psci                  |-- psci
>>>>  sysreset      0  [   ]   psci-sysreset         |   `-- psci-sysreset
>>>> ....
>>>>  blk           0  [ + ]   mmc_blk               |   |   `-- mmc at 58005000.blk
>>>>  usb           0  [   ]   ehci_generic          |   |-- usb at 5800d000
>>>>  video         0  [   ]   stm32_display         |   |-- display-controller at 5a001000
>>>> ....
>>>>
>>>> Ethernet and phy driver are both unbinded.
>>>> Now we can bind the usb_eher to the usb class
>>>>
>>>> STM32MP> bind usb 0 usb_ether
>>>> STM32MP> dm tree
>>>>  Class     Index  Probed  Driver                Name
>>>> -----------------------------------------------------------
>>>>  root          0  [ + ]   root_driver           root_driver
>>>>  firmware      0  [   ]   psci                  |-- psci
>>>>  sysreset      0  [   ]   psci-sysreset         |   `-- psci-sysreset
>>>> ....
>>>>  blk           0  [ + ]   mmc_blk               |   |   `-- mmc at 58005000.blk
>>>>  usb           0  [   ]   ehci_generic          |   |-- usb at 5800d000
>>>>  ethernet      0  [   ]   usb_ether             |   |   `-- usb_ether
>>>>  video         0  [   ]   stm32_display         |   |-- display-controller at 5a001000
>>>> ....
>>>>
>>>> usb_ether is now binded.
>>>> As example, if you can then use some ethernet command as dhcp or ping :
>>>>
>>>> STM32MP> dhcp
>>>> using dwc2-udc, OUT ep2out-bulk IN ep1in-bulk STATUS ep3in-int
>>>> MAC de:ad:be:ef:00:01
>>>> HOST MAC de:ad:be:ef:00:00
>>>> RNDIS ready
>>>> high speed config #2: 2 mA, Ethernet Gadget, using RNDIS
>>>> USB RNDIS network up!
>>>> BOOTP broadcast 1
>>>>
>>>>> I have enabled:
>>>>> CONFIG_DM_USB=y
>>>>> CONFIG_USB_GADGET=y
>>>>> CONFIG_USB_ETHER=y
>>>>>
>>>> In my case i enabled also CONFIG_USB_ETH_RNDIS=y
>>>>
>>>
>>> Patrice,
>>>
>>> In my case when I try to bind to usb_ether the device can not be found
>>> (as it is never registered in the first place):
>>> Ventana > unbind ethernet 0
>>> Ventana > bind usb 0 usb_ether
>>> Cannot find device 0 of class usb
>>
>> weird, because below, in the dm tree output, we can see :
>>
>>>  usb           0  [   ]   ehci_mx6              |   |   |-- usb at 2184000
>>>  usb           1  [   ]   ehci_mx6              |   |   |-- usb at 2184200
>>
>> so it should find a usb class device .....
>>
> 
> Patrice,
> 
> I added some debugging and found that 'bind usb 0 usb_ether' does the following:
> bind_by_class_index(class="usb" index=0 drv="usb_ether")
>   uclass_get_by_name(name="usb")
>     uclass_get_by_name_len(name="usb" len=3)
> ^^^ does a strncmp with name=usb and len=3 so it matches the first
> driver with a class name starting with 'usb' and in this case matches
> usb_mass_storage instead of 'usb'

Ah!

> Simon, this behavior comes from commit 4b030177b660 ("dm: core: Allow
> finding children / uclasses by partial name"). Why would
> device_find_child_by_name() want to use a substring match? I suppose
> if there is a valid reason for this such as your commit logs describe
> then those functions should directly call uclass_get_by_name_len() and
>  uclass_get_by_name() should be doing a full name match correct?
> 
> Patrice, if I make uclass_get_by_name match the full string with the
> below patch then 'bind usb 0 usb_ether' indeed works as planned:
> 
> diff --git a/drivers/core/uclass.c b/drivers/core/uclass.c
> index 2578803b7a4d..33dbca544574 100644
> --- a/drivers/core/uclass.c
> +++ b/drivers/core/uclass.c
> @@ -196,7 +196,16 @@ enum uclass_id uclass_get_by_name_len(const char
> *name, int len)
> 
>  enum uclass_id uclass_get_by_name(const char *name)
>  {
> -       return uclass_get_by_name_len(name, strlen(name));
> +       int i;
> +
> +       for (i = 0; i < UCLASS_COUNT; i++) {
> +               struct uclass_driver *uc_drv = lists_uclass_lookup(i);
> +
> +               if (uc_drv && !strcmp(uc_drv->name, name))
> +                       return i;
> +       }
> +
> +       return UCLASS_INVALID;
>  }
> 
>  int dev_get_uclass_index(struct udevice *dev, struct uclass **ucp)

I have to test, if this works for me too, but I have to find some time ...

> I don't see the need to unbind 'ethernet' as you can select the active
> network device with 'ethact':

Yep, no need for unbinding...

> Ventana > net list
> eth0 : ethernet at 2188000 00:d0:12:f3:f2:f5 active
> Ventana > bind usb 0 usb_ether
> Ventana > net list
> eth0 : ethernet at 2188000 00:d0:12:f3:f2:f5 active
> eth1 : usb_ether 00:00:00:00:00:00
> Ventana > setenv ethact eth1
> Ventana > ping 192.168.1.146
> ^^^ but now we run into the issue Heiko discovered where the
> usb_gadget_register_driver() call from ether.c ends up removing usb
> and usb_ether and without his hack to return from device-remove if
> device is usb_ether we hang:
> 
> @@ -201,12 +202,16 @@ int device_remove(struct udevice *dev, uint flags)
>         const struct driver *drv;
>         int ret;
> 
> +printf("%s %s\n", __func__, dev->name);
>         if (!dev)
>                 return -EINVAL;
> 
>         if (!(dev_get_flags(dev) & DM_FLAG_ACTIVATED))
>                 return 0;
> 
> +       if (!strncmp(dev->name, "usb_ether", 8))
> +               return 0;
> +
>         /*
>          * If the child returns EKEYREJECTED, continue. It just means that it
>          * didn't match the flags.

Yes, we have to debug here deeper...

Thanks for digging in here!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de