[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

Changes in v3:
	-Rework the entire intermediate step which was suggested in v2.
	 That means solving the race condition, but also cope with PM domains
	 that are initialized in powered off state.

Changes in v2:
	-Added some acks.
	-Updated commit messages.
	-Included a wider audience of the patchset. V1 was lacking SoC
	 maintainers.

Here are link to the first patchset, were some discussion started.
http://marc.info/?l=linux-pm&m=141208104729597&w=2

There may be more than one device in a PM domain which then will be
probed at different points in time.

Depending on timing and runtime PM support, in for the device related
driver/subsystem, a PM domain may be advised to power off after a
successful probe sequence.

A general requirement for a device within a PM domain, is that the
PM domain must stay powered during the probe sequence. To cope with
such requirement, let's add two new APIs, dev_pm_domain_get|put().

These APIs are intended to be invoked from subsystem-level code and the
calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a
usage count and to keep supplying power. dev_pm_domain_put(), does the
opposite.


Ulf Hansson (9):
  PM / Domains: Add dev_pm_domain_get|put() APIs
  PM / Domains: Enable genpd to support ->get|put() callbacks
  amba: Keep PM domain powered during ->probe()
  drivercore / platform: Keep PM domain powered during ->probe()
  i2c: core: Keep PM domain powered during ->probe()
  spi: core: Keep PM domain powered during ->probe()
  mmc: core: Attach PM domain prior probing of SDIO func driver
  mmmc: core: Keep PM domain powered during ->probe() of SDIO func
    driver
  PM / Domains: Remove pm_genpd_dev_need_restore() API

 drivers/amba/bus.c          |  8 ++++++++
 drivers/base/platform.c     |  7 +++++++
 drivers/base/power/common.c | 40 +++++++++++++++++++++++++++++++++++++++
 drivers/base/power/domain.c | 46 ++++++++++++++++++++++++---------------------
 drivers/i2c/i2c-core.c      |  7 +++++++
 drivers/mmc/core/sdio_bus.c | 21 +++++++++++++++++----
 drivers/spi/spi.c           |  7 +++++++
 include/linux/pm.h          |  2 ++
 include/linux/pm_domain.h   |  7 +++++--
 9 files changed, 118 insertions(+), 27 deletions(-)

-- 
1.9.1

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

Changes in v3:
	-Rework the entire intermediate step which was suggested in v2.
	 That means solving the race condition, but also cope with PM domains
	 that are initialized in powered off state.

Changes in v2:
	-Added some acks.
	-Updated commit messages.
	-Included a wider audience of the patchset. V1 was lacking SoC
	 maintainers.

Here are link to the first patchset, were some discussion started.
http://marc.info/?l=linux-pm&m=141208104729597&w=2

There may be more than one device in a PM domain which then will be
probed at different points in time.

Depending on timing and runtime PM support, in for the device related
driver/subsystem, a PM domain may be advised to power off after a
successful probe sequence.

A general requirement for a device within a PM domain, is that the
PM domain must stay powered during the probe sequence. To cope with
such requirement, let's add two new APIs, dev_pm_domain_get|put().

These APIs are intended to be invoked from subsystem-level code and the
calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a
usage count and to keep supplying power. dev_pm_domain_put(), does the
opposite.


Ulf Hansson (9):
  PM / Domains: Add dev_pm_domain_get|put() APIs
  PM / Domains: Enable genpd to support ->get|put() callbacks
  amba: Keep PM domain powered during ->probe()
  drivercore / platform: Keep PM domain powered during ->probe()
  i2c: core: Keep PM domain powered during ->probe()
  spi: core: Keep PM domain powered during ->probe()
  mmc: core: Attach PM domain prior probing of SDIO func driver
  mmmc: core: Keep PM domain powered during ->probe() of SDIO func
    driver
  PM / Domains: Remove pm_genpd_dev_need_restore() API

 drivers/amba/bus.c          |  8 ++++++++
 drivers/base/platform.c     |  7 +++++++
 drivers/base/power/common.c | 40 +++++++++++++++++++++++++++++++++++++++
 drivers/base/power/domain.c | 46 ++++++++++++++++++++++++---------------------
 drivers/i2c/i2c-core.c      |  7 +++++++
 drivers/mmc/core/sdio_bus.c | 21 +++++++++++++++++----
 drivers/spi/spi.c           |  7 +++++++
 include/linux/pm.h          |  2 ++
 include/linux/pm_domain.h   |  7 +++++--
 9 files changed, 118 insertions(+), 27 deletions(-)

-- 
1.9.1

[PATCH v3 1/9] PM / Domains: Add dev_pm_domain_get|put() APIs

[Ulf Hansson]

There may be more than one device in a PM domain which then will be
probed at different points in time.

Depending on timing and runtime PM support, in for the device related
driver/subsystem, a PM domain may be advised to power off after a
successful probe sequence.

A general requirement for a device within a PM domain, is that the
PM domain must stay powered during the probe sequence. To cope with
such requirement, let's add the dev_pm_domain_get|put() APIs.

These APIs are intended to be invoked from subsystem-level code and the
calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a
usage count and to keep supplying power. dev_pm_domain_put(), does the
opposite.

For a PM domain to support this feature, it must implement the optional
->get|put() callbacks.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/common.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pm.h          |  2 ++
 include/linux/pm_domain.h   |  4 ++++
 3 files changed, 46 insertions(+)

diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
index b0f1388..b4f0c8c 100644
--- a/drivers/base/power/common.c
+++ b/drivers/base/power/common.c
@@ -134,3 +134,43 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
 		dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+/**
+ * dev_pm_domain_get - Increase usage count to keep a PM domain powered.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself increase the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code prior drivers starts probing.
+ *
+ * Do note, it's optional to implement the ->get() callback for a PM domain.
+ *
+ * Returns 0 on successfully increased usage count or negative error code.
+ */
+int dev_pm_domain_get(struct dev_pm_domain *domain)
+{
+	int ret = 0;
+
+	if (domain && domain->get)
+		ret = domain->get(domain);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_get);
+
+/**
+ * dev_pm_domain_put - Decrease usage count to allow a PM domain to power off.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself decrease the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code after drivers has finished probing.
+ *
+ * Do note, it's optional to implement the ->put() callback for a PM domain.
+ */
+void dev_pm_domain_put(struct dev_pm_domain *domain)
+{
+	if (domain && domain->put)
+		domain->put(domain);
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_put);
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 383fd68..f71900d 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -620,6 +620,8 @@ extern int dev_pm_put_subsys_data(struct device *dev);
 struct dev_pm_domain {
 	struct dev_pm_ops	ops;
 	void (*detach)(struct device *dev, bool power_off);
+	int (*get)(struct dev_pm_domain *domain);
+	void (*put)(struct dev_pm_domain *domain);
 };
 
 /*
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 73e938b..8e2806f 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -304,12 +304,16 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np,
 #ifdef CONFIG_PM
 extern int dev_pm_domain_attach(struct device *dev, bool power_on);
 extern void dev_pm_domain_detach(struct device *dev, bool power_off);
+extern int dev_pm_domain_get(struct dev_pm_domain *domain);
+extern void dev_pm_domain_put(struct dev_pm_domain *domain);
 #else
 static inline int dev_pm_domain_attach(struct device *dev, bool power_on)
 {
 	return -ENODEV;
 }
 static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {}
+static inline int dev_pm_domain_get(struct dev_pm_domain *domain) { return 0; }
+static inline void dev_pm_domain_put(struct dev_pm_domain *domain) {}
 #endif
 
 #endif /* _LINUX_PM_DOMAIN_H */
-- 
1.9.1

[PATCH v3 1/9] PM / Domains: Add dev_pm_domain_get|put() APIs

[Ulf Hansson]

There may be more than one device in a PM domain which then will be
probed at different points in time.

Depending on timing and runtime PM support, in for the device related
driver/subsystem, a PM domain may be advised to power off after a
successful probe sequence.

A general requirement for a device within a PM domain, is that the
PM domain must stay powered during the probe sequence. To cope with
such requirement, let's add the dev_pm_domain_get|put() APIs.

These APIs are intended to be invoked from subsystem-level code and the
calls between get/put needs to be balanced.

dev_pm_domain_get(), tells the PM domain that it needs to increase a
usage count and to keep supplying power. dev_pm_domain_put(), does the
opposite.

For a PM domain to support this feature, it must implement the optional
->get|put() callbacks.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/common.c | 40 ++++++++++++++++++++++++++++++++++++++++
 include/linux/pm.h          |  2 ++
 include/linux/pm_domain.h   |  4 ++++
 3 files changed, 46 insertions(+)

diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
index b0f1388..b4f0c8c 100644
--- a/drivers/base/power/common.c
+++ b/drivers/base/power/common.c
@@ -134,3 +134,43 @@ void dev_pm_domain_detach(struct device *dev, bool power_off)
 		dev->pm_domain->detach(dev, power_off);
 }
 EXPORT_SYMBOL_GPL(dev_pm_domain_detach);
+
+/**
+ * dev_pm_domain_get - Increase usage count to keep a PM domain powered.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself increase the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code prior drivers starts probing.
+ *
+ * Do note, it's optional to implement the ->get() callback for a PM domain.
+ *
+ * Returns 0 on successfully increased usage count or negative error code.
+ */
+int dev_pm_domain_get(struct dev_pm_domain *domain)
+{
+	int ret = 0;
+
+	if (domain && domain->get)
+		ret = domain->get(domain);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_get);
+
+/**
+ * dev_pm_domain_put - Decrease usage count to allow a PM domain to power off.
+ * @domain: The PM domain to operate on.
+ *
+ * This function will not by itself decrease the usage count, that's up to each
+ * PM domain implementation to support. Typically it should be invoked from
+ * subsystem level code after drivers has finished probing.
+ *
+ * Do note, it's optional to implement the ->put() callback for a PM domain.
+ */
+void dev_pm_domain_put(struct dev_pm_domain *domain)
+{
+	if (domain && domain->put)
+		domain->put(domain);
+}
+EXPORT_SYMBOL_GPL(dev_pm_domain_put);
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 383fd68..f71900d 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -620,6 +620,8 @@ extern int dev_pm_put_subsys_data(struct device *dev);
 struct dev_pm_domain {
 	struct dev_pm_ops	ops;
 	void (*detach)(struct device *dev, bool power_off);
+	int (*get)(struct dev_pm_domain *domain);
+	void (*put)(struct dev_pm_domain *domain);
 };
 
 /*
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 73e938b..8e2806f 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -304,12 +304,16 @@ static inline int of_genpd_add_provider_onecell(struct device_node *np,
 #ifdef CONFIG_PM
 extern int dev_pm_domain_attach(struct device *dev, bool power_on);
 extern void dev_pm_domain_detach(struct device *dev, bool power_off);
+extern int dev_pm_domain_get(struct dev_pm_domain *domain);
+extern void dev_pm_domain_put(struct dev_pm_domain *domain);
 #else
 static inline int dev_pm_domain_attach(struct device *dev, bool power_on)
 {
 	return -ENODEV;
 }
 static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {}
+static inline int dev_pm_domain_get(struct dev_pm_domain *domain) { return 0; }
+static inline void dev_pm_domain_put(struct dev_pm_domain *domain) {}
 #endif
 
 #endif /* _LINUX_PM_DOMAIN_H */
-- 
1.9.1

[PATCH v3 2/9] PM / Domains: Enable genpd to support ->get|put() callbacks

[Ulf Hansson]

To provide users control over whether the power should be maintained,
implement the ->get|put() callbacks for genpd's PM domain.

A usage count variable keeps track of the number of users. A positive
value tells genpd to keep supplying power and also to power up if it's
the first user.

Do note, the above applies no matter of CONFIG_PM_RUNTIME. That's
because we allows a PM domain to be initialized in power off state and
thus we need a way to power it up.

Once the usage count reaches zero, genpd tries to power off its PM
domain. Although for !CONFIG_PM_RUNTIME we need to maintain the power
once it's been enabled, since drivers can't use runtime PM to restore
it dynamically.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 26 +++++++++++++++++++++++++-
 include/linux/pm_domain.h   |  1 +
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 40bc2f4..96fe72e 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -451,10 +451,12 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 	 * (2) The domain is waiting for its master to power up.
 	 * (3) One of the domain's devices is being resumed right now.
 	 * (4) System suspend is in progress.
+	 * (5) The usage_count > 0, since it tells us to stay powered.
 	 */
 	if (genpd->status == GPD_STATE_POWER_OFF
 	    || genpd->status == GPD_STATE_WAIT_MASTER
-	    || genpd->resume_count > 0 || genpd->prepared_count > 0)
+	    || genpd->resume_count > 0 || genpd->prepared_count > 0
+	    || atomic_read(&genpd->usage_count) > 0)
 		return 0;
 
 	if (atomic_read(&genpd->sd_count) > 0)
@@ -1358,6 +1360,25 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
 
 #endif /* CONFIG_PM_SLEEP */
 
