[PATCH 0/6] PCI/PM: Minor fix and cleanups

[PATCH 0/6] PCI/PM: Minor fix and cleanups

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

First, fix our D2 delay (which I think was just a bug -- we delayed in
microseconds instead of milliseconds, but I'm not sure we really even use
D2).

The rest are just cleanups that should not change any behavior.  These are
based on my current pci/pm branch (0d1685046e61) and I pushed them to
pci/pm-2 for now.

Bjorn Helgaas (6):
  PCI/PM: Apply D2 delay as milliseconds, not microseconds
  PCI/PM: Expand PM reset messages to mention D3hot (not just D3)
  PCI/PM: Simplify pci_set_power_state()
  xen-platform: Convert to generic power management
  PCI/PM: Remove unused pci_driver.resume_early() hook
  PCI/PM: Remove unused pci_driver.suspend_late() hook

 Documentation/power/pci.rst | 10 ++++-----
 drivers/pci/pci-driver.c    | 43 ++++++-------------------------------
 drivers/pci/pci.c           |  8 +++----
 drivers/xen/platform-pci.c  | 10 ++++++---
 include/linux/pci.h         |  4 ----
 5 files changed, 22 insertions(+), 53 deletions(-)

-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH 1/6] PCI/PM: Apply D2 delay as milliseconds, not microseconds

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

PCI_PM_D2_DELAY is defined as 200, which is milliseconds, but previously we
used udelay(), which only waited for 200 microseconds.  Use msleep()
instead so we wait the correct amount of time.  See PCIe r5.0, sec 5.9.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index e7982af9a5d8..cd96874ae76d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -886,7 +886,7 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
 	if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
 		pci_dev_d3_sleep(dev);
 	else if (state == PCI_D2 || dev->current_state == PCI_D2)
-		udelay(PCI_PM_D2_DELAY);
+		msleep(PCI_PM_D2_DELAY);
 
 	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
 	dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH 2/6] PCI/PM: Expand PM reset messages to mention D3hot (not just D3)

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

pci_pm_reset() resets a device by putting it in D3hot and bringing it back
to D0.  Clarify related messages to mention "D3hot" explicitly instead of
just "D3".

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index cd96874ae76d..a9d3653ef960 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4605,7 +4605,7 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
 	pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr);
 	pci_dev_d3_sleep(dev);
 
