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

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

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 <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  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.

> +		   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);
	}

>  	}
> 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.

>  #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
> -- 

Thanks,
Yazen