[U-Boot] [RFC PATCH v4 16/23] dm: eth: Add support for aliases
Joe Hershberger
joe.hershberger at gmail.com
Sun Mar 1 23:04:58 CET 2015
On Sun, Mar 1, 2015 at 12:07 PM, Simon Glass <sjg at chromium.org> wrote:
>
> 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.
OK
> Also it seems to requite a minimum length of 3
> characters?
Good point. This is a bug. I should be checking the size first. It is not
an intended requirement.
> > 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.
This is simply searching. If a device annot be probed, why error out a
search for presumably a different device? I can look into adding a unit
test to validate this behavior.
> > + /*
> > + * 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.
I'll think about how to make errors be reported when you explicitly ask for
a device, but not when you are just scanning through them.
> >
> > 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 = ð_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
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
More information about the U-Boot
mailing list