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.
