All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bjorn Helgaas <helgaas@kernel.org>
To: Stefan Roese <sr@denx.de>
Cc: "Pali Rohár" <pali@kernel.org>,
	linux-pci@vger.kernel.org,
	"Bharat Kumar Gogada" <bharat.kumar.gogada@xilinx.com>,
	"Michal Simek" <michal.simek@xilinx.com>
Subject: Re: [PATCH v4 1/2] PCI/portdrv: Add option to setup IRQs for platform-specific Service Errors
Date: Wed, 8 Jun 2022 13:39:18 -0500	[thread overview]
Message-ID: <20220608183918.GA409625@bhelgaas> (raw)
In-Reply-To: <f3731342-3ddb-1eff-3a6e-51bb1defc469@denx.de>

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.

  parent reply	other threads:[~2022-06-08 18:39 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-14  7:58 [PATCH v4 0/2] Add support to register platform service IRQ Stefan Roese
2022-01-14  7:58 ` [PATCH v4 1/2] PCI/portdrv: Add option to setup IRQs for platform-specific Service Errors Stefan Roese
2022-01-14 11:46   ` Pali Rohár
2022-05-28  0:09   ` Bjorn Helgaas
2022-05-30  8:18     ` Stefan Roese
2022-05-30  8:32       ` Pali Rohár
2022-05-31 21:31         ` Bjorn Helgaas
2022-05-31 22:57           ` Pali Rohár
2022-06-01 11:47           ` Stefan Roese
2022-06-01 11:53             ` Pali Rohár
2022-06-08 18:39             ` Bjorn Helgaas [this message]
2022-01-14  7:58 ` [PATCH v4 2/2] PCI: xilinx-nwl: Add method to init_platform_service_irqs hook Stefan Roese
2022-01-14 11:48   ` Pali Rohár
2022-01-14 12:13     ` Stefan Roese
2022-01-14 12:34       ` Pali Rohár
2022-01-14 17:03         ` Stefan Roese
2022-03-24 16:52 ` [PATCH v4 0/2] Add support to register platform service IRQ Stefan Roese
2022-03-31 15:30   ` Bjorn Helgaas
2022-04-01  6:28     ` Stefan Roese
2022-04-01 12:35       ` Bjorn Helgaas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220608183918.GA409625@bhelgaas \
    --to=helgaas@kernel.org \
    --cc=bharat.kumar.gogada@xilinx.com \
    --cc=linux-pci@vger.kernel.org \
    --cc=michal.simek@xilinx.com \
    --cc=pali@kernel.org \
    --cc=sr@denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.