[Intel-wired-lan] [PATCH 1/3] igc: Clean RX descriptor error flags

[Intel-wired-lan] [PATCH 1/3] igc: Clean RX descriptor error flags

[Andre Guedes]

I225 advanced receive descriptor doesn't have the following extend error
bits: CE, SE, SEQ, CXE. In addition to that, the bit TCPE is called L4E
in the datasheet.

This patch cleans up the code accordingly, and gets rid of the macro
IGC_RXDEXT_ERR_FRAME_ERR_MASK since it doesn't make much sense anymore.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_defines.h | 14 +-------------
 drivers/net/ethernet/intel/igc/igc_main.c    |  5 ++---
 2 files changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 28885be15ee8..21695476b8a5 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -324,22 +324,10 @@
 /* Advanced Receive Descriptor bit definitions */
 #define IGC_RXDADV_STAT_TSIP	0x08000 /* timestamp in packet */
 
-#define IGC_RXDEXT_STATERR_CE		0x01000000
-#define IGC_RXDEXT_STATERR_SE		0x02000000
-#define IGC_RXDEXT_STATERR_SEQ		0x04000000
-#define IGC_RXDEXT_STATERR_CXE		0x10000000
-#define IGC_RXDEXT_STATERR_TCPE		0x20000000
+#define IGC_RXDEXT_STATERR_L4E		0x20000000
 #define IGC_RXDEXT_STATERR_IPE		0x40000000
 #define IGC_RXDEXT_STATERR_RXE		0x80000000
 
-/* Same mask, but for extended and packet split descriptors */
-#define IGC_RXDEXT_ERR_FRAME_ERR_MASK ( \
-	IGC_RXDEXT_STATERR_CE  |	\
-	IGC_RXDEXT_STATERR_SE  |	\
-	IGC_RXDEXT_STATERR_SEQ |	\
-	IGC_RXDEXT_STATERR_CXE |	\
-	IGC_RXDEXT_STATERR_RXE)
-
 #define IGC_MRQC_RSS_FIELD_IPV4_TCP	0x00010000
 #define IGC_MRQC_RSS_FIELD_IPV4		0x00020000
 #define IGC_MRQC_RSS_FIELD_IPV6_TCP_EX	0x00040000
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index b303d05e0870..298f408519f4 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1437,7 +1437,7 @@ static void igc_rx_checksum(struct igc_ring *ring,
 
 	/* TCP/UDP checksum error bit is set */
 	if (igc_test_staterr(rx_desc,
-			     IGC_RXDEXT_STATERR_TCPE |
+			     IGC_RXDEXT_STATERR_L4E |
 			     IGC_RXDEXT_STATERR_IPE)) {
 		/* work around errata with sctp packets where the TCPE aka
 		 * L4E bit is set incorrectly on 64 byte (60 byte w/o crc)
@@ -1752,8 +1752,7 @@ static bool igc_cleanup_headers(struct igc_ring *rx_ring,
 				union igc_adv_rx_desc *rx_desc,
 				struct sk_buff *skb)
 {
-	if (unlikely((igc_test_staterr(rx_desc,
-				       IGC_RXDEXT_ERR_FRAME_ERR_MASK)))) {
+	if (unlikely(igc_test_staterr(rx_desc, IGC_RXDEXT_STATERR_RXE))) {
 		struct net_device *netdev = rx_ring->netdev;
 
 		if (!(netdev->features & NETIF_F_RXALL)) {
-- 
2.26.2

[Intel-wired-lan] [PATCH 2/3] igc: Check descriptor's DD bit in igc_clean_rx_irq()

[Andre Guedes]

I225 advanced receive descriptor provides the Descriptor Done (DD) bit
which indicates hardware is done with that receive descriptor and
software should handle it.

This patch fixes igc_clean_rx_irq() so we check that bit to determine if
we are done handling incoming packets instead of checking the packet
length information. It also gets rid of rx_desc->wb.upper.length
assignments spread through the code required to make the previous
approach to work.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_defines.h |  1 +
 drivers/net/ethernet/intel/igc/igc_main.c    | 14 ++++++++------
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
index 21695476b8a5..43a7c7944804 100644
--- a/drivers/net/ethernet/intel/igc/igc_defines.h
+++ b/drivers/net/ethernet/intel/igc/igc_defines.h
@@ -316,6 +316,7 @@
 #define IGC_SRRCTL_TIMER0SEL(timer)	(((timer) & 0x3) << 17)
 
 /* Receive Descriptor bit definitions */
+#define IGC_RXD_STAT_DD		0x01	/* Descriptor Done */
 #define IGC_RXD_STAT_EOP	0x02	/* End of Packet */
 #define IGC_RXD_STAT_IXSM	0x04	/* Ignore checksum */
 #define IGC_RXD_STAT_UDPCS	0x10	/* UDP xsum calculated */
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 298f408519f4..0c481dc906ad 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -551,7 +551,6 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
 
 	/* initialize Rx descriptor 0 */
 	rx_desc = IGC_RX_DESC(ring, 0);
-	rx_desc->wb.upper.length = 0;
 
 	/* enable receive descriptor fetching */
 	rxdctl |= IGC_RXDCTL_QUEUE_ENABLE;
@@ -1880,9 +1879,6 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count)
 			i -= rx_ring->count;
 		}
 
-		/* clear the length for the next_to_use descriptor */
-		rx_desc->wb.upper.length = 0;
-
 		cleaned_count--;
 	} while (cleaned_count);
 
@@ -1924,8 +1920,12 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 		}
 
 		rx_desc = IGC_RX_DESC(rx_ring, rx_ring->next_to_clean);
-		size = le16_to_cpu(rx_desc->wb.upper.length);
-		if (!size)
+
+		/* If we reached a descriptor with 'Descriptor Done' bit not
+		 * set, it means we have handled all descriptors owned by
+		 * software already so we should prematurely break the loop.
+		 */
+		if (!igc_test_staterr(rx_desc, IGC_RXD_STAT_DD))
 			break;
 
 		/* This memory barrier is needed to keep us from reading
@@ -1934,6 +1934,8 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 		 */
 		dma_rmb();
 
+		size = le16_to_cpu(rx_desc->wb.upper.length);
+
 		rx_buffer = igc_get_rx_buffer(rx_ring, size);
 
 		/* retrieve a buffer from the ring */
-- 
2.26.2

[Intel-wired-lan] [PATCH 3/3] igc: Fix SRRCTL register setup

[Andre Guedes]

SRRCTL register is set with 'one buffer descriptor' option (see DESCTYPE
setting a few lines below) so setting BSIZEHEADER bits is pointless.
They should be zero. Also, since there is no header buffer we should set
the header buffer address field from the receive descriptor to zero for
the sake of consistency.

Signed-off-by: Andre Guedes <andre.guedes@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 0c481dc906ad..a5d825d44002 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -531,14 +531,11 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
 	ring->next_to_clean = 0;
 	ring->next_to_use = 0;
 
-	/* set descriptor configuration */
-	srrctl = IGC_RX_HDR_LEN << IGC_SRRCTL_BSIZEHDRSIZE_SHIFT;
 	if (ring_uses_large_buffer(ring))
 		srrctl |= IGC_RXBUFFER_3072 >> IGC_SRRCTL_BSIZEPKT_SHIFT;
 	else
 		srrctl |= IGC_RXBUFFER_2048 >> IGC_SRRCTL_BSIZEPKT_SHIFT;
 	srrctl |= IGC_SRRCTL_DESCTYPE_ADV_ONEBUF;
-
 	wr32(IGC_SRRCTL(reg_idx), srrctl);
 
 	rxdctl |= IGC_RX_PTHRESH;
@@ -1869,6 +1866,7 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count)
 		 * because each write-back erases this info.
 		 */
 		rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset);
+		rx_desc->read.hdr_addr = 0;
 
 		rx_desc++;
 		bi++;
-- 
2.26.2

[Intel-wired-lan] [PATCH 3/3] igc: Fix SRRCTL register setup

[Alexander Duyck]

On Mon, Aug 10, 2020 at 2:08 PM Andre Guedes <andre.guedes@intel.com> wrote:
>
> SRRCTL register is set with 'one buffer descriptor' option (see DESCTYPE
> setting a few lines below) so setting BSIZEHEADER bits is pointless.
> They should be zero. Also, since there is no header buffer we should set
> the header buffer address field from the receive descriptor to zero for
> the sake of consistency.
>
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 0c481dc906ad..a5d825d44002 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -531,14 +531,11 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
>         ring->next_to_clean = 0;
>         ring->next_to_use = 0;
>
> -       /* set descriptor configuration */
> -       srrctl = IGC_RX_HDR_LEN << IGC_SRRCTL_BSIZEHDRSIZE_SHIFT;
>         if (ring_uses_large_buffer(ring))
>                 srrctl |= IGC_RXBUFFER_3072 >> IGC_SRRCTL_BSIZEPKT_SHIFT;
>         else
>                 srrctl |= IGC_RXBUFFER_2048 >> IGC_SRRCTL_BSIZEPKT_SHIFT;
>         srrctl |= IGC_SRRCTL_DESCTYPE_ADV_ONEBUF;
> -
>         wr32(IGC_SRRCTL(reg_idx), srrctl);
>
>         rxdctl |= IGC_RX_PTHRESH;

Some of this was left in place to leave parity with the ixgbe driver
which was required to populate that field in order to enable RSC/LRO.

> @@ -1869,6 +1866,7 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count)
>                  * because each write-back erases this info.
>                  */
>                 rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset);
> +               rx_desc->read.hdr_addr = 0;
>
>                 rx_desc++;
>                 bi++;

If you are going to do this it would be better to replace the line
that is setting the length to zero instead of just adding this line.
That way you can avoid having to rewrite it. I only had bothered with
clearing the length field as it was a 32b field, however if you are
wanting to flush the full 64b then I would recommend doing it there
rather than here.

[Intel-wired-lan] [PATCH 3/3] igc: Fix SRRCTL register setup

[Andre Guedes]

Quoting Alexander Duyck (2020-08-10 15:56:31)
> > @@ -1869,6 +1866,7 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count)
> >                  * because each write-back erases this info.
> >                  */
> >                 rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset);
> > +               rx_desc->read.hdr_addr = 0;
> >
> >                 rx_desc++;
> >                 bi++;
> 
> If you are going to do this it would be better to replace the line
> that is setting the length to zero instead of just adding this line.
> That way you can avoid having to rewrite it. I only had bothered with
> clearing the length field as it was a 32b field, however if you are
> wanting to flush the full 64b then I would recommend doing it there
> rather than here.

Just to make sure I'm on the same page, do you mean to move this line to
patch 2/3, right?

[Intel-wired-lan] [PATCH 2/3] igc: Check descriptor's DD bit in igc_clean_rx_irq()

[Alexander Duyck]

On Mon, Aug 10, 2020 at 2:08 PM Andre Guedes <andre.guedes@intel.com> wrote:
>
> I225 advanced receive descriptor provides the Descriptor Done (DD) bit
> which indicates hardware is done with that receive descriptor and
> software should handle it.
>
> This patch fixes igc_clean_rx_irq() so we check that bit to determine if
> we are done handling incoming packets instead of checking the packet
> length information. It also gets rid of rx_desc->wb.upper.length
> assignments spread through the code required to make the previous
> approach to work.
>
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>

I do not agree with this patch. The patch itself will break a number of things.

> ---
>  drivers/net/ethernet/intel/igc/igc_defines.h |  1 +
>  drivers/net/ethernet/intel/igc/igc_main.c    | 14 ++++++++------
>  2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_defines.h b/drivers/net/ethernet/intel/igc/igc_defines.h
> index 21695476b8a5..43a7c7944804 100644
> --- a/drivers/net/ethernet/intel/igc/igc_defines.h
> +++ b/drivers/net/ethernet/intel/igc/igc_defines.h
> @@ -316,6 +316,7 @@
>  #define IGC_SRRCTL_TIMER0SEL(timer)    (((timer) & 0x3) << 17)
>
>  /* Receive Descriptor bit definitions */
> +#define IGC_RXD_STAT_DD                0x01    /* Descriptor Done */
>  #define IGC_RXD_STAT_EOP       0x02    /* End of Packet */
>  #define IGC_RXD_STAT_IXSM      0x04    /* Ignore checksum */
>  #define IGC_RXD_STAT_UDPCS     0x10    /* UDP xsum calculated */
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 298f408519f4..0c481dc906ad 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -551,7 +551,6 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
>
>         /* initialize Rx descriptor 0 */
>         rx_desc = IGC_RX_DESC(ring, 0);
> -       rx_desc->wb.upper.length = 0;
>
>         /* enable receive descriptor fetching */
>         rxdctl |= IGC_RXDCTL_QUEUE_ENABLE;

This is initializing the ring for the first descriptor to 0. Without
this line you break the driver. Without this you need to memset the
entire descriptor ring to 0.

> @@ -1880,9 +1879,6 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count)
>                         i -= rx_ring->count;
>                 }
>
> -               /* clear the length for the next_to_use descriptor */
> -               rx_desc->wb.upper.length = 0;
> -
>                 cleaned_count--;
>         } while (cleaned_count);
>

Same here. Without doing this you can potentially hang as you need to
make sure the status bits are cleared before releasing a descriptor to
the device.

> @@ -1924,8 +1920,12 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>                 }
>
>                 rx_desc = IGC_RX_DESC(rx_ring, rx_ring->next_to_clean);
> -               size = le16_to_cpu(rx_desc->wb.upper.length);
> -               if (!size)
> +
> +               /* If we reached a descriptor with 'Descriptor Done' bit not
> +                * set, it means we have handled all descriptors owned by
> +                * software already so we should prematurely break the loop.
> +                */
> +               if (!igc_test_staterr(rx_desc, IGC_RXD_STAT_DD))
>                         break;
>
>                 /* This memory barrier is needed to keep us from reading

Why add an extra check when you don't need to? This doesn't make
sense. The DD bit tells you that the descriptor was written back but
you can do the same thing by reading the size field.

> @@ -1934,6 +1934,8 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>                  */
>                 dma_rmb();
>
> +               size = le16_to_cpu(rx_desc->wb.upper.length);
> +
>                 rx_buffer = igc_get_rx_buffer(rx_ring, size);
>
>                 /* retrieve a buffer from the ring */

This should be fine since the dma_rmb() will prevent the reads from
being reordered. However it is really redundant. For a bit of history
the reason for reading the size as the first field is because
previously we had bugs on PowerPC where the length field was being
read first, and then the DD bit. As such if the length is 0 before the
writeback, and non-zero after then you can spare yourself some cycles
by reading the size first and if it is non-zero then you know the
descriptor has valid data to be read.

[Intel-wired-lan] [PATCH 3/3] igc: Fix SRRCTL register setup

[Alexander Duyck]

On Mon, Aug 10, 2020 at 5:42 PM Andre Guedes <andre.guedes@intel.com> wrote:
>
> Quoting Alexander Duyck (2020-08-10 15:56:31)
> > > @@ -1869,6 +1866,7 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count)
> > >                  * because each write-back erases this info.
> > >                  */
> > >                 rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset);
> > > +               rx_desc->read.hdr_addr = 0;
> > >
> > >                 rx_desc++;
> > >                 bi++;
> >
> > If you are going to do this it would be better to replace the line
> > that is setting the length to zero instead of just adding this line.
> > That way you can avoid having to rewrite it. I only had bothered with
> > clearing the length field as it was a 32b field, however if you are
> > wanting to flush the full 64b then I would recommend doing it there
> > rather than here.
>
> Just to make sure I'm on the same page, do you mean to move this line to
> patch 2/3, right?

No, I hadn't had a chance to take a look at patch 2 yet. I think patch
2 is ill advised as the patch is currently broken, and even if done
correctly you don't get any benefit out of it that I can see. From
what I can tell this patch was likely covering up some of the errors
introduced in patch 2. Now that I see this in conjunction with patch 2
I would say you should probably just drop patch 2 and this one as well
since they aren't adding any real value other than removing a
redundant write which was just there to keep this mostly in sync with
how we did it for ixgbe.

The reason the driver path was coded the way it is in order to get
away from having to clear the entire descriptor after processing it
and to avoid having to explicitly track next_to_use and next_to_clean.
Instead we leave the descriptor as mostly read-only until we
reallocate the buffer and give it back to the device. All we have to
do is clear the length field of the next_to_use descriptor when we are
done allocating so that we do not overrun the descriptors when
cleaning. It makes it much easier to debug when the descriptors are
left in place as long as possible since we can then come back and look
at the memory and generally I have found performance is improved as we
are not having to dirty cachelines prematurely.

[Intel-wired-lan] [PATCH 2/3] igc: Check descriptor's DD bit in igc_clean_rx_irq()

[Andre Guedes]

Hi Alexander,

Quoting Alexander Duyck (2020-08-10 18:25:12)
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -551,7 +551,6 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
> >
> >         /* initialize Rx descriptor 0 */
> >         rx_desc = IGC_RX_DESC(ring, 0);
> > -       rx_desc->wb.upper.length = 0;
> >
> >         /* enable receive descriptor fetching */
> >         rxdctl |= IGC_RXDCTL_QUEUE_ENABLE;
> 
> This is initializing the ring for the first descriptor to 0. Without
> this line you break the driver. Without this you need to memset the
> entire descriptor ring to 0.
> 
> > @@ -1880,9 +1879,6 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count)
> >                         i -= rx_ring->count;
> >                 }
> >
> > -               /* clear the length for the next_to_use descriptor */
> > -               rx_desc->wb.upper.length = 0;
> > -
> >                 cleaned_count--;
> >         } while (cleaned_count);
> >
> 
> Same here. Without doing this you can potentially hang as you need to
> make sure the status bits are cleared before releasing a descriptor to
> the device.

Yes, after your first comment on patch 3/3 I realized this was bogus. Moving
the 'read.hdr_addr = 0' to this patch should fix it. That's why I thought you
were suggesting that.

> > @@ -1924,8 +1920,12 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >                 }
> >
> >                 rx_desc = IGC_RX_DESC(rx_ring, rx_ring->next_to_clean);
> > -               size = le16_to_cpu(rx_desc->wb.upper.length);
> > -               if (!size)
> > +
> > +               /* If we reached a descriptor with 'Descriptor Done' bit not
> > +                * set, it means we have handled all descriptors owned by
> > +                * software already so we should prematurely break the loop.
> > +                */
> > +               if (!igc_test_staterr(rx_desc, IGC_RXD_STAT_DD))
> >                         break;
> >
> >                 /* This memory barrier is needed to keep us from reading
> 
> Why add an extra check when you don't need to? This doesn't make
> sense. The DD bit tells you that the descriptor was written back but
> you can do the same thing by reading the size field.

I was using i40e and ice drivers as reference. If I'm reading it correctly,
they check the DD bit first (or similar) and then read the length information.
I don't see a strong reason besides making the code a bit more readable.

> > @@ -1934,6 +1934,8 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> >                  */
> >                 dma_rmb();
> >
> > +               size = le16_to_cpu(rx_desc->wb.upper.length);
> > +
> >                 rx_buffer = igc_get_rx_buffer(rx_ring, size);
> >
> >                 /* retrieve a buffer from the ring */
> 
> This should be fine since the dma_rmb() will prevent the reads from
> being reordered. However it is really redundant. For a bit of history
> the reason for reading the size as the first field is because
> previously we had bugs on PowerPC where the length field was being
> read first, and then the DD bit. As such if the length is 0 before the
> writeback, and non-zero after then you can spare yourself some cycles
> by reading the size first and if it is non-zero then you know the
> descriptor has valid data to be read.

Thanks for the history context.

Cheers,
Andre

[Intel-wired-lan] [PATCH 3/3] igc: Fix SRRCTL register setup

[Andre Guedes]

Quoting Alexander Duyck (2020-08-10 18:41:41)
> On Mon, Aug 10, 2020 at 5:42 PM Andre Guedes <andre.guedes@intel.com> wrote:
> >
> > Quoting Alexander Duyck (2020-08-10 15:56:31)
> > > > @@ -1869,6 +1866,7 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count)
> > > >                  * because each write-back erases this info.
> > > >                  */
> > > >                 rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset);
> > > > +               rx_desc->read.hdr_addr = 0;
> > > >
> > > >                 rx_desc++;
> > > >                 bi++;
> > >
> > > If you are going to do this it would be better to replace the line
> > > that is setting the length to zero instead of just adding this line.
> > > That way you can avoid having to rewrite it. I only had bothered with
> > > clearing the length field as it was a 32b field, however if you are
> > > wanting to flush the full 64b then I would recommend doing it there
> > > rather than here.
> >
> > Just to make sure I'm on the same page, do you mean to move this line to
> > patch 2/3, right?
> 
> No, I hadn't had a chance to take a look at patch 2 yet. I think patch
> 2 is ill advised as the patch is currently broken, and even if done
> correctly you don't get any benefit out of it that I can see. From
> what I can tell this patch was likely covering up some of the errors
> introduced in patch 2. Now that I see this in conjunction with patch 2
> I would say you should probably just drop patch 2 and this one as well
> since they aren't adding any real value other than removing a
> redundant write which was just there to keep this mostly in sync with
> how we did it for ixgbe.

What about not setting BSIZEHEADER bits since 'one buffer descriptor' option
is used in SRRCTL?

> The reason the driver path was coded the way it is in order to get
> away from having to clear the entire descriptor after processing it
> and to avoid having to explicitly track next_to_use and next_to_clean.
> Instead we leave the descriptor as mostly read-only until we
> reallocate the buffer and give it back to the device. All we have to
> do is clear the length field of the next_to_use descriptor when we are
> done allocating so that we do not overrun the descriptors when
> cleaning. It makes it much easier to debug when the descriptors are
> left in place as long as possible since we can then come back and look
> at the memory and generally I have found performance is improved as we
> are not having to dirty cachelines prematurely.

Thanks for the context.

- Andre

[Intel-wired-lan] [PATCH 2/3] igc: Check descriptor's DD bit in igc_clean_rx_irq()

[Alexander Duyck]