+static int pm_genpd_get(struct dev_pm_domain *domain)
+{
+	struct generic_pm_domain *genpd = pd_to_genpd(domain);
+
+	atomic_inc(&genpd->usage_count);
+
+	return pm_genpd_poweron(genpd);
+}
+
+static void pm_genpd_put(struct dev_pm_domain *domain)
+{
+	struct generic_pm_domain *genpd = pd_to_genpd(domain);
+
+	if (!atomic_dec_and_test(&genpd->usage_count))
+		return;
+
+	genpd_queue_power_off_work(genpd);
+}
+
 static struct generic_pm_domain_data *__pm_genpd_alloc_dev_data(struct device *dev)
 {
 	struct generic_pm_domain_data *gpd_data;
@@ -1875,10 +1896,13 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 	genpd->status = is_off ? GPD_STATE_POWER_OFF : GPD_STATE_ACTIVE;
 	init_waitqueue_head(&genpd->status_wait_queue);
 	genpd->poweroff_task = NULL;
+	atomic_set(&genpd->usage_count, 0);
 	genpd->resume_count = 0;
 	genpd->device_count = 0;
 	genpd->max_off_time_ns = -1;
 	genpd->max_off_time_changed = true;
+	genpd->domain.get = pm_genpd_get;
+	genpd->domain.put = pm_genpd_put;
 	genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend;
 	genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume;
 	genpd->domain.ops.prepare = pm_genpd_prepare;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 8e2806f..7b3cba2 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -58,6 +58,7 @@ struct generic_pm_domain {
 	enum gpd_status status;	/* Current state of the domain */
 	wait_queue_head_t status_wait_queue;
 	struct task_struct *poweroff_task;	/* Powering off task */
+	atomic_t usage_count;		/* Number of users of the domain */
 	unsigned int resume_count;	/* Number of devices being resumed */
 	unsigned int device_count;	/* Number of devices */
 	unsigned int suspended_count;	/* System suspend device counter */
-- 
1.9.1

[PATCH v3 2/9] PM / Domains: Enable genpd to support ->get|put() callbacks

[Ulf Hansson]

To provide users control over whether the power should be maintained,
implement the ->get|put() callbacks for genpd's PM domain.

A usage count variable keeps track of the number of users. A positive
value tells genpd to keep supplying power and also to power up if it's
the first user.

Do note, the above applies no matter of CONFIG_PM_RUNTIME. That's
because we allows a PM domain to be initialized in power off state and
thus we need a way to power it up.

Once the usage count reaches zero, genpd tries to power off its PM
domain. Although for !CONFIG_PM_RUNTIME we need to maintain the power
once it's been enabled, since drivers can't use runtime PM to restore
it dynamically.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 26 +++++++++++++++++++++++++-
 include/linux/pm_domain.h   |  1 +
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 40bc2f4..96fe72e 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -451,10 +451,12 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 	 * (2) The domain is waiting for its master to power up.
 	 * (3) One of the domain's devices is being resumed right now.
 	 * (4) System suspend is in progress.
+	 * (5) The usage_count > 0, since it tells us to stay powered.
 	 */
 	if (genpd->status == GPD_STATE_POWER_OFF
 	    || genpd->status == GPD_STATE_WAIT_MASTER
-	    || genpd->resume_count > 0 || genpd->prepared_count > 0)
+	    || genpd->resume_count > 0 || genpd->prepared_count > 0
+	    || atomic_read(&genpd->usage_count) > 0)
 		return 0;
 
 	if (atomic_read(&genpd->sd_count) > 0)
@@ -1358,6 +1360,25 @@ EXPORT_SYMBOL_GPL(pm_genpd_syscore_poweron);
 
 #endif /* CONFIG_PM_SLEEP */
 
+static int pm_genpd_get(struct dev_pm_domain *domain)
+{
+	struct generic_pm_domain *genpd = pd_to_genpd(domain);
+
+	atomic_inc(&genpd->usage_count);
+
+	return pm_genpd_poweron(genpd);
+}
+
+static void pm_genpd_put(struct dev_pm_domain *domain)
+{
+	struct generic_pm_domain *genpd = pd_to_genpd(domain);
+
+	if (!atomic_dec_and_test(&genpd->usage_count))
+		return;
+
+	genpd_queue_power_off_work(genpd);
+}
+
 static struct generic_pm_domain_data *__pm_genpd_alloc_dev_data(struct device *dev)
 {
 	struct generic_pm_domain_data *gpd_data;
@@ -1875,10 +1896,13 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 	genpd->status = is_off ? GPD_STATE_POWER_OFF : GPD_STATE_ACTIVE;
 	init_waitqueue_head(&genpd->status_wait_queue);
 	genpd->poweroff_task = NULL;
+	atomic_set(&genpd->usage_count, 0);
 	genpd->resume_count = 0;
 	genpd->device_count = 0;
 	genpd->max_off_time_ns = -1;
 	genpd->max_off_time_changed = true;
+	genpd->domain.get = pm_genpd_get;
+	genpd->domain.put = pm_genpd_put;
 	genpd->domain.ops.runtime_suspend = pm_genpd_runtime_suspend;
 	genpd->domain.ops.runtime_resume = pm_genpd_runtime_resume;
 	genpd->domain.ops.prepare = pm_genpd_prepare;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 8e2806f..7b3cba2 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -58,6 +58,7 @@ struct generic_pm_domain {
 	enum gpd_status status;	/* Current state of the domain */
 	wait_queue_head_t status_wait_queue;
 	struct task_struct *poweroff_task;	/* Powering off task */
+	atomic_t usage_count;		/* Number of users of the domain */
 	unsigned int resume_count;	/* Number of devices being resumed */
 	unsigned int device_count;	/* Number of devices */
 	unsigned int suspended_count;	/* System suspend device counter */
-- 
1.9.1

[PATCH v3 3/9] amba: Keep PM domain powered during ->probe()

[Ulf Hansson]

To sucessfully probe some devices their corresponding PM domains may
need to be powered.

Use the dev_pm_domain_get|put() APIs, to control the behavior of the PM
domain.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/amba/bus.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 47bbdc1..a8a8e13 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -187,8 +187,15 @@ static int amba_probe(struct device *dev)
 		if (ret == -EPROBE_DEFER)
 			break;
 
+		ret = dev_pm_domain_get(dev->pm_domain);
+		if (ret) {
+			dev_pm_domain_detach(dev, true);
+			break;
+		}
+
 		ret = amba_get_enable_pclk(pcdev);
 		if (ret) {
+			dev_pm_domain_put(dev->pm_domain);
 			dev_pm_domain_detach(dev, true);
 			break;
 		}
@@ -198,6 +205,7 @@ static int amba_probe(struct device *dev)
 		pm_runtime_enable(dev);
 
 		ret = pcdrv->probe(pcdev, id);
+		dev_pm_domain_put(dev->pm_domain);
 		if (ret == 0)
 			break;
 
-- 
1.9.1

[PATCH v3 3/9] amba: Keep PM domain powered during ->probe()

[Ulf Hansson]

To sucessfully probe some devices their corresponding PM domains may
need to be powered.

Use the dev_pm_domain_get|put() APIs, to control the behavior of the PM
domain.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/amba/bus.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 47bbdc1..a8a8e13 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -187,8 +187,15 @@ static int amba_probe(struct device *dev)
 		if (ret == -EPROBE_DEFER)
 			break;
 
+		ret = dev_pm_domain_get(dev->pm_domain);
+		if (ret) {
+			dev_pm_domain_detach(dev, true);
+			break;
+		}
+
 		ret = amba_get_enable_pclk(pcdev);
 		if (ret) {
+			dev_pm_domain_put(dev->pm_domain);
 			dev_pm_domain_detach(dev, true);
 			break;
 		}
@@ -198,6 +205,7 @@ static int amba_probe(struct device *dev)
 		pm_runtime_enable(dev);
 
 		ret = pcdrv->probe(pcdev, id);
+		dev_pm_domain_put(dev->pm_domain);
 		if (ret == 0)
 			break;
 
-- 
1.9.1

[PATCH v3 4/9] drivercore / platform: Keep PM domain powered during ->probe()

[Ulf Hansson]

To sucessfully probe some devices their corresponding PM domains may
need to be powered.

Use the dev_pm_domain_get|put() APIs, to control the behavior of the PM
domain.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/platform.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index b2afc29..43897ad 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -509,7 +509,14 @@ static int platform_drv_probe(struct device *_dev)
 
 	ret = dev_pm_domain_attach(_dev, true);
 	if (ret != -EPROBE_DEFER) {
+		ret = dev_pm_domain_get(_dev->pm_domain);
+		if (ret) {
+			dev_pm_domain_detach(_dev, true);
+			return ret;
+		}
+
 		ret = drv->probe(dev);
+		dev_pm_domain_put(_dev->pm_domain);
 		if (ret)
 			dev_pm_domain_detach(_dev, true);
 	}
-- 
1.9.1

[PATCH v3 4/9] drivercore / platform: Keep PM domain powered during ->probe()

[Ulf Hansson]

To sucessfully probe some devices their corresponding PM domains may
need to be powered.

Use the dev_pm_domain_get|put() APIs, to control the behavior of the PM
domain.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/platform.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index b2afc29..43897ad 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -509,7 +509,14 @@ static int platform_drv_probe(struct device *_dev)
 
 	ret = dev_pm_domain_attach(_dev, true);
 	if (ret != -EPROBE_DEFER) {
+		ret = dev_pm_domain_get(_dev->pm_domain);
+		if (ret) {
+			dev_pm_domain_detach(_dev, true);
+			return ret;
+		}
+
 		ret = drv->probe(dev);
+		dev_pm_domain_put(_dev->pm_domain);
 		if (ret)
 			dev_pm_domain_detach(_dev, true);
 	}
-- 
1.9.1

[PATCH v3 5/9] i2c: core: Keep PM domain powered during ->probe()

[Ulf Hansson]

To sucessfully probe some devices their corresponding PM domains may
need to be powered.

Use the dev_pm_domain_get|put() APIs, to control the behavior of the PM
domain.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/i2c-core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2f90ac6..37a52c6 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -646,8 +646,15 @@ static int i2c_device_probe(struct device *dev)
 
 	status = dev_pm_domain_attach(&client->dev, true);
 	if (status != -EPROBE_DEFER) {
+		status = dev_pm_domain_get(client->dev.pm_domain);
+		if (status) {
+			dev_pm_domain_detach(&client->dev, true);
+			return status;
+		}
+
 		status = driver->probe(client, i2c_match_id(driver->id_table,
 					client));
+		dev_pm_domain_put(client->dev.pm_domain);
 		if (status)
 			dev_pm_domain_detach(&client->dev, true);
 	}
-- 
1.9.1

[PATCH v3 5/9] i2c: core: Keep PM domain powered during ->probe()

[Ulf Hansson]

To sucessfully probe some devices their corresponding PM domains may
need to be powered.

Use the dev_pm_domain_get|put() APIs, to control the behavior of the PM
domain.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/i2c/i2c-core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 2f90ac6..37a52c6 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -646,8 +646,15 @@ static int i2c_device_probe(struct device *dev)
 
 	status = dev_pm_domain_attach(&client->dev, true);
 	if (status != -EPROBE_DEFER) {
+		status = dev_pm_domain_get(client->dev.pm_domain);
+		if (status) {
+			dev_pm_domain_detach(&client->dev, true);
+			return status;
+		}
+
 		status = driver->probe(client, i2c_match_id(driver->id_table,
 					client));
+		dev_pm_domain_put(client->dev.pm_domain);
 		if (status)
 			dev_pm_domain_detach(&client->dev, true);
 	}
-- 
1.9.1

[PATCH v3 6/9] spi: core: Keep PM domain powered during ->probe()

[Ulf Hansson]

To sucessfully probe some devices their corresponding PM domains may
need to be powered.

Use the dev_pm_domain_get|put() APIs, to control the behavior of the PM
domain.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/spi/spi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 3907f14..ad1a6f3 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -267,7 +267,14 @@ static int spi_drv_probe(struct device *dev)
 
 	ret = dev_pm_domain_attach(dev, true);
 	if (ret != -EPROBE_DEFER) {
+		ret = dev_pm_domain_get(dev->pm_domain);
+		if (ret) {
+			dev_pm_domain_detach(dev, true);
+			return ret;
+		}
+
 		ret = sdrv->probe(to_spi_device(dev));
+		dev_pm_domain_put(dev->pm_domain);
 		if (ret)
 			dev_pm_domain_detach(dev, true);
 	}
-- 
1.9.1

[PATCH v3 6/9] spi: core: Keep PM domain powered during ->probe()

[Ulf Hansson]

To sucessfully probe some devices their corresponding PM domains may
need to be powered.

Use the dev_pm_domain_get|put() APIs, to control the behavior of the PM
domain.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/spi/spi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 3907f14..ad1a6f3 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -267,7 +267,14 @@ static int spi_drv_probe(struct device *dev)
 
 	ret = dev_pm_domain_attach(dev, true);
 	if (ret != -EPROBE_DEFER) {
+		ret = dev_pm_domain_get(dev->pm_domain);
+		if (ret) {
+			dev_pm_domain_detach(dev, true);
+			return ret;
+		}
+
 		ret = sdrv->probe(to_spi_device(dev));
+		dev_pm_domain_put(dev->pm_domain);
 		if (ret)
 			dev_pm_domain_detach(dev, true);
 	}
-- 
1.9.1

[PATCH v3 7/9] mmc: core: Attach PM domain prior probing of SDIO func driver

[Ulf Hansson]

Align the behvaior to how other buses are handling attachment of PM
domains. Additionally, let's add error handling.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio_bus.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index 65cf7a7..ba97779 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -313,13 +313,19 @@ int sdio_add_func(struct sdio_func *func)
 	dev_set_name(&func->dev, "%s:%d", mmc_card_id(func->card), func->num);
 
 	sdio_acpi_set_handle(func);
+
+	ret = dev_pm_domain_attach(&func->dev, false);
+	if (ret)
+		return ret;
+
 	ret = device_add(&func->dev);
-	if (ret == 0) {
-		sdio_func_set_present(func);
-		dev_pm_domain_attach(&func->dev, false);
+	if (ret) {
+		dev_pm_domain_detach(&func->dev, false);
+		return ret;
 	}
 
-	return ret;
+	sdio_func_set_present(func);
+	return 0;
 }
 
 /*
-- 
1.9.1

[PATCH v3 7/9] mmc: core: Attach PM domain prior probing of SDIO func driver

[Ulf Hansson]

Align the behvaior to how other buses are handling attachment of PM
domains. Additionally, let's add error handling.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio_bus.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index 65cf7a7..ba97779 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -313,13 +313,19 @@ int sdio_add_func(struct sdio_func *func)
 	dev_set_name(&func->dev, "%s:%d", mmc_card_id(func->card), func->num);
 
 	sdio_acpi_set_handle(func);
+
+	ret = dev_pm_domain_attach(&func->dev, false);
+	if (ret)
+		return ret;
+
 	ret = device_add(&func->dev);
-	if (ret == 0) {
-		sdio_func_set_present(func);
-		dev_pm_domain_attach(&func->dev, false);
+	if (ret) {
+		dev_pm_domain_detach(&func->dev, false);
+		return ret;
 	}
 
-	return ret;
+	sdio_func_set_present(func);
+	return 0;
 }
 
 /*
-- 
1.9.1

[PATCH v3 8/9] mmmc: core: Keep PM domain powered during ->probe() of SDIO func driver

[Ulf Hansson]

To sucessfully probe some devices their corresponding PM domains may
need to be powered.

Use the dev_pm_domain_get|put() APIs, to control the behavior of the PM
domain.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio_bus.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index ba97779..83c8836 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -318,7 +318,14 @@ int sdio_add_func(struct sdio_func *func)
 	if (ret)
 		return ret;
 
+	ret = dev_pm_domain_get(func->dev.pm_domain);
+	if (ret) {
+		dev_pm_domain_detach(&func->dev, false);
+		return ret;
+	}
+
 	ret = device_add(&func->dev);
+	dev_pm_domain_put(func->dev.pm_domain);
 	if (ret) {
 		dev_pm_domain_detach(&func->dev, false);
 		return ret;
-- 
1.9.1

[PATCH v3 8/9] mmmc: core: Keep PM domain powered during ->probe() of SDIO func driver

[Ulf Hansson]

To sucessfully probe some devices their corresponding PM domains may
need to be powered.

Use the dev_pm_domain_get|put() APIs, to control the behavior of the PM
domain.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/mmc/core/sdio_bus.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index ba97779..83c8836 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -318,7 +318,14 @@ int sdio_add_func(struct sdio_func *func)
 	if (ret)
 		return ret;
 
+	ret = dev_pm_domain_get(func->dev.pm_domain);
+	if (ret) {
+		dev_pm_domain_detach(&func->dev, false);
+		return ret;
+	}
+
 	ret = device_add(&func->dev);
+	dev_pm_domain_put(func->dev.pm_domain);
 	if (ret) {
 		dev_pm_domain_detach(&func->dev, false);
 		return ret;
-- 
1.9.1

[PATCH v3 9/9] PM / Domains: Remove pm_genpd_dev_need_restore() API

[Ulf Hansson]

There are currently no users of this API, let's remove it.

Additionally, if such feature would be needed future wise, a better
option is likely use pm_runtime_set_active|suspended() in some form.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Kevin Hilman <khilman@linaro.org>
---
 drivers/base/power/domain.c | 20 --------------------
 include/linux/pm_domain.h   |  2 --
 2 files changed, 22 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 96fe72e..a717132 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1554,26 +1554,6 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 }
 
 /**
- * pm_genpd_dev_need_restore - Set/unset the device's "need restore" flag.
- * @dev: Device to set/unset the flag for.
- * @val: The new value of the device's "need restore" flag.
- */
-void pm_genpd_dev_need_restore(struct device *dev, bool val)
-{
-	struct pm_subsys_data *psd;
-	unsigned long flags;
-
-	spin_lock_irqsave(&dev->power.lock, flags);
-
-	psd = dev_to_psd(dev);
-	if (psd && psd->domain_data)
-		to_gpd_data(psd->domain_data)->need_restore = val;
-
-	spin_unlock_irqrestore(&dev->power.lock, flags);
-}
-EXPORT_SYMBOL_GPL(pm_genpd_dev_need_restore);
-
-/**
  * pm_genpd_add_subdomain - Add a subdomain to an I/O PM domain.
  * @genpd: Master PM domain to add the subdomain to.
  * @subdomain: Subdomain to be added.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 7b3cba2..d5df64e 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -130,7 +130,6 @@ extern int __pm_genpd_name_add_device(const char *domain_name,
 
 extern int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 				  struct device *dev);
-extern void pm_genpd_dev_need_restore(struct device *dev, bool val);
 extern int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 				  struct generic_pm_domain *new_subdomain);
 extern int pm_genpd_add_subdomain_names(const char *master_name,
@@ -176,7 +175,6 @@ static inline int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 {
 	return -ENOSYS;
 }
-static inline void pm_genpd_dev_need_restore(struct device *dev, bool val) {}
 static inline int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 					 struct generic_pm_domain *new_sd)
 {
-- 
1.9.1

[PATCH v3 9/9] PM / Domains: Remove pm_genpd_dev_need_restore() API

[Ulf Hansson]

There are currently no users of this API, let's remove it.

Additionally, if such feature would be needed future wise, a better
option is likely use pm_runtime_set_active|suspended() in some form.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Kevin Hilman <khilman@linaro.org>
---
 drivers/base/power/domain.c | 20 --------------------
 include/linux/pm_domain.h   |  2 --
 2 files changed, 22 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 96fe72e..a717132 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1554,26 +1554,6 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 }
 
 /**
- * pm_genpd_dev_need_restore - Set/unset the device's "need restore" flag.
- * @dev: Device to set/unset the flag for.
- * @val: The new value of the device's "need restore" flag.
- */
-void pm_genpd_dev_need_restore(struct device *dev, bool val)
-{
-	struct pm_subsys_data *psd;
-	unsigned long flags;
-
-	spin_lock_irqsave(&dev->power.lock, flags);
-
-	psd = dev_to_psd(dev);
-	if (psd && psd->domain_data)
-		to_gpd_data(psd->domain_data)->need_restore = val;
-
-	spin_unlock_irqrestore(&dev->power.lock, flags);
-}
-EXPORT_SYMBOL_GPL(pm_genpd_dev_need_restore);
-
-/**
  * pm_genpd_add_subdomain - Add a subdomain to an I/O PM domain.
  * @genpd: Master PM domain to add the subdomain to.
  * @subdomain: Subdomain to be added.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 7b3cba2..d5df64e 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -130,7 +130,6 @@ extern int __pm_genpd_name_add_device(const char *domain_name,
 
 extern int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 				  struct device *dev);
-extern void pm_genpd_dev_need_restore(struct device *dev, bool val);
 extern int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 				  struct generic_pm_domain *new_subdomain);
 extern int pm_genpd_add_subdomain_names(const char *master_name,
@@ -176,7 +175,6 @@ static inline int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 {
 	return -ENOSYS;
 }
-static inline void pm_genpd_dev_need_restore(struct device *dev, bool val) {}
 static inline int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 					 struct generic_pm_domain *new_sd)
 {
-- 
1.9.1

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> Changes in v3:
> 	-Rework the entire intermediate step which was suggested in v2.
> 	 That means solving the race condition, but also cope with PM domains
> 	 that are initialized in powered off state.
>
> Changes in v2:
> 	-Added some acks.
> 	-Updated commit messages.
> 	-Included a wider audience of the patchset. V1 was lacking SoC
> 	 maintainers.
>
> Here are link to the first patchset, were some discussion started.
> http://marc.info/?l=linux-pm&m=141208104729597&w=2
>
> There may be more than one device in a PM domain which then will be
> probed at different points in time.
>
> Depending on timing and runtime PM support, in for the device related
> driver/subsystem, a PM domain may be advised to power off after a
> successful probe sequence.
>
> A general requirement for a device within a PM domain, is that the
> PM domain must stay powered during the probe sequence. To cope with
> such requirement, let's add two new APIs, dev_pm_domain_get|put().
>
> These APIs are intended to be invoked from subsystem-level code and the
> calls between get/put needs to be balanced.
>
> dev_pm_domain_get(), tells the PM domain that it needs to increase a
> usage count and to keep supplying power. dev_pm_domain_put(), does the
> opposite.

I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

What's not explained here (or what I'm not understanding) is why a PM
domain is powering off if it has active devices.

Kevin

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> Changes in v3:
> 	-Rework the entire intermediate step which was suggested in v2.
> 	 That means solving the race condition, but also cope with PM domains
> 	 that are initialized in powered off state.
>
> Changes in v2:
> 	-Added some acks.
> 	-Updated commit messages.
> 	-Included a wider audience of the patchset. V1 was lacking SoC
> 	 maintainers.
>
> Here are link to the first patchset, were some discussion started.
> http://marc.info/?l=linux-pm&m=141208104729597&w=2
>
> There may be more than one device in a PM domain which then will be
> probed at different points in time.
>
> Depending on timing and runtime PM support, in for the device related
> driver/subsystem, a PM domain may be advised to power off after a
> successful probe sequence.
>
> A general requirement for a device within a PM domain, is that the
> PM domain must stay powered during the probe sequence. To cope with
> such requirement, let's add two new APIs, dev_pm_domain_get|put().
>
> These APIs are intended to be invoked from subsystem-level code and the
> calls between get/put needs to be balanced.
>
> dev_pm_domain_get(), tells the PM domain that it needs to increase a
> usage count and to keep supplying power. dev_pm_domain_put(), does the
> opposite.

I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

What's not explained here (or what I'm not understanding) is why a PM
domain is powering off if it has active devices.

Kevin

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Mark Brown]

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

On Fri, Oct 24, 2014 at 09:12:39AM -0700, Kevin Hilman wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:

> > There may be more than one device in a PM domain which then will be
> > probed at different points in time.

> > Depending on timing and runtime PM support, in for the device related
> > driver/subsystem, a PM domain may be advised to power off after a
> > successful probe sequence.

> > A general requirement for a device within a PM domain, is that the
> > PM domain must stay powered during the probe sequence. To cope with
> > such requirement, let's add two new APIs, dev_pm_domain_get|put().

> I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

> What's not explained here (or what I'm not understanding) is why a PM
> domain is powering off if it has active devices.

The issue AIUI is what happens during system boot - if one device in a
domain probes and marks itself runtime idle then that will trigger
domain powerdown even if there is another device in the domain that
hasn't yet been probed.  This can cause undesirable glitches (or worse)
during boot depending on what's getting powered down.

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

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Mark Brown]

On Fri, Oct 24, 2014 at 09:12:39AM -0700, Kevin Hilman wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:

> > There may be more than one device in a PM domain which then will be
> > probed at different points in time.

> > Depending on timing and runtime PM support, in for the device related
> > driver/subsystem, a PM domain may be advised to power off after a
> > successful probe sequence.

> > A general requirement for a device within a PM domain, is that the
> > PM domain must stay powered during the probe sequence. To cope with
> > such requirement, let's add two new APIs, dev_pm_domain_get|put().

> I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

> What's not explained here (or what I'm not understanding) is why a PM
> domain is powering off if it has active devices.

The issue AIUI is what happens during system boot - if one device in a
domain probes and marks itself runtime idle then that will trigger
domain powerdown even if there is another device in the domain that
hasn't yet been probed.  This can cause undesirable glitches (or worse)
during boot depending on what's getting powered down.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141024/5545a19c/attachment.sig>

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Kevin Hilman]

Mark Brown <broonie@kernel.org> writes:

> On Fri, Oct 24, 2014 at 09:12:39AM -0700, Kevin Hilman wrote:
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> > There may be more than one device in a PM domain which then will be
>> > probed at different points in time.
>
>> > Depending on timing and runtime PM support, in for the device related
>> > driver/subsystem, a PM domain may be advised to power off after a
>> > successful probe sequence.
>
>> > A general requirement for a device within a PM domain, is that the
>> > PM domain must stay powered during the probe sequence. To cope with
>> > such requirement, let's add two new APIs, dev_pm_domain_get|put().
>
>> I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?
>
>> What's not explained here (or what I'm not understanding) is why a PM
>> domain is powering off if it has active devices.
>
> The issue AIUI is what happens during system boot - if one device in a
> domain probes and marks itself runtime idle then that will trigger
> domain powerdown even if there is another device in the domain that
> hasn't yet been probed.  This can cause undesirable glitches (or worse)
> during boot depending on what's getting powered down.

I'm not quite seeing how this series fixes that problem.

Looking at platform devices in PATCH 4/9, the new _get() and _put() are
still happening around ->probe(), so if a platform device runtime suspends
after probe, don't we still have a PM domain that can turn off?

Kevin

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Kevin Hilman]

Mark Brown <broonie@kernel.org> writes:

> On Fri, Oct 24, 2014 at 09:12:39AM -0700, Kevin Hilman wrote:
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> > There may be more than one device in a PM domain which then will be
>> > probed at different points in time.
>
>> > Depending on timing and runtime PM support, in for the device related
>> > driver/subsystem, a PM domain may be advised to power off after a
>> > successful probe sequence.
>
>> > A general requirement for a device within a PM domain, is that the
>> > PM domain must stay powered during the probe sequence. To cope with
>> > such requirement, let's add two new APIs, dev_pm_domain_get|put().
>
>> I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?
>
>> What's not explained here (or what I'm not understanding) is why a PM
>> domain is powering off if it has active devices.
>
> The issue AIUI is what happens during system boot - if one device in a
> domain probes and marks itself runtime idle then that will trigger
> domain powerdown even if there is another device in the domain that
> hasn't yet been probed.  This can cause undesirable glitches (or worse)
> during boot depending on what's getting powered down.

I'm not quite seeing how this series fixes that problem.

Looking at platform devices in PATCH 4/9, the new _get() and _put() are
still happening around ->probe(), so if a platform device runtime suspends
after probe, don't we still have a PM domain that can turn off?

Kevin

Re: [PATCH v3 4/9] drivercore / platform: Keep PM domain powered during ->probe()

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> To sucessfully probe some devices their corresponding PM domains may
> need to be powered.

Isn't that what pm_runtime_get*() is supposed to be doing?  Why isn't
that working?

Kevin

[PATCH v3 4/9] drivercore / platform: Keep PM domain powered during ->probe()

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> To sucessfully probe some devices their corresponding PM domains may
> need to be powered.

Isn't that what pm_runtime_get*() is supposed to be doing?  Why isn't
that working?

Kevin

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Mark Brown]

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

On Thu, Oct 30, 2014 at 01:46:43PM -0700, Kevin Hilman wrote:
> Mark Brown <broonie@kernel.org> writes:
> > On Fri, Oct 24, 2014 at 09:12:39AM -0700, Kevin Hilman wrote:

> >> I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

> >> What's not explained here (or what I'm not understanding) is why a PM
> >> domain is powering off if it has active devices.

> > The issue AIUI is what happens during system boot - if one device in a
> > domain probes and marks itself runtime idle then that will trigger
> > domain powerdown even if there is another device in the domain that
> > hasn't yet been probed.  This can cause undesirable glitches (or worse)
> > during boot depending on what's getting powered down.

> I'm not quite seeing how this series fixes that problem.

> Looking at platform devices in PATCH 4/9, the new _get() and _put() are
> still happening around ->probe(), so if a platform device runtime suspends
> after probe, don't we still have a PM domain that can turn off?

Yeah, I haven't actually reviewed the series so I don't know if it does
the above but that was my understanding of the intention.

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

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Mark Brown]

On Thu, Oct 30, 2014 at 01:46:43PM -0700, Kevin Hilman wrote:
> Mark Brown <broonie@kernel.org> writes:
> > On Fri, Oct 24, 2014 at 09:12:39AM -0700, Kevin Hilman wrote:

> >> I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

> >> What's not explained here (or what I'm not understanding) is why a PM
> >> domain is powering off if it has active devices.

> > The issue AIUI is what happens during system boot - if one device in a
> > domain probes and marks itself runtime idle then that will trigger
> > domain powerdown even if there is another device in the domain that
> > hasn't yet been probed.  This can cause undesirable glitches (or worse)
> > during boot depending on what's getting powered down.

> I'm not quite seeing how this series fixes that problem.

> Looking at platform devices in PATCH 4/9, the new _get() and _put() are
> still happening around ->probe(), so if a platform device runtime suspends
> after probe, don't we still have a PM domain that can turn off?

Yeah, I haven't actually reviewed the series so I don't know if it does
the above but that was my understanding of the intention.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141030/54f67faa/attachment-0001.sig>

Re: [PATCH v3 4/9] drivercore / platform: Keep PM domain powered during ->probe()

[Dmitry Torokhov]

On Thu, Oct 30, 2014 at 01:47:27PM -0700, Kevin Hilman wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
> 
> > To sucessfully probe some devices their corresponding PM domains may
> > need to be powered.
> 
> Isn't that what pm_runtime_get*() is supposed to be doing?  Why isn't
> that working?

Also, I do not understand why we placing device into a power domain only
when we probe it. Why if I unbind device from its driver (or do not have
a driver for it) it disappears from its power domain? To me power domain
and having driver bound to a device are 2 orthogonal concepts.

Thanks.

-- 
Dmitry

[PATCH v3 4/9] drivercore / platform: Keep PM domain powered during ->probe()

[Dmitry Torokhov]

On Thu, Oct 30, 2014 at 01:47:27PM -0700, Kevin Hilman wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
> 
> > To sucessfully probe some devices their corresponding PM domains may
> > need to be powered.
> 
> Isn't that what pm_runtime_get*() is supposed to be doing?  Why isn't
> that working?

Also, I do not understand why we placing device into a power domain only
when we probe it. Why if I unbind device from its driver (or do not have
a driver for it) it disappears from its power domain? To me power domain
and having driver bound to a device are 2 orthogonal concepts.

Thanks.

-- 
Dmitry

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> Changes in v3:
>>       -Rework the entire intermediate step which was suggested in v2.
>>        That means solving the race condition, but also cope with PM domains
>>        that are initialized in powered off state.
>>
>> Changes in v2:
>>       -Added some acks.
>>       -Updated commit messages.
>>       -Included a wider audience of the patchset. V1 was lacking SoC
>>        maintainers.
>>
>> Here are link to the first patchset, were some discussion started.
>> http://marc.info/?l=linux-pm&m=141208104729597&w=2
>>
>> There may be more than one device in a PM domain which then will be
>> probed at different points in time.
>>
>> Depending on timing and runtime PM support, in for the device related
>> driver/subsystem, a PM domain may be advised to power off after a
>> successful probe sequence.
>>
>> A general requirement for a device within a PM domain, is that the
>> PM domain must stay powered during the probe sequence. To cope with
>> such requirement, let's add two new APIs, dev_pm_domain_get|put().
>>
>> These APIs are intended to be invoked from subsystem-level code and the
>> calls between get/put needs to be balanced.
>>
>> dev_pm_domain_get(), tells the PM domain that it needs to increase a
>> usage count and to keep supplying power. dev_pm_domain_put(), does the
>> opposite.
>
> I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

See, below.

>
> What's not explained here (or what I'm not understanding) is why a PM
> domain is powering off if it has active devices.

It doesn't. The problem is that using pm_runtime_get_sync() in this
path is not working.

Now, I failed to include some of the important information from
previous discussions around this patchset. Let me iterate the patchset
with better commit messages, but let's first continue this thread.

Here are some of the previous discussion:

http://marc.info/?l=linux-pm&m=141270897014653&w=2
http://marc.info/?l=linux-pm&m=141208104729597&w=2

Below is a summary of why I think "pm_runtime_get_sync()" isn't working for us.

1)
It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
to bring your resources to full power. The consequence would be a
driver that requires CONFIG_PM_RUNTIME to be even functional, which
just isn't acceptable.

Drivers that behaves well within this context, follows the runtime PM
documentation/recommendation. They use pm_runtime_set_active() during
->probe() to reflect that their devices are fully powered and capable
of handling I/O.

You may also have a look at these discussions which also touches this
topic, but within a context of another patchset.
https://lkml.org/lkml/2014/10/23/95

2)
Another good example why pm_runtime_get_sync() is a bad solution to
our problem, is the amba bus. Before it invokes the driver's ->probe()
callback it does the following.
- "enable bus clock"
- pm_runtime_get_noresume()
- pm_runtime_set_active()
- pm_runtime_enable()

For these scenarios a pm_runtime_get_sync() from any of amba driver's
->probe() callback wouldn't have any effect, since the device is
already active. In other words, the resources needs to be "manually"
enabled.


Kind regards
Uffe

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> Changes in v3:
>>       -Rework the entire intermediate step which was suggested in v2.
>>        That means solving the race condition, but also cope with PM domains
>>        that are initialized in powered off state.
>>
>> Changes in v2:
>>       -Added some acks.
>>       -Updated commit messages.
>>       -Included a wider audience of the patchset. V1 was lacking SoC
>>        maintainers.
>>
>> Here are link to the first patchset, were some discussion started.
>> http://marc.info/?l=linux-pm&m=141208104729597&w=2
>>
>> There may be more than one device in a PM domain which then will be
>> probed at different points in time.
>>
>> Depending on timing and runtime PM support, in for the device related
>> driver/subsystem, a PM domain may be advised to power off after a
>> successful probe sequence.
>>
>> A general requirement for a device within a PM domain, is that the
>> PM domain must stay powered during the probe sequence. To cope with
>> such requirement, let's add two new APIs, dev_pm_domain_get|put().
>>
>> These APIs are intended to be invoked from subsystem-level code and the
>> calls between get/put needs to be balanced.
>>
>> dev_pm_domain_get(), tells the PM domain that it needs to increase a
>> usage count and to keep supplying power. dev_pm_domain_put(), does the
>> opposite.
>
> I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?

See, below.

>
> What's not explained here (or what I'm not understanding) is why a PM
> domain is powering off if it has active devices.

It doesn't. The problem is that using pm_runtime_get_sync() in this
path is not working.

Now, I failed to include some of the important information from
previous discussions around this patchset. Let me iterate the patchset
with better commit messages, but let's first continue this thread.

Here are some of the previous discussion:

http://marc.info/?l=linux-pm&m=141270897014653&w=2
http://marc.info/?l=linux-pm&m=141208104729597&w=2

Below is a summary of why I think "pm_runtime_get_sync()" isn't working for us.

1)
It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
to bring your resources to full power. The consequence would be a
driver that requires CONFIG_PM_RUNTIME to be even functional, which
just isn't acceptable.

Drivers that behaves well within this context, follows the runtime PM
documentation/recommendation. They use pm_runtime_set_active() during
->probe() to reflect that their devices are fully powered and capable
of handling I/O.

You may also have a look at these discussions which also touches this
topic, but within a context of another patchset.
https://lkml.org/lkml/2014/10/23/95

2)
Another good example why pm_runtime_get_sync() is a bad solution to
our problem, is the amba bus. Before it invokes the driver's ->probe()
callback it does the following.
- "enable bus clock"
- pm_runtime_get_noresume()
- pm_runtime_set_active()
- pm_runtime_enable()

For these scenarios a pm_runtime_get_sync() from any of amba driver's
->probe() callback wouldn't have any effect, since the device is
already active. In other words, the resources needs to be "manually"
enabled.


Kind regards
Uffe

Re: [PATCH v3 4/9] drivercore / platform: Keep PM domain powered during ->probe()

[Ulf Hansson]

On 30 October 2014 21:47, Kevin Hilman <khilman@kernel.org> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> To sucessfully probe some devices their corresponding PM domains may
>> need to be powered.
>
> Isn't that what pm_runtime_get*() is supposed to be doing?  Why isn't
> that working?

Let stay at the discussion of the cover letter patch. I just replied
to this there.

Kind regards
Uffe

[PATCH v3 4/9] drivercore / platform: Keep PM domain powered during ->probe()

[Ulf Hansson]

On 30 October 2014 21:47, Kevin Hilman <khilman@kernel.org> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> To sucessfully probe some devices their corresponding PM domains may
>> need to be powered.
>
> Isn't that what pm_runtime_get*() is supposed to be doing?  Why isn't
> that working?

Let stay at the discussion of the cover letter patch. I just replied
to this there.

Kind regards
Uffe

Re: [PATCH v3 4/9] drivercore / platform: Keep PM domain powered during ->probe()

[Ulf Hansson]

On 31 October 2014 01:07, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Thu, Oct 30, 2014 at 01:47:27PM -0700, Kevin Hilman wrote:
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>
>> > To sucessfully probe some devices their corresponding PM domains may
>> > need to be powered.
>>
>> Isn't that what pm_runtime_get*() is supposed to be doing?  Why isn't
>> that working?
>
> Also, I do not understand why we placing device into a power domain only
> when we probe it. Why if I unbind device from its driver (or do not have
> a driver for it) it disappears from its power domain? To me power domain
> and having driver bound to a device are 2 orthogonal concepts.

That's a different discussion, I don't think we want to go there
within this context.

Kind regards
Uffe

[PATCH v3 4/9] drivercore / platform: Keep PM domain powered during ->probe()

[Ulf Hansson]

On 31 October 2014 01:07, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Thu, Oct 30, 2014 at 01:47:27PM -0700, Kevin Hilman wrote:
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>
>> > To sucessfully probe some devices their corresponding PM domains may
>> > need to be powered.
>>
>> Isn't that what pm_runtime_get*() is supposed to be doing?  Why isn't
>> that working?
>
> Also, I do not understand why we placing device into a power domain only
> when we probe it. Why if I unbind device from its driver (or do not have
> a driver for it) it disappears from its power domain? To me power domain
> and having driver bound to a device are 2 orthogonal concepts.

That's a different discussion, I don't think we want to go there
within this context.

Kind regards
Uffe

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:
> On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:
> > Ulf Hansson <ulf.hansson@linaro.org> writes:
> >
> >> Changes in v3:
> >>       -Rework the entire intermediate step which was suggested in v2.
> >>        That means solving the race condition, but also cope with PM domains
> >>        that are initialized in powered off state.
> >>
> >> Changes in v2:
> >>       -Added some acks.
> >>       -Updated commit messages.
> >>       -Included a wider audience of the patchset. V1 was lacking SoC
> >>        maintainers.
> >>
> >> Here are link to the first patchset, were some discussion started.
> >> http://marc.info/?l=linux-pm&m=141208104729597&w=2
> >>
> >> There may be more than one device in a PM domain which then will be
> >> probed at different points in time.
> >>
> >> Depending on timing and runtime PM support, in for the device related
> >> driver/subsystem, a PM domain may be advised to power off after a
> >> successful probe sequence.
> >>
> >> A general requirement for a device within a PM domain, is that the
> >> PM domain must stay powered during the probe sequence. To cope with
> >> such requirement, let's add two new APIs, dev_pm_domain_get|put().
> >>
> >> These APIs are intended to be invoked from subsystem-level code and the
> >> calls between get/put needs to be balanced.
> >>
> >> dev_pm_domain_get(), tells the PM domain that it needs to increase a
> >> usage count and to keep supplying power. dev_pm_domain_put(), does the
> >> opposite.
> >
> > I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?
> 
> See, below.
> 
> >
> > What's not explained here (or what I'm not understanding) is why a PM
> > domain is powering off if it has active devices.
> 
> It doesn't. The problem is that using pm_runtime_get_sync() in this
> path is not working.
> 
> Now, I failed to include some of the important information from
> previous discussions around this patchset. Let me iterate the patchset
> with better commit messages, but let's first continue this thread.
> 
> Here are some of the previous discussion:
> 
> http://marc.info/?l=linux-pm&m=141270897014653&w=2
> http://marc.info/?l=linux-pm&m=141208104729597&w=2
> 
> Below is a summary of why I think "pm_runtime_get_sync()" isn't working for us.
> 
> 1)
> It's bad practice to use pm_runtime_get_sync() in the ->probe() path,

Honestly, I'm no longer amused.

> to bring your resources to full power. The consequence would be a
> driver that requires CONFIG_PM_RUNTIME to be even functional, which
> just isn't acceptable.

Sorry, but this is utter nonsense.

CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
everything they need to be always on.  If that is not the case, then someone is
doing runtime PM behind the scenes and therefore cheating.  Or in different
words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
everything is on from the drivers' perspective.

If that is the case, then calling pm_runtime_get_sync() from ->probe
for CONFIG_PM_RUNTIME unset simply doesn't matter.

Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
pm_runtime_get_sync() from ->probe is the only way the driver can ensure
in a non-racy way that the device will be accessible going forward.

Why?  Simply because the probing need not happen during system initialization.
It very well may take places when the other devices in the same domain have
beein in use for quite a while and have been using runtime PM (in which
case the domain may go off at any time unless it is explicityly prevented from
doing that).

One thing that you may be missing is that, for CONFIG_PM_RUNTIME set,
runtime PM has to be either enabled or disabled for all devices in one
domain (and if it is disabled, then the domain needs to be always on for
all practical purposes).  Otherwise you can't just make all of them happy
at the same time.  The documentation doesn't cover this, because it had been
written before we even started to consider power domains.

> Drivers that behaves well within this context, follows the runtime PM
> documentation/recommendation.

So please go to Documentation/power/runtime_pm.txt and read it again.  Quote:

"If the default initial runtime PM status of the device (i.e. 'suspended')
reflects the actual state of the device, its bus type's or its driver's
->probe() callback will likely need to wake it up using one of the PM core's
helper functions described in Section 4.  In that case, pm_runtime_resume()
should be used.  Of course, for this purpose the device's runtime PM has to be
enabled earlier by calling pm_runtime_enable()."

So how is this in agreement with what you're saying, I wonder?

> They use pm_runtime_set_active() during ->probe() to reflect that their
> devices are fully powered and capable of handling I/O.

And how the heck can a driver (whose device belongs to a power domain) be sure
that the device is "fully powered and capable of handling I/O" duing ->probe()?

> You may also have a look at these discussions which also touches this
> topic, but within a context of another patchset.
> https://lkml.org/lkml/2014/10/23/95

Which looks like reiterating the same incorrect arguments.

> 2)
> Another good example why pm_runtime_get_sync() is a bad solution to
> our problem, is the amba bus. Before it invokes the driver's ->probe()
> callback it does the following.
> - "enable bus clock"
> - pm_runtime_get_noresume()
> - pm_runtime_set_active()
> - pm_runtime_enable()

Which may not work if power domains are involved.

> For these scenarios a pm_runtime_get_sync() from any of amba driver's
> ->probe() callback wouldn't have any effect, since the device is
> already active. In other words, the resources needs to be "manually"
> enabled.

There seems to be a lot of confusion around this, so let me summarize:

- For CONFIG_PM_RUNTIME unsed drivers should be able to *safely* assume
  that they can access devices at any time.  All of the runtime PM helpers
  don't matter then (up to the error codes returned).

- For CONFIG_PM_RUNTIME set, if power domains are in use, then runtime PM
  has to be either enabled or disabled for all devices in one domain (and
  if disabled, the expected behavior is like the above).

- For CONFIG_PM_RUNTIME set, if power domains are in use, drivers (or bus
  types etc) should not make any assumptions about devices being fully
  powered without ensuring that this is the case, usually by holding an
  active PM runtime reference to the device before accessing it.

Now, there may be places in the core/bus type code that aren't aligned with
the above, so they need to be fixed this way or another.

HTH,
Rafael

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:
> On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:
> > Ulf Hansson <ulf.hansson@linaro.org> writes:
> >
> >> Changes in v3:
> >>       -Rework the entire intermediate step which was suggested in v2.
> >>        That means solving the race condition, but also cope with PM domains
> >>        that are initialized in powered off state.
> >>
> >> Changes in v2:
> >>       -Added some acks.
> >>       -Updated commit messages.
> >>       -Included a wider audience of the patchset. V1 was lacking SoC
> >>        maintainers.
> >>
> >> Here are link to the first patchset, were some discussion started.
> >> http://marc.info/?l=linux-pm&m=141208104729597&w=2
> >>
> >> There may be more than one device in a PM domain which then will be
> >> probed at different points in time.
> >>
> >> Depending on timing and runtime PM support, in for the device related
> >> driver/subsystem, a PM domain may be advised to power off after a
> >> successful probe sequence.
> >>
> >> A general requirement for a device within a PM domain, is that the
> >> PM domain must stay powered during the probe sequence. To cope with
> >> such requirement, let's add two new APIs, dev_pm_domain_get|put().
> >>
> >> These APIs are intended to be invoked from subsystem-level code and the
> >> calls between get/put needs to be balanced.
> >>
> >> dev_pm_domain_get(), tells the PM domain that it needs to increase a
> >> usage count and to keep supplying power. dev_pm_domain_put(), does the
> >> opposite.
> >
> > I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?
> 
> See, below.
> 
> >
> > What's not explained here (or what I'm not understanding) is why a PM
> > domain is powering off if it has active devices.
> 
> It doesn't. The problem is that using pm_runtime_get_sync() in this
> path is not working.
> 
> Now, I failed to include some of the important information from
> previous discussions around this patchset. Let me iterate the patchset
> with better commit messages, but let's first continue this thread.
> 
> Here are some of the previous discussion:
> 
> http://marc.info/?l=linux-pm&m=141270897014653&w=2
> http://marc.info/?l=linux-pm&m=141208104729597&w=2
> 
> Below is a summary of why I think "pm_runtime_get_sync()" isn't working for us.
> 
> 1)
> It's bad practice to use pm_runtime_get_sync() in the ->probe() path,

Honestly, I'm no longer amused.

> to bring your resources to full power. The consequence would be a
> driver that requires CONFIG_PM_RUNTIME to be even functional, which
> just isn't acceptable.

Sorry, but this is utter nonsense.

CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
everything they need to be always on.  If that is not the case, then someone is
doing runtime PM behind the scenes and therefore cheating.  Or in different
words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
everything is on from the drivers' perspective.

If that is the case, then calling pm_runtime_get_sync() from ->probe
for CONFIG_PM_RUNTIME unset simply doesn't matter.

Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
pm_runtime_get_sync() from ->probe is the only way the driver can ensure
in a non-racy way that the device will be accessible going forward.

Why?  Simply because the probing need not happen during system initialization.
It very well may take places when the other devices in the same domain have
beein in use for quite a while and have been using runtime PM (in which
case the domain may go off at any time unless it is explicityly prevented from
doing that).

One thing that you may be missing is that, for CONFIG_PM_RUNTIME set,
runtime PM has to be either enabled or disabled for all devices in one
domain (and if it is disabled, then the domain needs to be always on for
all practical purposes).  Otherwise you can't just make all of them happy
at the same time.  The documentation doesn't cover this, because it had been
written before we even started to consider power domains.

> Drivers that behaves well within this context, follows the runtime PM
> documentation/recommendation.

So please go to Documentation/power/runtime_pm.txt and read it again.  Quote:

"If the default initial runtime PM status of the device (i.e. 'suspended')
reflects the actual state of the device, its bus type's or its driver's
->probe() callback will likely need to wake it up using one of the PM core's
helper functions described in Section 4.  In that case, pm_runtime_resume()
should be used.  Of course, for this purpose the device's runtime PM has to be
enabled earlier by calling pm_runtime_enable()."

So how is this in agreement with what you're saying, I wonder?

> They use pm_runtime_set_active() during ->probe() to reflect that their
> devices are fully powered and capable of handling I/O.

And how the heck can a driver (whose device belongs to a power domain) be sure
that the device is "fully powered and capable of handling I/O" duing ->probe()?

> You may also have a look at these discussions which also touches this
> topic, but within a context of another patchset.
> https://lkml.org/lkml/2014/10/23/95

Which looks like reiterating the same incorrect arguments.

> 2)
> Another good example why pm_runtime_get_sync() is a bad solution to
> our problem, is the amba bus. Before it invokes the driver's ->probe()
> callback it does the following.
> - "enable bus clock"
> - pm_runtime_get_noresume()
> - pm_runtime_set_active()
> - pm_runtime_enable()

Which may not work if power domains are involved.

> For these scenarios a pm_runtime_get_sync() from any of amba driver's
> ->probe() callback wouldn't have any effect, since the device is
> already active. In other words, the resources needs to be "manually"
> enabled.

There seems to be a lot of confusion around this, so let me summarize:

- For CONFIG_PM_RUNTIME unsed drivers should be able to *safely* assume
  that they can access devices at any time.  All of the runtime PM helpers
  don't matter then (up to the error codes returned).

- For CONFIG_PM_RUNTIME set, if power domains are in use, then runtime PM
  has to be either enabled or disabled for all devices in one domain (and
  if disabled, the expected behavior is like the above).

- For CONFIG_PM_RUNTIME set, if power domains are in use, drivers (or bus
  types etc) should not make any assumptions about devices being fully
  powered without ensuring that this is the case, usually by holding an
  active PM runtime reference to the device before accessing it.

Now, there may be places in the core/bus type code that aren't aligned with
the above, so they need to be fixed this way or another.

HTH,
Rafael

Re: [PATCH v3 4/9] drivercore / platform: Keep PM domain powered during ->probe()

[Rafael J. Wysocki]

On Friday, October 31, 2014 10:23:00 AM Ulf Hansson wrote:
> On 31 October 2014 01:07, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > On Thu, Oct 30, 2014 at 01:47:27PM -0700, Kevin Hilman wrote:
> >> Ulf Hansson <ulf.hansson@linaro.org> writes:
> >>
> >> > To sucessfully probe some devices their corresponding PM domains may
> >> > need to be powered.
> >>
> >> Isn't that what pm_runtime_get*() is supposed to be doing?  Why isn't
> >> that working?
> >
> > Also, I do not understand why we placing device into a power domain only
> > when we probe it. Why if I unbind device from its driver (or do not have
> > a driver for it) it disappears from its power domain? To me power domain
> > and having driver bound to a device are 2 orthogonal concepts.
> 
> That's a different discussion, I don't think we want to go there
> within this context.

Why don't we, exactly?

Rafael

[PATCH v3 4/9] drivercore / platform: Keep PM domain powered during ->probe()

[Rafael J. Wysocki]

On Friday, October 31, 2014 10:23:00 AM Ulf Hansson wrote:
> On 31 October 2014 01:07, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > On Thu, Oct 30, 2014 at 01:47:27PM -0700, Kevin Hilman wrote:
> >> Ulf Hansson <ulf.hansson@linaro.org> writes:
> >>
> >> > To sucessfully probe some devices their corresponding PM domains may
> >> > need to be powered.
> >>
> >> Isn't that what pm_runtime_get*() is supposed to be doing?  Why isn't
> >> that working?
> >
> > Also, I do not understand why we placing device into a power domain only
> > when we probe it. Why if I unbind device from its driver (or do not have
> > a driver for it) it disappears from its power domain? To me power domain
> > and having driver bound to a device are 2 orthogonal concepts.
> 
> That's a different discussion, I don't think we want to go there
> within this context.

Why don't we, exactly?

Rafael

pm_runtime_enable() in ->probe() (was: Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot)

[Rafael J. Wysocki]

On Saturday, November 01, 2014 01:20:38 AM Rafael J. Wysocki wrote:
> On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:
> > On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:

[cut]

> > 
> > 1)
> > It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
> 
> Honestly, I'm no longer amused.
> 
> > to bring your resources to full power. The consequence would be a
> > driver that requires CONFIG_PM_RUNTIME to be even functional, which
> > just isn't acceptable.
> 
> Sorry, but this is utter nonsense.
> 
> CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
> everything they need to be always on.  If that is not the case, then someone is
> doing runtime PM behind the scenes and therefore cheating.  Or in different
> words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
> everything is on from the drivers' perspective.
> 
> If that is the case, then calling pm_runtime_get_sync() from ->probe
> for CONFIG_PM_RUNTIME unset simply doesn't matter.
> 
> Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
> pm_runtime_get_sync() from ->probe is the only way the driver can ensure
> in a non-racy way that the device will be accessible going forward.
> 
> Why?  Simply because the probing need not happen during system initialization.
> It very well may take places when the other devices in the same domain have
> beein in use for quite a while and have been using runtime PM (in which
> case the domain may go off at any time unless it is explicityly prevented from
> doing that).
> 
> One thing that you may be missing is that, for CONFIG_PM_RUNTIME set,
> runtime PM has to be either enabled or disabled for all devices in one
> domain (and if it is disabled, then the domain needs to be always on for
> all practical purposes).  Otherwise you can't just make all of them happy
> at the same time.  The documentation doesn't cover this, because it had been
> written before we even started to consider power domains.
> 
> > Drivers that behaves well within this context, follows the runtime PM
> > documentation/recommendation.
> 
> So please go to Documentation/power/runtime_pm.txt and read it again.  Quote:
> 
> "If the default initial runtime PM status of the device (i.e. 'suspended')
> reflects the actual state of the device, its bus type's or its driver's
> ->probe() callback will likely need to wake it up using one of the PM core's
> helper functions described in Section 4.  In that case, pm_runtime_resume()
> should be used.  Of course, for this purpose the device's runtime PM has to be
> enabled earlier by calling pm_runtime_enable()."

All that said there is a logical error related to calling pm_runtime_enable()
and its derivatives from ->probe() that I've been overlooking pretty much
from the start.

Namely, really_probe() sets dev->driver to drv before calling ->probe()
for either the bus type or the driver itself, so if the driver's probe
calls pm_runtime_enable(), it will execute the driver's own runtime PM
resume callback before the driver can check whether or not it wants to handle
the device in the first place.  That doesn't sound quite right to me.

This means we need a different mechanism to ensure that the device will
be accessible to the driver and/or the bus type in ->probe().

At one point we had pm_runtime_get_sync() before really_probe() in
driver_proble_device() IIRC, but people complained about it, so we dropped it
and put a barrier in there instead, but that's not sufficient.  We need to
explicitly ensure that the device won't be powered off while ->probe() is
in progress *but* we need to avoid calling the driver's runtime PM resume
callback before ->probe() returns.

Alan, Kevin, ideas?

Rafael

pm_runtime_enable() in ->probe() (was: Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot)

[Rafael J. Wysocki]

On Saturday, November 01, 2014 01:20:38 AM Rafael J. Wysocki wrote:
> On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:
> > On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:

[cut]

> > 
> > 1)
> > It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
> 
> Honestly, I'm no longer amused.
> 
> > to bring your resources to full power. The consequence would be a
> > driver that requires CONFIG_PM_RUNTIME to be even functional, which
> > just isn't acceptable.
> 
> Sorry, but this is utter nonsense.
> 
> CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
> everything they need to be always on.  If that is not the case, then someone is
> doing runtime PM behind the scenes and therefore cheating.  Or in different
> words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
> everything is on from the drivers' perspective.
> 
> If that is the case, then calling pm_runtime_get_sync() from ->probe
> for CONFIG_PM_RUNTIME unset simply doesn't matter.
> 
> Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
> pm_runtime_get_sync() from ->probe is the only way the driver can ensure
> in a non-racy way that the device will be accessible going forward.
> 
> Why?  Simply because the probing need not happen during system initialization.
> It very well may take places when the other devices in the same domain have
> beein in use for quite a while and have been using runtime PM (in which
> case the domain may go off at any time unless it is explicityly prevented from
> doing that).
> 
> One thing that you may be missing is that, for CONFIG_PM_RUNTIME set,
> runtime PM has to be either enabled or disabled for all devices in one
> domain (and if it is disabled, then the domain needs to be always on for
> all practical purposes).  Otherwise you can't just make all of them happy
> at the same time.  The documentation doesn't cover this, because it had been
> written before we even started to consider power domains.
> 
> > Drivers that behaves well within this context, follows the runtime PM
> > documentation/recommendation.
> 
> So please go to Documentation/power/runtime_pm.txt and read it again.  Quote:
> 
> "If the default initial runtime PM status of the device (i.e. 'suspended')
> reflects the actual state of the device, its bus type's or its driver's
> ->probe() callback will likely need to wake it up using one of the PM core's
> helper functions described in Section 4.  In that case, pm_runtime_resume()
> should be used.  Of course, for this purpose the device's runtime PM has to be
> enabled earlier by calling pm_runtime_enable()."

All that said there is a logical error related to calling pm_runtime_enable()
and its derivatives from ->probe() that I've been overlooking pretty much
from the start.

Namely, really_probe() sets dev->driver to drv before calling ->probe()
for either the bus type or the driver itself, so if the driver's probe
calls pm_runtime_enable(), it will execute the driver's own runtime PM
resume callback before the driver can check whether or not it wants to handle
the device in the first place.  That doesn't sound quite right to me.

This means we need a different mechanism to ensure that the device will
be accessible to the driver and/or the bus type in ->probe().

At one point we had pm_runtime_get_sync() before really_probe() in
driver_proble_device() IIRC, but people complained about it, so we dropped it
and put a barrier in there instead, but that's not sufficient.  We need to
explicitly ensure that the device won't be powered off while ->probe() is
in progress *but* we need to avoid calling the driver's runtime PM resume
callback before ->probe() returns.

Alan, Kevin, ideas?

Rafael

Re: pm_runtime_enable() in ->probe() (was: Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot)

[Rafael J. Wysocki]

On Saturday, November 01, 2014 02:08:57 AM Rafael J. Wysocki wrote:
> On Saturday, November 01, 2014 01:20:38 AM Rafael J. Wysocki wrote:
> > On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:
> > > On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:
> 
> [cut]
> 
> > > 
> > > 1)
> > > It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
> > 
> > Honestly, I'm no longer amused.
> > 
> > > to bring your resources to full power. The consequence would be a
> > > driver that requires CONFIG_PM_RUNTIME to be even functional, which
> > > just isn't acceptable.
> > 
> > Sorry, but this is utter nonsense.
> > 
> > CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
> > everything they need to be always on.  If that is not the case, then someone is
> > doing runtime PM behind the scenes and therefore cheating.  Or in different
> > words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
> > everything is on from the drivers' perspective.
> > 
> > If that is the case, then calling pm_runtime_get_sync() from ->probe
> > for CONFIG_PM_RUNTIME unset simply doesn't matter.
> > 
> > Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
> > pm_runtime_get_sync() from ->probe is the only way the driver can ensure
> > in a non-racy way that the device will be accessible going forward.
> > 
> > Why?  Simply because the probing need not happen during system initialization.
> > It very well may take places when the other devices in the same domain have
> > beein in use for quite a while and have been using runtime PM (in which
> > case the domain may go off at any time unless it is explicityly prevented from
> > doing that).
> > 
> > One thing that you may be missing is that, for CONFIG_PM_RUNTIME set,
> > runtime PM has to be either enabled or disabled for all devices in one
> > domain (and if it is disabled, then the domain needs to be always on for
> > all practical purposes).  Otherwise you can't just make all of them happy
> > at the same time.  The documentation doesn't cover this, because it had been
> > written before we even started to consider power domains.
> > 
> > > Drivers that behaves well within this context, follows the runtime PM
> > > documentation/recommendation.
> > 
> > So please go to Documentation/power/runtime_pm.txt and read it again.  Quote:
> > 
> > "If the default initial runtime PM status of the device (i.e. 'suspended')
> > reflects the actual state of the device, its bus type's or its driver's
> > ->probe() callback will likely need to wake it up using one of the PM core's
> > helper functions described in Section 4.  In that case, pm_runtime_resume()
> > should be used.  Of course, for this purpose the device's runtime PM has to be
> > enabled earlier by calling pm_runtime_enable()."
> 
> All that said there is a logical error related to calling pm_runtime_enable()
> and its derivatives from ->probe() that I've been overlooking pretty much
> from the start.
> 
> Namely, really_probe() sets dev->driver to drv before calling ->probe()
> for either the bus type or the driver itself, so if the driver's probe
> calls pm_runtime_enable(), it will execute the driver's own runtime PM
> resume callback before the driver can check whether or not it wants to handle
> the device in the first place.  That doesn't sound quite right to me.
> 
> This means we need a different mechanism to ensure that the device will
> be accessible to the driver and/or the bus type in ->probe().
> 
> At one point we had pm_runtime_get_sync() before really_probe() in
> driver_proble_device() IIRC, but people complained about it, so we dropped it
> and put a barrier in there instead, but that's not sufficient.

So we actually had pm_runtime_get_noresume() before the barrier, but then
we dropped it due to complaints.

> We need to explicitly ensure that the device won't be powered off while
> ->probe() is in progress *but* we need to avoid calling the driver's runtime
> PM resume callback before ->probe() returns.

I wonder, then, if we just need to do pm_runtime_get_sync() instead of the
barrier and then pm_runtime_put() instead of pm_request_idle() in
driver_probe_device()?

Rafael

pm_runtime_enable() in ->probe() (was: Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot)

[Rafael J. Wysocki]

On Saturday, November 01, 2014 02:08:57 AM Rafael J. Wysocki wrote:
> On Saturday, November 01, 2014 01:20:38 AM Rafael J. Wysocki wrote:
> > On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:
> > > On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:
> 
> [cut]
> 
> > > 
> > > 1)
> > > It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
> > 
> > Honestly, I'm no longer amused.
> > 
> > > to bring your resources to full power. The consequence would be a
> > > driver that requires CONFIG_PM_RUNTIME to be even functional, which
> > > just isn't acceptable.
> > 
> > Sorry, but this is utter nonsense.
> > 
> > CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
> > everything they need to be always on.  If that is not the case, then someone is
> > doing runtime PM behind the scenes and therefore cheating.  Or in different
> > words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
> > everything is on from the drivers' perspective.
> > 
> > If that is the case, then calling pm_runtime_get_sync() from ->probe
> > for CONFIG_PM_RUNTIME unset simply doesn't matter.
> > 
> > Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
> > pm_runtime_get_sync() from ->probe is the only way the driver can ensure
> > in a non-racy way that the device will be accessible going forward.
> > 
> > Why?  Simply because the probing need not happen during system initialization.
> > It very well may take places when the other devices in the same domain have
> > beein in use for quite a while and have been using runtime PM (in which
> > case the domain may go off at any time unless it is explicityly prevented from
> > doing that).
> > 
> > One thing that you may be missing is that, for CONFIG_PM_RUNTIME set,
> > runtime PM has to be either enabled or disabled for all devices in one
> > domain (and if it is disabled, then the domain needs to be always on for
> > all practical purposes).  Otherwise you can't just make all of them happy
> > at the same time.  The documentation doesn't cover this, because it had been
> > written before we even started to consider power domains.
> > 
> > > Drivers that behaves well within this context, follows the runtime PM
> > > documentation/recommendation.
> > 
> > So please go to Documentation/power/runtime_pm.txt and read it again.  Quote:
> > 
> > "If the default initial runtime PM status of the device (i.e. 'suspended')
> > reflects the actual state of the device, its bus type's or its driver's
> > ->probe() callback will likely need to wake it up using one of the PM core's
> > helper functions described in Section 4.  In that case, pm_runtime_resume()
> > should be used.  Of course, for this purpose the device's runtime PM has to be
> > enabled earlier by calling pm_runtime_enable()."
> 
> All that said there is a logical error related to calling pm_runtime_enable()
> and its derivatives from ->probe() that I've been overlooking pretty much
> from the start.
> 
> Namely, really_probe() sets dev->driver to drv before calling ->probe()
> for either the bus type or the driver itself, so if the driver's probe
> calls pm_runtime_enable(), it will execute the driver's own runtime PM
> resume callback before the driver can check whether or not it wants to handle
> the device in the first place.  That doesn't sound quite right to me.
> 
> This means we need a different mechanism to ensure that the device will
> be accessible to the driver and/or the bus type in ->probe().
> 
> At one point we had pm_runtime_get_sync() before really_probe() in
> driver_proble_device() IIRC, but people complained about it, so we dropped it
> and put a barrier in there instead, but that's not sufficient.

So we actually had pm_runtime_get_noresume() before the barrier, but then
we dropped it due to complaints.

> We need to explicitly ensure that the device won't be powered off while
> ->probe() is in progress *but* we need to avoid calling the driver's runtime
> PM resume callback before ->probe() returns.

I wonder, then, if we just need to do pm_runtime_get_sync() instead of the
barrier and then pm_runtime_put() instead of pm_request_idle() in
driver_probe_device()?

Rafael

Re: pm_runtime_enable() in ->probe() (was: Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot)

[Alan Stern]

[CC: list drastically trimmed]

On Sat, 1 Nov 2014, Rafael J. Wysocki wrote:

> All that said there is a logical error related to calling pm_runtime_enable()
> and its derivatives from ->probe() that I've been overlooking pretty much
> from the start.
> 
> Namely, really_probe() sets dev->driver to drv before calling ->probe()
> for either the bus type or the driver itself, so if the driver's probe
> calls pm_runtime_enable(), it will execute the driver's own runtime PM
> resume callback before the driver can check whether or not it wants to handle
> the device in the first place.  That doesn't sound quite right to me.
> 
> This means we need a different mechanism to ensure that the device will
> be accessible to the driver and/or the bus type in ->probe().
> 
> At one point we had pm_runtime_get_sync() before really_probe() in
> driver_proble_device() IIRC, but people complained about it, so we dropped it
> and put a barrier in there instead, but that's not sufficient.  We need to
> explicitly ensure that the device won't be powered off while ->probe() is
> in progress *but* we need to avoid calling the driver's runtime PM resume
> callback before ->probe() returns.
> 
> Alan, Kevin, ideas?

I suspect this cannot be handled solely the driver core or the PM core.  
The bus subsystem has to manage things properly.

For example, take a look at drivers/pci/pci-driver.c:local_pci_probe().  
That routine doesn't set pci_dev->driver until _after_ doing the
pm_runtime_get_sync().  Therefore, if a runtime_resume callback does
occur, the PCI core's runtime_resume handler will see that
pci_dev->driver isn't set and won't invoke the driver's runtime_resume
method.

Things become a little more complicated if the driver has to handle 
everything by itself and the subsystem doesn't support runtime PM.  In 
that case, the driver's runtime_resume routine would have to check and 
see whether the device has successfully been bound to the driver (for 
example, by checking whether dev_get_drvdata() returns a non-NULL 
pointer) before doing anything else.

Note that a mirror version of this problem exists during unbinding,
because dev->driver doesn't get cleared until _after_ the driver's
remove routine has been called.  A runtime suspend callback occurring 
in between could mistakenly get sent to the driver's runtime_suspend 
routine.

Alan Stern

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

On 1 November 2014 01:20, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:
>> On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:
>> > Ulf Hansson <ulf.hansson@linaro.org> writes:
>> >
>> >> Changes in v3:
>> >>       -Rework the entire intermediate step which was suggested in v2.
>> >>        That means solving the race condition, but also cope with PM domains
>> >>        that are initialized in powered off state.
>> >>
>> >> Changes in v2:
>> >>       -Added some acks.
>> >>       -Updated commit messages.
>> >>       -Included a wider audience of the patchset. V1 was lacking SoC
>> >>        maintainers.
>> >>
>> >> Here are link to the first patchset, were some discussion started.
>> >> http://marc.info/?l=linux-pm&m=141208104729597&w=2
>> >>
>> >> There may be more than one device in a PM domain which then will be
>> >> probed at different points in time.
>> >>
>> >> Depending on timing and runtime PM support, in for the device related
>> >> driver/subsystem, a PM domain may be advised to power off after a
>> >> successful probe sequence.
>> >>
>> >> A general requirement for a device within a PM domain, is that the
>> >> PM domain must stay powered during the probe sequence. To cope with
>> >> such requirement, let's add two new APIs, dev_pm_domain_get|put().
>> >>
>> >> These APIs are intended to be invoked from subsystem-level code and the
>> >> calls between get/put needs to be balanced.
>> >>
>> >> dev_pm_domain_get(), tells the PM domain that it needs to increase a
>> >> usage count and to keep supplying power. dev_pm_domain_put(), does the
>> >> opposite.
>> >
>> > I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?
>>
>> See, below.
>>
>> >
>> > What's not explained here (or what I'm not understanding) is why a PM
>> > domain is powering off if it has active devices.
>>
>> It doesn't. The problem is that using pm_runtime_get_sync() in this
>> path is not working.
>>
>> Now, I failed to include some of the important information from
>> previous discussions around this patchset. Let me iterate the patchset
>> with better commit messages, but let's first continue this thread.
>>
>> Here are some of the previous discussion:
>>
>> http://marc.info/?l=linux-pm&m=141270897014653&w=2
>> http://marc.info/?l=linux-pm&m=141208104729597&w=2
>>
>> Below is a summary of why I think "pm_runtime_get_sync()" isn't working for us.
>>
>> 1)
>> It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
>
> Honestly, I'm no longer amused.
>
>> to bring your resources to full power. The consequence would be a
>> driver that requires CONFIG_PM_RUNTIME to be even functional, which
>> just isn't acceptable.
>
> Sorry, but this is utter nonsense.

I admit, I was too vague while stating this. Looking at the big
picture you are obviously right.

I should have referred to those SOCs/buses/drivers that I am working/looking at.

>
> CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
> everything they need to be always on.  If that is not the case, then someone is
> doing runtime PM behind the scenes and therefore cheating.  Or in different
> words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
> everything is on from the drivers' perspective.

I don't think I have stated anything that isn't in agreement with the above?

While I am struggling in making my points clearer, it seems like we
look a bit differently upon how runtime PM are being deployed.

For those drivers I am working on, it's common that these handles
runtime PM resources, like for example clocks. The clocks needs to be
enabled for the driver to handle I/O, but those may also be gated at
request inactivity to save power. That means, the clocks may be
considered as both functional clocks and runtime PM resources.

Therefore, the driver must enable its clocks during ->probe() and
without relying on CONFIG_PM_RUNTIME to be set. Similar to what you
stated for the buses above.

To also cope with the scenario where CONFIG_PM_RUNTIME is set, drivers
must update the device's runtime PM status using
pm_runtime_set_active(), to synchronize the state with the runtime PM
core. Otherwise we will get clock unbalance issues while
gating/ungating the clocks from the runtime PM callbacks.

The AMBA bus and some of its drivers a good example of how this has
been implemented:
driver/amba/bus.c
drivers/mmc/host/mmci.c
drivers/spi/spi-pl022.c

This conclusion I have made from this is:
- Using pm_runtime_get_sync() during the ->probe() path to explicitly
power up a PM domain, is not suitable as the _common_ solution to
solve the race condition. It certainly may work for some scenarios,
but not for those I am looking at.

>
> If that is the case, then calling pm_runtime_get_sync() from ->probe
> for CONFIG_PM_RUNTIME unset simply doesn't matter.
>
> Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
> pm_runtime_get_sync() from ->probe is the only way the driver can ensure
> in a non-racy way that the device will be accessible going forward.
>
> Why?  Simply because the probing need not happen during system initialization.
> It very well may take places when the other devices in the same domain have
> beein in use for quite a while and have been using runtime PM (in which
> case the domain may go off at any time unless it is explicityly prevented from
> doing that).

For PM domains that are initialized in powered off state, we can't
rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to
power on these PM domains. We need a different mechanism, which is
suggested in this v3 patchset.

The requirement of being able to initialize PM domains in powered off
state, was raised during review of v2 of this patchset. I do realize
that's not easy for you to keep track and remember of all discussions.
I apologize for not providing this as the topmost important argument
to why pm_runtime_get_sync() can't be used, in my reply to Kevin.

>
> One thing that you may be missing is that, for CONFIG_PM_RUNTIME set,
> runtime PM has to be either enabled or disabled for all devices in one
> domain (and if it is disabled, then the domain needs to be always on for
> all practical purposes).  Otherwise you can't just make all of them happy
> at the same time.  The documentation doesn't cover this, because it had been
> written before we even started to consider power domains.
>
>> Drivers that behaves well within this context, follows the runtime PM
>> documentation/recommendation.
>
> So please go to Documentation/power/runtime_pm.txt and read it again.  Quote:
>
> "If the default initial runtime PM status of the device (i.e. 'suspended')
> reflects the actual state of the device, its bus type's or its driver's
> ->probe() callback will likely need to wake it up using one of the PM core's
> helper functions described in Section 4.  In that case, pm_runtime_resume()
> should be used.  Of course, for this purpose the device's runtime PM has to be
> enabled earlier by calling pm_runtime_enable()."
>
> So how is this in agreement with what you're saying, I wonder?
>
>> They use pm_runtime_set_active() during ->probe() to reflect that their
>> devices are fully powered and capable of handling I/O.
>
> And how the heck can a driver (whose device belongs to a power domain) be sure
> that the device is "fully powered and capable of handling I/O" duing ->probe()?

Typically drivers expects the bus/subsystem level code to power on
their resources which is needed for it to succeed probing/handling
I/O. I believe this is also in agreement to what you stated earlier
above.

This patchset adopts the behaviour above, for those buses which may
hold devices belonging to PM domains.

That said, maybe the wording of "race conditions" should have been
better picked to better describe the issue I am trying to solve.

>
>> You may also have a look at these discussions which also touches this
>> topic, but within a context of another patchset.
>> https://lkml.org/lkml/2014/10/23/95
>
> Which looks like reiterating the same incorrect arguments.
>
>> 2)
>> Another good example why pm_runtime_get_sync() is a bad solution to
>> our problem, is the amba bus. Before it invokes the driver's ->probe()
>> callback it does the following.
>> - "enable bus clock"
>> - pm_runtime_get_noresume()
>> - pm_runtime_set_active()
>> - pm_runtime_enable()
>
> Which may not work if power domains are involved.
>
>> For these scenarios a pm_runtime_get_sync() from any of amba driver's
>> ->probe() callback wouldn't have any effect, since the device is
>> already active. In other words, the resources needs to be "manually"
>> enabled.
>
> There seems to be a lot of confusion around this, so let me summarize:
>
> - For CONFIG_PM_RUNTIME unsed drivers should be able to *safely* assume
>   that they can access devices at any time.  All of the runtime PM helpers
>   don't matter then (up to the error codes returned).
>
> - For CONFIG_PM_RUNTIME set, if power domains are in use, then runtime PM
>   has to be either enabled or disabled for all devices in one domain (and
>   if disabled, the expected behavior is like the above).
>
> - For CONFIG_PM_RUNTIME set, if power domains are in use, drivers (or bus
>   types etc) should not make any assumptions about devices being fully
>   powered without ensuring that this is the case, usually by holding an
>   active PM runtime reference to the device before accessing it.
>
> Now, there may be places in the core/bus type code that aren't aligned with
> the above, so they need to be fixed this way or another.
>
> HTH,
> Rafael
>

Kind regards
Uffe

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

On 1 November 2014 01:20, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:
>> On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:
>> > Ulf Hansson <ulf.hansson@linaro.org> writes:
>> >
>> >> Changes in v3:
>> >>       -Rework the entire intermediate step which was suggested in v2.
>> >>        That means solving the race condition, but also cope with PM domains
>> >>        that are initialized in powered off state.
>> >>
>> >> Changes in v2:
>> >>       -Added some acks.
>> >>       -Updated commit messages.
>> >>       -Included a wider audience of the patchset. V1 was lacking SoC
>> >>        maintainers.
>> >>
>> >> Here are link to the first patchset, were some discussion started.
>> >> http://marc.info/?l=linux-pm&m=141208104729597&w=2
>> >>
>> >> There may be more than one device in a PM domain which then will be
>> >> probed at different points in time.
>> >>
>> >> Depending on timing and runtime PM support, in for the device related
>> >> driver/subsystem, a PM domain may be advised to power off after a
>> >> successful probe sequence.
>> >>
>> >> A general requirement for a device within a PM domain, is that the
>> >> PM domain must stay powered during the probe sequence. To cope with
>> >> such requirement, let's add two new APIs, dev_pm_domain_get|put().
>> >>
>> >> These APIs are intended to be invoked from subsystem-level code and the
>> >> calls between get/put needs to be balanced.
>> >>
>> >> dev_pm_domain_get(), tells the PM domain that it needs to increase a
>> >> usage count and to keep supplying power. dev_pm_domain_put(), does the
>> >> opposite.
>> >
>> > I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?
>>
>> See, below.
>>
>> >
>> > What's not explained here (or what I'm not understanding) is why a PM
>> > domain is powering off if it has active devices.
>>
>> It doesn't. The problem is that using pm_runtime_get_sync() in this
>> path is not working.
>>
>> Now, I failed to include some of the important information from
>> previous discussions around this patchset. Let me iterate the patchset
>> with better commit messages, but let's first continue this thread.
>>
>> Here are some of the previous discussion:
>>
>> http://marc.info/?l=linux-pm&m=141270897014653&w=2
>> http://marc.info/?l=linux-pm&m=141208104729597&w=2
>>
>> Below is a summary of why I think "pm_runtime_get_sync()" isn't working for us.
>>
>> 1)
>> It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
>
> Honestly, I'm no longer amused.
>
>> to bring your resources to full power. The consequence would be a
>> driver that requires CONFIG_PM_RUNTIME to be even functional, which
>> just isn't acceptable.
>
> Sorry, but this is utter nonsense.

I admit, I was too vague while stating this. Looking at the big
picture you are obviously right.

I should have referred to those SOCs/buses/drivers that I am working/looking at.

>
> CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
> everything they need to be always on.  If that is not the case, then someone is
> doing runtime PM behind the scenes and therefore cheating.  Or in different
> words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
> everything is on from the drivers' perspective.

I don't think I have stated anything that isn't in agreement with the above?

While I am struggling in making my points clearer, it seems like we
look a bit differently upon how runtime PM are being deployed.

For those drivers I am working on, it's common that these handles
runtime PM resources, like for example clocks. The clocks needs to be
enabled for the driver to handle I/O, but those may also be gated at
request inactivity to save power. That means, the clocks may be
considered as both functional clocks and runtime PM resources.

Therefore, the driver must enable its clocks during ->probe() and
without relying on CONFIG_PM_RUNTIME to be set. Similar to what you
stated for the buses above.

To also cope with the scenario where CONFIG_PM_RUNTIME is set, drivers
must update the device's runtime PM status using
pm_runtime_set_active(), to synchronize the state with the runtime PM
core. Otherwise we will get clock unbalance issues while
gating/ungating the clocks from the runtime PM callbacks.

The AMBA bus and some of its drivers a good example of how this has
been implemented:
driver/amba/bus.c
drivers/mmc/host/mmci.c
drivers/spi/spi-pl022.c

This conclusion I have made from this is:
- Using pm_runtime_get_sync() during the ->probe() path to explicitly
power up a PM domain, is not suitable as the _common_ solution to
solve the race condition. It certainly may work for some scenarios,
but not for those I am looking at.

>
> If that is the case, then calling pm_runtime_get_sync() from ->probe
> for CONFIG_PM_RUNTIME unset simply doesn't matter.
>
> Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
> pm_runtime_get_sync() from ->probe is the only way the driver can ensure
> in a non-racy way that the device will be accessible going forward.
>
> Why?  Simply because the probing need not happen during system initialization.
> It very well may take places when the other devices in the same domain have
> beein in use for quite a while and have been using runtime PM (in which
> case the domain may go off at any time unless it is explicityly prevented from
> doing that).

For PM domains that are initialized in powered off state, we can't
rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to
power on these PM domains. We need a different mechanism, which is
suggested in this v3 patchset.

The requirement of being able to initialize PM domains in powered off
state, was raised during review of v2 of this patchset. I do realize
that's not easy for you to keep track and remember of all discussions.
I apologize for not providing this as the topmost important argument
to why pm_runtime_get_sync() can't be used, in my reply to Kevin.

>
> One thing that you may be missing is that, for CONFIG_PM_RUNTIME set,
> runtime PM has to be either enabled or disabled for all devices in one
> domain (and if it is disabled, then the domain needs to be always on for
> all practical purposes).  Otherwise you can't just make all of them happy
> at the same time.  The documentation doesn't cover this, because it had been
> written before we even started to consider power domains.
>
>> Drivers that behaves well within this context, follows the runtime PM
>> documentation/recommendation.
>
> So please go to Documentation/power/runtime_pm.txt and read it again.  Quote:
>
> "If the default initial runtime PM status of the device (i.e. 'suspended')
> reflects the actual state of the device, its bus type's or its driver's
> ->probe() callback will likely need to wake it up using one of the PM core's
> helper functions described in Section 4.  In that case, pm_runtime_resume()
> should be used.  Of course, for this purpose the device's runtime PM has to be
> enabled earlier by calling pm_runtime_enable()."
>
> So how is this in agreement with what you're saying, I wonder?
>
>> They use pm_runtime_set_active() during ->probe() to reflect that their
>> devices are fully powered and capable of handling I/O.
>
> And how the heck can a driver (whose device belongs to a power domain) be sure
> that the device is "fully powered and capable of handling I/O" duing ->probe()?

Typically drivers expects the bus/subsystem level code to power on
their resources which is needed for it to succeed probing/handling
I/O. I believe this is also in agreement to what you stated earlier
above.

This patchset adopts the behaviour above, for those buses which may
hold devices belonging to PM domains.

That said, maybe the wording of "race conditions" should have been
better picked to better describe the issue I am trying to solve.

>
>> You may also have a look at these discussions which also touches this
>> topic, but within a context of another patchset.
>> https://lkml.org/lkml/2014/10/23/95
>
> Which looks like reiterating the same incorrect arguments.
>
>> 2)
>> Another good example why pm_runtime_get_sync() is a bad solution to
>> our problem, is the amba bus. Before it invokes the driver's ->probe()
>> callback it does the following.
>> - "enable bus clock"
>> - pm_runtime_get_noresume()
>> - pm_runtime_set_active()
>> - pm_runtime_enable()
>
> Which may not work if power domains are involved.
>
>> For these scenarios a pm_runtime_get_sync() from any of amba driver's
>> ->probe() callback wouldn't have any effect, since the device is
>> already active. In other words, the resources needs to be "manually"
>> enabled.
>
> There seems to be a lot of confusion around this, so let me summarize:
>
> - For CONFIG_PM_RUNTIME unsed drivers should be able to *safely* assume
>   that they can access devices at any time.  All of the runtime PM helpers
>   don't matter then (up to the error codes returned).
>
> - For CONFIG_PM_RUNTIME set, if power domains are in use, then runtime PM
>   has to be either enabled or disabled for all devices in one domain (and
>   if disabled, the expected behavior is like the above).
>
> - For CONFIG_PM_RUNTIME set, if power domains are in use, drivers (or bus
>   types etc) should not make any assumptions about devices being fully
>   powered without ensuring that this is the case, usually by holding an
>   active PM runtime reference to the device before accessing it.
>
> Now, there may be places in the core/bus type code that aren't aligned with
> the above, so they need to be fixed this way or another.
>
> HTH,
> Rafael
>

Kind regards
Uffe

Re: pm_runtime_enable() in ->probe()

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@rjwysocki.net> writes:

> On Saturday, November 01, 2014 02:08:57 AM Rafael J. Wysocki wrote:
>> On Saturday, November 01, 2014 01:20:38 AM Rafael J. Wysocki wrote:
>> > On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:
>> > > On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:
>> 
>> [cut]
>> 
>> > > 
>> > > 1)
>> > > It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
>> > 
>> > Honestly, I'm no longer amused.
>> > 
>> > > to bring your resources to full power. The consequence would be a
>> > > driver that requires CONFIG_PM_RUNTIME to be even functional, which
>> > > just isn't acceptable.
>> > 
>> > Sorry, but this is utter nonsense.
>> > 
>> > CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
>> > everything they need to be always on.  If that is not the case, then someone is
>> > doing runtime PM behind the scenes and therefore cheating.  Or in different
>> > words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
>> > everything is on from the drivers' perspective.
>> > 
>> > If that is the case, then calling pm_runtime_get_sync() from ->probe
>> > for CONFIG_PM_RUNTIME unset simply doesn't matter.
>> > 
>> > Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
>> > pm_runtime_get_sync() from ->probe is the only way the driver can ensure
>> > in a non-racy way that the device will be accessible going forward.
>> > 
>> > Why?  Simply because the probing need not happen during system initialization.
>> > It very well may take places when the other devices in the same domain have
>> > beein in use for quite a while and have been using runtime PM (in which
>> > case the domain may go off at any time unless it is explicityly prevented from
>> > doing that).
>> > 
>> > One thing that you may be missing is that, for CONFIG_PM_RUNTIME set,
>> > runtime PM has to be either enabled or disabled for all devices in one
>> > domain (and if it is disabled, then the domain needs to be always on for
>> > all practical purposes).  Otherwise you can't just make all of them happy
>> > at the same time.  The documentation doesn't cover this, because it had been
>> > written before we even started to consider power domains.
>> > 
>> > > Drivers that behaves well within this context, follows the runtime PM
>> > > documentation/recommendation.
>> > 
>> > So please go to Documentation/power/runtime_pm.txt and read it again.  Quote:
>> > 
>> > "If the default initial runtime PM status of the device (i.e. 'suspended')
>> > reflects the actual state of the device, its bus type's or its driver's
>> > ->probe() callback will likely need to wake it up using one of the PM core's
>> > helper functions described in Section 4.  In that case, pm_runtime_resume()
>> > should be used.  Of course, for this purpose the device's runtime PM has to be
>> > enabled earlier by calling pm_runtime_enable()."
>> 
>> All that said there is a logical error related to calling pm_runtime_enable()
>> and its derivatives from ->probe() that I've been overlooking pretty much
>> from the start.
>> 
>> Namely, really_probe() sets dev->driver to drv before calling ->probe()
>> for either the bus type or the driver itself, so if the driver's probe
>> calls pm_runtime_enable(), it will execute the driver's own runtime PM
>> resume callback before the driver can check whether or not it wants to handle
>> the device in the first place.  That doesn't sound quite right to me.
>> 
>> This means we need a different mechanism to ensure that the device will
>> be accessible to the driver and/or the bus type in ->probe().
>> 
>> At one point we had pm_runtime_get_sync() before really_probe() in
>> driver_proble_device() IIRC, but people complained about it, so we dropped it
>> and put a barrier in there instead, but that's not sufficient.
>
> So we actually had pm_runtime_get_noresume() before the barrier, but then
> we dropped it due to complaints.
>
>> We need to explicitly ensure that the device won't be powered off while
>> ->probe() is in progress *but* we need to avoid calling the driver's runtime
>> PM resume callback before ->probe() returns.
>
> I wonder, then, if we just need to do pm_runtime_get_sync() instead of the
> barrier and then pm_runtime_put() instead of pm_request_idle() in
> driver_probe_device()?

Won't we still have the same problems in the case of ->probe failure
that were fixed by removing those calls[1]?

IOW, after the driver's ->probe returns failure, it's no longer safe to
call the driver's runtime PM callbacks, since they may be accessing
resources that no longer exits.

Hmm, thinking a little more, maybe this kind of failure is a good time
for the driver to use pm_runtime_force_suspend(), then later when the PM
core does the _put(), it will see the device aleady suspended and there
shouldn't be any problems.

So I think I'm OK with this approach, in theory.

Kevin

[1]
commit eed5d2150752bd08b22333d739f3120151773d28
Author: Rafael J. Wysocki <rjw@sisk.pl>
Date:   Thu Jul 12 11:51:48 2012 +0200

    PM / Runtime: Do not increment device usage counts before probing

    The pm_runtime_get_noresume() calls before really_probe() and
    before executing __device_attach() for each driver on the
    device's bus cause problems to happen if probing fails and if the
    driver has enabled runtime PM for the device in its .probe()
    callback.  Namely, in that case, if the device has been resumed
    by the driver after enabling its runtime PM and if it turns out that
    .probe() should return an error, the driver is supposed to suspend
    the device and disable its runtime PM before exiting .probe().
    However, because the device's runtime PM usage counter was
    incremented by the core before calling .probe(), the driver's attempt
    to suspend the device will not succeed and the device will remain in
    the full-power state after the failing .probe() has returned.

    To fix this issue, remove the pm_runtime_get_noresume() calls from
    driver_probe_device() and from device_attach() and replace the
    corresponding pm_runtime_put_sync() calls with pm_runtime_idle()
    to preserve the existing behavior (which is to check if the device
    is idle and to suspend it eventually in that case after probing).

    Reported-and-tested-by: Kevin Hilman <khilman@ti.com>
    Reviewed-by: Kevin Hilman <khilman@ti.com>
    Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

pm_runtime_enable() in ->probe()

[Kevin Hilman]

"Rafael J. Wysocki" <rjw@rjwysocki.net> writes:

> On Saturday, November 01, 2014 02:08:57 AM Rafael J. Wysocki wrote:
>> On Saturday, November 01, 2014 01:20:38 AM Rafael J. Wysocki wrote:
>> > On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:
>> > > On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:
>> 
>> [cut]
>> 
>> > > 
>> > > 1)
>> > > It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
>> > 
>> > Honestly, I'm no longer amused.
>> > 
>> > > to bring your resources to full power. The consequence would be a
>> > > driver that requires CONFIG_PM_RUNTIME to be even functional, which
>> > > just isn't acceptable.
>> > 
>> > Sorry, but this is utter nonsense.
>> > 
>> > CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
>> > everything they need to be always on.  If that is not the case, then someone is
>> > doing runtime PM behind the scenes and therefore cheating.  Or in different
>> > words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
>> > everything is on from the drivers' perspective.
>> > 
>> > If that is the case, then calling pm_runtime_get_sync() from ->probe
>> > for CONFIG_PM_RUNTIME unset simply doesn't matter.
>> > 
>> > Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
>> > pm_runtime_get_sync() from ->probe is the only way the driver can ensure
>> > in a non-racy way that the device will be accessible going forward.
>> > 
>> > Why?  Simply because the probing need not happen during system initialization.
>> > It very well may take places when the other devices in the same domain have
>> > beein in use for quite a while and have been using runtime PM (in which
>> > case the domain may go off at any time unless it is explicityly prevented from
>> > doing that).
>> > 
>> > One thing that you may be missing is that, for CONFIG_PM_RUNTIME set,
>> > runtime PM has to be either enabled or disabled for all devices in one
>> > domain (and if it is disabled, then the domain needs to be always on for
>> > all practical purposes).  Otherwise you can't just make all of them happy
>> > at the same time.  The documentation doesn't cover this, because it had been
>> > written before we even started to consider power domains.
>> > 
>> > > Drivers that behaves well within this context, follows the runtime PM
>> > > documentation/recommendation.
>> > 
>> > So please go to Documentation/power/runtime_pm.txt and read it again.  Quote:
>> > 
>> > "If the default initial runtime PM status of the device (i.e. 'suspended')
>> > reflects the actual state of the device, its bus type's or its driver's
>> > ->probe() callback will likely need to wake it up using one of the PM core's
>> > helper functions described in Section 4.  In that case, pm_runtime_resume()
>> > should be used.  Of course, for this purpose the device's runtime PM has to be
>> > enabled earlier by calling pm_runtime_enable()."
>> 
>> All that said there is a logical error related to calling pm_runtime_enable()
>> and its derivatives from ->probe() that I've been overlooking pretty much
>> from the start.
>> 
>> Namely, really_probe() sets dev->driver to drv before calling ->probe()
>> for either the bus type or the driver itself, so if the driver's probe
>> calls pm_runtime_enable(), it will execute the driver's own runtime PM
>> resume callback before the driver can check whether or not it wants to handle
>> the device in the first place.  That doesn't sound quite right to me.
>> 
>> This means we need a different mechanism to ensure that the device will
>> be accessible to the driver and/or the bus type in ->probe().
>> 
>> At one point we had pm_runtime_get_sync() before really_probe() in
>> driver_proble_device() IIRC, but people complained about it, so we dropped it
>> and put a barrier in there instead, but that's not sufficient.
>
> So we actually had pm_runtime_get_noresume() before the barrier, but then
> we dropped it due to complaints.
>
>> We need to explicitly ensure that the device won't be powered off while
>> ->probe() is in progress *but* we need to avoid calling the driver's runtime
>> PM resume callback before ->probe() returns.
>
> I wonder, then, if we just need to do pm_runtime_get_sync() instead of the
> barrier and then pm_runtime_put() instead of pm_request_idle() in
> driver_probe_device()?

Won't we still have the same problems in the case of ->probe failure
that were fixed by removing those calls[1]?

IOW, after the driver's ->probe returns failure, it's no longer safe to
call the driver's runtime PM callbacks, since they may be accessing
resources that no longer exits.

Hmm, thinking a little more, maybe this kind of failure is a good time
for the driver to use pm_runtime_force_suspend(), then later when the PM
core does the _put(), it will see the device aleady suspended and there
shouldn't be any problems.

So I think I'm OK with this approach, in theory.

Kevin

[1]
commit eed5d2150752bd08b22333d739f3120151773d28
Author: Rafael J. Wysocki <rjw@sisk.pl>
Date:   Thu Jul 12 11:51:48 2012 +0200

    PM / Runtime: Do not increment device usage counts before probing

    The pm_runtime_get_noresume() calls before really_probe() and
    before executing __device_attach() for each driver on the
    device's bus cause problems to happen if probing fails and if the
    driver has enabled runtime PM for the device in its .probe()
    callback.  Namely, in that case, if the device has been resumed
    by the driver after enabling its runtime PM and if it turns out that
    .probe() should return an error, the driver is supposed to suspend
    the device and disable its runtime PM before exiting .probe().
    However, because the device's runtime PM usage counter was
    incremented by the core before calling .probe(), the driver's attempt
    to suspend the device will not succeed and the device will remain in
    the full-power state after the failing .probe() has returned.

    To fix this issue, remove the pm_runtime_get_noresume() calls from
    driver_probe_device() and from device_attach() and replace the
    corresponding pm_runtime_put_sync() calls with pm_runtime_idle()
    to preserve the existing behavior (which is to check if the device
    is idle and to suspend it eventually in that case after probing).

    Reported-and-tested-by: Kevin Hilman <khilman@ti.com>
    Reviewed-by: Kevin Hilman <khilman@ti.com>
    Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: pm_runtime_enable() in ->probe()

[Alan Stern]

[CC: list drastically trimmed]

On Mon, 3 Nov 2014, Kevin Hilman wrote:

> >> All that said there is a logical error related to calling pm_runtime_enable()
> >> and its derivatives from ->probe() that I've been overlooking pretty much
> >> from the start.
> >> 
> >> Namely, really_probe() sets dev->driver to drv before calling ->probe()
> >> for either the bus type or the driver itself, so if the driver's probe
> >> calls pm_runtime_enable(), it will execute the driver's own runtime PM
> >> resume callback before the driver can check whether or not it wants to handle
> >> the device in the first place.  That doesn't sound quite right to me.
> >> 
> >> This means we need a different mechanism to ensure that the device will
> >> be accessible to the driver and/or the bus type in ->probe().
> >> 
> >> At one point we had pm_runtime_get_sync() before really_probe() in
> >> driver_proble_device() IIRC, but people complained about it, so we dropped it
> >> and put a barrier in there instead, but that's not sufficient.
> >
> > So we actually had pm_runtime_get_noresume() before the barrier, but then
> > we dropped it due to complaints.
> >
> >> We need to explicitly ensure that the device won't be powered off while
> >> ->probe() is in progress *but* we need to avoid calling the driver's runtime
> >> PM resume callback before ->probe() returns.
> >
> > I wonder, then, if we just need to do pm_runtime_get_sync() instead of the
> > barrier and then pm_runtime_put() instead of pm_request_idle() in
> > driver_probe_device()?
> 
> Won't we still have the same problems in the case of ->probe failure
> that were fixed by removing those calls[1]?
> 
> IOW, after the driver's ->probe returns failure, it's no longer safe to
> call the driver's runtime PM callbacks, since they may be accessing
> resources that no longer exits.
> 
> Hmm, thinking a little more, maybe this kind of failure is a good time
> for the driver to use pm_runtime_force_suspend(), then later when the PM
> core does the _put(), it will see the device aleady suspended and there
> shouldn't be any problems.
> 
> So I think I'm OK with this approach, in theory.

I still think this needs to be handled at the level of the subsystem 
and device driver, not at the level of the PM core.  If somebody needs 
to insure that the device won't be powered off while the probe routine 
is running, the subsystem/driver should take care of it.

After all, as Kevin points out, sometimes we really _do_ want the
device to end up powered-down when the probing is finished.  There's
no way the PM core can guess what the driver layer will want in the
end.

Alan Stern

Re: pm_runtime_enable() in ->probe()

[Rafael J. Wysocki]

On Monday, November 03, 2014 09:00:29 AM Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@rjwysocki.net> writes:
> 
> > On Saturday, November 01, 2014 02:08:57 AM Rafael J. Wysocki wrote:
> >> On Saturday, November 01, 2014 01:20:38 AM Rafael J. Wysocki wrote:
> >> > On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:
> >> > > On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:
> >> 
> >> [cut]
> >> 
> >> > > 
> >> > > 1)
> >> > > It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
> >> > 
> >> > Honestly, I'm no longer amused.
> >> > 
> >> > > to bring your resources to full power. The consequence would be a
> >> > > driver that requires CONFIG_PM_RUNTIME to be even functional, which
> >> > > just isn't acceptable.
> >> > 
> >> > Sorry, but this is utter nonsense.
> >> > 
> >> > CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
> >> > everything they need to be always on.  If that is not the case, then someone is
> >> > doing runtime PM behind the scenes and therefore cheating.  Or in different
> >> > words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
> >> > everything is on from the drivers' perspective.
> >> > 
> >> > If that is the case, then calling pm_runtime_get_sync() from ->probe
> >> > for CONFIG_PM_RUNTIME unset simply doesn't matter.
> >> > 
> >> > Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
> >> > pm_runtime_get_sync() from ->probe is the only way the driver can ensure
> >> > in a non-racy way that the device will be accessible going forward.
> >> > 
> >> > Why?  Simply because the probing need not happen during system initialization.
> >> > It very well may take places when the other devices in the same domain have
> >> > beein in use for quite a while and have been using runtime PM (in which
> >> > case the domain may go off at any time unless it is explicityly prevented from
> >> > doing that).
> >> > 
> >> > One thing that you may be missing is that, for CONFIG_PM_RUNTIME set,
> >> > runtime PM has to be either enabled or disabled for all devices in one
> >> > domain (and if it is disabled, then the domain needs to be always on for
> >> > all practical purposes).  Otherwise you can't just make all of them happy
> >> > at the same time.  The documentation doesn't cover this, because it had been
> >> > written before we even started to consider power domains.
> >> > 
> >> > > Drivers that behaves well within this context, follows the runtime PM
> >> > > documentation/recommendation.
> >> > 
> >> > So please go to Documentation/power/runtime_pm.txt and read it again.  Quote:
> >> > 
> >> > "If the default initial runtime PM status of the device (i.e. 'suspended')
> >> > reflects the actual state of the device, its bus type's or its driver's
> >> > ->probe() callback will likely need to wake it up using one of the PM core's
> >> > helper functions described in Section 4.  In that case, pm_runtime_resume()
> >> > should be used.  Of course, for this purpose the device's runtime PM has to be
> >> > enabled earlier by calling pm_runtime_enable()."
> >> 
> >> All that said there is a logical error related to calling pm_runtime_enable()
> >> and its derivatives from ->probe() that I've been overlooking pretty much
> >> from the start.
> >> 
> >> Namely, really_probe() sets dev->driver to drv before calling ->probe()
> >> for either the bus type or the driver itself, so if the driver's probe
> >> calls pm_runtime_enable(), it will execute the driver's own runtime PM
> >> resume callback before the driver can check whether or not it wants to handle
> >> the device in the first place.  That doesn't sound quite right to me.
> >> 
> >> This means we need a different mechanism to ensure that the device will
> >> be accessible to the driver and/or the bus type in ->probe().
> >> 
> >> At one point we had pm_runtime_get_sync() before really_probe() in
> >> driver_proble_device() IIRC, but people complained about it, so we dropped it
> >> and put a barrier in there instead, but that's not sufficient.
> >
> > So we actually had pm_runtime_get_noresume() before the barrier, but then
> > we dropped it due to complaints.
> >
> >> We need to explicitly ensure that the device won't be powered off while
> >> ->probe() is in progress *but* we need to avoid calling the driver's runtime
> >> PM resume callback before ->probe() returns.
> >
> > I wonder, then, if we just need to do pm_runtime_get_sync() instead of the
> > barrier and then pm_runtime_put() instead of pm_request_idle() in
> > driver_probe_device()?
> 
> Won't we still have the same problems in the case of ->probe failure
> that were fixed by removing those calls[1]?

Well, it looks like that might not be the best way to fix the problem.

First of all, I think that drivers can reasonably expect devices to be
accessible when ->probe() runs.  This means that the core and possibly
bus type/PM domain need to make that happen.

For this purpose it may be necessary to power up the device, but doing
that from the bus type's ->probe() requires either (a) not using runtime PM
or (b) bypassing the driver callbacks somehow.  On the other hand, arguably,
if either the bus type or a PM domain provide meaningful runtime suspend/resume
callbacks, then they should be available when driver_probe_device() runs
and it should be fine to run pm_runtime_resume() at that point to trigger
the power-up at least.

Now, pm_runtime_resume() would have been sufficient to power up the device
in driver_probe_device() if it hadn't trigger the "idle" check on return.
So, if you have concerns regarding the reference counting, we could do
something like

	/* Grab a referece on dev to prevent runtime idle from being triggered by resume. */
	pm_runtime_get_sync(dev);
	/* Drop the reference befor probing to make the driver's life easier. */
	pm_runtime_put_noidle(dev);
	ret = really_probe(dev, drv);
	pm_request_idle(dev);

in there.  But quite frankly, I don't see the point, because

	pm_runtime_get_sync(dev);
	ret = really_probe(dev, drv);
	pm_runtime_put(dev);

will suspend the device properly anyway after dropping the reference.

> IOW, after the driver's ->probe returns failure, it's no longer safe to
> call the driver's runtime PM callbacks, since they may be accessing
> resources that no longer exits.

Which we don't do anyway, because if ->probe() fails, dev->driver is cleared
by really_probe().

> Hmm, thinking a little more, maybe this kind of failure is a good time
> for the driver to use pm_runtime_force_suspend(), then later when the PM
> core does the _put(), it will see the device aleady suspended and there
> shouldn't be any problems.

Well, I rather think that ->probe() should leave the device in a state
which is as close as reasonably possible to the state it was in before.

Rafael

pm_runtime_enable() in ->probe()

[Rafael J. Wysocki]

On Monday, November 03, 2014 09:00:29 AM Kevin Hilman wrote:
> "Rafael J. Wysocki" <rjw@rjwysocki.net> writes:
> 
> > On Saturday, November 01, 2014 02:08:57 AM Rafael J. Wysocki wrote:
> >> On Saturday, November 01, 2014 01:20:38 AM Rafael J. Wysocki wrote:
> >> > On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:
> >> > > On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:
> >> 
> >> [cut]
> >> 
> >> > > 
> >> > > 1)
> >> > > It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
> >> > 
> >> > Honestly, I'm no longer amused.
> >> > 
> >> > > to bring your resources to full power. The consequence would be a
> >> > > driver that requires CONFIG_PM_RUNTIME to be even functional, which
> >> > > just isn't acceptable.
> >> > 
> >> > Sorry, but this is utter nonsense.
> >> > 
> >> > CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
> >> > everything they need to be always on.  If that is not the case, then someone is
> >> > doing runtime PM behind the scenes and therefore cheating.  Or in different
> >> > words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
> >> > everything is on from the drivers' perspective.
> >> > 
> >> > If that is the case, then calling pm_runtime_get_sync() from ->probe
> >> > for CONFIG_PM_RUNTIME unset simply doesn't matter.
> >> > 
> >> > Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
> >> > pm_runtime_get_sync() from ->probe is the only way the driver can ensure
> >> > in a non-racy way that the device will be accessible going forward.
> >> > 
> >> > Why?  Simply because the probing need not happen during system initialization.
> >> > It very well may take places when the other devices in the same domain have
> >> > beein in use for quite a while and have been using runtime PM (in which
> >> > case the domain may go off at any time unless it is explicityly prevented from
> >> > doing that).
> >> > 
> >> > One thing that you may be missing is that, for CONFIG_PM_RUNTIME set,
> >> > runtime PM has to be either enabled or disabled for all devices in one
> >> > domain (and if it is disabled, then the domain needs to be always on for
> >> > all practical purposes).  Otherwise you can't just make all of them happy
> >> > at the same time.  The documentation doesn't cover this, because it had been
> >> > written before we even started to consider power domains.
> >> > 
> >> > > Drivers that behaves well within this context, follows the runtime PM
> >> > > documentation/recommendation.
> >> > 
> >> > So please go to Documentation/power/runtime_pm.txt and read it again.  Quote:
> >> > 
> >> > "If the default initial runtime PM status of the device (i.e. 'suspended')
> >> > reflects the actual state of the device, its bus type's or its driver's
> >> > ->probe() callback will likely need to wake it up using one of the PM core's
> >> > helper functions described in Section 4.  In that case, pm_runtime_resume()
> >> > should be used.  Of course, for this purpose the device's runtime PM has to be
> >> > enabled earlier by calling pm_runtime_enable()."
> >> 
> >> All that said there is a logical error related to calling pm_runtime_enable()
> >> and its derivatives from ->probe() that I've been overlooking pretty much
> >> from the start.
> >> 
> >> Namely, really_probe() sets dev->driver to drv before calling ->probe()
> >> for either the bus type or the driver itself, so if the driver's probe
> >> calls pm_runtime_enable(), it will execute the driver's own runtime PM
> >> resume callback before the driver can check whether or not it wants to handle
> >> the device in the first place.  That doesn't sound quite right to me.
> >> 
> >> This means we need a different mechanism to ensure that the device will
> >> be accessible to the driver and/or the bus type in ->probe().
> >> 
> >> At one point we had pm_runtime_get_sync() before really_probe() in
> >> driver_proble_device() IIRC, but people complained about it, so we dropped it
> >> and put a barrier in there instead, but that's not sufficient.
> >
> > So we actually had pm_runtime_get_noresume() before the barrier, but then
> > we dropped it due to complaints.
> >
> >> We need to explicitly ensure that the device won't be powered off while
> >> ->probe() is in progress *but* we need to avoid calling the driver's runtime
> >> PM resume callback before ->probe() returns.
> >
> > I wonder, then, if we just need to do pm_runtime_get_sync() instead of the
> > barrier and then pm_runtime_put() instead of pm_request_idle() in
> > driver_probe_device()?
> 
> Won't we still have the same problems in the case of ->probe failure
> that were fixed by removing those calls[1]?

Well, it looks like that might not be the best way to fix the problem.

First of all, I think that drivers can reasonably expect devices to be
accessible when ->probe() runs.  This means that the core and possibly
bus type/PM domain need to make that happen.

For this purpose it may be necessary to power up the device, but doing
that from the bus type's ->probe() requires either (a) not using runtime PM
or (b) bypassing the driver callbacks somehow.  On the other hand, arguably,
if either the bus type or a PM domain provide meaningful runtime suspend/resume
callbacks, then they should be available when driver_probe_device() runs
and it should be fine to run pm_runtime_resume() at that point to trigger
the power-up at least.

Now, pm_runtime_resume() would have been sufficient to power up the device
in driver_probe_device() if it hadn't trigger the "idle" check on return.
So, if you have concerns regarding the reference counting, we could do
something like

	/* Grab a referece on dev to prevent runtime idle from being triggered by resume. */
	pm_runtime_get_sync(dev);
	/* Drop the reference befor probing to make the driver's life easier. */
	pm_runtime_put_noidle(dev);
	ret = really_probe(dev, drv);
	pm_request_idle(dev);

in there.  But quite frankly, I don't see the point, because

	pm_runtime_get_sync(dev);
	ret = really_probe(dev, drv);
	pm_runtime_put(dev);

will suspend the device properly anyway after dropping the reference.

> IOW, after the driver's ->probe returns failure, it's no longer safe to
> call the driver's runtime PM callbacks, since they may be accessing
> resources that no longer exits.

Which we don't do anyway, because if ->probe() fails, dev->driver is cleared
by really_probe().

> Hmm, thinking a little more, maybe this kind of failure is a good time
> for the driver to use pm_runtime_force_suspend(), then later when the PM
> core does the _put(), it will see the device aleady suspended and there
> shouldn't be any problems.

Well, I rather think that ->probe() should leave the device in a state
which is as close as reasonably possible to the state it was in before.

Rafael

Re: pm_runtime_enable() in ->probe()

[Rafael J. Wysocki]

On Monday, November 03, 2014 03:06:44 PM Alan Stern wrote:
> [CC: list drastically trimmed]
> 
> On Mon, 3 Nov 2014, Kevin Hilman wrote:
> 
> > >> All that said there is a logical error related to calling pm_runtime_enable()
> > >> and its derivatives from ->probe() that I've been overlooking pretty much
> > >> from the start.
> > >> 
> > >> Namely, really_probe() sets dev->driver to drv before calling ->probe()
> > >> for either the bus type or the driver itself, so if the driver's probe
> > >> calls pm_runtime_enable(), it will execute the driver's own runtime PM
> > >> resume callback before the driver can check whether or not it wants to handle
> > >> the device in the first place.  That doesn't sound quite right to me.
> > >> 
> > >> This means we need a different mechanism to ensure that the device will
> > >> be accessible to the driver and/or the bus type in ->probe().
> > >> 
> > >> At one point we had pm_runtime_get_sync() before really_probe() in
> > >> driver_proble_device() IIRC, but people complained about it, so we dropped it
> > >> and put a barrier in there instead, but that's not sufficient.
> > >
> > > So we actually had pm_runtime_get_noresume() before the barrier, but then
> > > we dropped it due to complaints.
> > >
> > >> We need to explicitly ensure that the device won't be powered off while
> > >> ->probe() is in progress *but* we need to avoid calling the driver's runtime
> > >> PM resume callback before ->probe() returns.
> > >
> > > I wonder, then, if we just need to do pm_runtime_get_sync() instead of the
> > > barrier and then pm_runtime_put() instead of pm_request_idle() in
> > > driver_probe_device()?
> > 
> > Won't we still have the same problems in the case of ->probe failure
> > that were fixed by removing those calls[1]?
> > 
> > IOW, after the driver's ->probe returns failure, it's no longer safe to
> > call the driver's runtime PM callbacks, since they may be accessing
> > resources that no longer exits.
> > 
> > Hmm, thinking a little more, maybe this kind of failure is a good time
> > for the driver to use pm_runtime_force_suspend(), then later when the PM
> > core does the _put(), it will see the device aleady suspended and there
> > shouldn't be any problems.
> > 
> > So I think I'm OK with this approach, in theory.
> 
> I still think this needs to be handled at the level of the subsystem 
> and device driver, not at the level of the PM core.  If somebody needs 
> to insure that the device won't be powered off while the probe routine 
> is running, the subsystem/driver should take care of it.

The problem with that is that doing pm_runtime_resume() (or equivalent)
anywhere in the bus type's or driver's ->probe() will trigger the driver's
PM callbacks (for the majority of bus types).

PM domains complicate that even further, because they may override bus type
callbacks.

> After all, as Kevin points out, sometimes we really _do_ want the
> device to end up powered-down when the probing is finished.  There's
> no way the PM core can guess what the driver layer will want in the
> end.

And as I said in a reply to Kevin, if we do

	pm_runtime_get_sync(dev);
	ret = really_probe(dev, drv);
	pm_runtime_put(dev);

in driver_probe_device(), then the last statement will power down the device
properly, unless the driver's ->probe() bumped up the reference counter.

Rafael

Re: pm_runtime_enable() in ->probe() (was: Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot)

[Rafael J. Wysocki]

On Saturday, November 01, 2014 11:15:59 AM Alan Stern wrote:
> [CC: list drastically trimmed]
> 
> On Sat, 1 Nov 2014, Rafael J. Wysocki wrote:
> 
> > All that said there is a logical error related to calling pm_runtime_enable()
> > and its derivatives from ->probe() that I've been overlooking pretty much
> > from the start.
> > 
> > Namely, really_probe() sets dev->driver to drv before calling ->probe()
> > for either the bus type or the driver itself, so if the driver's probe
> > calls pm_runtime_enable(), it will execute the driver's own runtime PM
> > resume callback before the driver can check whether or not it wants to handle
> > the device in the first place.  That doesn't sound quite right to me.
> > 
> > This means we need a different mechanism to ensure that the device will
> > be accessible to the driver and/or the bus type in ->probe().
> > 
> > At one point we had pm_runtime_get_sync() before really_probe() in
> > driver_proble_device() IIRC, but people complained about it, so we dropped it
> > and put a barrier in there instead, but that's not sufficient.  We need to
> > explicitly ensure that the device won't be powered off while ->probe() is
> > in progress *but* we need to avoid calling the driver's runtime PM resume
> > callback before ->probe() returns.
> > 
> > Alan, Kevin, ideas?
> 
> I suspect this cannot be handled solely the driver core or the PM core.  
> The bus subsystem has to manage things properly.
> 
> For example, take a look at drivers/pci/pci-driver.c:local_pci_probe().  
> That routine doesn't set pci_dev->driver until _after_ doing the
> pm_runtime_get_sync().  Therefore, if a runtime_resume callback does
> occur, the PCI core's runtime_resume handler will see that
> pci_dev->driver isn't set and won't invoke the driver's runtime_resume
> method.

Which means that the PCI bus type wouldn't be affected by the change I'm
talking about to my eyes.

> Things become a little more complicated if the driver has to handle 
> everything by itself and the subsystem doesn't support runtime PM.  In 
> that case, the driver's runtime_resume routine would have to check and 
> see whether the device has successfully been bound to the driver (for 
> example, by checking whether dev_get_drvdata() returns a non-NULL 
> pointer) before doing anything else.

Well, I'd rather say that the driver has to initialize runtime PM
properly in that case before trying to use it. :-)

> Note that a mirror version of this problem exists during unbinding,
> because dev->driver doesn't get cleared until _after_ the driver's
> remove routine has been called.  A runtime suspend callback occurring 
> in between could mistakenly get sent to the driver's runtime_suspend 
> routine.

At least the driver can't break things by calling runtime PM helpers from
->remove(), while it can to that by calling them from ->probe() if it is not
careful enough.

Moreover, the bus type can break things by calling runtime PM helpers
from ->probe() if it is not careful enough either.

Rafael

Re: pm_runtime_enable() in ->probe() (was: Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot)

[Rafael J. Wysocki]

On Tuesday, November 04, 2014 01:09:44 AM Rafael J. Wysocki wrote:
> On Saturday, November 01, 2014 11:15:59 AM Alan Stern wrote:
> > [CC: list drastically trimmed]
> > 
> > On Sat, 1 Nov 2014, Rafael J. Wysocki wrote:
> > 
> > > All that said there is a logical error related to calling pm_runtime_enable()
> > > and its derivatives from ->probe() that I've been overlooking pretty much
> > > from the start.
> > > 
> > > Namely, really_probe() sets dev->driver to drv before calling ->probe()
> > > for either the bus type or the driver itself, so if the driver's probe
> > > calls pm_runtime_enable(), it will execute the driver's own runtime PM
> > > resume callback before the driver can check whether or not it wants to handle
> > > the device in the first place.  That doesn't sound quite right to me.
> > > 
> > > This means we need a different mechanism to ensure that the device will
> > > be accessible to the driver and/or the bus type in ->probe().
> > > 
> > > At one point we had pm_runtime_get_sync() before really_probe() in
> > > driver_proble_device() IIRC, but people complained about it, so we dropped it
> > > and put a barrier in there instead, but that's not sufficient.  We need to
> > > explicitly ensure that the device won't be powered off while ->probe() is
> > > in progress *but* we need to avoid calling the driver's runtime PM resume
> > > callback before ->probe() returns.
> > > 
> > > Alan, Kevin, ideas?
> > 
> > I suspect this cannot be handled solely the driver core or the PM core.  
> > The bus subsystem has to manage things properly.
> > 
> > For example, take a look at drivers/pci/pci-driver.c:local_pci_probe().  
> > That routine doesn't set pci_dev->driver until _after_ doing the
> > pm_runtime_get_sync().  Therefore, if a runtime_resume callback does
> > occur, the PCI core's runtime_resume handler will see that
> > pci_dev->driver isn't set and won't invoke the driver's runtime_resume
> > method.
> 
> Which means that the PCI bus type wouldn't be affected by the change I'm
> talking about to my eyes.
> 
> > Things become a little more complicated if the driver has to handle 
> > everything by itself and the subsystem doesn't support runtime PM.  In 
> > that case, the driver's runtime_resume routine would have to check and 
> > see whether the device has successfully been bound to the driver (for 
> > example, by checking whether dev_get_drvdata() returns a non-NULL 
> > pointer) before doing anything else.
> 
> Well, I'd rather say that the driver has to initialize runtime PM
> properly in that case before trying to use it. :-)

