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

[Wei Wang]

On 5/4/26 1:31 PM, Krzysztof Wilczyński wrote:
> Hello,
>
> > +not_found:
> > +	if (ret < 0)
> > +		pr_err("PCI: Can't parse parameter: %s\n", p);
>
> Two issues here:
>
>    - At this time, when this messages is to be shown, the p can be already
>      at the NULL-terminator, as a result of parsing, so if the entire
>      parameter is some garbage value, then it will show nothing in dmesg.

Thanks for catching this. A simple fix is to store the original pointer at the
start of the function, e.g.,  "const char *orig_p = p;" and print orig_p instead.


>    - This new per_err() is not rate-limited, like the old messages were,
>      so if the parameter is invalid or malformed, and you have... 30, or
>      more on servers and serious hardware, devices in the system, then
>      it will print for every single one of them.
>
> Albeit, the second issue might not be an issue, and it might have been on
> purpose.  However, I am not convinced it was.

OK. Using pr_warn_once() matches the previous behavior in __pci_config_acs().

> > +	if (*p != ';' && *p != ',') {
> > +		/*
> > +		 * End of param or invalid format. Return -ENODEV so the caller
> > +		 * stops parsing.
> > +		 */
> > +		return -ENODEV;
> > +	}
>
> [...]
> > @@ -943,18 +960,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_info_once("PCI: Can't parse ACS command line parameter\n");
>
>
> [...]
> > @@ -6392,16 +6402,8 @@ static resource_size_t pci_specified_resource_alignment(struct pci_dev *dev,
> >   			align = 1ULL << align_order;
> >   			break;
> >   		} else if (ret < 0) {
> > -			pr_err("PCI: Can't parse resource_alignment parameter: %s\n",
> > -			       p);
>
> For the two above - it was nice to have the information about which
> parameter to what kernel boot option was actually causing issues.  The
> new code will only print a generic message.

Yes, it currently just prints the BDFs that encounter an error. I'm thinking about bringing the caller
info into the log for more contexts, and seems __builtin_return_address(0) isn't a good fit here
due to the possibility of function inlining. If no better options, I'll move the error print to each
caller.