On Tue, Aug 11, 2020 at 10:00 AM Andre Guedes <andre.guedes@intel.com> wrote:
>
> Hi Alexander,
>
> Quoting Alexander Duyck (2020-08-10 18:25:12)
> > > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > > @@ -551,7 +551,6 @@ static void igc_configure_rx_ring(struct igc_adapter *adapter,
> > >
> > >         /* initialize Rx descriptor 0 */
> > >         rx_desc = IGC_RX_DESC(ring, 0);
> > > -       rx_desc->wb.upper.length = 0;
> > >
> > >         /* enable receive descriptor fetching */
> > >         rxdctl |= IGC_RXDCTL_QUEUE_ENABLE;
> >
> > This is initializing the ring for the first descriptor to 0. Without
> > this line you break the driver. Without this you need to memset the
> > entire descriptor ring to 0.
> >
> > > @@ -1880,9 +1879,6 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count)
> > >                         i -= rx_ring->count;
> > >                 }
> > >
> > > -               /* clear the length for the next_to_use descriptor */
> > > -               rx_desc->wb.upper.length = 0;
> > > -
> > >                 cleaned_count--;
> > >         } while (cleaned_count);
> > >
> >
> > Same here. Without doing this you can potentially hang as you need to
> > make sure the status bits are cleared before releasing a descriptor to
> > the device.
>
> Yes, after your first comment on patch 3/3 I realized this was bogus. Moving
> the 'read.hdr_addr = 0' to this patch should fix it. That's why I thought you
> were suggesting that.
>
> > > @@ -1924,8 +1920,12 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> > >                 }
> > >
> > >                 rx_desc = IGC_RX_DESC(rx_ring, rx_ring->next_to_clean);
> > > -               size = le16_to_cpu(rx_desc->wb.upper.length);
> > > -               if (!size)
> > > +
> > > +               /* If we reached a descriptor with 'Descriptor Done' bit not
> > > +                * set, it means we have handled all descriptors owned by
> > > +                * software already so we should prematurely break the loop.
> > > +                */
> > > +               if (!igc_test_staterr(rx_desc, IGC_RXD_STAT_DD))
> > >                         break;
> > >
> > >                 /* This memory barrier is needed to keep us from reading
> >
> > Why add an extra check when you don't need to? This doesn't make
> > sense. The DD bit tells you that the descriptor was written back but
> > you can do the same thing by reading the size field.
>
> I was using i40e and ice drivers as reference. If I'm reading it correctly,
> they check the DD bit first (or similar) and then read the length information.
> I don't see a strong reason besides making the code a bit more readable.

The i40e driver does the same thing as igb and ixgbe. The ice driver
doesn't but in the case of that driver the pkt_len field resides in
the first qword which is the same as the pkt_addr so they overlap and
the same approach cannot be used.

> > > @@ -1934,6 +1934,8 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
> > >                  */
> > >                 dma_rmb();
> > >
> > > +               size = le16_to_cpu(rx_desc->wb.upper.length);
> > > +
> > >                 rx_buffer = igc_get_rx_buffer(rx_ring, size);
> > >
> > >                 /* retrieve a buffer from the ring */
> >
> > This should be fine since the dma_rmb() will prevent the reads from
> > being reordered. However it is really redundant. For a bit of history
> > the reason for reading the size as the first field is because
> > previously we had bugs on PowerPC where the length field was being
> > read first, and then the DD bit. As such if the length is 0 before the
> > writeback, and non-zero after then you can spare yourself some cycles
> > by reading the size first and if it is non-zero then you know the
> > descriptor has valid data to be read.
>
> Thanks for the history context.

No problem, just glad I saw this before it went too far down the path
of undoing the work that was done in the past.

Thanks.

- Alex

[Intel-wired-lan] [PATCH 3/3] igc: Fix SRRCTL register setup

[Alexander Duyck]

