Re: [PATCH v4 1/2] PCI/portdrv: Add option to setup IRQs for platform-specific Service Errors

[Bjorn Helgaas <helgaas@kernel.org>]

In subject line, I assume you mean "System Errors" instead of "Service
Errors"?

On Fri, Jan 14, 2022 at 08:58:33AM +0100, Stefan Roese wrote:
> From: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> 
> As per section 6.2.4.1.2, 6.2.6 in PCIe r4.0 (and later versions),
> platform-specific System Errors like AER can be delivered via platform-
> specific interrupt lines.

IIUC, this refers to the top left branch in Figure 6-3 of PCIe r6.0,
sec 6.2.6, which shows "System Error (platform specific)" controlled
by "System Error Enables (one per error class) in the Root Control
register," i.e., the PCI_EXP_RTCTL_SECEE, PCI_EXP_RTCTL_SENFEE, and
PCI_EXP_RTCTL_SEFEE bits.

Where are those enable bits set?  The only references I see are to
them being cleared via SYSTEM_ERROR_INTR_ON_MESG_MASK in
aer_enable_rootport().

> This patch adds the init_platform_service_irqs() hook to struct
> pci_host_bridge, making it possible that platforms may implement this
> function to hook IRQs for these platform-specific System Errors, like
> AER.
> 
> If these platform-specific service IRQs have been successfully
> installed via pcie_init_platform_service_irqs(),
> pcie_init_service_irqs() is skipped.
> 
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xilinx.com>
> Signed-off-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 | 39 ++++++++++++++++++++++++++++++++-
>  include/linux/pci.h             |  2 ++
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
> index e7dcb1f23210..27b990cedb4c 100644
> --- a/drivers/pci/pcie/portdrv_core.c
> +++ b/drivers/pci/pcie/portdrv_core.c
> @@ -190,6 +190,31 @@ static int pcie_init_service_irqs(struct pci_dev *dev, int *irqs, int mask)
>  	return 0;
>  }
>  
> +/**
> + * pcie_init_platform_service_irqs - initialize platform service irqs for
> + * platform-specific System Errors
> + * @dev: PCI Express port to handle
> + * @irqs: Array of irqs to populate
> + * @mask: Bitmask of capabilities

s/irqs/IRQs/ above (twice) for consistency.

> + * Return value: -ENODEV, in case no platform-specific IRQ is available
> + */
> +static int pcie_init_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->init_platform_service_irqs) {
> +			return bridge->init_platform_service_irqs(dev, irqs,
> +								  mask);
> +		}
> +	}
> +
> +	return -ENODEV;
> +}
> +
>  /**
>   * get_port_device_capability - discover capabilities of a PCI Express port
>   * @dev: PCI Express port to examine
> @@ -335,7 +360,19 @@ int pcie_port_device_register(struct pci_dev *dev)
>  		irq_services |= PCIE_PORT_SERVICE_DPC;
>  	irq_services &= capabilities;
>  
> -	if (irq_services) {
> +	/*
> +	 * Some platforms have dedicated interrupts from root complex to
> +	 * interrupt controller for PCIe platform-specific System Errors
> +	 * like AER/PME etc., check if the platform registered with any such
> +	 * IRQ.

I don't see "PME etc" mentioned in the spec sections you cite.
6.2.4.1.2 and 6.2.6 only cover interrupts in response to error
Messages.  Are there other sections that cover PME and whatever other
interrupts you have in mind?

6.7.3.4 ("Software Notification of Hot-Plug Events") talks about PME
and Hot-Plug Event interrupts, but these aren't errors, and I only see
signaling via INTx, MSI, or MSI-X.  Is there provision for a different
method?

> +	 */
> +	status = pcie_init_platform_service_irqs(dev, irqs, capabilities);
> +
> +	/*
> +	 * Only install service irqs, when the platform-specific hook was
> +	 * unsuccessful

s/irqs/IRQs/ again.

> +	 */
> +	if (irq_services && status) {
>  		/*
>  		 * Initialize service IRQs. Don't use service devices that
>  		 * require interrupts if there is no way to generate them.
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 18a75c8e615c..fb8aad3cb460 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -554,6 +554,8 @@ struct pci_host_bridge {
>  	u8 (*swizzle_irq)(struct pci_dev *, u8 *); /* Platform IRQ swizzler */
>  	int (*map_irq)(const struct pci_dev *, u8, u8);
>  	void (*release_fn)(struct pci_host_bridge *);
> +	int (*init_platform_service_irqs)(struct pci_dev *dev, int *irqs,
> +					  int plat_mask);
>  	void		*release_data;
>  	unsigned int	ignore_reset_delay:1;	/* For entire hierarchy */
>  	unsigned int	no_ext_tags:1;		/* No Extended Tags */
> -- 
> 2.34.1
>