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.
Details
Run "pciconf -lV" on many systems.
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 29370 Build 27266: arc lint + arc unit
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
sys/dev/pci/pci.c | ||
---|---|---|
1104 | 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 | 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. |