[PATCH net 0/4] net: aquantia: various fixes May, 2019

[PATCH net 0/4] net: aquantia: various fixes May, 2019

[Igor Russkikh]

Here is a set of various bug fixes found on recent verification stage.


Dmitry Bogdanov (2):
  net: aquantia: check rx csum for all packets in LRO session
  net: aquantia: fix LRO with FCS error

Igor Russkikh (1):
  net: aquantia: tx clean budget logic error

Nikita Danilov (1):
  net: aquantia: tcp checksum 0xffff being handled incorrectly

 .../net/ethernet/aquantia/atlantic/aq_ring.c  | 51 ++++++++++-----
 .../aquantia/atlantic/hw_atl/hw_atl_b0.c      | 64 ++++++++++---------
 2 files changed, 68 insertions(+), 47 deletions(-)

-- 
2.17.1

[PATCH net 1/4] net: aquantia: tx clean budget logic error

[Igor Russkikh]

In case no other traffic happening on the ring, full tx cleanup
may not be completed. That may cause socket buffer to overflow
and tx traffic to stuck until next activity on the ring happens.

This is due to logic error in budget variable decrementor.
Variable is compared with zero, and then post decremented,
causing it to become MAX_INT. Solution is remove decrementor
from the `for` statement and rewrite it in a clear way.

