[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 at 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