[PATCH v2] PCI/portdrv: Do not require an interrupt for all AER capable ports

[PATCH v2] PCI/portdrv: Do not require an interrupt for all AER capable ports

[Mika Westerberg]

Only Root Ports and Event Collectors use MSI for AER. PCIe Switch ports
or endpoints on the other hand only send messages (that get collected by
the former). For this reason do not require PCIe switch ports and
endpoints to use interrupt if they support AER.

This allows portdrv to attach PCIe switch ports of Intel DG1 and DG2
discrete graphics cards. These do not declare MSI or legacy interrupts.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Changes from v1:

 * Updated commit message to be more specific on which hardware this is
   needed.

 drivers/pci/pcie/portdrv_core.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 1ac7fec47d6f..1b1c386e50c4 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -164,7 +164,7 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
  */
 static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
 {
-	int ret, i;
+	int ret, i, type;
 
 	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
 		irqs[i] = -1;
@@ -177,6 +177,19 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
 	if ((mask & PCIE_PORT_SERVICE_PME) && pcie_pme_no_msi())
 		goto legacy_irq;
 
+	/*
+	 * Only root ports and event collectors use MSI for errors. Endpoints,
+	 * switch ports send messages to them but don't use MSI for that (PCIe
+	 * 5.0 sec 6.2.3.2).
+	 */
+	type = pci_pcie_type(dev);
+	if ((mask & PCIE_PORT_SERVICE_AER) &&
+	    type != PCI_EXP_TYPE_ROOT_PORT && type != PCI_EXP_TYPE_RC_EC)
+		mask &= ~PCIE_PORT_SERVICE_AER;
+
+	if (!mask)
+		return 0;
+
 	/* Try to use MSI-X or MSI if supported */
 	if (pcie_port_enable_irq_vec(dev, irqs, mask) == 0)
 		return 0;
-- 
2.35.1

Re: [PATCH v2] PCI/portdrv: Do not require an interrupt for all AER capable ports

[Sathyanarayanan Kuppuswamy]

On 12/7/22 12:41 AM, Mika Westerberg wrote:
> Only Root Ports and Event Collectors use MSI for AER. PCIe Switch ports
> or endpoints on the other hand only send messages (that get collected by
> the former). For this reason do not require PCIe switch ports and
> endpoints to use interrupt if they support AER.
> 
> This allows portdrv to attach PCIe switch ports of Intel DG1 and DG2
> discrete graphics cards. These do not declare MSI or legacy interrupts.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

> ---
> Changes from v1:
> 
>  * Updated commit message to be more specific on which hardware this is
>    needed.
> 
>  drivers/pci/pcie/portdrv_core.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 1ac7fec47d6f..1b1c386e50c4 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -164,7 +164,7 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
>   */
>  static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>  {
> -	int ret, i;
> +	int ret, i, type;
>  
>  	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
>  		irqs[i] = -1;
> @@ -177,6 +177,19 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>  	if ((mask & PCIE_PORT_SERVICE_PME) && pcie_pme_no_msi())
>  		goto legacy_irq;
>  
> +	/*
> +	 * Only root ports and event collectors use MSI for errors. Endpoints,
> +	 * switch ports send messages to them but don't use MSI for that (PCIe
> +	 * 5.0 sec 6.2.3.2).
> +	 */
> +	type = pci_pcie_type(dev);
> +	if ((mask & PCIE_PORT_SERVICE_AER) &&
> +	    type != PCI_EXP_TYPE_ROOT_PORT && type != PCI_EXP_TYPE_RC_EC)
> +		mask &= ~PCIE_PORT_SERVICE_AER;
> +
> +	if (!mask)
> +		return 0;
> +
>  	/* Try to use MSI-X or MSI if supported */
>  	if (pcie_port_enable_irq_vec(dev, irqs, mask) == 0)
>  		return 0;

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2] PCI/portdrv: Do not require an interrupt for all AER capable ports

[Bjorn Helgaas]

Hi Mika,

On Wed, Dec 07, 2022 at 10:41:05AM +0200, Mika Westerberg wrote:
> Only Root Ports and Event Collectors use MSI for AER. PCIe Switch ports
> or endpoints on the other hand only send messages (that get collected by
> the former). For this reason do not require PCIe switch ports and
> endpoints to use interrupt if they support AER.
> 
> This allows portdrv to attach PCIe switch ports of Intel DG1 and DG2
> discrete graphics cards. These do not declare MSI or legacy interrupts.

Help me understand more about this situation.  I guess we want portdrv
to attach not to a GPU itself, but to a switch port on the card that
*leads* to the GPU?

From the patch, it looks like the only PCIe port service this switch
port advertises is AER (not PME, DPC, hotplug, etc), and it doesn't
have MSI or MSI-X.

So aerdriver should be able to register for PCIE_PORT_SERVICE_AER, but
aer_probe() ignores everything except Root Ports and RCECs.  What's
the benefit then?  I must be missing something.

Bjorn

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
> Changes from v1:
> 
>  * Updated commit message to be more specific on which hardware this is
>    needed.
> 
>  drivers/pci/pcie/portdrv_core.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index 1ac7fec47d6f..1b1c386e50c4 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -164,7 +164,7 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask)
>   */
>  static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>  {
> -	int ret, i;
> +	int ret, i, type;
>  
>  	for (i = 0; i < PCIE_PORT_DEVICE_MAXSERVICES; i++)
>  		irqs[i] = -1;
> @@ -177,6 +177,19 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>  	if ((mask & PCIE_PORT_SERVICE_PME) && pcie_pme_no_msi())
>  		goto legacy_irq;
>  
> +	/*
> +	 * Only root ports and event collectors use MSI for errors. Endpoints,
> +	 * switch ports send messages to them but don't use MSI for that (PCIe
> +	 * 5.0 sec 6.2.3.2).
> +	 */
> +	type = pci_pcie_type(dev);
> +	if ((mask & PCIE_PORT_SERVICE_AER) &&
> +	    type != PCI_EXP_TYPE_ROOT_PORT && type != PCI_EXP_TYPE_RC_EC)
> +		mask &= ~PCIE_PORT_SERVICE_AER;
> +
> +	if (!mask)
> +		return 0;
> +
>  	/* Try to use MSI-X or MSI if supported */
>  	if (pcie_port_enable_irq_vec(dev, irqs, mask) == 0)
>  		return 0;
> -- 
> 2.35.1
> 

Re: [PATCH v2] PCI/portdrv: Do not require an interrupt for all AER capable ports

[Mika Westerberg]

On Wed, Dec 07, 2022 at 04:35:37PM -0600, Bjorn Helgaas wrote:
> Hi Mika,
> 
> On Wed, Dec 07, 2022 at 10:41:05AM +0200, Mika Westerberg wrote:
> > Only Root Ports and Event Collectors use MSI for AER. PCIe Switch ports
> > or endpoints on the other hand only send messages (that get collected by
> > the former). For this reason do not require PCIe switch ports and
> > endpoints to use interrupt if they support AER.
> > 
> > This allows portdrv to attach PCIe switch ports of Intel DG1 and DG2
> > discrete graphics cards. These do not declare MSI or legacy interrupts.
> 
> Help me understand more about this situation.  I guess we want portdrv
> to attach not to a GPU itself, but to a switch port on the card that
> *leads* to the GPU?

Yes correct.

> >From the patch, it looks like the only PCIe port service this switch
> port advertises is AER (not PME, DPC, hotplug, etc), and it doesn't
> have MSI or MSI-X.

Correct.

> So aerdriver should be able to register for PCIE_PORT_SERVICE_AER, but
> aer_probe() ignores everything except Root Ports and RCECs.  What's
> the benefit then?  I must be missing something.

The portdrv is needed for power management and everything else PCI even
if there is no actual "service" attached.

Re: [PATCH v2] PCI/portdrv: Do not require an interrupt for all AER capable ports

[Bjorn Helgaas]

On Thu, Dec 08, 2022 at 07:58:42AM +0200, Mika Westerberg wrote:
> On Wed, Dec 07, 2022 at 04:35:37PM -0600, Bjorn Helgaas wrote:
> > On Wed, Dec 07, 2022 at 10:41:05AM +0200, Mika Westerberg wrote:
> > > Only Root Ports and Event Collectors use MSI for AER. PCIe Switch ports
> > > or endpoints on the other hand only send messages (that get collected by
> > > the former). For this reason do not require PCIe switch ports and
> > > endpoints to use interrupt if they support AER.
> > > 
> > > This allows portdrv to attach PCIe switch ports of Intel DG1 and DG2
> > > discrete graphics cards. These do not declare MSI or legacy interrupts.
> > 
> > Help me understand more about this situation.  I guess we want portdrv
> > to attach not to a GPU itself, but to a switch port on the card that
> > *leads* to the GPU?
> 
> Yes correct.
> 
> > From the patch, it looks like the only PCIe port service this switch
> > port advertises is AER (not PME, DPC, hotplug, etc), and it doesn't
> > have MSI or MSI-X.
> 
> Correct.
> 
> > So aerdriver should be able to register for PCIE_PORT_SERVICE_AER, but
> > aer_probe() ignores everything except Root Ports and RCECs.  What's
> > the benefit then?  I must be missing something.
> 
> The portdrv is needed for power management and everything else PCI even
> if there is no actual "service" attached.

Thanks!  I'm trying to connect the dots to get to the specific bug fix
or improvement made by this patch.

The pcie_pme_driver itself doesn't seem involved because it registers
for PCIE_PORT_SERVICE_PME, and that's only set for Root Ports and
RCECs.

The pcie_portdrv_pm_ops would be another possibility, but with the
exception of pcie_port_runtime_idle(), all those ops just iterate over
service drivers, which aren't involved.

So I'm guessing this has to do with setting
DPM_FLAG_NO_DIRECT_COMPLETE and DPM_FLAG_SMART_SUSPEND in
pcie_portdrv_probe().  Does the lack of portdrv mean the GPU can't
suspend?  Does this reduce power consumption?  How would a user know
this change would help their system?

Bjorn

Re: [PATCH v2] PCI/portdrv: Do not require an interrupt for all AER capable ports

[Mika Westerberg]

Hi,

On Thu, Dec 08, 2022 at 06:23:49AM -0600, Bjorn Helgaas wrote:
> On Thu, Dec 08, 2022 at 07:58:42AM +0200, Mika Westerberg wrote:
> > On Wed, Dec 07, 2022 at 04:35:37PM -0600, Bjorn Helgaas wrote:
> > > On Wed, Dec 07, 2022 at 10:41:05AM +0200, Mika Westerberg wrote:
> > > > Only Root Ports and Event Collectors use MSI for AER. PCIe Switch ports
> > > > or endpoints on the other hand only send messages (that get collected by
> > > > the former). For this reason do not require PCIe switch ports and
> > > > endpoints to use interrupt if they support AER.
> > > > 
> > > > This allows portdrv to attach PCIe switch ports of Intel DG1 and DG2
> > > > discrete graphics cards. These do not declare MSI or legacy interrupts.
> > > 
> > > Help me understand more about this situation.  I guess we want portdrv
> > > to attach not to a GPU itself, but to a switch port on the card that
> > > *leads* to the GPU?
> > 
> > Yes correct.
> > 
> > > From the patch, it looks like the only PCIe port service this switch
> > > port advertises is AER (not PME, DPC, hotplug, etc), and it doesn't
> > > have MSI or MSI-X.
> > 
> > Correct.
> > 
> > > So aerdriver should be able to register for PCIE_PORT_SERVICE_AER, but
> > > aer_probe() ignores everything except Root Ports and RCECs.  What's
> > > the benefit then?  I must be missing something.
> > 
> > The portdrv is needed for power management and everything else PCI even
> > if there is no actual "service" attached.
> 
> Thanks!  I'm trying to connect the dots to get to the specific bug fix
> or improvement made by this patch.

Sure :)

