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

[Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

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