On Sun, Mar 1, 2015 at 12:07 PM, Simon Glass sjg@chromium.org wrote:
Hi Joe,
On 24 February 2015 at 17:02, Joe Hershberger joe.hershberger@ni.com
wrote:
Allow network devices to be referred to as "eth0" instead of "eth@12345678" when specified in ethact.
Add tests to verify this behavior.
Signed-off-by: Joe Hershberger joe.hershberger@ni.com
Reviewed-by: Simon Glass sjg@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@10002000", getenv("ethact"));setenv("ethact", "eth1");ut_assertok(NetLoop(PING));ut_asserteq_str("eth@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@10002000", getenv("ethact"));setenv("ethact", "eth5");ut_assertok(NetLoop(PING));ut_asserteq_str("eth@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@5"; testfdt8 = "/a-test";
eth0 = "/eth@10002000";eth5 = ð_5; }; uart0: serial {@@ -155,7 +157,7 @@ fake-host-hwaddr = <0x00 0x00 0x66 0x44 0x22 0x00>; };
eth@10003000 {
eth_5: eth@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@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot