From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
linux-pci@vger.kernel.org,
Mahesh J Salgaonkar <mahesh@linux.ibm.com>,
Oliver O'Halloran <oohall@gmail.com>,
Lukas Wunner <lukas@wunner.de>,
LKML <linux-kernel@vger.kernel.org>,
linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v3 1/2] PCI: Add TLP Prefix reading into pcie_read_tlp_log()
Date: Mon, 6 May 2024 17:35:34 +0300 (EEST) [thread overview]
Message-ID: <816d5e04-1af7-884c-1ec2-ad70c18068a7@linux.intel.com> (raw)
In-Reply-To: <20240503225305.GA1609388@bhelgaas>
[-- Attachment #1: Type: text/plain, Size: 2897 bytes --]
On Fri, 3 May 2024, Bjorn Helgaas wrote:
> On Fri, Apr 12, 2024 at 04:36:34PM +0300, Ilpo Järvinen wrote:
> > pcie_read_tlp_log() handles only 4 TLP Header Log DWORDs but TLP Prefix
> > Log (PCIe r6.1 secs 7.8.4.12 & 7.9.14.13) may also be present.
> >
> > Generalize pcie_read_tlp_log() and struct pcie_tlp_log to handle also
> > TLP Prefix Log. The layout of relevant registers in AER and DPC
> > Capability is not identical because the offsets of TLP Header Log and
> > TLP Prefix Log vary so the callers must pass the offsets to
> > pcie_read_tlp_log().
>
> I think the layouts of the Header Log and the TLP Prefix Log *are*
> identical, but they are at different offsets in the AER Capability vs
> the DPC Capability. Lukas and I have both stumbled over this.
I'll try to reword it once again.
The way it's spec'ed, there actually also a small difference in sizes too
(PCIe r6 7.9.14.13 says DPC one can be < 4 DWs whereas AER on is always 4
DWs regardless of the number of supported E-E Prefixes) so I'll just
rewrite it so it doesn't focus just on the offset.
> Similar and more comments at:
> https://lore.kernel.org/r/20240322193011.GA701027@bhelgaas
I'm really sorry, I missed those comments and only focused on that ixgbe
part.
> > Convert eetlp_prefix_path into integer called eetlp_prefix_max and
> > make is available also when CONFIG_PCI_PASID is not configured to
> > be able to determine the number of E-E Prefixes.
>
> s/make is/make it/
>
> I think this could be a separate patch.
Sure, I can make it own patch.
> > --- a/include/linux/aer.h
> > +++ b/include/linux/aer.h
> > @@ -20,6 +20,7 @@ struct pci_dev;
> >
> > struct pcie_tlp_log {
> > u32 dw[4];
> > + u32 prefix[4];
> > };
> >
> > struct aer_capability_regs {
> > @@ -37,7 +38,9 @@ struct aer_capability_regs {
> > u16 uncor_err_source;
> > };
> >
> > -int pcie_read_tlp_log(struct pci_dev *dev, int where, struct pcie_tlp_log *log);
> > +int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
> > + unsigned int tlp_len, struct pcie_tlp_log *log);
> > +unsigned int aer_tlp_log_len(struct pci_dev *dev);
>
> I think it was a mistake to expose pcie_read_tlp_log() outside
> drivers/pci, and I don't think we should expose aer_tlp_log_len()
> either.
Ah, my intention was to remove the exposure but I only ended up removing
the actual EXPORT and didn't realize I should have also moved the
prototype into another header.
I'll add also a patch to remove pcie_read_tlp_log() EXPORT too but I'm
wondering now whether I should also move these function(s) into
pcie/aer.c (or somewhere else that is only build if AER is enabled) since
there won't be callers ourside of AER/DPC?
> We might be stuck with exposing struct pcie_tlp_log since it looks
> like ras_event.h uses it.
Yes.
--
i.
next prev parent reply other threads:[~2024-05-06 14:35 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-12 13:36 [PATCH v3 0/2] PCI: Consolidate TLP Log reading and printing Ilpo Järvinen
2024-04-12 13:36 ` [PATCH v3 1/2] PCI: Add TLP Prefix reading into pcie_read_tlp_log() Ilpo Järvinen
2024-05-03 22:53 ` Bjorn Helgaas
2024-05-06 14:35 ` Ilpo Järvinen [this message]
2024-04-12 13:36 ` [PATCH v3 2/2] PCI: Create helper to print TLP Header and Prefix Log Ilpo Järvinen
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=816d5e04-1af7-884c-1ec2-ad70c18068a7@linux.intel.com \
--to=ilpo.jarvinen@linux.intel.com \
--cc=bhelgaas@google.com \
--cc=helgaas@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=lukas@wunner.de \
--cc=mahesh@linux.ibm.com \
--cc=oohall@gmail.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).