diff mbox series

[v7,7/7] PCI: Skip Enhanced Allocation (EA) initialization for VF device

Message ID 0b86bafdeaf8293d6360bed1760586493935f16e.1567029860.git.sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show
Series Fix PF/VF dependency issue | expand

Commit Message

Kuppuswamy Sathyanarayanan Aug. 28, 2019, 10:14 p.m. UTC
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

As per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced Allocation
Capability. So skip pci_ea_init() for virtual devices.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <keith.busch@intel.com>
Suggested-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 drivers/pci/pci.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Kuppuswamy Sathyanarayanan Oct. 3, 2019, 5:21 p.m. UTC | #1
Hi Bjorn,

On 8/28/19 3:14 PM, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> As per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced Allocation
> Capability. So skip pci_ea_init() for virtual devices.
>
> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Keith Busch <keith.busch@intel.com>
> Suggested-by: Ashok Raj <ashok.raj@intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
This patch was also dropped in your v8. Is this also intentional?
> ---
>   drivers/pci/pci.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 1b27b5af3d55..266600a11769 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3025,6 +3025,13 @@ void pci_ea_init(struct pci_dev *dev)
>   	int offset;
>   	int i;
>   
> +	/*
> +	 * Per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced
> +	 * Allocation Capability.
> +	 */
> +	if (dev->is_virtfn)
> +		return;
> +
>   	/* find PCI EA capability in list */
>   	ea = pci_find_capability(dev, PCI_CAP_ID_EA);
>   	if (!ea)
Bjorn Helgaas Oct. 3, 2019, 6:57 p.m. UTC | #2
On Thu, Oct 03, 2019 at 10:21:24AM -0700, Kuppuswamy Sathyanarayanan wrote:
> Hi Bjorn,
> 
> On 8/28/19 3:14 PM, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > 
> > As per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced Allocation
> > Capability. So skip pci_ea_init() for virtual devices.
> > 
> > Cc: Ashok Raj <ashok.raj@intel.com>
> > Cc: Keith Busch <keith.busch@intel.com>
> > Suggested-by: Ashok Raj <ashok.raj@intel.com>
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> This patch was also dropped in your v8. Is this also intentional?

Yes, I dropped it because I didn't think there was much motivation for
it.

If a device is broken, i.e., a VF has an EA capability, this patch
silently returns.  The existing code would try to use the EA
capability and something would probably blow up, so in that sense,
this patch makes the hardware issue less visible.

If a device is correct, i.e., a VF does *not* have an EA capability,
pci_find_capability() will fail anyway, so this patch doesn't change
the functional behavior.

This patch *does* avoid the pci_find_capability() in that case, which
is a performance optimization.  We could merge it on that basis, but
we should try to quantify the benefit to see if it's really worthwhile
and the commit log should use that as the explicit motivation.

> > ---
> >   drivers/pci/pci.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 1b27b5af3d55..266600a11769 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -3025,6 +3025,13 @@ void pci_ea_init(struct pci_dev *dev)
> >   	int offset;
> >   	int i;
> > +	/*
> > +	 * Per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced
> > +	 * Allocation Capability.
> > +	 */
> > +	if (dev->is_virtfn)
> > +		return;
> > +
> >   	/* find PCI EA capability in list */
> >   	ea = pci_find_capability(dev, PCI_CAP_ID_EA);
> >   	if (!ea)
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux kernel developer
>
Ashok Raj Oct. 3, 2019, 7:20 p.m. UTC | #3
On Thu, Oct 03, 2019 at 01:57:47PM -0500, Bjorn Helgaas wrote:
> On Thu, Oct 03, 2019 at 10:21:24AM -0700, Kuppuswamy Sathyanarayanan wrote:
> > Hi Bjorn,
> > 
> > On 8/28/19 3:14 PM, sathyanarayanan.kuppuswamy@linux.intel.com wrote:
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > 
> > > As per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced Allocation
> > > Capability. So skip pci_ea_init() for virtual devices.
> > > 
> > > Cc: Ashok Raj <ashok.raj@intel.com>
> > > Cc: Keith Busch <keith.busch@intel.com>
> > > Suggested-by: Ashok Raj <ashok.raj@intel.com>
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > This patch was also dropped in your v8. Is this also intentional?
> 
> Yes, I dropped it because I didn't think there was much motivation for
> it.

Agreed!

> 
> If a device is broken, i.e., a VF has an EA capability, this patch
> silently returns.  The existing code would try to use the EA
> capability and something would probably blow up, so in that sense,
> this patch makes the hardware issue less visible.
> 
> If a device is correct, i.e., a VF does *not* have an EA capability,
> pci_find_capability() will fail anyway, so this patch doesn't change
> the functional behavior.


But do you think while at this can we atleast do a warning
to make sure HW probably messed up just after the EA capability
is read? Atleast it would be an early warning vs. it trying to break
later. Like the other issues we ran into with the PIN interrupt
accidently set in some hardware for VF's for instance. 

> 
> This patch *does* avoid the pci_find_capability() in that case, which
> is a performance optimization.  We could merge it on that basis, but
> we should try to quantify the benefit to see if it's really worthwhile
> and the commit log should use that as the explicit motivation.
> 
> > > ---
> > >   drivers/pci/pci.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 1b27b5af3d55..266600a11769 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -3025,6 +3025,13 @@ void pci_ea_init(struct pci_dev *dev)
> > >   	int offset;
> > >   	int i;
> > > +	/*
> > > +	 * Per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced
> > > +	 * Allocation Capability.
> > > +	 */
> > > +	if (dev->is_virtfn)
> > > +		return;
> > > +
> > >   	/* find PCI EA capability in list */
> > >   	ea = pci_find_capability(dev, PCI_CAP_ID_EA);
> > >   	if (!ea)
> > 
> > -- 
> > Sathyanarayanan Kuppuswamy
> > Linux kernel developer
> >
diff mbox series

Patch

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1b27b5af3d55..266600a11769 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3025,6 +3025,13 @@  void pci_ea_init(struct pci_dev *dev)
 	int offset;
 	int i;
 
+	/*
+	 * Per PCIe r4.0, sec 9.3.6, VF must not implement Enhanced
+	 * Allocation Capability.
+	 */
+	if (dev->is_virtfn)
+		return;
+
 	/* find PCI EA capability in list */
 	ea = pci_find_capability(dev, PCI_CAP_ID_EA);
 	if (!ea)