On 28.09.2021 00:29, Krzysztof Wilczyński wrote: > Hi Heiner, > >>> [...] >>>> Instead let's add a simple sanity check on the number of found tags. >>>> A VPD image conforming to the PCI spec can have max. 4 tags: >>>> id string, ro section, rw section, end tag. >>> >>> It's always nice to check if something is compliant with the specification. >>> >>> Would you be able to either cite this part of the official specification or >>> mention where to find it? Like we do in other such changes related to some >>> official standards, mainly for posterity to benefit others that might look >>> at this commit in the future. >>> >> Right, I should have mentioned that: >> PCI 3.0 I.3.1. VPD Large and Small Resource Data Tags > > Very nice! Do you have plans to send v2 that include this information or > you reckon this is something Bjorn could add when merging if he has the > time, of course. > Back from vacation .. I'll send a v2. >>> [...] >>>> + /* We can have max 4 tags: STRING_ID, RO, RW, END */ >>>> + if (++num_tags > 4) >>>> + goto error; >>> >>> Do we want to let someone know that their device (or a device they might >>> have in the system) has non-compliant and/or malformed VPD which is why we >>> decided to return an error? I wonder if this would help with >>> troubleshooting or just simply had some informative value. So perhaps >>> a warning or debug level message? What do you think? >>> >> A message is printed, see code after error label. We differentiate >> between "hard" and "soft" error. Soft error here means that the VPD EEPROM >> is optional, in such a case it's not an actual error that the VPD reads >> return non-VPD data. > > Got it. Thank you! > > I had a look and, does the following: > > pci_info(dev, "invalid VPD tag %#04x (size %zu) at offset %zu%s\n", > header[0], size, off, off == 0 ? > "; assume missing optional EEPROM" : ""); > > Still apply to having too many tags? Would the error make sense? Forgive > me for asking about this, especially as I am not a VPD expert, and was > simply wondering. > The message still is applicable, just that the tag now is invalid in a different sense. > Also, does pci_info() there makes sense? Not pci_warn() or pci_err(), just > so this message has more appropriate weight and logging level. What do you > think? > Only impact typically is that the vpd sysfs attribute isn't available. Userspace applications like lspci can deal with this and simply report "can't read vpd". I doubt that it's worth it to add more complexity here. >>> Reviewed-by: Krzysztof Wilczyński <kw@xxxxxxxxx> > > Krzysztof > Heiner
