diff mbox series

PM / runtime: Fixup reference counting of device link suppliers at probe

Message ID 1526633329-31729-1-git-send-email-ulf.hansson@linaro.org
State Accepted
Commit 1e8378619841ef1d621b130bbd3fc3b7e6739b50
Headers show
Series PM / runtime: Fixup reference counting of device link suppliers at probe | expand

Commit Message

Ulf Hansson May 18, 2018, 8:48 a.m. UTC 
In the driver core, before it invokes really_probe() it runtime resumes the
suppliers for the device via calling pm_runtime_get_suppliers(), which also
increases the runtime PM usage count for each of the available supplier.

This makes sense, as to be able to allow the consumer device to be probed
by its driver. However, if the driver decides to add a new supplier link
during ->probe(), hence updating the list of suppliers, the following call
to pm_runtime_put_suppliers(), invoked after really_probe() in the driver
core, we get into trouble.

More precisely, pm_runtime_put() gets called also for the new supplier(s),
which is wrong as the driver core, didn't trigger pm_runtime_get_sync() to
be called for it in the first place. In other words, the new supplier may
be runtime suspended even in cases when it shouldn't.

Fix this behaviour, by runtime resume suppliers according to the same
conditions as managed by the runtime PM core, when runtime resume callbacks
are being invoked.

Additionally, don't try to runtime suspend any of the suppliers after
really_probe(), but instead rely on that to happen via the consumer device,
when it becomes runtime suspended.

Fixes: 21d5c57b3726 ("PM / runtime: Use device links")
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

---

An optional solution (requires to add code, rather than remove), in case we
think it's not suffient to rely on the consumer device to be runtime suspended
after probe.

This could be done via adding a new bool member to the struct device_link, and
set it from pm_runtime_get_suppliers(). Check it at pm_runtime_put_suppliers(),
and only call pm_runtime_put() on those suppliers that has it set.

---
 drivers/base/dd.c            |  3 +--
 drivers/base/power/runtime.c | 27 +++------------------------
 include/linux/pm_runtime.h   |  6 ++----
 3 files changed, 6 insertions(+), 30 deletions(-)

-- 
2.7.4

Comments

Rafael J. Wysocki May 29, 2018, 8:59 a.m. UTC | #1 
On Friday, May 18, 2018 11:07:02 AM CEST Ulf Hansson wrote:
> On 18 May 2018 at 10:58, Rafael J. Wysocki <rafael@kernel.org> wrote:

> > On Fri, May 18, 2018 at 10:48 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

> >> In the driver core, before it invokes really_probe() it runtime resumes the

> >> suppliers for the device via calling pm_runtime_get_suppliers(), which also

> >> increases the runtime PM usage count for each of the available supplier.

> >>

> >> This makes sense, as to be able to allow the consumer device to be probed

> >> by its driver. However, if the driver decides to add a new supplier link

> >> during ->probe(), hence updating the list of suppliers,

> >

> > Do any of the existing drivers do that?

> 

> Yes.

> 

> At least these, but possibly even more...

> 

> drivers/gpu/drm/rockchip/rockchip_drm_drv.c

> drivers/gpu/drm/tegra/dc.c

> drivers/gpu/ipu-v3/ipu-prg.c

> drivers/pci/dwc/pci-dra7xx.c

> 

> >

> >> the following call to pm_runtime_put_suppliers(), invoked after really_probe()

> >> in the driver core, we get into trouble.


Patch applied, thanks!
diff mbox series

Patch

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 10454fe..a41c91b 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -580,7 +580,7 @@  int driver_probe_device(struct device_driver *drv, struct device *dev)
 	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
 		 drv->bus->name, __func__, dev_name(dev), drv->name);
 
-	pm_runtime_get_suppliers(dev);
+	pm_runtime_resume_suppliers(dev);
 	if (dev->parent)
 		pm_runtime_get_sync(dev->parent);
 
@@ -591,7 +591,6 @@  int driver_probe_device(struct device_driver *drv, struct device *dev)
 	if (dev->parent)
 		pm_runtime_put(dev->parent);
 
-	pm_runtime_put_suppliers(dev);
 	return ret;
 }
 
diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 8bef3cb..6f4f50e 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -1563,37 +1563,16 @@  void pm_runtime_clean_up_links(struct device *dev)
 }
 
 /**
- * pm_runtime_get_suppliers - Resume and reference-count supplier devices.
+ * pm_runtime_resume_suppliers - Resume supplier devices.
  * @dev: Consumer device.
  */
-void pm_runtime_get_suppliers(struct device *dev)
+void pm_runtime_resume_suppliers(struct device *dev)
 {
-	struct device_link *link;
 	int idx;
 
 	idx = device_links_read_lock();
 
-	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
-		if (link->flags & DL_FLAG_PM_RUNTIME)
-			pm_runtime_get_sync(link->supplier);
-
-	device_links_read_unlock(idx);
-}
-
-/**
- * pm_runtime_put_suppliers - Drop references to supplier devices.
- * @dev: Consumer device.
- */
-void pm_runtime_put_suppliers(struct device *dev)
-{
-	struct device_link *link;
-	int idx;
-
-	idx = device_links_read_lock();
-
-	list_for_each_entry_rcu(link, &dev->links.suppliers, c_node)
-		if (link->flags & DL_FLAG_PM_RUNTIME)
-			pm_runtime_put(link->supplier);
+	rpm_get_suppliers(dev);
 
 	device_links_read_unlock(idx);
 }
diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
index f0fc470..db5dbbf 100644
--- a/include/linux/pm_runtime.h
+++ b/include/linux/pm_runtime.h
@@ -56,8 +56,7 @@  extern void pm_runtime_update_max_time_suspended(struct device *dev,
 						 s64 delta_ns);
 extern void pm_runtime_set_memalloc_noio(struct device *dev, bool enable);
 extern void pm_runtime_clean_up_links(struct device *dev);
-extern void pm_runtime_get_suppliers(struct device *dev);
-extern void pm_runtime_put_suppliers(struct device *dev);
+extern void pm_runtime_resume_suppliers(struct device *dev);
 extern void pm_runtime_new_link(struct device *dev);
 extern void pm_runtime_drop_link(struct device *dev);
 
@@ -173,8 +172,7 @@  static inline unsigned long pm_runtime_autosuspend_expiration(
 static inline void pm_runtime_set_memalloc_noio(struct device *dev,
 						bool enable){}
 static inline void pm_runtime_clean_up_links(struct device *dev) {}
-static inline void pm_runtime_get_suppliers(struct device *dev) {}
-static inline void pm_runtime_put_suppliers(struct device *dev) {}
+static inline void pm_runtime_resume_suppliers(struct device *dev) {}
 static inline void pm_runtime_new_link(struct device *dev) {}
 static inline void pm_runtime_drop_link(struct device *dev) {}