[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