[PATCH v2 RESEND 0/3] agp: convert to generic power management

[PATCH v2 RESEND 0/3] agp: convert to generic power management

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

This is a repost of patches from Vaibhav to convert from legacy PCI power
management to generic power management.  The most recent posting is here:

  https://lore.kernel.org/all/20210112080924.1038907-1-vaibhavgupta40@gmail.com/

Vaibhav has converted around 180 drivers to generic power management, and
over 100 of those conversions have made it upstream.  If we can finish off
the remaining ones, we'll be able to remove quite a bit of ugly legacy code
from the PCI core.

I updated the commit logs to try to make it easier to verify these.  The
patches themselves are unchanged other than being rebased to v5.16-rc1.

Vaibhav Gupta (3):
  amd64-agp: convert to generic power management
  sis-agp: convert to generic power management
  via-agp: convert to generic power management

 drivers/char/agp/amd64-agp.c | 24 ++++++------------------
 drivers/char/agp/sis-agp.c   | 25 ++++++-------------------
 drivers/char/agp/via-agp.c   | 25 +++++--------------------
 3 files changed, 17 insertions(+), 57 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] amd64-agp: convert to generic power management

[Bjorn Helgaas]

From: Vaibhav Gupta <vaibhavgupta40@gmail.com>

Convert amd64-agp from legacy PCI power management to the generic power
management framework.

Previously, amd64-agp used legacy PCI power management.
agp_amd64_suspend() looked like this:

  agp_amd64_suspend
    pci_save_state(pdev)
    pci_set_power_state(pdev, pci_choose_state(pdev, state))

With generic power management, these are both done by the PCI core in
pci_pm_runtime_suspend(), so drop agp_amd64_suspend() completely.

agp_amd64_resume() looked like this:

  agp_amd64_resume
    pci_set_power_state(pdev, PCI_D0)
    pci_restore_state(pdev)
    ...

With generic power management, the PCI parts are done by
pci_pm_runtime_resume(), so drop those from agp_amd64_resume().

[bhelgaas: commit log]
Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/char/agp/amd64-agp.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
index b40edae32817..dc78a4fb879e 100644
--- a/drivers/char/agp/amd64-agp.c
+++ b/drivers/char/agp/amd64-agp.c
@@ -588,20 +588,11 @@ static void agp_amd64_remove(struct pci_dev *pdev)
 	agp_bridges_found--;
 }
 
-#ifdef CONFIG_PM
+#define agp_amd64_suspend NULL
 
-static int agp_amd64_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __maybe_unused agp_amd64_resume(struct device *dev)
 {
-	pci_save_state(pdev);
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
-
-	return 0;
-}
-
-static int agp_amd64_resume(struct pci_dev *pdev)
-{
-	pci_set_power_state(pdev, PCI_D0);
-	pci_restore_state(pdev);
+	struct pci_dev *pdev = to_pci_dev(dev);
 
 	if (pdev->vendor == PCI_VENDOR_ID_NVIDIA)
 		nforce3_agp_init(pdev);
@@ -609,8 +600,6 @@ static int agp_amd64_resume(struct pci_dev *pdev)
 	return amd_8151_configure();
 }
 