> The pcie_pme_driver itself doesn't seem involved because it registers
> for PCIE_PORT_SERVICE_PME, and that's only set for Root Ports and
> RCECs.
> 
> The pcie_portdrv_pm_ops would be another possibility, but with the
> exception of pcie_port_runtime_idle(), all those ops just iterate over
> service drivers, which aren't involved.
> 
> So I'm guessing this has to do with setting
> DPM_FLAG_NO_DIRECT_COMPLETE and DPM_FLAG_SMART_SUSPEND in
> pcie_portdrv_probe().  Does the lack of portdrv mean the GPU can't
> suspend?  Does this reduce power consumption?  How would a user know
> this change would help their system?

If there is no driver attached to PCI device (this case upstream port)
it will not enter low power states. It allows GPU to suspend but it
does not allow the whole topology to enter low power states.

Re: [PATCH v2] PCI/portdrv: Do not require an interrupt for all AER capable ports

[Mika Westerberg]

On Thu, Dec 08, 2022 at 03:58:19PM +0200, Mika Westerberg wrote:
> If there is no driver attached to PCI device (this case upstream port)
> it will not enter low power states. It allows GPU to suspend but it
> does not allow the whole topology to enter low power states.

Ah, also in our case they saw the portdv probe fail in the logs.

Re: [PATCH v2] PCI/portdrv: Do not require an interrupt for all AER capable ports

