Re: [RESEND PATCH v2 2/4] PCI: Add pci_check_platform_service_irqs

Stefan Roese
2022-01-13T09:08:38Z

On 1/12/22 17:42, Bjorn Helgaas wrote:
> On Wed, Jan 12, 2022 at 10:42:49AM +0100, Stefan Roese wrote:
>> From: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
>>
>> Adding method pci_check_platform_service_irqs to check if platform
>> has registered method to proivde dedicated IRQ lines for PCIe services
>> like AER.
>>
>> 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>
>> ---
>> include/linux/pci.h | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 291eadade811..d6812d596ecc 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -2420,6 +2420,24 @@ static inline bool pci_ari_enabled(struct pci_bus *bus)
>> return bus->self && bus->self->ari_enabled;
>> }
>>
>> +/**
>> + * pci_check_platform_service_irqs - check platform service irq's
>> + * @pdev: PCI Express device to check
>> + * @irqs: Array of irqs to populate
>> + * @mask: Bitmask of capabilities
>> + */
>> +static inline void pci_check_platform_service_irqs(struct pci_dev *dev,
>> + int *irqs, int mask)
>> +{
>> + struct pci_host_bridge *bridge;
>> +
>> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) {
>> + bridge = pci_find_host_bridge(dev->bus);
>> + if (bridge && bridge->setup_platform_service_irq)
>> + bridge->setup_platform_service_irq(bridge, irqs, mask);
>> + }
>> +}
>
> I don't think this needs to be in include/linux/pci.h; I think it
> should be in drivers/pci/pcie/portdrv_core.c where it is called.

Agreed.

> The name and signature should be parallel to pcie_init_service_irqs()
> since it is basically doing the same thing for platform-specific
> interrupts.

I've changed it now to pcie_init_platform_service_irqs(). Just let me
know if you had some other naming in mind.

> These patches are split up a little bit *too* much. Each one should
> add some piece of functionality. Currently they add declarations that
> are not used, functions that are not called, etc. That makes it hard
> to read one patch and get any idea of what it's for.

Agreed. Let me see, how this is best done. Most likely 2 patches, one
adding the new infrastructure to the PCIe subsystem and one adding the
"user" to the Xilinx PCIe controller driver.

Thanks,
Stefan