Fixes: b647d3980948e ("net: aquantia: Add tx clean budget and valid budget handling logic")
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/aq_ring.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index 350e385528fd..63ed00415904 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -223,10 +223,10 @@ void aq_ring_queue_stop(struct aq_ring_s *ring)
 bool aq_ring_tx_clean(struct aq_ring_s *self)
 {
 	struct device *dev = aq_nic_get_dev(self->aq_nic);
-	unsigned int budget = AQ_CFG_TX_CLEAN_BUDGET;
+	unsigned int budget;
 
-	for (; self->sw_head != self->hw_head && budget--;
-		self->sw_head = aq_ring_next_dx(self, self->sw_head)) {
+	for (budget = AQ_CFG_TX_CLEAN_BUDGET;
+	     budget && self->sw_head != self->hw_head; budget--) {
 		struct aq_ring_buff_s *buff = &self->buff_ring[self->sw_head];
 
 		if (likely(buff->is_mapped)) {
@@ -251,6 +251,7 @@ bool aq_ring_tx_clean(struct aq_ring_s *self)
 
 		buff->pa = 0U;
 		buff->eop_index = 0xffffU;
+		self->sw_head = aq_ring_next_dx(self, self->sw_head);
 	}
 
 	return !!budget;
-- 
2.17.1

Re: [PATCH net 1/4] net: aquantia: tx clean budget logic error

[David Miller]

From: Igor Russkikh <Igor.Russkikh@aquantia.com>
Date: Sat, 25 May 2019 09:57:59 +0000

> In case no other traffic happening on the ring, full tx cleanup
> may not be completed. That may cause socket buffer to overflow
> and tx traffic to stuck until next activity on the ring happens.
> 
> This is due to logic error in budget variable decrementor.
> Variable is compared with zero, and then post decremented,
> causing it to become MAX_INT. Solution is remove decrementor
> from the `for` statement and rewrite it in a clear way.
> 
> Fixes: b647d3980948e ("net: aquantia: Add tx clean budget and valid budget handling logic")
> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>

I think the TX clean budget is a very bad idea.

You should always do as much TX clean work as there is TODO.

Re: [PATCH net 1/4] net: aquantia: tx clean budget logic error

[Igor Russkikh]

On 27.05.2019 8:15, David Miller wrote:
> From: Igor Russkikh <Igor.Russkikh@aquantia.com>
> Date: Sat, 25 May 2019 09:57:59 +0000
> 
>> In case no other traffic happening on the ring, full tx cleanup
>> may not be completed. That may cause socket buffer to overflow
>> and tx traffic to stuck until next activity on the ring happens.
>>
>> This is due to logic error in budget variable decrementor.
>> Variable is compared with zero, and then post decremented,
>> causing it to become MAX_INT. Solution is remove decrementor
>> from the `for` statement and rewrite it in a clear way.
>>
>> Fixes: b647d3980948e ("net: aquantia: Add tx clean budget and valid budget handling logic")
>> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
> 
> I think the TX clean budget is a very bad idea.
> 
> You should always do as much TX clean work as there is TODO.

Hi David,

This is not about introducing tx clean budget, but about fixing a bug.

tx clean budget logic is present in majority of the drivers as I see,
including igb,ixgbe,mlx5.

I see it as a logical action to limit the time driver spends in napi_poll
under napi budget.

Regards,
  Igor

Re: [PATCH net 1/4] net: aquantia: tx clean budget logic error

[David Miller]

From: Igor Russkikh <Igor.Russkikh@aquantia.com>
Date: Mon, 27 May 2019 09:57:18 +0000

> This is not about introducing tx clean budget, but about fixing a bug.
> 
> tx clean budget logic is present in majority of the drivers as I see,
> including igb,ixgbe,mlx5.
> 
> I see it as a logical action to limit the time driver spends in napi_poll
> under napi budget.

Ok, series applied, thank you.

[PATCH net 2/4] net: aquantia: check rx csum for all packets in LRO session

[Igor Russkikh]

From: Dmitry Bogdanov <dmitry.bogdanov@aquantia.com>

Atlantic hardware does not aggregate nor breaks LRO sessions
with bad csum packets. This means driver should take care of that.

If in LRO session there is a non-first descriptor with invalid
checksum (L2/L3/L4), the driver must account this information
in csum application logic.

Fixes: 018423e90bee8 ("net: ethernet: aquantia: Add ring support code")
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
Signed-off-by: Dmitry Bogdanov <dmitry.bogdanov@aquantia.com>
---
 .../net/ethernet/aquantia/atlantic/aq_ring.c  | 44 +++++++++++++------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index 63ed00415904..941b0beb87ef 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -299,35 +299,47 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
 		unsigned int i = 0U;
 		u16 hdr_len;
 
-		if (buff->is_error)
-			continue;
-
 		if (buff->is_cleaned)
 			continue;
 
 		if (!buff->is_eop) {
-			for (next_ = buff->next,
-			     buff_ = &self->buff_ring[next_]; true;
-			     next_ = buff_->next,
-			     buff_ = &self->buff_ring[next_]) {
+			buff_ = buff;
+			do {
+				next_ = buff_->next,
+				buff_ = &self->buff_ring[next_];
 				is_rsc_completed =
 					aq_ring_dx_in_range(self->sw_head,
 							    next_,
 							    self->hw_head);
 
-				if (unlikely(!is_rsc_completed)) {
-					is_rsc_completed = false;
+				if (unlikely(!is_rsc_completed))
 					break;
-				}
 
-				if (buff_->is_eop)
-					break;
-			}
+				buff->is_error |= buff_->is_error;
+
+			} while (!buff_->is_eop);
 
 			if (!is_rsc_completed) {
 				err = 0;
 				goto err_exit;
 			}
+			if (buff->is_error) {
+				buff_ = buff;
+				do {
+					next_ = buff_->next,
+					buff_ = &self->buff_ring[next_];
+
+					buff_->is_cleaned = true;
+				} while (!buff_->is_eop);
+
+				++self->stats.rx.errors;
+				continue;
+			}
+		}
+
+		if (buff->is_error) {
+			++self->stats.rx.errors;
+			continue;
 		}
 
 		dma_sync_single_range_for_cpu(aq_nic_get_dev(self->aq_nic),
@@ -390,6 +402,12 @@ int aq_ring_rx_clean(struct aq_ring_s *self,
 							AQ_CFG_RX_FRAME_MAX);
 					page_ref_inc(buff_->rxdata.page);
 					buff_->is_cleaned = 1;
+
+					buff->is_ip_cso &= buff_->is_ip_cso;
+					buff->is_udp_cso &= buff_->is_udp_cso;
+					buff->is_tcp_cso &= buff_->is_tcp_cso;
+					buff->is_cso_err |= buff_->is_cso_err;
+
 				} while (!buff_->is_eop);
 			}
 		}
-- 
2.17.1

[PATCH net 3/4] net: aquantia: fix LRO with FCS error

[Igor Russkikh]

From: Dmitry Bogdanov <dmitry.bogdanov@aquantia.com>

Driver stops producing skbs on ring if a packet with FCS error
was coalesced into LRO session. Ring gets hang forever.

Thats a logical error in driver processing descriptors:
When rx_stat indicates MAC Error, next pointer and eop flags
are not filled. This confuses driver so it waits for descriptor 0
to be filled by HW.

Solution is fill next pointer and eop flag even for packets with FCS error.

Fixes: bab6de8fd180b ("net: ethernet: aquantia: Atlantic A0 and B0 specific functions.")
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
Signed-off-by: Dmitry Bogdanov <dmitry.bogdanov@aquantia.com>
---
 .../aquantia/atlantic/hw_atl/hw_atl_b0.c      | 61 ++++++++++---------
 1 file changed, 32 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
index bfcda12d73de..e979f227cf0b 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
@@ -713,38 +713,41 @@ static int hw_atl_b0_hw_ring_rx_receive(struct aq_hw_s *self,
 		if ((rx_stat & BIT(0)) || rxd_wb->type & 0x1000U) {
 			/* MAC error or DMA error */
 			buff->is_error = 1U;
-		} else {
-			if (self->aq_nic_cfg->is_rss) {
-				/* last 4 byte */
-				u16 rss_type = rxd_wb->type & 0xFU;
-
-				if (rss_type && rss_type < 0x8U) {
-					buff->is_hash_l4 = (rss_type == 0x4 ||
-					rss_type == 0x5);
-					buff->rss_hash = rxd_wb->rss_hash;
-				}
+		}
+		if (self->aq_nic_cfg->is_rss) {
+			/* last 4 byte */
+			u16 rss_type = rxd_wb->type & 0xFU;
+
+			if (rss_type && rss_type < 0x8U) {
+				buff->is_hash_l4 = (rss_type == 0x4 ||
+				rss_type == 0x5);
+				buff->rss_hash = rxd_wb->rss_hash;
 			}
+		}
 
-			if (HW_ATL_B0_RXD_WB_STAT2_EOP & rxd_wb->status) {
-				buff->len = rxd_wb->pkt_len %
-					AQ_CFG_RX_FRAME_MAX;
-				buff->len = buff->len ?
-					buff->len : AQ_CFG_RX_FRAME_MAX;
-				buff->next = 0U;
-				buff->is_eop = 1U;
+		if (HW_ATL_B0_RXD_WB_STAT2_EOP & rxd_wb->status) {
+			buff->len = rxd_wb->pkt_len %
+				AQ_CFG_RX_FRAME_MAX;
+			buff->len = buff->len ?
+				buff->len : AQ_CFG_RX_FRAME_MAX;
+			buff->next = 0U;
+			buff->is_eop = 1U;
+		} else {
+			buff->len =
+				rxd_wb->pkt_len > AQ_CFG_RX_FRAME_MAX ?
+				AQ_CFG_RX_FRAME_MAX : rxd_wb->pkt_len;
+
+			if (HW_ATL_B0_RXD_WB_STAT2_RSCCNT &
+				rxd_wb->status) {
+				/* LRO */
+				buff->next = rxd_wb->next_desc_ptr;
+				++ring->stats.rx.lro_packets;
 			} else {
-				if (HW_ATL_B0_RXD_WB_STAT2_RSCCNT &
-					rxd_wb->status) {
-					/* LRO */
-					buff->next = rxd_wb->next_desc_ptr;
-					++ring->stats.rx.lro_packets;
-				} else {
-					/* jumbo */
-					buff->next =
-						aq_ring_next_dx(ring,
-								ring->hw_head);
-					++ring->stats.rx.jumbo_packets;
-				}
+				/* jumbo */
+				buff->next =
+					aq_ring_next_dx(ring,
+							ring->hw_head);
+				++ring->stats.rx.jumbo_packets;
 			}
 		}
 	}
-- 
2.17.1

[PATCH net 4/4] net: aquantia: tcp checksum 0xffff being handled incorrectly

[Igor Russkikh]

From: Nikita Danilov <ndanilov@aquantia.com>

Thats a known quirk in windows tcp stack it can produce 0xffff checksum.
Thats incorrect but it is.

Atlantic HW with LRO enabled handles that incorrectly and changes csum to
0xfffe - but indicates that csum is invalid. This causes driver to pass
packet to linux networking stack with CSUM_NONE, stack eventually drops
the packet.

There is a quirk in atlantic HW to enable correct processing of
0xffff incorrect csum. Enable it.

The visible bug is that windows link partner with software generated csums
caused TCP connection to be unstable since all packets that csum value
are dropped.

Reported-by: Dmitry Bezrukov <dmitry.bezrukov@aquantia.com>
Signed-off-by: Nikita Danilov <ndanilov@aquantia.com>
Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
---
 drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
index e979f227cf0b..5c3065bdfddf 100644
--- a/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
+++ b/drivers/net/ethernet/aquantia/atlantic/hw_atl/hw_atl_b0.c
@@ -266,12 +266,11 @@ static int hw_atl_b0_hw_offload_set(struct aq_hw_s *self,
 		 */
 		hw_atl_rpo_lro_max_coalescing_interval_set(self, 50);
 
-
 		hw_atl_rpo_lro_qsessions_lim_set(self, 1U);
 
 		hw_atl_rpo_lro_total_desc_lim_set(self, 2U);
 
-		hw_atl_rpo_lro_patch_optimization_en_set(self, 0U);
+		hw_atl_rpo_lro_patch_optimization_en_set(self, 1U);
 
 		hw_atl_rpo_lro_min_pay_of_first_pkt_set(self, 10U);
 
-- 
2.17.1