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

Thanks for the review, Alexander.

- Andre