Re: [PATCH v5 7/7] PCI: Create helper to print TLP Header and Prefix Log

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, May 14, 2024 at 02:31:09PM +0300, Ilpo Järvinen wrote:
> Add pcie_print_tlp_log() helper to print TLP Header and Prefix Log.
> Print End-End Prefixes only if they are non-zero.
> 
> Consolidate the few places which currently print TLP using custom
> formatting.
> 
> The first attempt used pr_cont() instead of building a string first but
> it turns out pr_cont() is not compatible with pci_err() and prints on a
> separate line. When I asked about this, Andy Shevchenko suggested
> pr_cont() should not be used in the first place (to eventually get rid
> of it) so pr_cont() is now replaced with building the string first.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/pci/pci.h      |  2 ++
>  drivers/pci/pcie/aer.c | 10 ++--------
>  drivers/pci/pcie/dpc.c |  5 +----
>  drivers/pci/pcie/tlp.c | 31 +++++++++++++++++++++++++++++++
>  4 files changed, 36 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 7afdd71f9026..45083e62892c 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -423,6 +423,8 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info);
>  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);
> +void pcie_print_tlp_log(const struct pci_dev *dev,
> +			const struct pcie_tlp_log *log, const char *pfx);
>  #endif	/* CONFIG_PCIEAER */
>  
>  #ifdef CONFIG_PCIEPORTBUS
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index ecc1dea5a208..efb9e728fe94 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -664,12 +664,6 @@ static void pci_rootport_aer_stats_incr(struct pci_dev *pdev,
>  	}
>  }
>  
> -static void __print_tlp_header(struct pci_dev *dev, struct pcie_tlp_log *t)
> -{
> -	pci_err(dev, "  TLP Header: %08x %08x %08x %08x\n",
> -		t->dw[0], t->dw[1], t->dw[2], t->dw[3]);
> -}
> -
>  static void __aer_print_error(struct pci_dev *dev,
>  			      struct aer_err_info *info)
>  {
> @@ -724,7 +718,7 @@ void aer_print_error(struct pci_dev *dev, struct aer_err_info *info)
>  	__aer_print_error(dev, info);
>  
>  	if (info->tlp_header_valid)
> -		__print_tlp_header(dev, &info->tlp);
> +		pcie_print_tlp_log(dev, &info->tlp, "  ");

I see you went to some trouble to preserve the previous output, down
to the number of spaces prefixing it.

But more than the leading spaces, I think what people will notice is
that previously AER and DPC dmesgs contain the "AER: " or "DPC: "
prefixes implicitly added by the dev_fmt definitions [1], where now
IIUC they won't.

I think adding dev_fmt("") here should take care of that, e.g.,

  pcie_print_tlp_log(dev, &info->tlp, dev_fmt(""));

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1990272

>  out:
>  	if (info->id && info->error_dev_num > 1 && info->id == id)
> @@ -796,7 +790,7 @@ void pci_print_aer(struct pci_dev *dev, int aer_severity,
>  			aer->uncor_severity);
>  
>  	if (tlp_header_valid)
> -		__print_tlp_header(dev, &aer->header_log);
> +		pcie_print_tlp_log(dev, &aer->header_log, "  ");
>  
>  	trace_aer_event(dev_name(&dev->dev), (status & ~mask),
>  			aer_severity, tlp_header_valid, &aer->header_log);
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 5056cc6961ec..598f74384471 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -220,10 +220,7 @@ static void dpc_process_rp_pio_error(struct pci_dev *pdev)
>  	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]);
> +	pcie_print_tlp_log(pdev, &tlp_log, "");
>  
>  	if (pdev->dpc_rp_log_size < 5)
>  		goto clear_status;
> diff --git a/drivers/pci/pcie/tlp.c b/drivers/pci/pcie/tlp.c
> index def9dd7b73e8..097ac8514e96 100644
> --- a/drivers/pci/pcie/tlp.c
> +++ b/drivers/pci/pcie/tlp.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/aer.h>
> +#include <linux/array_size.h>
>  #include <linux/pci.h>
>  #include <linux/string.h>
>  
> @@ -76,3 +77,33 @@ int pcie_read_tlp_log(struct pci_dev *dev, int where, int where2,
>  
>  	return 0;
>  }
> +
> +/**
> + * pcie_print_tlp_log - Print TLP Header / Prefix Log contents
> + * @dev: PCIe device
> + * @log: TLP Log structure
> + * @pfx: String prefix (for print out indentation)
> + *
> + * Prints TLP Header and Prefix Log information held by @log.
> + */
> +void pcie_print_tlp_log(const struct pci_dev *dev,
> +			const struct pcie_tlp_log *log, const char *pfx)
> +{
> +	char buf[(10 + 1) * (4 + ARRAY_SIZE(log->prefix)) + 14 + 1];
> +	unsigned int i;
> +	int len;
> +
> +	len = scnprintf(buf, sizeof(buf), "%#010x %#010x %#010x %#010x",
> +			log->dw[0], log->dw[1], log->dw[2], log->dw[3]);
> +
> +	if (log->prefix[0])
> +		len += scnprintf(buf + len, sizeof(buf) - len, " E-E Prefixes:");
> +	for (i = 0; i < ARRAY_SIZE(log->prefix); i++) {
> +		if (!log->prefix[i])
> +			break;
> +		len += scnprintf(buf + len, sizeof(buf) - len,
> +				 " %#010x", log->prefix[i]);
> +	}
> +
> +	pci_err(dev, "%sTLP Header: %s\n", pfx, buf);
> +}
> -- 
> 2.39.2
>