Re: [PATCH v8 2/7] PCI: Move TLP Log handling to own file

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

On Wed, Dec 18, 2024 at 04:37:42PM +0200, Ilpo Järvinen wrote:
> TLP Log is PCIe feature and is processed only by AER and DPC.
> Configwise, DPC depends AER being enabled. In lack of better place, the
> TLP Log handling code was initially placed into pci.c but it can be
> easily placed in a separate file.
> 
> Move TLP Log handling code to own file under pcie/ subdirectory and
> include it only when AER is enabled.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---

Reviewed-by: Yazen Ghannam <yazen.ghannam@amd.com>

Overall, looks good to me, but I have one idea below.

>  drivers/pci/pci.c         | 27 ---------------------------
>  drivers/pci/pci.h         |  2 +-
>  drivers/pci/pcie/Makefile |  2 +-
>  drivers/pci/pcie/tlp.c    | 39 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 41 insertions(+), 29 deletions(-)
>  create mode 100644 drivers/pci/pcie/tlp.c
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e0fdc9d10f91..02cd4c7eb80b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1099,33 +1099,6 @@ static void pci_enable_acs(struct pci_dev *dev)
>  	pci_write_config_word(dev, pos + PCI_ACS_CTRL, caps.ctrl);
>  }
>  
> -/**
> - * pcie_read_tlp_log - read TLP Header Log
> - * @dev: PCIe device
> - * @where: PCI Config offset of TLP Header Log
> - * @tlp_log: TLP Log structure to fill
> - *
> - * Fill @tlp_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 *tlp_log)
> -{
> -	int i, ret;
> -
> -	memset(tlp_log, 0, sizeof(*tlp_log));
> -
> -	for (i = 0; i < 4; i++) {
> -		ret = pci_read_config_dword(dev, where + i * 4,
> -					    &tlp_log->dw[i]);
> -		if (ret)
> -			return pcibios_err_to_errno(ret);
> -	}
> -
> -	return 0;
> -}
> -
>  /**
>   * pci_restore_bars - restore a device's BAR values (e.g. after wake-up)
>   * @dev: PCI device to have its BARs restored
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 8a60fc9e7786..55fcf3bac4f7 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -549,9 +549,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);
> -#endif	/* CONFIG_PCIEAER */
>  
>  int pcie_read_tlp_log(struct pci_dev *dev, int where, struct pcie_tlp_log *log);
> +#endif	/* CONFIG_PCIEAER */
>  
>  #ifdef CONFIG_PCIEPORTBUS
>  /* Cached RCEC Endpoint Association */
> diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
> index 53ccab62314d..173829aa02e6 100644
> --- a/drivers/pci/pcie/Makefile
> +++ b/drivers/pci/pcie/Makefile
> @@ -7,7 +7,7 @@ pcieportdrv-y			:= portdrv.o rcec.o
>  obj-$(CONFIG_PCIEPORTBUS)	+= pcieportdrv.o bwctrl.o
>  
>  obj-y				+= aspm.o
> -obj-$(CONFIG_PCIEAER)		+= aer.o err.o
> +obj-$(CONFIG_PCIEAER)		+= aer.o err.o tlp.o
>  obj-$(CONFIG_PCIEAER_INJECT)	+= aer_inject.o
>  obj-$(CONFIG_PCIE_PME)		+= pme.o
>  obj-$(CONFIG_PCIE_DPC)		+= dpc.o
> diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
> new file mode 100644
> index 000000000000..3f053cc62290
> --- /dev/null
> +++ b/drivers/pci/pcie/tlp.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PCIe TLP Log handling
> + *
> + * Copyright (C) 2024 Intel Corporation
> + */
> +
> +#include <linux/aer.h>
> +#include <linux/pci.h>
> +#include <linux/string.h>
> +
> +#include "../pci.h"
> +
> +/**
> + * pcie_read_tlp_log - read TLP Header Log
> + * @dev: PCIe device
> + * @where: PCI Config offset of TLP Header Log
> + * @tlp_log: TLP Log structure to fill
> + *
> + * Fill @tlp_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 *tlp_log)
> +{
> +	int i, ret;
> +
> +	memset(tlp_log, 0, sizeof(*tlp_log));
> +

Can we include a define for the number of registers? 

> +	for (i = 0; i < 4; i++) {

This '4' is "MIN_TLP_REGS" or something similar.

> +		ret = pci_read_config_dword(dev, where + i * 4,

This '4' is the register offset factor.

Another thought is to make the offset a variable and adjust it in the
for-loop conditions.

	int i, ret, offset = where;

	for (i = 0; i < MIN_TLP_REGS; i++, offset += 4) {
		ret = pci_read_config_dword(dev, offset, &tlp_log->dw[i]);

I think this will help as variable-size TLP logs are added in later
patches.

> +					    &tlp_log->dw[i]);
> +		if (ret)
> +			return pcibios_err_to_errno(ret);
> +	}
> +
> +	return 0;
> +}

Thanks,
Yazen