Re: [PATCH] PCI/ASPM: fix unexpected behavior when re-enabling L1

[Heiner Kallweit <hkallweit1@gmail.com>]

On 03.10.2023 05:33, Kuppuswamy Sathyanarayanan wrote:
> 
> 
> On 10/2/2023 8:14 AM, Bjorn Helgaas wrote:
>> [+cc Sathy, Lukas]
>>
>> On Sat, Aug 26, 2023 at 01:10:35PM +0200, Heiner Kallweit wrote:
>>> After the referenced commit we may see L1 sub-states being active
>>> unexpectedly. Following scenario as an example:
>>> r8169 disables L1 because of known hardware issues on a number of
>>> systems. Implicitly L1.1 and L1.2 are disabled too.
>>> On my system L1 and L1.1 work fine, but L1.2 causes missed
>>> rx packets. Therefore I write 1 to aspm_l1_1.
>>> This removes ASPM_STATE_L1 from the disabled modes and therefore
>>> unexpectedly enables also L1.2. So return to the old behavior.
> 
> IIUC, the above-mentioned SysFS issue will be fixed by your change to
> aspm_attr_store_common(), right?
> 
In my use case the driver uses pci_disable_link_state() to disable L1
and implicitly also L1.1 and L1.2. The following change is needed so
that L1.2 remains disabled if L1 is removed from the list of disabled
states later. That's the revert we're talking about.

> Can you elaborate why you need the following change?
> 
>>> @@ -1063,7 +1063,7 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
>>>  	if (state & PCIE_LINK_STATE_L1)
>>> -		link->aspm_disable |= ASPM_STATE_L1;
>>> +		link->aspm_disable |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
> 
>>>
>>> A comment in the commit message of the referenced change correctly points
>>> out that this behavior is inconsistent with aspm_attr_store_common().
>>> So change aspm_attr_store_common() accordingly.
>>
>> I think we should split this into a pure revert of fb097dcd5a28 with
>> the description of the unintended consequence, followed by another
>> patch to change aspm_attr_store_common().
>>
> 
> I agree, the revert and new change should be split into two patches.
> 
>> I guess the existing aspm_attr_store_common() behavior allows a
>> similar unexpected behavior?  For example, in this sequence:
>>
>>   - Write 0 to "l1_aspm" to disable L1
>>   - Write 1 to "l1_1_aspm" to enable L1.1
>>
>> does L1.2 get implicitly enabled as well even though that's clearly
>> not what the user intended?
>>
>> There's also the separate question of how the sysfs file and the
>> pci_disable_link_state() API should interact.  Drivers use that API
>> when they know about a defect in their device, but the user can
>> override that via syfs.
>>
>> In [1], we have a similar situation with D3cold support, where we're
>> thinking that we should not allow a user to use sysfs to override that
>> driver knowledge.
>>
>> Bjorn
>>
>> [1] https://lore.kernel.org/r/b8a7f4af2b73f6b506ad8ddee59d747cbf834606.1695025365.git.lukas@wunner.de
>>
>>> Fixes: fb097dcd5a28 ("PCI/ASPM: Disable only ASPM_STATE_L1 when driver disables L1")
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/pci/pcie/aspm.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>>> index 3dafba0b5..6d3788257 100644
>>> --- a/drivers/pci/pcie/aspm.c
>>> +++ b/drivers/pci/pcie/aspm.c
>>> @@ -1063,7 +1063,7 @@ static int __pci_disable_link_state(struct pci_dev *pdev, int state, bool sem)
>>>  	if (state & PCIE_LINK_STATE_L0S)
>>>  		link->aspm_disable |= ASPM_STATE_L0S;
>>>  	if (state & PCIE_LINK_STATE_L1)
>>> -		link->aspm_disable |= ASPM_STATE_L1;
>>> +		link->aspm_disable |= ASPM_STATE_L1 | ASPM_STATE_L1SS;
>>>  	if (state & PCIE_LINK_STATE_L1_1)
>>>  		link->aspm_disable |= ASPM_STATE_L1_1;
>>>  	if (state & PCIE_LINK_STATE_L1_2)
>>> @@ -1251,6 +1251,8 @@ static ssize_t aspm_attr_store_common(struct device *dev,
>>>  			link->aspm_disable &= ~ASPM_STATE_L1;
>>>  	} else {
>>>  		link->aspm_disable |= state;
>>> +		if (state & ASPM_STATE_L1)
>>> +			link->aspm_disable |= ASPM_STATE_L1SS;
>>>  	}
>>>  
>>>  	pcie_config_aspm_link(link, policy_to_aspm_state(link));
>>> -- 
>>> 2.42.0
>>>
>