[U-Boot] [PATCH v5 27/27] net: Improve error handling

Simon Glass sjg at chromium.org
Wed Mar 4 19:35:58 CET 2015


Hi Joe,

On 3 March 2015 at 19:41, Joe Hershberger <joe.hershberger at ni.com> wrote:
> 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 at ni.com>

Nice patch!

Reviewed-by: Simon Glass <sjg at chromium.org>

>
> ---
>
> Changes in v5:
> -New to v5
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
>  include/net.h |  3 ++-
>  net/eth.c     | 56 ++++++++++++++++++++++++++++++++++++++++++++++++--------
>  net/net.c     | 26 ++++++++++++++++++--------
>  test/dm/eth.c |  4 ++--
>  4 files changed, 70 insertions(+), 19 deletions(-)
>
> diff --git a/include/net.h b/include/net.h
> index 5846dfb..c702f2f 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -539,7 +539,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);
> @@ -609,6 +609,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 9966cf0..81ca436 100644
> --- 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)
> @@ -311,6 +347,10 @@ int eth_rx(void)
>                 else
>                         break;
>         }
> +       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 afec443..69f38f7 100644
> --- 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();

Where does this 'ret' get used?

>
>                 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 a0e9359..1923670 100644
> --- 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 at 10004000");
>         setenv("ethrotate", "no");
> -       ut_asserteq(-1, NetLoop(PING));
> +       ut_asserteq(-EINVAL, NetLoop(PING));
>         ut_asserteq_str("eth at 10004000", getenv("ethact"));
>
>         /* Restore the env */
> @@ -144,7 +144,7 @@ static int dm_test_net_retry(struct dm_test_state *dms)
>          */
>         setenv("ethact", "eth at 10004000");
>         setenv("netretry", "no");
> -       ut_asserteq(-1, NetLoop(PING));
> +       ut_asserteq(-ETIMEDOUT, NetLoop(PING));
>         ut_asserteq_str("eth at 10004000", getenv("ethact"));
>
>         /* Restore the env */
> --
> 1.7.11.5
>

Regards,
Simon


More information about the U-Boot mailing list