[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