diff mbox series

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

Message ID 20220825015618.39518-3-acelan.kao@canonical.com
State New
Headers show
Series System freeze after resuming from suspend due to PCI ASPM settings | expand

Commit Message

AceLan Kao Aug. 25, 2022, 1:56 a.m. UTC 
From: "Chia-Lin Kao (AceLan)" <acelan.kao@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/

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@canonical.com>
---
 drivers/pci/pci.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

Comments

Stefan Bader Aug. 25, 2022, 8:15 a.m. UTC | #1 
On 25.08.22 03:56, AceLan Kao wrote:
> From: "Chia-Lin Kao (AceLan)" <acelan.kao@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@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);
>   }
AceLan Kao Aug. 26, 2022, 1:44 a.m. UTC | #2 
Stefan Bader <stefan.bader@canonical.com> 於 2022年8月25日 週四 下午4:15寫道：
>
> On 25.08.22 03:56, AceLan Kao wrote:
> > From: "Chia-Lin Kao (AceLan)" <acelan.kao@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?
Limit the state change is from this commit
https://patchwork.ozlabs.org/project/linux-pci/patch/20220509073639.2048236-1-kai.heng.feng@canonical.com/
We have applied this commit on the OEM kernels, so I want to apply
this on Jammy 5.15, too.
But I can't apply this commit, because it removes the function, so I
apply the dmi check on the if statement to archive the same thing.

>
> -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@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);
> >   }
>
diff mbox series

Patch

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);
 }