From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Wed, 4 Mar 2015 11:35:58 -0700 Subject: [U-Boot] [PATCH v5 27/27] net: Improve error handling In-Reply-To: <1425436881-10323-28-git-send-email-joe.hershberger@ni.com> References: <1424822552-4366-1-git-send-email-joe.hershberger@ni.com> <1425436881-10323-1-git-send-email-joe.hershberger@ni.com> <1425436881-10323-28-git-send-email-joe.hershberger@ni.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Joe, On 3 March 2015 at 19:41, Joe Hershberger 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 Nice patch! Reviewed-by: Simon Glass > > --- > > 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 > #include > #include > +#include > #include > #if defined(CONFIG_STATUS_LED) > #include > @@ -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