Re: PCI VPD checksum ambiguity

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Apr 01, 2025 at 11:51:07PM +0200, Heiner Kallweit wrote:
> On 01.04.2025 22:55, Bjorn Helgaas wrote:
> > Hi,
> > 
> > The PCIe spec is ambiguous about how the VPD checksum should be
> > computed, and resolving this ambiguity might break drivers.
> > 
> > PCIe r6.0 sec 6.27 says only the VPD-R list should be included in the
> > checksum:
> > 
> >   One VPD-R (10h) tag is used as a header for the read-only keywords.
> >   The VPD-R list (including tag and length) must checksum to zero.
> 
> This requirement I don't find in the PCI 3.0 spec, not sure with which
> version it was added. 

I think it's there; that same text is in PCI r3.0, Appendix I, about
five lines before I.1.

> Interestingly this doesn't even require the presence of a RV
> keyword. Or the presence of the RV keyword is implicitly assumed.

I.3.1.1 says RV is required, and I guess it has to be last in VPD-R to
cover any reserved space (as needed, I suppose to align to the VPD-W
area, which might be in a different chip).

> Maybe this part isn't meant literally. I can imagine they wanted to
> clarify that checksum calculation excludes the VPD-W area.
> And unfortunately they weren't precise enough, and introduced the
> ambiguity you found.
> 
> > But sec 6.27.2.2 says "all bytes in VPD ... up to the checksum byte":
> > 
> >   RV   The first byte of this item is a checksum byte. The checksum is
> >        correct if the sum of all bytes in VPD (from VPD address 0 up
> >        to and including this byte) is zero.
> 
> This one can be found identically in the PCI v3.0 spec already:
> 
> The checksum is correct if the sum of all bytes in VPD (from
> VPD address 0 up to and including this byte) is zero.
> 
> I don't think they want to break backwards-compatibility, therefore
> this requirement should still be valid.

Yes, and I think the RV description is more specific and is what I
would have used to implement it.

> > These are obviously different unless VPD-R happens to be the first
> > item in VPD.  But sec 6.27 and 6.27.2.1 suggest that the Identifier
> > String item should be the first item, preceding the VPD-R list:
> > 
> >   The first VPD tag is the Identifier String (02h) and provides the
> >   product name of the device. [6.27]
> > 
> >   Large resource type Identifier String (02h)
> > 
> >     This tag is the first item in the VPD storage component. It
> >     contains the name of the add-in card in alphanumeric characters.
> >     [6.27.2.1, Table 6-23]
> > 
> > I think pci_vpd_check_csum() follows sec 6.27.2.2: it sums all the
> > bytes in the buffer up to and including the checksum byte of the RV
> > keyword.  The range starts at 0, not at the beginning of the VPD-R
> > read-only list, so it likely includes the Identifier String.
> > 
> > As far as I can tell, only the broadcom/tg3 and chelsio/cxgb4/t4
> > drivers use pci_vpd_check_csum().  Of course, other drivers might
> > compute the checksum themselves.
> > 
> > Any thoughts on how this spec ambiguity should be resolved?
> > 
> > Any idea how devices in the field populate their VPD?
> > 
> > Can you share any VPD dumps from devices that include an RV keyword
> > item?
> 
> I have only very dated devices which likely date back to before
> the existence of PCIe r6.0. So their VPD dump may not really help.
> 
> IIRC there's an ongoing discussion regarding making VPD content
> user-readable on mlx5 devices. Maybe check with the Mellanox/Nvidia
> guys how they interpret the spec and implemented VPD checksumming.

Good idea, cc'd.