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

["Pali Rohár" <pali@kernel.org>]

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

>  	/* Allocate child services if any */
>  	status = -ENODEV;
>  	nr_service = 0;
> -- 
> 2.34.1
>