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

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

On Fri, 13 Sep 2024 17:36:31 +0300
Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> 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>

A couple of trivial things inline but looks good to me either way.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

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

Code doesn't care if it is indentation or ponies.  So does it make
sense to say anything beyond String prefix?

> + *
> + * 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];

Can we associate the 14 with the prefixes string by having that as a
const char * and using strlen()  It was a tiny bit irritating to count
the characters whilst reviewing this ;)



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