[PATCH] PCI: Relax function 0 VPD test and relocate

[PATCH] PCI: Relax function 0 VPD test and relocate

[Alex Williamson]

When we quirk a device with PCI_DEV_FLAGS_VPD_REF_F0 we're expecting
to find a device where all the functions are identical.  If we don't
find that, we don't make VPD accessible through pci_vpd_ops.  That
means that if we quirk devices we shouldn't, we filter them out by
hiding VPD entirely rather than allowing default access.  Instead, we
can flip this around to only quirk devices that match a slightly more
rigorous test in the quirk, allowing regular access for anything else.

Tests for the multifunction flag are removed since a) function 0 and
the function under test are clearly a multifunction device if we're
scanning a non-zero function in the same slot and b) at this point the
flag is only set in the device under test if the multifunction bit is
set in the PCI HEADER, which is a point of interpretation for the PCI
spec.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

This is potentially another stable candiate since we're continuing to
iterate on 932c435caba8, but since we don't actually know of a device
where VPD is blocked (we don't think my Skylake example actually
supports VPD), I'm not including it.  I would support it if requested
though.

 drivers/pci/access.c |   22 ----------------------
 drivers/pci/quirks.c |   20 ++++++++++++++++++--
 2 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 5a5f0a7..59ac36f 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -475,23 +475,6 @@ static const struct pci_vpd_ops pci_vpd_f0_ops = {
 	.release = pci_vpd_pci22_release,
 };
 
-static int pci_vpd_f0_dev_check(struct pci_dev *dev)
-{
-	struct pci_dev *tdev = pci_get_slot(dev->bus,
-					    PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
-	int ret = 0;
-
-	if (!tdev)
-		return -ENODEV;
-	if (!tdev->vpd || !tdev->multifunction ||
-	    dev->class != tdev->class || dev->vendor != tdev->vendor ||
-	    dev->device != tdev->device)
-		ret = -ENODEV;
-
-	pci_dev_put(tdev);
-	return ret;
-}
-
 int pci_vpd_pci22_init(struct pci_dev *dev)
 {
 	struct pci_vpd_pci22 *vpd;
@@ -500,12 +483,7 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
 	cap = pci_find_capability(dev, PCI_CAP_ID_VPD);
 	if (!cap)
 		return -ENODEV;
-	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) {
-		int ret = pci_vpd_f0_dev_check(dev);
 
-		if (ret)
-			return ret;
-	}
 	vpd = kzalloc(sizeof(*vpd), GFP_ATOMIC);
 	if (!vpd)
 		return -ENOMEM;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 6a30252..b03373f 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1907,11 +1907,27 @@ static void quirk_netmos(struct pci_dev *dev)
 DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_NETMOS, PCI_ANY_ID,
 			 PCI_CLASS_COMMUNICATION_SERIAL, 8, quirk_netmos);
 
+/*
+ * Quirk non-zero PCI functions to route VPD access through function 0 for
+ * devices that share VPD resources between functions.  The functions are
+ * expected to be identical devices.
+ */
 static void quirk_f0_vpd_link(struct pci_dev *dev)
 {
-	if (!dev->multifunction || !PCI_FUNC(dev->devfn))
+	struct pci_dev *f0;
+
+	if (!PCI_FUNC(dev->devfn))
 		return;
-	dev->dev_flags |= PCI_DEV_FLAGS_VPD_REF_F0;
+
+	f0 = pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
+	if (!f0)
+		return;
+
+	if (f0->vpd && dev->class == f0->class &&
+	    dev->vendor == f0->vendor && dev->device == f0->device)
+		dev->dev_flags |= PCI_DEV_FLAGS_VPD_REF_F0;
+
+	pci_dev_put(f0);
 }
 DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
 			      PCI_CLASS_NETWORK_ETHERNET, 8, quirk_f0_vpd_link);

Re: [PATCH] PCI: Relax function 0 VPD test and relocate

[Myron Stowe]

