| Message ID | 20220114075834.1938409-2-sr@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | Add support to register platform service IRQ | expand |
On Friday 14 January 2022 08:58:33 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. > > 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> Reviewed-by: Pali Rohár <pali@kernel.org> > --- > 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 > + * > + * 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. > + */ > + status = pcie_init_platform_service_irqs(dev, irqs, capabilities); > + > + /* > + * Only install service irqs, when the platform-specific hook was > + * unsuccessful > + */ > + 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 >
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 >
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. > 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. 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 >>
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 > > >
On Mon, May 30, 2022 at 10:32:06AM +0200, Pali Rohár wrote: > 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 Here's the link to the more usable lore archive: https://lore.kernel.org/all/1542206878-24587-1-git-send-email-bharat.kumar.gogada@xilinx.com/ > > 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. The question I'm trying to answer is whether this series concerns the "System Error" mechanism or the "Error Interrupt" mechanism. We should figure out which one this is and use the correct name. The sec 6.2.4.1.2 cited below clearly refers to the AER Root Error Command register, which controls interrupt generation via INTx, MSI, or MSI-X, i.e., the standard "Error Interrupt" shown on the RIGHT side of Figure 6-3 in sec 6.2.6. The "System Error" signaling on the LEFT side of Figure 6-3 would be controlled by the Root Control register in the PCIe capability. It should be easy to use setpci to set/clear these two sets of enable bits and figure out which path is of interest here. > > > 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. > > > ... > > > 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. As far as I can tell, "dedicated IRQ lines for services like AER/PME etc" would violate the PCIe spec. That's OK, we can work around that sort of thing, but it needs to be clearly called out as some kind of quirk and not mixed in with things like System Error signaling, which is allowed to be platform-specific. Bjorn
On Tuesday 31 May 2022 16:31:58 Bjorn Helgaas wrote: > On Mon, May 30, 2022 at 10:32:06AM +0200, Pali Rohár wrote: > > 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 > > Here's the link to the more usable lore archive: > https://lore.kernel.org/all/1542206878-24587-1-git-send-email-bharat.kumar.gogada@xilinx.com/ > > > > 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. > > The question I'm trying to answer is whether this series concerns the > "System Error" mechanism or the "Error Interrupt" mechanism. We > should figure out which one this is and use the correct name. My understanding is that neither "System Error", nor "Error Interrupt". But patch series is about "dedicated IRQ lines for services". One of PCIe service is AER, which is one of the mostly used PCIe service in kernel and therefore it is the root of confusions with keyword "error". > The sec 6.2.4.1.2 cited below clearly refers to the AER Root Error > Command register, which controls interrupt generation via INTx, MSI, > or MSI-X, i.e., the standard "Error Interrupt" shown on the RIGHT side > of Figure 6-3 in sec 6.2.6. > > The "System Error" signaling on the LEFT side of Figure 6-3 would be > controlled by the Root Control register in the PCIe capability. > > It should be easy to use setpci to set/clear these two sets of enable > bits and figure out which path is of interest here. > > > > > 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. > > > > > ... > > > > 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. > > As far as I can tell, "dedicated IRQ lines for services like AER/PME > etc" would violate the PCIe spec. I think too, that this does not conform to PCIe spec. > That's OK, we can work around that > sort of thing, but it needs to be clearly called out as some kind of > quirk and not mixed in with things like System Error signaling, which > is allowed to be platform-specific. > > Bjorn I agree.
On 31.05.22 23:31, Bjorn Helgaas wrote: > On Mon, May 30, 2022 at 10:32:06AM +0200, Pali Rohár wrote: >> 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 > > Here's the link to the more usable lore archive: > https://lore.kernel.org/all/1542206878-24587-1-git-send-email-bharat.kumar.gogada@xilinx.com/ > >>> 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. > > The question I'm trying to answer is whether this series concerns the > "System Error" mechanism or the "Error Interrupt" mechanism. We > should figure out which one this is and use the correct name. > > The sec 6.2.4.1.2 cited below clearly refers to the AER Root Error > Command register, which controls interrupt generation via INTx, MSI, > or MSI-X, i.e., the standard "Error Interrupt" shown on the RIGHT side > of Figure 6-3 in sec 6.2.6. > > The "System Error" signaling on the LEFT side of Figure 6-3 would be > controlled by the Root Control register in the PCIe capability. "System Error" is probably incorrect. You've already stated, that these error bits are generally disabled in the PCI_EXP_RTCTL reg in aer_enable_rootport(): /* Disable system error generation in response to error messages */ pcie_capability_clear_word(pdev, PCI_EXP_RTCTL, SYSTEM_ERROR_INTR_ON_MESG_MASK); This leaves "Error Interrupt", but I might be wrong here. > It should be easy to use setpci to set/clear these two sets of enable > bits and figure out which path is of interest here. Here the value of the PCI_EXP_RTCTL register at runtime: # setpci -v -s 00:00.0 CAP_EXP+0x1c.w 0000:00:00.0 (cap 10 @60) @7c = 0010 So all "System Error" enable bits are disabled. Please let me know if I should make some other "setpci" tests. >>>> 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. > >>>> ... >>>> 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. > > As far as I can tell, "dedicated IRQ lines for services like AER/PME > etc" would violate the PCIe spec. AFAICT this is the case here. > That's OK, we can work around that > sort of thing, but it needs to be clearly called out as some kind of > quirk and not mixed in with things like System Error signaling, which > is allowed to be platform-specific. Agreed. So how to process with this patchset? Should I reword the patch subject line (and the commit text and comments) to something like: Add option to setup IRQs for platform-specific Error Interrupt ? Thanks, Stefan
On Wednesday 01 June 2022 13:47:12 Stefan Roese wrote: > On 31.05.22 23:31, Bjorn Helgaas wrote: > > On Mon, May 30, 2022 at 10:32:06AM +0200, Pali Rohár wrote: > > > 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 > > > > Here's the link to the more usable lore archive: > > https://lore.kernel.org/all/1542206878-24587-1-git-send-email-bharat.kumar.gogada@xilinx.com/ > > > > > > 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. > > > > The question I'm trying to answer is whether this series concerns the > > "System Error" mechanism or the "Error Interrupt" mechanism. We > > should figure out which one this is and use the correct name. > > > > The sec 6.2.4.1.2 cited below clearly refers to the AER Root Error > > Command register, which controls interrupt generation via INTx, MSI, > > or MSI-X, i.e., the standard "Error Interrupt" shown on the RIGHT side > > of Figure 6-3 in sec 6.2.6. > > > > The "System Error" signaling on the LEFT side of Figure 6-3 would be > > controlled by the Root Control register in the PCIe capability. > > "System Error" is probably incorrect. You've already stated, that > these error bits are generally disabled in the PCI_EXP_RTCTL reg in > aer_enable_rootport(): > > /* Disable system error generation in response to error messages */ > pcie_capability_clear_word(pdev, PCI_EXP_RTCTL, > SYSTEM_ERROR_INTR_ON_MESG_MASK); > > This leaves "Error Interrupt", but I might be wrong here. > > > It should be easy to use setpci to set/clear these two sets of enable > > bits and figure out which path is of interest here. > > Here the value of the PCI_EXP_RTCTL register at runtime: > # setpci -v -s 00:00.0 CAP_EXP+0x1c.w > 0000:00:00.0 (cap 10 @60) @7c = 0010 > > So all "System Error" enable bits are disabled. > > Please let me know if I should make some other "setpci" tests. > > > > > > 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. > > > > > > > ... > > > > > 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. > > > > As far as I can tell, "dedicated IRQ lines for services like AER/PME > > etc" would violate the PCIe spec. > > AFAICT this is the case here. > > > That's OK, we can work around that > > sort of thing, but it needs to be clearly called out as some kind of > > quirk and not mixed in with things like System Error signaling, which > > is allowed to be platform-specific. > > Agreed. So how to process with this patchset? Should I reword the > patch subject line (and the commit text and comments) to something like: > > Add option to setup IRQs for platform-specific Error Interrupt ? > > Thanks, > Stefan I think it should be named "Service Interrupt" or something like that as it is bound only to _errors_ but to any platform-specific interrupt.
On Wed, Jun 01, 2022 at 01:47:12PM +0200, Stefan Roese wrote: > On 31.05.22 23:31, Bjorn Helgaas wrote: > > On Mon, May 30, 2022 at 10:32:06AM +0200, Pali Rohár wrote: > > > 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 > > > > Here's the link to the more usable lore archive: > > https://lore.kernel.org/all/1542206878-24587-1-git-send-email-bharat.kumar.gogada@xilinx.com/ > > > > > > 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. > > > > The question I'm trying to answer is whether this series concerns the > > "System Error" mechanism or the "Error Interrupt" mechanism. We > > should figure out which one this is and use the correct name. > > > > The sec 6.2.4.1.2 cited below clearly refers to the AER Root Error > > Command register, which controls interrupt generation via INTx, MSI, > > or MSI-X, i.e., the standard "Error Interrupt" shown on the RIGHT side > > of Figure 6-3 in sec 6.2.6. > > > > The "System Error" signaling on the LEFT side of Figure 6-3 would be > > controlled by the Root Control register in the PCIe capability. > > "System Error" is probably incorrect. You've already stated, that > these error bits are generally disabled in the PCI_EXP_RTCTL reg in > aer_enable_rootport(): > > /* Disable system error generation in response to error messages */ > pcie_capability_clear_word(pdev, PCI_EXP_RTCTL, > SYSTEM_ERROR_INTR_ON_MESG_MASK); > > This leaves "Error Interrupt", but I might be wrong here. > > > It should be easy to use setpci to set/clear these two sets of enable > > bits and figure out which path is of interest here. > > Here the value of the PCI_EXP_RTCTL register at runtime: > # setpci -v -s 00:00.0 CAP_EXP+0x1c.w > 0000:00:00.0 (cap 10 @60) @7c = 0010 > > So all "System Error" enable bits are disabled. > > Please let me know if I should make some other "setpci" tests. I assume you have verified that neither PCI_EXP_RTCTL nor PCI_ERR_ROOT_COMMAND controls these interrupts. (I guess it's possible that PCI_ERR_ROOT_COR/UNCOR_RCV might be ANDed with the platform bits, but I think there are other potential interrupt sources, too.) So I think we need a description that is clearly not related to the PCIe spec terminology, e.g., "platform-specific PCIe interrupts". > > > > > 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. > > > > > > > ... > > > > > 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. > > > > As far as I can tell, "dedicated IRQ lines for services like AER/PME > > etc" would violate the PCIe spec. > > AFAICT this is the case here. > > > That's OK, we can work around that > > sort of thing, but it needs to be clearly called out as some kind of > > quirk and not mixed in with things like System Error signaling, which > > is allowed to be platform-specific. > > Agreed. So how to process with this patchset? Should I reword the > patch subject line (and the commit text and comments) to something like: > > Add option to setup IRQs for platform-specific Error Interrupt ? Yes. But "Error Interrupt" should not be capitalized because that implies a proper noun defined by the PCIe spec. And I thought there were potentially non-error interrupts coming, too.
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 + * + * 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. + */ + status = pcie_init_platform_service_irqs(dev, irqs, capabilities); + + /* + * Only install service irqs, when the platform-specific hook was + * unsuccessful + */ + 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 */