> On Dec 9, 2020, at 01:11, Heiner Kallweit <hkallweit1@xxxxxxxxx> wrote: > > Am 08.12.2020 um 09:25 schrieb Kai-Heng Feng: >> If we use sysfs to disable L1 ASPM, then enable one L1 ASPM substate >> again, all other substates will also be enabled too: >> >> link# grep . * >> clkpm:1 >> l0s_aspm:1 >> l1_1_aspm:1 >> l1_1_pcipm:1 >> l1_2_aspm:1 >> l1_2_pcipm:1 >> l1_aspm:1 >> >> link# echo 0 > l1_aspm >> >> link# grep . * >> clkpm:1 >> l0s_aspm:1 >> l1_1_aspm:0 >> l1_1_pcipm:0 >> l1_2_aspm:0 >> l1_2_pcipm:0 >> l1_aspm:0 >> >> link# echo 1 > l1_2_aspm >> >> link# grep . * >> clkpm:1 >> l0s_aspm:1 >> l1_1_aspm:1 >> l1_1_pcipm:1 >> l1_2_aspm:1 >> l1_2_pcipm:1 >> l1_aspm:1 >> >> This is because disabled ASPM states weren't saved, so enable any of the >> substate will also enable others. >> >> So store the disabled ASPM states for consistency. >> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> >> --- >> drivers/pci/pcie/aspm.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c >> index ac0557a305af..2ea9fddadfad 100644 >> --- a/drivers/pci/pcie/aspm.c >> +++ b/drivers/pci/pcie/aspm.c >> @@ -658,6 +658,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist) >> /* Setup initial capable state. Will be updated later */ >> link->aspm_capable = link->aspm_support; >> >> + link->aspm_disable = link->aspm_capable & ~link->aspm_default; >> + > > This makes sense only in combination with patch 2. However I think patch 1 > should be independent of patch 2. Especially if we consider patch 1 a fix > that is applied to stable whilst patch 2 is an improvement for next. > >> /* Get and check endpoint acceptable latencies */ >> list_for_each_entry(child, &linkbus->devices, bus_list) { >> u32 reg32, encoding; >> @@ -1226,11 +1228,15 @@ static ssize_t aspm_attr_store_common(struct device *dev, >> mutex_lock(&aspm_lock); >> >> if (state_enable) { >> - link->aspm_disable &= ~state; >> /* need to enable L1 for substates */ >> if (state & ASPM_STATE_L1SS) >> - link->aspm_disable &= ~ASPM_STATE_L1; >> + state |= ASPM_STATE_L1; >> + >> + link->aspm_disable &= ~state; > > I don't see what this part of the patch changes. Can you elaborate on why > this is needed? No this is just a cosmetic change. Of course "cosmetic" is really subjective. I'll drop this part in v2. > >> } else { >> + if (state == ASPM_STATE_L1) >> + state |= ASPM_STATE_L1SS; >> + > > I think this part should be sufficient to fix the behavior. because what > I think currently happens: > > 1. original status: policy powersupersave, nothing disabled -> L1 + L1SS active > 2. disable L1: L1 disabled, pcie_config_aspm_link() disabled L1 and L1SS > w/o adding L1SS to link-> aspm_disabled > 3. enable one L1SS state: aspm_attr_store_common() removes L1 from > link->aspm_disabled -> link->aspm_disabled is empty, resulting in > L1 + L1SS being active Yes. This is the case the patch solves. Kai-Heng > >> link->aspm_disable |= state; >> }