Re: [PATCH] PCI/VPD: Add simple sanity check to pci_vpd_size()

["Krzysztof Wilczyński" <kw@linux.com>]

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.

> > [...]
> >> +		/* 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.

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?

> > Reviewed-by: Krzysztof Wilczyński <kw@linux.com>

	Krzysztof