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).
> > 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
