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@xxxxxxxxxxxxxxx>
>>>
>>> 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