And this is important and it looks like I haven't described the problem clearly
enough.

Suppose that the device is in a power domain that was turned off before
probing the driver, whatever the reason.  It needs to but turned on for
the driver to be able to access the device in ->probe() and now the question
is how to do that.

The natural way would be to call pm_runtime_get_sync(dev) at the beginning
of ->probe(), but that would trigger the driver's own ->runtime_resume() to
run and it would require the driver to do some extra checks in that routine.
Those checks wouldn't be necessary at any other time.

However, if the core does pm_runtime_get_sync(dev) before calling
really_probe(), it may turn on the power domain without invoking runtime
PM callbacks from the driver.

Rafael

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Monday, November 03, 2014 03:03:46 PM Ulf Hansson wrote:
> On 1 November 2014 01:20, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:
> >> On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:
> >> > Ulf Hansson <ulf.hansson@linaro.org> writes:
> >> >
> >> >> Changes in v3:
> >> >>       -Rework the entire intermediate step which was suggested in v2.
> >> >>        That means solving the race condition, but also cope with PM domains
> >> >>        that are initialized in powered off state.
> >> >>
> >> >> Changes in v2:
> >> >>       -Added some acks.
> >> >>       -Updated commit messages.
> >> >>       -Included a wider audience of the patchset. V1 was lacking SoC
> >> >>        maintainers.
> >> >>
> >> >> Here are link to the first patchset, were some discussion started.
> >> >> http://marc.info/?l=linux-pm&m=141208104729597&w=2
> >> >>
> >> >> There may be more than one device in a PM domain which then will be
> >> >> probed at different points in time.
> >> >>
> >> >> Depending on timing and runtime PM support, in for the device related
> >> >> driver/subsystem, a PM domain may be advised to power off after a
> >> >> successful probe sequence.
> >> >>
> >> >> A general requirement for a device within a PM domain, is that the
> >> >> PM domain must stay powered during the probe sequence. To cope with
> >> >> such requirement, let's add two new APIs, dev_pm_domain_get|put().
> >> >>
> >> >> These APIs are intended to be invoked from subsystem-level code and the
> >> >> calls between get/put needs to be balanced.
> >> >>
> >> >> dev_pm_domain_get(), tells the PM domain that it needs to increase a
> >> >> usage count and to keep supplying power. dev_pm_domain_put(), does the
> >> >> opposite.
> >> >
> >> > I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?
> >>
> >> See, below.
> >>
> >> >
> >> > What's not explained here (or what I'm not understanding) is why a PM
> >> > domain is powering off if it has active devices.
> >>
> >> It doesn't. The problem is that using pm_runtime_get_sync() in this
> >> path is not working.
> >>
> >> Now, I failed to include some of the important information from
> >> previous discussions around this patchset. Let me iterate the patchset
> >> with better commit messages, but let's first continue this thread.
> >>
> >> Here are some of the previous discussion:
> >>
> >> http://marc.info/?l=linux-pm&m=141270897014653&w=2
> >> http://marc.info/?l=linux-pm&m=141208104729597&w=2
> >>
> >> Below is a summary of why I think "pm_runtime_get_sync()" isn't working for us.
> >>
> >> 1)
> >> It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
> >
> > Honestly, I'm no longer amused.
> >
> >> to bring your resources to full power. The consequence would be a
> >> driver that requires CONFIG_PM_RUNTIME to be even functional, which
> >> just isn't acceptable.
> >
> > Sorry, but this is utter nonsense.
> 
> I admit, I was too vague while stating this. Looking at the big
> picture you are obviously right.
> 
> I should have referred to those SOCs/buses/drivers that I am working/looking at.
> 
> >
> > CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
> > everything they need to be always on.  If that is not the case, then someone is
> > doing runtime PM behind the scenes and therefore cheating.  Or in different
> > words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
> > everything is on from the drivers' perspective.
> 
> I don't think I have stated anything that isn't in agreement with the above?
> 
> While I am struggling in making my points clearer, it seems like we
> look a bit differently upon how runtime PM are being deployed.
> 
> For those drivers I am working on, it's common that these handles
> runtime PM resources, like for example clocks. The clocks needs to be
> enabled for the driver to handle I/O, but those may also be gated at
> request inactivity to save power. That means, the clocks may be
> considered as both functional clocks and runtime PM resources.
> 
> Therefore, the driver must enable its clocks during ->probe() and
> without relying on CONFIG_PM_RUNTIME to be set. Similar to what you
> stated for the buses above.
> 
> To also cope with the scenario where CONFIG_PM_RUNTIME is set, drivers
> must update the device's runtime PM status using
> pm_runtime_set_active(), to synchronize the state with the runtime PM
> core. Otherwise we will get clock unbalance issues while
> gating/ungating the clocks from the runtime PM callbacks.

