Re: [RESEND PATCH v2 3/4] PCI/portdrv: Check platform supported service IRQ's

Stefan Roese
2022-01-13T09:05:13Z

On 1/12/22 17:36, Bjorn Helgaas wrote:
> 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@xilinx.com>
>>>
>>> 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").

Ok.

>>> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>> Tested-by: Stefan Roese <sr@denx.de>
>>> Cc: Bjorn Helgaas <helgaas@kernel.org>
>>> Cc: Pali Rohár <pali@kernel.org>
>>> Cc: Michal Simek <michal.simek@xilinx.com>
>>> ---
>>> 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.

Understood. I'll make the necessary changes in v3.

Thanks,
Stefan