On Tue, Sep 15, 2015 at 10:24 PM, Alex Williamson
<alex.williamson@redhat.com> wrote:
> When we quirk a device with PCI_DEV_FLAGS_VPD_REF_F0 we're expecting
> to find a device where all the functions are identical.  If we don't
> find that, we don't make VPD accessible through pci_vpd_ops.  That
> means that if we quirk devices we shouldn't, we filter them out by
> hiding VPD entirely rather than allowing default access.  Instead, we
> can flip this around to only quirk devices that match a slightly more
> rigorous test in the quirk, allowing regular access for anything else.
>
> Tests for the multifunction flag are removed since a) function 0 and
> the function under test are clearly a multifunction device if we're
> scanning a non-zero function in the same slot and b) at this point the
> flag is only set in the device under test if the multifunction bit is
> set in the PCI HEADER, which is a point of interpretation for the PCI
> spec.
>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>
> This is potentially another stable candiate since we're continuing to
> iterate on 932c435caba8, but since we don't actually know of a device
> where VPD is blocked (we don't think my Skylake example actually
> supports VPD), I'm not including it.  I would support it if requested
> though.
>
>  drivers/pci/access.c |   22 ----------------------
>  drivers/pci/quirks.c |   20 ++++++++++++++++++--
>  2 files changed, 18 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 5a5f0a7..59ac36f 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -475,23 +475,6 @@ static const struct pci_vpd_ops pci_vpd_f0_ops = {
>         .release = pci_vpd_pci22_release,
>  };
>
> -static int pci_vpd_f0_dev_check(struct pci_dev *dev)
> -{
> -       struct pci_dev *tdev = pci_get_slot(dev->bus,
> -                                           PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
> -       int ret = 0;
> -
> -       if (!tdev)
> -               return -ENODEV;
> -       if (!tdev->vpd || !tdev->multifunction ||
> -           dev->class != tdev->class || dev->vendor != tdev->vendor ||
> -           dev->device != tdev->device)
> -               ret = -ENODEV;
> -
> -       pci_dev_put(tdev);
> -       return ret;
> -}
> -
>  int pci_vpd_pci22_init(struct pci_dev *dev)
>  {
>         struct pci_vpd_pci22 *vpd;
> @@ -500,12 +483,7 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
>         cap = pci_find_capability(dev, PCI_CAP_ID_VPD);
>         if (!cap)
>                 return -ENODEV;
> -       if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) {
> -               int ret = pci_vpd_f0_dev_check(dev);
>
> -               if (ret)
> -                       return ret;
> -       }
>         vpd = kzalloc(sizeof(*vpd), GFP_ATOMIC);
>         if (!vpd)
>                 return -ENOMEM;
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 6a30252..b03373f 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -1907,11 +1907,27 @@ static void quirk_netmos(struct pci_dev *dev)
>  DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_NETMOS, PCI_ANY_ID,
>                          PCI_CLASS_COMMUNICATION_SERIAL, 8, quirk_netmos);
>
> +/*
> + * Quirk non-zero PCI functions to route VPD access through function 0 for
> + * devices that share VPD resources between functions.  The functions are
> + * expected to be identical devices.
> + */
>  static void quirk_f0_vpd_link(struct pci_dev *dev)
>  {
> -       if (!dev->multifunction || !PCI_FUNC(dev->devfn))
> +       struct pci_dev *f0;
> +
> +       if (!PCI_FUNC(dev->devfn))
>                 return;
> -       dev->dev_flags |= PCI_DEV_FLAGS_VPD_REF_F0;
> +
> +       f0 = pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
> +       if (!f0)
> +               return;
> +
> +       if (f0->vpd && dev->class == f0->class &&
> +           dev->vendor == f0->vendor && dev->device == f0->device)
> +               dev->dev_flags |= PCI_DEV_FLAGS_VPD_REF_F0;
> +
> +       pci_dev_put(f0);
>  }
>  DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
>                               PCI_CLASS_NETWORK_ETHERNET, 8, quirk_f0_vpd_link);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Acked-by: Myron Stowe <myron.stowe@redhat.com>

Re: [PATCH] PCI: Relax function 0 VPD test and relocate

[Rustad, Mark D]

[-- Attachment #1: Type: text/plain, Size: 1551 bytes --]


> On Sep 15, 2015, at 9:24 PM, Alex Williamson <alex.williamson@redhat.com> wrote:
> 
> When we quirk a device with PCI_DEV_FLAGS_VPD_REF_F0 we're expecting
> to find a device where all the functions are identical.  If we don't
> find that, we don't make VPD accessible through pci_vpd_ops.  That
> means that if we quirk devices we shouldn't, we filter them out by
> hiding VPD entirely rather than allowing default access.  Instead, we
> can flip this around to only quirk devices that match a slightly more
> rigorous test in the quirk, allowing regular access for anything else.
> 
> Tests for the multifunction flag are removed since a) function 0 and
> the function under test are clearly a multifunction device if we're
> scanning a non-zero function in the same slot and b) at this point the
> flag is only set in the device under test if the multifunction bit is
> set in the PCI HEADER, which is a point of interpretation for the PCI
> spec.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> This is potentially another stable candiate since we're continuing to
> iterate on 932c435caba8, but since we don't actually know of a device
> where VPD is blocked (we don't think my Skylake example actually
> supports VPD), I'm not including it.  I would support it if requested
> though.

This looks good to me. I can't really test the cases it addresses, but it seems reasonable.
Acked-by: Mark Rustad <mark.d.rustad@intel.com>

--
Mark Rustad, Networking Division, Intel Corporation


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 841 bytes --]

Re: [PATCH] PCI: Relax function 0 VPD test and relocate

[Bjorn Helgaas]