On Tue, Aug 11, 2020 at 10:00 AM Andre Guedes <andre.guedes@intel.com> wrote:
>
> Quoting Alexander Duyck (2020-08-10 18:41:41)
> > On Mon, Aug 10, 2020 at 5:42 PM Andre Guedes <andre.guedes@intel.com> wrote:
> > >
> > > Quoting Alexander Duyck (2020-08-10 15:56:31)
> > > > > @@ -1869,6 +1866,7 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count)
> > > > >                  * because each write-back erases this info.
> > > > >                  */
> > > > >                 rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset);
> > > > > +               rx_desc->read.hdr_addr = 0;
> > > > >
> > > > >                 rx_desc++;
> > > > >                 bi++;
> > > >
> > > > If you are going to do this it would be better to replace the line
> > > > that is setting the length to zero instead of just adding this line.
> > > > That way you can avoid having to rewrite it. I only had bothered with
> > > > clearing the length field as it was a 32b field, however if you are
> > > > wanting to flush the full 64b then I would recommend doing it there
> > > > rather than here.
> > >
> > > Just to make sure I'm on the same page, do you mean to move this line to
> > > patch 2/3, right?
> >
> > No, I hadn't had a chance to take a look at patch 2 yet. I think patch
> > 2 is ill advised as the patch is currently broken, and even if done
> > correctly you don't get any benefit out of it that I can see. From
> > what I can tell this patch was likely covering up some of the errors
> > introduced in patch 2. Now that I see this in conjunction with patch 2
> > I would say you should probably just drop patch 2 and this one as well
> > since they aren't adding any real value other than removing a
> > redundant write which was just there to keep this mostly in sync with
> > how we did it for ixgbe.
>
> What about not setting BSIZEHEADER bits since 'one buffer descriptor' option
> is used in SRRCTL?

Does it cause some sort of problem to be populating it? If not I would
say leave it. It isn't too different from just populating the field
with 0 and should have no effect since the field is unused when in one
buffer mode.

> > The reason the driver path was coded the way it is in order to get
> > away from having to clear the entire descriptor after processing it
> > and to avoid having to explicitly track next_to_use and next_to_clean.
> > Instead we leave the descriptor as mostly read-only until we
> > reallocate the buffer and give it back to the device. All we have to
> > do is clear the length field of the next_to_use descriptor when we are
> > done allocating so that we do not overrun the descriptors when
> > cleaning. It makes it much easier to debug when the descriptors are
> > left in place as long as possible since we can then come back and look
> > at the memory and generally I have found performance is improved as we
> > are not having to dirty cachelines prematurely.
>
> Thanks for the context.
>
> - Andre

[Intel-wired-lan] [PATCH 3/3] igc: Fix SRRCTL register setup

[Andre Guedes]

Quoting Alexander Duyck (2020-08-11 11:04:52)
> On Tue, Aug 11, 2020 at 10:00 AM Andre Guedes <andre.guedes@intel.com> wrote:
> >
> > Quoting Alexander Duyck (2020-08-10 18:41:41)
> > > On Mon, Aug 10, 2020 at 5:42 PM Andre Guedes <andre.guedes@intel.com> wrote:
> > > >
> > > > Quoting Alexander Duyck (2020-08-10 15:56:31)
> > > > > > @@ -1869,6 +1866,7 @@ static void igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16 cleaned_count)
> > > > > >                  * because each write-back erases this info.
> > > > > >                  */
> > > > > >                 rx_desc->read.pkt_addr = cpu_to_le64(bi->dma + bi->page_offset);
> > > > > > +               rx_desc->read.hdr_addr = 0;
> > > > > >
> > > > > >                 rx_desc++;
> > > > > >                 bi++;
> > > > >
> > > > > If you are going to do this it would be better to replace the line
> > > > > that is setting the length to zero instead of just adding this line.
> > > > > That way you can avoid having to rewrite it. I only had bothered with
> > > > > clearing the length field as it was a 32b field, however if you are
> > > > > wanting to flush the full 64b then I would recommend doing it there
> > > > > rather than here.
> > > >
> > > > Just to make sure I'm on the same page, do you mean to move this line to
> > > > patch 2/3, right?
> > >
> > > No, I hadn't had a chance to take a look at patch 2 yet. I think patch
> > > 2 is ill advised as the patch is currently broken, and even if done
> > > correctly you don't get any benefit out of it that I can see. From
> > > what I can tell this patch was likely covering up some of the errors
> > > introduced in patch 2. Now that I see this in conjunction with patch 2
> > > I would say you should probably just drop patch 2 and this one as well
> > > since they aren't adding any real value other than removing a
> > > redundant write which was just there to keep this mostly in sync with
> > > how we did it for ixgbe.
> >
> > What about not setting BSIZEHEADER bits since 'one buffer descriptor' option
> > is used in SRRCTL?
> 
> Does it cause some sort of problem to be populating it? If not I would
> say leave it. It isn't too different from just populating the field
> with 0 and should have no effect since the field is unused when in one
> buffer mode.

No problem that I'm aware of. When I first came across that code I found it a
bit misleading since no header buffer is actually allocated by the driver. That
was the only motivation for this patch.

So Jeff/Tony please disregard patches 2 and 3 from this series. Let's move forward with
patch 1 only.

