On Wed, Jan 12, 2022 at 11:34:02AM +0100, Pali Rohár wrote:
> On Wednesday 12 January 2022 10:42:50 Stefan Roese wrote:
> > From: Bharat Kumar Gogada <bharat.kumar.gogada@xxxxxxxxxx>
> >
> > Platforms may have dedicated IRQ lines for PCIe services like
> > AER/PME etc., check for such IRQ lines.
> > Check if platform has any dedicated IRQ lines for PCIe
> > services.
Use the terminology from the spec again ("platform-specific System
Errors").
> > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xxxxxxxxxx>
> > Signed-off-by: Stefan Roese <sr@xxxxxxx>
> > Tested-by: Stefan Roese <sr@xxxxxxx>
> > Cc: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> > Cc: Pali Rohár <pali@xxxxxxxxxx>
> > Cc: Michal Simek <michal.simek@xxxxxxxxxx>
> > ---
> > drivers/pci/pcie/portdrv_core.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> > index bda630889f95..70dd45671ed8 100644
> > --- a/drivers/pci/pcie/portdrv_core.c
> > +++ b/drivers/pci/pcie/portdrv_core.c
> > @@ -358,6 +358,14 @@ int pcie_port_device_register(struct pci_dev *dev)
> > }
> > }
> >
> > + /*
> > + * Some platforms have dedicated interrupt line from root complex to
> > + * interrupt controller for PCIe services like AER/PME etc., check
> > + * if platform registered with any such IRQ.
> > + */
> > + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> > + pci_check_platform_service_irqs(dev, irqs, capabilities);
> > +
>
> In my opinion calling this hook at this stage is too late. Few lines
> above is following code:
>
> if (irq_services) {
> /*
> * Initialize service IRQs. Don't use service devices that
> * require interrupts if there is no way to generate them.
> * However, some drivers may have a polling mode (e.g.
> * pciehp_poll_mode) that can be used in the absence of IRQs.
> * Allow them to determine if that is to be used.
> */
> status = pcie_init_service_irqs(dev, irqs, irq_services);
> if (status) {
> irq_services &= PCIE_PORT_SERVICE_HP;
> if (!irq_services)
> goto error_disable;
> }
> }
>
> Function pcie_init_service_irqs() fails if "dev" does not have any
> suitable interrupt. Which happens for devices / root ports without
> support for standard interrupts (INTx / MSI).
>
> I think that this new hook should have preference over
> pcie_init_service_irqs() and after this new hook should be
> pcie_init_service_irqs() called only for irq_services which new hook has
> not filled. And if at least new hook or pcie_init_service_irqs() passes
> then "error_disable" path should not be called.
I tend to agree. I expect that a host bridge will only use this new
mechanism if the standard INTx/MSI interrupts don't work.
I guess it's possible a device could use platform-specific errors for
some services and standard INTx/MSI for others, but unless we have an
example of that, I'm not sure it's worth trying to support that.
For now, I think it will be simpler to skip pcie_init_service_irqs()
completely if the platform-specific hook is successful.
Bjorn
