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? 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@xxxxxxxxx > >> Fixes: fb097dcd5a28 ("PCI/ASPM: Disable only ASPM_STATE_L1 when driver disables L1") >> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx> >> --- >> 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 >> -- Sathyanarayanan Kuppuswamy Linux Kernel Developer
