Re: [PATCH 1/5] PCI/VPD: Refactor pci_vpd_size

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Hannes]

On Thu, May 13, 2021 at 10:58:40PM +0200, Heiner Kallweit wrote:
> The only Short Resource Data Type tag is the end tag. This allows to
> remove the generic SRDT tag handling and the code be significantly
> simplified.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/pci/vpd.c | 46 ++++++++++++----------------------------------
>  1 file changed, 12 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index 26bf7c877..ecdce170f 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -73,50 +73,28 @@ EXPORT_SYMBOL(pci_write_vpd);
>  static size_t pci_vpd_size(struct pci_dev *dev, size_t old_size)
>  {
>  	size_t off = 0;
> -	unsigned char header[1+2];	/* 1 byte tag, 2 bytes length */
> +	u8 header[3];	/* 1 byte tag, 2 bytes length */
>  
>  	while (off < old_size && pci_read_vpd(dev, off, 1, header) == 1) {
> -		unsigned char tag;
> -
>  		if (!header[0] && !off) {
>  			pci_info(dev, "Invalid VPD tag 00, assume missing optional VPD EPROM\n");
>  			return 0;
>  		}
>  
> -		if (header[0] & PCI_VPD_LRDT) {
> -			/* Large Resource Data Type Tag */
> -			tag = pci_vpd_lrdt_tag(header);
> -			/* Only read length from known tag items */
> -			if ((tag == PCI_VPD_LTIN_ID_STRING) ||
> -			    (tag == PCI_VPD_LTIN_RO_DATA) ||
> -			    (tag == PCI_VPD_LTIN_RW_DATA)) {
> -				if (pci_read_vpd(dev, off+1, 2,
> -						 &header[1]) != 2) {
> -					pci_warn(dev, "invalid large VPD tag %02x size at offset %zu",
> -						 tag, off + 1);
> -					return 0;
> -				}
> -				off += PCI_VPD_LRDT_TAG_SIZE +
> -					pci_vpd_lrdt_size(header);
> -			}
> -		} else {
> -			/* Short Resource Data Type Tag */
> -			off += PCI_VPD_SRDT_TAG_SIZE +
> -				pci_vpd_srdt_size(header);
> -			tag = pci_vpd_srdt_tag(header);
> -		}
> -
> -		if (tag == PCI_VPD_STIN_END)	/* End tag descriptor */
> -			return off;
> +		if (header[0] == PCI_VPD_SRDT_END)
> +			return off + PCI_VPD_SRDT_TAG_SIZE;

This makes the code beautiful.  But I think pci_vpd_size() is too
picky even now, and this patch makes it more so.

I don't know why pci_vpd_size() currently checks the tags for
ID_STRING, RO_DATA, and RW_DATA.  That seems too aggressive to me.

I think these data items originally came from PNP ISA, and it defines
several other tags.  Of course, that wasn't for PCI devices, but a
Google search for '"invalid" "vpd tag" "at offset"' does find several
cases where VPD contains things that look like PNP ISA data items.

I think we should compute the VPD size by iterating through it looking
only at the type (small or large) and the data item length until we
find the End Tag.

This code originally came from 104daa71b396 ("PCI: Determine actual
VPD size on first access"), so I added Hannes in case there was some
reason we do the extra validation.

> -		if ((tag != PCI_VPD_LTIN_ID_STRING) &&
> -		    (tag != PCI_VPD_LTIN_RO_DATA) &&
> -		    (tag != PCI_VPD_LTIN_RW_DATA)) {
> -			pci_warn(dev, "invalid %s VPD tag %02x at offset %zu",
> -				 (header[0] & PCI_VPD_LRDT) ? "large" : "short",
> -				 tag, off);
> +		if (header[0] != PCI_VPD_LRDT_ID_STRING &&
> +		    header[0] != PCI_VPD_LRDT_RO_DATA &&
> +		    header[0] != PCI_VPD_LRDT_RW_DATA) {
> +			pci_warn(dev, "invalid VPD tag %02x at offset %zu", header[0], off);
>  			return 0;
>  		}
> +
> +		if (pci_read_vpd(dev, off + 1, 2, header + 1) != 2)
> +			return 0;
> +
> +		off += PCI_VPD_LRDT_TAG_SIZE + pci_vpd_lrdt_size(header);
>  	}
>  	return 0;
>  }
> -- 
> 2.31.1
> 
>