* [PATCH 0/3] dpaa2-eth: misc cleanup
@ 2019-10-04 9:21 Ioana Ciornei
2019-10-04 9:21 ` [PATCH 1/3] dpaa2-eth: Cleanup dead code Ioana Ciornei
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ioana Ciornei @ 2019-10-04 9:21 UTC (permalink / raw)
To: davem; +Cc: netdev, Ioana Ciornei
This patch set consists of some cleanup patches ranging from removing dead
code to fixing a minor issue in ethtool stats. Also, unbounded while loops
are removed from the driver by adding a maximum number of retries for DPIO
portal commands.
Ioana Radulescu (3):
dpaa2-eth: Cleanup dead code
dpaa2-eth: Fix minor bug in ethtool stats reporting
dpaa2-eth: Avoid unbounded while loops
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 37 +++++++++++++++-------
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h | 8 +++++
.../net/ethernet/freescale/dpaa2/dpaa2-ethtool.c | 2 +-
3 files changed, 34 insertions(+), 13 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 1/3] dpaa2-eth: Cleanup dead code 2019-10-04 9:21 [PATCH 0/3] dpaa2-eth: misc cleanup Ioana Ciornei @ 2019-10-04 9:21 ` Ioana Ciornei 2019-10-04 9:21 ` [PATCH 2/3] dpaa2-eth: Fix minor bug in ethtool stats reporting Ioana Ciornei 2019-10-04 9:21 ` [PATCH 3/3] dpaa2-eth: Avoid unbounded while loops Ioana Ciornei 2 siblings, 0 replies; 6+ messages in thread From: Ioana Ciornei @ 2019-10-04 9:21 UTC (permalink / raw) To: davem; +Cc: netdev, Ioana Radulescu, Ioana Ciornei From: Ioana Radulescu <ruxandra.radulescu@nxp.com> Remove one function call whose result was not used anywhere. Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> --- drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c index 162d7d8fb295..2c5072fa9aa0 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c @@ -2043,7 +2043,6 @@ static struct fsl_mc_device *setup_dpcon(struct dpaa2_eth_priv *priv) { struct fsl_mc_device *dpcon; struct device *dev = priv->net_dev->dev.parent; - struct dpcon_attr attrs; int err; err = fsl_mc_object_allocate(to_fsl_mc_device(dev), @@ -2068,12 +2067,6 @@ static struct fsl_mc_device *setup_dpcon(struct dpaa2_eth_priv *priv) goto close; } - err = dpcon_get_attributes(priv->mc_io, 0, dpcon->mc_handle, &attrs); - if (err) { - dev_err(dev, "dpcon_get_attributes() failed\n"); - goto close; - } - err = dpcon_enable(priv->mc_io, 0, dpcon->mc_handle); if (err) { dev_err(dev, "dpcon_enable() failed\n"); -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] dpaa2-eth: Fix minor bug in ethtool stats reporting 2019-10-04 9:21 [PATCH 0/3] dpaa2-eth: misc cleanup Ioana Ciornei 2019-10-04 9:21 ` [PATCH 1/3] dpaa2-eth: Cleanup dead code Ioana Ciornei @ 2019-10-04 9:21 ` Ioana Ciornei 2019-10-04 9:21 ` [PATCH 3/3] dpaa2-eth: Avoid unbounded while loops Ioana Ciornei 2 siblings, 0 replies; 6+ messages in thread From: Ioana Ciornei @ 2019-10-04 9:21 UTC (permalink / raw) To: davem; +Cc: netdev, Ioana Radulescu, Ioana Ciornei From: Ioana Radulescu <ruxandra.radulescu@nxp.com> Don't print error message for a successful return value. Fixes: d84c3a4ded96 ("dpaa2-eth: Add new DPNI statistics counters") Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> --- drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c index 0aa1c34019bb..dc9a6c36cac0 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-ethtool.c @@ -216,7 +216,7 @@ static void dpaa2_eth_get_ethtool_stats(struct net_device *net_dev, if (err == -EINVAL) /* Older firmware versions don't support all pages */ memset(&dpni_stats, 0, sizeof(dpni_stats)); - else + else if (err) netdev_warn(net_dev, "dpni_get_stats(%d) failed\n", j); num_cnt = dpni_stats_page_size[j] / sizeof(u64); -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] dpaa2-eth: Avoid unbounded while loops 2019-10-04 9:21 [PATCH 0/3] dpaa2-eth: misc cleanup Ioana Ciornei 2019-10-04 9:21 ` [PATCH 1/3] dpaa2-eth: Cleanup dead code Ioana Ciornei 2019-10-04 9:21 ` [PATCH 2/3] dpaa2-eth: Fix minor bug in ethtool stats reporting Ioana Ciornei @ 2019-10-04 9:21 ` Ioana Ciornei 2019-10-04 18:18 ` Andrew Lunn 2 siblings, 1 reply; 6+ messages in thread From: Ioana Ciornei @ 2019-10-04 9:21 UTC (permalink / raw) To: davem; +Cc: netdev, Ioana Radulescu, Ioana Ciornei From: Ioana Radulescu <ruxandra.radulescu@nxp.com> Throughout the driver there are several places where we wait indefinitely for DPIO portal commands to be executed, while the portal returns a busy response code. Even though in theory we are guaranteed the portals become available eventually, in practice the QBMan hardware module may become unresponsive in various corner cases. Make sure we can never get stuck in an infinite while loop by adding a retry counter for all portal commands. Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> --- drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 30 ++++++++++++++++++++---- drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h | 8 +++++++ 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c index 2c5072fa9aa0..29702756734c 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c @@ -221,6 +221,7 @@ static void xdp_release_buf(struct dpaa2_eth_priv *priv, struct dpaa2_eth_channel *ch, dma_addr_t addr) { + int retries = 0; int err; ch->xdp.drop_bufs[ch->xdp.drop_cnt++] = addr; @@ -229,8 +230,11 @@ static void xdp_release_buf(struct dpaa2_eth_priv *priv, while ((err = dpaa2_io_service_release(ch->dpio, priv->bpid, ch->xdp.drop_bufs, - ch->xdp.drop_cnt)) == -EBUSY) + ch->xdp.drop_cnt)) == -EBUSY) { + if (retries++ >= DPAA2_ETH_SWP_BUSY_RETRIES) + break; cpu_relax(); + } if (err) { free_bufs(priv, ch->xdp.drop_bufs, ch->xdp.drop_cnt); @@ -458,7 +462,7 @@ static int consume_frames(struct dpaa2_eth_channel *ch, struct dpaa2_eth_fq *fq = NULL; struct dpaa2_dq *dq; const struct dpaa2_fd *fd; - int cleaned = 0; + int cleaned = 0, retries = 0; int is_last; do { @@ -469,6 +473,11 @@ static int consume_frames(struct dpaa2_eth_channel *ch, * the store until we get some sort of valid response * token (either a valid frame or an "empty dequeue") */ + if (retries++ >= DPAA2_ETH_SWP_BUSY_RETRIES) { + netdev_err_once(priv->net_dev, + "Unable to read a valid dequeue response\n"); + return 0; + } continue; } @@ -477,6 +486,7 @@ static int consume_frames(struct dpaa2_eth_channel *ch, fq->consume(priv, ch, fd, fq); cleaned++; + retries = 0; } while (!is_last); if (!cleaned) @@ -949,6 +959,7 @@ static int add_bufs(struct dpaa2_eth_priv *priv, u64 buf_array[DPAA2_ETH_BUFS_PER_CMD]; struct page *page; dma_addr_t addr; + int retries = 0; int i, err; for (i = 0; i < DPAA2_ETH_BUFS_PER_CMD; i++) { @@ -980,8 +991,11 @@ static int add_bufs(struct dpaa2_eth_priv *priv, release_bufs: /* In case the portal is busy, retry until successful */ while ((err = dpaa2_io_service_release(ch->dpio, bpid, - buf_array, i)) == -EBUSY) + buf_array, i)) == -EBUSY) { + if (retries++ >= DPAA2_ETH_SWP_BUSY_RETRIES) + break; cpu_relax(); + } /* If release command failed, clean up and bail out; * not much else we can do about it @@ -1032,16 +1046,21 @@ static int seed_pool(struct dpaa2_eth_priv *priv, u16 bpid) static void drain_bufs(struct dpaa2_eth_priv *priv, int count) { u64 buf_array[DPAA2_ETH_BUFS_PER_CMD]; + int retries = 0; int ret; do { ret = dpaa2_io_service_acquire(NULL, priv->bpid, buf_array, count); if (ret < 0) { + if (ret == -EBUSY && + retries++ >= DPAA2_ETH_SWP_BUSY_RETRIES) + continue; netdev_err(priv->net_dev, "dpaa2_io_service_acquire() failed\n"); return; } free_bufs(priv, buf_array, ret); + retries = 0; } while (ret); } @@ -1094,7 +1113,7 @@ static int pull_channel(struct dpaa2_eth_channel *ch) ch->store); dequeues++; cpu_relax(); - } while (err == -EBUSY); + } while (err == -EBUSY && dequeues < DPAA2_ETH_SWP_BUSY_RETRIES); ch->stats.dequeue_portal_busy += dequeues; if (unlikely(err)) @@ -1118,6 +1137,7 @@ static int dpaa2_eth_poll(struct napi_struct *napi, int budget) struct netdev_queue *nq; int store_cleaned, work_done; struct list_head rx_list; + int retries = 0; int err; ch = container_of(napi, struct dpaa2_eth_channel, napi); @@ -1163,7 +1183,7 @@ static int dpaa2_eth_poll(struct napi_struct *napi, int budget) do { err = dpaa2_io_service_rearm(ch->dpio, &ch->nctx); cpu_relax(); - } while (err == -EBUSY); + } while (err == -EBUSY && retries++ < DPAA2_ETH_SWP_BUSY_RETRIES); WARN_ONCE(err, "CDAN notifications rearm failed on core %d", ch->nctx.desired_cpu); diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h index 8a0e65b3267f..686b651edcb2 100644 --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h @@ -245,6 +245,14 @@ static inline struct dpaa2_faead *dpaa2_get_faead(void *buf_addr, bool swa) */ #define DPAA2_ETH_ENQUEUE_RETRIES 10 +/* Number of times to retry DPIO portal operations while waiting + * for portal to finish executing current command and become + * available. We want to avoid being stuck in a while loop in case + * hardware becomes unresponsive, but not give up too easily if + * the portal really is busy for valid reasons + */ +#define DPAA2_ETH_SWP_BUSY_RETRIES 1000 + /* Driver statistics, other than those in struct rtnl_link_stats64. * These are usually collected per-CPU and aggregated by ethtool. */ -- 1.9.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] dpaa2-eth: Avoid unbounded while loops 2019-10-04 9:21 ` [PATCH 3/3] dpaa2-eth: Avoid unbounded while loops Ioana Ciornei @ 2019-10-04 18:18 ` Andrew Lunn 2019-10-07 10:49 ` Ioana Ciornei 0 siblings, 1 reply; 6+ messages in thread From: Andrew Lunn @ 2019-10-04 18:18 UTC (permalink / raw) To: Ioana Ciornei; +Cc: davem, netdev, Ioana Radulescu On Fri, Oct 04, 2019 at 12:21:33PM +0300, Ioana Ciornei wrote: > From: Ioana Radulescu <ruxandra.radulescu@nxp.com> > > Throughout the driver there are several places where we wait > indefinitely for DPIO portal commands to be executed, while > the portal returns a busy response code. > > Even though in theory we are guaranteed the portals become > available eventually, in practice the QBMan hardware module > may become unresponsive in various corner cases. > > Make sure we can never get stuck in an infinite while loop > by adding a retry counter for all portal commands. > > Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com> > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > --- > drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 30 ++++++++++++++++++++---- > drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h | 8 +++++++ > 2 files changed, 33 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > index 2c5072fa9aa0..29702756734c 100644 > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > @@ -221,6 +221,7 @@ static void xdp_release_buf(struct dpaa2_eth_priv *priv, > struct dpaa2_eth_channel *ch, > dma_addr_t addr) > { > + int retries = 0; > int err; > > ch->xdp.drop_bufs[ch->xdp.drop_cnt++] = addr; > @@ -229,8 +230,11 @@ static void xdp_release_buf(struct dpaa2_eth_priv *priv, > > while ((err = dpaa2_io_service_release(ch->dpio, priv->bpid, > ch->xdp.drop_bufs, > - ch->xdp.drop_cnt)) == -EBUSY) > + ch->xdp.drop_cnt)) == -EBUSY) { > + if (retries++ >= DPAA2_ETH_SWP_BUSY_RETRIES) > + break; > cpu_relax(); > + } > > if (err) { > free_bufs(priv, ch->xdp.drop_bufs, ch->xdp.drop_cnt); > @@ -458,7 +462,7 @@ static int consume_frames(struct dpaa2_eth_channel *ch, > struct dpaa2_eth_fq *fq = NULL; > struct dpaa2_dq *dq; > const struct dpaa2_fd *fd; > - int cleaned = 0; > + int cleaned = 0, retries = 0; > int is_last; > > do { > @@ -469,6 +473,11 @@ static int consume_frames(struct dpaa2_eth_channel *ch, > * the store until we get some sort of valid response > * token (either a valid frame or an "empty dequeue") > */ > + if (retries++ >= DPAA2_ETH_SWP_BUSY_RETRIES) { > + netdev_err_once(priv->net_dev, > + "Unable to read a valid dequeue response\n"); > + return 0; > + } > continue; Hi Ioana It seems a bit odd that here you could return -ETIMEDOUT, but you print an error, and return 0. But in the two cases above in void functions, you don't print anything. Please at least return -ETIMEDOUT when you can. Andrew ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH 3/3] dpaa2-eth: Avoid unbounded while loops 2019-10-04 18:18 ` Andrew Lunn @ 2019-10-07 10:49 ` Ioana Ciornei 0 siblings, 0 replies; 6+ messages in thread From: Ioana Ciornei @ 2019-10-07 10:49 UTC (permalink / raw) To: Andrew Lunn; +Cc: davem, netdev, Ioana Ciocoi Radulescu > Subject: Re: [PATCH 3/3] dpaa2-eth: Avoid unbounded while loops > > On Fri, Oct 04, 2019 at 12:21:33PM +0300, Ioana Ciornei wrote: > > From: Ioana Radulescu <ruxandra.radulescu@nxp.com> > > > > Throughout the driver there are several places where we wait > > indefinitely for DPIO portal commands to be executed, while the portal > > returns a busy response code. > > > > Even though in theory we are guaranteed the portals become available > > eventually, in practice the QBMan hardware module may become > > unresponsive in various corner cases. > > > > Make sure we can never get stuck in an infinite while loop by adding a > > retry counter for all portal commands. > > > > Signed-off-by: Ioana Radulescu <ruxandra.radulescu@nxp.com> > > Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> > > --- > > drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 30 > > ++++++++++++++++++++---- > > drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h | 8 +++++++ > > 2 files changed, 33 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > index 2c5072fa9aa0..29702756734c 100644 > > --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c > > @@ -221,6 +221,7 @@ static void xdp_release_buf(struct dpaa2_eth_priv > *priv, > > struct dpaa2_eth_channel *ch, > > dma_addr_t addr) > > { > > + int retries = 0; > > int err; > > > > ch->xdp.drop_bufs[ch->xdp.drop_cnt++] = addr; @@ -229,8 +230,11 > @@ > > static void xdp_release_buf(struct dpaa2_eth_priv *priv, > > > > while ((err = dpaa2_io_service_release(ch->dpio, priv->bpid, > > ch->xdp.drop_bufs, > > - ch->xdp.drop_cnt)) == -EBUSY) > > + ch->xdp.drop_cnt)) == -EBUSY) { > > + if (retries++ >= DPAA2_ETH_SWP_BUSY_RETRIES) > > + break; > > cpu_relax(); > > + } > > > > if (err) { > > free_bufs(priv, ch->xdp.drop_bufs, ch->xdp.drop_cnt); @@ - > 458,7 > > +462,7 @@ static int consume_frames(struct dpaa2_eth_channel *ch, > > struct dpaa2_eth_fq *fq = NULL; > > struct dpaa2_dq *dq; > > const struct dpaa2_fd *fd; > > - int cleaned = 0; > > + int cleaned = 0, retries = 0; > > int is_last; > > > > do { > > @@ -469,6 +473,11 @@ static int consume_frames(struct dpaa2_eth_channel > *ch, > > * the store until we get some sort of valid response > > * token (either a valid frame or an "empty dequeue") > > */ > > + if (retries++ >= DPAA2_ETH_SWP_BUSY_RETRIES) { > > + netdev_err_once(priv->net_dev, > > + "Unable to read a valid > dequeue response\n"); > > + return 0; > > + } > > continue; > > Hi Ioana > > It seems a bit odd that here you could return -ETIMEDOUT, but you print an > error, and return 0. But in the two cases above in void functions, you don't print > anything. > > Please at least return -ETIMEDOUT when you can. Hi Andrew, That's a good point. I'll send out a v2 fixing this. Thanks, Ioana > > Andrew ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-10-07 10:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-04 9:21 [PATCH 0/3] dpaa2-eth: misc cleanup Ioana Ciornei 2019-10-04 9:21 ` [PATCH 1/3] dpaa2-eth: Cleanup dead code Ioana Ciornei 2019-10-04 9:21 ` [PATCH 2/3] dpaa2-eth: Fix minor bug in ethtool stats reporting Ioana Ciornei 2019-10-04 9:21 ` [PATCH 3/3] dpaa2-eth: Avoid unbounded while loops Ioana Ciornei 2019-10-04 18:18 ` Andrew Lunn 2019-10-07 10:49 ` Ioana Ciornei
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).