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@xxxxxxxxxxxxxxx>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> ---
Reviewed-by: Yazen Ghannam <yazen.ghannam@xxxxxxx>
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