-	return pci_dev_wait(dev, "PM D3->D0", PCIE_RESET_READY_POLL_MS);
+	return pci_dev_wait(dev, "PM D3hot->D0", PCIE_RESET_READY_POLL_MS);
 }
 /**
  * pcie_wait_for_link - Wait until link is active or inactive
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH 3/6] PCI/PM: Simplify pci_set_power_state()

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Check for the PCI_DEV_FLAGS_NO_D3 quirk early, before calling
__pci_start_power_transition().  This way all the cases where we don't need
to do anything at all are checked up front.

This doesn't fix anything because if the caller requested D3hot or D3cold,
__pci_start_power_transition() is a no-op.  But calling it is pointless and
makes the code harder to analyze.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a9d3653ef960..281bc8fc51b3 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1130,8 +1130,6 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	if (dev->current_state == state)
 		return 0;
 
-	__pci_start_power_transition(dev, state);
-
 	/*
 	 * This device is quirked not to be put into D3, so don't put it in
 	 * D3
@@ -1139,6 +1137,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
 	if (state >= PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
 		return 0;
 
+	__pci_start_power_transition(dev, state);
+
 	/*
 	 * To put device in D3cold, we put device into D3hot in native
 	 * way, then put device into D3cold with platform ops
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH 4/6] xen-platform: Convert to generic power management

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Convert xen-platform from the legacy PCI power management callbacks to the
generic operations.  This is one step towards removing support for the
legacy PCI callbacks.

The generic .resume_noirq() operation is called by pci_pm_resume_noirq() at
the same point the legacy PCI .resume_early() callback was, so this patch
should not change the xen-platform behavior.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: KarimAllah Ahmed <karahmed@amazon.de>
---
 drivers/xen/platform-pci.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/xen/platform-pci.c b/drivers/xen/platform-pci.c
index 5e30602fdbad..e06e8769eb84 100644
--- a/drivers/xen/platform-pci.c
+++ b/drivers/xen/platform-pci.c
@@ -168,13 +168,17 @@ static const struct pci_device_id platform_pci_tbl[] = {
 	{0,}
 };
 
+static struct dev_pm_ops platform_pm_ops = {
+	.resume_noirq =   platform_pci_resume,
+};
+
 static struct pci_driver platform_driver = {
 	.name =           DRV_NAME,
 	.probe =          platform_pci_probe,
 	.id_table =       platform_pci_tbl,
-#ifdef CONFIG_PM
-	.resume_early =   platform_pci_resume,
-#endif
+	.driver = {
+		.pm =     &platform_pm_ops,
+	},
 };
 
 builtin_pci_driver(platform_driver);
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH 5/6] PCI/PM: Remove unused pci_driver.resume_early() hook

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

The struct pci_driver.resume_early() hook is one of the legacy PCI power
management callbacks, and there are no remaining users of it.  Remove it.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 Documentation/power/pci.rst |  2 +-
 drivers/pci/pci-driver.c    | 23 ++++++-----------------
 include/linux/pci.h         |  2 --
 3 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/Documentation/power/pci.rst b/Documentation/power/pci.rst
index a90e82c70a3b..ff7029b94068 100644
--- a/Documentation/power/pci.rst
+++ b/Documentation/power/pci.rst
@@ -692,7 +692,7 @@ controlling the runtime power management of their devices.
 At the time of this writing there are two ways to define power management
 callbacks for a PCI device driver, the recommended one, based on using a
 dev_pm_ops structure described in Documentation/driver-api/pm/devices.rst, and
-the "legacy" one, in which the .suspend(), .suspend_late(), .resume_early(), and
+the "legacy" one, in which the .suspend(), .suspend_late(), and
 .resume() callbacks from struct pci_driver are used.  The legacy approach,
 however, doesn't allow one to define runtime power management callbacks and is
 not really suitable for any new drivers.  Therefore it is not covered by this
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 5337cbbd69de..fc372c2d529a 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -630,15 +630,6 @@ static int pci_legacy_suspend_late(struct device *dev, pm_message_t state)
 	return 0;
 }
 
-static int pci_legacy_resume_early(struct device *dev)
-{
-	struct pci_dev *pci_dev = to_pci_dev(dev);
-	struct pci_driver *drv = pci_dev->driver;
-
-	return drv && drv->resume_early ?
-			drv->resume_early(pci_dev) : 0;
-}
-
 static int pci_legacy_resume(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
@@ -662,8 +653,7 @@ static void pci_pm_default_suspend(struct pci_dev *pci_dev)
 static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev)
 {
 	struct pci_driver *drv = pci_dev->driver;
-	bool ret = drv && (drv->suspend || drv->suspend_late || drv->resume
-		|| drv->resume_early);
+	bool ret = drv && (drv->suspend || drv->suspend_late || drv->resume);
 
 	/*
 	 * Legacy PM support is used by default, so warn if the new framework is
@@ -944,7 +934,7 @@ static int pci_pm_resume_noirq(struct device *dev)
 	pcie_pme_root_status_cleanup(pci_dev);
 
 	if (pci_has_legacy_pm_support(pci_dev))
-		return pci_legacy_resume_early(dev);
+		return 0;
 
 	if (pm && pm->resume_noirq)
 		return pm->resume_noirq(dev);
@@ -1074,9 +1064,8 @@ static int pci_pm_thaw_noirq(struct device *dev)
 	}
 
 	/*
-	 * Both the legacy ->resume_early() and the new pm->thaw_noirq()
-	 * callbacks assume the device has been returned to D0 and its
-	 * config state has been restored.
+	 * The pm->thaw_noirq() callback assumes the device has been
+	 * returned to D0 and its config state has been restored.
 	 *
 	 * In addition, pci_restore_state() restores MSI-X state in MMIO
 	 * space, which requires the device to be in D0, so return it to D0
@@ -1087,7 +1076,7 @@ static int pci_pm_thaw_noirq(struct device *dev)
 	pci_restore_state(pci_dev);
 
 	if (pci_has_legacy_pm_support(pci_dev))
-		return pci_legacy_resume_early(dev);
+		return 0;
 
 	if (pm && pm->thaw_noirq)
 		return pm->thaw_noirq(dev);
@@ -1219,7 +1208,7 @@ static int pci_pm_restore_noirq(struct device *dev)
 	pci_fixup_device(pci_fixup_resume_early, pci_dev);
 
 	if (pci_has_legacy_pm_support(pci_dev))
-		return pci_legacy_resume_early(dev);
+		return 0;
 
 	if (pm && pm->restore_noirq)
 		return pm->restore_noirq(dev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 4846306d521c..dd4596fc1208 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -806,7 +806,6 @@ struct module;
  *		context, so it can sleep.
  * @suspend:	Put device into low power state.
  * @suspend_late: Put device into low power state.
- * @resume_early: Wake device from low power state.
  * @resume:	Wake device from low power state.
  *		(Please see Documentation/power/pci.rst for descriptions
  *		of PCI Power Management and the related functions.)
@@ -830,7 +829,6 @@ struct pci_driver {
 	void (*remove)(struct pci_dev *dev);	/* Device removed (NULL if not a hot-plug capable driver) */
 	int  (*suspend)(struct pci_dev *dev, pm_message_t state);	/* Device suspended */
 	int  (*suspend_late)(struct pci_dev *dev, pm_message_t state);