[Bjorn Helgaas]

On Wed, Dec 07, 2022 at 10:41:05AM +0200, Mika Westerberg wrote:
> Only Root Ports and Event Collectors use MSI for AER. PCIe Switch ports
> or endpoints on the other hand only send messages (that get collected by
> the former). For this reason do not require PCIe switch ports and
> endpoints to use interrupt if they support AER.
> 
> This allows portdrv to attach PCIe switch ports of Intel DG1 and DG2
> discrete graphics cards. These do not declare MSI or legacy interrupts.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks for the additional info!  This seems like something we should
definitely do.

I'm wondering whether we should check for this in
get_port_device_capability().  It already has similar checks for
device type for other services.  This would skip pci_set_master() for
these non-RP, non-RCEC devices, which is probably harmless, since I
assume we only need that to make sure MSI works.

It would also prevent allocation of the AER service for non-RP,
non-RCEC devices.  That's also probably harmless, since aer_probe()
ignores those devices anyway.

What do you think of something like this?  (This is based on my
pci/portdrv branch which squashed everything into portdrv.c:
https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/portdrv)

diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index a6c4225505d5..8b16e96ec15c 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -232,7 +232,9 @@ static int get_port_device_capability(struct pci_dev *dev)
 	}
 
 #ifdef CONFIG_PCIEAER
