[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[Michael Walle]

Am 2021-02-24 19:14, schrieb Vladimir Oltean:
> On Wed, Feb 24, 2021 at 06:21:33PM +0100, Michael Walle wrote:
>> Am 2021-01-19 21:40, schrieb Tom Rini:
>> > On Tue, Jan 19, 2021 at 03:01:38PM -0500, Tom Rini wrote:
>> > > On Fri, Jan 08, 2021 at 10:53:05AM +0800, David Wu wrote:
>> > >
>> > > > dev_read_alias_seq() used uc_drv->name compared to alias
>> > > > stem string, Ethernet's alias stem uses "ethernet", which
>> > > > does not match the eth-uclass driver name "eth", can not
>> > > > get the correct index of ethernet alias namer. So it seems
>> > > > change uclass driver name to match the alias stem is a more
>> > > > reasonable way.
>> > > >
>> > > > Signed-off-by: David Wu <david.wu at rock-chips.com>
>> > > > Reviewed-by: Simon Glass <sjg at chromium.org>
>> > >
>> > > Applied to u-boot/master, thanks!
>> >
>> > I'm reverting this change as it breaks a number of tests that need to be
>> > updated to match on the new name.
>> 
>> David, are you planning to submit a new version? If I'm not
>> mistaken, the following changes should be enought to make the
>> tests pass again:
>> 
>> --- a/arch/sandbox/dts/test.dts
>> +++ b/arch/sandbox/dts/test.dts
>> @@ -14,9 +14,9 @@
>> 
>>         aliases {
>>                 console = &uart0;
>> -               eth0 = "/eth at 10002000";
>> -               eth3 = &eth_3;
>> -               eth5 = &eth_5;
>> +               ethernet0 = "/eth at 10002000";
>> +               ethernet3 = &eth_3;
>> +               ethernet5 = &eth_5;
>>                 gpio1 = &gpio_a;
>>                 gpio2 = &gpio_b;
>>                 gpio3 = &gpio_c;
>> 
>> In commit cc32fd911aa9 ("arm: dts: ls1028a: Add Ethernet switch
>> node and dependencies") there was recently an additon to a board
>> which actually uses these aliases. So this patch should change
>> them too.
>> 
>> I was actually under the impression that the alias was
>> "ethernetN" and added them to my board, see
>> arch/arm/fsl-ls1028a-kontron*u-boot.dtsi. Seems like I wasn't
>> the first one which was mistaken:
>> 
>> $ grep "ethernet. =" arch/**/*u-boot.dtsi
>> arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi:		ethernet2 = 
>> &enetc2;
>> arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi:		ethernet3 = 
>> &enetc6;
>> arch/arm/dts/fsl-ls1028a-kontron-sl28-var1-u-boot.dtsi:		ethernet0 =
>> &enetc1;
>> arch/arm/dts/fsl-ls1028a-kontron-sl28-var3-u-boot.dtsi:		ethernet0 =
>> &enetc0;
>> arch/arm/dts/fsl-ls1028a-kontron-sl28-var4-u-boot.dtsi:		ethernet0 =
>> &enetc0;
>> arch/arm/dts/fsl-ls1028a-kontron-sl28-var4-u-boot.dtsi:		ethernet1 =
>> &enetc1;
>> arch/arm/dts/k3-am654-base-board-u-boot.dtsi:		ethernet0 = 
>> &cpsw_port1;
>> arch/arm/dts/k3-j7200-common-proc-board-u-boot.dtsi:		ethernet0 =
>> &cpsw_port1;
>> arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi:		ethernet0 =
>> &cpsw_port1;
>> arch/arm/dts/stm32mp15xx-dhcom-u-boot.dtsi:		ethernet1 = &ksz8851;
>> 
>> So at the moment  I'm not sure if I should fix my dtsi files
>> to use the ethN aliases or if I should just wait because this
>> patch will make it into u-boot soon.
>> 
>> I could also pick up this patch, amend it and resubmit it
>> myself.
>> 
>> -michael
> 
> Sorry, not sure that I understand. We are talking about the following
> code path, right?
> 
> device_bind_common
> -> dev_read_alias_seq
>    -> of_alias_get_id
>       -> strcmp(app->stem, stem)
> 
> Why on earth is it called "stem" if strcmp is used?
> 
> I'm not sure if there's any C standard library for string prefix
> comparison, but at least I know iproute2 uses a custom function
> (copy-pasted below):

I'm not sure if it should really be just a prefix match but a full
string compare.

> Wasn't the intention of David's patch, in fact, to have the new uclass
> name also match on "eth" aliases? If I'm correct, doesn't this mean
> we'll have to replace the strcmp with an actual stem check?

I guess it was intended the other way around, to rename the "ethN"
aliases to "ethernetN". The latter are used way more in u-boot's device
trees and linux' device trees just use ethernetN aliases, though I'm
not sure where they are used (can't find any of_alias_get_id("ethernet")
in linux). So this makes sense, no?

> I would not, under any circumstance, break compatibility with "eth"
> alias names.

Assuming that ethN is not used expect in u-boot device trees, how would
we break backwards compatibility? Also it seems that the only users of
the ethN aliases are the sandbox and u-boot's DSA.

-michael