Generally, there are two or even three levels of runtime PM handling,
driver, (possibly) bus type and (possibly) PM domain (and multiple levels
of these are possible in principle).  All of them have to be initialized
at different times.

Quite arguably, the PM domain and/or bus type runtime PM handling should
be initialized even before registerind the device or during device
registration.  Doing that later may be too late.  When the device has been
registered, runtime PM should work to an extent allowing the driver to access
the device and configure it further after calling pm_runtime_resume().

Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
it must take the fact that the driver's own ->runtime_resume() may be called
as a result of this into account.  That's why I'm asking whether or not the
core should call pm_runtime_resume() before calling really_probe() in a
followup branch of this thread.

The driver's own runtime PM handling must be initialized in the driver and
the only place suitable for that is ->probe().  However, it needs to be done
*before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
is executed.  If that is done properly, it should be possible to cover
both the CONFIG_PM_RUNTIME set/unset cases in that code.

And I wouldn't recommend anyone to do the runtime PM initialization in
->runtime_resume() (when it is called for the first time), as that would be
error prone and fragile.

> The AMBA bus and some of its drivers a good example of how this has
> been implemented:
> driver/amba/bus.c
> drivers/mmc/host/mmci.c
> drivers/spi/spi-pl022.c
> 
> This conclusion I have made from this is:
> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
> power up a PM domain, is not suitable as the _common_ solution to
> solve the race condition. It certainly may work for some scenarios,
> but not for those I am looking at.

