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