Re: Raphael, I'd like your help upstreaming this VMD power-saving patch, please

[Kenneth Crudup <kenny@panix.com>]

What about a quirk, or whitelist?

That's hacky AF, but that way for systems that this is known to work on, 
it could be enabled?

-K


On 5/12/25 14:09, Bjorn Helgaas wrote:
> [+cc Sergey, Nirmal, Jonathan, Jian-Hong, David]
> 
> On Sun, May 11, 2025 at 01:10:24PM -0700, Kenneth R. Crudup wrote:
>>
>> Hello Raphael,
>>
>> For almost two years now, I've been trying to get patches from Ubuntu that
>> enable ASPM for devices behind Intel's VMD, necessary to get full lower-power
>> states (including very-reduced power usage during s0ix sleep) on my Alderlake
>> (et al.) laptop, upstreamed into mainline.
>>
>> One such thread: https://lore.kernel.org/linux-pm/218aa81f-9c6-5929-578d-8dc15f83dd48@panix.com/
>>
>> Since the original set of patches on this, most of the work has been pushed
>> upstream, with only this last patch required to get fully into the "CPU%LPI"
>> and "SYS%LPI" (names according to "turbostat") states.
>>
>> I'm surprised that with the number of VMD-enabled laptops out there (which I
>> had to keep on so I could dual-boot into Win11 (the disk geometry changes if
>> I disable it, rendering the Win11 partition useless)), that there haven't been
>> many reports of excessive power usage in Linux during sleep; perhaps because
>> many installations are running stock Ubuntu kernels (where I assume variants
>> of this patch remain) it isn't an issue, but I do believe having this upstreamed
>> is still valuable.
>>
>> I don't have the resources you've got to test this fully for regressions, nor
>> the expertise getting a patch into the kernel, so I'd like to again bring this
>> up for discussion (hence the phone-book of a CC: here).
>>
>> If there's anything I can do to help get this done, please let me know.
>>
>> Thank you,
>>
>> -Kenneth Crudup
>>
>> -- 
>> Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange County CA
> 
>>  From ee3618a598a261bbc8a875557d42d6dbbbc4cdd0 Mon Sep 17 00:00:00 2001
>> From: "Kenneth R. Crudup" <kenny@panix.com>
>> Date: Fri, 13 Dec 2024 15:28:42 -0800
>> Subject: [PATCH] PCI/ASPM: Fixup ASPM for VMD bridges
>>
>> Effectively a squashed commit of:
>> UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD domain
>> UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind VMD
>> UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name instead
>> ---
>>   drivers/pci/pcie/aspm.c | 28 +++++++++++++++++++++++++++-
>>   1 file changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index 29fcb0689a91..fdc1ce2755ff 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -788,6 +788,31 @@ static void aspm_l1ss_init(struct pcie_link_state *link)
>>   		aspm_calc_l12_info(link, parent_l1ss_cap, child_l1ss_cap);
>>   }
>>   
>> +/*
>> + * BIOS may not be able to access config space of devices under VMD domain, so
>> + * it relies on software to enable ASPM for links under VMD.
>> + */
>> +static bool pci_fixup_vmd_bridge_enable_aspm(struct pci_dev *pdev)
>> +{
>> +       struct pci_bus *bus = pdev->bus;
>> +       struct device *dev;
>> +       struct pci_driver *pdrv;
>> +
>> +       if (!pci_is_root_bus(bus))
>> +               return false;
>> +
>> +       dev = bus->bridge->parent;
>> +       if (dev == NULL)
>> +               return false;
>> +
>> +       pdrv = pci_dev_driver(to_pci_dev(dev));
>> +       if (pdrv == NULL || strcmp("vmd", pdrv->name))
>> +               return false;
>> +
>> +       pci_info(pdev, "enable ASPM for pci bridge behind vmd");
>> +       return true;
>> +}
>> +
>>   static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>>   {
>>   	struct pci_dev *child = link->downstream, *parent = link->pdev;
>> @@ -866,7 +891,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>>   	}
>>   
>>   	/* Save default state */
>> -	link->aspm_default = link->aspm_enabled;
>> +	link->aspm_default = pci_fixup_vmd_bridge_enable_aspm(parent) ?
>> +		PCIE_LINK_STATE_ASPM_ALL : link->aspm_enabled;
> 
> PCIE_LINK_STATE_ASPM_ALL includes PCIE_LINK_STATE_L1_2, so I think
> this potentially enables L1.2.  The L1.2 configuration depends on
> T_POWER_ON and Common_Mode_Restore_Time, which depend on electrical
> design and are not discoverable by the kernel.  See PCIe r6.0, sec
> 5.5.4:
> 
>    The TPOWER_ON and Common_Mode_Restore_Time fields must be programmed
>    to the appropriate values based on the components and AC coupling
>    capacitors used in the connection linking the two components. The
>    determination of these values is design implementation specific.
> 
> aspm_calc_l12_info() tries to compute these values, but I don't think
> it can do it correctly, and we have a related problem report where
> BIOS set them correctly and then Linux came along and messed them up:
> 
>    https://lore.kernel.org/r/20250407155742.GA178541@bhelgaas
> 
> I think the only thing we can do with L1.2 is rely on values already
> programmed by BIOS.  There is a _DSM method for Latency Tolerance
> Reporting configuration (PCI Firmware r3.3, sec 4.6.6), but AFAICT it
> only addresses the LTR Capability in upstream ports, not the
> T_POWER_ON and Common_Mode_Restore_Time values.
> 
> _HPX Type 2 and 3 records (ACPI r6.5, sec 6.2.9.3 and 6.2.9.4) are
> quite general and maybe there's a way to use them to configure the
> L1.2 parameters, but I am skeptical that these have been implemented
> in Linux and BIOSes.  At least, I don't recall any reports of problems
> or of them being used.
> 
> Maybe something like this patch can be done for ASPM *except* for
> L1.2?
> 
> Bjorn
> 

-- 
Kenneth R. Crudup / Sr. SW Engineer, Scott County Consulting, Orange 
County CA