I think, however, that it might work if the core calls pm_runtime_get_sync()
from driver_probe_device().

> >
> > If that is the case, then calling pm_runtime_get_sync() from ->probe
> > for CONFIG_PM_RUNTIME unset simply doesn't matter.
> >
> > Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
> > pm_runtime_get_sync() from ->probe is the only way the driver can ensure
> > in a non-racy way that the device will be accessible going forward.
> >
> > Why?  Simply because the probing need not happen during system initialization.
> > It very well may take places when the other devices in the same domain have
> > beein in use for quite a while and have been using runtime PM (in which
> > case the domain may go off at any time unless it is explicityly prevented from
> > doing that).
> 
> For PM domains that are initialized in powered off state, we can't
> rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to
> power on these PM domains. We need a different mechanism, which is
> suggested in this v3 patchset.

That is quite simple to address, though.  You can register a bus type
notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
(where the target device belongs to the domain), and do that only for
CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).

> The requirement of being able to initialize PM domains in powered off
> state, was raised during review of v2 of this patchset. I do realize
> that's not easy for you to keep track and remember of all discussions.
> I apologize for not providing this as the topmost important argument
> to why pm_runtime_get_sync() can't be used, in my reply to Kevin.

OK

It looks like we need an agreement on what to do and what the expectations
should be for each driver core operation at the high level.  That is,
what ->probe() should expect, what ->remove() should expect, what the
state after executing each of them should be for CONFIG_PM_RUNTIME set and
unset etc.  Otherwise we'll always have various bus types doing what *they*
think is appropriate and not agreeing with each other.

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Monday, November 03, 2014 03:03:46 PM Ulf Hansson wrote:
> On 1 November 2014 01:20, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:
> >> On 24 October 2014 18:12, Kevin Hilman <khilman@kernel.org> wrote:
> >> > Ulf Hansson <ulf.hansson@linaro.org> writes:
> >> >
> >> >> Changes in v3:
> >> >>       -Rework the entire intermediate step which was suggested in v2.
> >> >>        That means solving the race condition, but also cope with PM domains
> >> >>        that are initialized in powered off state.
> >> >>
> >> >> Changes in v2:
> >> >>       -Added some acks.
> >> >>       -Updated commit messages.
> >> >>       -Included a wider audience of the patchset. V1 was lacking SoC
> >> >>        maintainers.
> >> >>
> >> >> Here are link to the first patchset, were some discussion started.
> >> >> http://marc.info/?l=linux-pm&m=141208104729597&w=2
> >> >>
> >> >> There may be more than one device in a PM domain which then will be
> >> >> probed at different points in time.
> >> >>
> >> >> Depending on timing and runtime PM support, in for the device related
> >> >> driver/subsystem, a PM domain may be advised to power off after a
> >> >> successful probe sequence.
> >> >>
> >> >> A general requirement for a device within a PM domain, is that the
> >> >> PM domain must stay powered during the probe sequence. To cope with
> >> >> such requirement, let's add two new APIs, dev_pm_domain_get|put().
> >> >>
> >> >> These APIs are intended to be invoked from subsystem-level code and the
> >> >> calls between get/put needs to be balanced.
> >> >>
> >> >> dev_pm_domain_get(), tells the PM domain that it needs to increase a
> >> >> usage count and to keep supplying power. dev_pm_domain_put(), does the
> >> >> opposite.
> >> >
> >> > I'm confused. Why arent' pm_runtime_get*() and pm_runtime_put*() working?
> >>
> >> See, below.
> >>
> >> >
> >> > What's not explained here (or what I'm not understanding) is why a PM
> >> > domain is powering off if it has active devices.
> >>
> >> It doesn't. The problem is that using pm_runtime_get_sync() in this
> >> path is not working.
> >>
> >> Now, I failed to include some of the important information from
> >> previous discussions around this patchset. Let me iterate the patchset
> >> with better commit messages, but let's first continue this thread.
> >>
> >> Here are some of the previous discussion:
> >>
> >> http://marc.info/?l=linux-pm&m=141270897014653&w=2
> >> http://marc.info/?l=linux-pm&m=141208104729597&w=2
> >>
> >> Below is a summary of why I think "pm_runtime_get_sync()" isn't working for us.
> >>
> >> 1)
> >> It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
> >
> > Honestly, I'm no longer amused.
> >
> >> to bring your resources to full power. The consequence would be a
> >> driver that requires CONFIG_PM_RUNTIME to be even functional, which
> >> just isn't acceptable.
> >
> > Sorry, but this is utter nonsense.
> 
> I admit, I was too vague while stating this. Looking at the big
> picture you are obviously right.
> 
> I should have referred to those SOCs/buses/drivers that I am working/looking at.
> 
> >
> > CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
> > everything they need to be always on.  If that is not the case, then someone is
> > doing runtime PM behind the scenes and therefore cheating.  Or in different
> > words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
> > everything is on from the drivers' perspective.
> 
> I don't think I have stated anything that isn't in agreement with the above?
> 
> While I am struggling in making my points clearer, it seems like we
> look a bit differently upon how runtime PM are being deployed.
> 
> For those drivers I am working on, it's common that these handles
> runtime PM resources, like for example clocks. The clocks needs to be
> enabled for the driver to handle I/O, but those may also be gated at
> request inactivity to save power. That means, the clocks may be
> considered as both functional clocks and runtime PM resources.
> 
> Therefore, the driver must enable its clocks during ->probe() and
> without relying on CONFIG_PM_RUNTIME to be set. Similar to what you
> stated for the buses above.
> 
> To also cope with the scenario where CONFIG_PM_RUNTIME is set, drivers
> must update the device's runtime PM status using
> pm_runtime_set_active(), to synchronize the state with the runtime PM
> core. Otherwise we will get clock unbalance issues while
> gating/ungating the clocks from the runtime PM callbacks.

Generally, there are two or even three levels of runtime PM handling,
driver, (possibly) bus type and (possibly) PM domain (and multiple levels
of these are possible in principle).  All of them have to be initialized
at different times.

Quite arguably, the PM domain and/or bus type runtime PM handling should
be initialized even before registerind the device or during device
registration.  Doing that later may be too late.  When the device has been
registered, runtime PM should work to an extent allowing the driver to access
the device and configure it further after calling pm_runtime_resume().

Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
it must take the fact that the driver's own ->runtime_resume() may be called
as a result of this into account.  That's why I'm asking whether or not the
core should call pm_runtime_resume() before calling really_probe() in a
followup branch of this thread.

The driver's own runtime PM handling must be initialized in the driver and
the only place suitable for that is ->probe().  However, it needs to be done
*before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
is executed.  If that is done properly, it should be possible to cover
both the CONFIG_PM_RUNTIME set/unset cases in that code.

And I wouldn't recommend anyone to do the runtime PM initialization in
->runtime_resume() (when it is called for the first time), as that would be
error prone and fragile.

> The AMBA bus and some of its drivers a good example of how this has
> been implemented:
> driver/amba/bus.c
> drivers/mmc/host/mmci.c
> drivers/spi/spi-pl022.c
> 
> This conclusion I have made from this is:
> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
> power up a PM domain, is not suitable as the _common_ solution to
> solve the race condition. It certainly may work for some scenarios,
> but not for those I am looking at.

I think, however, that it might work if the core calls pm_runtime_get_sync()
from driver_probe_device().

> >
> > If that is the case, then calling pm_runtime_get_sync() from ->probe
> > for CONFIG_PM_RUNTIME unset simply doesn't matter.
> >
> > Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
> > pm_runtime_get_sync() from ->probe is the only way the driver can ensure
> > in a non-racy way that the device will be accessible going forward.
> >
> > Why?  Simply because the probing need not happen during system initialization.
> > It very well may take places when the other devices in the same domain have
> > beein in use for quite a while and have been using runtime PM (in which
> > case the domain may go off at any time unless it is explicityly prevented from
> > doing that).
> 
> For PM domains that are initialized in powered off state, we can't
> rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to
> power on these PM domains. We need a different mechanism, which is
> suggested in this v3 patchset.

That is quite simple to address, though.  You can register a bus type
notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
(where the target device belongs to the domain), and do that only for
CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).

> The requirement of being able to initialize PM domains in powered off
> state, was raised during review of v2 of this patchset. I do realize
> that's not easy for you to keep track and remember of all discussions.
> I apologize for not providing this as the topmost important argument
> to why pm_runtime_get_sync() can't be used, in my reply to Kevin.

OK

It looks like we need an agreement on what to do and what the expectations
should be for each driver core operation at the high level.  That is,
what ->probe() should expect, what ->remove() should expect, what the
state after executing each of them should be for CONFIG_PM_RUNTIME set and
unset etc.  Otherwise we'll always have various bus types doing what *they*
think is appropriate and not agreeing with each other.

--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Geert Uytterhoeven]

On Tue, Nov 4, 2014 at 2:43 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> That is quite simple to address, though.  You can register a bus type
> notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
> (where the target device belongs to the domain), and do that only for
> CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).

Registering a bus type notifier (that will be a platform bus notifier,
right?) will affect
all platform devices, not only the on-SoC devices.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Geert Uytterhoeven]

On Tue, Nov 4, 2014 at 2:43 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> That is quite simple to address, though.  You can register a bus type
> notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
> (where the target device belongs to the domain), and do that only for
> CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).

Registering a bus type notifier (that will be a platform bus notifier,
right?) will affect
all platform devices, not only the on-SoC devices.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

[...]

> Generally, there are two or even three levels of runtime PM handling,
> driver, (possibly) bus type and (possibly) PM domain (and multiple levels
> of these are possible in principle).  All of them have to be initialized
> at different times.
>
> Quite arguably, the PM domain and/or bus type runtime PM handling should
> be initialized even before registerind the device or during device
> registration.  Doing that later may be too late.  When the device has been
> registered, runtime PM should work to an extent allowing the driver to access
> the device and configure it further after calling pm_runtime_resume().
>
> Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
> it must take the fact that the driver's own ->runtime_resume() may be called
> as a result of this into account.  That's why I'm asking whether or not the
> core should call pm_runtime_resume() before calling really_probe() in a
> followup branch of this thread.

I am reading the other thread, let's see.

>
> The driver's own runtime PM handling must be initialized in the driver and
> the only place suitable for that is ->probe().  However, it needs to be done
> *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
> is executed.  If that is done properly, it should be possible to cover
> both the CONFIG_PM_RUNTIME set/unset cases in that code.
>
> And I wouldn't recommend anyone to do the runtime PM initialization in
> ->runtime_resume() (when it is called for the first time), as that would be
> error prone and fragile.

Great! That's means we are at least aligned on this topic. :-)

>
>> The AMBA bus and some of its drivers a good example of how this has
>> been implemented:
>> driver/amba/bus.c
>> drivers/mmc/host/mmci.c
>> drivers/spi/spi-pl022.c
>>
>> This conclusion I have made from this is:
>> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
>> power up a PM domain, is not suitable as the _common_ solution to
>> solve the race condition. It certainly may work for some scenarios,
>> but not for those I am looking at.
>
> I think, however, that it might work if the core calls pm_runtime_get_sync()
> from driver_probe_device().

Currently this won't work.

That's because the buses' ->probe() are invoked in this path and they
are doing the attachment of the device to its PM domain.

In other words, we can't power up the PM domain using
pm_runtime_get_sync(), until the device has been attached to its PM
domain. Right?

[...]

>> For PM domains that are initialized in powered off state, we can't
>> rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to
>> power on these PM domains. We need a different mechanism, which is
>> suggested in this v3 patchset.
>
> That is quite simple to address, though.  You can register a bus type
> notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
> (where the target device belongs to the domain), and do that only for
> CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).

I guess we could use notifiers, but I am not sure I see any benefit.
The code will be more complex and we need error handling as well.

>
>> The requirement of being able to initialize PM domains in powered off
>> state, was raised during review of v2 of this patchset. I do realize
>> that's not easy for you to keep track and remember of all discussions.
>> I apologize for not providing this as the topmost important argument
>> to why pm_runtime_get_sync() can't be used, in my reply to Kevin.
>
> OK
>
> It looks like we need an agreement on what to do and what the expectations
> should be for each driver core operation at the high level.  That is,
> what ->probe() should expect, what ->remove() should expect, what the
> state after executing each of them should be for CONFIG_PM_RUNTIME set and
> unset etc.  Otherwise we'll always have various bus types doing what *they*
> think is appropriate and not agreeing with each other.
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Kind regards
Uffe

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

[...]

> Generally, there are two or even three levels of runtime PM handling,
> driver, (possibly) bus type and (possibly) PM domain (and multiple levels
> of these are possible in principle).  All of them have to be initialized
> at different times.
>
> Quite arguably, the PM domain and/or bus type runtime PM handling should
> be initialized even before registerind the device or during device
> registration.  Doing that later may be too late.  When the device has been
> registered, runtime PM should work to an extent allowing the driver to access
> the device and configure it further after calling pm_runtime_resume().
>
> Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
> it must take the fact that the driver's own ->runtime_resume() may be called
> as a result of this into account.  That's why I'm asking whether or not the
> core should call pm_runtime_resume() before calling really_probe() in a
> followup branch of this thread.

I am reading the other thread, let's see.

>
> The driver's own runtime PM handling must be initialized in the driver and
> the only place suitable for that is ->probe().  However, it needs to be done
> *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
> is executed.  If that is done properly, it should be possible to cover
> both the CONFIG_PM_RUNTIME set/unset cases in that code.
>
> And I wouldn't recommend anyone to do the runtime PM initialization in
> ->runtime_resume() (when it is called for the first time), as that would be
> error prone and fragile.

Great! That's means we are at least aligned on this topic. :-)

>
>> The AMBA bus and some of its drivers a good example of how this has
>> been implemented:
>> driver/amba/bus.c
>> drivers/mmc/host/mmci.c
>> drivers/spi/spi-pl022.c
>>
>> This conclusion I have made from this is:
>> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
>> power up a PM domain, is not suitable as the _common_ solution to
>> solve the race condition. It certainly may work for some scenarios,
>> but not for those I am looking at.
>
> I think, however, that it might work if the core calls pm_runtime_get_sync()
> from driver_probe_device().

Currently this won't work.

That's because the buses' ->probe() are invoked in this path and they
are doing the attachment of the device to its PM domain.

In other words, we can't power up the PM domain using
pm_runtime_get_sync(), until the device has been attached to its PM
domain. Right?

[...]

>> For PM domains that are initialized in powered off state, we can't
>> rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to
>> power on these PM domains. We need a different mechanism, which is
>> suggested in this v3 patchset.
>
> That is quite simple to address, though.  You can register a bus type
> notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
> (where the target device belongs to the domain), and do that only for
> CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).

I guess we could use notifiers, but I am not sure I see any benefit.
The code will be more complex and we need error handling as well.

>
>> The requirement of being able to initialize PM domains in powered off
>> state, was raised during review of v2 of this patchset. I do realize
>> that's not easy for you to keep track and remember of all discussions.
>> I apologize for not providing this as the topmost important argument
>> to why pm_runtime_get_sync() can't be used, in my reply to Kevin.
>
> OK
>
> It looks like we need an agreement on what to do and what the expectations
> should be for each driver core operation at the high level.  That is,
> what ->probe() should expect, what ->remove() should expect, what the
> state after executing each of them should be for CONFIG_PM_RUNTIME set and
> unset etc.  Otherwise we'll always have various bus types doing what *they*
> think is appropriate and not agreeing with each other.
>
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

Kind regards
Uffe

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Dmitry Torokhov]

On Tue, Nov 04, 2014 at 09:54:19AM +0100, Ulf Hansson wrote:
> [...]
> 
> > Generally, there are two or even three levels of runtime PM handling,
> > driver, (possibly) bus type and (possibly) PM domain (and multiple levels
> > of these are possible in principle).  All of them have to be initialized
> > at different times.
> >
> > Quite arguably, the PM domain and/or bus type runtime PM handling should
> > be initialized even before registerind the device or during device
> > registration.  Doing that later may be too late.  When the device has been
> > registered, runtime PM should work to an extent allowing the driver to access
> > the device and configure it further after calling pm_runtime_resume().
> >
> > Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
> > it must take the fact that the driver's own ->runtime_resume() may be called
> > as a result of this into account.  That's why I'm asking whether or not the
> > core should call pm_runtime_resume() before calling really_probe() in a
> > followup branch of this thread.
> 
> I am reading the other thread, let's see.
> 
> >
> > The driver's own runtime PM handling must be initialized in the driver and
> > the only place suitable for that is ->probe().  However, it needs to be done
> > *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
> > is executed.  If that is done properly, it should be possible to cover
> > both the CONFIG_PM_RUNTIME set/unset cases in that code.
> >
> > And I wouldn't recommend anyone to do the runtime PM initialization in
> > ->runtime_resume() (when it is called for the first time), as that would be
> > error prone and fragile.
> 
> Great! That's means we are at least aligned on this topic. :-)
> 
> >
> >> The AMBA bus and some of its drivers a good example of how this has
> >> been implemented:
> >> driver/amba/bus.c
> >> drivers/mmc/host/mmci.c
> >> drivers/spi/spi-pl022.c
> >>
> >> This conclusion I have made from this is:
> >> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
> >> power up a PM domain, is not suitable as the _common_ solution to
> >> solve the race condition. It certainly may work for some scenarios,
> >> but not for those I am looking at.
> >
> > I think, however, that it might work if the core calls pm_runtime_get_sync()
> > from driver_probe_device().
> 
> Currently this won't work.
> 
> That's because the buses' ->probe() are invoked in this path and they
> are doing the attachment of the device to its PM domain.
> 
> In other words, we can't power up the PM domain using
> pm_runtime_get_sync(), until the device has been attached to its PM
> domain. Right?


I think this is one of the issues that we have there. Why do we conflate
probing and placing the device into a power domain? The latter should
happen when we register the device. The fact that a device was probed
and has a driver bound or not bound to it should have no bearing on
whether the device is member of power domain or not.

Thanks.

-- 
Dmitry

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Dmitry Torokhov]

On Tue, Nov 04, 2014 at 09:54:19AM +0100, Ulf Hansson wrote:
> [...]
> 
> > Generally, there are two or even three levels of runtime PM handling,
> > driver, (possibly) bus type and (possibly) PM domain (and multiple levels
> > of these are possible in principle).  All of them have to be initialized
> > at different times.
> >
> > Quite arguably, the PM domain and/or bus type runtime PM handling should
> > be initialized even before registerind the device or during device
> > registration.  Doing that later may be too late.  When the device has been
> > registered, runtime PM should work to an extent allowing the driver to access
> > the device and configure it further after calling pm_runtime_resume().
> >
> > Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
> > it must take the fact that the driver's own ->runtime_resume() may be called
> > as a result of this into account.  That's why I'm asking whether or not the
> > core should call pm_runtime_resume() before calling really_probe() in a
> > followup branch of this thread.
> 
> I am reading the other thread, let's see.
> 
> >
> > The driver's own runtime PM handling must be initialized in the driver and
> > the only place suitable for that is ->probe().  However, it needs to be done
> > *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
> > is executed.  If that is done properly, it should be possible to cover
> > both the CONFIG_PM_RUNTIME set/unset cases in that code.
> >
> > And I wouldn't recommend anyone to do the runtime PM initialization in
> > ->runtime_resume() (when it is called for the first time), as that would be
> > error prone and fragile.
> 
> Great! That's means we are at least aligned on this topic. :-)
> 
> >
> >> The AMBA bus and some of its drivers a good example of how this has
> >> been implemented:
> >> driver/amba/bus.c
> >> drivers/mmc/host/mmci.c
> >> drivers/spi/spi-pl022.c
> >>
> >> This conclusion I have made from this is:
> >> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
> >> power up a PM domain, is not suitable as the _common_ solution to
> >> solve the race condition. It certainly may work for some scenarios,
> >> but not for those I am looking at.
> >
> > I think, however, that it might work if the core calls pm_runtime_get_sync()
> > from driver_probe_device().
> 
> Currently this won't work.
> 
> That's because the buses' ->probe() are invoked in this path and they
> are doing the attachment of the device to its PM domain.
> 
> In other words, we can't power up the PM domain using
> pm_runtime_get_sync(), until the device has been attached to its PM
> domain. Right?


I think this is one of the issues that we have there. Why do we conflate
probing and placing the device into a power domain? The latter should
happen when we register the device. The fact that a device was probed
and has a driver bound or not bound to it should have no bearing on
whether the device is member of power domain or not.

Thanks.

-- 
Dmitry

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

On 4 November 2014 10:05, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Tue, Nov 04, 2014 at 09:54:19AM +0100, Ulf Hansson wrote:
>> [...]
>>
>> > Generally, there are two or even three levels of runtime PM handling,
>> > driver, (possibly) bus type and (possibly) PM domain (and multiple levels
>> > of these are possible in principle).  All of them have to be initialized
>> > at different times.
>> >
>> > Quite arguably, the PM domain and/or bus type runtime PM handling should
>> > be initialized even before registerind the device or during device
>> > registration.  Doing that later may be too late.  When the device has been
>> > registered, runtime PM should work to an extent allowing the driver to access
>> > the device and configure it further after calling pm_runtime_resume().
>> >
>> > Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
>> > it must take the fact that the driver's own ->runtime_resume() may be called
>> > as a result of this into account.  That's why I'm asking whether or not the
>> > core should call pm_runtime_resume() before calling really_probe() in a
>> > followup branch of this thread.
>>
>> I am reading the other thread, let's see.
>>
>> >
>> > The driver's own runtime PM handling must be initialized in the driver and
>> > the only place suitable for that is ->probe().  However, it needs to be done
>> > *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
>> > is executed.  If that is done properly, it should be possible to cover
>> > both the CONFIG_PM_RUNTIME set/unset cases in that code.
>> >
>> > And I wouldn't recommend anyone to do the runtime PM initialization in
>> > ->runtime_resume() (when it is called for the first time), as that would be
>> > error prone and fragile.
>>
>> Great! That's means we are at least aligned on this topic. :-)
>>
>> >
>> >> The AMBA bus and some of its drivers a good example of how this has
>> >> been implemented:
>> >> driver/amba/bus.c
>> >> drivers/mmc/host/mmci.c
>> >> drivers/spi/spi-pl022.c
>> >>
>> >> This conclusion I have made from this is:
>> >> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
>> >> power up a PM domain, is not suitable as the _common_ solution to
>> >> solve the race condition. It certainly may work for some scenarios,
>> >> but not for those I am looking at.
>> >
>> > I think, however, that it might work if the core calls pm_runtime_get_sync()
>> > from driver_probe_device().
>>
>> Currently this won't work.
>>
>> That's because the buses' ->probe() are invoked in this path and they
>> are doing the attachment of the device to its PM domain.
>>
>> In other words, we can't power up the PM domain using
>> pm_runtime_get_sync(), until the device has been attached to its PM
>> domain. Right?
>
>
> I think this is one of the issues that we have there. Why do we conflate
> probing and placing the device into a power domain? The latter should
> happen when we register the device. The fact that a device was probed
> and has a driver bound or not bound to it should have no bearing on
> whether the device is member of power domain or not.

Devices that are created while "discoverable buses" are being probed
can't be attached to a PM domain before the probing is done, because
those simply doesn't exist.

Now, I haven't yet seen a demand for such a cases, but it seems wrong
to not consider them. The current solution cover these.

Kind regards
Uffe

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

On 4 November 2014 10:05, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Tue, Nov 04, 2014 at 09:54:19AM +0100, Ulf Hansson wrote:
>> [...]
>>
>> > Generally, there are two or even three levels of runtime PM handling,
>> > driver, (possibly) bus type and (possibly) PM domain (and multiple levels
>> > of these are possible in principle).  All of them have to be initialized
>> > at different times.
>> >
>> > Quite arguably, the PM domain and/or bus type runtime PM handling should
>> > be initialized even before registerind the device or during device
>> > registration.  Doing that later may be too late.  When the device has been
>> > registered, runtime PM should work to an extent allowing the driver to access
>> > the device and configure it further after calling pm_runtime_resume().
>> >
>> > Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
>> > it must take the fact that the driver's own ->runtime_resume() may be called
>> > as a result of this into account.  That's why I'm asking whether or not the
>> > core should call pm_runtime_resume() before calling really_probe() in a
>> > followup branch of this thread.
>>
>> I am reading the other thread, let's see.
>>
>> >
>> > The driver's own runtime PM handling must be initialized in the driver and
>> > the only place suitable for that is ->probe().  However, it needs to be done
>> > *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
>> > is executed.  If that is done properly, it should be possible to cover
>> > both the CONFIG_PM_RUNTIME set/unset cases in that code.
>> >
>> > And I wouldn't recommend anyone to do the runtime PM initialization in
>> > ->runtime_resume() (when it is called for the first time), as that would be
>> > error prone and fragile.
>>
>> Great! That's means we are at least aligned on this topic. :-)
>>
>> >
>> >> The AMBA bus and some of its drivers a good example of how this has
>> >> been implemented:
>> >> driver/amba/bus.c
>> >> drivers/mmc/host/mmci.c
>> >> drivers/spi/spi-pl022.c
>> >>
>> >> This conclusion I have made from this is:
>> >> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
>> >> power up a PM domain, is not suitable as the _common_ solution to
>> >> solve the race condition. It certainly may work for some scenarios,
>> >> but not for those I am looking at.
>> >
>> > I think, however, that it might work if the core calls pm_runtime_get_sync()
>> > from driver_probe_device().
>>
>> Currently this won't work.
>>
>> That's because the buses' ->probe() are invoked in this path and they
>> are doing the attachment of the device to its PM domain.
>>
>> In other words, we can't power up the PM domain using
>> pm_runtime_get_sync(), until the device has been attached to its PM
>> domain. Right?
>
>
> I think this is one of the issues that we have there. Why do we conflate
> probing and placing the device into a power domain? The latter should
> happen when we register the device. The fact that a device was probed
> and has a driver bound or not bound to it should have no bearing on
> whether the device is member of power domain or not.

