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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241211165638.00006f33@huawei.com>
Date: Wed, 11 Dec 2024 16:56:38 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC: <linux-pci@...r.kernel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, "Mahesh
 J Salgaonkar" <mahesh@...ux.ibm.com>, Oliver O'Halloran <oohall@...il.com>,
	Lukas Wunner <lukas@...ner.de>, Krzysztof Wilczyński
	<kw@...ux.com>, <linux-kernel@...r.kernel.org>,
	<linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH v6 7/8] PCI: Create helper to print TLP Header and
 Prefix Log

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

A couple of trivial things inline but looks good to me either way.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.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);
> +}