[PATCH v2 2/2][SRU][J] UBUNTU: SAUCE: whitelist platforms that needs save/restore ASPM L1SS for suspend/resume

[Stefan Bader]

On 25.08.22 03:56, AceLan Kao wrote:
> From: "Chia-Lin Kao (AceLan)" <acelan.kao at canonical.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1980829
> 
> Add a DMI quirk for the previous commit
> "PCI/ASPM: Save/restore L1SS Capability for suspend/resume"
> The DMI quirk lists the platforms that needs this patch, and also
> applied the concept of the below commit to not call
> pcie_aspm_pm_state_change() if the platform is listed in the whitelist
> https://patchwork.ozlabs.org/project/linux-pci/patch/20220509073639.2048236-1-kai.heng.feng@canonical.com/

To limit the save/restore calls makes sense. However it is hard to say whether 
limiting the state change as well is good or bad. At least it feels like it 
might change behavior for other platforms... Why was this done?

-Stefan

> 
> v2.
>     1. added the missing null terminator at the end of the quirk
>     2. changed the DMI match for LENOVO to its DMI_BIOS_VERSION
> 
> Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao at canonical.com>
> ---
>   drivers/pci/pci.c | 35 ++++++++++++++++++++++++++++-------
>   1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 5c7ef86db60b..d8b71a7b8cd0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -166,6 +166,23 @@ __setup("pcie_port_pm=", pcie_port_pm_setup);
>   /* Time to wait after a reset for device to become responsive */
>   #define PCIE_RESET_READY_POLL_MS 60000
>   
> +static const struct dmi_system_id aspm_fix_whitelist[] = {
> +	{
> +		.ident = "LENOVO Stealth Thinkstation",
> +		.matches = {
> +			DMI_MATCH(DMI_BIOS_VERSION, "S07K"),
> +		},
> +	},
> +	{
> +		.ident = "Dell Inc. Precision 7960 Tower",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 7960 Tower"),
> +		},
> +	},
> +	{}
> +};
> +
>   /**
>    * pci_bus_max_busnr - returns maximum PCI bus number of given bus' children
>    * @bus: pointer to PCI bus structure to search
> @@ -1143,7 +1160,7 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
>   	if (need_restore)
>   		pci_restore_bars(dev);
>   
> -	if (dev->bus->self)
> +	if (dev->bus->self && !dmi_check_system(aspm_fix_whitelist))
>   		pcie_aspm_pm_state_change(dev->bus->self);
>   
>   	return 0;
> @@ -1623,7 +1640,8 @@ int pci_save_state(struct pci_dev *dev)
>   		return i;
>   
>   	pci_save_ltr_state(dev);
> -	pci_save_aspm_l1ss_state(dev);
> +	if (dmi_check_system(aspm_fix_whitelist))
> +		pci_save_aspm_l1ss_state(dev);
>   	pci_save_dpc_state(dev);
>   	pci_save_aer_state(dev);
>   	pci_save_ptm_state(dev);
> @@ -1730,7 +1748,8 @@ void pci_restore_state(struct pci_dev *dev)
>   	 * LTR itself (in the PCIe capability).
>   	 */
>   	pci_restore_ltr_state(dev);
> -	pci_restore_aspm_l1ss_state(dev);
> +	if (dmi_check_system(aspm_fix_whitelist))
> +		pci_restore_aspm_l1ss_state(dev);
>   
>   	pci_restore_pcie_state(dev);
>   	pci_restore_pasid_state(dev);
> @@ -3437,10 +3456,12 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
>   	if (error)
>   		pci_err(dev, "unable to allocate suspend buffer for LTR\n");
>   
> -	error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_L1SS,
> -					    2 * sizeof(u32));
> -	if (error)
> -		pci_err(dev, "unable to allocate suspend buffer for ASPM-L1SS\n");
> +	if (dmi_check_system(aspm_fix_whitelist)) {
> +		error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_L1SS,
> +						    2 * sizeof(u32));
> +		if (error)
> +			pci_err(dev, "unable to allocate suspend buffer for ASPM-L1SS\n");
> +	}
>   
>   	pci_allocate_vc_save_buffers(dev);
>   }

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20220825/08f84946/attachment.sig>