Re: [PATCH v3 1/2] PCI: Add TLP Prefix reading into pcie_read_tlp_log()

[Ilpo Järvinen]

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.