Re: [PATCH v9 3/8] PCI: Add defines for TLP Header/Prefix log sizes

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 14 Jan 2025 19:08:35 +0200
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> wrote:

> Add defines for AER and DPC capabilities TLP Header Logging register
> sizes (PCIe r6.2, sec 7.8.4 / 7.9.14) and replace literals with them.
> 
> Suggested-by: Yazen Ghannam <yazen.ghannam@amd.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Hi Ilpo,

Where it is simply the number of entries I think the changes make sense.

Where it is about whether the log reaches a defined register I'd be more
tempted to use the register addresses to figure it out + length for the last
one.  That avoids a rather magic +- 1 in various places.

Jonathan


> ---
>  drivers/pci/pcie/dpc.c | 10 ++++++----
>  drivers/pci/pcie/tlp.c |  2 +-
>  drivers/pci/quirks.c   |  6 ++++--
>  include/linux/aer.h    |  9 ++++++++-
>  4 files changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 2b6ef7efa3c1..0674d8c89bfa 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -215,18 +215,18 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
>  				first_error == i ? " (First)" : "");
>  	}
>  
> -	if (pdev->dpc_rp_log_size < 4)
> +	if (pdev->dpc_rp_log_size < PCIE_STD_NUM_TLP_HEADERLOG)

This I'm fine with.

>  		goto clear_status;
>  	pcie_read_tlp_log(pdev, cap + PCI_EXP_DPC_RP_PIO_HEADER_LOG, &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]);
>  
> -	if (pdev->dpc_rp_log_size < 5)
> +	if (pdev->dpc_rp_log_size < PCIE_STD_NUM_TLP_HEADERLOG + 1)

As below. Can we build this from difference in register instead so something like

	if (pdev->dpc_rpc_log_size <=
            (PCIE_EXP_DPC_RP_PIO_IMPSPEC_LOG - PCIE_EXP_DPC_RP_PIO_HEADER_LOG) / 4)

>  		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++) {
> +	for (i = 0; i < pdev->dpc_rp_log_size - PCIE_STD_NUM_TLP_HEADERLOG - 1; i++) {

This is a bit ugly.  Can we instead build it based on difference in registers and
avoid the mysterious +-1?
(PCI_EXP_DPC_RP_PIO_TLPPREFIX_LOG - PCI_EXP_DPC_RPIO_HEADER_LOG) / 4 ?

>  		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);
> @@ -404,7 +404,9 @@ void pci_dpc_init(struct pci_dev *pdev)
>  	if (!pdev->dpc_rp_log_size) {
>  		pdev->dpc_rp_log_size =
>  				FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, cap);
> -		if (pdev->dpc_rp_log_size < 4 || pdev->dpc_rp_log_size > 9) {
> +		if (pdev->dpc_rp_log_size < PCIE_STD_NUM_TLP_HEADERLOG ||
> +		    pdev->dpc_rp_log_size > PCIE_STD_NUM_TLP_HEADERLOG + 1 +
> +					    PCIE_STD_MAX_TLP_PREFIXLOG) {
Could also construct this one from register offset and the max size of the log.

>  			pci_err(pdev, "RP PIO log size %u is invalid\n",
>  				pdev->dpc_rp_log_size);
>  			pdev->dpc_rp_log_size = 0;
> diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
> index 3f053cc62290..4cc76bd1867a 100644
> --- a/drivers/pci/pcie/tlp.c
> +++ b/drivers/pci/pcie/tlp.c
> @@ -28,7 +28,7 @@ int pcie_read_tlp_log(struct pci_dev *dev, int where,
>  
>  	memset(tlp_log, 0, sizeof(*tlp_log));
>  
> -	for (i = 0; i < 4; i++) {
> +	for (i = 0; i < PCIE_STD_NUM_TLP_HEADERLOG; i++) {
>  		ret = pci_read_config_dword(dev, where + i * 4,
>  					    &tlp_log->dw[i]);
>  		if (ret)
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 76f4df75b08a..84487615e1d1 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -12,6 +12,7 @@
>   * file, where their drivers can use them.
>   */
>  
> +#include <linux/aer.h>
>  #include <linux/align.h>
>  #include <linux/bitfield.h>
>  #include <linux/types.h>
> @@ -6233,8 +6234,9 @@ static void dpc_log_size(struct pci_dev *dev)
>  		return;
>  
>  	if (FIELD_GET(PCI_EXP_DPC_RP_PIO_LOG_SIZE, val) == 0) {
> -		pci_info(dev, "Overriding RP PIO Log Size to 4\n");
> -		dev->dpc_rp_log_size = 4;
> +		pci_info(dev, "Overriding RP PIO Log Size to %d\n",
> +			 PCIE_STD_NUM_TLP_HEADERLOG);
> +		dev->dpc_rp_log_size = PCIE_STD_NUM_TLP_HEADERLOG;
>  	}
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x461f, dpc_log_size);
> diff --git a/include/linux/aer.h b/include/linux/aer.h
> index 190a0a2061cd..4ef6515c3205 100644
> --- a/include/linux/aer.h
> +++ b/include/linux/aer.h
> @@ -16,10 +16,17 @@
>  #define AER_CORRECTABLE			2
>  #define DPC_FATAL			3
>  
> +/*
> + * AER and DPC capabilities TLP Logging register sizes (PCIe r6.2, sec 7.8.4
> + * & 7.9.14).
> + */
> +#define PCIE_STD_NUM_TLP_HEADERLOG     4
> +#define PCIE_STD_MAX_TLP_PREFIXLOG     4
> +
>  struct pci_dev;
>  
>  struct pcie_tlp_log {
> -	u32 dw[4];
> +	u32 dw[PCIE_STD_NUM_TLP_HEADERLOG];
>  };
>  
>  struct aer_capability_regs {