U-Boot crashes when doing a 'ping' with the following test scenario:

- All ethernet devices are not probed - "ethaddr" for all ethernet devices are not set - "ethact" is set to a valid ethernet device name

Add a new test case 'dm_test_eth_act' to hit such scenario.

Signed-off-by: Bin Meng bmeng.cn@gmail.com

---

Changes in v2: - New patch to add a new test case against dm eth codes for NULL pointer access

test/dm/eth.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)

diff --git a/test/dm/eth.c b/test/dm/eth.c index fcfb3e1..6288ae2 100644 --- a/test/dm/eth.c +++ b/test/dm/eth.c @@ -13,11 +13,15 @@ #include <malloc.h> #include <net.h> #include <dm/test.h> +#include <dm/device-internal.h> +#include <dm/uclass-internal.h> #include <asm/eth.h> #include <test/ut.h>

DECLARE_GLOBAL_DATA_PTR;

+#define DM_TEST_ETH_NUM 4 + static int dm_test_eth(struct unit_test_state *uts) { net_ping_ip = string_to_ip("1.1.2.2"); @@ -82,6 +86,66 @@ static int dm_test_eth_prime(struct unit_test_state *uts) } DM_TEST(dm_test_eth_prime, DM_TESTF_SCAN_FDT);

+/** + * This test case is trying to test the following scenario: + * - All ethernet devices are not probed + * - "ethaddr" for all ethernet devices are not set + * - "ethact" is set to a valid ethernet device name + * + * With Sandbox default test configuration, all ethernet devices are + * probed after power-up, so we have to manually create such scenario: + * - Remove all ethernet devices + * - Remove all "ethaddr" environment variables + * - Set "ethact" to the first ethernet device + * + * Do a ping test to see if anything goes wrong. + */ +static int dm_test_eth_act(struct unit_test_state *uts) +{ + struct udevice *dev[DM_TEST_ETH_NUM]; + const char *ethname[DM_TEST_ETH_NUM] = {"eth@10002000", "eth@10003000", + "sbe5", "eth@10004000"}; + const char *addrname[DM_TEST_ETH_NUM] = {"ethaddr", "eth5addr", + "eth3addr", "eth1addr"}; + char ethaddr[DM_TEST_ETH_NUM][18]; + int i; + + net_ping_ip = string_to_ip("1.1.2.2"); + + /* Prepare the test scenario */ + for (i = 0; i < DM_TEST_ETH_NUM; i++) { + ut_assertok(uclass_find_device_by_name(UCLASS_ETH, + ethname[i], &dev[i])); + ut_assertok(device_remove(dev[i])); + + /* Invalidate MAC address */ + strcpy(ethaddr[i], getenv(addrname[i])); + /* Must disable access protection for ethaddr before clearing */ + setenv(".flags", addrname[i]); + setenv(addrname[i], NULL); + } + + /* Set ethact to "eth@10002000" */ + setenv("ethact", ethname[0]); + + /* Segment fault might happen if something is wrong */ + ut_asserteq(-ENODEV, net_loop(PING)); + + for (i = 0; i < DM_TEST_ETH_NUM; i++) { + /* Restore the env */ + setenv(".flags", addrname[i]); + setenv(addrname[i], ethaddr[i]); + + /* Probe the device again */ + ut_assertok(device_probe(dev[i])); + } + setenv(".flags", NULL); + setenv("ethact", NULL); + + return 0; +} +DM_TEST(dm_test_eth_act, DM_TESTF_SCAN_FDT); + /* The asserts include a return on fail; cleanup in the caller */ static int _dm_test_eth_rotate1(struct unit_test_state *uts) {

eth_get_dev() can return NULL which means device_probe() fails for that ethernet device. Add return value check in various places or U-Boot will crash due to NULL pointer access.

With this commit, 'dm_test_eth_act' test case passes.

Signed-off-by: Bin Meng bmeng.cn@gmail.com

---

Changes in v2: - Change check logic in eth_init() (do not use break) - Add device_probe() return value check

net/eth.c | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/net/eth.c b/net/eth.c index 35f9641..0040836 100644 --- a/net/eth.c +++ b/net/eth.c @@ -179,8 +179,12 @@ struct udevice *eth_get_dev(void) */ static void eth_set_dev(struct udevice *dev) { - if (dev && !device_active(dev)) + if (dev && !device_active(dev)) { eth_errno = device_probe(dev); + if (eth_errno) + dev = NULL; + } + eth_get_uclass_priv()->current = dev; }

@@ -213,10 +217,9 @@ struct udevice *eth_get_dev_by_name(const char *devname) * match an alias or it will match a literal name and we'll pick * up the error when we try to probe again in eth_set_dev(). */ - device_probe(it); - /* - * Check for the name or the sequence number to match - */ + if (device_probe(it)) + continue; + /* Check for the name or the sequence number to match */ if (strcmp(it->name, devname) == 0 || (endp > startp && it->seq == seq)) return it; @@ -346,23 +349,27 @@ int eth_init(void)

old_current = current; do { - debug("Trying %s\n", current->name); - - if (device_active(current)) { - ret = eth_get_ops(current)->start(current); - if (ret >= 0) { - struct eth_device_priv *priv = - current->uclass_priv; - - priv->state = ETH_STATE_ACTIVE; - return 0; + if (current) { + debug("Trying %s\n", current->name); + + if (device_active(current)) { + ret = eth_get_ops(current)->start(current); + if (ret >= 0) { + struct eth_device_priv *priv = + current->uclass_priv; + + priv->state = ETH_STATE_ACTIVE; + return 0; + } + } else { + ret = eth_errno; } + + debug("FAIL\n"); } else { - ret = eth_errno; + debug("PROBE FAIL\n"); }

- debug("FAIL\n"); - /* * If ethrotate is enabled, this will change "current", * otherwise we will drop out of this while loop immediately

Hi Bin,

On Thu, Sep 10, 2015 at 4:29 AM, Bin Meng bmeng.cn@gmail.com wrote:

When removing an Ethernet device, the MAC address saved in the dev's platdata should be cleared.

Why is this important to do? Test case?

Signed-off-by: Bin Meng bmeng.cn@gmail.com

---

Changes in v2:

• New patch to clear MAC address in eth_pre_remove()

net/eth.c | 5 +++++ 1 file changed, 5 insertions(+)

diff --git a/net/eth.c b/net/eth.c index 26520d3..35f9641 100644 --- a/net/eth.c +++ b/net/eth.c @@ -564,8 +564,13 @@ static int eth_post_probe(struct udevice *dev)

static int eth_pre_remove(struct udevice *dev) {

•   struct eth_pdata *pdata = dev->platdata;
  

•   eth_get_ops(dev)->stop(dev);
  
  

•   /* clear the MAC address */
  

•   memset(pdata->enetaddr, 0, 6);
  

•   return 0;
  

}

-- 1.8.2.1

---

U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot