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

[Yazen Ghannam <yazen.ghannam@amd.com>]

On Thu, Jan 09, 2025 at 11:36:17AM +0200, Ilpo Järvinen wrote:

[...]

> > > -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 i;
> > > -	int ret;
> > > +	int off, ret;
> > > +	u32 *to;
> > >  
> > >  	memset(log, 0, sizeof(*log));
> > >  
> > > -	for (i = 0; i < 4; i++) {
> > > -		ret = pci_read_config_dword(dev, where + i * 4, &log->dw[i]);
> > > +	for (i = 0; i < tlp_len; i++) {
> > > +		if (i < 4) {
> > > +			off = where + i * 4;
> > > +			to = &log->dw[i];
> > > +		} else {
> > > +			off = where2 + (i - 4) * 4;
> > > +			to = &log->prefix[i - 4];
> > > +		}
> > > +
> > > +		ret = pci_read_config_dword(dev, off, to);
> > >  		if (ret)
> > >  			return pcibios_err_to_errno(ret);
> > 
> > Could we do two loops? Sorry if this was already discussed.
> > 
> > 	for (i = 0; i < min(tlp_len, BASE_NR_TLP); i++, where += 4, tlp_len--) {
> > 		ret = pci_read_config_dword(dev, where, &log->dw[i]);
> > 		if (ret)
> > 			return pcibios_err_to_errno(ret);
> > 	}
> > 
> > 	for (i = 0; i < tlp_len; i++, where2 += 4) {
> > 		ret = pci_read_config_dword(dev, where2, &log->prefix[i]);
> > 		if (ret)
> > 			return pcibios_err_to_errno(ret);
> > 	}
> 
> I'm not convinced splitting it would be clearly better. After the flit 
> mode patch, only variation that will remain is the offset calculation 
> (extended ->dw[] entires will be overlapping with ->prefix[] using union 
> trickery so I can just use ->dw[i] in the loop).
>

Ah okay, no problem.

> > >  	}
> > > diff --git a/include/linux/aer.h b/include/linux/aer.h
> > > index 190a0a2061cd..dc498adaa1c8 100644
> > > --- 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];
> > 
> > Another place for "BASE_NR_*".
> > 
> > >  };
> > >  
> > >  struct aer_capability_regs {
> > > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > > index 14a6306c4ce1..82866ac0bda7 100644
> > > --- a/include/uapi/linux/pci_regs.h
> > > +++ b/include/uapi/linux/pci_regs.h
> > > @@ -790,10 +790,11 @@
> > >  	/* Same bits as above */
> > >  #define PCI_ERR_CAP		0x18	/* Advanced Error Capabilities & Ctrl*/
> > >  #define  PCI_ERR_CAP_FEP(x)	((x) & 0x1f)	/* First Error Pointer */
> > > -#define  PCI_ERR_CAP_ECRC_GENC	0x00000020	/* ECRC Generation Capable */
> > > -#define  PCI_ERR_CAP_ECRC_GENE	0x00000040	/* ECRC Generation Enable */
> > > -#define  PCI_ERR_CAP_ECRC_CHKC	0x00000080	/* ECRC Check Capable */
> > > -#define  PCI_ERR_CAP_ECRC_CHKE	0x00000100	/* ECRC Check Enable */
> > > +#define  PCI_ERR_CAP_ECRC_GENC		0x00000020 /* ECRC Generation Capable */
> > > +#define  PCI_ERR_CAP_ECRC_GENE		0x00000040 /* ECRC Generation Enable */
> > > +#define  PCI_ERR_CAP_ECRC_CHKC		0x00000080 /* ECRC Check Capable */
> > > +#define  PCI_ERR_CAP_ECRC_CHKE		0x00000100 /* ECRC Check Enable */
> > > +#define  PCI_ERR_CAP_PREFIX_LOG_PRESENT	0x00000800 /* TLP Prefix Log Present */
> > 
> > I didn't think to mention this in a previous patch, but could/should we
> > use GENMASK() for bitmasks updates? I know it's a break from the current
> > style though.
> 
> Bjorn called himself "a dinosaur" when it comes to GENMASK() :-):
> 
> https://lore.kernel.org/linux-pci/20231031200312.GA25127@bhelgaas/
> 

I'll try to remember. :)

Thanks,
Yazen