net: Improve error handling

Take a pass at plumbing errors through to the users of the network stack

Currently only the start() function errors will be returned from
NetLoop(). recv() tends not to have errors, so that is likely not worth
adding. send() certainly can return errors, but this patch does not
attempt to plumb them yet. halt() is not expected to error.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
Reviewed-by: Simon Glass <sjg@chromium.org>

diff --git a/include/net.h b/include/net.h
index 942fa4c929ecd84a6f4f300c0c3c28ead5eadeca..e7f28d7149741ee5a91c2df14e6ae5b2d6b50b94 100644 (file)
--- a/include/net.h
+++ b/include/net.h
@@ -543,7 +543,7 @@ int NetLoop(enum proto_t);
 void   NetStop(void);
 
 /* Load failed.         Start again. */
-void   NetStartAgain(void);
+int    NetStartAgain(void);
 
 /* Get size of the ethernet header when we send */
 int    NetEthHdrSize(void);
@@ -613,6 +613,7 @@ static inline void net_set_state(enum net_loop_state state)
 /* Transmit a packet */
 static inline void NetSendPacket(uchar *pkt, int len)
 {
+       /* Currently no way to return errors from eth_send() */
        (void) eth_send(pkt, len);
 }
 

diff --git a/net/eth.c b/net/eth.c
index b6c2af3de6a9deacdf9b29b8432dcc9de95590e5..13b7723bb47e545bdf480ccd65c2654f1e74ba35 100644 (file)
--- a/net/eth.c
+++ b/net/eth.c
@@ -98,6 +98,9 @@ struct eth_uclass_priv {
        struct udevice *current;
 };
 
+/* eth_errno - This stores the most recent failure code from DM functions */
+static int eth_errno;
+
 static struct eth_uclass_priv *eth_get_uclass_priv(void)
 {
        struct uclass *uc;
@@ -118,20 +121,32 @@ static void eth_set_current_to_next(void)
                uclass_first_device(UCLASS_ETH, &uc_priv->current);
 }
 
+/*
+ * Typically this will simply return the active device.
+ * In the case where the most recent active device was unset, this will attempt
+ * to return the first device. If that device doesn't exist or fails to probe,
+ * this function will return NULL.
+ */
 struct udevice *eth_get_dev(void)
 {
        struct eth_uclass_priv *uc_priv;
 
        uc_priv = eth_get_uclass_priv();
        if (!uc_priv->current)
-               uclass_first_device(UCLASS_ETH,
+               eth_errno = uclass_first_device(UCLASS_ETH,
                                    &uc_priv->current);
        return uc_priv->current;
 }
 
+/*
+ * Typically this will just store a device pointer.
+ * In case it was not probed, we will attempt to do so.
+ * dev may be NULL to unset the active device.
+ */
 static void eth_set_dev(struct udevice *dev)
 {
-       device_probe(dev);
+       if (dev && !device_active(dev))
+               eth_errno = device_probe(dev);
        eth_get_uclass_priv()->current = dev;
 }
 
@@ -155,7 +170,14 @@ struct udevice *eth_get_dev_by_name(const char *devname)
 
        uclass_get(UCLASS_ETH, &uc);
        uclass_foreach_dev(it, uc) {
-               /* We need the seq to be valid, so make sure it's probed */
+               /*
+                * We need the seq to be valid, so try to probe it.
+                * If the probe fails, the seq will not match since it will be
+                * -1 instead of what we are looking for.
+                * We don't care about errors from probe here. Either they won't
+                * 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
@@ -221,6 +243,7 @@ int eth_init(void)
 {
        struct udevice *current;
        struct udevice *old_current;
+       int ret = -ENODEV;
 
        current = eth_get_dev();
        if (!current) {
@@ -243,22 +266,29 @@ int eth_init(void)
                        else
                                memset(pdata->enetaddr, 0, 6);
 
-                       if (eth_get_ops(current)->start(current) >= 0) {
+                       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");
 
-               /* This will ensure the new "current" attempted to probe */
+               /*
+                * If ethrotate is enabled, this will change "current",
+                * otherwise we will drop out of this while loop immediately
+                */
                eth_try_another(0);
+               /* This will ensure the new "current" attempted to probe */
                current = eth_get_dev();
        } while (old_current != current);
 
-       return -ENODEV;
+       return ret;
 }
 
 void eth_halt(void)
@@ -278,6 +308,7 @@ void eth_halt(void)
 int eth_send(void *packet, int length)
 {
        struct udevice *current;
+       int ret;
 
        current = eth_get_dev();
        if (!current)
@@ -286,7 +317,12 @@ int eth_send(void *packet, int length)
        if (!device_active(current))
                return -EINVAL;
 
-       return eth_get_ops(current)->send(current, packet, length);
+       ret = eth_get_ops(current)->send(current, packet, length);
+       if (ret < 0) {
+               /* We cannot completely return the error at present */
+               debug("%s: send() returned error %d\n", __func__, ret);
+       }
+       return ret;
 }
 
 int eth_rx(void)
