Re: [PATCH V3 03/10] PCI/TPH: Add pci=notph to prevent use of TPH

[Bjorn Helgaas]

On Wed, Jul 17, 2024 at 03:55:04PM -0500, Wei Huang wrote:
> TLP headers with incorrect steering tags (e.g. caused by buggy driver)
> can potentially cause issues when the system hardware consumes the tags.

Hmm.  What kind of issues?  Crash?  Data corruption?  Poor
performance?

> Provide a kernel option, with related helper functions, to completely
> prevent TPH from being enabled.

Also would be nice to have a hint about the difference between "notph"
and "nostmode".  Maybe that goes in the "nostmode" patch?  I'm not
super clear on all the differences here.

> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4655,6 +4655,7 @@
>  		nomio		[S390] Do not use MIO instructions.
>  		norid		[S390] ignore the RID field and force use of
>  				one PCI domain per PCI function
> +		notph		[PCIE] Do not use PCIe TPH

Expand acronym here since there's no helpful context.  Can also
include "(TPH)" if that's useful.

> @@ -322,8 +323,12 @@ static long local_pci_probe(void *_ddi)
>  	pm_runtime_get_sync(dev);
>  	pci_dev->driver = pci_drv;
>  	rc = pci_drv->probe(pci_dev, ddi->id);
> -	if (!rc)
> +	if (!rc) {
> +		if (pci_tph_disabled())
> +			pcie_tph_disable(pci_dev);

I'm not really a fan of cluttering probe() like this.  Can't we
disable it in pcie_tph_init() so all devices start off with TPH
disabled, and then check pci_tph_disabled() in whatever interface
drivers use to enable TPH?

> +bool pci_tph_disabled(void)
> +{
> +	return pcie_tph_disabled;
> +}
> +EXPORT_SYMBOL_GPL(pci_tph_disabled);

Other related interfaces use "pcie" prefix; I think this should match.

Do drivers need this?  Would be nice not to export it unless they do.

Bjorn