Page MenuHomeFreeBSD

Remove spurious warning about invalid VPD data.
ClosedPublic

Authored by np on Feb 14 2020, 1:08 AM.
Tags
None
Referenced Files
F81641693: D23679.diff
Fri, Apr 19, 9:23 AM
Unknown Object (File)
Tue, Apr 9, 4:59 PM
Unknown Object (File)
Jan 13 2024, 11:18 AM
Unknown Object (File)
Dec 20 2023, 5:20 AM
Unknown Object (File)
Nov 27 2023, 10:52 PM
Unknown Object (File)
Nov 18 2023, 7:39 PM
Unknown Object (File)
Oct 28 2023, 4:15 AM
Unknown Object (File)
Oct 13 2023, 7:27 PM
Subscribers

Details

Summary

vrs.off is the offset from the start of the VPD and increments by 4
for every dword read. VPDs are not limited to 508B or 512B and it's not
clear what the (0x7f * 4 - vrs.off) is trying to do. remain is the
length of the next value.

Test Plan

Run "pciconf -lV" on many systems.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I see this with slightly modifed prints in and around the code being removed. This is with a card with a 512B VPD.

[56] vpd: val: 0x00000000, off: 256, bytesinval: 1, byte: 0x00, state: 3, remain: 2, name: 0x10, i: 164
[56] vpd: val: 0x00000000, off: 256, bytesinval: 0, byte: 0x00, state: 3, remain: 1, name: 0x10, i: 165
[56] vpd: val: 0x5200fc91, off: 260, bytesinval: 3, byte: 0x91, state: 0, remain: 0, name: 0x10, i: 167
[56] pci0:7:0:0: invalid VPD data, off 260, remain 252
[56] vpd: val: 0x00000052, off: 260, bytesinval: 0, byte: 0x52, state: 5, remain: 252, name: 0x11, i: 167
[56] vpd: val: 0x00000000, off: 264, bytesinval: 1, byte: 0x00, state: 6, remain: 249, name: 0x11, i: 0
[56] vpd: val: 0x00000000, off: 264, bytesinval: 0, byte: 0x00, state: 6, remain: 248, name: 0x11, i: 1
[56] vpd: val: 0x00000000, off: 268, bytesinval: 3, byte: 0x00, state: 6, remain: 247, name: 0x11, i: 2
[56] vpd: val: 0x00000000, off: 268, bytesinval: 2, byte: 0x00, state: 6, remain: 246, name: 0x11, i: 3

There's nothing wrong with the VPD as the length (byte 0x103 to byte 0x1fe, both inclusive) really is
0xfc (252). The last byte of the VPD is 0x78 as expected.

00f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
0100: 91 fc 00 52 57 f9 00 00 00 00 00 00 00 00 00 00 ...RW...........
0110: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
....
01e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
01f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 78 ...............x

Adding original author of the code as a reviewer.

sys/dev/pci/pci.c
1104 ↗(On Diff #68297)

I suspect this was an attempt to handle the F bit from the vpd cap address register. It should be 0x7fff instead of 0x7f*4, then.

sys/dev/pci/pci.c
1104 ↗(On Diff #68297)

Or to also avoid inaddressible locations. However, the "short" case would also need the same check since you could have a "short" tag at the end of the VPD area that could potentially overflow.

I would perhaps move this down to the bottom of this case before the 'switch (name)' (line 1115 in the old code in this review) so it tests both cases and code it something like:

if (vrs.off + remain - vrs.bytesinval > 0x8000) {
    pci_printf(cfg, "VPD data overflow, remain %#x\n", remain);
    state = -1;
    break;
}

The missing break in the old code isn't great either as you could potentially malloc() a really large value.

This revision is now accepted and ready to land.Mar 17 2020, 8:41 AM
This revision was automatically updated to reflect the committed changes.