Re: [PATCH v7 3/6] PCI: Consolidate delimiter handling into pci_dev_str_match()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] When path-based parsing fails, the error message logs the remaining parameters (or an empty string) instead of the malformed parameter.
- [Medium] Replacing a rate-limited print with an unconditional `pr_err()` causes severe boot log spam for malformed parameters.
--

commit aaf5aed76bd0649688730dce72d818789919c430
Author: Wei Wang <wei.w.wang@hotmail.com>

PCI: Consolidate delimiter handling into pci_dev_str_match()

This commit moves the ';' or ',' delimiter checking logic directly into
pci_dev_str_match(), simplifying caller code. It also updates the function
to return -ENODEV when parsing reaches the end of the list or encounters an
invalid format.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e017200694d8a..ebd1e81264d21 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
[ ... ]
> @@ -397,16 +402,28 @@ static int pci_dev_str_match(struct pci_dev *dev, const char *p,
>  		 */
>  		ret = pci_dev_str_match_path(dev, p, &p);
>  		if (ret < 0)
> -			return ret;
> +			goto not_found;

In pci_dev_str_match_path(), the pointer p is advanced immediately:

static int pci_dev_str_match_path(struct pci_dev *dev, const char *path,
				  const char **endptr)
{
...
	*endptr = strchrnul(path, ';');

Since pci_dev_str_match() calls pci_dev_str_match_path(dev, p, &p), the local
pointer p is updated to the end of the parameter before returning an error.

When we jump to not_found, does this mean the pr_err() will print the
remaining parameters (or an empty string) instead of the malformed parameter?

>  		else if (ret)
>  			goto found;
>  	}
>  
> -	*endptr = p;
> -	return 0;
> +not_found:
> +	if (ret < 0)
> +		pr_err("PCI: Can't parse parameter: %s\n", p);

Because pci_dev_str_match() is evaluated for every PCI device during boot
enumeration (for example, through __pci_config_acs()), it is called
repeatedly.

Could this unconditional pr_err() cause severe boot log spam if a malformed
parameter is passed on the kernel command line?

The previous code in __pci_config_acs() used a rate-limited pr_warn_once(),
which this commit removed:

[ ... ]
> @@ -933,18 +950,11 @@ static void __pci_config_acs(struct pci_dev *dev, struct pci_acs *caps,
>  
>  		ret = pci_dev_str_match(dev, p, &p);
>  		if (ret < 0) {
> -			pr_warn_once("PCI: Can't parse ACS command line parameter\n");
>  			break;
>  		} else if (ret == 1) {
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/SI2PR01MB439385689A32A1DDA9CEABE1DC3F2@SI2PR01MB4393.apcprd01.prod.exchangelabs.com?part=3