On 7/23/24 17:41, Bjorn Helgaas wrote:
> 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?
Not crash or functionality errors. Usually it is QoS related because of
resource competition. AMD has
>
>> 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.
I can combine them. Here is the combination and it meaning based on TPH
Control Register values:
Requestor Enable | ST Mode | Meaning
---------------------------------------------------------------
00 | xx | TPH disabled (i.e. notph)
01 | 00 | TPH enabled, NO ST Mode (i.e. nostmode)
01 or 11 | 01 | Interrupt Vector mode
01 or 11 | 10 | Device specific mode
If you have any other thoughts on how to approach these modes, please
let me know.
>
>> +++ 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