-	if (dev->aer_cap && pci_aer_available() &&
+	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
+             pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
+	    dev->aer_cap && pci_aer_available() &&
 	    (pcie_ports_native || host->native_aer))
 		services |= PCIE_PORT_SERVICE_AER;
 #endif

Re: [PATCH v2] PCI/portdrv: Do not require an interrupt for all AER capable ports

[Sathyanarayanan Kuppuswamy]

On 12/9/22 9:07 AM, Bjorn Helgaas wrote:
> On Wed, Dec 07, 2022 at 10:41:05AM +0200, Mika Westerberg wrote:
>> Only Root Ports and Event Collectors use MSI for AER. PCIe Switch ports
>> or endpoints on the other hand only send messages (that get collected by
>> the former). For this reason do not require PCIe switch ports and
>> endpoints to use interrupt if they support AER.
>>
>> This allows portdrv to attach PCIe switch ports of Intel DG1 and DG2
>> discrete graphics cards. These do not declare MSI or legacy interrupts.
>>
>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Thanks for the additional info!  This seems like something we should
> definitely do.
> 
> I'm wondering whether we should check for this in
> get_port_device_capability().  It already has similar checks for
> device type for other services.  This would skip pci_set_master() for
> these non-RP, non-RCEC devices, which is probably harmless, since I
> assume we only need that to make sure MSI works.

Currently, we only have high level (cap or enable/disable) checks in 
get_port_device_capability(). Why bring in more AER specific checks
there and make it complicated? Is there any benefit in doing this?

> 
> It would also prevent allocation of the AER service for non-RP,
> non-RCEC devices.  That's also probably harmless, since aer_probe()
> ignores those devices anyway.
> 
> What do you think of something like this?  (This is based on my
> pci/portdrv branch which squashed everything into portdrv.c:
> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/portdrv)
> 
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index a6c4225505d5..8b16e96ec15c 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -232,7 +232,9 @@ static int get_port_device_capability(struct pci_dev *dev)
>  	}
>  
>  #ifdef CONFIG_PCIEAER
> -	if (dev->aer_cap && pci_aer_available() &&
> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> +             pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
> +	    dev->aer_cap && pci_aer_available() &&
>  	    (pcie_ports_native || host->native_aer))
>  		services |= PCIE_PORT_SERVICE_AER;
>  #endif

If you want to do it, will you remove the relevant check in AER driver
probe?


-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2] PCI/portdrv: Do not require an interrupt for all AER capable ports