-	int  (*resume_early)(struct pci_dev *dev);
 	int  (*resume)(struct pci_dev *dev);	/* Device woken up */
 	void (*shutdown)(struct pci_dev *dev);
 	int  (*sriov_configure)(struct pci_dev *dev, int num_vfs); /* On PF */
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH 6/6] PCI/PM: Remove unused pci_driver.suspend_late() hook

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

The struct pci_driver.suspend_late() hook is one of the legacy PCI power
management callbacks, and there are no remaining users of it.  Remove it.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 Documentation/power/pci.rst | 10 +++++-----
 drivers/pci/pci-driver.c    | 22 +---------------------
 include/linux/pci.h         |  2 --
 3 files changed, 6 insertions(+), 28 deletions(-)

diff --git a/Documentation/power/pci.rst b/Documentation/power/pci.rst
index ff7029b94068..0924d29636ad 100644
--- a/Documentation/power/pci.rst
+++ b/Documentation/power/pci.rst
@@ -692,11 +692,11 @@ controlling the runtime power management of their devices.
 At the time of this writing there are two ways to define power management
 callbacks for a PCI device driver, the recommended one, based on using a
 dev_pm_ops structure described in Documentation/driver-api/pm/devices.rst, and
-the "legacy" one, in which the .suspend(), .suspend_late(), and
-.resume() callbacks from struct pci_driver are used.  The legacy approach,
-however, doesn't allow one to define runtime power management callbacks and is
-not really suitable for any new drivers.  Therefore it is not covered by this
-document (refer to the source code to learn more about it).
+the "legacy" one, in which the .suspend() and .resume() callbacks from struct
+pci_driver are used.  The legacy approach, however, doesn't allow one to define
+runtime power management callbacks and is not really suitable for any new
+drivers.  Therefore it is not covered by this document (refer to the source code
+to learn more about it).
 
 It is recommended that all PCI device drivers define a struct dev_pm_ops object
 containing pointers to power management (PM) callbacks that will be executed by
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index fc372c2d529a..e89fd90eaa93 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -599,32 +599,12 @@ static int pci_legacy_suspend(struct device *dev, pm_message_t state)
 static int pci_legacy_suspend_late(struct device *dev, pm_message_t state)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	struct pci_driver *drv = pci_dev->driver;
-
-	if (drv && drv->suspend_late) {
-		pci_power_t prev = pci_dev->current_state;
-		int error;
-
-		error = drv->suspend_late(pci_dev, state);
-		suspend_report_result(drv->suspend_late, error);
-		if (error)
-			return error;
-
-		if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
-		    && pci_dev->current_state != PCI_UNKNOWN) {
-			pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
-				      "PCI PM: Device state not saved by %pS\n",
-				      drv->suspend_late);
-			goto Fixup;
-		}
-	}
 
 	if (!pci_dev->state_saved)
 		pci_save_state(pci_dev);
 
 	pci_pm_set_unknown_state(pci_dev);
 
-Fixup:
 	pci_fixup_device(pci_fixup_suspend_late, pci_dev);
 
 	return 0;
@@ -653,7 +633,7 @@ static void pci_pm_default_suspend(struct pci_dev *pci_dev)
 static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev)
 {
 	struct pci_driver *drv = pci_dev->driver;
-	bool ret = drv && (drv->suspend || drv->suspend_late || drv->resume);
+	bool ret = drv && (drv->suspend || drv->resume);
 
 	/*
 	 * Legacy PM support is used by default, so warn if the new framework is
diff --git a/include/linux/pci.h b/include/linux/pci.h
index dd4596fc1208..9b0e35e09874 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -805,7 +805,6 @@ struct module;
  *		The remove function always gets called from process
  *		context, so it can sleep.
  * @suspend:	Put device into low power state.
- * @suspend_late: Put device into low power state.
  * @resume:	Wake device from low power state.
  *		(Please see Documentation/power/pci.rst for descriptions
  *		of PCI Power Management and the related functions.)
@@ -828,7 +827,6 @@ struct pci_driver {
 	int  (*probe)(struct pci_dev *dev, const struct pci_device_id *id);	/* New device inserted */
 	void (*remove)(struct pci_dev *dev);	/* Device removed (NULL if not a hot-plug capable driver) */
 	int  (*suspend)(struct pci_dev *dev, pm_message_t state);	/* Device suspended */
-	int  (*suspend_late)(struct pci_dev *dev, pm_message_t state);
 	int  (*resume)(struct pci_dev *dev);	/* Device woken up */
 	void (*shutdown)(struct pci_dev *dev);
 	int  (*sriov_configure)(struct pci_dev *dev, int num_vfs); /* On PF */
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

Re: [PATCH 1/6] PCI/PM: Apply D2 delay as milliseconds, not microseconds

[Rafael J. Wysocki]

On Fri, Nov 1, 2019 at 9:46 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> PCI_PM_D2_DELAY is defined as 200, which is milliseconds, but previously we
> used udelay(), which only waited for 200 microseconds.  Use msleep()
> instead so we wait the correct amount of time.  See PCIe r5.0, sec 5.9.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index e7982af9a5d8..cd96874ae76d 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -886,7 +886,7 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
>         if (state == PCI_D3hot || dev->current_state == PCI_D3hot)
>                 pci_dev_d3_sleep(dev);
>         else if (state == PCI_D2 || dev->current_state == PCI_D2)
> -               udelay(PCI_PM_D2_DELAY);
> +               msleep(PCI_PM_D2_DELAY);
>
>         pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
>         dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
> --
> 2.24.0.rc1.363.gb1bccd3e3d-goog
>

Re: [PATCH 2/6] PCI/PM: Expand PM reset messages to mention D3hot (not just D3)

[Rafael J. Wysocki]

On Fri, Nov 1, 2019 at 9:46 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> pci_pm_reset() resets a device by putting it in D3hot and bringing it back
> to D0.  Clarify related messages to mention "D3hot" explicitly instead of
> just "D3".
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/pci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index cd96874ae76d..a9d3653ef960 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4605,7 +4605,7 @@ static int pci_pm_reset(struct pci_dev *dev, int probe)
>         pci_write_config_word(dev, dev->pm_cap + PCI_PM_CTRL, csr);
>         pci_dev_d3_sleep(dev);
>
> -       return pci_dev_wait(dev, "PM D3->D0", PCIE_RESET_READY_POLL_MS);
> +       return pci_dev_wait(dev, "PM D3hot->D0", PCIE_RESET_READY_POLL_MS);
>  }
>  /**
>   * pcie_wait_for_link - Wait until link is active or inactive
> --
> 2.24.0.rc1.363.gb1bccd3e3d-goog
>

Re: [PATCH 3/6] PCI/PM: Simplify pci_set_power_state()

[Rafael J. Wysocki]

On Fri, Nov 1, 2019 at 9:46 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Check for the PCI_DEV_FLAGS_NO_D3 quirk early, before calling
> __pci_start_power_transition().  This way all the cases where we don't need
> to do anything at all are checked up front.
>
> This doesn't fix anything because if the caller requested D3hot or D3cold,
> __pci_start_power_transition() is a no-op.  But calling it is pointless and
> makes the code harder to analyze.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/pci/pci.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index a9d3653ef960..281bc8fc51b3 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1130,8 +1130,6 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>         if (dev->current_state == state)
>                 return 0;
>
> -       __pci_start_power_transition(dev, state);
> -
>         /*
>          * This device is quirked not to be put into D3, so don't put it in
>          * D3
> @@ -1139,6 +1137,8 @@ int pci_set_power_state(struct pci_dev *dev, pci_power_t state)
>         if (state >= PCI_D3hot && (dev->dev_flags & PCI_DEV_FLAGS_NO_D3))
>                 return 0;
>
> +       __pci_start_power_transition(dev, state);
> +
>         /*
>          * To put device in D3cold, we put device into D3hot in native
>          * way, then put device into D3cold with platform ops
> --
> 2.24.0.rc1.363.gb1bccd3e3d-goog
>

Re: [PATCH 4/6] xen-platform: Convert to generic power management

[Rafael J. Wysocki]

On Fri, Nov 1, 2019 at 9:46 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Convert xen-platform from the legacy PCI power management callbacks to the
> generic operations.  This is one step towards removing support for the
> legacy PCI callbacks.
>
> The generic .resume_noirq() operation is called by pci_pm_resume_noirq() at
> the same point the legacy PCI .resume_early() callback was, so this patch
> should not change the xen-platform behavior.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: KarimAllah Ahmed <karahmed@amazon.de>

Good idea:

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/xen/platform-pci.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/xen/platform-pci.c b/drivers/xen/platform-pci.c
> index 5e30602fdbad..e06e8769eb84 100644
> --- a/drivers/xen/platform-pci.c
> +++ b/drivers/xen/platform-pci.c
> @@ -168,13 +168,17 @@ static const struct pci_device_id platform_pci_tbl[] = {
>         {0,}
>  };
>
> +static struct dev_pm_ops platform_pm_ops = {
> +       .resume_noirq =   platform_pci_resume,
> +};
> +
>  static struct pci_driver platform_driver = {
>         .name =           DRV_NAME,
>         .probe =          platform_pci_probe,
>         .id_table =       platform_pci_tbl,
> -#ifdef CONFIG_PM
> -       .resume_early =   platform_pci_resume,
> -#endif
> +       .driver = {
> +               .pm =     &platform_pm_ops,
> +       },
>  };
>
>  builtin_pci_driver(platform_driver);
> --
> 2.24.0.rc1.363.gb1bccd3e3d-goog
>

Re: [PATCH 5/6] PCI/PM: Remove unused pci_driver.resume_early() hook

[Rafael J. Wysocki]

On Fri, Nov 1, 2019 at 9:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> The struct pci_driver.resume_early() hook is one of the legacy PCI power
> management callbacks, and there are no remaining users of it.  Remove it.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  Documentation/power/pci.rst |  2 +-
>  drivers/pci/pci-driver.c    | 23 ++++++-----------------
>  include/linux/pci.h         |  2 --
>  3 files changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/power/pci.rst b/Documentation/power/pci.rst
> index a90e82c70a3b..ff7029b94068 100644
> --- a/Documentation/power/pci.rst
> +++ b/Documentation/power/pci.rst
> @@ -692,7 +692,7 @@ controlling the runtime power management of their devices.
>  At the time of this writing there are two ways to define power management
>  callbacks for a PCI device driver, the recommended one, based on using a
>  dev_pm_ops structure described in Documentation/driver-api/pm/devices.rst, and
> -the "legacy" one, in which the .suspend(), .suspend_late(), .resume_early(), and
> +the "legacy" one, in which the .suspend(), .suspend_late(), and
>  .resume() callbacks from struct pci_driver are used.  The legacy approach,
>  however, doesn't allow one to define runtime power management callbacks and is
>  not really suitable for any new drivers.  Therefore it is not covered by this
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 5337cbbd69de..fc372c2d529a 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -630,15 +630,6 @@ static int pci_legacy_suspend_late(struct device *dev, pm_message_t state)
>         return 0;
>  }
>
> -static int pci_legacy_resume_early(struct device *dev)
> -{
> -       struct pci_dev *pci_dev = to_pci_dev(dev);
> -       struct pci_driver *drv = pci_dev->driver;
> -
> -       return drv && drv->resume_early ?
> -                       drv->resume_early(pci_dev) : 0;
> -}
> -
>  static int pci_legacy_resume(struct device *dev)
>  {
>         struct pci_dev *pci_dev = to_pci_dev(dev);
> @@ -662,8 +653,7 @@ static void pci_pm_default_suspend(struct pci_dev *pci_dev)
>  static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev)
>  {
>         struct pci_driver *drv = pci_dev->driver;
> -       bool ret = drv && (drv->suspend || drv->suspend_late || drv->resume
> -               || drv->resume_early);
> +       bool ret = drv && (drv->suspend || drv->suspend_late || drv->resume);
>
>         /*
>          * Legacy PM support is used by default, so warn if the new framework is
> @@ -944,7 +934,7 @@ static int pci_pm_resume_noirq(struct device *dev)
>         pcie_pme_root_status_cleanup(pci_dev);
>
>         if (pci_has_legacy_pm_support(pci_dev))
> -               return pci_legacy_resume_early(dev);
> +               return 0;
>
>         if (pm && pm->resume_noirq)
>                 return pm->resume_noirq(dev);
> @@ -1074,9 +1064,8 @@ static int pci_pm_thaw_noirq(struct device *dev)
>         }
>
>         /*
> -        * Both the legacy ->resume_early() and the new pm->thaw_noirq()
> -        * callbacks assume the device has been returned to D0 and its
> -        * config state has been restored.
> +        * The pm->thaw_noirq() callback assumes the device has been
> +        * returned to D0 and its config state has been restored.
>          *
>          * In addition, pci_restore_state() restores MSI-X state in MMIO
>          * space, which requires the device to be in D0, so return it to D0
> @@ -1087,7 +1076,7 @@ static int pci_pm_thaw_noirq(struct device *dev)
>         pci_restore_state(pci_dev);
>
>         if (pci_has_legacy_pm_support(pci_dev))
> -               return pci_legacy_resume_early(dev);
> +               return 0;
>
>         if (pm && pm->thaw_noirq)
>                 return pm->thaw_noirq(dev);
> @@ -1219,7 +1208,7 @@ static int pci_pm_restore_noirq(struct device *dev)
>         pci_fixup_device(pci_fixup_resume_early, pci_dev);
>
>         if (pci_has_legacy_pm_support(pci_dev))
> -               return pci_legacy_resume_early(dev);
> +               return 0;
>
>         if (pm && pm->restore_noirq)
>                 return pm->restore_noirq(dev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 4846306d521c..dd4596fc1208 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -806,7 +806,6 @@ struct module;
>   *             context, so it can sleep.
>   * @suspend:   Put device into low power state.
>   * @suspend_late: Put device into low power state.
> - * @resume_early: Wake device from low power state.
>   * @resume:    Wake device from low power state.
>   *             (Please see Documentation/power/pci.rst for descriptions
>   *             of PCI Power Management and the related functions.)
> @@ -830,7 +829,6 @@ struct pci_driver {
>         void (*remove)(struct pci_dev *dev);    /* Device removed (NULL if not a hot-plug capable driver) */
>         int  (*suspend)(struct pci_dev *dev, pm_message_t state);       /* Device suspended */
>         int  (*suspend_late)(struct pci_dev *dev, pm_message_t state);
> -       int  (*resume_early)(struct pci_dev *dev);
>         int  (*resume)(struct pci_dev *dev);    /* Device woken up */
>         void (*shutdown)(struct pci_dev *dev);
>         int  (*sriov_configure)(struct pci_dev *dev, int num_vfs); /* On PF */
> --
> 2.24.0.rc1.363.gb1bccd3e3d-goog
>

Re: [PATCH 6/6] PCI/PM: Remove unused pci_driver.suspend_late() hook

[Rafael J. Wysocki]

On Fri, Nov 1, 2019 at 9:47 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> The struct pci_driver.suspend_late() hook is one of the legacy PCI power
> management callbacks, and there are no remaining users of it.  Remove it.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  Documentation/power/pci.rst | 10 +++++-----
>  drivers/pci/pci-driver.c    | 22 +---------------------
>  include/linux/pci.h         |  2 --
>  3 files changed, 6 insertions(+), 28 deletions(-)
>
> diff --git a/Documentation/power/pci.rst b/Documentation/power/pci.rst
> index ff7029b94068..0924d29636ad 100644
> --- a/Documentation/power/pci.rst
> +++ b/Documentation/power/pci.rst
> @@ -692,11 +692,11 @@ controlling the runtime power management of their devices.
>  At the time of this writing there are two ways to define power management
>  callbacks for a PCI device driver, the recommended one, based on using a
>  dev_pm_ops structure described in Documentation/driver-api/pm/devices.rst, and
> -the "legacy" one, in which the .suspend(), .suspend_late(), and
> -.resume() callbacks from struct pci_driver are used.  The legacy approach,
> -however, doesn't allow one to define runtime power management callbacks and is
> -not really suitable for any new drivers.  Therefore it is not covered by this
> -document (refer to the source code to learn more about it).
> +the "legacy" one, in which the .suspend() and .resume() callbacks from struct
> +pci_driver are used.  The legacy approach, however, doesn't allow one to define
> +runtime power management callbacks and is not really suitable for any new
> +drivers.  Therefore it is not covered by this document (refer to the source code
> +to learn more about it).
>
>  It is recommended that all PCI device drivers define a struct dev_pm_ops object
>  containing pointers to power management (PM) callbacks that will be executed by
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index fc372c2d529a..e89fd90eaa93 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -599,32 +599,12 @@ static int pci_legacy_suspend(struct device *dev, pm_message_t state)
>  static int pci_legacy_suspend_late(struct device *dev, pm_message_t state)
>  {
>         struct pci_dev *pci_dev = to_pci_dev(dev);
> -       struct pci_driver *drv = pci_dev->driver;
> -
> -       if (drv && drv->suspend_late) {
> -               pci_power_t prev = pci_dev->current_state;
> -               int error;
> -
> -               error = drv->suspend_late(pci_dev, state);
> -               suspend_report_result(drv->suspend_late, error);
> -               if (error)
> -                       return error;
> -
> -               if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0
> -                   && pci_dev->current_state != PCI_UNKNOWN) {
> -                       pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
> -                                     "PCI PM: Device state not saved by %pS\n",
> -                                     drv->suspend_late);
> -                       goto Fixup;
> -               }
> -       }
>
>         if (!pci_dev->state_saved)
>                 pci_save_state(pci_dev);
>
>         pci_pm_set_unknown_state(pci_dev);
>
> -Fixup:
>         pci_fixup_device(pci_fixup_suspend_late, pci_dev);
>
>         return 0;
> @@ -653,7 +633,7 @@ static void pci_pm_default_suspend(struct pci_dev *pci_dev)
>  static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev)
>  {
>         struct pci_driver *drv = pci_dev->driver;
> -       bool ret = drv && (drv->suspend || drv->suspend_late || drv->resume);
> +       bool ret = drv && (drv->suspend || drv->resume);
>
>         /*
>          * Legacy PM support is used by default, so warn if the new framework is
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index dd4596fc1208..9b0e35e09874 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -805,7 +805,6 @@ struct module;
>   *             The remove function always gets called from process
>   *             context, so it can sleep.
>   * @suspend:   Put device into low power state.
> - * @suspend_late: Put device into low power state.
>   * @resume:    Wake device from low power state.
>   *             (Please see Documentation/power/pci.rst for descriptions
>   *             of PCI Power Management and the related functions.)
> @@ -828,7 +827,6 @@ struct pci_driver {
>         int  (*probe)(struct pci_dev *dev, const struct pci_device_id *id);     /* New device inserted */
>         void (*remove)(struct pci_dev *dev);    /* Device removed (NULL if not a hot-plug capable driver) */
>         int  (*suspend)(struct pci_dev *dev, pm_message_t state);       /* Device suspended */
> -       int  (*suspend_late)(struct pci_dev *dev, pm_message_t state);
>         int  (*resume)(struct pci_dev *dev);    /* Device woken up */
>         void (*shutdown)(struct pci_dev *dev);
>         int  (*sriov_configure)(struct pci_dev *dev, int num_vfs); /* On PF */
> --
> 2.24.0.rc1.363.gb1bccd3e3d-goog
>

Re: [PATCH 4/6] xen-platform: Convert to generic power management

[Bjorn Helgaas]

On Fri, Nov 01, 2019 at 03:45:56PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Convert xen-platform from the legacy PCI power management callbacks to the
> generic operations.  This is one step towards removing support for the
> legacy PCI callbacks.
> 
> The generic .resume_noirq() operation is called by pci_pm_resume_noirq() at
> the same point the legacy PCI .resume_early() callback was, so this patch
> should not change the xen-platform behavior.
> 
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

I made the tweak below to fix the compile error.  I could swear I
built this, but I must have been mistaken.

> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: KarimAllah Ahmed <karahmed@amazon.de>
> ---
>  drivers/xen/platform-pci.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/xen/platform-pci.c b/drivers/xen/platform-pci.c
> index 5e30602fdbad..e06e8769eb84 100644
> --- a/drivers/xen/platform-pci.c
> +++ b/drivers/xen/platform-pci.c
> @@ -168,13 +168,17 @@ static const struct pci_device_id platform_pci_tbl[] = {
>  	{0,}
>  };
>  
> +static struct dev_pm_ops platform_pm_ops = {
> +	.resume_noirq =   platform_pci_resume,
> +};
> +
>  static struct pci_driver platform_driver = {
>  	.name =           DRV_NAME,
>  	.probe =          platform_pci_probe,
>  	.id_table =       platform_pci_tbl,
> -#ifdef CONFIG_PM
> -	.resume_early =   platform_pci_resume,
> -#endif
> +	.driver = {
> +		.pm =     &platform_pm_ops,
> +	},
>  };
>  
>  builtin_pci_driver(platform_driver);
> -- 
> 2.24.0.rc1.363.gb1bccd3e3d-goog

diff --git a/drivers/xen/platform-pci.c b/drivers/xen/platform-pci.c
index e06e8769eb84..59e85e408c23 100644
--- a/drivers/xen/platform-pci.c
+++ b/drivers/xen/platform-pci.c
@@ -74,7 +74,7 @@ static int xen_allocate_irq(struct pci_dev *pdev)
 			"xen-platform-pci", pdev);
 }
 
-static int platform_pci_resume(struct pci_dev *pdev)
+static int platform_pci_resume(struct device *dev)
 {
 	int err;
 
@@ -83,7 +83,7 @@ static int platform_pci_resume(struct pci_dev *pdev)
 
 	err = xen_set_callback_via(callback_via);
 	if (err) {
-		dev_err(&pdev->dev, "platform_pci_resume failure!\n");
+		dev_err(dev, "platform_pci_resume failure!\n");
 		return err;
 	}
 	return 0;

Re: [PATCH 4/6] xen-platform: Convert to generic power management

[Bjorn Helgaas]

On Mon, Nov 04, 2019 at 09:23:30AM -0600, Bjorn Helgaas wrote:
> On Fri, Nov 01, 2019 at 03:45:56PM -0500, Bjorn Helgaas wrote:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Convert xen-platform from the legacy PCI power management callbacks to the
> > generic operations.  This is one step towards removing support for the
> > legacy PCI callbacks.
> > 
> > The generic .resume_noirq() operation is called by pci_pm_resume_noirq() at
> > the same point the legacy PCI .resume_early() callback was, so this patch
> > should not change the xen-platform behavior.
> > 
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> I made the tweak below to fix the compile error.  I could swear I
> built this, but I must have been mistaken.
> 
> > Cc: Stefano Stabellini <sstabellini@kernel.org>
> > Cc: KarimAllah Ahmed <karahmed@amazon.de>

I applied this to pci/pm for v5.5 as part of the larger series:
https://lore.kernel.org/r/20191101204558.210235-1-helgaas@kernel.org

> > ---
> >  drivers/xen/platform-pci.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/xen/platform-pci.c b/drivers/xen/platform-pci.c
> > index 5e30602fdbad..e06e8769eb84 100644
> > --- a/drivers/xen/platform-pci.c
> > +++ b/drivers/xen/platform-pci.c
> > @@ -168,13 +168,17 @@ static const struct pci_device_id platform_pci_tbl[] = {
> >  	{0,}
> >  };
> >  
> > +static struct dev_pm_ops platform_pm_ops = {
> > +	.resume_noirq =   platform_pci_resume,
> > +};
> > +
> >  static struct pci_driver platform_driver = {
> >  	.name =           DRV_NAME,
> >  	.probe =          platform_pci_probe,
> >  	.id_table =       platform_pci_tbl,
> > -#ifdef CONFIG_PM
> > -	.resume_early =   platform_pci_resume,
> > -#endif
> > +	.driver = {
> > +		.pm =     &platform_pm_ops,
> > +	},
> >  };
> >  
> >  builtin_pci_driver(platform_driver);
> > -- 
> > 2.24.0.rc1.363.gb1bccd3e3d-goog
> 
> diff --git a/drivers/xen/platform-pci.c b/drivers/xen/platform-pci.c
> index e06e8769eb84..59e85e408c23 100644
> --- a/drivers/xen/platform-pci.c
> +++ b/drivers/xen/platform-pci.c
> @@ -74,7 +74,7 @@ static int xen_allocate_irq(struct pci_dev *pdev)
>  			"xen-platform-pci", pdev);
>  }
>  
> -static int platform_pci_resume(struct pci_dev *pdev)
> +static int platform_pci_resume(struct device *dev)
>  {
>  	int err;
>  
> @@ -83,7 +83,7 @@ static int platform_pci_resume(struct pci_dev *pdev)
>  
>  	err = xen_set_callback_via(callback_via);
>  	if (err) {
> -		dev_err(&pdev->dev, "platform_pci_resume failure!\n");
> +		dev_err(dev, "platform_pci_resume failure!\n");
>  		return err;
>  	}
>  	return 0;

Re: [PATCH 0/6] PCI/PM: Minor fix and cleanups

[Bjorn Helgaas]

On Fri, Nov 01, 2019 at 03:45:52PM -0500, Bjorn Helgaas wrote:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> First, fix our D2 delay (which I think was just a bug -- we delayed in
> microseconds instead of milliseconds, but I'm not sure we really even use
> D2).
> 
> The rest are just cleanups that should not change any behavior.  These are
> based on my current pci/pm branch (0d1685046e61) and I pushed them to
> pci/pm-2 for now.

I applied these with Rafael's reviewed-by (thanks!) to my pci/pm
branch for v5.5.

> Bjorn Helgaas (6):
>   PCI/PM: Apply D2 delay as milliseconds, not microseconds
>   PCI/PM: Expand PM reset messages to mention D3hot (not just D3)
>   PCI/PM: Simplify pci_set_power_state()
>   xen-platform: Convert to generic power management
>   PCI/PM: Remove unused pci_driver.resume_early() hook
>   PCI/PM: Remove unused pci_driver.suspend_late() hook
> 
>  Documentation/power/pci.rst | 10 ++++-----
>  drivers/pci/pci-driver.c    | 43 ++++++-------------------------------
>  drivers/pci/pci.c           |  8 +++----
>  drivers/xen/platform-pci.c  | 10 ++++++---
>  include/linux/pci.h         |  4 ----
>  5 files changed, 22 insertions(+), 53 deletions(-)
> 
> -- 
> 2.24.0.rc1.363.gb1bccd3e3d-goog
>