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

[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):

-----------------------------[ cut here ]-----------------------------
#include <stdbool.h>
#include <stdio.h>
#include <string.h>

/* Returns false if 'prefix' is a not empty prefix of 'string'.
 */
bool matches(const char *prefix, const char *string)
{
	if (!*prefix)
		return true;
	while (*string && *prefix == *string) {
		prefix++;
		string++;
	}

	return !!*prefix;
}

int main(void)
{
	char *str1 = "eth";
	char *str2 = "ethernet";

	printf("strcmp returns %d\n", strcmp(str1, str2));
	printf("matches returns %d\n", matches(str2, str1));
	printf("reverse matches returns %d\n", matches(str1, str2));

	return 0;
}
-----------------------------[ cut here ]-----------------------------

strcmp returns -101
matches returns 0

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 would not, under any circumstance, break compatibility with "eth"
alias names.