-#endif /* CONFIG_PM */
-
 static const struct pci_device_id agp_amd64_pci_table[] = {
 	{
 	.class		= (PCI_CLASS_BRIDGE_HOST << 8),
@@ -738,15 +727,14 @@ static const struct pci_device_id agp_amd64_pci_promisc_table[] = {
 	{ }
 };
 
+static SIMPLE_DEV_PM_OPS(agp_amd64_pm_ops, agp_amd64_suspend, agp_amd64_resume);
+
 static struct pci_driver agp_amd64_pci_driver = {
 	.name		= "agpgart-amd64",
 	.id_table	= agp_amd64_pci_table,
 	.probe		= agp_amd64_probe,
 	.remove		= agp_amd64_remove,
-#ifdef CONFIG_PM
-	.suspend	= agp_amd64_suspend,
-	.resume		= agp_amd64_resume,
-#endif
+	.driver.pm  = &agp_amd64_pm_ops,
 };
 
 
-- 
2.25.1

[PATCH v2 2/3] sis-agp: convert to generic power management

[Bjorn Helgaas]

From: Vaibhav Gupta <vaibhavgupta40@gmail.com>

Convert sis-agp from legacy PCI power management to the generic power
management framework.

Previously, sis-agp used legacy PCI power management.  agp_sis_suspend()
looked like this:

  agp_sis_suspend
    pci_save_state(pdev)
    pci_set_power_state(pdev, pci_choose_state(pdev, state))

With generic power management, these are both done by the PCI core in
pci_pm_runtime_suspend(), so drop agp_sis_suspend() completely.

agp_sis_resume() looked like this:

  agp_sis_resume
    pci_set_power_state(pdev, PCI_D0)
    pci_restore_state(pdev)
    sis_driver.configure()

With generic power management, the PCI parts are done by
pci_pm_runtime_resume(), so drop those from agp_sis_resume().

[bhelgaas: commit log]
Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/char/agp/sis-agp.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/char/agp/sis-agp.c b/drivers/char/agp/sis-agp.c
index 14909fc5d767..f8a02f4bef1b 100644
--- a/drivers/char/agp/sis-agp.c
+++ b/drivers/char/agp/sis-agp.c
@@ -217,26 +217,14 @@ static void agp_sis_remove(struct pci_dev *pdev)
 	agp_put_bridge(bridge);
 }
 
-#ifdef CONFIG_PM
+#define agp_sis_suspend NULL
 
-static int agp_sis_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __maybe_unused agp_sis_resume(
+	__attribute__((unused)) struct device *dev)
 {
-	pci_save_state(pdev);
-	pci_set_power_state(pdev, pci_choose_state(pdev, state));
-
-	return 0;
-}
-
-static int agp_sis_resume(struct pci_dev *pdev)
-{
-	pci_set_power_state(pdev, PCI_D0);
-	pci_restore_state(pdev);
-
 	return sis_driver.configure();
 }
 
-#endif /* CONFIG_PM */
-
 static const struct pci_device_id agp_sis_pci_table[] = {
 	{
 		.class		= (PCI_CLASS_BRIDGE_HOST << 8),
@@ -419,15 +407,14 @@ static const struct pci_device_id agp_sis_pci_table[] = {
 
 MODULE_DEVICE_TABLE(pci, agp_sis_pci_table);
 
+static SIMPLE_DEV_PM_OPS(agp_sis_pm_ops, agp_sis_suspend, agp_sis_resume);
+
 static struct pci_driver agp_sis_pci_driver = {
 	.name		= "agpgart-sis",
 	.id_table	= agp_sis_pci_table,
 	.probe		= agp_sis_probe,
 	.remove		= agp_sis_remove,
-#ifdef CONFIG_PM
-	.suspend	= agp_sis_suspend,
-	.resume		= agp_sis_resume,
-#endif
+	.driver.pm      = &agp_sis_pm_ops,
 };
 
 static int __init agp_sis_init(void)
-- 
2.25.1

[PATCH v2 3/3] via-agp: convert to generic power management

[Bjorn Helgaas]

From: Vaibhav Gupta <vaibhavgupta40@gmail.com>

Convert via-agp from legacy PCI power management to the generic power
management framework.

Previously, via-agp used legacy PCI power management.  agp_via_suspend()
looked like this:

  agp_via_suspend
    pci_save_state(pdev)
    pci_set_power_state(pdev, pci_choose_state(pdev, state))

With generic power management, these are both done by the PCI core in
pci_pm_runtime_suspend(), so drop agp_via_suspend() completely.

agp_via_resume() looked like this:

  agp_via_resume
    pci_set_power_state(pdev, PCI_D0)
    pci_restore_state(pdev)
    ...

With generic power management, the PCI parts are done by
pci_pm_runtime_resume(), so drop those from agp_via_resume().

[bhelgaas: commit log]
Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/char/agp/via-agp.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/char/agp/via-agp.c b/drivers/char/agp/via-agp.c
index 87a92a044570..a460ae352772 100644
--- a/drivers/char/agp/via-agp.c
+++ b/drivers/char/agp/via-agp.c
@@ -492,22 +492,11 @@ static void agp_via_remove(struct pci_dev *pdev)
 	agp_put_bridge(bridge);
 }
 
-#ifdef CONFIG_PM
+#define agp_via_suspend NULL
 
-static int agp_via_suspend(struct pci_dev *pdev, pm_message_t state)
+static int __maybe_unused agp_via_resume(struct device *dev)
 {
-	pci_save_state (pdev);
-	pci_set_power_state (pdev, PCI_D3hot);
-
-	return 0;
-}
-
-static int agp_via_resume(struct pci_dev *pdev)
-{
-	struct agp_bridge_data *bridge = pci_get_drvdata(pdev);
-
-	pci_set_power_state (pdev, PCI_D0);
-	pci_restore_state(pdev);
+	struct agp_bridge_data *bridge = dev_get_drvdata(dev);
 
 	if (bridge->driver == &via_agp3_driver)
 		return via_configure_agp3();
@@ -517,8 +506,6 @@ static int agp_via_resume(struct pci_dev *pdev)
 	return 0;
 }
 
-#endif /* CONFIG_PM */
-
 /* must be the same order as name table above */
 static const struct pci_device_id agp_via_pci_table[] = {
 #define ID(x) \
@@ -567,16 +554,14 @@ static const struct pci_device_id agp_via_pci_table[] = {
 
 MODULE_DEVICE_TABLE(pci, agp_via_pci_table);
 
+static SIMPLE_DEV_PM_OPS(agp_via_pm_ops, agp_via_suspend, agp_via_resume);
 
 static struct pci_driver agp_via_pci_driver = {
 	.name		= "agpgart-via",
 	.id_table	= agp_via_pci_table,
 	.probe		= agp_via_probe,
 	.remove		= agp_via_remove,
-#ifdef CONFIG_PM
-	.suspend	= agp_via_suspend,
-	.resume		= agp_via_resume,
-#endif
+	.driver.pm      = &agp_via_pm_ops,
 };
 
 
-- 
2.25.1

Re: [PATCH v2 1/3] amd64-agp: convert to generic power management

[Bjorn Helgaas]

[cc->to: Rafael: help :)]

On Tue, Nov 30, 2021 at 08:54:17PM -0600, Bjorn Helgaas wrote:
> From: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> 
> Convert amd64-agp from legacy PCI power management to the generic power
> management framework.
> 
> Previously, amd64-agp used legacy PCI power management.
> agp_amd64_suspend() looked like this:
> 
>   agp_amd64_suspend
>     pci_save_state(pdev)
>     pci_set_power_state(pdev, pci_choose_state(pdev, state))
> 
> With generic power management, these are both done by the PCI core in
> pci_pm_runtime_suspend(), so drop agp_amd64_suspend() completely.

I think the *patch* is correct, but my explanation is wrong.  Would
appreciate any corrections!

Prior to this patch, agp_amd64_suspend() is a pci_driver.suspend()
method and is called in this path:

  pci_pm_suspend
    pci_legacy_suspend
      drv->suspend

After this patch, agp_amd64_suspend() is not implemented at all, and
we do the pci_save_state() and pci_set_power_state() in PCI generic
code.

But I think those actually happen in pci_pm_suspend_noirq(), not in
pci_pm_runtime_suspend(), i.e., in this path:

  suspend_devices_and_enter
    dpm_suspend_start(PMSG_SUSPEND)
      dpm_suspend(PMSG_SUSPEND)
        device_suspend
          __device_suspend
            callback = pm_op(dev->bus->pm, state)
            dpm_run_callback(callback)
              pci_pm_suspend                            # PCI bus method
                dev->driver->pm->suspend
                  agp_amd64_suspend                     # <-- no longer needed
    suspend_enter
      dpm_suspend_noirq(PMSG_SUSPEND)
        dpm_noirq_suspend_devices(PMSG_SUSPEND)
          device_suspend_noirq
            __device_suspend_noirq
              callback = pm_noirq_op(dev->bus->pm, state)
              dpm_run_callback(callback)
                pci_pm_suspend_noirq                    # PCI bus method
                  pci_save_state                        # <-- now done here
                  pci_prepare_to_sleep
                    pci_set_power_state                 # <-- and here
                  
I got confused because I couldn't find the call chain leading to
pci_pm_suspend_noirq().  pm_op() and dpm_run_callback() essentially
break the call chain, which makes it a little hard to follow.

> agp_amd64_resume() looked like this:
> 
>   agp_amd64_resume
>     pci_set_power_state(pdev, PCI_D0)
>     pci_restore_state(pdev)
>     ...
> 
> With generic power management, the PCI parts are done by
> pci_pm_runtime_resume(), so drop those from agp_amd64_resume().
> 
> [bhelgaas: commit log]
> Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>  drivers/char/agp/amd64-agp.c | 24 ++++++------------------
>  1 file changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/char/agp/amd64-agp.c b/drivers/char/agp/amd64-agp.c
> index b40edae32817..dc78a4fb879e 100644
> --- a/drivers/char/agp/amd64-agp.c
> +++ b/drivers/char/agp/amd64-agp.c
> @@ -588,20 +588,11 @@ static void agp_amd64_remove(struct pci_dev *pdev)
>  	agp_bridges_found--;
>  }
>  
> -#ifdef CONFIG_PM
> +#define agp_amd64_suspend NULL
>  
> -static int agp_amd64_suspend(struct pci_dev *pdev, pm_message_t state)
> +static int __maybe_unused agp_amd64_resume(struct device *dev)
>  {
> -	pci_save_state(pdev);
> -	pci_set_power_state(pdev, pci_choose_state(pdev, state));
> -
> -	return 0;
> -}
> -
> -static int agp_amd64_resume(struct pci_dev *pdev)
> -{
> -	pci_set_power_state(pdev, PCI_D0);
> -	pci_restore_state(pdev);
> +	struct pci_dev *pdev = to_pci_dev(dev);
>  
>  	if (pdev->vendor == PCI_VENDOR_ID_NVIDIA)
>  		nforce3_agp_init(pdev);
> @@ -609,8 +600,6 @@ static int agp_amd64_resume(struct pci_dev *pdev)
>  	return amd_8151_configure();
>  }
>  
> -#endif /* CONFIG_PM */
> -
>  static const struct pci_device_id agp_amd64_pci_table[] = {
>  	{
>  	.class		= (PCI_CLASS_BRIDGE_HOST << 8),
> @@ -738,15 +727,14 @@ static const struct pci_device_id agp_amd64_pci_promisc_table[] = {
>  	{ }
>  };
>  
> +static SIMPLE_DEV_PM_OPS(agp_amd64_pm_ops, agp_amd64_suspend, agp_amd64_resume);
> +
>  static struct pci_driver agp_amd64_pci_driver = {
>  	.name		= "agpgart-amd64",
>  	.id_table	= agp_amd64_pci_table,
>  	.probe		= agp_amd64_probe,
>  	.remove		= agp_amd64_remove,
> -#ifdef CONFIG_PM
> -	.suspend	= agp_amd64_suspend,
> -	.resume		= agp_amd64_resume,
> -#endif
> +	.driver.pm  = &agp_amd64_pm_ops,
>  };
>  
>  
> -- 
> 2.25.1
>