Re: My AlderLake Dell (XPS-9320) needs these patches to get full standby/low-power modes

[Kenneth Crudup <kenny@panix.com>]

Turns out that my current VMD is already listed as "VMD_FEATS_CLIENT" 
after all:

----
         {PCI_VDEVICE(INTEL, 0x467f),
                 .driver_data = VMD_FEATS_CLIENT,},
----
0000:00:0e.0 0104: 8086:467f
         Subsystem: 1028:0af3
         Flags: bus master, fast devsel, latency 0, IOMMU group 9
         Memory at 603c000000 (64-bit, non-prefetchable) [size=32M]
         Memory at 72000000 (32-bit, non-prefetchable) [size=32M]
         Memory at 6040100000 (64-bit, non-prefetchable) [size=1M]
         Capabilities: [80] MSI-X: Enable+ Count=19 Masked-
         Capabilities: [90] Express Root Complex Integrated Endpoint, MSI 00
         Capabilities: [e0] Power Management version 3
         Kernel driver in use: vmd
----

I'm going to compare the commits you'd pointed out in master to what the 
working (Ubuntu) patches do and figure out where the disconnect is.

-K

On 12/13/24 11:48, Kenneth Crudup wrote:
> 
> I was really hopeful this would have handled it, but no joy.
> 
> I also tried dropping "PCI/ASPM: Enable LTR for endpoints behind VMD" 
> and "PCI/ASPM: Enable ASPM for links under VMD domain" each separately 
> on top of the below quirk patch to no avail.
> 
> The only thing that works is the aggregate patch I've added.
> 
>  > However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
>  > VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
>  > so I'd be surprised if VMD worked for those devices unless BIOS set
>  > up the VMD itself.
> 
> Yeah, my BIOS does- not sure if you'd missed it, but I'd rewritten 
> "Enable LTR for endpoints behind VMD" to print if the BIOS already does 
> that for you, and sent the patch here (since I wasn't seeing the message 
> printed when the fixup was being done and wanted to know why).
> 
> I'd REALLY like to get this into mainline, so if there's anything I can 
> do to help, LMK.
> 
> Thanks,
> -Kenny
> 
> 
> On 12/13/24 08:43, Bjorn Helgaas wrote:
>> [+cc David, Nirmal, linux-pci]
>>
>> On Thu, Dec 12, 2024 at 03:04:53PM -0800, Kenneth Crudup wrote:
>>> On 12/12/24 12:56, Bjorn Helgaas wrote:
>>>> On Wed, Dec 11, 2024 at 03:26:37PM -0800, Kenneth Crudup wrote:
>>>>> So we're on 6.13-rc2 and the patches are getting closer and
>>>>> closer, but they still need to be manually added.
>>>>>
>>>>> The good news is now only (variants of) "PCI/ASPM: Enable LTR
>>>>> for endpoints behind VMD" and "PCI/ASPM: Enable ASPM for links
>>>>> under VMD domain" are needed.
>>> ...
>>
>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>> lunar/commit/?id=47c7bfd31514e7b54a1f830f7707297aebbb8679
>>
>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable LTR for endpoints behind
>> VMD", which writes PCI_LTR_MAX_SNOOP_LAT and PCI_LTR_MAX_NOSNOOP_LAT
>> for PCI_CLASS_STORAGE_EXPRESS devices directly below VMDs with Device
>> IDs 0x9a09, 0xa0b0, or 0xa0bc.
>>
>> This looks equivalent in spirit to upstream
>> https://git.kernel.org/linus/f492edb40b54 ("PCI: vmd: Add quirk to
>> configure PCIe ASPM and LTR"), which writes PCI_LTR_MAX_SNOOP_LAT and
>> PCI_LTR_MAX_NOSNOOP_LAT for any kind of device below VMDs tagged with
>> VMD_FEATS_CLIENT, which includes 0x467f, 0x4c3d, 0x7d0b, 0x9a0b,
>> 0xa77f, 0xad0b, 0xb06f, 0xb60b.
>>
>> However, IDs 0x9a09, 0xa0b0, and 0xa0bc are NOT tagged with
>> VMD_FEATS_CLIENT.  In fact, they're not included in vmd_ids[] at all,
>> so I'd be surprised if VMD worked for those devices unless BIOS set
>> up the VMD itself.
>>
>> Maybe David or Nirmal can comment on this?
>>
>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>> lunar/commit/?id=1a0102a08f206149d9abd56c2b28877c878b5526
>>
>> This is "UBUNTU: SAUCE: PCI/ASPM: Enable ASPM for links under VMD
>> domain", which adds "link->aspm_default = ASPM_STATE_ALL" for device
>> IDs 0x9a09 and 0xa0b0.
>>
>> This looks like it should also be handled by upstream f492edb40b54
>> ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR") [1], which adds
>> "pci_enable_link_state(pdev, PCIE_LINK_STATE_ALL)".
>>
>> But again, the Device IDs mentioned in the Ubuntu commit are NOT
>> included in the upstream VMD_FEATS_CLIENT list.
>>
>>> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/ 
>>> lunar/commit/?id=154d48da2c57514e4b5dadc7b8c70a4edb550981
>>
>> This is "UBUNTU: SAUCE: vmd: fixup bridge ASPM by driver name
>> instead", which applies the quirk that writes PCI_LTR_MAX_SNOOP_LAT
>> and PCI_LTR_MAX_NOSNOOP_LAT for PCI_CLASS_STORAGE_EXPRESS devices
>> below any VMD claimed by the "vmd" driver, not just VMDs with Device
>> IDs 0x9a09, 0xa0b0, or 0xa0bc.
>>
>> I think the only thing that's missing is that the upstream vmd_ids[]
>> needs to be updated with some new VMD Device IDs that are tagged with
>> VMD_FEATS_CLIENT.
>>
>> I don't know what the vmd_ids[] strategy is, but Kenneth, you might
>> try an upstream patch like the one below.  If that resolves the
>> standby/low-power issues, maybe David or Nirmal can figure out the
>> "right" way to do this.
>>
>> Bjorn
>>
>> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
>> index 9d9596947350..4de7ff3bbf23 100644
>> --- a/drivers/pci/controller/vmd.c
>> +++ b/drivers/pci/controller/vmd.c
>> @@ -1109,6 +1109,12 @@ static const struct pci_device_id vmd_ids[] = {
>>                   .driver_data = VMD_FEATS_CLIENT,},
>>       {PCI_VDEVICE(INTEL, 0xb06f),
>>                   .driver_data = VMD_FEATS_CLIENT,},
>> +    {PCI_VDEVICE(INTEL, 0x9a09),
>> +                .driver_data = VMD_FEATS_CLIENT,},
>> +    {PCI_VDEVICE(INTEL, 0xa0b0),
>> +                .driver_data = VMD_FEATS_CLIENT,},
>> +    {PCI_VDEVICE(INTEL, 0xa0bc),
>> +                .driver_data = VMD_FEATS_CLIENT,},
>>       {0,}
>>   };
>>   MODULE_DEVICE_TABLE(pci, vmd_ids);
>>
> 

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