Devices that are created while "discoverable buses" are being probed
can't be attached to a PM domain before the probing is done, because
those simply doesn't exist.

Now, I haven't yet seen a demand for such a cases, but it seems wrong
to not consider them. The current solution cover these.

Kind regards
Uffe

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Tuesday, November 04, 2014 09:20:44 AM Geert Uytterhoeven wrote:
> On Tue, Nov 4, 2014 at 2:43 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > That is quite simple to address, though.  You can register a bus type
> > notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
> > (where the target device belongs to the domain), and do that only for
> > CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).
> 
> Registering a bus type notifier (that will be a platform bus notifier,
> right?) will affect
> all platform devices, not only the on-SoC devices.

Yes it will, but the "is the device in this domain?" check is trivial to do.

Rafael

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Tuesday, November 04, 2014 09:20:44 AM Geert Uytterhoeven wrote:
> On Tue, Nov 4, 2014 at 2:43 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > That is quite simple to address, though.  You can register a bus type
> > notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
> > (where the target device belongs to the domain), and do that only for
> > CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).
> 
> Registering a bus type notifier (that will be a platform bus notifier,
> right?) will affect
> all platform devices, not only the on-SoC devices.

Yes it will, but the "is the device in this domain?" check is trivial to do.

Rafael

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Tuesday, November 04, 2014 09:54:19 AM Ulf Hansson wrote:
> [...]
> 
> > Generally, there are two or even three levels of runtime PM handling,
> > driver, (possibly) bus type and (possibly) PM domain (and multiple levels
> > of these are possible in principle).  All of them have to be initialized
> > at different times.
> >
> > Quite arguably, the PM domain and/or bus type runtime PM handling should
> > be initialized even before registerind the device or during device
> > registration.  Doing that later may be too late.  When the device has been
> > registered, runtime PM should work to an extent allowing the driver to access
> > the device and configure it further after calling pm_runtime_resume().
> >
> > Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
> > it must take the fact that the driver's own ->runtime_resume() may be called
> > as a result of this into account.  That's why I'm asking whether or not the
> > core should call pm_runtime_resume() before calling really_probe() in a
> > followup branch of this thread.
> 
> I am reading the other thread, let's see.
> 
> >
> > The driver's own runtime PM handling must be initialized in the driver and
> > the only place suitable for that is ->probe().  However, it needs to be done
> > *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
> > is executed.  If that is done properly, it should be possible to cover
> > both the CONFIG_PM_RUNTIME set/unset cases in that code.
> >
> > And I wouldn't recommend anyone to do the runtime PM initialization in
> > ->runtime_resume() (when it is called for the first time), as that would be
> > error prone and fragile.
> 
> Great! That's means we are at least aligned on this topic. :-)
> 
> >
> >> The AMBA bus and some of its drivers a good example of how this has
> >> been implemented:
> >> driver/amba/bus.c
> >> drivers/mmc/host/mmci.c
> >> drivers/spi/spi-pl022.c
> >>
> >> This conclusion I have made from this is:
> >> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
> >> power up a PM domain, is not suitable as the _common_ solution to
> >> solve the race condition. It certainly may work for some scenarios,
> >> but not for those I am looking at.
> >
> > I think, however, that it might work if the core calls pm_runtime_get_sync()
> > from driver_probe_device().
> 
> Currently this won't work.
> 
> That's because the buses' ->probe() are invoked in this path and they
> are doing the attachment of the device to its PM domain.
> 
> In other words, we can't power up the PM domain using
> pm_runtime_get_sync(), until the device has been attached to its PM
> domain. Right?

Yes, but my point was that those bus types might need to be changed.

We can't make everyone happy at the same time if their ideas about what to do
are different.

> 
> [...]
> 
> >> For PM domains that are initialized in powered off state, we can't
> >> rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to
> >> power on these PM domains. We need a different mechanism, which is
> >> suggested in this v3 patchset.
> >
> > That is quite simple to address, though.  You can register a bus type
> > notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
> > (where the target device belongs to the domain), and do that only for
> > CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).
> 
> I guess we could use notifiers, but I am not sure I see any benefit.
> The code will be more complex and we need error handling as well.

Like "oh, I can't power up this thing, so I should fail ->probe()"?

Then your driver would need to depend on the specific knowledge about the
given PM domain, I'm afraid.

If you want error handling like that, it needs to be handled by the core,
so as to avoid calling the bus type's ->probe() as well in that case.

So to summarize:

- Devices need to be added to power domains before really_probe() is called
  for them.  Otherwise we'll have ordering problems all over.

- Runtime PM (if compiled in) needs to be enabled for all devices in power
  domains by default.  Otherwise devices may lose power as a result for
  power management of the other devices in the same domain.

- The core should try to power up domains before calling really_probe() both
  for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the
  "device is accessible" assumption.

- Bus types may need to do more on top of that in their ->probe(), so the
  driver's ->probe() can make that assumption too in all cases.

Does that make sense to you?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Tuesday, November 04, 2014 09:54:19 AM Ulf Hansson wrote:
> [...]
> 
> > Generally, there are two or even three levels of runtime PM handling,
> > driver, (possibly) bus type and (possibly) PM domain (and multiple levels
> > of these are possible in principle).  All of them have to be initialized
> > at different times.
> >
> > Quite arguably, the PM domain and/or bus type runtime PM handling should
> > be initialized even before registerind the device or during device
> > registration.  Doing that later may be too late.  When the device has been
> > registered, runtime PM should work to an extent allowing the driver to access
> > the device and configure it further after calling pm_runtime_resume().
> >
> > Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
> > it must take the fact that the driver's own ->runtime_resume() may be called
> > as a result of this into account.  That's why I'm asking whether or not the
> > core should call pm_runtime_resume() before calling really_probe() in a
> > followup branch of this thread.
> 
> I am reading the other thread, let's see.
> 
> >
> > The driver's own runtime PM handling must be initialized in the driver and
> > the only place suitable for that is ->probe().  However, it needs to be done
> > *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
> > is executed.  If that is done properly, it should be possible to cover
> > both the CONFIG_PM_RUNTIME set/unset cases in that code.
> >
> > And I wouldn't recommend anyone to do the runtime PM initialization in
> > ->runtime_resume() (when it is called for the first time), as that would be
> > error prone and fragile.
> 
> Great! That's means we are at least aligned on this topic. :-)
> 
> >
> >> The AMBA bus and some of its drivers a good example of how this has
> >> been implemented:
> >> driver/amba/bus.c
> >> drivers/mmc/host/mmci.c
> >> drivers/spi/spi-pl022.c
> >>
> >> This conclusion I have made from this is:
> >> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
> >> power up a PM domain, is not suitable as the _common_ solution to
> >> solve the race condition. It certainly may work for some scenarios,
> >> but not for those I am looking at.
> >
> > I think, however, that it might work if the core calls pm_runtime_get_sync()
> > from driver_probe_device().
> 
> Currently this won't work.
> 
> That's because the buses' ->probe() are invoked in this path and they
> are doing the attachment of the device to its PM domain.
> 
> In other words, we can't power up the PM domain using
> pm_runtime_get_sync(), until the device has been attached to its PM
> domain. Right?

Yes, but my point was that those bus types might need to be changed.

We can't make everyone happy at the same time if their ideas about what to do
are different.

> 
> [...]
> 
> >> For PM domains that are initialized in powered off state, we can't
> >> rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to
> >> power on these PM domains. We need a different mechanism, which is
> >> suggested in this v3 patchset.
> >
> > That is quite simple to address, though.  You can register a bus type
> > notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
> > (where the target device belongs to the domain), and do that only for
> > CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).
> 
> I guess we could use notifiers, but I am not sure I see any benefit.
> The code will be more complex and we need error handling as well.

Like "oh, I can't power up this thing, so I should fail ->probe()"?

Then your driver would need to depend on the specific knowledge about the
given PM domain, I'm afraid.

If you want error handling like that, it needs to be handled by the core,
so as to avoid calling the bus type's ->probe() as well in that case.

So to summarize:

- Devices need to be added to power domains before really_probe() is called
  for them.  Otherwise we'll have ordering problems all over.

- Runtime PM (if compiled in) needs to be enabled for all devices in power
  domains by default.  Otherwise devices may lose power as a result for
  power management of the other devices in the same domain.

- The core should try to power up domains before calling really_probe() both
  for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the
  "device is accessible" assumption.

- Bus types may need to do more on top of that in their ->probe(), so the
  driver's ->probe() can make that assumption too in all cases.

Does that make sense to you?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Tuesday, November 04, 2014 01:05:21 AM Dmitry Torokhov wrote:
> On Tue, Nov 04, 2014 at 09:54:19AM +0100, Ulf Hansson wrote:
> > [...]
> > 
> > > Generally, there are two or even three levels of runtime PM handling,
> > > driver, (possibly) bus type and (possibly) PM domain (and multiple levels
> > > of these are possible in principle).  All of them have to be initialized
> > > at different times.
> > >
> > > Quite arguably, the PM domain and/or bus type runtime PM handling should
> > > be initialized even before registerind the device or during device
> > > registration.  Doing that later may be too late.  When the device has been
> > > registered, runtime PM should work to an extent allowing the driver to access
> > > the device and configure it further after calling pm_runtime_resume().
> > >
> > > Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
> > > it must take the fact that the driver's own ->runtime_resume() may be called
> > > as a result of this into account.  That's why I'm asking whether or not the
> > > core should call pm_runtime_resume() before calling really_probe() in a
> > > followup branch of this thread.
> > 
> > I am reading the other thread, let's see.
> > 
> > >
> > > The driver's own runtime PM handling must be initialized in the driver and
> > > the only place suitable for that is ->probe().  However, it needs to be done
> > > *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
> > > is executed.  If that is done properly, it should be possible to cover
> > > both the CONFIG_PM_RUNTIME set/unset cases in that code.
> > >
> > > And I wouldn't recommend anyone to do the runtime PM initialization in
> > > ->runtime_resume() (when it is called for the first time), as that would be
> > > error prone and fragile.
> > 
> > Great! That's means we are at least aligned on this topic. :-)
> > 
> > >
> > >> The AMBA bus and some of its drivers a good example of how this has
> > >> been implemented:
> > >> driver/amba/bus.c
> > >> drivers/mmc/host/mmci.c
> > >> drivers/spi/spi-pl022.c
> > >>
> > >> This conclusion I have made from this is:
> > >> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
> > >> power up a PM domain, is not suitable as the _common_ solution to
> > >> solve the race condition. It certainly may work for some scenarios,
> > >> but not for those I am looking at.
> > >
> > > I think, however, that it might work if the core calls pm_runtime_get_sync()
> > > from driver_probe_device().
> > 
> > Currently this won't work.
> > 
> > That's because the buses' ->probe() are invoked in this path and they
> > are doing the attachment of the device to its PM domain.
> > 
> > In other words, we can't power up the PM domain using
> > pm_runtime_get_sync(), until the device has been attached to its PM
> > domain. Right?
> 
> 
> I think this is one of the issues that we have there. Why do we conflate
> probing and placing the device into a power domain? The latter should
> happen when we register the device. The fact that a device was probed
> and has a driver bound or not bound to it should have no bearing on
> whether the device is member of power domain or not.

Agreed.

Rafael

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Tuesday, November 04, 2014 01:05:21 AM Dmitry Torokhov wrote:
> On Tue, Nov 04, 2014 at 09:54:19AM +0100, Ulf Hansson wrote:
> > [...]
> > 
> > > Generally, there are two or even three levels of runtime PM handling,
> > > driver, (possibly) bus type and (possibly) PM domain (and multiple levels
> > > of these are possible in principle).  All of them have to be initialized
> > > at different times.
> > >
> > > Quite arguably, the PM domain and/or bus type runtime PM handling should
> > > be initialized even before registerind the device or during device
> > > registration.  Doing that later may be too late.  When the device has been
> > > registered, runtime PM should work to an extent allowing the driver to access
> > > the device and configure it further after calling pm_runtime_resume().
> > >
> > > Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
> > > it must take the fact that the driver's own ->runtime_resume() may be called
> > > as a result of this into account.  That's why I'm asking whether or not the
> > > core should call pm_runtime_resume() before calling really_probe() in a
> > > followup branch of this thread.
> > 
> > I am reading the other thread, let's see.
> > 
> > >
> > > The driver's own runtime PM handling must be initialized in the driver and
> > > the only place suitable for that is ->probe().  However, it needs to be done
> > > *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
> > > is executed.  If that is done properly, it should be possible to cover
> > > both the CONFIG_PM_RUNTIME set/unset cases in that code.
> > >
> > > And I wouldn't recommend anyone to do the runtime PM initialization in
> > > ->runtime_resume() (when it is called for the first time), as that would be
> > > error prone and fragile.
> > 
> > Great! That's means we are at least aligned on this topic. :-)
> > 
> > >
> > >> The AMBA bus and some of its drivers a good example of how this has
> > >> been implemented:
> > >> driver/amba/bus.c
> > >> drivers/mmc/host/mmci.c
> > >> drivers/spi/spi-pl022.c
> > >>
> > >> This conclusion I have made from this is:
> > >> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
> > >> power up a PM domain, is not suitable as the _common_ solution to
> > >> solve the race condition. It certainly may work for some scenarios,
> > >> but not for those I am looking at.
> > >
> > > I think, however, that it might work if the core calls pm_runtime_get_sync()
> > > from driver_probe_device().
> > 
> > Currently this won't work.
> > 
> > That's because the buses' ->probe() are invoked in this path and they
> > are doing the attachment of the device to its PM domain.
> > 
> > In other words, we can't power up the PM domain using
> > pm_runtime_get_sync(), until the device has been attached to its PM
> > domain. Right?
> 
> 
> I think this is one of the issues that we have there. Why do we conflate
> probing and placing the device into a power domain? The latter should
> happen when we register the device. The fact that a device was probed
> and has a driver bound or not bound to it should have no bearing on
> whether the device is member of power domain or not.

Agreed.

Rafael

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Tuesday, November 04, 2014 10:24:35 AM Ulf Hansson wrote:
> On 4 November 2014 10:05, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > On Tue, Nov 04, 2014 at 09:54:19AM +0100, Ulf Hansson wrote:
> >> [...]
> >>
> >> > Generally, there are two or even three levels of runtime PM handling,
> >> > driver, (possibly) bus type and (possibly) PM domain (and multiple levels
> >> > of these are possible in principle).  All of them have to be initialized
> >> > at different times.
> >> >
> >> > Quite arguably, the PM domain and/or bus type runtime PM handling should
> >> > be initialized even before registerind the device or during device
> >> > registration.  Doing that later may be too late.  When the device has been
> >> > registered, runtime PM should work to an extent allowing the driver to access
> >> > the device and configure it further after calling pm_runtime_resume().
> >> >
> >> > Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
> >> > it must take the fact that the driver's own ->runtime_resume() may be called
> >> > as a result of this into account.  That's why I'm asking whether or not the
> >> > core should call pm_runtime_resume() before calling really_probe() in a
> >> > followup branch of this thread.
> >>
> >> I am reading the other thread, let's see.
> >>
> >> >
> >> > The driver's own runtime PM handling must be initialized in the driver and
> >> > the only place suitable for that is ->probe().  However, it needs to be done
> >> > *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
> >> > is executed.  If that is done properly, it should be possible to cover
> >> > both the CONFIG_PM_RUNTIME set/unset cases in that code.
> >> >
> >> > And I wouldn't recommend anyone to do the runtime PM initialization in
> >> > ->runtime_resume() (when it is called for the first time), as that would be
> >> > error prone and fragile.
> >>
> >> Great! That's means we are at least aligned on this topic. :-)
> >>
> >> >
> >> >> The AMBA bus and some of its drivers a good example of how this has
> >> >> been implemented:
> >> >> driver/amba/bus.c
> >> >> drivers/mmc/host/mmci.c
> >> >> drivers/spi/spi-pl022.c
> >> >>
> >> >> This conclusion I have made from this is:
> >> >> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
> >> >> power up a PM domain, is not suitable as the _common_ solution to
> >> >> solve the race condition. It certainly may work for some scenarios,
> >> >> but not for those I am looking at.
> >> >
> >> > I think, however, that it might work if the core calls pm_runtime_get_sync()
> >> > from driver_probe_device().
> >>
> >> Currently this won't work.
> >>
> >> That's because the buses' ->probe() are invoked in this path and they
> >> are doing the attachment of the device to its PM domain.
> >>
> >> In other words, we can't power up the PM domain using
> >> pm_runtime_get_sync(), until the device has been attached to its PM
> >> domain. Right?
> >
> >
> > I think this is one of the issues that we have there. Why do we conflate
> > probing and placing the device into a power domain? The latter should
> > happen when we register the device. The fact that a device was probed
> > and has a driver bound or not bound to it should have no bearing on
> > whether the device is member of power domain or not.
> 
> Devices that are created while "discoverable buses" are being probed
> can't be attached to a PM domain before the probing is done, because
> those simply doesn't exist.

Honestly, I'm not sure what you're talking about.

Devices on a "discoverable* bus (say PCI) are added when the *controller* is
probed, not when *they* are probed.

You very much need to have a struct device registered to be able to call
really_probe() for it.

> Now, I haven't yet seen a demand for such a cases, but it seems wrong
> to not consider them. The current solution cover these.

Oh dear.  Please rethink this.

Rafael

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Tuesday, November 04, 2014 10:24:35 AM Ulf Hansson wrote:
> On 4 November 2014 10:05, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> > On Tue, Nov 04, 2014 at 09:54:19AM +0100, Ulf Hansson wrote:
> >> [...]
> >>
> >> > Generally, there are two or even three levels of runtime PM handling,
> >> > driver, (possibly) bus type and (possibly) PM domain (and multiple levels
> >> > of these are possible in principle).  All of them have to be initialized
> >> > at different times.
> >> >
> >> > Quite arguably, the PM domain and/or bus type runtime PM handling should
> >> > be initialized even before registerind the device or during device
> >> > registration.  Doing that later may be too late.  When the device has been
> >> > registered, runtime PM should work to an extent allowing the driver to access
> >> > the device and configure it further after calling pm_runtime_resume().
> >> >
> >> > Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
> >> > it must take the fact that the driver's own ->runtime_resume() may be called
> >> > as a result of this into account.  That's why I'm asking whether or not the
> >> > core should call pm_runtime_resume() before calling really_probe() in a
> >> > followup branch of this thread.
> >>
> >> I am reading the other thread, let's see.
> >>
> >> >
> >> > The driver's own runtime PM handling must be initialized in the driver and
> >> > the only place suitable for that is ->probe().  However, it needs to be done
> >> > *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
> >> > is executed.  If that is done properly, it should be possible to cover
> >> > both the CONFIG_PM_RUNTIME set/unset cases in that code.
> >> >
> >> > And I wouldn't recommend anyone to do the runtime PM initialization in
> >> > ->runtime_resume() (when it is called for the first time), as that would be
> >> > error prone and fragile.
> >>
> >> Great! That's means we are at least aligned on this topic. :-)
> >>
> >> >
> >> >> The AMBA bus and some of its drivers a good example of how this has
> >> >> been implemented:
> >> >> driver/amba/bus.c
> >> >> drivers/mmc/host/mmci.c
> >> >> drivers/spi/spi-pl022.c
> >> >>
> >> >> This conclusion I have made from this is:
> >> >> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
> >> >> power up a PM domain, is not suitable as the _common_ solution to
> >> >> solve the race condition. It certainly may work for some scenarios,
> >> >> but not for those I am looking at.
> >> >
> >> > I think, however, that it might work if the core calls pm_runtime_get_sync()
> >> > from driver_probe_device().
> >>
> >> Currently this won't work.
> >>
> >> That's because the buses' ->probe() are invoked in this path and they
> >> are doing the attachment of the device to its PM domain.
> >>
> >> In other words, we can't power up the PM domain using
> >> pm_runtime_get_sync(), until the device has been attached to its PM
> >> domain. Right?
> >
> >
> > I think this is one of the issues that we have there. Why do we conflate
> > probing and placing the device into a power domain? The latter should
> > happen when we register the device. The fact that a device was probed
> > and has a driver bound or not bound to it should have no bearing on
> > whether the device is member of power domain or not.
> 
> Devices that are created while "discoverable buses" are being probed
> can't be attached to a PM domain before the probing is done, because
> those simply doesn't exist.

Honestly, I'm not sure what you're talking about.

Devices on a "discoverable* bus (say PCI) are added when the *controller* is
probed, not when *they* are probed.

You very much need to have a struct device registered to be able to call
really_probe() for it.

> Now, I haven't yet seen a demand for such a cases, but it seems wrong
> to not consider them. The current solution cover these.

Oh dear.  Please rethink this.

Rafael

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

On 4 November 2014 14:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, November 04, 2014 09:54:19 AM Ulf Hansson wrote:
>> [...]
>>
>> > Generally, there are two or even three levels of runtime PM handling,
>> > driver, (possibly) bus type and (possibly) PM domain (and multiple levels
>> > of these are possible in principle).  All of them have to be initialized
>> > at different times.
>> >
>> > Quite arguably, the PM domain and/or bus type runtime PM handling should
>> > be initialized even before registerind the device or during device
>> > registration.  Doing that later may be too late.  When the device has been
>> > registered, runtime PM should work to an extent allowing the driver to access
>> > the device and configure it further after calling pm_runtime_resume().
>> >
>> > Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
>> > it must take the fact that the driver's own ->runtime_resume() may be called
>> > as a result of this into account.  That's why I'm asking whether or not the
>> > core should call pm_runtime_resume() before calling really_probe() in a
>> > followup branch of this thread.
>>
>> I am reading the other thread, let's see.
>>
>> >
>> > The driver's own runtime PM handling must be initialized in the driver and
>> > the only place suitable for that is ->probe().  However, it needs to be done
>> > *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
>> > is executed.  If that is done properly, it should be possible to cover
>> > both the CONFIG_PM_RUNTIME set/unset cases in that code.
>> >
>> > And I wouldn't recommend anyone to do the runtime PM initialization in
>> > ->runtime_resume() (when it is called for the first time), as that would be
>> > error prone and fragile.
>>
>> Great! That's means we are at least aligned on this topic. :-)
>>
>> >
>> >> The AMBA bus and some of its drivers a good example of how this has
>> >> been implemented:
>> >> driver/amba/bus.c
>> >> drivers/mmc/host/mmci.c
>> >> drivers/spi/spi-pl022.c
>> >>
>> >> This conclusion I have made from this is:
>> >> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
>> >> power up a PM domain, is not suitable as the _common_ solution to
>> >> solve the race condition. It certainly may work for some scenarios,
>> >> but not for those I am looking at.
>> >
>> > I think, however, that it might work if the core calls pm_runtime_get_sync()
>> > from driver_probe_device().
>>
>> Currently this won't work.
>>
>> That's because the buses' ->probe() are invoked in this path and they
>> are doing the attachment of the device to its PM domain.
>>
>> In other words, we can't power up the PM domain using
>> pm_runtime_get_sync(), until the device has been attached to its PM
>> domain. Right?
>
> Yes, but my point was that those bus types might need to be changed.
>
> We can't make everyone happy at the same time if their ideas about what to do
> are different.

Urgh. I fail to understand this comment.

Why do we prefer the pm_runtime_get_sync() solution in favour of this
pathset's approach?

What are the benefit do we get with pm_runtime_get_sync()?

>
>>
>> [...]
>>
>> >> For PM domains that are initialized in powered off state, we can't
>> >> rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to
>> >> power on these PM domains. We need a different mechanism, which is
>> >> suggested in this v3 patchset.
>> >
>> > That is quite simple to address, though.  You can register a bus type
>> > notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
>> > (where the target device belongs to the domain), and do that only for
>> > CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).
>>
>> I guess we could use notifiers, but I am not sure I see any benefit.
>> The code will be more complex and we need error handling as well.
>
> Like "oh, I can't power up this thing, so I should fail ->probe()"?
>
> Then your driver would need to depend on the specific knowledge about the
> given PM domain, I'm afraid.
>
> If you want error handling like that, it needs to be handled by the core,
> so as to avoid calling the bus type's ->probe() as well in that case.

Yes, I want this error handling - but I fail to understand why the bus
can't handle the errors.

If works perfectly in this patchset's approach.

>
> So to summarize:
>
> - Devices need to be added to power domains before really_probe() is called
>   for them.  Otherwise we'll have ordering problems all over.

What ordering problems?

>
> - Runtime PM (if compiled in) needs to be enabled for all devices in power
>   domains by default.  Otherwise devices may lose power as a result for
>   power management of the other devices in the same domain.
>
> - The core should try to power up domains before calling really_probe() both
>   for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the
>   "device is accessible" assumption.

And how exactly will you then power up the PM domain when
CONFIG_PM_RUNTIME is unset?

>
> - Bus types may need to do more on top of that in their ->probe(), so the
>   driver's ->probe() can make that assumption too in all cases.
>
> Does that make sense to you?

I working on fully understanding your idea. :-)

Kind regards
Uffe

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

On 4 November 2014 14:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, November 04, 2014 09:54:19 AM Ulf Hansson wrote:
>> [...]
>>
>> > Generally, there are two or even three levels of runtime PM handling,
>> > driver, (possibly) bus type and (possibly) PM domain (and multiple levels
>> > of these are possible in principle).  All of them have to be initialized
>> > at different times.
>> >
>> > Quite arguably, the PM domain and/or bus type runtime PM handling should
>> > be initialized even before registerind the device or during device
>> > registration.  Doing that later may be too late.  When the device has been
>> > registered, runtime PM should work to an extent allowing the driver to access
>> > the device and configure it further after calling pm_runtime_resume().
>> >
>> > Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
>> > it must take the fact that the driver's own ->runtime_resume() may be called
>> > as a result of this into account.  That's why I'm asking whether or not the
>> > core should call pm_runtime_resume() before calling really_probe() in a
>> > followup branch of this thread.
>>
>> I am reading the other thread, let's see.
>>
>> >
>> > The driver's own runtime PM handling must be initialized in the driver and
>> > the only place suitable for that is ->probe().  However, it needs to be done
>> > *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
>> > is executed.  If that is done properly, it should be possible to cover
>> > both the CONFIG_PM_RUNTIME set/unset cases in that code.
>> >
>> > And I wouldn't recommend anyone to do the runtime PM initialization in
>> > ->runtime_resume() (when it is called for the first time), as that would be
>> > error prone and fragile.
>>
>> Great! That's means we are at least aligned on this topic. :-)
>>
>> >
>> >> The AMBA bus and some of its drivers a good example of how this has
>> >> been implemented:
>> >> driver/amba/bus.c
>> >> drivers/mmc/host/mmci.c
>> >> drivers/spi/spi-pl022.c
>> >>
>> >> This conclusion I have made from this is:
>> >> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
>> >> power up a PM domain, is not suitable as the _common_ solution to
>> >> solve the race condition. It certainly may work for some scenarios,
>> >> but not for those I am looking at.
>> >
>> > I think, however, that it might work if the core calls pm_runtime_get_sync()
>> > from driver_probe_device().
>>
>> Currently this won't work.
>>
>> That's because the buses' ->probe() are invoked in this path and they
>> are doing the attachment of the device to its PM domain.
>>
>> In other words, we can't power up the PM domain using
>> pm_runtime_get_sync(), until the device has been attached to its PM
>> domain. Right?
>
> Yes, but my point was that those bus types might need to be changed.
>
> We can't make everyone happy at the same time if their ideas about what to do
> are different.

Urgh. I fail to understand this comment.

Why do we prefer the pm_runtime_get_sync() solution in favour of this
pathset's approach?

What are the benefit do we get with pm_runtime_get_sync()?

>
>>
>> [...]
>>
>> >> For PM domains that are initialized in powered off state, we can't
>> >> rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to
>> >> power on these PM domains. We need a different mechanism, which is
>> >> suggested in this v3 patchset.
>> >
>> > That is quite simple to address, though.  You can register a bus type
>> > notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
>> > (where the target device belongs to the domain), and do that only for
>> > CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).
>>
>> I guess we could use notifiers, but I am not sure I see any benefit.
>> The code will be more complex and we need error handling as well.
>
> Like "oh, I can't power up this thing, so I should fail ->probe()"?
>
> Then your driver would need to depend on the specific knowledge about the
> given PM domain, I'm afraid.
>
> If you want error handling like that, it needs to be handled by the core,
> so as to avoid calling the bus type's ->probe() as well in that case.

Yes, I want this error handling - but I fail to understand why the bus
can't handle the errors.

If works perfectly in this patchset's approach.

>
> So to summarize:
>
> - Devices need to be added to power domains before really_probe() is called
>   for them.  Otherwise we'll have ordering problems all over.

What ordering problems?

>
> - Runtime PM (if compiled in) needs to be enabled for all devices in power
>   domains by default.  Otherwise devices may lose power as a result for
>   power management of the other devices in the same domain.
>
> - The core should try to power up domains before calling really_probe() both
>   for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the
>   "device is accessible" assumption.

And how exactly will you then power up the PM domain when
CONFIG_PM_RUNTIME is unset?

>
> - Bus types may need to do more on top of that in their ->probe(), so the
>   driver's ->probe() can make that assumption too in all cases.
>
> Does that make sense to you?

I working on fully understanding your idea. :-)

Kind regards
Uffe

Re: pm_runtime_enable() in ->probe()

[Alan Stern]

On Tue, 4 Nov 2014, Rafael J. Wysocki wrote:

> > I still think this needs to be handled at the level of the subsystem 
> > and device driver, not at the level of the PM core.  If somebody needs 
> > to insure that the device won't be powered off while the probe routine 
> > is running, the subsystem/driver should take care of it.
> 
> The problem with that is that doing pm_runtime_resume() (or equivalent)
> anywhere in the bus type's or driver's ->probe() will trigger the driver's
> PM callbacks (for the majority of bus types).

If the subsystems do the wrong thing, they should be fixed.

> PM domains complicate that even further, because they may override bus type
> callbacks.
> 
> > After all, as Kevin points out, sometimes we really _do_ want the
> > device to end up powered-down when the probing is finished.  There's
> > no way the PM core can guess what the driver layer will want in the
> > end.
> 
> And as I said in a reply to Kevin, if we do
> 
> 	pm_runtime_get_sync(dev);
> 	ret = really_probe(dev, drv);
> 	pm_runtime_put(dev);
> 
> in driver_probe_device(), then the last statement will power down the device
> properly, unless the driver's ->probe() bumped up the reference counter.

The problem is that the division of labor between the subsystem and the 
device driver is not clearly defined, and different subsystems handle 
things in different ways.

Scenario 1:	The subsystem does all the necessary preparation
		when the device is detected and probed.  The driver
		doesn't have to worry about runtime PM during either
		probe or remove.

		In this case it doesn't much matter what the PM core
		does.  Your suggestion would work well, and the 
		existing code is already okay.

Scenario 2:	The subsystem does very little preparation and expects
		the driver to take care of all the runtime PM.  When
		the driver's probe routine is called, the device is 
		still in its initial low-power state.  If the probe
		fails, the driver wants to leave the device in a low-
		power state and disabled for runtime PM.

		In this case calling pm_runtime_get_sync() before
		really_probe() and pm_runtime_put() afterward won't
		work.  The pm_runtime_put() will fail and the device 
		will be left at full power, because after the probe
		routine doesn't succeed, runtime PM is left disabled.

Nothing the PM core can do will work perfectly for both scenarios and 
relieve the subsystem and driver of their responsibilities.


> Suppose that the device is in a power domain that was turned off before
> probing the driver, whatever the reason.  It needs to but turned on for
> the driver to be able to access the device in ->probe() and now the question
> is how to do that.

