Re: [PATCH v3 3/9] vfio: selftests: igb: Use advanced TX and RX descriptors

[Alex Williamson <alex@shazbot.org>]

On Wed, 08 Jul 2026 23:29:25 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Programming `SRRCTL` with `E1000_SRRCTL_DESCTYPE_ADV_ONEBUF` overwrites the register, clearing the `BSIZEPACKET` (Receive Buffer Size) field to 0, which violates the 82576 datasheet.
> - [Medium] The commit message incorrectly claims to add new header macros and remove legacy CMD macros, but the patch contains no such header modifications.
> --
> 
> --- Patch [3]: [PATCH v3 3/9] vfio: selftests: igb: Use advanced TX and RX descriptors ---
> commit 44f58d3e0349aa95894e8741ae9ff09ee319dcae
> Author: Alex Williamson <alex.williamson@nvidia.com>
> 
> vfio: selftests: igb: Use advanced TX and RX descriptors
> 
> The submitted driver builds a partial legacy TX descriptor (just
> DTALEN | CMD_EOP) and never programs SRRCTL.DESCTYPE.  QEMU's emulated
> igb tolerates this by treating descriptors as advanced regardless of
> DESCTYPE, but real 82576 hardware does not.
> 
> [ ... ]
> 
> >     Remove the legacy CMD macros (IGB_TXD_CMD_EOP, IGB_TXD_CMD_IFCS,
> >     IGB_TXD_CMD_RS, IGB_TXD_CMD_SHIFT, IGB_TXD_CMD_LEGACY_FORMAT) that
> >     become unused, and add the SRRCTL register offset, DESCTYPE encoding,
> >     and advanced TX descriptor field macros.  
> 
> [Severity: Medium]
> Does the patch actually include these macro changes?
> 
> The commit message claims to remove legacy CMD macros and add new ones, but
> there are no header modifications in the diff.

Entire paragraph is stale after including E1000 headers, drop.
 
> > diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > index 58c6aceea63a9..f4f6d2ee262da 100644
> > --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > @@ -235,6 +235,17 @@ static void igb_init(struct vfio_pci_device *device)
> >  	igb_write32(igb, E1000_RDLEN(0), RING_SIZE * sizeof(struct igb_rx_desc));
> >  	igb_write32(igb, E1000_RDH(0), 0);
> >  	igb_write32(igb, E1000_RDT(0), 0);
> > +
> > +	/*
> > +	 * Select the advanced one-buffer descriptor format.  Per 82576
> > +	 * datasheet section 7.1.5.2: "SRRCTL[n].DESCTYPE must be set to a
> > +	 * value other than 000b for the 82576 to write back the special
> > +	 * descriptors."  struct igb_rx_desc matches the advanced one-buffer
> > +	 * writeback layout (section 7.1.5.2), so polling rx.wb.status_error
> > +	 * requires this format.  Section 8.10.2 specifies DESCTYPE[27:25].
> > +	 */
> > +	igb_write32(igb, E1000_SRRCTL(0), E1000_SRRCTL_DESCTYPE_ADV_ONEBUF);  
> 
> [Severity: Medium]
> Will this overwrite the SRRCTL register and clear the BSIZEPACKET field to 0?
> 
> By clearing BSIZEPACKET (bits 6:0 of SRRCTL), real 82576 hardware may see a
> 0-byte receive buffer and truncate or drop received packets, causing the test
> to fail on hardware.

Same false positive as flagged and rebutted[1] last time.  If we
extrapolate that a human reviewer might have the same confusion, it
might be worth expanding this comment to include:

  +      *
  +      * The direct write also zeroes SRRCTL.BSIZEPACKET, which is           
  +      * intentional: per section 7.1.3.1 a zero BSIZEPACKET falls back to   
  +      * the RCTL.BSIZE buffer size, whose reset default (00b) is 2048       
  +      * bytes -- ample for the loopback frames here.

Thanks,
Alex

[1]https://lore.kernel.org/all/20260529142439.5bd03e5f@nvidia.com/