[Bjorn Helgaas]

On Fri, Dec 09, 2022 at 01:04:22PM -0800, Sathyanarayanan Kuppuswamy wrote:
> On 12/9/22 9:07 AM, Bjorn Helgaas wrote:
> > On Wed, Dec 07, 2022 at 10:41:05AM +0200, Mika Westerberg wrote:
> >> Only Root Ports and Event Collectors use MSI for AER. PCIe Switch ports
> >> or endpoints on the other hand only send messages (that get collected by
> >> the former). For this reason do not require PCIe switch ports and
> >> endpoints to use interrupt if they support AER.
> >>
> >> This allows portdrv to attach PCIe switch ports of Intel DG1 and DG2
> >> discrete graphics cards. These do not declare MSI or legacy interrupts.
> >>
> >> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > 
> > Thanks for the additional info!  This seems like something we should
> > definitely do.
> > 
> > I'm wondering whether we should check for this in
> > get_port_device_capability().  It already has similar checks for
> > device type for other services.  This would skip pci_set_master() for
> > these non-RP, non-RCEC devices, which is probably harmless, since I
> > assume we only need that to make sure MSI works.
> 
> Currently, we only have high level (cap or enable/disable) checks in 
> get_port_device_capability(). Why bring in more AER specific checks
> there and make it complicated? Is there any benefit in doing this?

Thanks a lot for taking a look!

I agree, I hate how complicated the expressions in
get_port_device_capability() are, but I don't think my idea is
significantly worse than what's already there.

Here's some existing code from get_port_device_capability():

        /* Root Ports and Root Complex Event Collectors may generate PMEs */
        if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
             pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
            (pcie_ports_native || host->native_pme)) {
                services |= PCIE_PORT_SERVICE_PME;

And here's what the corresponding AER code would look like:

        if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
             pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
            dev->aer_cap && pci_aer_available() &&
            (pcie_ports_native || host->native_aer))
                services |= PCIE_PORT_SERVICE_AER;

I do have some ideas about simplifying these, see below.

The benefits would be to make similar checks in the same place, avoid
setting Bus Master when we don't need it, and remove the AER child
service for non-RP/RCECs (it wouldn't appear in sysfs and they
wouldn't be eligible for PCIE_PORT_SERVICE_AER registration).

> > It would also prevent allocation of the AER service for non-RP,
> > non-RCEC devices.  That's also probably harmless, since aer_probe()
> > ignores those devices anyway.
> > 
> > What do you think of something like this?  (This is based on my
> > pci/portdrv branch which squashed everything into portdrv.c:
> > https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/portdrv)
> > 
> > diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> > index a6c4225505d5..8b16e96ec15c 100644
> > --- a/drivers/pci/pcie/portdrv.c
> > +++ b/drivers/pci/pcie/portdrv.c
> > @@ -232,7 +232,9 @@ static int get_port_device_capability(struct pci_dev *dev)
> >  	}
> >  
> >  #ifdef CONFIG_PCIEAER
> > -	if (dev->aer_cap && pci_aer_available() &&
> > +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
> > +             pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
> > +	    dev->aer_cap && pci_aer_available() &&
> >  	    (pcie_ports_native || host->native_aer))
> >  		services |= PCIE_PORT_SERVICE_AER;
> >  #endif
> 
> If you want to do it, will you remove the relevant check in AER driver
> probe?

That would be a good idea, although I was hoping to squeeze this into
v6.2, and I would probably postpone the rest until the next cycle.

I think aer_probe() could also be simplified by dropping the
set_downstream_devices_error_reporting() stuff.  pci_aer_init()
already takes care of that, IIUC, and that's a more natural place for
it since it handles the hot-add case.

There may also be opportunity to simplify some of these ugly checks of
pci_aer_available(), pcie_ports_native, and host->native_aer that are
littered all over the place by doing them in pci_aer_init() and
setting dev->aer_cap only if they are satisfied.

Bjorn

Re: [PATCH v2] PCI/portdrv: Do not require an interrupt for all AER capable ports

[Sathyanarayanan Kuppuswamy]

