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

[Stefan Roese <sr@denx.de>]

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