How did the device get put into the power domain in the first place?  
If the subsystem put it there, then presumably the subsystem is
sophisticated enough about runtime PM to handle the requirements
properly (or to be fixed if it doesn't).

Similarly, power domains need to understand how runtime PM is meant to 
work.

> The natural way would be to call pm_runtime_get_sync(dev) at the beginning
> of ->probe(), but that would trigger the driver's own ->runtime_resume() to
> run and it would require the driver to do some extra checks in that routine.
> Those checks wouldn't be necessary at any other time.
> 
> However, if the core does pm_runtime_get_sync(dev) before calling
> really_probe(), it may turn on the power domain without invoking runtime
> PM callbacks from the driver.

What if the device is disabled for runtime PM before really_probe() 
runs?  Isn't that the default situation?

In general, if the kernel expects a device to operate properly only
when it is bound to the right driver, then we shouldn't expect runtime
PM to operate properly if the binding doesn't exist yet or is only
partially complete.


> It looks like we need an agreement on what to do and what the expectations
> should be for each driver core operation at the high level.  That is,
> what ->probe() should expect, what ->remove() should expect, what the   
> state after executing each of them should be for CONFIG_PM_RUNTIME set and
> unset etc.  Otherwise we'll always have various bus types doing what *they*
> think is appropriate and not agreeing with each other.

That's a very good idea.  If it can be done... since there is so much 
variation among all the different subsystems.

Alan Stern

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

>>
>> Devices that are created while "discoverable buses" are being probed
>> can't be attached to a PM domain before the probing is done, because
>> those simply doesn't exist.
>
> Honestly, I'm not sure what you're talking about.
>
> Devices on a "discoverable* bus (say PCI) are added when the *controller* is
> probed, not when *they* are probed.

Okay, so maybe "discoverable buses" isn't the proper term.

>
> You very much need to have a struct device registered to be able to call
> really_probe() for it.

Yes. But my point is that the struct device may be created dynamically
at some point in time.

This is how mmc/sd/sdio cards are handled. We don't have the
information about the card and thus not the struct device of it, until
we have detected it. Maybe we could at that point try to add the
device to its PM domain?

>
>> Now, I haven't yet seen a demand for such a cases, but it seems wrong
>> to not consider them. The current solution cover these.
>
> Oh dear.  Please rethink this.

Currently, dev_pm_domain_attach() is being invoked at the point when a
SDIO card has been found. From sdio_add_func(). How would that be
solved?

Kind regards
Uffe

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

>>
>> Devices that are created while "discoverable buses" are being probed
>> can't be attached to a PM domain before the probing is done, because
>> those simply doesn't exist.
>
> Honestly, I'm not sure what you're talking about.
>
> Devices on a "discoverable* bus (say PCI) are added when the *controller* is
> probed, not when *they* are probed.

Okay, so maybe "discoverable buses" isn't the proper term.

>
> You very much need to have a struct device registered to be able to call
> really_probe() for it.

Yes. But my point is that the struct device may be created dynamically
at some point in time.

This is how mmc/sd/sdio cards are handled. We don't have the
information about the card and thus not the struct device of it, until
we have detected it. Maybe we could at that point try to add the
device to its PM domain?

>
>> Now, I haven't yet seen a demand for such a cases, but it seems wrong
>> to not consider them. The current solution cover these.
>
> Oh dear.  Please rethink this.

Currently, dev_pm_domain_attach() is being invoked at the point when a
SDIO card has been found. From sdio_add_func(). How would that be
solved?

Kind regards
Uffe

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Dmitry Torokhov]

On Tue, Nov 04, 2014 at 06:01:44PM +0100, Ulf Hansson wrote:
> >>
> >> Devices that are created while "discoverable buses" are being probed
> >> can't be attached to a PM domain before the probing is done, because
> >> those simply doesn't exist.
> >
> > Honestly, I'm not sure what you're talking about.
> >
> > Devices on a "discoverable* bus (say PCI) are added when the *controller* is
> > probed, not when *they* are probed.
> 
> Okay, so maybe "discoverable buses" isn't the proper term.
> 
> >
> > You very much need to have a struct device registered to be able to call
> > really_probe() for it.
> 
> Yes. But my point is that the struct device may be created dynamically
> at some point in time.

And that is fine.

> 
> This is how mmc/sd/sdio cards are handled. We don't have the
> information about the card and thus not the struct device of it, until
> we have detected it. Maybe we could at that point try to add the
> device to its PM domain?

Well, I think we need to first define what PM domain they will fall
into. Do they have to be in one? The concept of power domain, as far as I
understand it, is needed when we need to form relations going outside
the standard parent/child relationship. Here we have a controller and
then one or more cards in it. To be able to use card you need to power
up parent and you can not power down parent until all children are
powered down.

But in any case, device discovery and binding them to drivers are 2
separate steps. You have a PCI bus. You enumerate it - new PCI devices
are created. Some of them may be put in a certain power domain. You then
bind drivers to PCI devices (not strictly 'then' but we could implement
driver core to postpone binding until current round of enumeration is
complete) - and you discover USB controller and maybe i2c controller.
Then you bind drivers to them which causes enumeration of the new bus
and new devices are created. But in all these cases enumeration and
creation of new 'struct device's, and driver binding are logically
separate steps.

> 
> >
> >> Now, I haven't yet seen a demand for such a cases, but it seems wrong
> >> to not consider them. The current solution cover these.
> >
> > Oh dear.  Please rethink this.
> 
> Currently, dev_pm_domain_attach() is being invoked at the point when a
> SDIO card has been found. From sdio_add_func(). How would that be
> solved?
> 

It is probably the one place where we currently doing it correctly (form
logical point of view, not implementation wise - implementation wise in
sdio we add device to power domain after calling device_add() which
means that probe() could have been called before we added the new device
to any power domain).

Thanks.

-- 
Dmitry

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Dmitry Torokhov]

On Tue, Nov 04, 2014 at 06:01:44PM +0100, Ulf Hansson wrote:
> >>
> >> Devices that are created while "discoverable buses" are being probed
> >> can't be attached to a PM domain before the probing is done, because
> >> those simply doesn't exist.
> >
> > Honestly, I'm not sure what you're talking about.
> >
> > Devices on a "discoverable* bus (say PCI) are added when the *controller* is
> > probed, not when *they* are probed.
> 
> Okay, so maybe "discoverable buses" isn't the proper term.
> 
> >
> > You very much need to have a struct device registered to be able to call
> > really_probe() for it.
> 
> Yes. But my point is that the struct device may be created dynamically
> at some point in time.

And that is fine.

> 
> This is how mmc/sd/sdio cards are handled. We don't have the
> information about the card and thus not the struct device of it, until
> we have detected it. Maybe we could at that point try to add the
> device to its PM domain?

Well, I think we need to first define what PM domain they will fall
into. Do they have to be in one? The concept of power domain, as far as I
understand it, is needed when we need to form relations going outside
the standard parent/child relationship. Here we have a controller and
then one or more cards in it. To be able to use card you need to power
up parent and you can not power down parent until all children are
powered down.

But in any case, device discovery and binding them to drivers are 2
separate steps. You have a PCI bus. You enumerate it - new PCI devices
are created. Some of them may be put in a certain power domain. You then
bind drivers to PCI devices (not strictly 'then' but we could implement
driver core to postpone binding until current round of enumeration is
complete) - and you discover USB controller and maybe i2c controller.
Then you bind drivers to them which causes enumeration of the new bus
and new devices are created. But in all these cases enumeration and
creation of new 'struct device's, and driver binding are logically
separate steps.

> 
> >
> >> Now, I haven't yet seen a demand for such a cases, but it seems wrong
> >> to not consider them. The current solution cover these.
> >
> > Oh dear.  Please rethink this.
> 
> Currently, dev_pm_domain_attach() is being invoked at the point when a
> SDIO card has been found. From sdio_add_func(). How would that be
> solved?
> 

It is probably the one place where we currently doing it correctly (form
logical point of view, not implementation wise - implementation wise in
sdio we add device to power domain after calling device_add() which
means that probe() could have been called before we added the new device
to any power domain).

Thanks.

-- 
Dmitry

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Tuesday, November 04, 2014 10:29:20 AM Dmitry Torokhov wrote:
> On Tue, Nov 04, 2014 at 06:01:44PM +0100, Ulf Hansson wrote:
> > >>
> > >> Devices that are created while "discoverable buses" are being probed
> > >> can't be attached to a PM domain before the probing is done, because
> > >> those simply doesn't exist.
> > >
> > > Honestly, I'm not sure what you're talking about.
> > >
> > > Devices on a "discoverable* bus (say PCI) are added when the *controller* is
> > > probed, not when *they* are probed.
> > 
> > Okay, so maybe "discoverable buses" isn't the proper term.
> > 
> > >
> > > You very much need to have a struct device registered to be able to call
> > > really_probe() for it.
> > 
> > Yes. But my point is that the struct device may be created dynamically
> > at some point in time.
> 
> And that is fine.
> 
> > 
> > This is how mmc/sd/sdio cards are handled. We don't have the
> > information about the card and thus not the struct device of it, until
> > we have detected it. Maybe we could at that point try to add the
> > device to its PM domain?
> 
> Well, I think we need to first define what PM domain they will fall
> into. Do they have to be in one? The concept of power domain, as far as I
> understand it, is needed when we need to form relations going outside
> the standard parent/child relationship. Here we have a controller and
> then one or more cards in it. To be able to use card you need to power
> up parent and you can not power down parent until all children are
> powered down.
> 
> But in any case, device discovery and binding them to drivers are 2
> separate steps. You have a PCI bus. You enumerate it - new PCI devices
> are created. Some of them may be put in a certain power domain. You then
> bind drivers to PCI devices (not strictly 'then' but we could implement
> driver core to postpone binding until current round of enumeration is
> complete) - and you discover USB controller and maybe i2c controller.
> Then you bind drivers to them which causes enumeration of the new bus
> and new devices are created. But in all these cases enumeration and
> creation of new 'struct device's, and driver binding are logically
> separate steps.
> 
> > 
> > >
> > >> Now, I haven't yet seen a demand for such a cases, but it seems wrong
> > >> to not consider them. The current solution cover these.
> > >
> > > Oh dear.  Please rethink this.
> > 
> > Currently, dev_pm_domain_attach() is being invoked at the point when a
> > SDIO card has been found. From sdio_add_func(). How would that be
> > solved?
> > 
> 
> It is probably the one place where we currently doing it correctly (form
> logical point of view, not implementation wise - implementation wise in
> sdio we add device to power domain after calling device_add() which
> means that probe() could have been called before we added the new device
> to any power domain).

I agree.  Ideally, devices should be added to power (or generally PM) domains
before registration.  That is, before calling device_add() for them.

That's the first thing to fix here.

Rafael

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Tuesday, November 04, 2014 10:29:20 AM Dmitry Torokhov wrote:
> On Tue, Nov 04, 2014 at 06:01:44PM +0100, Ulf Hansson wrote:
> > >>
> > >> Devices that are created while "discoverable buses" are being probed
> > >> can't be attached to a PM domain before the probing is done, because
> > >> those simply doesn't exist.
> > >
> > > Honestly, I'm not sure what you're talking about.
> > >
> > > Devices on a "discoverable* bus (say PCI) are added when the *controller* is
> > > probed, not when *they* are probed.
> > 
> > Okay, so maybe "discoverable buses" isn't the proper term.
> > 
> > >
> > > You very much need to have a struct device registered to be able to call
> > > really_probe() for it.
> > 
> > Yes. But my point is that the struct device may be created dynamically
> > at some point in time.
> 
> And that is fine.
> 
> > 
> > This is how mmc/sd/sdio cards are handled. We don't have the
> > information about the card and thus not the struct device of it, until
> > we have detected it. Maybe we could at that point try to add the
> > device to its PM domain?
> 
> Well, I think we need to first define what PM domain they will fall
> into. Do they have to be in one? The concept of power domain, as far as I
> understand it, is needed when we need to form relations going outside
> the standard parent/child relationship. Here we have a controller and
> then one or more cards in it. To be able to use card you need to power
> up parent and you can not power down parent until all children are
> powered down.
> 
> But in any case, device discovery and binding them to drivers are 2
> separate steps. You have a PCI bus. You enumerate it - new PCI devices
> are created. Some of them may be put in a certain power domain. You then
> bind drivers to PCI devices (not strictly 'then' but we could implement
> driver core to postpone binding until current round of enumeration is
> complete) - and you discover USB controller and maybe i2c controller.
> Then you bind drivers to them which causes enumeration of the new bus
> and new devices are created. But in all these cases enumeration and
> creation of new 'struct device's, and driver binding are logically
> separate steps.
> 
> > 
> > >
> > >> Now, I haven't yet seen a demand for such a cases, but it seems wrong
> > >> to not consider them. The current solution cover these.
> > >
> > > Oh dear.  Please rethink this.
> > 
> > Currently, dev_pm_domain_attach() is being invoked at the point when a
> > SDIO card has been found. From sdio_add_func(). How would that be
> > solved?
> > 
> 
> It is probably the one place where we currently doing it correctly (form
> logical point of view, not implementation wise - implementation wise in
> sdio we add device to power domain after calling device_add() which
> means that probe() could have been called before we added the new device
> to any power domain).

I agree.  Ideally, devices should be added to power (or generally PM) domains
before registration.  That is, before calling device_add() for them.

That's the first thing to fix here.

Rafael

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

On 4 November 2014 22:38, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, November 04, 2014 10:29:20 AM Dmitry Torokhov wrote:
>> On Tue, Nov 04, 2014 at 06:01:44PM +0100, Ulf Hansson wrote:
>> > >>
>> > >> Devices that are created while "discoverable buses" are being probed
>> > >> can't be attached to a PM domain before the probing is done, because
>> > >> those simply doesn't exist.
>> > >
>> > > Honestly, I'm not sure what you're talking about.
>> > >
>> > > Devices on a "discoverable* bus (say PCI) are added when the *controller* is
>> > > probed, not when *they* are probed.
>> >
>> > Okay, so maybe "discoverable buses" isn't the proper term.
>> >
>> > >
>> > > You very much need to have a struct device registered to be able to call
>> > > really_probe() for it.
>> >
>> > Yes. But my point is that the struct device may be created dynamically
>> > at some point in time.
>>
>> And that is fine.
>>
>> >
>> > This is how mmc/sd/sdio cards are handled. We don't have the
>> > information about the card and thus not the struct device of it, until
>> > we have detected it. Maybe we could at that point try to add the
>> > device to its PM domain?
>>
>> Well, I think we need to first define what PM domain they will fall
>> into. Do they have to be in one? The concept of power domain, as far as I
>> understand it, is needed when we need to form relations going outside
>> the standard parent/child relationship. Here we have a controller and
>> then one or more cards in it. To be able to use card you need to power
>> up parent and you can not power down parent until all children are
>> powered down.
>>
>> But in any case, device discovery and binding them to drivers are 2
>> separate steps. You have a PCI bus. You enumerate it - new PCI devices
>> are created. Some of them may be put in a certain power domain. You then
>> bind drivers to PCI devices (not strictly 'then' but we could implement
>> driver core to postpone binding until current round of enumeration is
>> complete) - and you discover USB controller and maybe i2c controller.
>> Then you bind drivers to them which causes enumeration of the new bus
>> and new devices are created. But in all these cases enumeration and
>> creation of new 'struct device's, and driver binding are logically
>> separate steps.
>>
>> >
>> > >
>> > >> Now, I haven't yet seen a demand for such a cases, but it seems wrong
>> > >> to not consider them. The current solution cover these.
>> > >
>> > > Oh dear.  Please rethink this.
>> >
>> > Currently, dev_pm_domain_attach() is being invoked at the point when a
>> > SDIO card has been found. From sdio_add_func(). How would that be
>> > solved?
>> >
>>
>> It is probably the one place where we currently doing it correctly (form
>> logical point of view, not implementation wise - implementation wise in
>> sdio we add device to power domain after calling device_add() which
>> means that probe() could have been called before we added the new device
>> to any power domain).
>
> I agree.  Ideally, devices should be added to power (or generally PM) domains
> before registration.  That is, before calling device_add() for them.
>
> That's the first thing to fix here.

It's done in patch 7 in this patchset.

[PATCH v3 7/9] mmc: core: Attach PM domain prior probing of SDIO func driver

Kind regards
Uffe

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

On 4 November 2014 22:38, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, November 04, 2014 10:29:20 AM Dmitry Torokhov wrote:
>> On Tue, Nov 04, 2014 at 06:01:44PM +0100, Ulf Hansson wrote:
>> > >>
>> > >> Devices that are created while "discoverable buses" are being probed
>> > >> can't be attached to a PM domain before the probing is done, because
>> > >> those simply doesn't exist.
>> > >
>> > > Honestly, I'm not sure what you're talking about.
>> > >
>> > > Devices on a "discoverable* bus (say PCI) are added when the *controller* is
>> > > probed, not when *they* are probed.
>> >
>> > Okay, so maybe "discoverable buses" isn't the proper term.
>> >
>> > >
>> > > You very much need to have a struct device registered to be able to call
>> > > really_probe() for it.
>> >
>> > Yes. But my point is that the struct device may be created dynamically
>> > at some point in time.
>>
>> And that is fine.
>>
>> >
>> > This is how mmc/sd/sdio cards are handled. We don't have the
>> > information about the card and thus not the struct device of it, until
>> > we have detected it. Maybe we could at that point try to add the
>> > device to its PM domain?
>>
>> Well, I think we need to first define what PM domain they will fall
>> into. Do they have to be in one? The concept of power domain, as far as I
>> understand it, is needed when we need to form relations going outside
>> the standard parent/child relationship. Here we have a controller and
>> then one or more cards in it. To be able to use card you need to power
>> up parent and you can not power down parent until all children are
>> powered down.
>>
>> But in any case, device discovery and binding them to drivers are 2
>> separate steps. You have a PCI bus. You enumerate it - new PCI devices
>> are created. Some of them may be put in a certain power domain. You then
>> bind drivers to PCI devices (not strictly 'then' but we could implement
>> driver core to postpone binding until current round of enumeration is
>> complete) - and you discover USB controller and maybe i2c controller.
>> Then you bind drivers to them which causes enumeration of the new bus
>> and new devices are created. But in all these cases enumeration and
>> creation of new 'struct device's, and driver binding are logically
>> separate steps.
>>
>> >
>> > >
>> > >> Now, I haven't yet seen a demand for such a cases, but it seems wrong
>> > >> to not consider them. The current solution cover these.
>> > >
>> > > Oh dear.  Please rethink this.
>> >
>> > Currently, dev_pm_domain_attach() is being invoked at the point when a
>> > SDIO card has been found. From sdio_add_func(). How would that be
>> > solved?
>> >
>>
>> It is probably the one place where we currently doing it correctly (form
>> logical point of view, not implementation wise - implementation wise in
>> sdio we add device to power domain after calling device_add() which
>> means that probe() could have been called before we added the new device
>> to any power domain).
>
> I agree.  Ideally, devices should be added to power (or generally PM) domains
> before registration.  That is, before calling device_add() for them.
>
> That's the first thing to fix here.

It's done in patch 7 in this patchset.

[PATCH v3 7/9] mmc: core: Attach PM domain prior probing of SDIO func driver

Kind regards
Uffe

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Grygorii Strashko]

Hi All,

Uh... just finished reading this thread and finally decided to add my 5 cents here
as I've already had very bad experience with some changes introduced to DD/PM core :(
(just check the history of the change 1e2ef05bb
"PM: Limit race conditions between runtime PM and system sleep (v2)"

On 11/04/2014 06:42 PM, Ulf Hansson wrote:
> On 4 November 2014 14:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Tuesday, November 04, 2014 09:54:19 AM Ulf Hansson wrote:
>>> [...]
>>>
>>>> Generally, there are two or even three levels of runtime PM handling,
>>>> driver, (possibly) bus type and (possibly) PM domain (and multiple levels
>>>> of these are possible in principle).  All of them have to be initialized
>>>> at different times.
>>>>
>>>> Quite arguably, the PM domain and/or bus type runtime PM handling should
>>>> be initialized even before registerind the device or during device
>>>> registration.  Doing that later may be too late.  When the device has been
>>>> registered, runtime PM should work to an extent allowing the driver to access
>>>> the device and configure it further after calling pm_runtime_resume().
>>>>
>>>> Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
>>>> it must take the fact that the driver's own ->runtime_resume() may be called
>>>> as a result of this into account.  That's why I'm asking whether or not the
>>>> core should call pm_runtime_resume() before calling really_probe() in a
>>>> followup branch of this thread.
>>>
>>> I am reading the other thread, let's see.
>>>
>>>>
>>>> The driver's own runtime PM handling must be initialized in the driver and
>>>> the only place suitable for that is ->probe().  However, it needs to be done
>>>> *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
>>>> is executed.  If that is done properly, it should be possible to cover
>>>> both the CONFIG_PM_RUNTIME set/unset cases in that code.
>>>>
>>>> And I wouldn't recommend anyone to do the runtime PM initialization in
>>>> ->runtime_resume() (when it is called for the first time), as that would be
>>>> error prone and fragile.
>>>
>>> Great! That's means we are at least aligned on this topic. :-)
>>>
>>>>
>>>>> The AMBA bus and some of its drivers a good example of how this has
>>>>> been implemented:
>>>>> driver/amba/bus.c
>>>>> drivers/mmc/host/mmci.c
>>>>> drivers/spi/spi-pl022.c
>>>>>
>>>>> This conclusion I have made from this is:
>>>>> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
>>>>> power up a PM domain, is not suitable as the _common_ solution to
>>>>> solve the race condition. It certainly may work for some scenarios,
>>>>> but not for those I am looking at.

Amba bus is not good example as for me, because it's cheating ;)
Why? Bus/Host can't know when its child device is powered on or not unless it's
been notified directly about that and, therefor it's not correct to call
pm_runtime_set_active() from amba_probe() - instead it should be called from driver.
For example:
drivers/mmc/host/mmci.c will be active only when its functional clock is enabled.

Apparently someone decided to save few lines of code.

[Pls, don't comment here unless you have read all my reply :)]

>>>>
>>>> I think, however, that it might work if the core calls pm_runtime_get_sync()
>>>> from driver_probe_device().
>>>
>>> Currently this won't work.
>>>
>>> That's because the buses' ->probe() are invoked in this path and they
>>> are doing the attachment of the device to its PM domain.
>>>
>>> In other words, we can't power up the PM domain using
>>> pm_runtime_get_sync(), until the device has been attached to its PM
>>> domain. Right?
>>
>> Yes, but my point was that those bus types might need to be changed.
>>
>> We can't make everyone happy at the same time if their ideas about what to do
>> are different.
> 
> Urgh. I fail to understand this comment.
> 
> Why do we prefer the pm_runtime_get_sync() solution in favour of this
> pathset's approach?
> 
> What are the benefit do we get with pm_runtime_get_sync()?
> 

So, few points below:
1) It will be good to treat Generic Power domain as new feature (in 3.18 there are
 only 5 users of it). From another side, there are at about 500 users of Runtime PM.
So, Problems discussed here could be treated as not related to Runtime PM but rather
related to GPD ;)

For example (lets use AMBA bus):
- after the commit 207f1a2d "amba: Add support for attach/detach of PM domains" 
and if any device of amba bus will be added to some GPD then
amba_pm_runtime_suspend/resume callbacks will never be called for this device and 
"apb_pclk" will not be enabled/disabled.
Simply because GPD will set dev->pm_domain == &genpd->domain, which PM ops will have higher
priority for Runtime PM than Bus PM ops.

2) There are no requirements for arch/platforms/drivers to work in both cases
CONFIG_PM_RUNTIME=y/n. But they must be built without errors/warning for both
cases. 

For example, for Keystone 2 only CONFIG_PM_RUNTIME=y is going to be supported
and if some one decided to disable it - it can be perfectly done from sys_fs/
user space.

3) pm_runtime_get_sync()(or similar) is good not only because i's waking up device, but also
because:
- it can wake up chain of devices (dev->parent->parent->...)
- it can wake up power domain
- it connects device to domain/class/type/bus and so allow to add additional PM layer on top
  of Platform bus (for example arch/arm/mach-omap2/omap_device.c).

So, it will do all needed things, and if it doesn't that problem is in platfrom/bus/driver
code and not in Runtime PM. 
if pm_runtime_get_sync() will be dropped - than all above will need to be implemented
around the ->probe().

4) I've copied here comment from Rafael:
 >>>> Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
 >>>> it must take the fact that the driver's own ->runtime_resume() may be called
 >>>> as a result of this into account.
 Agree, that's a little bit annoying, but we are living with that for more then
 5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior
 (just walk through the drivers and you will see how many drivers expecting the same).

So, any volunteers to check and fix ~500 drivers.

>>
>>>
>>> [...]
>>>
>>>>> For PM domains that are initialized in powered off state, we can't
>>>>> rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to
>>>>> power on these PM domains. We need a different mechanism, which is
>>>>> suggested in this v3 patchset.
>>>>
>>>> That is quite simple to address, though.  You can register a bus type
>>>> notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
>>>> (where the target device belongs to the domain), and do that only for
>>>> CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).
>>>
>>> I guess we could use notifiers, but I am not sure I see any benefit.
>>> The code will be more complex and we need error handling as well.
>>
>> Like "oh, I can't power up this thing, so I should fail ->probe()"?
>>
>> Then your driver would need to depend on the specific knowledge about the
>> given PM domain, I'm afraid.
>>
>> If you want error handling like that, it needs to be handled by the core,
>> so as to avoid calling the bus type's ->probe() as well in that case.
> 
> Yes, I want this error handling - but I fail to understand why the bus
> can't handle the errors.
> 
> If works perfectly in this patchset's approach.
> 
>>
>> So to summarize:
>>
>> - Devices need to be added to power domains before really_probe() is called
>>    for them.  Otherwise we'll have ordering problems all over.

Looks like it's done now. is it?
 

> What ordering problems?
> 
>>
>> - Runtime PM (if compiled in) needs to be enabled for all devices in power
>>    domains by default.  Otherwise devices may lose power as a result for
>>    power management of the other devices in the same domain.

It should prevent enabling/disabling of RPM from sys_fs too.

>>
>> - The core should try to power up domains before calling really_probe() both
>>    for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the
>>    "device is accessible" assumption.

Here I'm still think that pm_runtime_get_sync() (or similar) API should work.
Another way, PM domain should decide what to do when the first device attached to it
- power up and stay powered on for example
 (It will work if devices are attached before ->probe();
  it will not work if devices will be attached once they've been created, but this is different
  question)

> 
> And how exactly will you then power up the PM domain when
> CONFIG_PM_RUNTIME is unset?
> 
>>
>> - Bus types may need to do more on top of that in their ->probe(), so the
>>    driver's ->probe() can make that assumption too in all cases.
>>
>> Does that make sense to you?
> 

I would like to take the liberty to add a couple of points from me:
 - it seems reasonable to have ability to disable Runtime PM globally from sys_fs:
   once disabled - "get" should power on device, "put" should do nothing all the
   time except when ->remove() is called.
 
 - It might be reasonable to add API like pm_runtime_probe_getXXX() which will do
   everything what standard pm_runtime_get_sync() is doing with one exception:
   it will not call driver's .runtime_resume() callback.

 - Just opinion. For GPD, It looks like integration/design problem and may be
   it shouldn't be integrated with Runtime PM through dev_pm_ops. Instead, it
   might be better to invoke it directly from  RPM core.

regards,
-grygorii

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Grygorii Strashko]

Hi All,

Uh... just finished reading this thread and finally decided to add my 5 cents here
as I've already had very bad experience with some changes introduced to DD/PM core :(
(just check the history of the change 1e2ef05bb
"PM: Limit race conditions between runtime PM and system sleep (v2)"

On 11/04/2014 06:42 PM, Ulf Hansson wrote:
> On 4 November 2014 14:51, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> On Tuesday, November 04, 2014 09:54:19 AM Ulf Hansson wrote:
>>> [...]
>>>
>>>> Generally, there are two or even three levels of runtime PM handling,
>>>> driver, (possibly) bus type and (possibly) PM domain (and multiple levels
>>>> of these are possible in principle).  All of them have to be initialized
>>>> at different times.
>>>>
>>>> Quite arguably, the PM domain and/or bus type runtime PM handling should
>>>> be initialized even before registerind the device or during device
>>>> registration.  Doing that later may be too late.  When the device has been
>>>> registered, runtime PM should work to an extent allowing the driver to access
>>>> the device and configure it further after calling pm_runtime_resume().
>>>>
>>>> Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
>>>> it must take the fact that the driver's own ->runtime_resume() may be called
>>>> as a result of this into account.  That's why I'm asking whether or not the
>>>> core should call pm_runtime_resume() before calling really_probe() in a
>>>> followup branch of this thread.
>>>
>>> I am reading the other thread, let's see.
>>>
>>>>
>>>> The driver's own runtime PM handling must be initialized in the driver and
>>>> the only place suitable for that is ->probe().  However, it needs to be done
>>>> *before* the driver's own ->runtime_resume() or ->runtime_suspend() callback
>>>> is executed.  If that is done properly, it should be possible to cover
>>>> both the CONFIG_PM_RUNTIME set/unset cases in that code.
>>>>
>>>> And I wouldn't recommend anyone to do the runtime PM initialization in
>>>> ->runtime_resume() (when it is called for the first time), as that would be
>>>> error prone and fragile.
>>>
>>> Great! That's means we are at least aligned on this topic. :-)
>>>
>>>>
>>>>> The AMBA bus and some of its drivers a good example of how this has
>>>>> been implemented:
>>>>> driver/amba/bus.c
>>>>> drivers/mmc/host/mmci.c
>>>>> drivers/spi/spi-pl022.c
>>>>>
>>>>> This conclusion I have made from this is:
>>>>> - Using pm_runtime_get_sync() during the ->probe() path to explicitly
>>>>> power up a PM domain, is not suitable as the _common_ solution to
>>>>> solve the race condition. It certainly may work for some scenarios,
>>>>> but not for those I am looking at.

Amba bus is not good example as for me, because it's cheating ;)
Why? Bus/Host can't know when its child device is powered on or not unless it's
been notified directly about that and, therefor it's not correct to call
pm_runtime_set_active() from amba_probe() - instead it should be called from driver.
For example:
drivers/mmc/host/mmci.c will be active only when its functional clock is enabled.

Apparently someone decided to save few lines of code.

[Pls, don't comment here unless you have read all my reply :)]

>>>>
>>>> I think, however, that it might work if the core calls pm_runtime_get_sync()
>>>> from driver_probe_device().
>>>
>>> Currently this won't work.
>>>
>>> That's because the buses' ->probe() are invoked in this path and they
>>> are doing the attachment of the device to its PM domain.
>>>
>>> In other words, we can't power up the PM domain using
>>> pm_runtime_get_sync(), until the device has been attached to its PM
>>> domain. Right?
>>
>> Yes, but my point was that those bus types might need to be changed.
>>
>> We can't make everyone happy at the same time if their ideas about what to do
>> are different.
> 
> Urgh. I fail to understand this comment.
> 
> Why do we prefer the pm_runtime_get_sync() solution in favour of this
> pathset's approach?
> 
> What are the benefit do we get with pm_runtime_get_sync()?
> 

So, few points below:
1) It will be good to treat Generic Power domain as new feature (in 3.18 there are
 only 5 users of it). From another side, there are at about 500 users of Runtime PM.
So, Problems discussed here could be treated as not related to Runtime PM but rather
related to GPD ;)

For example (lets use AMBA bus):
- after the commit 207f1a2d "amba: Add support for attach/detach of PM domains" 
and if any device of amba bus will be added to some GPD then
amba_pm_runtime_suspend/resume callbacks will never be called for this device and 
"apb_pclk" will not be enabled/disabled.
Simply because GPD will set dev->pm_domain == &genpd->domain, which PM ops will have higher
priority for Runtime PM than Bus PM ops.

2) There are no requirements for arch/platforms/drivers to work in both cases
CONFIG_PM_RUNTIME=y/n. But they must be built without errors/warning for both
cases. 

For example, for Keystone 2 only CONFIG_PM_RUNTIME=y is going to be supported
and if some one decided to disable it - it can be perfectly done from sys_fs/
user space.

3) pm_runtime_get_sync()(or similar) is good not only because i's waking up device, but also
because:
- it can wake up chain of devices (dev->parent->parent->...)
- it can wake up power domain
- it connects device to domain/class/type/bus and so allow to add additional PM layer on top
  of Platform bus (for example arch/arm/mach-omap2/omap_device.c).

