[PATCH net 0/2]cxgb4: Fix ethtool selftest flits calculation

[PATCH net 0/2]cxgb4: Fix ethtool selftest flits calculation

[Ganji Aravind]

Patch 1 will fix work request size calculation for loopback selftest.

Patch 2 will fix race between loopback selftest and normal Tx handler.

Thanks,
Ganji Aravind.

Ganji Aravind (2):
  cxgb4: Fix work request size calculation for loopback test
  cxgb4: Fix race between loopback and normal Tx path

 drivers/net/ethernet/chelsio/cxgb4/sge.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

-- 
2.26.2

[PATCH net 1/2] cxgb4: Fix work request size calculation for loopback test

[Ganji Aravind]

Work request used for sending loopback packet needs to add
the firmware work request only once. So, fix by using
correct structure size.

Fixes: 7235ffae3d2c ("cxgb4: add loopback ethtool self-test")
Signed-off-by: Ganji Aravind <ganji.aravind@chelsio.com>
---
 drivers/net/ethernet/chelsio/cxgb4/sge.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
index d2b587d1670a..7c9fe4bc235b 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
@@ -2553,8 +2553,8 @@ int cxgb4_selftest_lb_pkt(struct net_device *netdev)
 
 	pkt_len = ETH_HLEN + sizeof(CXGB4_SELFTEST_LB_STR);
 
-	flits = DIV_ROUND_UP(pkt_len + sizeof(struct cpl_tx_pkt) +
-			     sizeof(*wr), sizeof(__be64));
+	flits = DIV_ROUND_UP(pkt_len + sizeof(*cpl) + sizeof(*wr),
+			     sizeof(__be64));
 	ndesc = flits_to_desc(flits);
 
 	lb = &pi->ethtool_lb;
-- 
2.26.2

Re: [PATCH net 1/2] cxgb4: Fix work request size calculation for loopback test

[Jesse Brandeburg]

Ganji Aravind wrote:

> Work request used for sending loopback packet needs to add
> the firmware work request only once. So, fix by using
> correct structure size.
> 
> Fixes: 7235ffae3d2c ("cxgb4: add loopback ethtool self-test")
> Signed-off-by: Ganji Aravind <ganji.aravind@chelsio.com>

changes look ok, but to understand why this change fixed the bug, you
could have just mentioned that the cpl_tx_pkt struct has a _core member
inside of it, and then I wouldn't have had to waste review time digging
through the code in the kernel.

Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>

[PATCH net 2/2] cxgb4: Fix race between loopback and normal Tx path

[Ganji Aravind]

Even after Tx queues are marked stopped, there exists a
small window where the current packet in the normal Tx
path is still being sent out and loopback selftest ends
up corrupting the same Tx ring. So, ensure selftest takes
the Tx lock to synchronize access the Tx ring.

Fixes: 7235ffae3d2c ("cxgb4: add loopback ethtool self-test")
Signed-off-by: Ganji Aravind <ganji.aravind@chelsio.com>
---
 drivers/net/ethernet/chelsio/cxgb4/sge.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/sge.c b/drivers/net/ethernet/chelsio/cxgb4/sge.c
index 7c9fe4bc235b..869431a1eedd 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/sge.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/sge.c
@@ -2561,11 +2561,14 @@ int cxgb4_selftest_lb_pkt(struct net_device *netdev)
 	lb->loopback = 1;
 
 	q = &adap->sge.ethtxq[pi->first_qset];
+	__netif_tx_lock(q->txq, smp_processor_id());
 
 	reclaim_completed_tx(adap, &q->q, -1, true);
 	credits = txq_avail(&q->q) - ndesc;
-	if (unlikely(credits < 0))
+	if (unlikely(credits < 0)) {
+		__netif_tx_unlock(q->txq);
 		return -ENOMEM;
+	}
 
 	wr = (void *)&q->q.desc[q->q.pidx];
 	memset(wr, 0, sizeof(struct tx_desc));
@@ -2598,6 +2601,7 @@ int cxgb4_selftest_lb_pkt(struct net_device *netdev)
 	init_completion(&lb->completion);
 	txq_advance(&q->q, ndesc);
 	cxgb4_ring_tx_db(adap, &q->q, ndesc);
+	__netif_tx_unlock(q->txq);
 
 	/* wait for the pkt to return */
 	ret = wait_for_completion_timeout(&lb->completion, 10 * HZ);
-- 
2.26.2

Re: [PATCH net 2/2] cxgb4: Fix race between loopback and normal Tx path

[Jesse Brandeburg]

Ganji Aravind wrote:

> Even after Tx queues are marked stopped, there exists a
> small window where the current packet in the normal Tx
> path is still being sent out and loopback selftest ends
> up corrupting the same Tx ring. So, ensure selftest takes
> the Tx lock to synchronize access the Tx ring.
> 
> Fixes: 7235ffae3d2c ("cxgb4: add loopback ethtool self-test")
> Signed-off-by: Ganji Aravind <ganji.aravind@chelsio.com>

Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>

Re: [PATCH net 0/2]cxgb4: Fix ethtool selftest flits calculation

[David Miller]

From: Ganji Aravind <ganji.aravind@chelsio.com>
Date: Tue, 18 Aug 2020 21:10:56 +0530

> Patch 1 will fix work request size calculation for loopback selftest.
> 
> Patch 2 will fix race between loopback selftest and normal Tx handler.

Series applied.

Thanks for the review Jesse.