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

[Heiner Kallweit <hkallweit1@gmail.com>]

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@linux.com>
> 
> 	Krzysztof
> 

Heiner