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@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.
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
