Re: [PATCH v6 6/8] PCI: Add TLP Prefix reading into pcie_read_tlp_log()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2667 bytes --]

On Wed, 11 Dec 2024, Jonathan Cameron wrote:

> On Fri, 13 Sep 2024 17:36:30 +0300
> Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:
> 
> > pcie_read_tlp_log() handles only 4 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 relevant registers are formatted identically in AER
> > and DPC Capability, but has these variations:
> > 
> > a) The offsets of TLP Prefix Log registers vary.
> > b) DPC RP PIO TLP Prefix Log register can be < 4 DWORDs.
> > 
> > Therefore callers must pass the offset of the TLP Prefix Log register
> > and the entire length to pcie_read_tlp_log() to be able to read the
> > correct number of TLP Prefix DWORDs from the correct offset.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> 
> Trivial comments below
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Would have been nice if they'd just made the formats have the
> same sized holes etc!

That's not even the worst problem.

They managed to copy-paste most of the stuff into DPC (copy-paste is 
really obvious because the text still refers to AER in a DPC section :-)) 
but forgot to add a few capability fields into the DPC capability, most 
importantly, the bit that indicates whether TLP was logged in Flit mode
or not And now we get to keep the pieces how to interpret the Log 
Registers (relates to the follow up series). :-(

> > diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
> > index 65ac7b5d8a87..def9dd7b73e8 100644
> > --- a/drivers/pci/pcie/tlp.c
> > +++ b/drivers/pci/pcie/tlp.c
> > @@ -11,26 +11,65 @@
> 
> >  /**
> >   * pcie_read_tlp_log - read TLP Header Log
> Maybe update this to read TLP Header and Prefix Logs
> >   * @dev: PCIe device
> >   * @where: PCI Config offset of TLP Header Log
> > + * @where2: PCI Config offset of TLP Prefix Log
> 
> Is it worth giving it a more specific name than where2?
> Possibly renaming where as well!

Sure, why not.

-- 
 i.

> > + * @tlp_len: TLP Log length (Header Log + TLP Prefix Log in DWORDs)
> >   * @log: TLP Log structure to fill
> >   *
> >   * Fill @log from TLP Header Log registers, e.g., AER or DPC.
> >   *
> >   * Return: 0 on success and filled TLP Log structure, <0 on error.
> >   */
> > -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)
> >  {