[U-Boot] [RFC PATCH v4 16/23] dm: eth: Add support for aliases
Simon Glass
sjg at chromium.org
Mon Mar 2 03:23:58 CET 2015
Hi Joe,
On 1 March 2015 at 15:04, Joe Hershberger <joe.hershberger at gmail.com> wrote:
>
>
> 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.
Well normal error behaviour is to report the error to the upper layers
which may or may not stop.
But a failure to probe a device which should be there seems bad.
Anyway if you are wanting to not check these errors you should at
least add big comments in those places as to why. I'd hate for people
not to understand the rationale and just assume that errors don't
matter (because they don't understand the special cases here).
>
>> > + /*
>> > + * 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.
It's an interesting question, and fair enough - this is a design
decision. But please comment it.
[snip]
Regards,
Simon
More information about the U-Boot
mailing list