Re: [PATCH 2/6] PCI/VPD: Remove struct pci_vpd_ops

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Mark, Alex D, Jesse, Alex W]

On Sun, Aug 08, 2021 at 07:20:05PM +0200, Heiner Kallweit wrote:
> Code can be significantly simplified by removing struct pci_vpd_ops and
> avoiding the indirect calls.

I really like this patch.

Nothing to do with *this* patch, but I have a little heartburn about
the "access somebody else's VPD" approach.

I think the beginning of this was Mark's post [1].  IIUC, there are
Intel multifunction NICs that share some VPD hardware between
functions, and concurrent accesses to VPD of different functions
doesn't work correctly.

I'm pretty sure this is a defect per spec, because PCIe r5.0, sec
7.9.19 doesn't say anything about having to treat VPD on
multi-function devices specially.

The current solution is for all Intel multi-function NICs to redirect
VPD access to function 0.  That single-threads VPD access across all
the functions because we hold function 0's vpd->lock mutex while
reading or writing.

But I think we still have the problem that this implicit sharing of
function 0's VPD opens a channel between functions: if functions are
assigned to different VMs, the VMs can communicate by reading and
writing VPD.

So I wonder if we should just disallow VPD access for these NICs
except on function 0.  There was a little bit of discussion in that
direction at [2].

[1] https://lore.kernel.org/linux-pci/20150519000037.56109.68356.stgit@mdrustad-wks.jf.intel.com/
[2] https://lore.kernel.org/linux-pci/20150519160158.00002cd6@unknown/

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/pci/vpd.c | 90 ++++++++++++++++++-----------------------------
>  1 file changed, 34 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/pci/vpd.c b/drivers/pci/vpd.c
> index e87f299ee..6a0d617b2 100644
> --- a/drivers/pci/vpd.c
> +++ b/drivers/pci/vpd.c
> @@ -13,13 +13,7 @@
>  
>  /* VPD access through PCI 2.2+ VPD capability */
>  
> -struct pci_vpd_ops {
> -	ssize_t (*read)(struct pci_dev *dev, loff_t pos, size_t count, void *buf);
> -	ssize_t (*write)(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);
> -};
> -
>  struct pci_vpd {
> -	const struct pci_vpd_ops *ops;
>  	struct mutex	lock;
>  	unsigned int	len;
>  	u8		cap;
> @@ -123,11 +117,16 @@ static int pci_vpd_wait(struct pci_dev *dev, bool set)
>  static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
>  			    void *arg)
>  {
> -	struct pci_vpd *vpd = dev->vpd;
> +	struct pci_vpd *vpd;
>  	int ret = 0;
>  	loff_t end = pos + count;
>  	u8 *buf = arg;
>  
> +	if (!dev || !dev->vpd)
> +		return -ENODEV;
> +
> +	vpd = dev->vpd;
> +
>  	if (pos < 0)
>  		return -EINVAL;
>  
> @@ -189,11 +188,16 @@ static ssize_t pci_vpd_read(struct pci_dev *dev, loff_t pos, size_t count,
>  static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
>  			     const void *arg)
>  {
> -	struct pci_vpd *vpd = dev->vpd;
> +	struct pci_vpd *vpd;
>  	const u8 *buf = arg;
>  	loff_t end = pos + count;
>  	int ret = 0;
>  
> +	if (!dev || !dev->vpd)
> +		return -ENODEV;
> +
> +	vpd = dev->vpd;
> +
>  	if (pos < 0 || (pos & 3) || (count & 3))
>  		return -EINVAL;
>  
> @@ -238,44 +242,6 @@ static ssize_t pci_vpd_write(struct pci_dev *dev, loff_t pos, size_t count,
>  	return ret ? ret : count;
>  }
>  
> -static const struct pci_vpd_ops pci_vpd_ops = {
> -	.read = pci_vpd_read,
> -	.write = pci_vpd_write,
> -};
> -
> -static ssize_t pci_vpd_f0_read(struct pci_dev *dev, loff_t pos, size_t count,
> -			       void *arg)
> -{
> -	struct pci_dev *tdev = pci_get_func0_dev(dev);
> -	ssize_t ret;
> -
> -	if (!tdev)
> -		return -ENODEV;
> -
> -	ret = pci_read_vpd(tdev, pos, count, arg);
> -	pci_dev_put(tdev);
> -	return ret;
> -}
> -
> -static ssize_t pci_vpd_f0_write(struct pci_dev *dev, loff_t pos, size_t count,
> -				const void *arg)
> -{
> -	struct pci_dev *tdev = pci_get_func0_dev(dev);
> -	ssize_t ret;
> -
> -	if (!tdev)
> -		return -ENODEV;
> -
> -	ret = pci_write_vpd(tdev, pos, count, arg);
> -	pci_dev_put(tdev);
> -	return ret;
> -}
> -
> -static const struct pci_vpd_ops pci_vpd_f0_ops = {
> -	.read = pci_vpd_f0_read,
> -	.write = pci_vpd_f0_write,
> -};
> -
>  void pci_vpd_init(struct pci_dev *dev)
>  {
>  	struct pci_vpd *vpd;
> @@ -290,10 +256,6 @@ void pci_vpd_init(struct pci_dev *dev)
>  		return;
>  
>  	vpd->len = PCI_VPD_MAX_SIZE;
> -	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0)
> -		vpd->ops = &pci_vpd_f0_ops;
> -	else
> -		vpd->ops = &pci_vpd_ops;
>  	mutex_init(&vpd->lock);
>  	vpd->cap = cap;
>  	vpd->valid = 0;
> @@ -388,9 +350,17 @@ EXPORT_SYMBOL_GPL(pci_vpd_find_info_keyword);
>   */
>  ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf)
>  {
> -	if (!dev->vpd || !dev->vpd->ops)
> -		return -ENODEV;
> -	return dev->vpd->ops->read(dev, pos, count, buf);
> +	ssize_t ret;
> +
> +	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) {
> +		dev = pci_get_func0_dev(dev);
> +		ret = pci_vpd_read(dev, pos, count, buf);
> +		pci_dev_put(dev);
> +	} else {
> +		ret = pci_vpd_read(dev, pos, count, buf);
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(pci_read_vpd);
>  
> @@ -403,9 +373,17 @@ EXPORT_SYMBOL(pci_read_vpd);
>   */
>  ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf)
>  {
> -	if (!dev->vpd || !dev->vpd->ops)
> -		return -ENODEV;
> -	return dev->vpd->ops->write(dev, pos, count, buf);
> +	ssize_t ret;
> +
> +	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) {
> +		dev = pci_get_func0_dev(dev);
> +		ret = pci_vpd_write(dev, pos, count, buf);
> +		pci_dev_put(dev);
> +	} else {
> +		ret = pci_vpd_write(dev, pos, count, buf);
> +	}
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(pci_write_vpd);
>  
> -- 
> 2.32.0
> 
>