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@xxxxxxxxxx> > > > > > > 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@xxxxxxxxxx> > > > Signed-off-by: Stefan Roese <sr@xxxxxxx> > > > Cc: Bjorn Helgaas <helgaas@xxxxxxxxxx> > > > Cc: Pali Rohár <pali@xxxxxxxxxx> > > > Cc: Michal Simek <michal.simek@xxxxxxxxxx> > > > --- > > > 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 > > >