On 12/9/22 1:48 PM, Bjorn Helgaas wrote:
> On Fri, Dec 09, 2022 at 01:04:22PM -0800, Sathyanarayanan Kuppuswamy wrote:
>> On 12/9/22 9:07 AM, Bjorn Helgaas wrote:
>>> On Wed, Dec 07, 2022 at 10:41:05AM +0200, Mika Westerberg wrote:
>>>> Only Root Ports and Event Collectors use MSI for AER. PCIe Switch ports
>>>> or endpoints on the other hand only send messages (that get collected by
>>>> the former). For this reason do not require PCIe switch ports and
>>>> endpoints to use interrupt if they support AER.
>>>>
>>>> This allows portdrv to attach PCIe switch ports of Intel DG1 and DG2
>>>> discrete graphics cards. These do not declare MSI or legacy interrupts.
>>>>
>>>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>>>
>>> Thanks for the additional info!  This seems like something we should
>>> definitely do.
>>>
>>> I'm wondering whether we should check for this in
>>> get_port_device_capability().  It already has similar checks for
>>> device type for other services.  This would skip pci_set_master() for
>>> these non-RP, non-RCEC devices, which is probably harmless, since I
>>> assume we only need that to make sure MSI works.
>>
>> Currently, we only have high level (cap or enable/disable) checks in 
>> get_port_device_capability(). Why bring in more AER specific checks
>> there and make it complicated? Is there any benefit in doing this?
> 
> Thanks a lot for taking a look!
> 
> I agree, I hate how complicated the expressions in
> get_port_device_capability() are, but I don't think my idea is
> significantly worse than what's already there.
> 
> Here's some existing code from get_port_device_capability():
> 
>         /* Root Ports and Root Complex Event Collectors may generate PMEs */
>         if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
>              pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
>             (pcie_ports_native || host->native_pme)) {
>                 services |= PCIE_PORT_SERVICE_PME;
> 
> And here's what the corresponding AER code would look like:
> 
>         if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
>              pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
>             dev->aer_cap && pci_aer_available() &&
>             (pcie_ports_native || host->native_aer))
>                 services |= PCIE_PORT_SERVICE_AER;
> 
> I do have some ideas about simplifying these, see below.
> 
> The benefits would be to make similar checks in the same place, avoid
> setting Bus Master when we don't need it, and remove the AER child
> service for non-RP/RCECs (it wouldn't appear in sysfs and they
> wouldn't be eligible for PCIE_PORT_SERVICE_AER registration).

I did not notice the PME part. But if possible, we should simplify
these conditions in the future.

I have no objections about this change.

> 
>>> It would also prevent allocation of the AER service for non-RP,
>>> non-RCEC devices.  That's also probably harmless, since aer_probe()
>>> ignores those devices anyway.
>>>
>>> What do you think of something like this?  (This is based on my
>>> pci/portdrv branch which squashed everything into portdrv.c:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/portdrv)
>>>
>>> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
>>> index a6c4225505d5..8b16e96ec15c 100644
>>> --- a/drivers/pci/pcie/portdrv.c
>>> +++ b/drivers/pci/pcie/portdrv.c
>>> @@ -232,7 +232,9 @@ static int get_port_device_capability(struct pci_dev *dev)
>>>  	}
>>>  
>>>  #ifdef CONFIG_PCIEAER
>>> -	if (dev->aer_cap && pci_aer_available() &&
>>> +	if ((pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT ||
>>> +             pci_pcie_type(dev) == PCI_EXP_TYPE_RC_EC) &&
>>> +	    dev->aer_cap && pci_aer_available() &&
>>>  	    (pcie_ports_native || host->native_aer))
>>>  		services |= PCIE_PORT_SERVICE_AER;
>>>  #endif
>>
>> If you want to do it, will you remove the relevant check in AER driver
>> probe?
> 
> That would be a good idea, although I was hoping to squeeze this into
> v6.2, and I would probably postpone the rest until the next cycle.
> 
> I think aer_probe() could also be simplified by dropping the
> set_downstream_devices_error_reporting() stuff.  pci_aer_init()
> already takes care of that, IIUC, and that's a more natural place for
> it since it handles the hot-add case.

Agree. Since pci_ear_init() already configures the AER bits for all
devices, repeating it in aer_probe() is redundant. 

> 
> There may also be opportunity to simplify some of these ugly checks of
> pci_aer_available(), pcie_ports_native, and host->native_aer that are
> littered all over the place by doing them in pci_aer_init() and
> setting dev->aer_cap only if they are satisfied.
> 
> Bjorn

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer