All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/8] Add helper for accessing Power Management callbacs
@ 2020-05-25 18:26 Krzysztof Wilczyński
  2020-05-25 18:26 ` [PATCH 1/8] driver core: " Krzysztof Wilczyński
                   ` (7 more replies)
  0 siblings, 8 replies; 30+ messages in thread
From: Krzysztof Wilczyński @ 2020-05-25 18:26 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Rafael J. Wysocki, Len Brown, Kevin Hilman, Ulf Hansson,
	Pavel Machek, Greg Kroah-Hartman, Johan Hovold, Alex Elder,
	Bjorn Helgaas, James E.J. Bottomley, Martin K. Petersen,
	Felipe Balbi, Julian Wiedmann, Karsten Graul, Ursula Braun,
	Jakub Kicinski, Bjorn Andersson, John Stultz, David S. Miller,
	greybus-dev, netdev, linux-acpi, linux-pci, linux-pm, linux-s390,
	linux-scsi, linux-usb

This series aims to add a new driver_to_pm() helper allowing for
accessing the Power Management callbacs for a particular device.

Access to the callbacs (struct dev_pm_ops) is normally done through
using the pm pointer that is embedded within the device_driver struct.

This new helper allows for the code required to reference the pm pointer
and access Power Management callbas to be simplified.  Changing the
following:

  struct device_driver *drv = dev->driver;
  if (dev->driver && dev->driver->pm && dev->driver->pm->prepare) {
      int ret = dev->driver->pm->prepare(dev);

To:

  const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
  if (pm && pm->prepare) {
      int ret = pm->prepare(dev);

Or, changing the following:

     const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;

To:
     const struct dev_pm_ops *pm = driver_to_pm(dev->driver);

This series builds on top of the previous commit 6da2f2ccfd2d ("PCI/PM:
Make power management op coding style consistent") that had an aim to
make accessing the Power Managemnet callbacs more consistent.

No functional change intended.

Links:
  https://lore.kernel.org/driverdev-devel/20191014230016.240912-6-helgaas@kernel.org/
  https://lore.kernel.org/driverdev-devel/8592302.r4xC6RIy69@kreacher/
  https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/

Krzysztof Wilczyński (8):
  driver core: Add helper for accessing Power Management callbacs
  ACPI: PM: Use the new device_to_pm() helper to access struct
    dev_pm_ops
  greybus: Use the new device_to_pm() helper to access struct dev_pm_ops
  scsi: pm: Use the new device_to_pm() helper to access struct
    dev_pm_ops
  usb: phy: fsl: Use the new device_to_pm() helper to access struct
    dev_pm_ops
  PCI/PM: Use the new device_to_pm() helper to access struct dev_pm_ops
  PM: Use the new device_to_pm() helper to access struct dev_pm_ops
  net/iucv: Use the new device_to_pm() helper to access struct
    dev_pm_ops

 drivers/acpi/device_pm.c         |  5 ++-
 drivers/base/power/domain.c      | 12 ++++--
 drivers/base/power/generic_ops.c | 65 ++++++++++++++------------------
 drivers/base/power/main.c        | 48 +++++++++++++++--------
 drivers/base/power/runtime.c     |  7 ++--
 drivers/greybus/bundle.c         |  4 +-
 drivers/pci/pci-driver.c         | 32 ++++++++--------
 drivers/scsi/scsi_pm.c           |  8 ++--
 drivers/usb/phy/phy-fsl-usb.c    | 11 ++++--
 include/linux/device/driver.h    | 15 ++++++++
 net/iucv/iucv.c                  | 30 +++++++++------
 11 files changed, 138 insertions(+), 99 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 1/8] driver core: Add helper for accessing Power Management callbacs
  2020-05-25 18:26 [PATCH 0/8] Add helper for accessing Power Management callbacs Krzysztof Wilczyński
@ 2020-05-25 18:26 ` Krzysztof Wilczyński
  2020-05-26  6:33   ` Greg Kroah-Hartman
  2020-05-25 18:26 ` [PATCH 2/8] ACPI: PM: Use the new device_to_pm() helper to access struct dev_pm_ops Krzysztof Wilczyński
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 30+ messages in thread
From: Krzysztof Wilczyński @ 2020-05-25 18:26 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Rafael J. Wysocki, Len Brown, Kevin Hilman, Ulf Hansson,
	Pavel Machek, Greg Kroah-Hartman, Johan Hovold, Alex Elder,
	Bjorn Helgaas, James E.J. Bottomley, Martin K. Petersen,
	Felipe Balbi, Julian Wiedmann, Karsten Graul, Ursula Braun,
	Jakub Kicinski, Bjorn Andersson, John Stultz, David S. Miller,
	greybus-dev, netdev, linux-acpi, linux-pci, linux-pm, linux-s390,
	linux-scsi, linux-usb

Add driver_to_pm() helper allowing for accessing the Power Management
callbacs for a particular device.  Access to the callbacs (struct
dev_pm_ops) is normally done through using the pm pointer that is
embedded within the device_driver struct.

Helper allows for the code required to reference the pm pointer and
access Power Management callbas to be simplified.  Changing the
following:

  struct device_driver *drv = dev->driver;
  if (dev->driver && dev->driver->pm && dev->driver->pm->prepare) {
      int ret = dev->driver->pm->prepare(dev);

To:

  const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
  if (pm && pm->prepare) {
      int ret = pm->prepare(dev);

Or, changing the following:

     const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;

To:
     const struct dev_pm_ops *pm = driver_to_pm(dev->driver);

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 include/linux/device/driver.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
index ee7ba5b5417e..ccd0b315fd93 100644
--- a/include/linux/device/driver.h
+++ b/include/linux/device/driver.h
@@ -236,6 +236,21 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev)
 }
 #endif
 
+/**
+ * driver_to_pm - Return Power Management callbacs (struct dev_pm_ops) for
+ *                a particular device.
+ * @drv: Pointer to a device (struct device_driver) for which you want to access
+ *       the Power Management callbacks.
+ *
+ * Returns a pointer to the struct dev_pm_ops embedded within the device (struct
+ * device_driver), or returns NULL if Power Management is not present and the
+ * pointer is not valid.
+ */
+static inline const struct dev_pm_ops *driver_to_pm(struct device_driver *drv)
+{
+	return drv && drv->pm ? drv->pm : NULL;
+}
+
 extern int driver_deferred_probe_timeout;
 void driver_deferred_probe_add(struct device *dev);
 int driver_deferred_probe_check_state(struct device *dev);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 2/8] ACPI: PM: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-25 18:26 [PATCH 0/8] Add helper for accessing Power Management callbacs Krzysztof Wilczyński
  2020-05-25 18:26 ` [PATCH 1/8] driver core: " Krzysztof Wilczyński
@ 2020-05-25 18:26 ` Krzysztof Wilczyński
  2020-05-26  8:37     ` Rafael J. Wysocki
  2020-05-25 18:26 ` [PATCH 3/8] greybus: " Krzysztof Wilczyński
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 30+ messages in thread
From: Krzysztof Wilczyński @ 2020-05-25 18:26 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Rafael J. Wysocki, Len Brown, Kevin Hilman, Ulf Hansson,
	Pavel Machek, Greg Kroah-Hartman, Johan Hovold, Alex Elder,
	Bjorn Helgaas, James E.J. Bottomley, Martin K. Petersen,
	Felipe Balbi, Julian Wiedmann, Karsten Graul, Ursula Braun,
	Jakub Kicinski, Bjorn Andersson, John Stultz, David S. Miller,
	greybus-dev, netdev, linux-acpi, linux-pci, linux-pm, linux-s390,
	linux-scsi, linux-usb

Use the new device_to_pm() helper to access Power Management callbacs
(struct dev_pm_ops) for a particular device (struct device_driver).

No functional change intended.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 drivers/acpi/device_pm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 5832bc10aca8..b98a32c48fbe 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1022,9 +1022,10 @@ static bool acpi_dev_needs_resume(struct device *dev, struct acpi_device *adev)
 int acpi_subsys_prepare(struct device *dev)
 {
 	struct acpi_device *adev = ACPI_COMPANION(dev);
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
-	if (dev->driver && dev->driver->pm && dev->driver->pm->prepare) {
-		int ret = dev->driver->pm->prepare(dev);
+	if (pm && pm->prepare) {
+		int ret = pm->prepare(dev);
 
 		if (ret < 0)
 			return ret;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 3/8] greybus: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-25 18:26 [PATCH 0/8] Add helper for accessing Power Management callbacs Krzysztof Wilczyński
  2020-05-25 18:26 ` [PATCH 1/8] driver core: " Krzysztof Wilczyński
  2020-05-25 18:26 ` [PATCH 2/8] ACPI: PM: Use the new device_to_pm() helper to access struct dev_pm_ops Krzysztof Wilczyński
@ 2020-05-25 18:26 ` Krzysztof Wilczyński
  2020-05-26 11:53   ` [greybus-dev] " Alex Elder
  2020-05-25 18:26 ` [PATCH 4/8] scsi: pm: " Krzysztof Wilczyński
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 30+ messages in thread
From: Krzysztof Wilczyński @ 2020-05-25 18:26 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Rafael J. Wysocki, Len Brown, Kevin Hilman, Ulf Hansson,
	Pavel Machek, Greg Kroah-Hartman, Johan Hovold, Alex Elder,
	Bjorn Helgaas, James E.J. Bottomley, Martin K. Petersen,
	Felipe Balbi, Julian Wiedmann, Karsten Graul, Ursula Braun,
	Jakub Kicinski, Bjorn Andersson, John Stultz, David S. Miller,
	greybus-dev, netdev, linux-acpi, linux-pci, linux-pm, linux-s390,
	linux-scsi, linux-usb

Use the new device_to_pm() helper to access Power Management callbacs
(struct dev_pm_ops) for a particular device (struct device_driver).

No functional change intended.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 drivers/greybus/bundle.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/greybus/bundle.c b/drivers/greybus/bundle.c
index 84660729538b..d38d3a630812 100644
--- a/drivers/greybus/bundle.c
+++ b/drivers/greybus/bundle.c
@@ -108,7 +108,7 @@ static void gb_bundle_enable_all_connections(struct gb_bundle *bundle)
 static int gb_bundle_suspend(struct device *dev)
 {
 	struct gb_bundle *bundle = to_gb_bundle(dev);
-	const struct dev_pm_ops *pm = dev->driver->pm;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 	int ret;
 
 	if (pm && pm->runtime_suspend) {
@@ -135,7 +135,7 @@ static int gb_bundle_suspend(struct device *dev)
 static int gb_bundle_resume(struct device *dev)
 {
 	struct gb_bundle *bundle = to_gb_bundle(dev);
-	const struct dev_pm_ops *pm = dev->driver->pm;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 	int ret;
 
 	ret = gb_control_bundle_resume(bundle->intf->control, bundle->id);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 4/8] scsi: pm: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-25 18:26 [PATCH 0/8] Add helper for accessing Power Management callbacs Krzysztof Wilczyński
                   ` (2 preceding siblings ...)
  2020-05-25 18:26 ` [PATCH 3/8] greybus: " Krzysztof Wilczyński
@ 2020-05-25 18:26 ` Krzysztof Wilczyński
  2020-05-25 18:26 ` [PATCH 5/8] usb: phy: fsl: " Krzysztof Wilczyński
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 30+ messages in thread
From: Krzysztof Wilczyński @ 2020-05-25 18:26 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Rafael J. Wysocki, Len Brown, Kevin Hilman, Ulf Hansson,
	Pavel Machek, Greg Kroah-Hartman, Johan Hovold, Alex Elder,
	Bjorn Helgaas, James E.J. Bottomley, Martin K. Petersen,
	Felipe Balbi, Julian Wiedmann, Karsten Graul, Ursula Braun,
	Jakub Kicinski, Bjorn Andersson, John Stultz, David S. Miller,
	greybus-dev, netdev, linux-acpi, linux-pci, linux-pm, linux-s390,
	linux-scsi, linux-usb

Use the new device_to_pm() helper to access Power Management callbacs
(struct dev_pm_ops) for a particular device (struct device_driver).

No functional change intended.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 drivers/scsi/scsi_pm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 5f0ad8b32e3a..8f40b60d3383 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -53,7 +53,7 @@ static int do_scsi_restore(struct device *dev, const struct dev_pm_ops *pm)
 static int scsi_dev_type_suspend(struct device *dev,
 		int (*cb)(struct device *, const struct dev_pm_ops *))
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 	int err;
 
 	/* flush pending in-flight resume operations, suspend is synchronous */
@@ -72,7 +72,7 @@ static int scsi_dev_type_suspend(struct device *dev,
 static int scsi_dev_type_resume(struct device *dev,
 		int (*cb)(struct device *, const struct dev_pm_ops *))
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 	int err = 0;
 
 	err = cb(dev, pm);
@@ -232,7 +232,7 @@ static int scsi_bus_restore(struct device *dev)
 
 static int sdev_runtime_suspend(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 	struct scsi_device *sdev = to_scsi_device(dev);
 	int err = 0;
 
@@ -262,7 +262,7 @@ static int scsi_runtime_suspend(struct device *dev)
 static int sdev_runtime_resume(struct device *dev)
 {
 	struct scsi_device *sdev = to_scsi_device(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 	int err = 0;
 
 	blk_pre_runtime_resume(sdev->request_queue);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 5/8] usb: phy: fsl: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-25 18:26 [PATCH 0/8] Add helper for accessing Power Management callbacs Krzysztof Wilczyński
                   ` (3 preceding siblings ...)
  2020-05-25 18:26 ` [PATCH 4/8] scsi: pm: " Krzysztof Wilczyński
@ 2020-05-25 18:26 ` Krzysztof Wilczyński
  2020-05-26  8:38     ` Rafael J. Wysocki
  2020-05-25 18:26 ` [PATCH 6/8] PCI/PM: " Krzysztof Wilczyński
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 30+ messages in thread
From: Krzysztof Wilczyński @ 2020-05-25 18:26 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Rafael J. Wysocki, Len Brown, Kevin Hilman, Ulf Hansson,
	Pavel Machek, Greg Kroah-Hartman, Johan Hovold, Alex Elder,
	Bjorn Helgaas, James E.J. Bottomley, Martin K. Petersen,
	Felipe Balbi, Julian Wiedmann, Karsten Graul, Ursula Braun,
	Jakub Kicinski, Bjorn Andersson, John Stultz, David S. Miller,
	greybus-dev, netdev, linux-acpi, linux-pci, linux-pm, linux-s390,
	linux-scsi, linux-usb

Use the new device_to_pm() helper to access Power Management callbacs
(struct dev_pm_ops) for a particular device (struct device_driver).

No functional change intended.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 drivers/usb/phy/phy-fsl-usb.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
index b451f4695f3f..3b9ad5db8380 100644
--- a/drivers/usb/phy/phy-fsl-usb.c
+++ b/drivers/usb/phy/phy-fsl-usb.c
@@ -460,6 +460,7 @@ int fsl_otg_start_host(struct otg_fsm *fsm, int on)
 	struct device *dev;
 	struct fsl_otg *otg_dev =
 		container_of(otg->usb_phy, struct fsl_otg, phy);
+	const struct dev_pm_ops *pm;
 	u32 retval = 0;
 
 	if (!otg->host)
@@ -479,8 +480,9 @@ int fsl_otg_start_host(struct otg_fsm *fsm, int on)
 		else {
 			otg_reset_controller();
 			VDBG("host on......\n");
-			if (dev->driver->pm && dev->driver->pm->resume) {
-				retval = dev->driver->pm->resume(dev);
+			pm = driver_to_pm(dev->driver);
+			if (pm && pm->resume) {
+				retval = pm->resume(dev);
 				if (fsm->id) {
 					/* default-b */
 					fsl_otg_drv_vbus(fsm, 1);
@@ -504,8 +506,9 @@ int fsl_otg_start_host(struct otg_fsm *fsm, int on)
 		else {
 			VDBG("host off......\n");
 			if (dev && dev->driver) {
-				if (dev->driver->pm && dev->driver->pm->suspend)
-					retval = dev->driver->pm->suspend(dev);
+				pm = driver_to_pm(dev->driver);
+				if (pm && pm->suspend)
+					retval = pm->suspend(dev);
 				if (fsm->id)
 					/* default-b */
 					fsl_otg_drv_vbus(fsm, 0);
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 6/8] PCI/PM: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-25 18:26 [PATCH 0/8] Add helper for accessing Power Management callbacs Krzysztof Wilczyński
                   ` (4 preceding siblings ...)
  2020-05-25 18:26 ` [PATCH 5/8] usb: phy: fsl: " Krzysztof Wilczyński
@ 2020-05-25 18:26 ` Krzysztof Wilczyński
  2020-05-25 18:26 ` [PATCH 7/8] PM: " Krzysztof Wilczyński
  2020-05-25 18:26 ` [PATCH 8/8] net/iucv: " Krzysztof Wilczyński
  7 siblings, 0 replies; 30+ messages in thread
From: Krzysztof Wilczyński @ 2020-05-25 18:26 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Rafael J. Wysocki, Len Brown, Kevin Hilman, Ulf Hansson,
	Pavel Machek, Greg Kroah-Hartman, Johan Hovold, Alex Elder,
	Bjorn Helgaas, James E.J. Bottomley, Martin K. Petersen,
	Felipe Balbi, Julian Wiedmann, Karsten Graul, Ursula Braun,
	Jakub Kicinski, Bjorn Andersson, John Stultz, David S. Miller,
	greybus-dev, netdev, linux-acpi, linux-pci, linux-pm, linux-s390,
	linux-scsi, linux-usb

Use the new device_to_pm() helper to access Power Management callbacs
(struct dev_pm_ops) for a particular device (struct device_driver).

No functional change intended.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 drivers/pci/pci-driver.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 0454ca0e4e3f..bb52bb6642a0 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -652,7 +652,7 @@ static bool pci_has_legacy_pm_support(struct pci_dev *pci_dev)
 static int pci_pm_prepare(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	if (pm && pm->prepare) {
 		int error = pm->prepare(dev);
@@ -721,7 +721,7 @@ static void pcie_pme_root_status_cleanup(struct pci_dev *pci_dev)
 static int pci_pm_suspend(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	pci_dev->skip_bus_pm = false;
 
@@ -787,7 +787,7 @@ static int pci_pm_suspend_late(struct device *dev)
 static int pci_pm_suspend_noirq(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	if (dev_pm_smart_suspend_and_suspended(dev)) {
 		dev->power.may_skip_resume = true;
@@ -889,7 +889,7 @@ static int pci_pm_suspend_noirq(struct device *dev)
 static int pci_pm_resume_noirq(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 	pci_power_t prev_state = pci_dev->current_state;
 	bool skip_bus_pm = pci_dev->skip_bus_pm;
 
@@ -931,7 +931,7 @@ static int pci_pm_resume_noirq(struct device *dev)
 static int pci_pm_resume(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	/*
 	 * This is necessary for the suspend error path in which resume is
@@ -976,7 +976,7 @@ struct dev_pm_ops __weak pcibios_pm_ops;
 static int pci_pm_freeze(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_FREEZE);
@@ -1012,7 +1012,7 @@ static int pci_pm_freeze(struct device *dev)
 static int pci_pm_freeze_noirq(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend_late(dev, PMSG_FREEZE);
@@ -1040,7 +1040,7 @@ static int pci_pm_freeze_noirq(struct device *dev)
 static int pci_pm_thaw_noirq(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 	int error;
 
 	if (pcibios_pm_ops.thaw_noirq) {
@@ -1073,7 +1073,7 @@ static int pci_pm_thaw_noirq(struct device *dev)
 static int pci_pm_thaw(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 	int error = 0;
 
 	if (pci_has_legacy_pm_support(pci_dev))
@@ -1094,7 +1094,7 @@ static int pci_pm_thaw(struct device *dev)
 static int pci_pm_poweroff(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	if (pci_has_legacy_pm_support(pci_dev))
 		return pci_legacy_suspend(dev, PMSG_HIBERNATE);
@@ -1138,7 +1138,7 @@ static int pci_pm_poweroff_late(struct device *dev)
 static int pci_pm_poweroff_noirq(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	if (dev_pm_smart_suspend_and_suspended(dev))
 		return 0;
@@ -1181,7 +1181,7 @@ static int pci_pm_poweroff_noirq(struct device *dev)
 static int pci_pm_restore_noirq(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 	int error;
 
 	if (pcibios_pm_ops.restore_noirq) {
@@ -1205,7 +1205,7 @@ static int pci_pm_restore_noirq(struct device *dev)
 static int pci_pm_restore(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	/*
 	 * This is necessary for the hibernation error path in which restore is
@@ -1248,7 +1248,7 @@ static int pci_pm_restore(struct device *dev)
 static int pci_pm_runtime_suspend(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 	pci_power_t prev = pci_dev->current_state;
 	int error;
 
@@ -1303,7 +1303,7 @@ static int pci_pm_runtime_suspend(struct device *dev)
 static int pci_pm_runtime_resume(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 	pci_power_t prev_state = pci_dev->current_state;
 	int error = 0;
 
@@ -1334,7 +1334,7 @@ static int pci_pm_runtime_resume(struct device *dev)
 static int pci_pm_runtime_idle(struct device *dev)
 {
 	struct pci_dev *pci_dev = to_pci_dev(dev);
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	/*
 	 * If pci_dev->driver is not set (unbound), the device should
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 7/8] PM: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-25 18:26 [PATCH 0/8] Add helper for accessing Power Management callbacs Krzysztof Wilczyński
                   ` (5 preceding siblings ...)
  2020-05-25 18:26 ` [PATCH 6/8] PCI/PM: " Krzysztof Wilczyński
@ 2020-05-25 18:26 ` Krzysztof Wilczyński
  2020-05-26  8:33     ` Rafael J. Wysocki
  2020-05-25 18:26 ` [PATCH 8/8] net/iucv: " Krzysztof Wilczyński
  7 siblings, 1 reply; 30+ messages in thread
From: Krzysztof Wilczyński @ 2020-05-25 18:26 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Rafael J. Wysocki, Len Brown, Kevin Hilman, Ulf Hansson,
	Pavel Machek, Greg Kroah-Hartman, Johan Hovold, Alex Elder,
	Bjorn Helgaas, James E.J. Bottomley, Martin K. Petersen,
	Felipe Balbi, Julian Wiedmann, Karsten Graul, Ursula Braun,
	Jakub Kicinski, Bjorn Andersson, John Stultz, David S. Miller,
	greybus-dev, netdev, linux-acpi, linux-pci, linux-pm, linux-s390,
	linux-scsi, linux-usb

Use the new device_to_pm() helper to access Power Management callbacs
(struct dev_pm_ops) for a particular device (struct device_driver).

No functional change intended.

This change builds on top of the previous commit 6da2f2ccfd2d ("PCI/PM:
Make power management op coding style consistent").

Links:
  https://lore.kernel.org/driverdev-devel/20191014230016.240912-6-helgaas@kernel.org/
  https://lore.kernel.org/driverdev-devel/8592302.r4xC6RIy69@kreacher/
  https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 drivers/base/power/domain.c      | 12 ++++--
 drivers/base/power/generic_ops.c | 65 ++++++++++++++------------------
 drivers/base/power/main.c        | 48 +++++++++++++++--------
 drivers/base/power/runtime.c     |  7 ++--
 4 files changed, 73 insertions(+), 59 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 0a01df608849..92a96fcb2717 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -703,6 +703,7 @@ static void genpd_power_off_work_fn(struct work_struct *work)
 static int __genpd_runtime_suspend(struct device *dev)
 {
 	int (*cb)(struct device *__dev);
+	const struct dev_pm_ops *pm;
 
 	if (dev->type && dev->type->pm)
 		cb = dev->type->pm->runtime_suspend;
@@ -713,8 +714,9 @@ static int __genpd_runtime_suspend(struct device *dev)
 	else
 		cb = NULL;
 
-	if (!cb && dev->driver && dev->driver->pm)
-		cb = dev->driver->pm->runtime_suspend;
+	pm = driver_to_pm(dev->driver);
+	if (!cb && pm)
+		cb = pm->runtime_suspend;
 
 	return cb ? cb(dev) : 0;
 }
@@ -726,6 +728,7 @@ static int __genpd_runtime_suspend(struct device *dev)
 static int __genpd_runtime_resume(struct device *dev)
 {
 	int (*cb)(struct device *__dev);
+	const struct dev_pm_ops *pm;
 
 	if (dev->type && dev->type->pm)
 		cb = dev->type->pm->runtime_resume;
@@ -736,8 +739,9 @@ static int __genpd_runtime_resume(struct device *dev)
 	else
 		cb = NULL;
 
-	if (!cb && dev->driver && dev->driver->pm)
-		cb = dev->driver->pm->runtime_resume;
+	pm = driver_to_pm(dev->driver);
+	if (!cb && pm)
+		cb = pm->runtime_resume;
 
 	return cb ? cb(dev) : 0;
 }
diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
index 4fa525668cb7..fbd2edef0201 100644
--- a/drivers/base/power/generic_ops.c
+++ b/drivers/base/power/generic_ops.c
@@ -19,12 +19,9 @@
  */
 int pm_generic_runtime_suspend(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	int ret;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
-	ret = pm && pm->runtime_suspend ? pm->runtime_suspend(dev) : 0;
-
-	return ret;
+	return pm && pm->runtime_suspend ? pm->runtime_suspend(dev) : 0;
 }
 EXPORT_SYMBOL_GPL(pm_generic_runtime_suspend);
 
@@ -38,12 +35,9 @@ EXPORT_SYMBOL_GPL(pm_generic_runtime_suspend);
  */
 int pm_generic_runtime_resume(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	int ret;
-
-	ret = pm && pm->runtime_resume ? pm->runtime_resume(dev) : 0;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
-	return ret;
+	return pm && pm->runtime_resume ? pm->runtime_resume(dev) : 0;
 }
 EXPORT_SYMBOL_GPL(pm_generic_runtime_resume);
 #endif /* CONFIG_PM */
@@ -57,13 +51,12 @@ EXPORT_SYMBOL_GPL(pm_generic_runtime_resume);
  */
 int pm_generic_prepare(struct device *dev)
 {
-	struct device_driver *drv = dev->driver;
-	int ret = 0;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
-	if (drv && drv->pm && drv->pm->prepare)
-		ret = drv->pm->prepare(dev);
+	if (pm && pm->prepare)
+		return pm->prepare(dev);
 
-	return ret;
+	return 0;
 }
 
 /**
@@ -72,7 +65,7 @@ int pm_generic_prepare(struct device *dev)
  */
 int pm_generic_suspend_noirq(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->suspend_noirq ? pm->suspend_noirq(dev) : 0;
 }
@@ -84,7 +77,7 @@ EXPORT_SYMBOL_GPL(pm_generic_suspend_noirq);
  */
 int pm_generic_suspend_late(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->suspend_late ? pm->suspend_late(dev) : 0;
 }
@@ -96,7 +89,7 @@ EXPORT_SYMBOL_GPL(pm_generic_suspend_late);
  */
 int pm_generic_suspend(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->suspend ? pm->suspend(dev) : 0;
 }
@@ -108,7 +101,7 @@ EXPORT_SYMBOL_GPL(pm_generic_suspend);
  */
 int pm_generic_freeze_noirq(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->freeze_noirq ? pm->freeze_noirq(dev) : 0;
 }
@@ -120,7 +113,7 @@ EXPORT_SYMBOL_GPL(pm_generic_freeze_noirq);
  */
 int pm_generic_freeze_late(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->freeze_late ? pm->freeze_late(dev) : 0;
 }
@@ -132,7 +125,7 @@ EXPORT_SYMBOL_GPL(pm_generic_freeze_late);
  */
 int pm_generic_freeze(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->freeze ? pm->freeze(dev) : 0;
 }
@@ -144,7 +137,7 @@ EXPORT_SYMBOL_GPL(pm_generic_freeze);
  */
 int pm_generic_poweroff_noirq(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->poweroff_noirq ? pm->poweroff_noirq(dev) : 0;
 }
@@ -156,7 +149,7 @@ EXPORT_SYMBOL_GPL(pm_generic_poweroff_noirq);
  */
 int pm_generic_poweroff_late(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->poweroff_late ? pm->poweroff_late(dev) : 0;
 }
@@ -168,7 +161,7 @@ EXPORT_SYMBOL_GPL(pm_generic_poweroff_late);
  */
 int pm_generic_poweroff(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->poweroff ? pm->poweroff(dev) : 0;
 }
@@ -180,7 +173,7 @@ EXPORT_SYMBOL_GPL(pm_generic_poweroff);
  */
 int pm_generic_thaw_noirq(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->thaw_noirq ? pm->thaw_noirq(dev) : 0;
 }
@@ -192,7 +185,7 @@ EXPORT_SYMBOL_GPL(pm_generic_thaw_noirq);
  */
 int pm_generic_thaw_early(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->thaw_early ? pm->thaw_early(dev) : 0;
 }
@@ -204,7 +197,7 @@ EXPORT_SYMBOL_GPL(pm_generic_thaw_early);
  */
 int pm_generic_thaw(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->thaw ? pm->thaw(dev) : 0;
 }
@@ -216,7 +209,7 @@ EXPORT_SYMBOL_GPL(pm_generic_thaw);
  */
 int pm_generic_resume_noirq(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->resume_noirq ? pm->resume_noirq(dev) : 0;
 }
@@ -228,7 +221,7 @@ EXPORT_SYMBOL_GPL(pm_generic_resume_noirq);
  */
 int pm_generic_resume_early(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->resume_early ? pm->resume_early(dev) : 0;
 }
@@ -240,7 +233,7 @@ EXPORT_SYMBOL_GPL(pm_generic_resume_early);
  */
 int pm_generic_resume(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->resume ? pm->resume(dev) : 0;
 }
@@ -252,7 +245,7 @@ EXPORT_SYMBOL_GPL(pm_generic_resume);
  */
 int pm_generic_restore_noirq(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->restore_noirq ? pm->restore_noirq(dev) : 0;
 }
@@ -264,7 +257,7 @@ EXPORT_SYMBOL_GPL(pm_generic_restore_noirq);
  */
 int pm_generic_restore_early(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->restore_early ? pm->restore_early(dev) : 0;
 }
@@ -276,7 +269,7 @@ EXPORT_SYMBOL_GPL(pm_generic_restore_early);
  */
 int pm_generic_restore(struct device *dev)
 {
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 	return pm && pm->restore ? pm->restore(dev) : 0;
 }
@@ -290,9 +283,9 @@ EXPORT_SYMBOL_GPL(pm_generic_restore);
  */
 void pm_generic_complete(struct device *dev)
 {
-	struct device_driver *drv = dev->driver;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
-	if (drv && drv->pm && drv->pm->complete)
-		drv->pm->complete(dev);
+	if (pm && pm->complete)
+		pm->complete(dev);
 }
 #endif /* CONFIG_PM_SLEEP */
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 0e07e17c2def..6c41da0bebb0 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -640,6 +640,7 @@ static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
 static int device_resume_noirq(struct device *dev, pm_message_t state, bool async)
 {
 	pm_callback_t callback;
+	const struct dev_pm_ops *pm;
 	const char *info;
 	bool skip_resume;
 	int error = 0;
@@ -687,9 +688,10 @@ static int device_resume_noirq(struct device *dev, pm_message_t state, bool asyn
 		}
 	}
 
-	if (dev->driver && dev->driver->pm) {
+	pm = driver_to_pm(dev->driver);
+	if (pm) {
 		info = "noirq driver ";
-		callback = pm_noirq_op(dev->driver->pm, state);
+		callback = pm_noirq_op(pm, state);
 	}
 
 Run:
@@ -850,6 +852,7 @@ static pm_callback_t dpm_subsys_resume_early_cb(struct device *dev,
 static int device_resume_early(struct device *dev, pm_message_t state, bool async)
 {
 	pm_callback_t callback;
+	const struct dev_pm_ops *pm;
 	const char *info;
 	int error = 0;
 
@@ -867,9 +870,10 @@ static int device_resume_early(struct device *dev, pm_message_t state, bool asyn
 
 	callback = dpm_subsys_resume_early_cb(dev, state, &info);
 
-	if (!callback && dev->driver && dev->driver->pm) {
+	pm = driver_to_pm(dev->driver);
+	if (!callback && pm) {
 		info = "early driver ";
-		callback = pm_late_early_op(dev->driver->pm, state);
+		callback = pm_late_early_op(pm, state);
 	}
 
 	error = dpm_run_callback(callback, dev, state, info);
@@ -963,6 +967,7 @@ static int device_resume(struct device *dev, pm_message_t state, bool async)
 {
 	pm_callback_t callback = NULL;
 	const char *info = NULL;
+	const struct dev_pm_ops *pm = NULL;
 	int error = 0;
 	DECLARE_DPM_WATCHDOG_ON_STACK(wd);
 
@@ -1023,9 +1028,10 @@ static int device_resume(struct device *dev, pm_message_t state, bool async)
 	}
 
  Driver:
-	if (!callback && dev->driver && dev->driver->pm) {
+	pm = driver_to_pm(dev->driver);
+	if (!callback && pm) {
 		info = "driver ";
-		callback = pm_op(dev->driver->pm, state);
+		callback = pm_op(pm, state);
 	}
 
  End:
@@ -1116,6 +1122,7 @@ void dpm_resume(pm_message_t state)
 static void device_complete(struct device *dev, pm_message_t state)
 {
 	void (*callback)(struct device *) = NULL;
+	const struct dev_pm_ops *pm = NULL;
 	const char *info = NULL;
 
 	if (dev->power.syscore)
@@ -1137,9 +1144,10 @@ static void device_complete(struct device *dev, pm_message_t state)
 		callback = dev->bus->pm->complete;
 	}
 
-	if (!callback && dev->driver && dev->driver->pm) {
+	pm = driver_to_pm(dev->driver);
+	if (!callback && pm) {
 		info = "completing driver ";
-		callback = dev->driver->pm->complete;
+		callback = pm->complete;
 	}
 
 	if (callback) {
@@ -1312,6 +1320,7 @@ static bool device_must_resume(struct device *dev, pm_message_t state,
 static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool async)
 {
 	pm_callback_t callback;
+	const struct dev_pm_ops *pm;
 	const char *info;
 	bool no_subsys_cb = false;
 	int error = 0;
@@ -1336,9 +1345,10 @@ static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool a
 	if (dev_pm_smart_suspend_and_suspended(dev) && no_subsys_cb)
 		goto Skip;
 
-	if (dev->driver && dev->driver->pm) {
+	pm = driver_to_pm(dev->driver);
+	if (pm) {
 		info = "noirq driver ";
-		callback = pm_noirq_op(dev->driver->pm, state);
+		callback = pm_noirq_op(pm, state);
 	}
 
 Run:
@@ -1514,6 +1524,7 @@ static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
 static int __device_suspend_late(struct device *dev, pm_message_t state, bool async)
 {
 	pm_callback_t callback;
+	const struct dev_pm_ops *pm;
 	const char *info;
 	int error = 0;
 
@@ -1543,9 +1554,10 @@ static int __device_suspend_late(struct device *dev, pm_message_t state, bool as
 	    !dpm_subsys_suspend_noirq_cb(dev, state, NULL))
 		goto Skip;
 
-	if (dev->driver && dev->driver->pm) {
+	pm = driver_to_pm(dev->driver);
+	if (pm) {
 		info = "late driver ";
-		callback = pm_late_early_op(dev->driver->pm, state);
+		callback = pm_late_early_op(pm, state);
 	}
 
 Run:
@@ -1717,6 +1729,7 @@ static void dpm_clear_superiors_direct_complete(struct device *dev)
 static int __device_suspend(struct device *dev, pm_message_t state, bool async)
 {
 	pm_callback_t callback = NULL;
+	const struct dev_pm_ops *pm = NULL;
 	const char *info = NULL;
 	int error = 0;
 	DECLARE_DPM_WATCHDOG_ON_STACK(wd);
@@ -1803,9 +1816,10 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
 	}
 
  Run:
-	if (!callback && dev->driver && dev->driver->pm) {
+	pm = driver_to_pm(dev->driver);
+	if (!callback && pm) {
 		info = "driver ";
-		callback = pm_op(dev->driver->pm, state);
+		callback = pm_op(pm, state);
 	}
 
 	error = dpm_run_callback(callback, dev, state, info);
@@ -1917,6 +1931,7 @@ int dpm_suspend(pm_message_t state)
 static int device_prepare(struct device *dev, pm_message_t state)
 {
 	int (*callback)(struct device *) = NULL;
+	const struct dev_pm_ops *pm = NULL;
 	int ret = 0;
 
 	if (dev->power.syscore)
@@ -1946,8 +1961,9 @@ static int device_prepare(struct device *dev, pm_message_t state)
 	else if (dev->bus && dev->bus->pm)
 		callback = dev->bus->pm->prepare;
 
-	if (!callback && dev->driver && dev->driver->pm)
-		callback = dev->driver->pm->prepare;
+	pm = driver_to_pm(dev->driver);
+	if (!callback && pm)
+		callback = pm->prepare;
 
 	if (callback)
 		ret = callback(dev);
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 99c7da112c95..c142824c7541 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -21,7 +21,7 @@ typedef int (*pm_callback_t)(struct device *);
 static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset)
 {
 	pm_callback_t cb;
-	const struct dev_pm_ops *ops;
+	const struct dev_pm_ops *ops, *pm;
 
 	if (dev->pm_domain)
 		ops = &dev->pm_domain->ops;
@@ -39,8 +39,9 @@ static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset)
 	else
 		cb = NULL;
 
-	if (!cb && dev->driver && dev->driver->pm)
-		cb = *(pm_callback_t *)((void *)dev->driver->pm + cb_offset);
+	pm = driver_to_pm(dev->driver);
+	if (!cb && pm)
+		cb = *(pm_callback_t *)((void *)pm + cb_offset);
 
 	return cb;
 }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 8/8] net/iucv: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-25 18:26 [PATCH 0/8] Add helper for accessing Power Management callbacs Krzysztof Wilczyński
                   ` (6 preceding siblings ...)
  2020-05-25 18:26 ` [PATCH 7/8] PM: " Krzysztof Wilczyński
@ 2020-05-25 18:26 ` Krzysztof Wilczyński
  2020-05-26  6:35   ` Greg Kroah-Hartman
  2020-05-26  7:07   ` Ursula Braun
  7 siblings, 2 replies; 30+ messages in thread
From: Krzysztof Wilczyński @ 2020-05-25 18:26 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Rafael J. Wysocki, Len Brown, Kevin Hilman, Ulf Hansson,
	Pavel Machek, Greg Kroah-Hartman, Johan Hovold, Alex Elder,
	Bjorn Helgaas, James E.J. Bottomley, Martin K. Petersen,
	Felipe Balbi, Julian Wiedmann, Karsten Graul, Ursula Braun,
	Jakub Kicinski, Bjorn Andersson, John Stultz, David S. Miller,
	greybus-dev, netdev, linux-acpi, linux-pci, linux-pm, linux-s390,
	linux-scsi, linux-usb

Use the new device_to_pm() helper to access Power Management callbacs
(struct dev_pm_ops) for a particular device (struct device_driver).

No functional change intended.

Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
---
 net/iucv/iucv.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
index 9a2d023842fe..1a3029ab7c1f 100644
--- a/net/iucv/iucv.c
+++ b/net/iucv/iucv.c
@@ -1836,23 +1836,23 @@ static void iucv_external_interrupt(struct ext_code ext_code,
 
 static int iucv_pm_prepare(struct device *dev)
 {
-	int rc = 0;
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
 
 #ifdef CONFIG_PM_DEBUG
 	printk(KERN_INFO "iucv_pm_prepare\n");
 #endif
-	if (dev->driver && dev->driver->pm && dev->driver->pm->prepare)
-		rc = dev->driver->pm->prepare(dev);
-	return rc;
+	return pm && pm->prepare ? pm->prepare(dev) : 0;
 }
 
 static void iucv_pm_complete(struct device *dev)
 {
+	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
+
 #ifdef CONFIG_PM_DEBUG
 	printk(KERN_INFO "iucv_pm_complete\n");
 #endif
-	if (dev->driver && dev->driver->pm && dev->driver->pm->complete)
-		dev->driver->pm->complete(dev);
+	if (pm && pm->complete)
+		pm->complete(dev);
 }
 
 /**
@@ -1883,6 +1883,7 @@ static int iucv_path_table_empty(void)
 static int iucv_pm_freeze(struct device *dev)
 {
 	int cpu;
+	const struct dev_pm_ops *pm;
 	struct iucv_irq_list *p, *n;
 	int rc = 0;
 
@@ -1902,8 +1903,9 @@ static int iucv_pm_freeze(struct device *dev)
 		}
 	}
 	iucv_pm_state = IUCV_PM_FREEZING;
-	if (dev->driver && dev->driver->pm && dev->driver->pm->freeze)
-		rc = dev->driver->pm->freeze(dev);
+	pm = driver_to_pm(dev->driver);
+	if (pm && pm->freeze)
+		rc = pm->freeze(dev);
 	if (iucv_path_table_empty())
 		iucv_disable();
 	return rc;
@@ -1919,6 +1921,7 @@ static int iucv_pm_freeze(struct device *dev)
  */
 static int iucv_pm_thaw(struct device *dev)
 {
+	const struct dev_pm_ops *pm;
 	int rc = 0;
 
 #ifdef CONFIG_PM_DEBUG
@@ -1938,8 +1941,9 @@ static int iucv_pm_thaw(struct device *dev)
 			/* enable interrupts on all cpus */
 			iucv_setmask_mp();
 	}
-	if (dev->driver && dev->driver->pm && dev->driver->pm->thaw)
-		rc = dev->driver->pm->thaw(dev);
+	pm = driver_to_pm(dev->driver);
+	if (pm && pm->thaw)
+		rc = pm->thaw(dev);
 out:
 	return rc;
 }
@@ -1954,6 +1958,7 @@ static int iucv_pm_thaw(struct device *dev)
  */
 static int iucv_pm_restore(struct device *dev)
 {
+	const struct dev_pm_ops *pm;
 	int rc = 0;
 
 #ifdef CONFIG_PM_DEBUG
@@ -1968,8 +1973,9 @@ static int iucv_pm_restore(struct device *dev)
 		if (rc)
 			goto out;
 	}
-	if (dev->driver && dev->driver->pm && dev->driver->pm->restore)
-		rc = dev->driver->pm->restore(dev);
+	pm = driver_to_pm(dev->driver);
+	if (pm && pm->restore)
+		rc = pm->restore(dev);
 out:
 	return rc;
 }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/8] driver core: Add helper for accessing Power Management callbacs
  2020-05-25 18:26 ` [PATCH 1/8] driver core: " Krzysztof Wilczyński
@ 2020-05-26  6:33   ` Greg Kroah-Hartman
  2020-05-26 11:53     ` [greybus-dev] " Alex Elder
  0 siblings, 1 reply; 30+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-26  6:33 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Dan Carpenter, Rafael J. Wysocki, Len Brown, Kevin Hilman,
	Ulf Hansson, Pavel Machek, Johan Hovold, Alex Elder,
	Bjorn Helgaas, James E.J. Bottomley, Martin K. Petersen,
	Felipe Balbi, Julian Wiedmann, Karsten Graul, Ursula Braun,
	Jakub Kicinski, Bjorn Andersson, John Stultz, David S. Miller,
	greybus-dev, netdev, linux-acpi, linux-pci, linux-pm, linux-s390,
	linux-scsi, linux-usb

On Mon, May 25, 2020 at 06:26:01PM +0000, Krzysztof Wilczyński wrote:
> Add driver_to_pm() helper allowing for accessing the Power Management
> callbacs for a particular device.  Access to the callbacs (struct
> dev_pm_ops) is normally done through using the pm pointer that is
> embedded within the device_driver struct.
> 
> Helper allows for the code required to reference the pm pointer and
> access Power Management callbas to be simplified.  Changing the
> following:
> 
>   struct device_driver *drv = dev->driver;
>   if (dev->driver && dev->driver->pm && dev->driver->pm->prepare) {
>       int ret = dev->driver->pm->prepare(dev);
> 
> To:
> 
>   const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>   if (pm && pm->prepare) {
>       int ret = pm->prepare(dev);
> 
> Or, changing the following:
> 
>      const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> 
> To:
>      const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
> 
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> ---
>  include/linux/device/driver.h | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
> index ee7ba5b5417e..ccd0b315fd93 100644
> --- a/include/linux/device/driver.h
> +++ b/include/linux/device/driver.h
> @@ -236,6 +236,21 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev)
>  }
>  #endif
>  
> +/**
> + * driver_to_pm - Return Power Management callbacs (struct dev_pm_ops) for
> + *                a particular device.
> + * @drv: Pointer to a device (struct device_driver) for which you want to access
> + *       the Power Management callbacks.
> + *
> + * Returns a pointer to the struct dev_pm_ops embedded within the device (struct
> + * device_driver), or returns NULL if Power Management is not present and the
> + * pointer is not valid.
> + */
> +static inline const struct dev_pm_ops *driver_to_pm(struct device_driver *drv)
> +{
> +	return drv && drv->pm ? drv->pm : NULL;

I hate ? : lines with a passion, as they break normal pattern mattching
in my brain.  Please just spell this all out:
	if (drv && drv->pm)
		return drv->pm;
	return NULL;

Much easier to read, and the compiler will do the exact same thing.

Only place ? : are ok to use in my opinion, are as function arguments.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 8/8] net/iucv: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-25 18:26 ` [PATCH 8/8] net/iucv: " Krzysztof Wilczyński
@ 2020-05-26  6:35   ` Greg Kroah-Hartman
  2020-05-26 15:07     ` Krzysztof Wilczyński
  2020-05-26  7:07   ` Ursula Braun
  1 sibling, 1 reply; 30+ messages in thread
From: Greg Kroah-Hartman @ 2020-05-26  6:35 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Dan Carpenter, Rafael J. Wysocki, Len Brown, Kevin Hilman,
	Ulf Hansson, Pavel Machek, Johan Hovold, Alex Elder,
	Bjorn Helgaas, James E.J. Bottomley, Martin K. Petersen,
	Felipe Balbi, Julian Wiedmann, Karsten Graul, Ursula Braun,
	Jakub Kicinski, Bjorn Andersson, John Stultz, David S. Miller,
	greybus-dev, netdev, linux-acpi, linux-pci, linux-pm, linux-s390,
	linux-scsi, linux-usb

On Mon, May 25, 2020 at 06:26:08PM +0000, Krzysztof Wilczyński wrote:
> Use the new device_to_pm() helper to access Power Management callbacs
> (struct dev_pm_ops) for a particular device (struct device_driver).
> 
> No functional change intended.
> 
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> ---
>  net/iucv/iucv.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
> index 9a2d023842fe..1a3029ab7c1f 100644
> --- a/net/iucv/iucv.c
> +++ b/net/iucv/iucv.c
> @@ -1836,23 +1836,23 @@ static void iucv_external_interrupt(struct ext_code ext_code,
>  
>  static int iucv_pm_prepare(struct device *dev)
>  {
> -	int rc = 0;
> +	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>  
>  #ifdef CONFIG_PM_DEBUG
>  	printk(KERN_INFO "iucv_pm_prepare\n");
>  #endif
> -	if (dev->driver && dev->driver->pm && dev->driver->pm->prepare)
> -		rc = dev->driver->pm->prepare(dev);
> -	return rc;
> +	return pm && pm->prepare ? pm->prepare(dev) : 0;

No need for ? : here either, just use if () please.

It's "interesting" how using your new helper doesn't actually make the
code smaller.  Perhaps it isn't a good helper function?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 8/8] net/iucv: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-25 18:26 ` [PATCH 8/8] net/iucv: " Krzysztof Wilczyński
  2020-05-26  6:35   ` Greg Kroah-Hartman
@ 2020-05-26  7:07   ` Ursula Braun
  2020-05-26 14:57     ` Krzysztof Wilczyński
  1 sibling, 1 reply; 30+ messages in thread
From: Ursula Braun @ 2020-05-26  7:07 UTC (permalink / raw)
  To: Krzysztof Wilczyński, Dan Carpenter
  Cc: Rafael J. Wysocki, Len Brown, Kevin Hilman, Ulf Hansson,
	Pavel Machek, Greg Kroah-Hartman, Johan Hovold, Alex Elder,
	Bjorn Helgaas, James E.J. Bottomley, Martin K. Petersen,
	Felipe Balbi, Julian Wiedmann, Karsten Graul, Jakub Kicinski,
	Bjorn Andersson, John Stultz, David S. Miller, greybus-dev,
	netdev, linux-acpi, linux-pci, linux-pm, linux-s390, linux-scsi,
	linux-usb



On 5/25/20 8:26 PM, Krzysztof Wilczyński wrote:
> Use the new device_to_pm() helper to access Power Management callbacs
> (struct dev_pm_ops) for a particular device (struct device_driver).
> 
> No functional change intended.
> 
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>

pm support is going to be removed (for s390 in general and) for
net/iucv/iucv.c with this net-next patch:

commit 4b32f86bf1673acb16441dd55d7b325609f54897
Author: Julian Wiedmann <jwi@linux.ibm.com>
Date:   Tue May 19 21:10:08 2020 +0200

    net/iucv: remove pm support
    
    commit 394216275c7d ("s390: remove broken hibernate / power management support")
    removed support for ARCH_HIBERNATION_POSSIBLE from s390.
    
    So drop the unused pm ops from the s390-only iucv bus driver.
    
    CC: Hendrik Brueckner <brueckner@linux.ibm.com>
    Signed-off-by: Julian Wiedmann <jwi@linux.ibm.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

> ---
>  net/iucv/iucv.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/net/iucv/iucv.c b/net/iucv/iucv.c
> index 9a2d023842fe..1a3029ab7c1f 100644
> --- a/net/iucv/iucv.c
> +++ b/net/iucv/iucv.c

...

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 7/8] PM: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-25 18:26 ` [PATCH 7/8] PM: " Krzysztof Wilczyński
@ 2020-05-26  8:33     ` Rafael J. Wysocki
  0 siblings, 0 replies; 30+ messages in thread
From: Rafael J. Wysocki @ 2020-05-26  8:33 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Dan Carpenter, Rafael J. Wysocki, Len Brown, Kevin Hilman,
	Ulf Hansson, Pavel Machek, Greg Kroah-Hartman, Johan Hovold,
	Alex Elder, Bjorn Helgaas, James E.J. Bottomley,
	Martin K. Petersen, Felipe Balbi, Julian Wiedmann, Karsten Graul,
	Ursula Braun, Jakub Kicinski, Bjorn Andersson, John Stultz,
	David S. Miller, greybus-dev, netdev, ACPI Devel Maling List,
	Linux PCI, Linux PM, linux-s390, open list:TARGET SUBSYSTEM,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

On Mon, May 25, 2020 at 8:26 PM Krzysztof Wilczyński <kw@linux.com> wrote:
>
> Use the new device_to_pm() helper to access Power Management callbacs
> (struct dev_pm_ops) for a particular device (struct device_driver).
>
> No functional change intended.
>
> This change builds on top of the previous commit 6da2f2ccfd2d ("PCI/PM:
> Make power management op coding style consistent").
>
> Links:
>   https://lore.kernel.org/driverdev-devel/20191014230016.240912-6-helgaas@kernel.org/
>   https://lore.kernel.org/driverdev-devel/8592302.r4xC6RIy69@kreacher/
>   https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/
>
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> ---
>  drivers/base/power/domain.c      | 12 ++++--
>  drivers/base/power/generic_ops.c | 65 ++++++++++++++------------------
>  drivers/base/power/main.c        | 48 +++++++++++++++--------
>  drivers/base/power/runtime.c     |  7 ++--
>  4 files changed, 73 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 0a01df608849..92a96fcb2717 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -703,6 +703,7 @@ static void genpd_power_off_work_fn(struct work_struct *work)
>  static int __genpd_runtime_suspend(struct device *dev)
>  {
>         int (*cb)(struct device *__dev);
> +       const struct dev_pm_ops *pm;
>
>         if (dev->type && dev->type->pm)
>                 cb = dev->type->pm->runtime_suspend;
> @@ -713,8 +714,9 @@ static int __genpd_runtime_suspend(struct device *dev)
>         else
>                 cb = NULL;
>
> -       if (!cb && dev->driver && dev->driver->pm)
> -               cb = dev->driver->pm->runtime_suspend;
> +       pm = driver_to_pm(dev->driver);
> +       if (!cb && pm)
> +               cb = pm->runtime_suspend;

So why exactly is the new version better?

1. It adds lines of code.
2. It adds checks.
3. It adds function calls.
4. It makes one need to see the extra driver_to_pm() function to find
out what's going on.

Which of the above is an improvement?

>         return cb ? cb(dev) : 0;
>  }
> @@ -726,6 +728,7 @@ static int __genpd_runtime_suspend(struct device *dev)
>  static int __genpd_runtime_resume(struct device *dev)
>  {
>         int (*cb)(struct device *__dev);
> +       const struct dev_pm_ops *pm;
>
>         if (dev->type && dev->type->pm)
>                 cb = dev->type->pm->runtime_resume;
> @@ -736,8 +739,9 @@ static int __genpd_runtime_resume(struct device *dev)
>         else
>                 cb = NULL;
>
> -       if (!cb && dev->driver && dev->driver->pm)
> -               cb = dev->driver->pm->runtime_resume;
> +       pm = driver_to_pm(dev->driver);
> +       if (!cb && pm)
> +               cb = pm->runtime_resume;
>
>         return cb ? cb(dev) : 0;
>  }
> diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
> index 4fa525668cb7..fbd2edef0201 100644
> --- a/drivers/base/power/generic_ops.c
> +++ b/drivers/base/power/generic_ops.c
> @@ -19,12 +19,9 @@
>   */
>  int pm_generic_runtime_suspend(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> -       int ret;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
> -       ret = pm && pm->runtime_suspend ? pm->runtime_suspend(dev) : 0;
> -
> -       return ret;
> +       return pm && pm->runtime_suspend ? pm->runtime_suspend(dev) : 0;
>  }
>  EXPORT_SYMBOL_GPL(pm_generic_runtime_suspend);
>
> @@ -38,12 +35,9 @@ EXPORT_SYMBOL_GPL(pm_generic_runtime_suspend);
>   */
>  int pm_generic_runtime_resume(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> -       int ret;
> -
> -       ret = pm && pm->runtime_resume ? pm->runtime_resume(dev) : 0;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
> -       return ret;
> +       return pm && pm->runtime_resume ? pm->runtime_resume(dev) : 0;
>  }
>  EXPORT_SYMBOL_GPL(pm_generic_runtime_resume);
>  #endif /* CONFIG_PM */
> @@ -57,13 +51,12 @@ EXPORT_SYMBOL_GPL(pm_generic_runtime_resume);
>   */
>  int pm_generic_prepare(struct device *dev)
>  {
> -       struct device_driver *drv = dev->driver;
> -       int ret = 0;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
> -       if (drv && drv->pm && drv->pm->prepare)
> -               ret = drv->pm->prepare(dev);
> +       if (pm && pm->prepare)
> +               return pm->prepare(dev);
>
> -       return ret;
> +       return 0;
>  }
>
>  /**
> @@ -72,7 +65,7 @@ int pm_generic_prepare(struct device *dev)
>   */
>  int pm_generic_suspend_noirq(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->suspend_noirq ? pm->suspend_noirq(dev) : 0;
>  }
> @@ -84,7 +77,7 @@ EXPORT_SYMBOL_GPL(pm_generic_suspend_noirq);
>   */
>  int pm_generic_suspend_late(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->suspend_late ? pm->suspend_late(dev) : 0;
>  }
> @@ -96,7 +89,7 @@ EXPORT_SYMBOL_GPL(pm_generic_suspend_late);
>   */
>  int pm_generic_suspend(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->suspend ? pm->suspend(dev) : 0;
>  }
> @@ -108,7 +101,7 @@ EXPORT_SYMBOL_GPL(pm_generic_suspend);
>   */
>  int pm_generic_freeze_noirq(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->freeze_noirq ? pm->freeze_noirq(dev) : 0;
>  }
> @@ -120,7 +113,7 @@ EXPORT_SYMBOL_GPL(pm_generic_freeze_noirq);
>   */
>  int pm_generic_freeze_late(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->freeze_late ? pm->freeze_late(dev) : 0;
>  }
> @@ -132,7 +125,7 @@ EXPORT_SYMBOL_GPL(pm_generic_freeze_late);
>   */
>  int pm_generic_freeze(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->freeze ? pm->freeze(dev) : 0;
>  }
> @@ -144,7 +137,7 @@ EXPORT_SYMBOL_GPL(pm_generic_freeze);
>   */
>  int pm_generic_poweroff_noirq(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->poweroff_noirq ? pm->poweroff_noirq(dev) : 0;
>  }
> @@ -156,7 +149,7 @@ EXPORT_SYMBOL_GPL(pm_generic_poweroff_noirq);
>   */
>  int pm_generic_poweroff_late(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->poweroff_late ? pm->poweroff_late(dev) : 0;
>  }
> @@ -168,7 +161,7 @@ EXPORT_SYMBOL_GPL(pm_generic_poweroff_late);
>   */
>  int pm_generic_poweroff(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->poweroff ? pm->poweroff(dev) : 0;
>  }
> @@ -180,7 +173,7 @@ EXPORT_SYMBOL_GPL(pm_generic_poweroff);
>   */
>  int pm_generic_thaw_noirq(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->thaw_noirq ? pm->thaw_noirq(dev) : 0;
>  }
> @@ -192,7 +185,7 @@ EXPORT_SYMBOL_GPL(pm_generic_thaw_noirq);
>   */
>  int pm_generic_thaw_early(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->thaw_early ? pm->thaw_early(dev) : 0;
>  }
> @@ -204,7 +197,7 @@ EXPORT_SYMBOL_GPL(pm_generic_thaw_early);
>   */
>  int pm_generic_thaw(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->thaw ? pm->thaw(dev) : 0;
>  }
> @@ -216,7 +209,7 @@ EXPORT_SYMBOL_GPL(pm_generic_thaw);
>   */
>  int pm_generic_resume_noirq(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->resume_noirq ? pm->resume_noirq(dev) : 0;
>  }
> @@ -228,7 +221,7 @@ EXPORT_SYMBOL_GPL(pm_generic_resume_noirq);
>   */
>  int pm_generic_resume_early(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->resume_early ? pm->resume_early(dev) : 0;
>  }
> @@ -240,7 +233,7 @@ EXPORT_SYMBOL_GPL(pm_generic_resume_early);
>   */
>  int pm_generic_resume(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->resume ? pm->resume(dev) : 0;
>  }
> @@ -252,7 +245,7 @@ EXPORT_SYMBOL_GPL(pm_generic_resume);
>   */
>  int pm_generic_restore_noirq(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->restore_noirq ? pm->restore_noirq(dev) : 0;
>  }
> @@ -264,7 +257,7 @@ EXPORT_SYMBOL_GPL(pm_generic_restore_noirq);
>   */
>  int pm_generic_restore_early(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->restore_early ? pm->restore_early(dev) : 0;
>  }
> @@ -276,7 +269,7 @@ EXPORT_SYMBOL_GPL(pm_generic_restore_early);
>   */
>  int pm_generic_restore(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->restore ? pm->restore(dev) : 0;
>  }
> @@ -290,9 +283,9 @@ EXPORT_SYMBOL_GPL(pm_generic_restore);
>   */
>  void pm_generic_complete(struct device *dev)
>  {
> -       struct device_driver *drv = dev->driver;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
> -       if (drv && drv->pm && drv->pm->complete)
> -               drv->pm->complete(dev);
> +       if (pm && pm->complete)
> +               pm->complete(dev);
>  }
>  #endif /* CONFIG_PM_SLEEP */
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 0e07e17c2def..6c41da0bebb0 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -640,6 +640,7 @@ static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
>  static int device_resume_noirq(struct device *dev, pm_message_t state, bool async)
>  {
>         pm_callback_t callback;
> +       const struct dev_pm_ops *pm;
>         const char *info;
>         bool skip_resume;
>         int error = 0;
> @@ -687,9 +688,10 @@ static int device_resume_noirq(struct device *dev, pm_message_t state, bool asyn
>                 }
>         }
>
> -       if (dev->driver && dev->driver->pm) {
> +       pm = driver_to_pm(dev->driver);
> +       if (pm) {
>                 info = "noirq driver ";
> -               callback = pm_noirq_op(dev->driver->pm, state);
> +               callback = pm_noirq_op(pm, state);
>         }
>
>  Run:
> @@ -850,6 +852,7 @@ static pm_callback_t dpm_subsys_resume_early_cb(struct device *dev,
>  static int device_resume_early(struct device *dev, pm_message_t state, bool async)
>  {
>         pm_callback_t callback;
> +       const struct dev_pm_ops *pm;
>         const char *info;
>         int error = 0;
>
> @@ -867,9 +870,10 @@ static int device_resume_early(struct device *dev, pm_message_t state, bool asyn
>
>         callback = dpm_subsys_resume_early_cb(dev, state, &info);
>
> -       if (!callback && dev->driver && dev->driver->pm) {
> +       pm = driver_to_pm(dev->driver);
> +       if (!callback && pm) {
>                 info = "early driver ";
> -               callback = pm_late_early_op(dev->driver->pm, state);
> +               callback = pm_late_early_op(pm, state);
>         }
>
>         error = dpm_run_callback(callback, dev, state, info);
> @@ -963,6 +967,7 @@ static int device_resume(struct device *dev, pm_message_t state, bool async)
>  {
>         pm_callback_t callback = NULL;
>         const char *info = NULL;
> +       const struct dev_pm_ops *pm = NULL;
>         int error = 0;
>         DECLARE_DPM_WATCHDOG_ON_STACK(wd);
>
> @@ -1023,9 +1028,10 @@ static int device_resume(struct device *dev, pm_message_t state, bool async)
>         }
>
>   Driver:
> -       if (!callback && dev->driver && dev->driver->pm) {
> +       pm = driver_to_pm(dev->driver);
> +       if (!callback && pm) {
>                 info = "driver ";
> -               callback = pm_op(dev->driver->pm, state);
> +               callback = pm_op(pm, state);
>         }
>
>   End:
> @@ -1116,6 +1122,7 @@ void dpm_resume(pm_message_t state)
>  static void device_complete(struct device *dev, pm_message_t state)
>  {
>         void (*callback)(struct device *) = NULL;
> +       const struct dev_pm_ops *pm = NULL;
>         const char *info = NULL;
>
>         if (dev->power.syscore)
> @@ -1137,9 +1144,10 @@ static void device_complete(struct device *dev, pm_message_t state)
>                 callback = dev->bus->pm->complete;
>         }
>
> -       if (!callback && dev->driver && dev->driver->pm) {
> +       pm = driver_to_pm(dev->driver);
> +       if (!callback && pm) {
>                 info = "completing driver ";
> -               callback = dev->driver->pm->complete;
> +               callback = pm->complete;
>         }
>
>         if (callback) {
> @@ -1312,6 +1320,7 @@ static bool device_must_resume(struct device *dev, pm_message_t state,
>  static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool async)
>  {
>         pm_callback_t callback;
> +       const struct dev_pm_ops *pm;
>         const char *info;
>         bool no_subsys_cb = false;
>         int error = 0;
> @@ -1336,9 +1345,10 @@ static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool a
>         if (dev_pm_smart_suspend_and_suspended(dev) && no_subsys_cb)
>                 goto Skip;
>
> -       if (dev->driver && dev->driver->pm) {
> +       pm = driver_to_pm(dev->driver);
> +       if (pm) {
>                 info = "noirq driver ";
> -               callback = pm_noirq_op(dev->driver->pm, state);
> +               callback = pm_noirq_op(pm, state);
>         }
>
>  Run:
> @@ -1514,6 +1524,7 @@ static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
>  static int __device_suspend_late(struct device *dev, pm_message_t state, bool async)
>  {
>         pm_callback_t callback;
> +       const struct dev_pm_ops *pm;
>         const char *info;
>         int error = 0;
>
> @@ -1543,9 +1554,10 @@ static int __device_suspend_late(struct device *dev, pm_message_t state, bool as
>             !dpm_subsys_suspend_noirq_cb(dev, state, NULL))
>                 goto Skip;
>
> -       if (dev->driver && dev->driver->pm) {
> +       pm = driver_to_pm(dev->driver);
> +       if (pm) {
>                 info = "late driver ";
> -               callback = pm_late_early_op(dev->driver->pm, state);
> +               callback = pm_late_early_op(pm, state);
>         }
>
>  Run:
> @@ -1717,6 +1729,7 @@ static void dpm_clear_superiors_direct_complete(struct device *dev)
>  static int __device_suspend(struct device *dev, pm_message_t state, bool async)
>  {
>         pm_callback_t callback = NULL;
> +       const struct dev_pm_ops *pm = NULL;
>         const char *info = NULL;
>         int error = 0;
>         DECLARE_DPM_WATCHDOG_ON_STACK(wd);
> @@ -1803,9 +1816,10 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
>         }
>
>   Run:
> -       if (!callback && dev->driver && dev->driver->pm) {
> +       pm = driver_to_pm(dev->driver);
> +       if (!callback && pm) {
>                 info = "driver ";
> -               callback = pm_op(dev->driver->pm, state);
> +               callback = pm_op(pm, state);
>         }
>
>         error = dpm_run_callback(callback, dev, state, info);
> @@ -1917,6 +1931,7 @@ int dpm_suspend(pm_message_t state)
>  static int device_prepare(struct device *dev, pm_message_t state)
>  {
>         int (*callback)(struct device *) = NULL;
> +       const struct dev_pm_ops *pm = NULL;
>         int ret = 0;
>
>         if (dev->power.syscore)
> @@ -1946,8 +1961,9 @@ static int device_prepare(struct device *dev, pm_message_t state)
>         else if (dev->bus && dev->bus->pm)
>                 callback = dev->bus->pm->prepare;
>
> -       if (!callback && dev->driver && dev->driver->pm)
> -               callback = dev->driver->pm->prepare;
> +       pm = driver_to_pm(dev->driver);
> +       if (!callback && pm)
> +               callback = pm->prepare;
>
>         if (callback)
>                 ret = callback(dev);
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 99c7da112c95..c142824c7541 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -21,7 +21,7 @@ typedef int (*pm_callback_t)(struct device *);
>  static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset)
>  {
>         pm_callback_t cb;
> -       const struct dev_pm_ops *ops;
> +       const struct dev_pm_ops *ops, *pm;
>
>         if (dev->pm_domain)
>                 ops = &dev->pm_domain->ops;
> @@ -39,8 +39,9 @@ static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset)
>         else
>                 cb = NULL;
>
> -       if (!cb && dev->driver && dev->driver->pm)
> -               cb = *(pm_callback_t *)((void *)dev->driver->pm + cb_offset);
> +       pm = driver_to_pm(dev->driver);
> +       if (!cb && pm)
> +               cb = *(pm_callback_t *)((void *)pm + cb_offset);
>
>         return cb;
>  }
> --
> 2.26.2
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 7/8] PM: Use the new device_to_pm() helper to access struct dev_pm_ops
@ 2020-05-26  8:33     ` Rafael J. Wysocki
  0 siblings, 0 replies; 30+ messages in thread
From: Rafael J. Wysocki @ 2020-05-26  8:33 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Dan Carpenter, Rafael J. Wysocki, Len Brown, Kevin Hilman,
	Ulf Hansson, Pavel Machek, Greg Kroah-Hartman, Johan Hovold,
	Alex Elder, Bjorn Helgaas, James E.J. Bottomley,
	Martin K. Petersen, Felipe Balbi, Julian Wiedmann, Karsten Graul,
	Ursula Braun, Jakub Kicinski, Bjorn Andersson, John Stultz,
	David S. Miller, greybus-dev, netdev, ACPI Devel Maling List,
	Linux PCI, Linux PM, linux-s390, open list:TARGET SUBSYSTEM,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

On Mon, May 25, 2020 at 8:26 PM Krzysztof Wilczyński <kw@linux.com> wrote:
>
> Use the new device_to_pm() helper to access Power Management callbacs
> (struct dev_pm_ops) for a particular device (struct device_driver).
>
> No functional change intended.
>
> This change builds on top of the previous commit 6da2f2ccfd2d ("PCI/PM:
> Make power management op coding style consistent").
>
> Links:
>   https://lore.kernel.org/driverdev-devel/20191014230016.240912-6-helgaas@kernel.org/
>   https://lore.kernel.org/driverdev-devel/8592302.r4xC6RIy69@kreacher/
>   https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/
>
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> ---
>  drivers/base/power/domain.c      | 12 ++++--
>  drivers/base/power/generic_ops.c | 65 ++++++++++++++------------------
>  drivers/base/power/main.c        | 48 +++++++++++++++--------
>  drivers/base/power/runtime.c     |  7 ++--
>  4 files changed, 73 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 0a01df608849..92a96fcb2717 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -703,6 +703,7 @@ static void genpd_power_off_work_fn(struct work_struct *work)
>  static int __genpd_runtime_suspend(struct device *dev)
>  {
>         int (*cb)(struct device *__dev);
> +       const struct dev_pm_ops *pm;
>
>         if (dev->type && dev->type->pm)
>                 cb = dev->type->pm->runtime_suspend;
> @@ -713,8 +714,9 @@ static int __genpd_runtime_suspend(struct device *dev)
>         else
>                 cb = NULL;
>
> -       if (!cb && dev->driver && dev->driver->pm)
> -               cb = dev->driver->pm->runtime_suspend;
> +       pm = driver_to_pm(dev->driver);
> +       if (!cb && pm)
> +               cb = pm->runtime_suspend;

So why exactly is the new version better?

1. It adds lines of code.
2. It adds checks.
3. It adds function calls.
4. It makes one need to see the extra driver_to_pm() function to find
out what's going on.

Which of the above is an improvement?

>         return cb ? cb(dev) : 0;
>  }
> @@ -726,6 +728,7 @@ static int __genpd_runtime_suspend(struct device *dev)
>  static int __genpd_runtime_resume(struct device *dev)
>  {
>         int (*cb)(struct device *__dev);
> +       const struct dev_pm_ops *pm;
>
>         if (dev->type && dev->type->pm)
>                 cb = dev->type->pm->runtime_resume;
> @@ -736,8 +739,9 @@ static int __genpd_runtime_resume(struct device *dev)
>         else
>                 cb = NULL;
>
> -       if (!cb && dev->driver && dev->driver->pm)
> -               cb = dev->driver->pm->runtime_resume;
> +       pm = driver_to_pm(dev->driver);
> +       if (!cb && pm)
> +               cb = pm->runtime_resume;
>
>         return cb ? cb(dev) : 0;
>  }
> diff --git a/drivers/base/power/generic_ops.c b/drivers/base/power/generic_ops.c
> index 4fa525668cb7..fbd2edef0201 100644
> --- a/drivers/base/power/generic_ops.c
> +++ b/drivers/base/power/generic_ops.c
> @@ -19,12 +19,9 @@
>   */
>  int pm_generic_runtime_suspend(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> -       int ret;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
> -       ret = pm && pm->runtime_suspend ? pm->runtime_suspend(dev) : 0;
> -
> -       return ret;
> +       return pm && pm->runtime_suspend ? pm->runtime_suspend(dev) : 0;
>  }
>  EXPORT_SYMBOL_GPL(pm_generic_runtime_suspend);
>
> @@ -38,12 +35,9 @@ EXPORT_SYMBOL_GPL(pm_generic_runtime_suspend);
>   */
>  int pm_generic_runtime_resume(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> -       int ret;
> -
> -       ret = pm && pm->runtime_resume ? pm->runtime_resume(dev) : 0;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
> -       return ret;
> +       return pm && pm->runtime_resume ? pm->runtime_resume(dev) : 0;
>  }
>  EXPORT_SYMBOL_GPL(pm_generic_runtime_resume);
>  #endif /* CONFIG_PM */
> @@ -57,13 +51,12 @@ EXPORT_SYMBOL_GPL(pm_generic_runtime_resume);
>   */
>  int pm_generic_prepare(struct device *dev)
>  {
> -       struct device_driver *drv = dev->driver;
> -       int ret = 0;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
> -       if (drv && drv->pm && drv->pm->prepare)
> -               ret = drv->pm->prepare(dev);
> +       if (pm && pm->prepare)
> +               return pm->prepare(dev);
>
> -       return ret;
> +       return 0;
>  }
>
>  /**
> @@ -72,7 +65,7 @@ int pm_generic_prepare(struct device *dev)
>   */
>  int pm_generic_suspend_noirq(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->suspend_noirq ? pm->suspend_noirq(dev) : 0;
>  }
> @@ -84,7 +77,7 @@ EXPORT_SYMBOL_GPL(pm_generic_suspend_noirq);
>   */
>  int pm_generic_suspend_late(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->suspend_late ? pm->suspend_late(dev) : 0;
>  }
> @@ -96,7 +89,7 @@ EXPORT_SYMBOL_GPL(pm_generic_suspend_late);
>   */
>  int pm_generic_suspend(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->suspend ? pm->suspend(dev) : 0;
>  }
> @@ -108,7 +101,7 @@ EXPORT_SYMBOL_GPL(pm_generic_suspend);
>   */
>  int pm_generic_freeze_noirq(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->freeze_noirq ? pm->freeze_noirq(dev) : 0;
>  }
> @@ -120,7 +113,7 @@ EXPORT_SYMBOL_GPL(pm_generic_freeze_noirq);
>   */
>  int pm_generic_freeze_late(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->freeze_late ? pm->freeze_late(dev) : 0;
>  }
> @@ -132,7 +125,7 @@ EXPORT_SYMBOL_GPL(pm_generic_freeze_late);
>   */
>  int pm_generic_freeze(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->freeze ? pm->freeze(dev) : 0;
>  }
> @@ -144,7 +137,7 @@ EXPORT_SYMBOL_GPL(pm_generic_freeze);
>   */
>  int pm_generic_poweroff_noirq(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->poweroff_noirq ? pm->poweroff_noirq(dev) : 0;
>  }
> @@ -156,7 +149,7 @@ EXPORT_SYMBOL_GPL(pm_generic_poweroff_noirq);
>   */
>  int pm_generic_poweroff_late(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->poweroff_late ? pm->poweroff_late(dev) : 0;
>  }
> @@ -168,7 +161,7 @@ EXPORT_SYMBOL_GPL(pm_generic_poweroff_late);
>   */
>  int pm_generic_poweroff(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->poweroff ? pm->poweroff(dev) : 0;
>  }
> @@ -180,7 +173,7 @@ EXPORT_SYMBOL_GPL(pm_generic_poweroff);
>   */
>  int pm_generic_thaw_noirq(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->thaw_noirq ? pm->thaw_noirq(dev) : 0;
>  }
> @@ -192,7 +185,7 @@ EXPORT_SYMBOL_GPL(pm_generic_thaw_noirq);
>   */
>  int pm_generic_thaw_early(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->thaw_early ? pm->thaw_early(dev) : 0;
>  }
> @@ -204,7 +197,7 @@ EXPORT_SYMBOL_GPL(pm_generic_thaw_early);
>   */
>  int pm_generic_thaw(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->thaw ? pm->thaw(dev) : 0;
>  }
> @@ -216,7 +209,7 @@ EXPORT_SYMBOL_GPL(pm_generic_thaw);
>   */
>  int pm_generic_resume_noirq(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->resume_noirq ? pm->resume_noirq(dev) : 0;
>  }
> @@ -228,7 +221,7 @@ EXPORT_SYMBOL_GPL(pm_generic_resume_noirq);
>   */
>  int pm_generic_resume_early(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->resume_early ? pm->resume_early(dev) : 0;
>  }
> @@ -240,7 +233,7 @@ EXPORT_SYMBOL_GPL(pm_generic_resume_early);
>   */
>  int pm_generic_resume(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->resume ? pm->resume(dev) : 0;
>  }
> @@ -252,7 +245,7 @@ EXPORT_SYMBOL_GPL(pm_generic_resume);
>   */
>  int pm_generic_restore_noirq(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->restore_noirq ? pm->restore_noirq(dev) : 0;
>  }
> @@ -264,7 +257,7 @@ EXPORT_SYMBOL_GPL(pm_generic_restore_noirq);
>   */
>  int pm_generic_restore_early(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->restore_early ? pm->restore_early(dev) : 0;
>  }
> @@ -276,7 +269,7 @@ EXPORT_SYMBOL_GPL(pm_generic_restore_early);
>   */
>  int pm_generic_restore(struct device *dev)
>  {
> -       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
>         return pm && pm->restore ? pm->restore(dev) : 0;
>  }
> @@ -290,9 +283,9 @@ EXPORT_SYMBOL_GPL(pm_generic_restore);
>   */
>  void pm_generic_complete(struct device *dev)
>  {
> -       struct device_driver *drv = dev->driver;
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>
> -       if (drv && drv->pm && drv->pm->complete)
> -               drv->pm->complete(dev);
> +       if (pm && pm->complete)
> +               pm->complete(dev);
>  }
>  #endif /* CONFIG_PM_SLEEP */
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 0e07e17c2def..6c41da0bebb0 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -640,6 +640,7 @@ static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
>  static int device_resume_noirq(struct device *dev, pm_message_t state, bool async)
>  {
>         pm_callback_t callback;
> +       const struct dev_pm_ops *pm;
>         const char *info;
>         bool skip_resume;
>         int error = 0;
> @@ -687,9 +688,10 @@ static int device_resume_noirq(struct device *dev, pm_message_t state, bool asyn
>                 }
>         }
>
> -       if (dev->driver && dev->driver->pm) {
> +       pm = driver_to_pm(dev->driver);
> +       if (pm) {
>                 info = "noirq driver ";
> -               callback = pm_noirq_op(dev->driver->pm, state);
> +               callback = pm_noirq_op(pm, state);
>         }
>
>  Run:
> @@ -850,6 +852,7 @@ static pm_callback_t dpm_subsys_resume_early_cb(struct device *dev,
>  static int device_resume_early(struct device *dev, pm_message_t state, bool async)
>  {
>         pm_callback_t callback;
> +       const struct dev_pm_ops *pm;
>         const char *info;
>         int error = 0;
>
> @@ -867,9 +870,10 @@ static int device_resume_early(struct device *dev, pm_message_t state, bool asyn
>
>         callback = dpm_subsys_resume_early_cb(dev, state, &info);
>
> -       if (!callback && dev->driver && dev->driver->pm) {
> +       pm = driver_to_pm(dev->driver);
> +       if (!callback && pm) {
>                 info = "early driver ";
> -               callback = pm_late_early_op(dev->driver->pm, state);
> +               callback = pm_late_early_op(pm, state);
>         }
>
>         error = dpm_run_callback(callback, dev, state, info);
> @@ -963,6 +967,7 @@ static int device_resume(struct device *dev, pm_message_t state, bool async)
>  {
>         pm_callback_t callback = NULL;
>         const char *info = NULL;
> +       const struct dev_pm_ops *pm = NULL;
>         int error = 0;
>         DECLARE_DPM_WATCHDOG_ON_STACK(wd);
>
> @@ -1023,9 +1028,10 @@ static int device_resume(struct device *dev, pm_message_t state, bool async)
>         }
>
>   Driver:
> -       if (!callback && dev->driver && dev->driver->pm) {
> +       pm = driver_to_pm(dev->driver);
> +       if (!callback && pm) {
>                 info = "driver ";
> -               callback = pm_op(dev->driver->pm, state);
> +               callback = pm_op(pm, state);
>         }
>
>   End:
> @@ -1116,6 +1122,7 @@ void dpm_resume(pm_message_t state)
>  static void device_complete(struct device *dev, pm_message_t state)
>  {
>         void (*callback)(struct device *) = NULL;
> +       const struct dev_pm_ops *pm = NULL;
>         const char *info = NULL;
>
>         if (dev->power.syscore)
> @@ -1137,9 +1144,10 @@ static void device_complete(struct device *dev, pm_message_t state)
>                 callback = dev->bus->pm->complete;
>         }
>
> -       if (!callback && dev->driver && dev->driver->pm) {
> +       pm = driver_to_pm(dev->driver);
> +       if (!callback && pm) {
>                 info = "completing driver ";
> -               callback = dev->driver->pm->complete;
> +               callback = pm->complete;
>         }
>
>         if (callback) {
> @@ -1312,6 +1320,7 @@ static bool device_must_resume(struct device *dev, pm_message_t state,
>  static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool async)
>  {
>         pm_callback_t callback;
> +       const struct dev_pm_ops *pm;
>         const char *info;
>         bool no_subsys_cb = false;
>         int error = 0;
> @@ -1336,9 +1345,10 @@ static int __device_suspend_noirq(struct device *dev, pm_message_t state, bool a
>         if (dev_pm_smart_suspend_and_suspended(dev) && no_subsys_cb)
>                 goto Skip;
>
> -       if (dev->driver && dev->driver->pm) {
> +       pm = driver_to_pm(dev->driver);
> +       if (pm) {
>                 info = "noirq driver ";
> -               callback = pm_noirq_op(dev->driver->pm, state);
> +               callback = pm_noirq_op(pm, state);
>         }
>
>  Run:
> @@ -1514,6 +1524,7 @@ static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
>  static int __device_suspend_late(struct device *dev, pm_message_t state, bool async)
>  {
>         pm_callback_t callback;
> +       const struct dev_pm_ops *pm;
>         const char *info;
>         int error = 0;
>
> @@ -1543,9 +1554,10 @@ static int __device_suspend_late(struct device *dev, pm_message_t state, bool as
>             !dpm_subsys_suspend_noirq_cb(dev, state, NULL))
>                 goto Skip;
>
> -       if (dev->driver && dev->driver->pm) {
> +       pm = driver_to_pm(dev->driver);
> +       if (pm) {
>                 info = "late driver ";
> -               callback = pm_late_early_op(dev->driver->pm, state);
> +               callback = pm_late_early_op(pm, state);
>         }
>
>  Run:
> @@ -1717,6 +1729,7 @@ static void dpm_clear_superiors_direct_complete(struct device *dev)
>  static int __device_suspend(struct device *dev, pm_message_t state, bool async)
>  {
>         pm_callback_t callback = NULL;
> +       const struct dev_pm_ops *pm = NULL;
>         const char *info = NULL;
>         int error = 0;
>         DECLARE_DPM_WATCHDOG_ON_STACK(wd);
> @@ -1803,9 +1816,10 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
>         }
>
>   Run:
> -       if (!callback && dev->driver && dev->driver->pm) {
> +       pm = driver_to_pm(dev->driver);
> +       if (!callback && pm) {
>                 info = "driver ";
> -               callback = pm_op(dev->driver->pm, state);
> +               callback = pm_op(pm, state);
>         }
>
>         error = dpm_run_callback(callback, dev, state, info);
> @@ -1917,6 +1931,7 @@ int dpm_suspend(pm_message_t state)
>  static int device_prepare(struct device *dev, pm_message_t state)
>  {
>         int (*callback)(struct device *) = NULL;
> +       const struct dev_pm_ops *pm = NULL;
>         int ret = 0;
>
>         if (dev->power.syscore)
> @@ -1946,8 +1961,9 @@ static int device_prepare(struct device *dev, pm_message_t state)
>         else if (dev->bus && dev->bus->pm)
>                 callback = dev->bus->pm->prepare;
>
> -       if (!callback && dev->driver && dev->driver->pm)
> -               callback = dev->driver->pm->prepare;
> +       pm = driver_to_pm(dev->driver);
> +       if (!callback && pm)
> +               callback = pm->prepare;
>
>         if (callback)
>                 ret = callback(dev);
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 99c7da112c95..c142824c7541 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -21,7 +21,7 @@ typedef int (*pm_callback_t)(struct device *);
>  static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset)
>  {
>         pm_callback_t cb;
> -       const struct dev_pm_ops *ops;
> +       const struct dev_pm_ops *ops, *pm;
>
>         if (dev->pm_domain)
>                 ops = &dev->pm_domain->ops;
> @@ -39,8 +39,9 @@ static pm_callback_t __rpm_get_callback(struct device *dev, size_t cb_offset)
>         else
>                 cb = NULL;
>
> -       if (!cb && dev->driver && dev->driver->pm)
> -               cb = *(pm_callback_t *)((void *)dev->driver->pm + cb_offset);
> +       pm = driver_to_pm(dev->driver);
> +       if (!cb && pm)
> +               cb = *(pm_callback_t *)((void *)pm + cb_offset);
>
>         return cb;
>  }
> --
> 2.26.2
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/8] ACPI: PM: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-25 18:26 ` [PATCH 2/8] ACPI: PM: Use the new device_to_pm() helper to access struct dev_pm_ops Krzysztof Wilczyński
@ 2020-05-26  8:37     ` Rafael J. Wysocki
  0 siblings, 0 replies; 30+ messages in thread
From: Rafael J. Wysocki @ 2020-05-26  8:37 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Dan Carpenter, Rafael J. Wysocki, Len Brown, Kevin Hilman,
	Ulf Hansson, Pavel Machek, Greg Kroah-Hartman, Johan Hovold,
	Alex Elder, Bjorn Helgaas, James E.J. Bottomley,
	Martin K. Petersen, Felipe Balbi, Julian Wiedmann, Karsten Graul,
	Ursula Braun, Jakub Kicinski, Bjorn Andersson, John Stultz,
	David S. Miller, greybus-dev, netdev, ACPI Devel Maling List,
	Linux PCI, Linux PM, linux-s390, open list:TARGET SUBSYSTEM,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

On Mon, May 25, 2020 at 8:26 PM Krzysztof Wilczyński <kw@linux.com> wrote:
>
> Use the new device_to_pm() helper to access Power Management callbacs
> (struct dev_pm_ops) for a particular device (struct device_driver).
>
> No functional change intended.
>
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> ---
>  drivers/acpi/device_pm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index 5832bc10aca8..b98a32c48fbe 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -1022,9 +1022,10 @@ static bool acpi_dev_needs_resume(struct device *dev, struct acpi_device *adev)
>  int acpi_subsys_prepare(struct device *dev)
>  {
>         struct acpi_device *adev = ACPI_COMPANION(dev);
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);

I don't really see a reason for this change.

What's wrong with the check below?

>
> -       if (dev->driver && dev->driver->pm && dev->driver->pm->prepare) {
> -               int ret = dev->driver->pm->prepare(dev);
> +       if (pm && pm->prepare) {
> +               int ret = pm->prepare(dev);
>
>                 if (ret < 0)
>                         return ret;
> --
> 2.26.2
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/8] ACPI: PM: Use the new device_to_pm() helper to access struct dev_pm_ops
@ 2020-05-26  8:37     ` Rafael J. Wysocki
  0 siblings, 0 replies; 30+ messages in thread
From: Rafael J. Wysocki @ 2020-05-26  8:37 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Dan Carpenter, Rafael J. Wysocki, Len Brown, Kevin Hilman,
	Ulf Hansson, Pavel Machek, Greg Kroah-Hartman, Johan Hovold,
	Alex Elder, Bjorn Helgaas, James E.J. Bottomley,
	Martin K. Petersen, Felipe Balbi, Julian Wiedmann, Karsten Graul,
	Ursula Braun, Jakub Kicinski, Bjorn Andersson, John Stultz,
	David S. Miller, greybus-dev, netdev, ACPI Devel Maling List,
	Linux PCI, Linux PM, linux-s390, open list:TARGET SUBSYSTEM,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

On Mon, May 25, 2020 at 8:26 PM Krzysztof Wilczyński <kw@linux.com> wrote:
>
> Use the new device_to_pm() helper to access Power Management callbacs
> (struct dev_pm_ops) for a particular device (struct device_driver).
>
> No functional change intended.
>
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> ---
>  drivers/acpi/device_pm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index 5832bc10aca8..b98a32c48fbe 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -1022,9 +1022,10 @@ static bool acpi_dev_needs_resume(struct device *dev, struct acpi_device *adev)
>  int acpi_subsys_prepare(struct device *dev)
>  {
>         struct acpi_device *adev = ACPI_COMPANION(dev);
> +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);

I don't really see a reason for this change.

What's wrong with the check below?

>
> -       if (dev->driver && dev->driver->pm && dev->driver->pm->prepare) {
> -               int ret = dev->driver->pm->prepare(dev);
> +       if (pm && pm->prepare) {
> +               int ret = pm->prepare(dev);
>
>                 if (ret < 0)
>                         return ret;
> --
> 2.26.2
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 5/8] usb: phy: fsl: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-25 18:26 ` [PATCH 5/8] usb: phy: fsl: " Krzysztof Wilczyński
@ 2020-05-26  8:38     ` Rafael J. Wysocki
  0 siblings, 0 replies; 30+ messages in thread
From: Rafael J. Wysocki @ 2020-05-26  8:38 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Dan Carpenter, Rafael J. Wysocki, Len Brown, Kevin Hilman,
	Ulf Hansson, Pavel Machek, Greg Kroah-Hartman, Johan Hovold,
	Alex Elder, Bjorn Helgaas, James E.J. Bottomley,
	Martin K. Petersen, Felipe Balbi, Julian Wiedmann, Karsten Graul,
	Ursula Braun, Jakub Kicinski, Bjorn Andersson, John Stultz,
	David S. Miller, greybus-dev, netdev, ACPI Devel Maling List,
	Linux PCI, Linux PM, linux-s390, open list:TARGET SUBSYSTEM,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

On Mon, May 25, 2020 at 8:26 PM Krzysztof Wilczyński <kw@linux.com> wrote:
>
> Use the new device_to_pm() helper to access Power Management callbacs
> (struct dev_pm_ops) for a particular device (struct device_driver).
>
> No functional change intended.
>
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> ---
>  drivers/usb/phy/phy-fsl-usb.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
> index b451f4695f3f..3b9ad5db8380 100644
> --- a/drivers/usb/phy/phy-fsl-usb.c
> +++ b/drivers/usb/phy/phy-fsl-usb.c
> @@ -460,6 +460,7 @@ int fsl_otg_start_host(struct otg_fsm *fsm, int on)
>         struct device *dev;
>         struct fsl_otg *otg_dev =
>                 container_of(otg->usb_phy, struct fsl_otg, phy);
> +       const struct dev_pm_ops *pm;
>         u32 retval = 0;
>
>         if (!otg->host)
> @@ -479,8 +480,9 @@ int fsl_otg_start_host(struct otg_fsm *fsm, int on)
>                 else {
>                         otg_reset_controller();
>                         VDBG("host on......\n");
> -                       if (dev->driver->pm && dev->driver->pm->resume) {
> -                               retval = dev->driver->pm->resume(dev);
> +                       pm = driver_to_pm(dev->driver);
> +                       if (pm && pm->resume) {
> +                               retval = pm->resume(dev);

And why is the new version better this time?

>                                 if (fsm->id) {
>                                         /* default-b */
>                                         fsl_otg_drv_vbus(fsm, 1);
> @@ -504,8 +506,9 @@ int fsl_otg_start_host(struct otg_fsm *fsm, int on)
>                 else {
>                         VDBG("host off......\n");
>                         if (dev && dev->driver) {
> -                               if (dev->driver->pm && dev->driver->pm->suspend)
> -                                       retval = dev->driver->pm->suspend(dev);
> +                               pm = driver_to_pm(dev->driver);
> +                               if (pm && pm->suspend)
> +                                       retval = pm->suspend(dev);
>                                 if (fsm->id)
>                                         /* default-b */
>                                         fsl_otg_drv_vbus(fsm, 0);
> --
> 2.26.2
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 5/8] usb: phy: fsl: Use the new device_to_pm() helper to access struct dev_pm_ops
@ 2020-05-26  8:38     ` Rafael J. Wysocki
  0 siblings, 0 replies; 30+ messages in thread
From: Rafael J. Wysocki @ 2020-05-26  8:38 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Dan Carpenter, Rafael J. Wysocki, Len Brown, Kevin Hilman,
	Ulf Hansson, Pavel Machek, Greg Kroah-Hartman, Johan Hovold,
	Alex Elder, Bjorn Helgaas, James E.J. Bottomley,
	Martin K. Petersen, Felipe Balbi, Julian Wiedmann, Karsten Graul,
	Ursula Braun, Jakub Kicinski, Bjorn Andersson, John Stultz,
	David S. Miller, greybus-dev, netdev, ACPI Devel Maling List,
	Linux PCI, Linux PM, linux-s390, open list:TARGET SUBSYSTEM,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

On Mon, May 25, 2020 at 8:26 PM Krzysztof Wilczyński <kw@linux.com> wrote:
>
> Use the new device_to_pm() helper to access Power Management callbacs
> (struct dev_pm_ops) for a particular device (struct device_driver).
>
> No functional change intended.
>
> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> ---
>  drivers/usb/phy/phy-fsl-usb.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/phy/phy-fsl-usb.c b/drivers/usb/phy/phy-fsl-usb.c
> index b451f4695f3f..3b9ad5db8380 100644
> --- a/drivers/usb/phy/phy-fsl-usb.c
> +++ b/drivers/usb/phy/phy-fsl-usb.c
> @@ -460,6 +460,7 @@ int fsl_otg_start_host(struct otg_fsm *fsm, int on)
>         struct device *dev;
>         struct fsl_otg *otg_dev =
>                 container_of(otg->usb_phy, struct fsl_otg, phy);
> +       const struct dev_pm_ops *pm;
>         u32 retval = 0;
>
>         if (!otg->host)
> @@ -479,8 +480,9 @@ int fsl_otg_start_host(struct otg_fsm *fsm, int on)
>                 else {
>                         otg_reset_controller();
>                         VDBG("host on......\n");
> -                       if (dev->driver->pm && dev->driver->pm->resume) {
> -                               retval = dev->driver->pm->resume(dev);
> +                       pm = driver_to_pm(dev->driver);
> +                       if (pm && pm->resume) {
> +                               retval = pm->resume(dev);

And why is the new version better this time?

>                                 if (fsm->id) {
>                                         /* default-b */
>                                         fsl_otg_drv_vbus(fsm, 1);
> @@ -504,8 +506,9 @@ int fsl_otg_start_host(struct otg_fsm *fsm, int on)
>                 else {
>                         VDBG("host off......\n");
>                         if (dev && dev->driver) {
> -                               if (dev->driver->pm && dev->driver->pm->suspend)
> -                                       retval = dev->driver->pm->suspend(dev);
> +                               pm = driver_to_pm(dev->driver);
> +                               if (pm && pm->suspend)
> +                                       retval = pm->suspend(dev);
>                                 if (fsm->id)
>                                         /* default-b */
>                                         fsl_otg_drv_vbus(fsm, 0);
> --
> 2.26.2
>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/8] ACPI: PM: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-26  8:37     ` Rafael J. Wysocki
  (?)
@ 2020-05-26  9:45     ` Pavel Machek
  2020-05-26 10:35       ` Rafael J. Wysocki
  -1 siblings, 1 reply; 30+ messages in thread
From: Pavel Machek @ 2020-05-26  9:45 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Krzysztof Wilczyński, Dan Carpenter, Rafael J. Wysocki,
	Len Brown, Kevin Hilman, Ulf Hansson, Greg Kroah-Hartman,
	Johan Hovold, Alex Elder, Bjorn Helgaas, James E.J. Bottomley,
	Martin K. Petersen, Felipe Balbi, Julian Wiedmann, Karsten Graul,
	Ursula Braun, Jakub Kicinski, Bjorn Andersson, John Stultz,
	David S. Miller, greybus-dev, netdev, ACPI Devel Maling List,
	Linux PCI, Linux PM, linux-s390, open list:TARGET SUBSYSTEM,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

On Tue 2020-05-26 10:37:36, Rafael J. Wysocki wrote:
> On Mon, May 25, 2020 at 8:26 PM Krzysztof Wilczyński <kw@linux.com> wrote:
> >
> > Use the new device_to_pm() helper to access Power Management callbacs
> > (struct dev_pm_ops) for a particular device (struct device_driver).
> >
> > No functional change intended.
> >
> > Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> > ---
> >  drivers/acpi/device_pm.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > index 5832bc10aca8..b98a32c48fbe 100644
> > --- a/drivers/acpi/device_pm.c
> > +++ b/drivers/acpi/device_pm.c
> > @@ -1022,9 +1022,10 @@ static bool acpi_dev_needs_resume(struct device *dev, struct acpi_device *adev)
> >  int acpi_subsys_prepare(struct device *dev)
> >  {
> >         struct acpi_device *adev = ACPI_COMPANION(dev);
> > +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
> 
> I don't really see a reason for this change.
> 
> What's wrong with the check below?

Duplicated code. Yes, compiler can sort it out, but... new version
looks better to me.

Best regards,
								pavel

> >
> > -       if (dev->driver && dev->driver->pm && dev->driver->pm->prepare) {
> > -               int ret = dev->driver->pm->prepare(dev);
> > +       if (pm && pm->prepare) {
> > +               int ret = pm->prepare(dev);



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/8] ACPI: PM: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-26  9:45     ` Pavel Machek
@ 2020-05-26 10:35       ` Rafael J. Wysocki
  0 siblings, 0 replies; 30+ messages in thread
From: Rafael J. Wysocki @ 2020-05-26 10:35 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Rafael J. Wysocki, Krzysztof Wilczyński, Dan Carpenter,
	Rafael J. Wysocki, Len Brown, Kevin Hilman, Ulf Hansson,
	Greg Kroah-Hartman, Johan Hovold, Alex Elder, Bjorn Helgaas,
	James E.J. Bottomley, Martin K. Petersen, Felipe Balbi,
	Julian Wiedmann, Karsten Graul, Ursula Braun, Jakub Kicinski,
	Bjorn Andersson, John Stultz, David S. Miller, greybus-dev,
	netdev, ACPI Devel Maling List, Linux PCI, Linux PM, linux-s390,
	open list:TARGET SUBSYSTEM,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

On Tue, May 26, 2020 at 11:45 AM Pavel Machek <pavel@denx.de> wrote:
>
> On Tue 2020-05-26 10:37:36, Rafael J. Wysocki wrote:
> > On Mon, May 25, 2020 at 8:26 PM Krzysztof Wilczyński <kw@linux.com> wrote:
> > >
> > > Use the new device_to_pm() helper to access Power Management callbacs
> > > (struct dev_pm_ops) for a particular device (struct device_driver).
> > >
> > > No functional change intended.
> > >
> > > Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> > > ---
> > >  drivers/acpi/device_pm.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> > > index 5832bc10aca8..b98a32c48fbe 100644
> > > --- a/drivers/acpi/device_pm.c
> > > +++ b/drivers/acpi/device_pm.c
> > > @@ -1022,9 +1022,10 @@ static bool acpi_dev_needs_resume(struct device *dev, struct acpi_device *adev)
> > >  int acpi_subsys_prepare(struct device *dev)
> > >  {
> > >         struct acpi_device *adev = ACPI_COMPANION(dev);
> > > +       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
> >
> > I don't really see a reason for this change.
> >
> > What's wrong with the check below?
>
> Duplicated code. Yes, compiler can sort it out, but... new version
> looks better to me.

So the new code would not be duplicated?

Look at the other patches in the series then. :-)

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [greybus-dev] [PATCH 1/8] driver core: Add helper for accessing Power Management callbacs
  2020-05-26  6:33   ` Greg Kroah-Hartman
@ 2020-05-26 11:53     ` Alex Elder
  2020-05-26 15:01       ` Krzysztof Wilczyński
  0 siblings, 1 reply; 30+ messages in thread
From: Alex Elder @ 2020-05-26 11:53 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Krzysztof Wilczyński
  Cc: Ulf Hansson, linux-pci, Bjorn Andersson, Pavel Machek,
	linux-s390, linux-scsi, Kevin Hilman, Julian Wiedmann,
	linux-acpi, Jakub Kicinski, Dan Carpenter, Len Brown, linux-pm,
	James E.J. Bottomley, Ursula Braun, Johan Hovold, greybus-dev,
	John Stultz, Bjorn Helgaas, Felipe Balbi, Alex Elder,
	Martin K. Petersen, netdev, linux-usb, Rafael J. Wysocki,
	Karsten Graul, David S. Miller

On 5/26/20 1:33 AM, Greg Kroah-Hartman wrote:
> On Mon, May 25, 2020 at 06:26:01PM +0000, Krzysztof Wilczyński wrote:
>> Add driver_to_pm() helper allowing for accessing the Power Management
>> callbacs for a particular device.  Access to the callbacs (struct
>> dev_pm_ops) is normally done through using the pm pointer that is
>> embedded within the device_driver struct.
>>
>> Helper allows for the code required to reference the pm pointer and
>> access Power Management callbas to be simplified.  Changing the
>> following:
>>
>>    struct device_driver *drv = dev->driver;
>>    if (dev->driver && dev->driver->pm && dev->driver->pm->prepare) {
>>        int ret = dev->driver->pm->prepare(dev);
>>
>> To:
>>
>>    const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>>    if (pm && pm->prepare) {
>>        int ret = pm->prepare(dev);
>>
>> Or, changing the following:
>>
>>       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>>
>> To:
>>       const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>>
>> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
>> ---
>>   include/linux/device/driver.h | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
>> index ee7ba5b5417e..ccd0b315fd93 100644
>> --- a/include/linux/device/driver.h
>> +++ b/include/linux/device/driver.h
>> @@ -236,6 +236,21 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev)
>>   }
>>   #endif
>>   
>> +/**
>> + * driver_to_pm - Return Power Management callbacs (struct dev_pm_ops) for
>> + *                a particular device.
>> + * @drv: Pointer to a device (struct device_driver) for which you want to access
>> + *       the Power Management callbacks.
>> + *
>> + * Returns a pointer to the struct dev_pm_ops embedded within the device (struct
>> + * device_driver), or returns NULL if Power Management is not present and the
>> + * pointer is not valid.
>> + */
>> +static inline const struct dev_pm_ops *driver_to_pm(struct device_driver *drv)
>> +{
>> +	return drv && drv->pm ? drv->pm : NULL;

This could just be:

	if (drv)
		return drv->pm;

	return NULL;

Or if you want to evoke passion in Greg:

	return drv ? drv->pm : NULL;

					-Alex

> I hate ? : lines with a passion, as they break normal pattern mattching
> in my brain.  Please just spell this all out:
> 	if (drv && drv->pm)
> 		return drv->pm;
> 	return NULL;
> 
> Much easier to read, and the compiler will do the exact same thing.
> 
> Only place ? : are ok to use in my opinion, are as function arguments.
> 
> thanks,
> 
> greg k-h
> _______________________________________________
> greybus-dev mailing list
> greybus-dev@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [greybus-dev] [PATCH 3/8] greybus: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-25 18:26 ` [PATCH 3/8] greybus: " Krzysztof Wilczyński
@ 2020-05-26 11:53   ` Alex Elder
  0 siblings, 0 replies; 30+ messages in thread
From: Alex Elder @ 2020-05-26 11:53 UTC (permalink / raw)
  To: Krzysztof Wilczyński, Dan Carpenter
  Cc: Ulf Hansson, linux-pci, Bjorn Andersson, Pavel Machek,
	linux-s390, linux-scsi, Kevin Hilman, Julian Wiedmann,
	linux-acpi, Jakub Kicinski, Len Brown, linux-pm,
	James E.J. Bottomley, Ursula Braun, Johan Hovold, greybus-dev,
	John Stultz, Bjorn Helgaas, Felipe Balbi, Alex Elder,
	Martin K. Petersen, linux-usb, Rafael J. Wysocki, Karsten Graul,
	netdev, David S. Miller

On 5/25/20 1:26 PM, Krzysztof Wilczyński wrote:
> Use the new device_to_pm() helper to access Power Management callbacs
> (struct dev_pm_ops) for a particular device (struct device_driver).
> 
> No functional change intended.

Looks fine to me.

Reviewed-by: Alex Elder <elder@linaro.org>

> Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> ---
>   drivers/greybus/bundle.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/greybus/bundle.c b/drivers/greybus/bundle.c
> index 84660729538b..d38d3a630812 100644
> --- a/drivers/greybus/bundle.c
> +++ b/drivers/greybus/bundle.c
> @@ -108,7 +108,7 @@ static void gb_bundle_enable_all_connections(struct gb_bundle *bundle)
>   static int gb_bundle_suspend(struct device *dev)
>   {
>   	struct gb_bundle *bundle = to_gb_bundle(dev);
> -	const struct dev_pm_ops *pm = dev->driver->pm;
> +	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>   	int ret;
>   
>   	if (pm && pm->runtime_suspend) {
> @@ -135,7 +135,7 @@ static int gb_bundle_suspend(struct device *dev)
>   static int gb_bundle_resume(struct device *dev)
>   {
>   	struct gb_bundle *bundle = to_gb_bundle(dev);
> -	const struct dev_pm_ops *pm = dev->driver->pm;
> +	const struct dev_pm_ops *pm = driver_to_pm(dev->driver);
>   	int ret;
>   
>   	ret = gb_control_bundle_resume(bundle->intf->control, bundle->id);
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 8/8] net/iucv: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-26  7:07   ` Ursula Braun
@ 2020-05-26 14:57     ` Krzysztof Wilczyński
  0 siblings, 0 replies; 30+ messages in thread
From: Krzysztof Wilczyński @ 2020-05-26 14:57 UTC (permalink / raw)
  To: Ursula Braun
  Cc: Dan Carpenter, Rafael J. Wysocki, Len Brown, Kevin Hilman,
	Ulf Hansson, Pavel Machek, Greg Kroah-Hartman, Johan Hovold,
	Alex Elder, Bjorn Helgaas, James E.J. Bottomley,
	Martin K. Petersen, Felipe Balbi, Julian Wiedmann, Karsten Graul,
	Jakub Kicinski, Bjorn Andersson, John Stultz, David S. Miller,
	greybus-dev, netdev, linux-acpi, linux-pci, linux-pm, linux-s390,
	linux-scsi, linux-usb

Hi Ursula,

On 20-05-26 09:07:27, Ursula Braun wrote:
> 
> 
> On 5/25/20 8:26 PM, Krzysztof Wilczyński wrote:
> > Use the new device_to_pm() helper to access Power Management callbacs
> > (struct dev_pm_ops) for a particular device (struct device_driver).
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Krzysztof Wilczyński <kw@linux.com>
> 
> pm support is going to be removed (for s390 in general and) for
> net/iucv/iucv.c with this net-next patch:
[...]

Good to know!  Thank you for letting me know.  I appreciate that.

Krzysztof

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [greybus-dev] [PATCH 1/8] driver core: Add helper for accessing Power Management callbacs
  2020-05-26 11:53     ` [greybus-dev] " Alex Elder
@ 2020-05-26 15:01       ` Krzysztof Wilczyński
  0 siblings, 0 replies; 30+ messages in thread
From: Krzysztof Wilczyński @ 2020-05-26 15:01 UTC (permalink / raw)
  To: Alex Elder
  Cc: Greg Kroah-Hartman, Ulf Hansson, linux-pci, Bjorn Andersson,
	Pavel Machek, linux-s390, linux-scsi, Kevin Hilman,
	Julian Wiedmann, linux-acpi, Jakub Kicinski, Dan Carpenter,
	Len Brown, linux-pm, James E.J. Bottomley, Ursula Braun,
	Johan Hovold, greybus-dev, John Stultz, Bjorn Helgaas,
	Felipe Balbi, Alex Elder, Martin K. Petersen, netdev, linux-usb,
	Rafael J. Wysocki, Karsten Graul, David S. Miller

Hello Alex and Greg,

[...]
> This could just be:
> 
> 	if (drv)
> 		return drv->pm;
> 
> 	return NULL;
> 
> Or if you want to evoke passion in Greg:
> 
> 	return drv ? drv->pm : NULL;
> 
> 					-Alex
> 
> > I hate ? : lines with a passion, as they break normal pattern mattching
> > in my brain.  Please just spell this all out:
> > 	if (drv && drv->pm)
> > 		return drv->pm;
> > 	return NULL;
> > 
> > Much easier to read, and the compiler will do the exact same thing.
> > 
> > Only place ? : are ok to use in my opinion, are as function arguments.

I will steer away from the ternary operator next time.  Also, good to
learn about Greg's preference.

Thank you both!

Krzysztof

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 8/8] net/iucv: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-26  6:35   ` Greg Kroah-Hartman
@ 2020-05-26 15:07     ` Krzysztof Wilczyński
  2020-05-26 15:19         ` Rafael J. Wysocki
  2020-05-26 16:48       ` [greybus-dev] " Alex Elder
  0 siblings, 2 replies; 30+ messages in thread
From: Krzysztof Wilczyński @ 2020-05-26 15:07 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Dan Carpenter, Rafael J. Wysocki, Len Brown, Kevin Hilman,
	Ulf Hansson, Pavel Machek, Johan Hovold, Alex Elder,
	Bjorn Helgaas, James E.J. Bottomley, Martin K. Petersen,
	Felipe Balbi, Julian Wiedmann, Karsten Graul, Ursula Braun,
	Jakub Kicinski, Bjorn Andersson, John Stultz, David S. Miller,
	greybus-dev, netdev, linux-acpi, linux-pci, linux-pm, linux-s390,
	linux-scsi, linux-usb

Hello Greg,

[...]
> It's "interesting" how using your new helper doesn't actually make the
> code smaller.  Perhaps it isn't a good helper function?

The idea for the helper was inspired by the comment Dan made to Bjorn
about Bjorn's change, as per:

  https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/

It looked like a good idea to try to reduce the following:

  dev->driver && dev->driver->pm && dev->driver->pm->prepare

Into something more succinct.  Albeit, given the feedback from yourself
and Rafael, I gather that this helper is not really a good addition.

Thank you everyone and sorry for the commotion!

Krzysztof

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 8/8] net/iucv: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-26 15:07     ` Krzysztof Wilczyński
@ 2020-05-26 15:19         ` Rafael J. Wysocki
  2020-05-26 16:48       ` [greybus-dev] " Alex Elder
  1 sibling, 0 replies; 30+ messages in thread
From: Rafael J. Wysocki @ 2020-05-26 15:19 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Greg Kroah-Hartman, Dan Carpenter, Rafael J. Wysocki, Len Brown,
	Kevin Hilman, Ulf Hansson, Pavel Machek, Johan Hovold,
	Alex Elder, Bjorn Helgaas, James E.J. Bottomley,
	Martin K. Petersen, Felipe Balbi, Julian Wiedmann, Karsten Graul,
	Ursula Braun, Jakub Kicinski, Bjorn Andersson, John Stultz,
	David S. Miller, greybus-dev, netdev, ACPI Devel Maling List,
	Linux PCI, Linux PM, linux-s390, open list:TARGET SUBSYSTEM,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

On Tue, May 26, 2020 at 5:07 PM Krzysztof Wilczyński <kw@linux.com> wrote:
>
> Hello Greg,
>
> [...]
> > It's "interesting" how using your new helper doesn't actually make the
> > code smaller.  Perhaps it isn't a good helper function?
>
> The idea for the helper was inspired by the comment Dan made to Bjorn
> about Bjorn's change, as per:
>
>   https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/
>
> It looked like a good idea to try to reduce the following:
>
>   dev->driver && dev->driver->pm && dev->driver->pm->prepare
>
> Into something more succinct.  Albeit, given the feedback from yourself
> and Rafael, I gather that this helper is not really a good addition.

IMO it could be used for reducing code duplication like you did in the
PCI code, but not necessarily in the other places where the code in
question is not exactly duplicated.

Thanks!

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 8/8] net/iucv: Use the new device_to_pm() helper to access struct dev_pm_ops
@ 2020-05-26 15:19         ` Rafael J. Wysocki
  0 siblings, 0 replies; 30+ messages in thread
From: Rafael J. Wysocki @ 2020-05-26 15:19 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Greg Kroah-Hartman, Dan Carpenter, Rafael J. Wysocki, Len Brown,
	Kevin Hilman, Ulf Hansson, Pavel Machek, Johan Hovold,
	Alex Elder, Bjorn Helgaas, James E.J. Bottomley,
	Martin K. Petersen, Felipe Balbi, Julian Wiedmann, Karsten Graul,
	Ursula Braun, Jakub Kicinski, Bjorn Andersson, John Stultz,
	David S. Miller, greybus-dev, netdev, ACPI Devel Maling List,
	Linux PCI, Linux PM, linux-s390, open list:TARGET SUBSYSTEM,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

On Tue, May 26, 2020 at 5:07 PM Krzysztof Wilczyński <kw@linux.com> wrote:
>
> Hello Greg,
>
> [...]
> > It's "interesting" how using your new helper doesn't actually make the
> > code smaller.  Perhaps it isn't a good helper function?
>
> The idea for the helper was inspired by the comment Dan made to Bjorn
> about Bjorn's change, as per:
>
>   https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/
>
> It looked like a good idea to try to reduce the following:
>
>   dev->driver && dev->driver->pm && dev->driver->pm->prepare
>
> Into something more succinct.  Albeit, given the feedback from yourself
> and Rafael, I gather that this helper is not really a good addition.

IMO it could be used for reducing code duplication like you did in the
PCI code, but not necessarily in the other places where the code in
question is not exactly duplicated.

Thanks!

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 8/8] net/iucv: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-26 15:19         ` Rafael J. Wysocki
  (?)
@ 2020-05-26 15:28         ` Alan Stern
  2020-05-26 16:06           ` Rafael J. Wysocki
  -1 siblings, 1 reply; 30+ messages in thread
From: Alan Stern @ 2020-05-26 15:28 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Krzysztof Wilczyński, Greg Kroah-Hartman, Dan Carpenter,
	Rafael J. Wysocki, Len Brown, Kevin Hilman, Ulf Hansson,
	Pavel Machek, Johan Hovold, Alex Elder, Bjorn Helgaas,
	James E.J. Bottomley, Martin K. Petersen, Felipe Balbi,
	Julian Wiedmann, Karsten Graul, Ursula Braun, Jakub Kicinski,
	Bjorn Andersson, John Stultz, David S. Miller, greybus-dev,
	netdev, ACPI Devel Maling List, Linux PCI, Linux PM, linux-s390,
	open list:TARGET SUBSYSTEM,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

On Tue, May 26, 2020 at 05:19:07PM +0200, Rafael J. Wysocki wrote:
> On Tue, May 26, 2020 at 5:07 PM Krzysztof Wilczyński <kw@linux.com> wrote:
> >
> > Hello Greg,
> >
> > [...]
> > > It's "interesting" how using your new helper doesn't actually make the
> > > code smaller.  Perhaps it isn't a good helper function?
> >
> > The idea for the helper was inspired by the comment Dan made to Bjorn
> > about Bjorn's change, as per:
> >
> >   https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/
> >
> > It looked like a good idea to try to reduce the following:
> >
> >   dev->driver && dev->driver->pm && dev->driver->pm->prepare
> >
> > Into something more succinct.  Albeit, given the feedback from yourself
> > and Rafael, I gather that this helper is not really a good addition.
> 
> IMO it could be used for reducing code duplication like you did in the
> PCI code, but not necessarily in the other places where the code in
> question is not exactly duplicated.

The code could be a little more succinct, although it wouldn't fit every 
usage.  For example,

#define pm_do_callback(dev, method) \
	(dev->driver && dev->driver->pm && dev->driver->pm->callback ? \
	dev->driver->pm->callback(dev) : 0)

Then the usage is something like:

	ret = pm_do_callback(dev, prepare);

Would this be an overall improvement?

Alan Stern

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 8/8] net/iucv: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-26 15:28         ` Alan Stern
@ 2020-05-26 16:06           ` Rafael J. Wysocki
  0 siblings, 0 replies; 30+ messages in thread
From: Rafael J. Wysocki @ 2020-05-26 16:06 UTC (permalink / raw)
  To: Alan Stern
  Cc: Rafael J. Wysocki, Krzysztof Wilczyński, Greg Kroah-Hartman,
	Dan Carpenter, Rafael J. Wysocki, Len Brown, Kevin Hilman,
	Ulf Hansson, Pavel Machek, Johan Hovold, Alex Elder,
	Bjorn Helgaas, James E.J. Bottomley, Martin K. Petersen,
	Felipe Balbi, Julian Wiedmann, Karsten Graul, Ursula Braun,
	Jakub Kicinski, Bjorn Andersson, John Stultz, David S. Miller,
	greybus-dev, netdev, ACPI Devel Maling List, Linux PCI, Linux PM,
	linux-s390, open list:TARGET SUBSYSTEM,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:

On Tue, May 26, 2020 at 5:28 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Tue, May 26, 2020 at 05:19:07PM +0200, Rafael J. Wysocki wrote:
> > On Tue, May 26, 2020 at 5:07 PM Krzysztof Wilczyński <kw@linux.com> wrote:
> > >
> > > Hello Greg,
> > >
> > > [...]
> > > > It's "interesting" how using your new helper doesn't actually make the
> > > > code smaller.  Perhaps it isn't a good helper function?
> > >
> > > The idea for the helper was inspired by the comment Dan made to Bjorn
> > > about Bjorn's change, as per:
> > >
> > >   https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/
> > >
> > > It looked like a good idea to try to reduce the following:
> > >
> > >   dev->driver && dev->driver->pm && dev->driver->pm->prepare
> > >
> > > Into something more succinct.  Albeit, given the feedback from yourself
> > > and Rafael, I gather that this helper is not really a good addition.
> >
> > IMO it could be used for reducing code duplication like you did in the
> > PCI code, but not necessarily in the other places where the code in
> > question is not exactly duplicated.
>
> The code could be a little more succinct, although it wouldn't fit every
> usage.  For example,
>
> #define pm_do_callback(dev, method) \
>         (dev->driver && dev->driver->pm && dev->driver->pm->callback ? \
>         dev->driver->pm->callback(dev) : 0)
>
> Then the usage is something like:
>
>         ret = pm_do_callback(dev, prepare);
>
> Would this be an overall improvement?

It wouldn't cover all of the use cases.

For example, PCI does other things in addition to running a callback
when it is present.

Something like this might be enough though:

#define pm_driver_callback_is_present(dev, method) \
        (dev->driver && dev->driver->pm && dev->driver->pm->method)

#define pm_run_driver_callback(dev, method) \
        (pm_driver_callback_is_present(dev, method) ?
dev->driver->pm->method(dev) : 0)

#define pm_get_driver_callback(dev, method) \
        (pm_driver_callback_is_present(dev, method) ?
dev->driver->pm->method : NULL)

so whoever needs the callback pointer can use pm_get_driver_callback()
and whoever only needs to run the callback can use
pm_run_driver_callback().

Cheers!

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [greybus-dev] [PATCH 8/8] net/iucv: Use the new device_to_pm() helper to access struct dev_pm_ops
  2020-05-26 15:07     ` Krzysztof Wilczyński
  2020-05-26 15:19         ` Rafael J. Wysocki
@ 2020-05-26 16:48       ` Alex Elder
  1 sibling, 0 replies; 30+ messages in thread
From: Alex Elder @ 2020-05-26 16:48 UTC (permalink / raw)
  To: Krzysztof Wilczyński, Greg Kroah-Hartman
  Cc: Ulf Hansson, linux-pci, Bjorn Andersson, Pavel Machek,
	linux-s390, linux-scsi, Kevin Hilman, Julian Wiedmann,
	linux-acpi, Jakub Kicinski, Dan Carpenter, Len Brown, linux-pm,
	James E.J. Bottomley, Ursula Braun, Johan Hovold, greybus-dev,
	John Stultz, Bjorn Helgaas, Felipe Balbi, Alex Elder,
	Martin K. Petersen, netdev, linux-usb, Rafael J. Wysocki,
	Karsten Graul, David S. Miller

On 5/26/20 10:07 AM, Krzysztof Wilczyński wrote:
> Hello Greg,
> 
> [...]
>> It's "interesting" how using your new helper doesn't actually make the
>> code smaller.  Perhaps it isn't a good helper function?

Helper functions often improve code readability, which is
beneficial even if it doesn't reduce code size or efficiency.

But I won't argue for or against this particular change.
It's OK with me either way.

					-Alex

> The idea for the helper was inspired by the comment Dan made to Bjorn
> about Bjorn's change, as per:
> 
>    https://lore.kernel.org/driverdev-devel/20191016135002.GA24678@kadam/
> 
> It looked like a good idea to try to reduce the following:
> 
>    dev->driver && dev->driver->pm && dev->driver->pm->prepare
> 
> Into something more succinct.  Albeit, given the feedback from yourself
> and Rafael, I gather that this helper is not really a good addition.
> 
> Thank you everyone and sorry for the commotion!
> 
> Krzysztof
> _______________________________________________
> greybus-dev mailing list
> greybus-dev@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
> 


^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2020-05-26 16:48 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-25 18:26 [PATCH 0/8] Add helper for accessing Power Management callbacs Krzysztof Wilczyński
2020-05-25 18:26 ` [PATCH 1/8] driver core: " Krzysztof Wilczyński
2020-05-26  6:33   ` Greg Kroah-Hartman
2020-05-26 11:53     ` [greybus-dev] " Alex Elder
2020-05-26 15:01       ` Krzysztof Wilczyński
2020-05-25 18:26 ` [PATCH 2/8] ACPI: PM: Use the new device_to_pm() helper to access struct dev_pm_ops Krzysztof Wilczyński
2020-05-26  8:37   ` Rafael J. Wysocki
2020-05-26  8:37     ` Rafael J. Wysocki
2020-05-26  9:45     ` Pavel Machek
2020-05-26 10:35       ` Rafael J. Wysocki
2020-05-25 18:26 ` [PATCH 3/8] greybus: " Krzysztof Wilczyński
2020-05-26 11:53   ` [greybus-dev] " Alex Elder
2020-05-25 18:26 ` [PATCH 4/8] scsi: pm: " Krzysztof Wilczyński
2020-05-25 18:26 ` [PATCH 5/8] usb: phy: fsl: " Krzysztof Wilczyński
2020-05-26  8:38   ` Rafael J. Wysocki
2020-05-26  8:38     ` Rafael J. Wysocki
2020-05-25 18:26 ` [PATCH 6/8] PCI/PM: " Krzysztof Wilczyński
2020-05-25 18:26 ` [PATCH 7/8] PM: " Krzysztof Wilczyński
2020-05-26  8:33   ` Rafael J. Wysocki
2020-05-26  8:33     ` Rafael J. Wysocki
2020-05-25 18:26 ` [PATCH 8/8] net/iucv: " Krzysztof Wilczyński
2020-05-26  6:35   ` Greg Kroah-Hartman
2020-05-26 15:07     ` Krzysztof Wilczyński
2020-05-26 15:19       ` Rafael J. Wysocki
2020-05-26 15:19         ` Rafael J. Wysocki
2020-05-26 15:28         ` Alan Stern
2020-05-26 16:06           ` Rafael J. Wysocki
2020-05-26 16:48       ` [greybus-dev] " Alex Elder
2020-05-26  7:07   ` Ursula Braun
2020-05-26 14:57     ` Krzysztof Wilczyński

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.