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
