[U-Boot] [RFC PATCH v4 16/23] dm: eth: Add support for aliases

Simon Glass sjg at chromium.org
Sun Mar 1 19:07:30 CET 2015


Hi Joe,

On 24 February 2015 at 17:02, Joe Hershberger <joe.hershberger at ni.com> wrote:
> Allow network devices to be referred to as "eth0" instead of
> "eth at 12345678" when specified in ethact.
>
> Add tests to verify this behavior.
>
> Signed-off-by: Joe Hershberger <joe.hershberger at ni.com>
>

Reviewed-by: Simon Glass <sjg at chromium.org>

Again a few comments on error handling for follow-up.

> ---
>
> Changes in v4:
> -Use only the seq from DM to find aliases
>
> Changes in v3:
> -Added support for aliases
>
> Changes in v2: None
>
>  include/configs/sandbox.h |  2 +-
>  include/net.h             |  1 +
>  net/eth.c                 | 47 ++++++++++++++++++++++++++++++++++++++---------
>  test/dm/eth.c             | 24 ++++++++++++++++++++++++
>  test/dm/test.dts          |  4 +++-
>  5 files changed, 67 insertions(+), 11 deletions(-)
>
> diff --git a/include/configs/sandbox.h b/include/configs/sandbox.h
> index 9189f6a..caf9f5a 100644
> --- a/include/configs/sandbox.h
> +++ b/include/configs/sandbox.h
> @@ -174,7 +174,7 @@
>
>  #define SANDBOX_ETH_SETTINGS           "ethaddr=00:00:11:22:33:44\0" \
>                                         "eth1addr=00:00:11:22:33:45\0" \
> -                                       "eth2addr=00:00:11:22:33:46\0" \
> +                                       "eth5addr=00:00:11:22:33:46\0" \
>                                         "ipaddr=1.2.3.4\0"
>
>  #define CONFIG_EXTRA_ENV_SETTINGS      SANDBOX_SERIAL_SETTINGS \
> diff --git a/include/net.h b/include/net.h
> index 508c572..e9cb4a3 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -122,6 +122,7 @@ struct eth_ops {
>  #define eth_get_ops(dev) ((struct eth_ops *)(dev)->driver->ops)
>
>  struct udevice *eth_get_dev(void); /* get the current device */
> +struct udevice *eth_get_dev_by_name(const char *devname);

This needs a comment to describe what devname is exactly. I thought it
was a device name. Also it seems to requite a minimum length of 3
characters?

>  unsigned char *eth_get_ethaddr(void); /* get the current device MAC */
>  /* Used only when NetConsole is enabled */
>  int eth_init_state_only(void); /* Set active state */
> diff --git a/net/eth.c b/net/eth.c
> index 9c2dfb9..8b853e8 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -132,6 +132,36 @@ static void eth_set_dev(struct udevice *dev)
>         eth_get_uclass_priv()->current = dev;
>  }
>
> +/*
> + * Find the udevice that either has the name passed in as devname or has an
> + * alias named devname.
> + */
> +struct udevice *eth_get_dev_by_name(const char *devname)
> +{
> +       int seq;
> +       char *endp = NULL;
> +       const char *startp;
> +       struct udevice *it;
> +       struct uclass *uc;
> +
> +       startp = devname + strlen("eth");
> +       seq = simple_strtoul(startp, &endp, 10);
> +
> +       uclass_get(UCLASS_ETH, &uc);
> +       uclass_foreach_dev(it, uc) {
> +               /* We need the seq to be valid, so make sure it's probed */
> +               device_probe(it);

Error check. I think this function is should return an error.

> +               /*
> +                * Check for the name or the sequence number to match
> +                */
> +               if (strcmp(it->name, devname) == 0 ||
> +                   (endp > startp && it->seq == seq))
> +                       return it;
> +       }
> +
> +       return NULL;
> +}
> +
>  unsigned char *eth_get_ethaddr(void)
>  {
>         struct eth_pdata *pdata;
> @@ -405,6 +435,7 @@ UCLASS_DRIVER(eth) = {
>         .pre_remove     = eth_pre_remove,
>         .priv_auto_alloc_size = sizeof(struct eth_uclass_priv),
>         .per_device_auto_alloc_size = sizeof(struct eth_device_priv),
> +       .flags          = DM_UC_FLAG_SEQ_ALIAS,
>  };
>  #endif
>
> @@ -437,6 +468,11 @@ static void eth_set_current_to_next(void)
>         eth_current = eth_current->next;
>  }
>
> +static void eth_set_dev(struct eth_device *dev)
> +{
> +       eth_current = dev;
> +}
> +
>  struct eth_device *eth_get_dev_by_name(const char *devname)
>  {
>         struct eth_device *dev, *target_dev;
> @@ -853,7 +889,6 @@ void eth_set_current(void)
>  {
>         static char *act;
>         static int  env_changed_id;
> -       void *old_current;
>         int     env_id;
>
>         env_id = get_env_id();
> @@ -861,14 +896,8 @@ void eth_set_current(void)
>                 act = getenv("ethact");
>                 env_changed_id = env_id;
>         }
> -       if (act != NULL) {
> -               old_current = eth_get_dev();
> -               do {
> -                       if (strcmp(eth_get_name(), act) == 0)
> -                               return;
> -                       eth_set_current_to_next();
> -               } while (old_current != eth_get_dev());
> -       }
> +       if (act != NULL)
> +               eth_set_dev(eth_get_dev_by_name(act));

Again I think the error handling here is dodgy. You may have a device
which fails to probe but it will not be reported here.

>
>         eth_current_changed();
>  }
> diff --git a/test/dm/eth.c b/test/dm/eth.c
> index 04ccf49..5688b71 100644
> --- a/test/dm/eth.c
> +++ b/test/dm/eth.c
> @@ -36,3 +36,27 @@ static int dm_test_eth(struct dm_test_state *dms)
>         return 0;
>  }
>  DM_TEST(dm_test_eth, DM_TESTF_SCAN_FDT);
> +
> +static int dm_test_eth_alias(struct dm_test_state *dms)
> +{
> +       NetPingIP = string_to_ip("1.1.2.2");
> +       setenv("ethact", "eth0");
> +       ut_assertok(NetLoop(PING));
> +       ut_asserteq_str("eth at 10002000", getenv("ethact"));
> +
> +       setenv("ethact", "eth1");
> +       ut_assertok(NetLoop(PING));
> +       ut_asserteq_str("eth at 10004000", getenv("ethact"));
> +
> +       /* Expected to fail since eth2 is not defined in the device tree */
> +       setenv("ethact", "eth2");
> +       ut_assertok(NetLoop(PING));
> +       ut_asserteq_str("eth at 10002000", getenv("ethact"));
> +
> +       setenv("ethact", "eth5");
> +       ut_assertok(NetLoop(PING));
> +       ut_asserteq_str("eth at 10003000", getenv("ethact"));

Looks good to me.

> +
> +       return 0;
> +}
> +DM_TEST(dm_test_eth_alias, DM_TESTF_SCAN_FDT);
> diff --git a/test/dm/test.dts b/test/dm/test.dts
> index 2f68cdf..bc2409d 100644
> --- a/test/dm/test.dts
> +++ b/test/dm/test.dts
> @@ -17,6 +17,8 @@
>                 testfdt3 = "/b-test";
>                 testfdt5 = "/some-bus/c-test at 5";
>                 testfdt8 = "/a-test";
> +               eth0 = "/eth at 10002000";
> +               eth5 = &eth_5;
>         };
>
>         uart0: serial {
> @@ -155,7 +157,7 @@
>                 fake-host-hwaddr = <0x00 0x00 0x66 0x44 0x22 0x00>;
>         };
>
> -       eth at 10003000 {
> +       eth_5: eth at 10003000 {
>                 compatible = "sandbox,eth";
>                 reg = <0x10003000 0x1000>;
>                 fake-host-hwaddr = <0x00 0x00 0x66 0x44 0x22 0x11>;
> --
> 1.7.11.5
>

Regards,
Simon


More information about the U-Boot mailing list