@@ -313,6 +349,10 @@ int eth_rx(void)
        }
        if (ret == -EAGAIN)
                ret = 0;
+       if (ret < 0) {
+               /* We cannot completely return the error at present */
+               debug("%s: recv() returned error %d\n", __func__, ret);
+       }
        return ret;
 }
 

diff --git a/net/net.c b/net/net.c
index afec4433f07c8ea24a074c98bab7a1fb4aba7c92..69f38f7483c66b8b298658006b5ee843b0c14280 100644 (file)
--- a/net/net.c
+++ b/net/net.c
@@ -84,6 +84,7 @@
 #include <common.h>
 #include <command.h>
 #include <environment.h>
+#include <errno.h>
 #include <net.h>
 #if defined(CONFIG_STATUS_LED)
 #include <miiphy.h>
@@ -333,7 +334,7 @@ void net_init(void)
 
 int NetLoop(enum proto_t protocol)
 {
-       int ret = -1;
+       int ret = -EINVAL;
 
        NetRestarted = 0;
        NetDevExists = 0;
@@ -345,9 +346,10 @@ int NetLoop(enum proto_t protocol)
        if (eth_is_on_demand_init() || protocol != NETCONS) {
                eth_halt();
                eth_set_current();
-               if (eth_init() < 0) {
+               ret = eth_init();
+               if (ret < 0) {
                        eth_halt();
-                       return -1;
+                       return ret;
                }
        } else
                eth_init_state_only();
@@ -370,7 +372,7 @@ restart:
        case 1:
                /* network not configured */
                eth_halt();
-               return -1;
+               return -ENODEV;
 
        case 2:
                /* network device not configured */
@@ -484,6 +486,8 @@ restart:
                /*
                 *      Check the ethernet for a new packet.  The ethernet
                 *      receive routine will process it.
+                *      Most drivers return the most recent packet size, but not
+                *      errors that may have happened.
                 */
                eth_rx();
 
@@ -537,7 +541,7 @@ restart:
                }
 
                if (net_state == NETLOOP_FAIL)
-                       NetStartAgain();
+                       ret = NetStartAgain();
 
                switch (net_state) {
 
@@ -597,11 +601,12 @@ startAgainTimeout(void)
        net_set_state(NETLOOP_RESTART);
 }
 
-void NetStartAgain(void)
+int NetStartAgain(void)
 {
        char *nretry;
        int retry_forever = 0;
        unsigned long retrycnt = 0;
+       int ret;
 
        nretry = getenv("netretry");
        if (nretry) {
@@ -621,7 +626,11 @@ void NetStartAgain(void)
        if ((!retry_forever) && (NetTryCount >= retrycnt)) {
                eth_halt();
                net_set_state(NETLOOP_FAIL);
-               return;
+               /*
+                * We don't provide a way for the protocol to return an error,
+                * but this is almost always the reason.
+                */
+               return -ETIMEDOUT;
        }
 
        NetTryCount++;
@@ -630,7 +639,7 @@ void NetStartAgain(void)
 #if !defined(CONFIG_NET_DO_NOT_TRY_ANOTHER)
        eth_try_another(!NetRestarted);
 #endif
-       eth_init();
+       ret = eth_init();
        if (NetRestartWrap) {
                NetRestartWrap = 0;
                if (NetDevExists) {
@@ -642,6 +651,7 @@ void NetStartAgain(void)
        } else {
                net_set_state(NETLOOP_RESTART);
        }
+       return ret;
 }
 
 /**********************************************************************/

diff --git a/test/dm/eth.c b/test/dm/eth.c
index a0e935939f012ee2da4754cc097c61fcfb8be09b..19236705e28c561d6082dc3bbb1f8aa47e7233dd 100644 (file)
--- a/test/dm/eth.c
+++ b/test/dm/eth.c
@@ -99,7 +99,7 @@ static int dm_test_eth_rotate(struct dm_test_state *dms)
        /* If ethrotate is no, then we should fail on a bad MAC */
        setenv("ethact", "eth@10004000");
        setenv("ethrotate", "no");
-       ut_asserteq(-1, NetLoop(PING));
+       ut_asserteq(-EINVAL, NetLoop(PING));
        ut_asserteq_str("eth@10004000", getenv("ethact"));
 
        /* Restore the env */
@@ -144,7 +144,7 @@ static int dm_test_net_retry(struct dm_test_state *dms)
         */
        setenv("ethact", "eth@10004000");
        setenv("netretry", "no");
-       ut_asserteq(-1, NetLoop(PING));
+       ut_asserteq(-ETIMEDOUT, NetLoop(PING));
        ut_asserteq_str("eth@10004000", getenv("ethact"));
 
        /* Restore the env */