On Tue, Sep 15, 2015 at 10:24:46PM -0600, Alex Williamson wrote:
> When we quirk a device with PCI_DEV_FLAGS_VPD_REF_F0 we're expecting
> to find a device where all the functions are identical.  If we don't
> find that, we don't make VPD accessible through pci_vpd_ops.  That
> means that if we quirk devices we shouldn't, we filter them out by
> hiding VPD entirely rather than allowing default access.  Instead, we
> can flip this around to only quirk devices that match a slightly more
> rigorous test in the quirk, allowing regular access for anything else.
> 
> Tests for the multifunction flag are removed since a) function 0 and
> the function under test are clearly a multifunction device if we're
> scanning a non-zero function in the same slot and b) at this point the
> flag is only set in the device under test if the multifunction bit is
> set in the PCI HEADER, which is a point of interpretation for the PCI
> spec.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> This is potentially another stable candiate since we're continuing to
> iterate on 932c435caba8, but since we don't actually know of a device
> where VPD is blocked (we don't think my Skylake example actually
> supports VPD), I'm not including it.  I would support it if requested
> though.

Applied with acks from Myron & Mark to for-linus for v4.3 with the
changelog below.  I added a stable tag on the theory that there's not
really any benefit in having different flavors of this brand-new code.

    PCI: Use function 0 VPD for identical functions, regular VPD for others
    
    932c435caba8 ("PCI: Add dev_flags bit to access VPD through function 0")
    added PCI_DEV_FLAGS_VPD_REF_F0.  Previously, we set the flag on every
    non-zero function of quirked devices.  If a function turned out to be
    different from function 0, i.e., it had a different class, vendor ID, or
    device ID, the flag remained set but we didn't make VPD accessible at all.
    
    Flip this around so we only set PCI_DEV_FLAGS_VPD_REF_F0 for functions that
    are identical to function 0, and allow regular VPD access for any other
    functions.
    
    [bhelgaas: changelog, stable tag]
    Fixes: 932c435caba8 ("PCI: Add dev_flags bit to access VPD through function 0")
    Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
    Signed-off-by: Bjorn Helgaas <helgaas@kernel.org>
    Acked-by: Myron Stowe <myron.stowe@redhat.com>
    Acked-by: Mark Rustad <mark.d.rustad@intel.com>
    CC: stable@vger.kernel.org

> 
>  drivers/pci/access.c |   22 ----------------------
>  drivers/pci/quirks.c |   20 ++++++++++++++++++--
>  2 files changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 5a5f0a7..59ac36f 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -475,23 +475,6 @@ static const struct pci_vpd_ops pci_vpd_f0_ops = {
>  	.release = pci_vpd_pci22_release,
>  };
>  
> -static int pci_vpd_f0_dev_check(struct pci_dev *dev)
> -{
> -	struct pci_dev *tdev = pci_get_slot(dev->bus,
> -					    PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
> -	int ret = 0;
> -
> -	if (!tdev)
> -		return -ENODEV;
> -	if (!tdev->vpd || !tdev->multifunction ||
> -	    dev->class != tdev->class || dev->vendor != tdev->vendor ||
> -	    dev->device != tdev->device)
> -		ret = -ENODEV;
> -
> -	pci_dev_put(tdev);
> -	return ret;
> -}
> -
>  int pci_vpd_pci22_init(struct pci_dev *dev)
>  {
>  	struct pci_vpd_pci22 *vpd;
> @@ -500,12 +483,7 @@ int pci_vpd_pci22_init(struct pci_dev *dev)
>  	cap = pci_find_capability(dev, PCI_CAP_ID_VPD);
>  	if (!cap)
>  		return -ENODEV;
> -	if (dev->dev_flags & PCI_DEV_FLAGS_VPD_REF_F0) {
> -		int ret = pci_vpd_f0_dev_check(dev);
>  
> -		if (ret)
> -			return ret;
> -	}
>  	vpd = kzalloc(sizeof(*vpd), GFP_ATOMIC);
>  	if (!vpd)
>  		return -ENOMEM;
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 6a30252..b03373f 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -1907,11 +1907,27 @@ static void quirk_netmos(struct pci_dev *dev)
>  DECLARE_PCI_FIXUP_CLASS_HEADER(PCI_VENDOR_ID_NETMOS, PCI_ANY_ID,
>  			 PCI_CLASS_COMMUNICATION_SERIAL, 8, quirk_netmos);
>  
> +/*
> + * Quirk non-zero PCI functions to route VPD access through function 0 for
> + * devices that share VPD resources between functions.  The functions are
> + * expected to be identical devices.
> + */
>  static void quirk_f0_vpd_link(struct pci_dev *dev)
>  {
> -	if (!dev->multifunction || !PCI_FUNC(dev->devfn))
> +	struct pci_dev *f0;
> +
> +	if (!PCI_FUNC(dev->devfn))
>  		return;
> -	dev->dev_flags |= PCI_DEV_FLAGS_VPD_REF_F0;
> +
> +	f0 = pci_get_slot(dev->bus, PCI_DEVFN(PCI_SLOT(dev->devfn), 0));
> +	if (!f0)
> +		return;
> +
> +	if (f0->vpd && dev->class == f0->class &&
> +	    dev->vendor == f0->vendor && dev->device == f0->device)
> +		dev->dev_flags |= PCI_DEV_FLAGS_VPD_REF_F0;
> +
> +	pci_dev_put(f0);
>  }
>  DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
>  			      PCI_CLASS_NETWORK_ETHERNET, 8, quirk_f0_vpd_link);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html