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

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

On Monday 30 May 2022 10:18:41 Stefan Roese wrote:
> On 28.05.22 02:09, Bjorn Helgaas wrote:
> > In subject line, I assume you mean "System Errors" instead of "Service
> > Errors"?
> 
> Background: I took over submitting this patchset from Bharat. Here his
> last revision:
> https://www.spinics.net/lists/kernel/msg2960164.html
> 
> Just to explain, that I didn't choose the naming.
> 
> To answer your question I personally think too, that "System Errors" is
> more appropriate than "Service Errors". But still this patchset replaces
> or better enhances the already present pcie_init_service_irqs() by a
> platform-specific version. I can only suspect, that this is the
> reasoning for this "Service" naming.

Hello! Based on the below text "Here the quote from Bharat's original
cover letter:" I think that the better naming should be: "Service
interrupts". Because it adds support for interrupts from PCIe services
like AER, PME or HP. Only AER are errors, other IRQs are just services.

> > 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().
> 
> Interesting, thanks. Again, I didn't write the original commit text,
> so my comments are a bit "limited" here. Perhaps Bharat might have
> something add here?
> 
> > > 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.
> 
> Yes.
> 
> > > + * 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?
> 
> Bharat?
> 
> > 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?
> 
> Here the quote from Bharat's original cover letter:
> "Some platforms have dedicated IRQ lines for PCIe services like AER/PME
> etc. The root complex on these platform will use these seperate IRQ
> lines to report AER/PME etc., interrupts and will not generate MSI/
> MSI-X/INTx interrupts for these services.

This is the best explanation of this change.

> These patches will add new method for these kind of platforms to
> register the platform IRQ number with respective PCIe services."
> 
> To sum it up, on our Xilinx ZynqMP platform this patch series is needed
> to deliver AER related interrupts. As this SoC needs this platform
> specific IRQ line for signalling of these events.
> 
> > > +	 */
> > > +	status = pcie_init_platform_service_irqs(dev, irqs, capabilities);
> > > +
> > > +	/*
> > > +	 * Only install service irqs, when the platform-specific hook was
> > > +	 * unsuccessful
> > 
> > s/irqs/IRQs/ again.
> 
> Yes.
> 
> Thanks,
> Stefan
> 
> > > +	 */
> > > +	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
> > >