On Wed, 8 Jan 2025, Yazen Ghannam wrote:
> On Wed, Dec 18, 2024 at 04:37:46PM +0200, Ilpo Järvinen 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.
> > c) AER TLP Prefix Log Present (PCIe r6.1 sec 7.8.4.7) can indicate
> > Prefix Log is not present.
> >
> > 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 <[email protected]>
> > Reviewed-by: Jonathan Cameron <[email protected]>
> > ---
> > drivers/pci/pci.h | 5 +++-
> > drivers/pci/pcie/aer.c | 5 +++-
> > drivers/pci/pcie/dpc.c | 13 +++++----
> > drivers/pci/pcie/tlp.c | 51 +++++++++++++++++++++++++++++++----
> > include/linux/aer.h | 1 +
> > include/uapi/linux/pci_regs.h | 10 ++++---
> > 6 files changed, 67 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 55fcf3bac4f7..7797b2544d00 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -550,7 +550,9 @@ struct aer_err_info {
> > int aer_get_device_error_info(struct pci_dev *dev, struct aer_err_info
> > *info);
> > void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
> >
> > -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, u32 aercc);
> > #endif /* CONFIG_PCIEAER */
> >
> > #ifdef CONFIG_PCIEPORTBUS
> > @@ -569,6 +571,7 @@ void pci_dpc_init(struct pci_dev *pdev);
> > void dpc_process_error(struct pci_dev *pdev);
> > pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
> > bool pci_dpc_recovered(struct pci_dev *pdev);
> > +unsigned int dpc_tlp_log_len(struct pci_dev *dev);
> > #else
> > static inline void pci_save_dpc_state(struct pci_dev *dev) { }
> > static inline void pci_restore_dpc_state(struct pci_dev *dev) { }
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index 80c5ba8d8296..656dbf1ac45b 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -1248,7 +1248,10 @@ int aer_get_device_error_info(struct pci_dev *dev,
> > struct aer_err_info *info)
> >
> > if (info->status & AER_LOG_TLP_MASKS) {
> > info->tlp_header_valid = 1;
> > - pcie_read_tlp_log(dev, aer + PCI_ERR_HEADER_LOG,
> > &info->tlp);
> > + pcie_read_tlp_log(dev, aer + PCI_ERR_HEADER_LOG,
> > + aer + PCI_ERR_PREFIX_LOG,
> > + aer_tlp_log_len(dev, aercc),
> > + &info->tlp);
> > }
> > }
> >
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index 2b6ef7efa3c1..7933b3cedb59 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -190,7 +190,7 @@ pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
> > static void dpc_process_rp_pio_error(struct pci_dev *pdev)
> > {
> > u16 cap = pdev->dpc_cap, dpc_status, first_error;
> > - u32 status, mask, sev, syserr, exc, log, prefix;
> > + u32 status, mask, sev, syserr, exc, log;
> > struct pcie_tlp_log tlp_log;
> > int i;
> >
> > @@ -217,20 +217,19 @@ static void dpc_process_rp_pio_error(struct pci_dev
> > *pdev)
> >
> > if (pdev->dpc_rp_log_size < 4)
> > goto clear_status;
> > - pcie_read_tlp_log(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG, &tlp_log);
> > + pcie_read_tlp_log(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG,
> > + cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG,
> > + dpc_tlp_log_len(pdev), &tlp_log);
> > pci_err(pdev, "TLP Header: %#010x %#010x %#010x %#010x\n",
> > tlp_log.dw[0], tlp_log.dw[1], tlp_log.dw[2], tlp_log.dw[3]);
> > + for (i = 0; i < pdev->dpc_rp_log_size - 5; i++)
> > + pci_err(pdev, "TLP Prefix Header: dw%d, %#010x\n", i,
> > tlp_log.prefix[i]);
> >
> > if (pdev->dpc_rp_log_size < 5)
> > goto clear_status;
> > pci_read_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_IMPSPEC_LOG, &log);
> > pci_err(pdev, "RP PIO ImpSpec Log %#010x\n", log);
> >
> > - for (i = 0; i < pdev->dpc_rp_log_size - 5; i++) {
> > - pci_read_config_dword(pdev,
> > - cap + PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG + i * 4,
> > &prefix);
> > - pci_err(pdev, "TLP Prefix Header: dw%d, %#010x\n", i, prefix);
> > - }
> > clear_status:
> > pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS, status);
> > }
> > diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
> > index 65ac7b5d8a87..302ba99e64e6 100644
> > --- a/drivers/pci/pcie/tlp.c
> > +++ b/drivers/pci/pcie/tlp.c
> > @@ -11,26 +11,67 @@
> >
> > #include "../pci.h"
> >
> > +/**
> > + * aer_tlp_log_len - Calculates AER Capability TLP Header/Prefix Log length
> > + * @dev: PCIe device
> > + * @aercc: AER Capabilities and Control register value
> > + *
> > + * Return: TLP Header/Prefix Log length
> > + */
> > +unsigned int aer_tlp_log_len(struct pci_dev *dev, u32 aercc)
> > +{
> > + return 4 + (aercc & PCI_ERR_CAP_PREFIX_LOG_PRESENT) ?
>
> Another place for a "BASE_NR_*" define. I incorrectly said "MAX_NR_*" in
> a previous patch. But "BASE" or "STD" seems more appropriate.
Thanks for the reviews!
I don't know why I didn't realize I was adding a literal 4 in many places.
I'll surely add a define for it now that you pointed it out, thanks.
> > + dev->eetlp_prefix_max : 0;
> > +}
> > +
> > +#ifdef CONFIG_PCIE_DPC
> > +/**
> > + * dpc_tlp_log_len - Calculates DPC RP PIO TLP Header/Prefix Log length
> > + * @dev: PCIe device
> > + *
> > + * Return: TLP Header/Prefix Log length
> > + */
> > +unsigned int dpc_tlp_log_len(struct pci_dev *dev)
> > +{
> > + /* Remove ImpSpec Log register from the count */
> > + if (dev->dpc_rp_log_size >= 5)
> > + return dev->dpc_rp_log_size - 1;
> > +
> > + return dev->dpc_rp_log_size;
> > +}
> > +#endif
> > +
> > /**
> > * pcie_read_tlp_log - read TLP Header Log
> > * @dev: PCIe device
> > * @where: PCI Config offset of TLP Header Log
> > + * @where2: PCI Config offset of TLP Prefix Log
> > + * @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)
> > {
> > 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).
> > }
> > 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/
> > #define PCI_ERR_HEADER_LOG 0x1c /* Header Log Register (16 bytes) */
> > #define PCI_ERR_ROOT_COMMAND 0x2c /* Root Error Command */
> > #define PCI_ERR_ROOT_CMD_COR_EN 0x00000001 /* Correctable Err Reporting
> > Enable */
> > @@ -809,6 +810,7 @@
> > #define PCI_ERR_ROOT_FATAL_RCV 0x00000040 /* Fatal Received */
> > #define PCI_ERR_ROOT_AER_IRQ 0xf8000000 /* Advanced Error
> > Interrupt Message Number */
> > #define PCI_ERR_ROOT_ERR_SRC 0x34 /* Error Source Identification
> > */
> > +#define PCI_ERR_PREFIX_LOG 0x38 /* TLP Prefix LOG Register (up to 16
> > bytes) */
> >
> > /* Virtual Channel */
> > #define PCI_VC_PORT_CAP1 0x04
--
i.