Thanks for the review, Alexander.

- Andre

[Intel-wired-lan] [PATCH 3/3] igc: Fix SRRCTL register setup

[Nguyen, Anthony L]

On Tue, 2020-08-11 at 11:38 -0700, Andre Guedes wrote:
> Quoting Alexander Duyck (2020-08-11 11:04:52)
> > On Tue, Aug 11, 2020 at 10:00 AM Andre Guedes <
> > andre.guedes at intel.com> wrote:
> > > 
> > > Quoting Alexander Duyck (2020-08-10 18:41:41)
> > > > On Mon, Aug 10, 2020 at 5:42 PM Andre Guedes <
> > > > andre.guedes at intel.com> wrote:
> > > > > 
> > > > > Quoting Alexander Duyck (2020-08-10 15:56:31)
> > > > > > > @@ -1869,6 +1866,7 @@ static void
> > > > > > > igc_alloc_rx_buffers(struct igc_ring *rx_ring, u16
> > > > > > > cleaned_count)
> > > > > > >                  * because each write-back erases this
> > > > > > > info.
> > > > > > >                  */
> > > > > > >                 rx_desc->read.pkt_addr = cpu_to_le64(bi-
> > > > > > > >dma + bi->page_offset);
> > > > > > > +               rx_desc->read.hdr_addr = 0;
> > > > > > > 
> > > > > > >                 rx_desc++;
> > > > > > >                 bi++;
> > > > > > 
> > > > > > If you are going to do this it would be better to replace
> > > > > > the line
> > > > > > that is setting the length to zero instead of just adding
> > > > > > this line.
> > > > > > That way you can avoid having to rewrite it. I only had
> > > > > > bothered with
> > > > > > clearing the length field as it was a 32b field, however if
> > > > > > you are
> > > > > > wanting to flush the full 64b then I would recommend doing
> > > > > > it there
> > > > > > rather than here.
> > > > > 
> > > > > Just to make sure I'm on the same page, do you mean to move
> > > > > this line to
> > > > > patch 2/3, right?
> > > > 
> > > > No, I hadn't had a chance to take a look at patch 2 yet. I
> > > > think patch
> > > > 2 is ill advised as the patch is currently broken, and even if
> > > > done
> > > > correctly you don't get any benefit out of it that I can see.
> > > > From
> > > > what I can tell this patch was likely covering up some of the
> > > > errors
> > > > introduced in patch 2. Now that I see this in conjunction with
> > > > patch 2
> > > > I would say you should probably just drop patch 2 and this one
> > > > as well
> > > > since they aren't adding any real value other than removing a
> > > > redundant write which was just there to keep this mostly in
> > > > sync with
> > > > how we did it for ixgbe.
> > > 
> > > What about not setting BSIZEHEADER bits since 'one buffer
> > > descriptor' option
> > > is used in SRRCTL?
> > 
> > Does it cause some sort of problem to be populating it? If not I
> > would
> > say leave it. It isn't too different from just populating the field
> > with 0 and should have no effect since the field is unused when in
> > one
> > buffer mode.
> 
> No problem that I'm aware of. When I first came across that code I
> found it a
> bit misleading since no header buffer is actually allocated by the
> driver. That
> was the only motivation for this patch.
> 
> So Jeff/Tony please disregard patches 2 and 3 from this series. Let's
> move forward with
> patch 1 only.

I will apply patch 1 and drop the other 2.

Thanks,
Tony

[Intel-wired-lan] [PATCH 1/3] igc: Clean RX descriptor error flags

[Brown, Aaron F]

> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Andre Guedes
> Sent: Monday, August 10, 2020 2:09 PM
> To: intel-wired-lan at lists.osuosl.org
> Subject: [Intel-wired-lan] [PATCH 1/3] igc: Clean RX descriptor error flags
> 
> I225 advanced receive descriptor doesn't have the following extend error
> bits: CE, SE, SEQ, CXE. In addition to that, the bit TCPE is called L4E
> in the datasheet.
> 
> This patch cleans up the code accordingly, and gets rid of the macro
> IGC_RXDEXT_ERR_FRAME_ERR_MASK since it doesn't make much sense
> anymore.
> 
> Signed-off-by: Andre Guedes <andre.guedes@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_defines.h | 14 +-------------
>  drivers/net/ethernet/intel/igc/igc_main.c    |  5 ++---
>  2 files changed, 3 insertions(+), 16 deletions(-)
> 
Tested-by: Aaron Brown <aaron.f.brown@intel.com>