So, it will do all needed things, and if it doesn't that problem is in platfrom/bus/driver
code and not in Runtime PM. 
if pm_runtime_get_sync() will be dropped - than all above will need to be implemented
around the ->probe().

4) I've copied here comment from Rafael:
 >>>> Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
 >>>> it must take the fact that the driver's own ->runtime_resume() may be called
 >>>> as a result of this into account.
 Agree, that's a little bit annoying, but we are living with that for more then
 5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior
 (just walk through the drivers and you will see how many drivers expecting the same).

So, any volunteers to check and fix ~500 drivers.

>>
>>>
>>> [...]
>>>
>>>>> For PM domains that are initialized in powered off state, we can't
>>>>> rely on CONFIG_PM_RUNTIME and thus not on pm_runtime_get_sync() to
>>>>> power on these PM domains. We need a different mechanism, which is
>>>>> suggested in this v3 patchset.
>>>>
>>>> That is quite simple to address, though.  You can register a bus type
>>>> notifier that will power up the domain on BUS_NOTIFY_ADD_DEVICE events
>>>> (where the target device belongs to the domain), and do that only for
>>>> CONFIG_PM_RUNTIME unset (otherwise runtime PM should take care of this).
>>>
>>> I guess we could use notifiers, but I am not sure I see any benefit.
>>> The code will be more complex and we need error handling as well.
>>
>> Like "oh, I can't power up this thing, so I should fail ->probe()"?
>>
>> Then your driver would need to depend on the specific knowledge about the
>> given PM domain, I'm afraid.
>>
>> If you want error handling like that, it needs to be handled by the core,
>> so as to avoid calling the bus type's ->probe() as well in that case.
> 
> Yes, I want this error handling - but I fail to understand why the bus
> can't handle the errors.
> 
> If works perfectly in this patchset's approach.
> 
>>
>> So to summarize:
>>
>> - Devices need to be added to power domains before really_probe() is called
>>    for them.  Otherwise we'll have ordering problems all over.

Looks like it's done now. is it?
 

> What ordering problems?
> 
>>
>> - Runtime PM (if compiled in) needs to be enabled for all devices in power
>>    domains by default.  Otherwise devices may lose power as a result for
>>    power management of the other devices in the same domain.

It should prevent enabling/disabling of RPM from sys_fs too.

>>
>> - The core should try to power up domains before calling really_probe() both
>>    for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the
>>    "device is accessible" assumption.

Here I'm still think that pm_runtime_get_sync() (or similar) API should work.
Another way, PM domain should decide what to do when the first device attached to it
- power up and stay powered on for example
 (It will work if devices are attached before ->probe();
  it will not work if devices will be attached once they've been created, but this is different
  question)

> 
> And how exactly will you then power up the PM domain when
> CONFIG_PM_RUNTIME is unset?
> 
>>
>> - Bus types may need to do more on top of that in their ->probe(), so the
>>    driver's ->probe() can make that assumption too in all cases.
>>
>> Does that make sense to you?
> 

I would like to take the liberty to add a couple of points from me:
 - it seems reasonable to have ability to disable Runtime PM globally from sys_fs:
   once disabled - "get" should power on device, "put" should do nothing all the
   time except when ->remove() is called.
 
 - It might be reasonable to add API like pm_runtime_probe_getXXX() which will do
   everything what standard pm_runtime_get_sync() is doing with one exception:
   it will not call driver's .runtime_resume() callback.

 - Just opinion. For GPD, It looks like integration/design problem and may be
   it shouldn't be integrated with Runtime PM through dev_pm_ops. Instead, it
   might be better to invoke it directly from  RPM core.

regards,
-grygorii

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

[...]

> 2) There are no requirements for arch/platforms/drivers to work in both cases
> CONFIG_PM_RUNTIME=y/n. But they must be built without errors/warning for both
> cases.

For cross SoC drivers this statement is not correct! Driver _must_
support the various combinations of CONFIG_PM*.

Therefore, I think it's better to strive towards a common solution and
to get the building blocks in place.

>
> For example, for Keystone 2 only CONFIG_PM_RUNTIME=y is going to be supported
> and if some one decided to disable it - it can be perfectly done from sys_fs/
> user space.

I can't see why the sysfs option to disable runtime PM should affect
this discussion. That's done at runtime and after the device has been
probed.

>
> 3) pm_runtime_get_sync()(or similar) is good not only because i's waking up device, but also
> because:
> - it can wake up chain of devices (dev->parent->parent->...)

That's right. But that's not what this patchset aims to do.

I realize that the header of the cover letter isn't describing the
problem I am trying to solve very well. I guess the below header would
have been better:

"PM / Domains: Power up PM domains prior drivers starts to probe their devices"

> - it can wake up power domain

Yes, but it requires CONFIG_PM_RUNTIME to be set.

Thus, to solve the problem during driver ->probe() we need another
solution which don't require CONFIG_PM_RUNTIME to be set. As this
patchset proposes.

> - it connects device to domain/class/type/bus and so allow to add additional PM layer on top
>   of Platform bus (for example arch/arm/mach-omap2/omap_device.c).
>
> So, it will do all needed things, and if it doesn't that problem is in platfrom/bus/driver
> code and not in Runtime PM.
> if pm_runtime_get_sync() will be dropped - than all above will need to be implemented
> around the ->probe().

I am not sure what you mean about dropping pm_runtime_get_sync()? All
I am saying is that we can't use it to power on PM domains during the
probe sequence.

Of course, you may still use pm_runtime_get_sync() from anywhere it's
needed, to for example handle device's parent/child relationships.

>
> 4) I've copied here comment from Rafael:
>  >>>> Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
>  >>>> it must take the fact that the driver's own ->runtime_resume() may be called
>  >>>> as a result of this into account.
>  Agree, that's a little bit annoying, but we are living with that for more then
>  5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior
>  (just walk through the drivers and you will see how many drivers expecting the same).
>
> So, any volunteers to check and fix ~500 drivers.

We don't have to change these drivers. An certainly they are not 500. :-)

They will still work as is!

Though we need this fix to comply with them, which is supposed to go
in for 3.18 rc[n].
"PM / Domains: Fix initial default state of the need_restore flag"

[...]

Kind regards
Uffe

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Ulf Hansson]

[...]

> 2) There are no requirements for arch/platforms/drivers to work in both cases
> CONFIG_PM_RUNTIME=y/n. But they must be built without errors/warning for both
> cases.

For cross SoC drivers this statement is not correct! Driver _must_
support the various combinations of CONFIG_PM*.

Therefore, I think it's better to strive towards a common solution and
to get the building blocks in place.

>
> For example, for Keystone 2 only CONFIG_PM_RUNTIME=y is going to be supported
> and if some one decided to disable it - it can be perfectly done from sys_fs/
> user space.

I can't see why the sysfs option to disable runtime PM should affect
this discussion. That's done at runtime and after the device has been
probed.

>
> 3) pm_runtime_get_sync()(or similar) is good not only because i's waking up device, but also
> because:
> - it can wake up chain of devices (dev->parent->parent->...)

That's right. But that's not what this patchset aims to do.

I realize that the header of the cover letter isn't describing the
problem I am trying to solve very well. I guess the below header would
have been better:

"PM / Domains: Power up PM domains prior drivers starts to probe their devices"

> - it can wake up power domain

Yes, but it requires CONFIG_PM_RUNTIME to be set.

Thus, to solve the problem during driver ->probe() we need another
solution which don't require CONFIG_PM_RUNTIME to be set. As this
patchset proposes.

> - it connects device to domain/class/type/bus and so allow to add additional PM layer on top
>   of Platform bus (for example arch/arm/mach-omap2/omap_device.c).
>
> So, it will do all needed things, and if it doesn't that problem is in platfrom/bus/driver
> code and not in Runtime PM.
> if pm_runtime_get_sync() will be dropped - than all above will need to be implemented
> around the ->probe().

I am not sure what you mean about dropping pm_runtime_get_sync()? All
I am saying is that we can't use it to power on PM domains during the
probe sequence.

Of course, you may still use pm_runtime_get_sync() from anywhere it's
needed, to for example handle device's parent/child relationships.

>
> 4) I've copied here comment from Rafael:
>  >>>> Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
>  >>>> it must take the fact that the driver's own ->runtime_resume() may be called
>  >>>> as a result of this into account.
>  Agree, that's a little bit annoying, but we are living with that for more then
>  5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior
>  (just walk through the drivers and you will see how many drivers expecting the same).
>
> So, any volunteers to check and fix ~500 drivers.

We don't have to change these drivers. An certainly they are not 500. :-)

They will still work as is!

Though we need this fix to comply with them, which is supposed to go
in for 3.18 rc[n].
"PM / Domains: Fix initial default state of the need_restore flag"

[...]

Kind regards
Uffe

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Grygorii Strashko]

Hi Ulf,

On 11/11/2014 01:05 PM, Ulf Hansson wrote:
> [...]
> 
>> 2) There are no requirements for arch/platforms/drivers to work in both cases
>> CONFIG_PM_RUNTIME=y/n. But they must be built without errors/warning for both
>> cases.
> 
> For cross SoC drivers this statement is not correct! Driver _must_
> support the various combinations of CONFIG_PM*.
> 
> Therefore, I think it's better to strive towards a common solution and
> to get the building blocks in place.

There are no common solution ;), because CONFIG_PM_GENERIC_DOMAINS can be "n" too :)
and number of combinations of config options to handle will grow. Right?
What if voltage or clocks domains will be added?

So, everything is about the decision made for arch/platform/soc/driver
and such problems should be solved individually.

Look, once you'll add in core code smth. like in this series - the driver's
code will partially lose ability to control PM. This, in turn, may lead
to the fact that PM management for some drivers or even SoC will be impossible
to add (assume you know that HW engineers can do absolutely crazy things to
reach power savings targets).

> 
>>
>> For example, for Keystone 2 only CONFIG_PM_RUNTIME=y is going to be supported
>> and if some one decided to disable it - it can be perfectly done from sys_fs/
>> user space.
> 
> I can't see why the sysfs option to disable runtime PM should affect
> this discussion. That's done at runtime and after the device has been
> probed.
> 
>>
>> 3) pm_runtime_get_sync()(or similar) is good not only because i's waking up device, but also
>> because:
>> - it can wake up chain of devices (dev->parent->parent->...)
> 
> That's right. But that's not what this patchset aims to do.
> 
> I realize that the header of the cover letter isn't describing the
> problem I am trying to solve very well. I guess the below header would
> have been better:
> 
> "PM / Domains: Power up PM domains prior drivers starts to probe their devices"

Yep. ;) You've mixed a lot of things together in your comments (.
And if i understand right - first of all you are fixing the issue for the case
CONFIG_PM_RUNTIME=y and, in my opinion, the root cause of such issues is 
exactly the usage of pm_runtime_set_active(), because it breaks
symmetry of Runtime PM.

As for me It much more simple to deal with well known limitation of Runtime PM:
"calling of driver's own ->runtime_resume() when pm_runtime_resume() called from probe"
and still benefit from all features provided by pm_runtime_get/put() than playing with pm_runtime_set_active() and:
- fixing unbalanced get/put calls (good example is http://marc.info/?l=linux-pm&m=141580289601886&w=2
  where unexpected (as for me) call of pm_runtime_get_noresume() is added in .remove() callback);
- fixing PD
- fixing enabling of parent devices (it will happen, believe me;).

> 
>> - it can wake up power domain
> 
> Yes, but it requires CONFIG_PM_RUNTIME to be set.
> 
> Thus, to solve the problem during driver ->probe() we need another
> solution which don't require CONFIG_PM_RUNTIME to be set. As this
> patchset proposes.
> 
>> - it connects device to domain/class/type/bus and so allow to add additional PM layer on top
>>    of Platform bus (for example arch/arm/mach-omap2/omap_device.c).
>>
>> So, it will do all needed things, and if it doesn't that problem is in platfrom/bus/driver
>> code and not in Runtime PM.
>> if pm_runtime_get_sync() will be dropped - than all above will need to be implemented
>> around the ->probe().
> 
> I am not sure what you mean about dropping pm_runtime_get_sync()? All
> I am saying is that we can't use it to power on PM domains during the
> probe sequence.

What I'm saying is that it should work with pm_runtime_get_sync
for the case CONFIG_PM_RUNTIME=y ;) and It might be better to power on PD once the
first device is attached to it if CONFIG_PM_RUNTIME=n and do this inside GPD code.
(^ it could even make your patch smaller)

> 
> Of course, you may still use pm_runtime_get_sync() from anywhere it's
> needed, to for example handle device's parent/child relationships.
> 
>>
>> 4) I've copied here comment from Rafael:
>>   >>>> Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
>>   >>>> it must take the fact that the driver's own ->runtime_resume() may be called
>>   >>>> as a result of this into account.
>>   Agree, that's a little bit annoying, but we are living with that for more then
>>   5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior
>>   (just walk through the drivers and you will see how many drivers expecting the same).
>>
>> So, any volunteers to check and fix ~500 drivers.
> 
> We don't have to change these drivers. An certainly they are not 500. :-)

+- 10% - I've calculated number of occurrences of pm_runtime_enable()  :)

> 
> They will still work as is!
> 
> Though we need this fix to comply with them, which is supposed to go
> in for 3.18 rc[n].
> "PM / Domains: Fix initial default state of the need_restore flag"

Ok. Looks like we both still hold one's ground :) I give up (found at about ~70 
drivers which use set_active() in probe - and it's at about of 20% of all 
Runtime PM enabled drivers).

But could you, at least, consider my proposition to enable PD from
dev_pm_domain_attach/detach once first device is attached to it, pls?

regards,
-grygorii

	

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Grygorii Strashko]

Hi Ulf,

On 11/11/2014 01:05 PM, Ulf Hansson wrote:
> [...]
> 
>> 2) There are no requirements for arch/platforms/drivers to work in both cases
>> CONFIG_PM_RUNTIME=y/n. But they must be built without errors/warning for both
>> cases.
> 
> For cross SoC drivers this statement is not correct! Driver _must_
> support the various combinations of CONFIG_PM*.
> 
> Therefore, I think it's better to strive towards a common solution and
> to get the building blocks in place.

There are no common solution ;), because CONFIG_PM_GENERIC_DOMAINS can be "n" too :)
and number of combinations of config options to handle will grow. Right?
What if voltage or clocks domains will be added?

So, everything is about the decision made for arch/platform/soc/driver
and such problems should be solved individually.

Look, once you'll add in core code smth. like in this series - the driver's
code will partially lose ability to control PM. This, in turn, may lead
to the fact that PM management for some drivers or even SoC will be impossible
to add (assume you know that HW engineers can do absolutely crazy things to
reach power savings targets).

> 
>>
>> For example, for Keystone 2 only CONFIG_PM_RUNTIME=y is going to be supported
>> and if some one decided to disable it - it can be perfectly done from sys_fs/
>> user space.
> 
> I can't see why the sysfs option to disable runtime PM should affect
> this discussion. That's done at runtime and after the device has been
> probed.
> 
>>
>> 3) pm_runtime_get_sync()(or similar) is good not only because i's waking up device, but also
>> because:
>> - it can wake up chain of devices (dev->parent->parent->...)
> 
> That's right. But that's not what this patchset aims to do.
> 
> I realize that the header of the cover letter isn't describing the
> problem I am trying to solve very well. I guess the below header would
> have been better:
> 
> "PM / Domains: Power up PM domains prior drivers starts to probe their devices"

Yep. ;) You've mixed a lot of things together in your comments (.
And if i understand right - first of all you are fixing the issue for the case
CONFIG_PM_RUNTIME=y and, in my opinion, the root cause of such issues is 
exactly the usage of pm_runtime_set_active(), because it breaks
symmetry of Runtime PM.

As for me It much more simple to deal with well known limitation of Runtime PM:
"calling of driver's own ->runtime_resume() when pm_runtime_resume() called from probe"
and still benefit from all features provided by pm_runtime_get/put() than playing with pm_runtime_set_active() and:
- fixing unbalanced get/put calls (good example is http://marc.info/?l=linux-pm&m=141580289601886&w=2
  where unexpected (as for me) call of pm_runtime_get_noresume() is added in .remove() callback);
- fixing PD
- fixing enabling of parent devices (it will happen, believe me;).

> 
>> - it can wake up power domain
> 
> Yes, but it requires CONFIG_PM_RUNTIME to be set.
> 
> Thus, to solve the problem during driver ->probe() we need another
> solution which don't require CONFIG_PM_RUNTIME to be set. As this
> patchset proposes.
> 
>> - it connects device to domain/class/type/bus and so allow to add additional PM layer on top
>>    of Platform bus (for example arch/arm/mach-omap2/omap_device.c).
>>
>> So, it will do all needed things, and if it doesn't that problem is in platfrom/bus/driver
>> code and not in Runtime PM.
>> if pm_runtime_get_sync() will be dropped - than all above will need to be implemented
>> around the ->probe().
> 
> I am not sure what you mean about dropping pm_runtime_get_sync()? All
> I am saying is that we can't use it to power on PM domains during the
> probe sequence.

What I'm saying is that it should work with pm_runtime_get_sync
for the case CONFIG_PM_RUNTIME=y ;) and It might be better to power on PD once the
first device is attached to it if CONFIG_PM_RUNTIME=n and do this inside GPD code.
(^ it could even make your patch smaller)

> 
> Of course, you may still use pm_runtime_get_sync() from anywhere it's
> needed, to for example handle device's parent/child relationships.
> 
>>
>> 4) I've copied here comment from Rafael:
>>   >>>> Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
>>   >>>> it must take the fact that the driver's own ->runtime_resume() may be called
>>   >>>> as a result of this into account.
>>   Agree, that's a little bit annoying, but we are living with that for more then
>>   5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior
>>   (just walk through the drivers and you will see how many drivers expecting the same).
>>
>> So, any volunteers to check and fix ~500 drivers.
> 
> We don't have to change these drivers. An certainly they are not 500. :-)

+- 10% - I've calculated number of occurrences of pm_runtime_enable()  :)

> 
> They will still work as is!
> 
> Though we need this fix to comply with them, which is supposed to go
> in for 3.18 rc[n].
> "PM / Domains: Fix initial default state of the need_restore flag"

Ok. Looks like we both still hold one's ground :) I give up (found at about ~70 
drivers which use set_active() in probe - and it's at about of 20% of all 
Runtime PM enabled drivers).

But could you, at least, consider my proposition to enable PD from
dev_pm_domain_attach/detach once first device is attached to it, pls?

regards,
-grygorii

	

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Friday, November 07, 2014 07:25:08 PM Grygorii Strashko wrote:

[cut]

> 
> 4) I've copied here comment from Rafael:
>  >>>> Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
>  >>>> it must take the fact that the driver's own ->runtime_resume() may be called
>  >>>> as a result of this into account.
>  Agree, that's a little bit annoying, but we are living with that for more then
>  5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior
>  (just walk through the drivers and you will see how many drivers expecting the same).
> 
> So, any volunteers to check and fix ~500 drivers.

Where did you get that number from?

Also please note that some bus types don't have this problem by design (e.g. PCI,
as pointed to by Alan).

[cut]

> > 
> >>
> >> - Runtime PM (if compiled in) needs to be enabled for all devices in power
> >>    domains by default.  Otherwise devices may lose power as a result for
> >>    power management of the other devices in the same domain.
> 
> It should prevent enabling/disabling of RPM from sys_fs too.

It looks like you're confusing disable/enable with auto/on.  These are different
things.

> >>
> >> - The core should try to power up domains before calling really_probe() both
> >>    for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the
> >>    "device is accessible" assumption.
> 
> Here I'm still think that pm_runtime_get_sync() (or similar) API should work.
> Another way, PM domain should decide what to do when the first device attached to it
> - power up and stay powered on for example
>  (It will work if devices are attached before ->probe();
>   it will not work if devices will be attached once they've been created, but this is different
>   question)

The PM domain itself can't do that.  The only place that has enough knowledge
is the code that enumerates devices (DT, ACPI or board-specific).

> > 
> > And how exactly will you then power up the PM domain when
> > CONFIG_PM_RUNTIME is unset?
> > 
> >>
> >> - Bus types may need to do more on top of that in their ->probe(), so the
> >>    driver's ->probe() can make that assumption too in all cases.
> >>
> >> Does that make sense to you?
> > 
> 
> I would like to take the liberty to add a couple of points from me:
>  - it seems reasonable to have ability to disable Runtime PM globally from sys_fs:
>    once disabled - "get" should power on device, "put" should do nothing all the
>    time except when ->remove() is called.

This is how the "power/control" file works and you can easily have this by writing
"on" to that file for every device.

>  
>  - It might be reasonable to add API like pm_runtime_probe_getXXX() which will do
>    everything what standard pm_runtime_get_sync() is doing with one exception:
>    it will not call driver's .runtime_resume() callback.

Use case?

>  - Just opinion. For GPD, It looks like integration/design problem and may be
>    it shouldn't be integrated with Runtime PM through dev_pm_ops. Instead, it
>    might be better to invoke it directly from  RPM core.

That I can't parse really, sorry.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Rafael J. Wysocki]

On Friday, November 07, 2014 07:25:08 PM Grygorii Strashko wrote:

[cut]

> 
> 4) I've copied here comment from Rafael:
>  >>>> Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
>  >>>> it must take the fact that the driver's own ->runtime_resume() may be called
>  >>>> as a result of this into account.
>  Agree, that's a little bit annoying, but we are living with that for more then
>  5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior
>  (just walk through the drivers and you will see how many drivers expecting the same).
> 
> So, any volunteers to check and fix ~500 drivers.

Where did you get that number from?

Also please note that some bus types don't have this problem by design (e.g. PCI,
as pointed to by Alan).

[cut]

> > 
> >>
> >> - Runtime PM (if compiled in) needs to be enabled for all devices in power
> >>    domains by default.  Otherwise devices may lose power as a result for
> >>    power management of the other devices in the same domain.
> 
> It should prevent enabling/disabling of RPM from sys_fs too.

It looks like you're confusing disable/enable with auto/on.  These are different
things.

> >>
> >> - The core should try to power up domains before calling really_probe() both
> >>    for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the
> >>    "device is accessible" assumption.
> 
> Here I'm still think that pm_runtime_get_sync() (or similar) API should work.
> Another way, PM domain should decide what to do when the first device attached to it
> - power up and stay powered on for example
>  (It will work if devices are attached before ->probe();
>   it will not work if devices will be attached once they've been created, but this is different
>   question)

The PM domain itself can't do that.  The only place that has enough knowledge
is the code that enumerates devices (DT, ACPI or board-specific).

> > 
> > And how exactly will you then power up the PM domain when
> > CONFIG_PM_RUNTIME is unset?
> > 
> >>
> >> - Bus types may need to do more on top of that in their ->probe(), so the
> >>    driver's ->probe() can make that assumption too in all cases.
> >>
> >> Does that make sense to you?
> > 
> 
> I would like to take the liberty to add a couple of points from me:
>  - it seems reasonable to have ability to disable Runtime PM globally from sys_fs:
>    once disabled - "get" should power on device, "put" should do nothing all the
>    time except when ->remove() is called.

This is how the "power/control" file works and you can easily have this by writing
"on" to that file for every device.

>  
>  - It might be reasonable to add API like pm_runtime_probe_getXXX() which will do
>    everything what standard pm_runtime_get_sync() is doing with one exception:
>    it will not call driver's .runtime_resume() callback.

Use case?

>  - Just opinion. For GPD, It looks like integration/design problem and may be
>    it shouldn't be integrated with Runtime PM through dev_pm_ops. Instead, it
>    might be better to invoke it directly from  RPM core.

That I can't parse really, sorry.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Pavel Machek]

Hi!

> 2) There are no requirements for arch/platforms/drivers to work in both cases
> CONFIG_PM_RUNTIME=y/n. But they must be built without errors/warning for both
> cases.

I don't think where you got that. They are config options, they should
work.

It is prefer not to compile than to compile but not work.
									Pavel

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

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Pavel Machek]

Hi!

> 2) There are no requirements for arch/platforms/drivers to work in both cases
> CONFIG_PM_RUNTIME=y/n. But they must be built without errors/warning for both
> cases.

I don't think where you got that. They are config options, they should
work.

It is prefer not to compile than to compile but not work.
									Pavel

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

Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Grygorii Strashko]

On 11/13/2014 04:07 AM, Rafael J. Wysocki wrote:
> On Friday, November 07, 2014 07:25:08 PM Grygorii Strashko wrote:
> 
> [cut]
> 
>>
>> 4) I've copied here comment from Rafael:
>>   >>>> Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
>>   >>>> it must take the fact that the driver's own ->runtime_resume() may be called
>>   >>>> as a result of this into account.
>>   Agree, that's a little bit annoying, but we are living with that for more then
>>   5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior
>>   (just walk through the drivers and you will see how many drivers expecting the same).
>>
>> So, any volunteers to check and fix ~500 drivers.
> 
> Where did you get that number from?

Sry, my bad. Rechecked - found 289 occurrences of pm_runtime_enable().
Number of Runtime PM enabled drivers = at about 289 +-10%.
- headers, suspend/resume,..
+ buses like amba and spmi 

> 
> Also please note that some bus types don't have this problem by design (e.g. PCI,
> as pointed to by Alan).

Right. I worry about Platform bus first of all, because HW PM implementation
(at least for ARM SoCs) can be very different there.

> 
> [cut]
> 
>>>
>>>>
>>>> - Runtime PM (if compiled in) needs to be enabled for all devices in power
>>>>     domains by default.  Otherwise devices may lose power as a result for
>>>>     power management of the other devices in the same domain.
>>
>> It should prevent enabling/disabling of RPM from sys_fs too.
> 
> It looks like you're confusing disable/enable with auto/on.  These are different
> things.
> 
>>>>
>>>> - The core should try to power up domains before calling really_probe() both
>>>>     for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the
>>>>     "device is accessible" assumption.
>>
>> Here I'm still think that pm_runtime_get_sync() (or similar) API should work.
>> Another way, PM domain should decide what to do when the first device attached to it
>> - power up and stay powered on for example
>>   (It will work if devices are attached before ->probe();
>>    it will not work if devices will be attached once they've been created, but this is different
>>    question)
> 
> The PM domain itself can't do that.  The only place that has enough knowledge
> is the code that enumerates devices (DT, ACPI or board-specific).
> 
>>>
>>> And how exactly will you then power up the PM domain when
>>> CONFIG_PM_RUNTIME is unset?
>>>
>>>>
>>>> - Bus types may need to do more on top of that in their ->probe(), so the
>>>>     driver's ->probe() can make that assumption too in all cases.
>>>>
>>>> Does that make sense to you?
>>>
>>
>> I would like to take the liberty to add a couple of points from me:
>>   - it seems reasonable to have ability to disable Runtime PM globally from sys_fs:
>>     once disabled - "get" should power on device, "put" should do nothing all the
>>     time except when ->remove() is called.
> 
> This is how the "power/control" file works and you can easily have this by writing
> "on" to that file for every device.
> 
>>   
>>   - It might be reasonable to add API like pm_runtime_probe_getXXX() which will do
>>     everything what standard pm_runtime_get_sync() is doing with one exception:
>>     it will not call driver's .runtime_resume() callback.
> 
> Use case?

This is just a different view (RFC) on your idea to call pm_runtime_get_sync()
 before dev->driver is initialized
("(b) bypassing the driver callbacks somehow".
  http://marc.info/?l=linux-pm&m=141505768026211&w=2)

- add some flag like dev->is_probing
- in pm_runtime_probe_get_sync() do
  dev->is_probing = true;
  pm_runtime_get_sync();
  dev->is_probing = false;
  
- update 3 places in code pm_generic_runtime_resume, pm_genpd_default_save_state and
  __rpm_get_callback like below (only for .runtime_resume):

+++ b/drivers/base/power/generic_ops.c
@@ -42,6 +42,9 @@ int pm_generic_runtime_resume(struct device *dev)
 {
        const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
        int ret;
+       
+       if (dev->is_probing)
+               pm = NULL;

Then drivers (at least platform drivers) will be able to call
  pm_runtime_enable(dev);
  pm_runtime_probe_get_sync(dev);
at any time they think is right (no problem with parent devices,
Power domain will be enabled, class/type/bus callback will be called,
no need to use pm_runtime_set_active()).

regards,
-grygorii

[PATCH v3 0/9] PM / Domains: Fix race conditions during boot

[Grygorii Strashko]

On 11/13/2014 04:07 AM, Rafael J. Wysocki wrote:
> On Friday, November 07, 2014 07:25:08 PM Grygorii Strashko wrote:
> 
> [cut]
> 
>>
>> 4) I've copied here comment from Rafael:
>>   >>>> Of course, if ->probe() is to call pm_runtime_resume() for this purpose,
>>   >>>> it must take the fact that the driver's own ->runtime_resume() may be called
>>   >>>> as a result of this into account.
>>   Agree, that's a little bit annoying, but we are living with that for more then
>>   5 years already (I'm 3 years) - so, I am, as driver developer, expecting above behavior
>>   (just walk through the drivers and you will see how many drivers expecting the same).
>>
>> So, any volunteers to check and fix ~500 drivers.
> 
> Where did you get that number from?

Sry, my bad. Rechecked - found 289 occurrences of pm_runtime_enable().
Number of Runtime PM enabled drivers = at about 289 +-10%.
- headers, suspend/resume,..
+ buses like amba and spmi 

> 
> Also please note that some bus types don't have this problem by design (e.g. PCI,
> as pointed to by Alan).

Right. I worry about Platform bus first of all, because HW PM implementation
(at least for ARM SoCs) can be very different there.

> 
> [cut]
> 
>>>
>>>>
>>>> - Runtime PM (if compiled in) needs to be enabled for all devices in power
>>>>     domains by default.  Otherwise devices may lose power as a result for
>>>>     power management of the other devices in the same domain.
>>
>> It should prevent enabling/disabling of RPM from sys_fs too.
> 
> It looks like you're confusing disable/enable with auto/on.  These are different
> things.
> 
>>>>
>>>> - The core should try to power up domains before calling really_probe() both
>>>>     for CONFIG_PM_RUNTIME set and unset, so ->probe() can always make the
>>>>     "device is accessible" assumption.
>>
>> Here I'm still think that pm_runtime_get_sync() (or similar) API should work.
>> Another way, PM domain should decide what to do when the first device attached to it
>> - power up and stay powered on for example
>>   (It will work if devices are attached before ->probe();
>>    it will not work if devices will be attached once they've been created, but this is different
>>    question)
> 
> The PM domain itself can't do that.  The only place that has enough knowledge
> is the code that enumerates devices (DT, ACPI or board-specific).
> 
>>>
>>> And how exactly will you then power up the PM domain when
>>> CONFIG_PM_RUNTIME is unset?
>>>
>>>>
>>>> - Bus types may need to do more on top of that in their ->probe(), so the
>>>>     driver's ->probe() can make that assumption too in all cases.
>>>>
>>>> Does that make sense to you?
>>>
>>
>> I would like to take the liberty to add a couple of points from me:
>>   - it seems reasonable to have ability to disable Runtime PM globally from sys_fs:
>>     once disabled - "get" should power on device, "put" should do nothing all the
>>     time except when ->remove() is called.
> 
> This is how the "power/control" file works and you can easily have this by writing
> "on" to that file for every device.
> 
>>   
>>   - It might be reasonable to add API like pm_runtime_probe_getXXX() which will do
>>     everything what standard pm_runtime_get_sync() is doing with one exception:
>>     it will not call driver's .runtime_resume() callback.
> 
> Use case?

This is just a different view (RFC) on your idea to call pm_runtime_get_sync()
 before dev->driver is initialized
("(b) bypassing the driver callbacks somehow".
  http://marc.info/?l=linux-pm&m=141505768026211&w=2)

- add some flag like dev->is_probing
- in pm_runtime_probe_get_sync() do
  dev->is_probing = true;
  pm_runtime_get_sync();
  dev->is_probing = false;
  
- update 3 places in code pm_generic_runtime_resume, pm_genpd_default_save_state and
  __rpm_get_callback like below (only for .runtime_resume):

+++ b/drivers/base/power/generic_ops.c
@@ -42,6 +42,9 @@ int pm_generic_runtime_resume(struct device *dev)
 {
        const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
        int ret;
+       
+       if (dev->is_probing)
+               pm = NULL;

Then drivers (at least platform drivers) will be able to call
  pm_runtime_enable(dev);
  pm_runtime_probe_get_sync(dev);
at any time they think is right (no problem with parent devices,
Power domain will be enabled, class/type/bus callback will be called,
no need to use pm_runtime_set_active()).

regards,
-grygorii