[PATCH v2 0/7] PM / Domains: Generic PM domains for CPUs/Clusters

[PATCH v2 0/7] PM / Domains: Generic PM domains for CPUs/Clusters

[Lina Iyer]

Changes since v1 [3]:
- Remove compatible for CPU PM domains, platform explicity calls an API to
  create and register a CPU PD.
- Remove init section initialization of platform callbacks.
- Review comments addressed in domain.c
- Move cpu-pd.c and cpu-pd.h out of ARM. However, changes to hotplug and
  cpuidle has only be made for ARM at this time.
- Define CPU devices as IRQ safe at init
- Prevent lockdep RCU false warnings
- Added Reviewed, Ack tags

Changes since RFC v2 [2]:

- Fix memory not released on error in pm_genpd_add_subdomain()
- Reworded commit texts and documentation
- Add Documentation for CPU PM domain and device tree
- Clean up CPU PD initialization code
- Add runtime PM support for CPU idle and hotplug instead of notifications
- Allow platform drivers to register for CPU PD callbacks
- Send CPU_PM notifications for cluster from the common code.
- Not including platform code as part of this series. Will submit separately.
- Rebased on top of linux-next
- Minor fix to comment in CPU_PM

Changes since RFC v1 [1]:

- Address review comments on v1.
- Incorporate Kevin's arch/arm/domain.c changes
- Drop drivers/base/power/cpu_domain.c
- Rebase on top of linux-next (to date)
- Reference implementation added.

This patchset fashions CPU clusters as generic PM domains. CPUs in most new
SOCs are grouped together as clusters, along with some supporting hardware,
GIC, L2 caches etc. When the CPUs in the cluster are powered off, these
hardware may also be powered off.

Generic PM domain  framework provides the necessary backend to build a cluster
hierarchy through devices, domains and nested domains. When devices and
sub-domains of a genpd are suspended, the genpd may also be suspended and
resumed before the first of the devices resumes. This works well for devices
and domains that operate in process context.

CPU idle operates with IRQs disabled. IRQs are disabled early in the CPU idle
operation and therefore any activity related to CPU's idle cannot sleep. The
cluster hardware has to support atomic operations if they are to be powered
on/off, along with the CPU. The limitation in using genpd framework for cluster
idle, is that genpd inherently uses mutexes for locking PM domains during
runtime suspend and resume and therefore may sleep during these operations. If
this limitation were to be removed, CPU clusters can be represented as devices
attached to a PM domain and when the CPUs are in runtime idle, the PM domain
can also be suspended.

The approach here is simple, allow genpd domains to specify the type of locking
the domain would use. A genpd that can suspend and resume in an IRQ safe
context, would initialize a spinlock as the genpd lock instead of a mutex.
Therefore, IRQ safe devices may initiate a genpd suspend when the last active
device goes idle. In a CPU domain, the last CPU powering down, may now program
the domain hardware to suspend, when the CPU enters idle. Thus when all the
CPUs are in idle, the domain and therefore the caches, VFP, GIC, Coresight,
power controller and other peripheral hardware may also be in a low power
state. This can save a considerable amount of runtime power.

Thanks,
Lina

[1]. http://www.spinics.net/lists/arm-kernel/msg423430.html
[2]. http://lists.infradead.org/pipermail/linux-arm-kernel/2015-June/352787.html
[3]. https://lwn.net/Articles/653579/

Lina Iyer (7):
  PM / Domains: Allocate memory outside domain locks
  PM / Domains: Support IRQ safe PM domains
  drivers: cpu: Define CPU devices as IRQ safe
  PM / Domains: Introduce PM domains for CPUs/clusters
  ARM: cpuidle: Add runtime PM support for CPU idle
  ARM64: smp: Add runtime PM support for CPU hotplug
  ARM: smp: Add runtime PM support for CPU hotplug

 Documentation/arm/cpu-domains.txt |  56 +++++++++
 Documentation/power/devices.txt   |  11 +-
 arch/arm/kernel/smp.c             |  18 ++-
 arch/arm64/kernel/smp.c           |  16 +++
 drivers/base/cpu.c                |   6 +-
 drivers/base/power/Makefile       |   2 +-
 drivers/base/power/cpu-pd.c       | 206 ++++++++++++++++++++++++++++++++
 drivers/base/power/domain.c       | 242 +++++++++++++++++++++++++++++---------
 drivers/cpuidle/cpuidle-arm.c     |  11 ++
 include/linux/cpu-pd.h            |  35 ++++++
 include/linux/pm_domain.h         |  11 +-
 11 files changed, 550 insertions(+), 64 deletions(-)
 create mode 100644 Documentation/arm/cpu-domains.txt
 create mode 100644 drivers/base/power/cpu-pd.c
 create mode 100644 include/linux/cpu-pd.h

-- 
2.1.4

[PATCH v2 1/7] PM / Domains: Allocate memory outside domain locks

[Lina Iyer]

In preparation for supporting IRQ-safe domains, allocate domain data
outside the domain locks. These functions are not called in an atomic
context, so we can always allocate memory using GFP_KERNEL. By
allocating memory before the locks, we can safely lock the domain using
spinlocks instead of mutexes.

Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Krzysztof Koz?owski <k.kozlowski@samsung.com>
Reviewed-by: Kevin Hilman <khilman@linaro.org>
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/base/power/domain.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index e45a0ef..ef8d19f 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1381,13 +1381,17 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 			   struct generic_pm_domain *subdomain)
 {
-	struct gpd_link *link;
+	struct gpd_link *link, *itr;
 	int ret = 0;
 
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain)
 	    || genpd == subdomain)
 		return -EINVAL;
 
+	link = kzalloc(sizeof(*link), GFP_KERNEL);
+	if (!link)
+		return -ENOMEM;
+
 	mutex_lock(&genpd->lock);
 	mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
 
@@ -1397,18 +1401,13 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 		goto out;
 	}
 
-	list_for_each_entry(link, &genpd->master_links, master_node) {
-		if (link->slave == subdomain && link->master == genpd) {
+	list_for_each_entry(itr, &genpd->master_links, master_node) {
+		if (itr->slave == subdomain && itr->master == genpd) {
 			ret = -EINVAL;
 			goto out;
 		}
 	}
 
-	link = kzalloc(sizeof(*link), GFP_KERNEL);
-	if (!link) {
-		ret = -ENOMEM;
-		goto out;
-	}
 	link->master = genpd;
 	list_add_tail(&link->master_node, &genpd->master_links);
 	link->slave = subdomain;
@@ -1419,7 +1418,8 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
  out:
 	mutex_unlock(&subdomain->lock);
 	mutex_unlock(&genpd->lock);
-
+	if (ret)
+		kfree(link);
 	return ret;
 }
 
@@ -1510,17 +1510,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
 	if (IS_ERR_OR_NULL(genpd) || state < 0)
 		return -EINVAL;
 
+	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
+	if (!cpuidle_data)
+		return -ENOMEM;
+
 	mutex_lock(&genpd->lock);
 
 	if (genpd->cpuidle_data) {
 		ret = -EEXIST;
-		goto out;
-	}
-	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
-	if (!cpuidle_data) {
-		ret = -ENOMEM;
-		goto out;
+		goto err_drv;
 	}
+
 	cpuidle_drv = cpuidle_driver_ref();
 	if (!cpuidle_drv) {
 		ret = -ENODEV;
-- 
2.1.4

[PATCH v2 2/7] PM / Domains: Support IRQ safe PM domains

[Lina Iyer]

Generic Power Domains currently support turning on/off only in process
context. This prevents the usage of PM domains for domains that could be
powered on/off in a context where IRQs are disabled. Many such domains
exist today and do not get powered off, when the IRQ safe devices in
that domain are powered off, because of this limitation.

However, not all domains can operate in IRQ safe contexts. Genpd
therefore, has to support both cases where the domain may or may not
operate in IRQ safe contexts. Configuring genpd to use an appropriate
lock for that domain, would allow domains that have IRQ safe devices to
runtime suspend and resume, in atomic context.

To achieve domain specific locking, set the domain's ->flag to
GENPD_FLAG_IRQ_SAFE while defining the domain. This indicates that genpd
should use a spinlock instead of a mutex for locking the domain. Locking
is abstracted through genpd_lock() and genpd_unlock() functions that use
the flag to determine the appropriate lock to be used for that domain.
Domains that have lower latency to suspend and resume and can operate
with IRQs disabled may now be able to save power, when the component
devices and sub-domains are idle at runtime.

The restriction this imposes on the domain hierarchy is that sub-domains
and all devices in the IRQ safe domain's hierarchy also have to be IRQ
safe, so that we dont try to lock a mutex, while holding a spinlock.
Non-IRQ safe domains may continue to have devices and sub-domains that
may or may not be IRQ safe.

Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Krzysztof Koz?owski <k.kozlowski@samsung.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 Documentation/power/devices.txt |  11 ++-
 drivers/base/power/domain.c     | 212 ++++++++++++++++++++++++++++++++--------
 include/linux/pm_domain.h       |  11 ++-
 3 files changed, 189 insertions(+), 45 deletions(-)

diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
index 8ba6625..bde6141 100644
--- a/Documentation/power/devices.txt
+++ b/Documentation/power/devices.txt
@@ -607,7 +607,16 @@ individually.  Instead, a set of devices sharing a power resource can be put
 into a low-power state together at the same time by turning off the shared
 power resource.  Of course, they also need to be put into the full-power state
 together, by turning the shared power resource on.  A set of devices with this
-property is often referred to as a power domain.
+property is often referred to as a power domain. A power domain may also be
+nested inside another power domain.
+
+Devices, by default, operate in process context and if a device can operate in
+IRQ safe context, has to be explicitly set as IRQ safe. Power domains by
+default, operate in process context but could have devices that are IRQ safe.
+Such power domains cannot be powered on/off during runtime PM. On the other
+hand, an IRQ safe PM domain that can be powered on/off and suspended or resumed
+in an atomic context, may contain IRQ safe devices. Such domains may only
+contain IRQ safe devices or IRQ safe sub-domains.
 
 Support for power domains is provided through the pm_domain field of struct
 device.  This field is a pointer to an object of type struct dev_pm_domain,
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index ef8d19f..9849338 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -53,6 +53,80 @@
 static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
 
+static inline int genpd_lock_nosleep(struct generic_pm_domain *genpd,
+					unsigned int subclass)
+	__acquires(&genpd->slock)
+{
+	unsigned long flags;
+
+	if (subclass > 0)
+		spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
+	else
+		spin_lock_irqsave(&genpd->slock, flags);
+
+	genpd->lock_flags = flags;
+	return 0;
+}
+
+static inline void genpd_unlock_nosleep(struct generic_pm_domain *genpd)
+	__releases(&genpd->slock)
+{
+	spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
+}
+
+static inline int genpd_lock_irq(struct generic_pm_domain *genpd,
+					unsigned int subclass)
+	__acquires(&genpd->mlock)
+{
+	if (subclass > 0)
+		mutex_lock_nested(&genpd->mlock, subclass);
+	else
+		mutex_lock(&genpd->mlock);
+	return 0;
+}
+
+static inline int genpd_lock_interruptible_irq(struct generic_pm_domain *genpd)
+	__acquires(&genpd->mlock)
+{
+	return mutex_lock_interruptible(&genpd->mlock);
+}
+
+static inline void genpd_unlock_irq(struct generic_pm_domain *genpd)
+	__releases(&genpd->mlock)
+{
+	mutex_unlock(&genpd->mlock);
+}
+
+static inline int genpd_lock(struct generic_pm_domain *genpd)
+{
+	return genpd->irq_safe ? genpd_lock_nosleep(genpd, 0)
+			: genpd_lock_irq(genpd, 0);
+}
+
+static inline int genpd_lock_nested(struct generic_pm_domain *genpd)
+{
+	return genpd->irq_safe ? genpd_lock_nosleep(genpd, SINGLE_DEPTH_NESTING)
+			: genpd_lock_irq(genpd, SINGLE_DEPTH_NESTING);
+}
+
+static inline int genpd_lock_interruptible(struct generic_pm_domain *genpd)
+{
+	return genpd->irq_safe ? genpd_lock_nosleep(genpd, 0)
+			: genpd_lock_interruptible_irq(genpd);
+}
+
+static inline void genpd_unlock(struct generic_pm_domain *genpd)
+{
+	return genpd->irq_safe ? genpd_unlock_nosleep(genpd)
+			: genpd_unlock_irq(genpd);
+}
+
+static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev,
+		struct generic_pm_domain *genpd)
+{
+	return dev->power.irq_safe && !genpd->irq_safe;
+}
+
 static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
 {
 	struct generic_pm_domain *genpd = NULL, *gpd;
@@ -273,9 +347,9 @@ int pm_genpd_poweron(struct generic_pm_domain *genpd)
 {
 	int ret;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 	ret = __pm_genpd_poweron(genpd);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 	return ret;
 }
 
@@ -335,9 +409,9 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
 		spin_unlock_irq(&dev->power.lock);
 
 		if (!IS_ERR(genpd)) {
-			mutex_lock(&genpd->lock);
+			genpd_lock(genpd);
 			genpd->max_off_time_changed = true;
-			mutex_unlock(&genpd->lock);
+			genpd_unlock(genpd);
 		}
 
 		dev = dev->parent;
@@ -394,8 +468,17 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 		if (stat > PM_QOS_FLAGS_NONE)
 			return -EBUSY;
 
-		if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
+		/*
+		 * We do not want to power off the domain if the device is
+		 * not suspended or an IRQ safe device is part of this
+		 * non-IRQ safe domain.
+		 */
+		if (!pm_runtime_suspended(pdd->dev) ||
+			irq_safe_dev_in_no_sleep_domain(pdd->dev, genpd))
 			not_suspended++;
+		WARN_ONCE(irq_safe_dev_in_no_sleep_domain(pdd->dev, genpd),
+				"PM domain %s will not be powered off\n",
+				genpd->name);
 	}
 
 	if (not_suspended > genpd->in_progress)
@@ -460,9 +543,9 @@ static void genpd_power_off_work_fn(struct work_struct *work)
 
 	genpd = container_of(work, struct generic_pm_domain, power_off_work);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 	pm_genpd_poweroff(genpd);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 }
 
 /**
@@ -500,17 +583,21 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 	}
 
 	/*
-	 * If power.irq_safe is set, this routine will be run with interrupts
-	 * off, so it can't use mutexes.
+	 * If power.irq_safe is set, this routine may be run with
+	 * IRQ disabled, so suspend only if the power domain is
+	 * irq_safe.
 	 */
-	if (dev->power.irq_safe)
+	WARN_ONCE(irq_safe_dev_in_no_sleep_domain(dev, genpd),
+			"genpd %s will not be powered off\n", genpd->name);
+	if (irq_safe_dev_in_no_sleep_domain(dev, genpd))
 		return 0;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
+
 	genpd->in_progress++;
 	pm_genpd_poweroff(genpd);
 	genpd->in_progress--;
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	return 0;
 }
@@ -535,15 +622,18 @@ static int pm_genpd_runtime_resume(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	/* If power.irq_safe, the PM domain is never powered off. */
-	if (dev->power.irq_safe) {
+	/*
+	 * As we dont power off a non IRQ safe domain, which holds
+	 * an IRQ safe device, we dont need to restore power to it.
+	 */
+	if (dev->power.irq_safe && !genpd->irq_safe) {
 		timed = false;
 		goto out;
 	}
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 	ret = __pm_genpd_poweron(genpd);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	if (ret)
 		return ret;
@@ -743,14 +833,14 @@ static int pm_genpd_prepare(struct device *dev)
 	if (resume_needed(dev, genpd))
 		pm_runtime_resume(dev);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->prepared_count++ == 0) {
 		genpd->suspended_count = 0;
 		genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
 	}
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	if (genpd->suspend_power_off) {
 		pm_runtime_put_noidle(dev);
@@ -768,12 +858,12 @@ static int pm_genpd_prepare(struct device *dev)
 
 	ret = pm_generic_prepare(dev);
 	if (ret) {
-		mutex_lock(&genpd->lock);
+		genpd_lock(genpd);
 
 		if (--genpd->prepared_count == 0)
 			genpd->suspend_power_off = false;
 
-		mutex_unlock(&genpd->lock);
+		genpd_unlock(genpd);
 		pm_runtime_enable(dev);
 	}
 
@@ -1130,13 +1220,13 @@ static void pm_genpd_complete(struct device *dev)
 	if (IS_ERR(genpd))
 		return;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	run_complete = !genpd->suspend_power_off;
 	if (--genpd->prepared_count == 0)
 		genpd->suspend_power_off = false;
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	if (run_complete) {
 		pm_generic_complete(dev);
@@ -1280,11 +1370,18 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
 		return -EINVAL;
 
+	if (genpd->irq_safe && !dev->power.irq_safe) {
+		dev_err(dev,
+			"PM Domain %s is IRQ safe; device has to IRQ safe.\n",
+			genpd->name);
+		return -EINVAL;
+	}
+
 	gpd_data = genpd_alloc_dev_data(dev, genpd, td);
 	if (IS_ERR(gpd_data))
 		return PTR_ERR(gpd_data);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->prepared_count > 0) {
 		ret = -EAGAIN;
@@ -1301,7 +1398,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
 	list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
 
  out:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	if (ret)
 		genpd_free_dev_data(dev, gpd_data);
@@ -1345,7 +1442,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 	gpd_data = to_gpd_data(pdd);
 	dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->prepared_count > 0) {
 		ret = -EAGAIN;
@@ -1360,14 +1457,14 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
 	list_del_init(&pdd->list_node);
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	genpd_free_dev_data(dev, gpd_data);
 
 	return 0;
 
  out:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 	dev_pm_qos_add_notifier(dev, &gpd_data->nb);
 
 	return ret;
@@ -1388,12 +1485,23 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 	    || genpd == subdomain)
 		return -EINVAL;
 
+	/*
+	 * If the domain can be powered on/off in an IRQ safe
+	 * context, ensure that the subdomain can also be
+	 * powered on/off in that context.
+	 */
+	if (genpd->irq_safe && !subdomain->irq_safe) {
+		WARN("Sub-domain (%s) in an IRQ-safe domain (%s) has to be IRQ safe\n",
+				subdomain->name, genpd->name);
+		return -EINVAL;
+	}
+
 	link = kzalloc(sizeof(*link), GFP_KERNEL);
 	if (!link)
 		return -ENOMEM;
 
-	mutex_lock(&genpd->lock);
-	mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
+	genpd_lock(genpd);
+	genpd_lock_nested(subdomain);
 
 	if (genpd->status == GPD_STATE_POWER_OFF
 	    &&  subdomain->status != GPD_STATE_POWER_OFF) {
@@ -1416,8 +1524,8 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
 		genpd_sd_counter_inc(genpd);
 
  out:
-	mutex_unlock(&subdomain->lock);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(subdomain);
+	genpd_unlock(genpd);
 	if (ret)
 		kfree(link);
 	return ret;
@@ -1466,13 +1574,13 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 	if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain))
 		return -EINVAL;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	list_for_each_entry(link, &genpd->master_links, master_node) {
 		if (link->slave != subdomain)
 			continue;
 
-		mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
+		genpd_lock_nested(subdomain);
 
 		list_del(&link->master_node);
 		list_del(&link->slave_node);
@@ -1480,13 +1588,13 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
 		if (subdomain->status != GPD_STATE_POWER_OFF)
 			genpd_sd_counter_dec(genpd);
 
-		mutex_unlock(&subdomain->lock);
+		genpd_unlock(subdomain);
 
 		ret = 0;
 		break;
 	}
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	return ret;
 }
@@ -1510,11 +1618,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
 	if (IS_ERR_OR_NULL(genpd) || state < 0)
 		return -EINVAL;
 
+	if (genpd->irq_safe) {
+		WARN("Domain %s is IRQ safe, cannot attach to cpuidle\n",
+				genpd->name);
+		return -EINVAL;
+	}
+
 	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
 	if (!cpuidle_data)
 		return -ENOMEM;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	if (genpd->cpuidle_data) {
 		ret = -EEXIST;
@@ -1541,7 +1655,7 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
 	genpd_recalc_cpu_exit_latency(genpd);
 
  out:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 	return ret;
 
  err:
@@ -1578,7 +1692,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
 	if (IS_ERR_OR_NULL(genpd))
 		return -EINVAL;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock(genpd);
 
 	cpuidle_data = genpd->cpuidle_data;
 	if (!cpuidle_data) {
@@ -1596,7 +1710,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
 	kfree(cpuidle_data);
 
  out:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 	return ret;
 }
 
@@ -1657,6 +1771,17 @@ static int pm_genpd_default_restore_state(struct device *dev)
 	return cb ? cb(dev) : 0;
 }
 
+static void genpd_lock_init(struct generic_pm_domain *genpd)
+{
+	if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
+		spin_lock_init(&genpd->slock);
+		genpd->irq_safe = true;
+	} else {
+		mutex_init(&genpd->mlock);
+		genpd->irq_safe = false;
+	}
+}
+
 /**
  * pm_genpd_init - Initialize a generic I/O PM domain object.
  * @genpd: PM domain object to initialize.
@@ -1672,7 +1797,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
 	INIT_LIST_HEAD(&genpd->master_links);
 	INIT_LIST_HEAD(&genpd->slave_links);
 	INIT_LIST_HEAD(&genpd->dev_list);
-	mutex_init(&genpd->lock);
+	genpd_lock_init(genpd);
 	genpd->gov = gov;
 	INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
 	genpd->in_progress = 0;
@@ -2067,7 +2192,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
 	struct gpd_link *link;
 	int ret;
 
-	ret = mutex_lock_interruptible(&genpd->lock);
+	ret = genpd_lock_interruptible(genpd);
 	if (ret)
 		return -ERESTARTSYS;
 
@@ -2087,7 +2212,8 @@ static int pm_genpd_summary_one(struct seq_file *s,
 	}
 
 	list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
-		kobj_path = kobject_get_path(&pm_data->dev->kobj, GFP_KERNEL);
+		kobj_path = kobject_get_path(&pm_data->dev->kobj,
+				genpd->irq_safe ? GFP_ATOMIC : GFP_KERNEL);
 		if (kobj_path == NULL)
 			continue;
 
@@ -2098,7 +2224,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
 
 	seq_puts(s, "\n");
 exit:
-	mutex_unlock(&genpd->lock);
+	genpd_unlock(genpd);
 
 	return 0;
 }
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index b2725e6..dc7cb53 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -16,9 +16,11 @@
 #include <linux/of.h>
 #include <linux/notifier.h>
 #include <linux/cpuidle.h>
+#include <linux/spinlock.h>
 
 /* Defines used for the flags field in the struct generic_pm_domain */
 #define GENPD_FLAG_PM_CLK	(1U << 0) /* PM domain uses PM clk */
+#define GENPD_FLAG_IRQ_SAFE	(1U << 1) /* PM domain operates in atomic */
 
 enum gpd_status {
 	GPD_STATE_ACTIVE = 0,	/* PM domain is active */
@@ -49,7 +51,6 @@ struct generic_pm_domain {
 	struct list_head master_links;	/* Links with PM domain as a master */
 	struct list_head slave_links;	/* Links with PM domain as a slave */
 	struct list_head dev_list;	/* List of devices */
-	struct mutex lock;
 	struct dev_power_governor *gov;
 	struct work_struct power_off_work;
 	const char *name;
@@ -74,6 +75,14 @@ struct generic_pm_domain {
 	void (*detach_dev)(struct generic_pm_domain *domain,
 			   struct device *dev);
 	unsigned int flags;		/* Bit field of configs for genpd */
+	bool irq_safe;
+	union {
+		struct mutex mlock;
+		struct {
+			spinlock_t slock;
+			unsigned long lock_flags;
+		};
+	};
 };
 
 static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
-- 
2.1.4

[PATCH v2 3/7] drivers: cpu: Define CPU devices as IRQ safe

[Lina Iyer]

CPUs may be powered off from CPUIdle or hotplug and are called with
IRQ's disabled. Define CPU devices as IRQ safe, so they may be runtime
suspended/resumed.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/base/cpu.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 78720e7..56cb1d2 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -17,6 +17,7 @@
 #include <linux/of.h>
 #include <linux/cpufeature.h>
 #include <linux/tick.h>
+#include <linux/pm_runtime.h>
 
 #include "base.h"
 
@@ -371,10 +372,11 @@ int register_cpu(struct cpu *cpu, int num)
 	if (cpu->hotpluggable)
 		cpu->dev.groups = hotplugable_cpu_attr_groups;
 	error = device_register(&cpu->dev);
-	if (!error)
+	if (!error) {
+		pm_runtime_irq_safe(&cpu->dev);
 		per_cpu(cpu_sys_devices, num) = &cpu->dev;
-	if (!error)
 		register_cpu_under_node(num, cpu_to_node(num));
+	}
 
 	return error;
 }
-- 
2.1.4

[PATCH v2 4/7] PM / Domains: Introduce PM domains for CPUs/clusters

[Lina Iyer]

Define and add Generic PM domains (genpd) for CPU clusters. Many new
SoCs group CPUs as clusters. Clusters share common resources like power
rails, caches, VFP, Coresight etc. When all CPUs in the cluster are
idle, these shared resources may also be put in their idle state.

The idle time between the last CPU entering idle and a CPU resuming
execution is an opportunity for these shared resources to be powered
down. Generic PM domain provides a framework for defining such power
domains and attach devices to the domain. When the devices in the domain
are idle at runtime, the domain would also be suspended and resumed
before the first of the devices resume execution.

We define a generic PM domain for each cluster and attach CPU devices in
the cluster to that PM domain. The DT definitions for the SoC describe
this relationship. Genpd callbacks for power_on and power_off can then
be used to power up/down the shared resources for the domain.

Cc: Stephen Boyd <sboyd@codeaurora.org>
Cc: Kevin Hilman <khilman@linaro.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Kevin Hilman <khilman@linaro.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 Documentation/arm/cpu-domains.txt |  56 +++++++++++
 drivers/base/power/Makefile       |   2 +-
 drivers/base/power/cpu-pd.c       | 206 ++++++++++++++++++++++++++++++++++++++
 include/linux/cpu-pd.h            |  35 +++++++
 4 files changed, 298 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/arm/cpu-domains.txt
 create mode 100644 drivers/base/power/cpu-pd.c
 create mode 100644 include/linux/cpu-pd.h

diff --git a/Documentation/arm/cpu-domains.txt b/Documentation/arm/cpu-domains.txt
new file mode 100644
index 0000000..0ac0411
--- /dev/null
+++ b/Documentation/arm/cpu-domains.txt
@@ -0,0 +1,56 @@
+CPU Clusters and PM domain
+
+Newer CPUs are grouped in a SoC as clusters. A cluster in addition to the
+CPUs may have caches, GIC, VFP and architecture specific power controller to
+power the cluster. A cluster may also be nested in another cluster, the
+hierarchy of which is depicted in the device tree. CPUIdle frameworks enables
+the CPUs to determine the sleep time and enter low power state to save power
+during periods of idle. CPUs in a cluster may enter and exit idle state
+independently. During the time when all the CPUs are in idle state, the
+cluster can safely be in idle state as well. When the last of the CPUs is
+powered off as a result of idle, the cluster may also be powered down, but the
+domain must be powered on before the first of the CPUs in the cluster resumes
+execution.
+
+SoCs can power down the CPU and resume execution in a few uSecs and the domain
+that powers the CPU cluster also have comparable idle latencies. The CPU WFI
+signal in ARM CPUs is used as a hardware trigger for the cluster hardware to
+enter their idle state. The hardware can be programmed in advance to put the
+cluster in the desired idle state befitting the wakeup latency requested by
+the CPUs. When all the CPUs in a cluster have executed their WFI instruction,
+the state machine for the power controller may put the cluster components in
+their power down or idle state. Generally, the domains would power on with the
+hardware sensing the CPU's interrupts. The domains may however, need to be
+reconfigured by the CPU to remain active, until the last CPU is ready to enter
+idle again. To power down a cluster, it is generally required to power down
+all the CPUs. The caches would also need to be flushed. The hardware state of
+some of the components may need to be saved and restored when powered back on.
+SoC vendors may also have hardware specific configuration that must be done
+before the cluster can be powered off. When the cluster is powered off,
+notifications may be sent out to other SoC components to scale down or even
+power off their resources.
+
+Power management domains represent relationship of devices and their power
+controllers. They are represented in the DT as domain consumers and providers.
+A device may have a domain provider and a domain provider may support multiple
+domain consumers. Domains like clusters, may also be nested inside one
+another. A domain that has no active consumer, may be powered off and any
+resuming consumer would trigger the domain back to active. Parent domains may
+be powered off when the child domains are powered off. The CPU cluster can be
+fashioned as a PM domain. When the CPU devices are powered off, the PM domain
+may be powered off.
+
+The code in Generic PM domains handles the hierarchy of devices, domains and
+the reference counting of objects leading to last man down and first man up.
+The CPU domains core code defines PM domains for each CPU cluster and attaches
+the domains' CPU devices to as specified in the DT. Platform drivers may use
+the following API to register their CPU PM domains.
+
+of_init_cpu_pm_domain() -
+Provides a single step registration of the CPU PM domain and attach CPUs to
+the genpd. Platform drivers may additionally register callbacks for power_on
+and power_off operations.
+
+of_register_cpu_pm_domain() -
+Platforms that need to initialize their genpd, can use this method to
+initialize a CPU PD. The genpd and the callbacks are supplied through the @pd.
diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
index f94a6cc..db2787f 100644
--- a/drivers/base/power/Makefile
+++ b/drivers/base/power/Makefile
@@ -2,7 +2,7 @@ obj-$(CONFIG_PM)	+= sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
 obj-$(CONFIG_PM_SLEEP)	+= main.o wakeup.o
 obj-$(CONFIG_PM_TRACE_RTC)	+= trace.o
 obj-$(CONFIG_PM_OPP)	+= opp.o
-obj-$(CONFIG_PM_GENERIC_DOMAINS)	+=  domain.o domain_governor.o
+obj-$(CONFIG_PM_GENERIC_DOMAINS)	+=  domain.o domain_governor.o cpu-pd.o
 obj-$(CONFIG_HAVE_CLK)	+= clock_ops.o
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
diff --git a/drivers/base/power/cpu-pd.c b/drivers/base/power/cpu-pd.c
new file mode 100644
index 0000000..5f30025
--- /dev/null
+++ b/drivers/base/power/cpu-pd.c
@@ -0,0 +1,206 @@
+/*
+ * CPU Generic PM Domain.
+ *
+ * Copyright (C) 2015 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define DEBUG
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/cpu_pm.h>
+#include <linux/cpu-pd.h>
+#include <linux/device.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+
+#define NAME_MAX 36
+
+/* List of CPU PM domains we care about */
+static LIST_HEAD(of_cpu_pd_list);
+
+static inline
+struct cpu_pm_domain *to_cpu_pd(struct generic_pm_domain *d)
+{
+	struct cpu_pm_domain *pd;
+
+	list_for_each_entry(pd, &of_cpu_pd_list, link) {
+		if (pd->genpd == d)
+			return pd;
+	}
+
+	return NULL;
+}
+
+static int cpu_pd_power_off(struct generic_pm_domain *genpd)
+{
+	struct cpu_pm_domain *pd = to_cpu_pd(genpd);
+
+	if (pd->plat_ops.power_off)
+		pd->plat_ops.power_off(genpd);
+
+	/*
+	 * Notify CPU PM domain power down
+	 * TODO: Call the notificated directly from here.
+	 */
+	cpu_cluster_pm_enter();
+
+	return 0;
+}
+
+static int cpu_pd_power_on(struct generic_pm_domain *genpd)
+{
+	struct cpu_pm_domain *pd = to_cpu_pd(genpd);
+
+	if (pd->plat_ops.power_on)
+		pd->plat_ops.power_on(genpd);
+
+	/* Notify CPU PM domain power up */
+	cpu_cluster_pm_exit();
+
+	return 0;
+}
+
+static void run_cpu(void *unused)
+{
+	struct device *cpu_dev = get_cpu_device(smp_processor_id());
+
+	/* We are running, increment the usage count */
+	pm_runtime_get_noresume(cpu_dev);
+}
+
+static int of_pm_domain_attach_cpus(void)
+{
+	int cpuid, ret;
+
+	/* Find any CPU nodes with a phandle to this power domain */
+	for_each_possible_cpu(cpuid) {
+		struct device *cpu_dev;
+
+		cpu_dev = get_cpu_device(cpuid);
+		if (!cpu_dev) {
+			pr_warn("%s: Unable to get device for CPU%d\n",
+					__func__, cpuid);
+			return -ENODEV;
+		}
+
+		if (cpu_online(cpuid)) {
+			pm_runtime_set_active(cpu_dev);
+			/*
+			 * Execute the below on that 'cpu' to ensure that the
+			 * reference counting is correct. Its possible that
+			 * while this code is executing, the 'cpu' may be
+			 * powered down, but we may incorrectly increment the
+			 * usage. By executing the get_cpu on the 'cpu',
+			 * we can ensure that the 'cpu' and its usage count are
+			 * matched.
+			 */
+			smp_call_function_single(cpuid, run_cpu, NULL, true);
+		} else {
+			pm_runtime_set_suspended(cpu_dev);
+		}
+		pm_runtime_enable(cpu_dev);
+
+		/*
+		 * We attempt to attach the device to genpd again. We would
+		 * have failed in our earlier attempt to attach to the domain
+		 * provider as the CPU device would not have been IRQ safe,
+		 * while the domain is defined as IRQ safe. IRQ safe domains
+		 * can only have IRQ safe devices.
+		 */
+		ret = genpd_dev_pm_attach(cpu_dev);
+		if (ret) {
+			dev_warn(cpu_dev,
+				"%s: Unable to attach to power-domain: %d\n",
+				__func__, ret);
+			pm_runtime_disable(cpu_dev);
+		} else
+			dev_dbg(cpu_dev, "Attached to PM domain\n");
+	}
+
+	return 0;
+}
+
+/**
+ * of_register_cpu_pm_domain() - Register the CPU PM domain with GenPD
+ * framework
+ * @dn: PM domain provider device node
+ * @pd: The CPU PM domain that has been initialized
+ *
+ * This can be used by the platform code to setup the ->genpd of the @pd
+ * The platform can also set up its callbacks in the ->plat_ops.
+ */
+int of_register_cpu_pm_domain(struct device_node *dn,
+		struct cpu_pm_domain *pd)
+{
+
+	if (!pd || !pd->genpd)
+		return -EINVAL;
+
+	/*
+	 * The platform should not set up the genpd callbacks.
+	 * They should setup the pd->plat_ops instead.
+	 */
+	WARN_ON(pd->genpd->power_off);
+	WARN_ON(pd->genpd->power_on);
+
+	pd->genpd->power_off = cpu_pd_power_off;
+	pd->genpd->power_on = cpu_pd_power_on;
+	pd->genpd->flags |= GENPD_FLAG_IRQ_SAFE;
+
+	INIT_LIST_HEAD(&pd->link);
+	list_add(&pd->link, &of_cpu_pd_list);
+	pd->dn = dn;
+
+	/* Register the CPU genpd */
+	pr_debug("adding %s as generic power domain.\n", pd->genpd->name);
+	pm_genpd_init(pd->genpd, &simple_qos_governor, false);
+	of_genpd_add_provider_simple(dn, pd->genpd);
+
+	/* Attach the CPUs to the CPU PM domain */
+	return of_pm_domain_attach_cpus();
+}
+EXPORT_SYMBOL(of_register_cpu_pm_domain);
+
+/**
+ * of_init_cpu_pm_domain() - Initialize a CPU PM domain using the CPU pd
+ * provided
+ * @dn: PM domain provider device node
+ * @ops: CPU PM domain platform specific ops for callback
+ *
+ * This is a single step initialize the CPU PM domain with defaults,
+ * also register the genpd and attach CPUs to the genpd.
+ */
+int of_init_cpu_pm_domain(struct device_node *dn, struct cpu_pm_ops *ops)
+{
+	struct cpu_pm_domain *pd;
+
+	if (!of_device_is_available(dn))
+		return -ENODEV;
+
+	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
+	if (!pd)
+		return -ENOMEM;
+
+	pd->genpd = kzalloc(sizeof(*(pd->genpd)), GFP_KERNEL);
+	if (!pd->genpd) {
+		kfree(pd);
+		return -ENOMEM;
+	}
+
+	if (ops) {
+		pd->plat_ops.power_off = ops->power_off;
+		pd->plat_ops.power_on = ops->power_on;
+	}
+
+	pd->genpd->name = kstrndup(dn->full_name, NAME_MAX, GFP_KERNEL);
+
+	return of_register_cpu_pm_domain(dn, pd);
+}
+EXPORT_SYMBOL(of_init_cpu_pm_domain);
diff --git a/include/linux/cpu-pd.h b/include/linux/cpu-pd.h
new file mode 100644
index 0000000..9ae6f5b
--- /dev/null
+++ b/include/linux/cpu-pd.h
@@ -0,0 +1,35 @@
+/*
+ * include/linux/cpu-pd.h
+ *
+ * Copyright (C) 2015 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __CPU_PD_H__
+#define __CPU_PD_H__
+
+#include <linux/list.h>
+#include <linux/of.h>
+#include <linux/pm_domain.h>
+
+struct cpu_pm_ops {
+	int (*power_off)(struct generic_pm_domain *genpd);
+	int (*power_on)(struct generic_pm_domain *genpd);
+};
+
+struct cpu_pm_domain {
+	struct list_head link;
+	struct generic_pm_domain *genpd;
+	struct device_node *dn;
+	struct cpu_pm_ops plat_ops;
+};
+
+extern int of_register_cpu_pm_domain(struct device_node *dn,
+		struct cpu_pm_domain *pd);
+extern int of_init_cpu_pm_domain(struct device_node *dn,
+		struct cpu_pm_ops *ops);
+
+#endif /* __CPU_PD_H__ */
-- 
2.1.4

[PATCH v2 5/7] ARM: cpuidle: Add runtime PM support for CPU idle

[Lina Iyer]

Notify runtime PM when the CPU is going to be powered off in the idle
state. This allows for runtime PM suspend/resume of the CPU as well as
its  PM domain.

Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/cpuidle/cpuidle-arm.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index 545069d..7c791f9 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -11,13 +11,16 @@
 
 #define pr_fmt(fmt) "CPUidle arm: " fmt
 
+#include <linux/cpu.h>
 #include <linux/cpuidle.h>
 #include <linux/cpumask.h>
 #include <linux/cpu_pm.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/pm_runtime.h>
 #include <linux/slab.h>
+#include <linux/rcupdate.h>
 
 #include <asm/cpuidle.h>
 
@@ -37,6 +40,7 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int idx)
 {
 	int ret;
+	struct device *cpu_dev = get_cpu_device(dev->cpu);
 
 	if (!idx) {
 		cpu_do_idle();
@@ -46,12 +50,19 @@ static int arm_enter_idle_state(struct cpuidle_device *dev,
 	ret = cpu_pm_enter();
 	if (!ret) {
 		/*
+		 * Notify runtime PM as well of this cpu powering down
+		 * TODO: Merge CPU_PM and runtime PM.
+		 */
+		RCU_NONIDLE(pm_runtime_put_sync(cpu_dev));
+
+		/*
 		 * Pass idle state index to cpu_suspend which in turn will
 		 * call the CPU ops suspend protocol with idle index as a
 		 * parameter.
 		 */
 		arm_cpuidle_suspend(idx);
 
+		RCU_NONIDLE(pm_runtime_get_sync(cpu_dev));
 		cpu_pm_exit();
 	}
 
-- 
2.1.4

[PATCH v2 6/7] ARM64: smp: Add runtime PM support for CPU hotplug

[Lina Iyer]

Enable runtime PM for CPU devices. Do a runtime get of the CPU device
when the CPU is hotplugged in and runtime put of the CPU device when the
CPU is hotplugged off. When all the CPUs in a domain are hotplugged off,
the domain may also be powered off and cluster_pm_enter/exit()
notifications are be sent out.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm64/kernel/smp.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index dbdaacd..4076962 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -135,6 +135,7 @@ asmlinkage void secondary_start_kernel(void)
 {
 	struct mm_struct *mm = &init_mm;
 	unsigned int cpu = smp_processor_id();
+	struct device *cpu_dev;
 
 	/*
 	 * All kernel threads share the same mm context; grab a
@@ -185,6 +186,11 @@ asmlinkage void secondary_start_kernel(void)
 	local_irq_enable();
 	local_async_enable();
 
+	/* We are running, enable runtime PM for the CPU. */
+	cpu_dev = get_cpu_device(cpu);
+	if (cpu_dev)
+		pm_runtime_get_sync(cpu_dev);
+
 	/*
 	 * OK, it's off to the idle thread for us
 	 */
@@ -292,6 +298,16 @@ void __cpu_die(unsigned int cpu)
 void cpu_die(void)
 {
 	unsigned int cpu = smp_processor_id();
+	struct device *cpu_dev;
+
+	/*
+	 * We dont need the CPU device anymore.
+	 * Lets do this before IRQs are disabled to allow
+	 * runtime PM to suspend the domain as well.
+	 */
+	cpu_dev = get_cpu_device(cpu);
+	if (cpu_dev)
+		pm_runtime_put_sync(cpu_dev);
 
 	idle_task_exit();
 
-- 
2.1.4

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Lina Iyer]

Enable runtime PM for CPU devices. Do a runtime get of the CPU device
when the CPU is hotplugged in and a runtime put of the CPU device when
the CPU is hotplugged off. When all the CPUs in a domain are hotplugged
off, the domain may also be powered off and cluster_pm_enter/exit()
notifications are be sent out.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/kernel/smp.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 0496b48..077c55f 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -27,6 +27,7 @@
 #include <linux/completion.h>
 #include <linux/cpufreq.h>
 #include <linux/irq_work.h>
+#include <linux/pm_runtime.h>
 
 #include <linux/atomic.h>
 #include <asm/smp.h>
@@ -137,7 +138,6 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
 		pr_err("CPU%u: failed to boot: %d\n", cpu, ret);
 	}
 
-
 	memset(&secondary_data, 0, sizeof(secondary_data));
 	return ret;
 }
@@ -271,6 +271,16 @@ void __cpu_die(unsigned int cpu)
 void __ref cpu_die(void)
 {
 	unsigned int cpu = smp_processor_id();
+	struct device *cpu_dev;
+
+	/*
+	 * We dont need the CPU device anymore.
+	 * Lets do this before IRQs are disabled to allow
+	 * runtime PM to suspend the domain as well.
+	 */
+	cpu_dev = get_cpu_device(cpu);
+	if (cpu_dev)
+		pm_runtime_put_sync(cpu_dev);
 
 	idle_task_exit();
 
@@ -352,6 +362,7 @@ asmlinkage void secondary_start_kernel(void)
 {
 	struct mm_struct *mm = &init_mm;
 	unsigned int cpu;
+	struct device *cpu_dev;
 
 	/*
 	 * The identity mapping is uncached (strongly ordered), so
@@ -401,6 +412,11 @@ asmlinkage void secondary_start_kernel(void)
 	local_irq_enable();
 	local_fiq_enable();
 
+	/* We are running, enable runtime PM for the CPU. */
+	cpu_dev = get_cpu_device(cpu);
+	if (cpu_dev)
+		pm_runtime_get_sync(cpu_dev);
+
 	/*
 	 * OK, it's off to the idle thread for us
 	 */
-- 
2.1.4

[PATCH v2 4/7] PM / Domains: Introduce PM domains for CPUs/clusters

[Stephen Boyd]

On 09/03, Lina Iyer wrote:
> diff --git a/drivers/base/power/cpu-pd.c b/drivers/base/power/cpu-pd.c
> new file mode 100644
> index 0000000..5f30025
> --- /dev/null
> +++ b/drivers/base/power/cpu-pd.c
> @@ -0,0 +1,206 @@
> +/*
> + * CPU Generic PM Domain.
> + *
> + * Copyright (C) 2015 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#define DEBUG
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>

Did you mean export.h here?

> +#include <linux/cpu_pm.h>
> +#include <linux/cpu-pd.h>
> +#include <linux/device.h>
> +#include <linux/of_platform.h>

Is this include used?

> +#include <linux/platform_device.h>

And this one?

> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>

Add list.h for LIST_HEAD usage here?

> +
> +#define NAME_MAX 36
> +
> +/* List of CPU PM domains we care about */
> +static LIST_HEAD(of_cpu_pd_list);
> +
> +static inline
> +struct cpu_pm_domain *to_cpu_pd(struct generic_pm_domain *d)
> +{
> +	struct cpu_pm_domain *pd;
> +
> +	list_for_each_entry(pd, &of_cpu_pd_list, link) {
> +		if (pd->genpd == d)
> +			return pd;
> +	}

Braces are unnecessary.

> +
> +	return NULL;
> +}
> +
> +static int cpu_pd_power_off(struct generic_pm_domain *genpd)
> +{
> +	struct cpu_pm_domain *pd = to_cpu_pd(genpd);
> +
> +	if (pd->plat_ops.power_off)
> +		pd->plat_ops.power_off(genpd);
> +
> +	/*
> +	 * Notify CPU PM domain power down
> +	 * TODO: Call the notificated directly from here.
> +	 */
> +	cpu_cluster_pm_enter();
> +
> +	return 0;
> +}
> +
> +static int cpu_pd_power_on(struct generic_pm_domain *genpd)
> +{
> +	struct cpu_pm_domain *pd = to_cpu_pd(genpd);
> +
> +	if (pd->plat_ops.power_on)
> +		pd->plat_ops.power_on(genpd);
> +
> +	/* Notify CPU PM domain power up */
> +	cpu_cluster_pm_exit();

Perhaps we should swap these two so they're symmetric? 

> +
> +	return 0;
> +}
> +
> +static void run_cpu(void *unused)
> +{
> +	struct device *cpu_dev = get_cpu_device(smp_processor_id());
> +
> +	/* We are running, increment the usage count */
> +	pm_runtime_get_noresume(cpu_dev);
> +}
> +
> +static int of_pm_domain_attach_cpus(void)
> +{
> +	int cpuid, ret;
> +
> +	/* Find any CPU nodes with a phandle to this power domain */
> +	for_each_possible_cpu(cpuid) {
> +		struct device *cpu_dev;
> +
> +		cpu_dev = get_cpu_device(cpuid);
> +		if (!cpu_dev) {
> +			pr_warn("%s: Unable to get device for CPU%d\n",
> +					__func__, cpuid);
> +			return -ENODEV;
> +		}
> +
> +		if (cpu_online(cpuid)) {
> +			pm_runtime_set_active(cpu_dev);
> +			/*
> +			 * Execute the below on that 'cpu' to ensure that the
> +			 * reference counting is correct. Its possible that

s/Its/It's/

> +			 * while this code is executing, the 'cpu' may be
> +			 * powered down, but we may incorrectly increment the
> +			 * usage. By executing the get_cpu on the 'cpu',
> +			 * we can ensure that the 'cpu' and its usage count are
> +			 * matched.
> +			 */
> +			smp_call_function_single(cpuid, run_cpu, NULL, true);
> +		} else {
> +			pm_runtime_set_suspended(cpu_dev);
> +		}
> +		pm_runtime_enable(cpu_dev);
> +
> +		/*
> +		 * We attempt to attach the device to genpd again. We would
> +		 * have failed in our earlier attempt to attach to the domain
> +		 * provider as the CPU device would not have been IRQ safe,
> +		 * while the domain is defined as IRQ safe. IRQ safe domains
> +		 * can only have IRQ safe devices.
> +		 */
> +		ret = genpd_dev_pm_attach(cpu_dev);
> +		if (ret) {
> +			dev_warn(cpu_dev,
> +				"%s: Unable to attach to power-domain: %d\n",
> +				__func__, ret);
> +			pm_runtime_disable(cpu_dev);
> +		} else
> +			dev_dbg(cpu_dev, "Attached to PM domain\n");
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * of_register_cpu_pm_domain() - Register the CPU PM domain with GenPD
> + * framework
> + * @dn: PM domain provider device node
> + * @pd: The CPU PM domain that has been initialized
> + *
> + * This can be used by the platform code to setup the ->genpd of the @pd
> + * The platform can also set up its callbacks in the ->plat_ops.
> + */
> +int of_register_cpu_pm_domain(struct device_node *dn,
> +		struct cpu_pm_domain *pd)
> +{
> +
> +	if (!pd || !pd->genpd)
> +		return -EINVAL;
> +
> +	/*
> +	 * The platform should not set up the genpd callbacks.
> +	 * They should setup the pd->plat_ops instead.
> +	 */
> +	WARN_ON(pd->genpd->power_off);
> +	WARN_ON(pd->genpd->power_on);
> +
> +	pd->genpd->power_off = cpu_pd_power_off;
> +	pd->genpd->power_on = cpu_pd_power_on;
> +	pd->genpd->flags |= GENPD_FLAG_IRQ_SAFE;
> +
> +	INIT_LIST_HEAD(&pd->link);
> +	list_add(&pd->link, &of_cpu_pd_list);
> +	pd->dn = dn;
> +
> +	/* Register the CPU genpd */
> +	pr_debug("adding %s as generic power domain.\n", pd->genpd->name);

Maybe it should say CPU domain?

> +	pm_genpd_init(pd->genpd, &simple_qos_governor, false);
> +	of_genpd_add_provider_simple(dn, pd->genpd);
> +
> +	/* Attach the CPUs to the CPU PM domain */
> +	return of_pm_domain_attach_cpus();

Should we remove the provider on failure?

> +}
> +EXPORT_SYMBOL(of_register_cpu_pm_domain);
> +
> +/**
> + * of_init_cpu_pm_domain() - Initialize a CPU PM domain using the CPU pd
> + * provided
> + * @dn: PM domain provider device node
> + * @ops: CPU PM domain platform specific ops for callback
> + *
> + * This is a single step initialize the CPU PM domain with defaults,
> + * also register the genpd and attach CPUs to the genpd.
> + */
> +int of_init_cpu_pm_domain(struct device_node *dn, struct cpu_pm_ops *ops)
> +{
> +	struct cpu_pm_domain *pd;
> +
> +	if (!of_device_is_available(dn))
> +		return -ENODEV;
> +
> +	pd = kzalloc(sizeof(*pd), GFP_KERNEL);
> +	if (!pd)
> +		return -ENOMEM;
> +
> +	pd->genpd = kzalloc(sizeof(*(pd->genpd)), GFP_KERNEL);
> +	if (!pd->genpd) {
> +		kfree(pd);
> +		return -ENOMEM;
> +	}
> +
> +	if (ops) {
> +		pd->plat_ops.power_off = ops->power_off;
> +		pd->plat_ops.power_on = ops->power_on;
> +	}
> +
> +	pd->genpd->name = kstrndup(dn->full_name, NAME_MAX, GFP_KERNEL);

This could fail. We should check for a failure.

> +
> +	return of_register_cpu_pm_domain(dn, pd);

Did we just leak memory if this fails?

> +}
> +EXPORT_SYMBOL(of_init_cpu_pm_domain);
> diff --git a/include/linux/cpu-pd.h b/include/linux/cpu-pd.h
> new file mode 100644
> index 0000000..9ae6f5b
> --- /dev/null
> +++ b/include/linux/cpu-pd.h
> @@ -0,0 +1,35 @@
> +/*
> + * include/linux/cpu-pd.h
> + *
> + * Copyright (C) 2015 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef __CPU_PD_H__
> +#define __CPU_PD_H__
> +
> +#include <linux/list.h>
> +#include <linux/of.h>
> +#include <linux/pm_domain.h>

These last two includes could be dropped and forward declare
generic_pm_domain and device_node instead.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Stephen Boyd]

On 09/03, Lina Iyer wrote:
> @@ -137,7 +138,6 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
>  		pr_err("CPU%u: failed to boot: %d\n", cpu, ret);
>  	}
>  
> -

Please remove noise.

>  	memset(&secondary_data, 0, sizeof(secondary_data));
>  	return ret;
>  }
> @@ -271,6 +271,16 @@ void __cpu_die(unsigned int cpu)
>  void __ref cpu_die(void)
>  {
>  	unsigned int cpu = smp_processor_id();
> +	struct device *cpu_dev;
> +
> +	/*
> +	 * We dont need the CPU device anymore.

s/dont/don't/

> +	 * Lets do this before IRQs are disabled to allow

s/Lets/Let's/

> +	 * runtime PM to suspend the domain as well.
> +	 */

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 3/7] drivers: cpu: Define CPU devices as IRQ safe

[Stephen Boyd]

On 09/03, Lina Iyer wrote:
> CPUs may be powered off from CPUIdle or hotplug and are called with
> IRQ's disabled. Define CPU devices as IRQ safe, so they may be runtime
> suspended/resumed.
> 
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---

Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Geert Uytterhoeven]

On Thu, Sep 3, 2015 at 9:58 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> Enable runtime PM for CPU devices. Do a runtime get of the CPU device
> when the CPU is hotplugged in and a runtime put of the CPU device when

s/in/on/

> the CPU is hotplugged off. When all the CPUs in a domain are hotplugged
> off, the domain may also be powered off and cluster_pm_enter/exit()
> notifications are be sent out.

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

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Russell King - ARM Linux]

On Thu, Sep 03, 2015 at 01:58:34PM -0600, Lina Iyer wrote:
> @@ -401,6 +412,11 @@ asmlinkage void secondary_start_kernel(void)
>  	local_irq_enable();
>  	local_fiq_enable();
>  
> +	/* We are running, enable runtime PM for the CPU. */
> +	cpu_dev = get_cpu_device(cpu);
> +	if (cpu_dev)
> +		pm_runtime_get_sync(cpu_dev);
> +

Please no.  This is fragile.

pm_runtime_get_sync() can sleep, and this path is used when the system
is fully up and running.  Locks may be held, which cause this to sleep.
However, we're calling it from a context where we can't sleep - which
is the general rule for any part of system initialisation where we
have not entered the idle thread yet (the idle thread is what we run
when sleeping.)

NAK on this.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Russell King - ARM Linux]

On Fri, Sep 04, 2015 at 10:17:35AM +0100, Russell King - ARM Linux wrote:
> On Thu, Sep 03, 2015 at 01:58:34PM -0600, Lina Iyer wrote:
> > @@ -401,6 +412,11 @@ asmlinkage void secondary_start_kernel(void)
> >  	local_irq_enable();
> >  	local_fiq_enable();
> >  
> > +	/* We are running, enable runtime PM for the CPU. */
> > +	cpu_dev = get_cpu_device(cpu);
> > +	if (cpu_dev)
> > +		pm_runtime_get_sync(cpu_dev);
> > +
> 
> Please no.  This is fragile.
> 
> pm_runtime_get_sync() can sleep, and this path is used when the system
> is fully up and running.  Locks may be held, which cause this to sleep.
> However, we're calling it from a context where we can't sleep - which
> is the general rule for any part of system initialisation where we
> have not entered the idle thread yet (the idle thread is what we run
> when sleeping.)
> 
> NAK on this.

There's another issue here.  This is obviously wrong.  If the CPU is
inside the PM domain, then don't you need the PM domain powered up for
the CPU to start executing instructions?

If the CPU can run instructions with the PM domain that it's allegedly
inside powered down, then the CPU is _not_ part of the PM domain, and
this whole thing is total crap.

Sorry, I'm really not impressed by the "design" here, it seems to be
totally screwed.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v2 2/7] PM / Domains: Support IRQ safe PM domains

[Ulf Hansson]

On 3 September 2015 at 21:58, Lina Iyer <lina.iyer@linaro.org> wrote:
> Generic Power Domains currently support turning on/off only in process
> context. This prevents the usage of PM domains for domains that could be
> powered on/off in a context where IRQs are disabled. Many such domains
> exist today and do not get powered off, when the IRQ safe devices in
> that domain are powered off, because of this limitation.
>
> However, not all domains can operate in IRQ safe contexts. Genpd
> therefore, has to support both cases where the domain may or may not
> operate in IRQ safe contexts. Configuring genpd to use an appropriate
> lock for that domain, would allow domains that have IRQ safe devices to
> runtime suspend and resume, in atomic context.
>
> To achieve domain specific locking, set the domain's ->flag to
> GENPD_FLAG_IRQ_SAFE while defining the domain. This indicates that genpd
> should use a spinlock instead of a mutex for locking the domain. Locking
> is abstracted through genpd_lock() and genpd_unlock() functions that use
> the flag to determine the appropriate lock to be used for that domain.
> Domains that have lower latency to suspend and resume and can operate
> with IRQs disabled may now be able to save power, when the component
> devices and sub-domains are idle at runtime.
>
> The restriction this imposes on the domain hierarchy is that sub-domains
> and all devices in the IRQ safe domain's hierarchy also have to be IRQ
> safe, so that we dont try to lock a mutex, while holding a spinlock.

Moreover we can't have IRQ safe domains mixed with non-IRQ safe
subdomains domains, right?

> Non-IRQ safe domains may continue to have devices and sub-domains that
> may or may not be IRQ safe.
>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Krzysztof Koz?owski <k.kozlowski@samsung.com>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>  Documentation/power/devices.txt |  11 ++-
>  drivers/base/power/domain.c     | 212 ++++++++++++++++++++++++++++++++--------
>  include/linux/pm_domain.h       |  11 ++-
>  3 files changed, 189 insertions(+), 45 deletions(-)
>
> diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
> index 8ba6625..bde6141 100644
> --- a/Documentation/power/devices.txt
> +++ b/Documentation/power/devices.txt
> @@ -607,7 +607,16 @@ individually.  Instead, a set of devices sharing a power resource can be put
>  into a low-power state together at the same time by turning off the shared
>  power resource.  Of course, they also need to be put into the full-power state
>  together, by turning the shared power resource on.  A set of devices with this
> -property is often referred to as a power domain.
> +property is often referred to as a power domain. A power domain may also be
> +nested inside another power domain.

This seems like a separate change.

> +
> +Devices, by default, operate in process context and if a device can operate in
> +IRQ safe context, has to be explicitly set as IRQ safe. Power domains by
> +default, operate in process context but could have devices that are IRQ safe.
> +Such power domains cannot be powered on/off during runtime PM. On the other
> +hand, an IRQ safe PM domain that can be powered on/off and suspended or resumed
> +in an atomic context, may contain IRQ safe devices. Such domains may only
> +contain IRQ safe devices or IRQ safe sub-domains.

This is combination of describing the current behaviour and the
changed behaviour after @subject patch. Could you please split this
into separate patches?

>
>  Support for power domains is provided through the pm_domain field of struct
>  device.  This field is a pointer to an object of type struct dev_pm_domain,
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index ef8d19f..9849338 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -53,6 +53,80 @@
>  static LIST_HEAD(gpd_list);
>  static DEFINE_MUTEX(gpd_list_lock);
>
> +static inline int genpd_lock_nosleep(struct generic_pm_domain *genpd,
> +                                       unsigned int subclass)
> +       __acquires(&genpd->slock)
> +{
> +       unsigned long flags;
> +
> +       if (subclass > 0)
> +               spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
> +       else
> +               spin_lock_irqsave(&genpd->slock, flags);
> +
> +       genpd->lock_flags = flags;
> +       return 0;
> +}
> +
> +static inline void genpd_unlock_nosleep(struct generic_pm_domain *genpd)
> +       __releases(&genpd->slock)
> +{
> +       spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
> +}
> +
> +static inline int genpd_lock_irq(struct generic_pm_domain *genpd,
> +                                       unsigned int subclass)
> +       __acquires(&genpd->mlock)
> +{
> +       if (subclass > 0)
> +               mutex_lock_nested(&genpd->mlock, subclass);
> +       else
> +               mutex_lock(&genpd->mlock);
> +       return 0;
> +}
> +
> +static inline int genpd_lock_interruptible_irq(struct generic_pm_domain *genpd)
> +       __acquires(&genpd->mlock)
> +{
> +       return mutex_lock_interruptible(&genpd->mlock);
> +}
> +
> +static inline void genpd_unlock_irq(struct generic_pm_domain *genpd)
> +       __releases(&genpd->mlock)
> +{
> +       mutex_unlock(&genpd->mlock);
> +}
> +
> +static inline int genpd_lock(struct generic_pm_domain *genpd)
> +{
> +       return genpd->irq_safe ? genpd_lock_nosleep(genpd, 0)
> +                       : genpd_lock_irq(genpd, 0);
> +}
> +
> +static inline int genpd_lock_nested(struct generic_pm_domain *genpd)
> +{
> +       return genpd->irq_safe ? genpd_lock_nosleep(genpd, SINGLE_DEPTH_NESTING)
> +                       : genpd_lock_irq(genpd, SINGLE_DEPTH_NESTING);
> +}
> +
> +static inline int genpd_lock_interruptible(struct generic_pm_domain *genpd)
> +{
> +       return genpd->irq_safe ? genpd_lock_nosleep(genpd, 0)
> +                       : genpd_lock_interruptible_irq(genpd);
> +}
> +
> +static inline void genpd_unlock(struct generic_pm_domain *genpd)
> +{
> +       return genpd->irq_safe ? genpd_unlock_nosleep(genpd)
> +                       : genpd_unlock_irq(genpd);
> +}

I think all the above functions that check the genpd->irq_safe flag
can be converted into dealing only with one type of lock, and then
assigned as functions pointers at initialization, depending on the
configuration.

This would mean we don't need to check the genpd->irq_safe each time
we acquire or release a lock.

> +
> +static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev,
> +               struct generic_pm_domain *genpd)
> +{
> +       return dev->power.irq_safe && !genpd->irq_safe;
> +}
> +
>  static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>  {
>         struct generic_pm_domain *genpd = NULL, *gpd;
> @@ -273,9 +347,9 @@ int pm_genpd_poweron(struct generic_pm_domain *genpd)
>  {
>         int ret;
>
> -       mutex_lock(&genpd->lock);
> +       genpd_lock(genpd);
>         ret = __pm_genpd_poweron(genpd);
> -       mutex_unlock(&genpd->lock);
> +       genpd_unlock(genpd);
>         return ret;
>  }
>
> @@ -335,9 +409,9 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
>                 spin_unlock_irq(&dev->power.lock);
>
>                 if (!IS_ERR(genpd)) {
> -                       mutex_lock(&genpd->lock);
> +                       genpd_lock(genpd);
>                         genpd->max_off_time_changed = true;
> -                       mutex_unlock(&genpd->lock);
> +                       genpd_unlock(genpd);
>                 }
>
>                 dev = dev->parent;
> @@ -394,8 +468,17 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
>                 if (stat > PM_QOS_FLAGS_NONE)
>                         return -EBUSY;
>
> -               if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)

I don't remember having above line in genpd. I guess you missed to
include the below patch within in this patchset:
"PM / Domains: Remove dev->driver check for runtime PM".

> +               /*
> +                * We do not want to power off the domain if the device is
> +                * not suspended or an IRQ safe device is part of this
> +                * non-IRQ safe domain.
> +                */
> +               if (!pm_runtime_suspended(pdd->dev) ||
> +                       irq_safe_dev_in_no_sleep_domain(pdd->dev, genpd))
>                         not_suspended++;
> +               WARN_ONCE(irq_safe_dev_in_no_sleep_domain(pdd->dev, genpd),
> +                               "PM domain %s will not be powered off\n",
> +                               genpd->name);
>         }
>
>         if (not_suspended > genpd->in_progress)
> @@ -460,9 +543,9 @@ static void genpd_power_off_work_fn(struct work_struct *work)
>
>         genpd = container_of(work, struct generic_pm_domain, power_off_work);
>
> -       mutex_lock(&genpd->lock);
> +       genpd_lock(genpd);
>         pm_genpd_poweroff(genpd);
> -       mutex_unlock(&genpd->lock);
> +       genpd_unlock(genpd);
>  }
>
>  /**
> @@ -500,17 +583,21 @@ static int pm_genpd_runtime_suspend(struct device *dev)
>         }
>
>         /*
> -        * If power.irq_safe is set, this routine will be run with interrupts
> -        * off, so it can't use mutexes.
> +        * If power.irq_safe is set, this routine may be run with
> +        * IRQ disabled, so suspend only if the power domain is
> +        * irq_safe.

Maybe elaborate on that the locking mechanism differ, which is why we
can't proceed in some cases...

>          */
> -       if (dev->power.irq_safe)
> +       WARN_ONCE(irq_safe_dev_in_no_sleep_domain(dev, genpd),
> +                       "genpd %s will not be powered off\n", genpd->name);

Perhaps, unless the string becomes too long, some information about *why*.

> +       if (irq_safe_dev_in_no_sleep_domain(dev, genpd))
>                 return 0;
>
> -       mutex_lock(&genpd->lock);
> +       genpd_lock(genpd);
> +
>         genpd->in_progress++;
>         pm_genpd_poweroff(genpd);
>         genpd->in_progress--;
> -       mutex_unlock(&genpd->lock);
> +       genpd_unlock(genpd);
>
>         return 0;
>  }
> @@ -535,15 +622,18 @@ static int pm_genpd_runtime_resume(struct device *dev)
>         if (IS_ERR(genpd))
>                 return -EINVAL;
>
> -       /* If power.irq_safe, the PM domain is never powered off. */
> -       if (dev->power.irq_safe) {
> +       /*
> +        * As we dont power off a non IRQ safe domain, which holds
> +        * an IRQ safe device, we dont need to restore power to it.
> +        */
> +       if (dev->power.irq_safe && !genpd->irq_safe) {
>                 timed = false;
>                 goto out;
>         }
>
> -       mutex_lock(&genpd->lock);
> +       genpd_lock(genpd);
>         ret = __pm_genpd_poweron(genpd);
> -       mutex_unlock(&genpd->lock);
> +       genpd_unlock(genpd);
>
>         if (ret)
>                 return ret;
> @@ -743,14 +833,14 @@ static int pm_genpd_prepare(struct device *dev)
>         if (resume_needed(dev, genpd))
>                 pm_runtime_resume(dev);
>
> -       mutex_lock(&genpd->lock);
> +       genpd_lock(genpd);
>
>         if (genpd->prepared_count++ == 0) {
>                 genpd->suspended_count = 0;
>                 genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
>         }
>
> -       mutex_unlock(&genpd->lock);
> +       genpd_unlock(genpd);
>
>         if (genpd->suspend_power_off) {
>                 pm_runtime_put_noidle(dev);
> @@ -768,12 +858,12 @@ static int pm_genpd_prepare(struct device *dev)
>
>         ret = pm_generic_prepare(dev);
>         if (ret) {
> -               mutex_lock(&genpd->lock);
> +               genpd_lock(genpd);
>
>                 if (--genpd->prepared_count == 0)
>                         genpd->suspend_power_off = false;
>
> -               mutex_unlock(&genpd->lock);
> +               genpd_unlock(genpd);
>                 pm_runtime_enable(dev);
>         }
>
> @@ -1130,13 +1220,13 @@ static void pm_genpd_complete(struct device *dev)
>         if (IS_ERR(genpd))
>                 return;
>
> -       mutex_lock(&genpd->lock);
> +       genpd_lock(genpd);
>
>         run_complete = !genpd->suspend_power_off;
>         if (--genpd->prepared_count == 0)
>                 genpd->suspend_power_off = false;
>
> -       mutex_unlock(&genpd->lock);
> +       genpd_unlock(genpd);
>
>         if (run_complete) {
>                 pm_generic_complete(dev);
> @@ -1280,11 +1370,18 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>         if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
>                 return -EINVAL;
>
> +       if (genpd->irq_safe && !dev->power.irq_safe) {
> +               dev_err(dev,
> +                       "PM Domain %s is IRQ safe; device has to IRQ safe.\n",

/s/; device has to IRQ safe/, but device isn't.

> +                       genpd->name);
> +               return -EINVAL;
> +       }
> +
>         gpd_data = genpd_alloc_dev_data(dev, genpd, td);
>         if (IS_ERR(gpd_data))
>                 return PTR_ERR(gpd_data);
>
> -       mutex_lock(&genpd->lock);
> +       genpd_lock(genpd);
>
>         if (genpd->prepared_count > 0) {
>                 ret = -EAGAIN;
> @@ -1301,7 +1398,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>         list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
>
>   out:
> -       mutex_unlock(&genpd->lock);
> +       genpd_unlock(genpd);
>
>         if (ret)
>                 genpd_free_dev_data(dev, gpd_data);
> @@ -1345,7 +1442,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>         gpd_data = to_gpd_data(pdd);
>         dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
>
> -       mutex_lock(&genpd->lock);
> +       genpd_lock(genpd);
>
>         if (genpd->prepared_count > 0) {
>                 ret = -EAGAIN;
> @@ -1360,14 +1457,14 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>
>         list_del_init(&pdd->list_node);
>
> -       mutex_unlock(&genpd->lock);
> +       genpd_unlock(genpd);
>
>         genpd_free_dev_data(dev, gpd_data);
>
>         return 0;
>
>   out:
> -       mutex_unlock(&genpd->lock);
> +       genpd_unlock(genpd);
>         dev_pm_qos_add_notifier(dev, &gpd_data->nb);
>
>         return ret;
> @@ -1388,12 +1485,23 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>             || genpd == subdomain)
>                 return -EINVAL;
>
> +       /*
> +        * If the domain can be powered on/off in an IRQ safe
> +        * context, ensure that the subdomain can also be
> +        * powered on/off in that context.
> +        */
> +       if (genpd->irq_safe && !subdomain->irq_safe) {

Is this really enough? What if the subdomain is IRQ safe but its
parent isn't, that's not allowed either, right?

> +               WARN("Sub-domain (%s) in an IRQ-safe domain (%s) has to be IRQ safe\n",
> +                               subdomain->name, genpd->name);
> +               return -EINVAL;
> +       }
> +
>         link = kzalloc(sizeof(*link), GFP_KERNEL);
>         if (!link)
>                 return -ENOMEM;
>
> -       mutex_lock(&genpd->lock);
> -       mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
> +       genpd_lock(genpd);
> +       genpd_lock_nested(subdomain);
>
>         if (genpd->status == GPD_STATE_POWER_OFF
>             &&  subdomain->status != GPD_STATE_POWER_OFF) {
> @@ -1416,8 +1524,8 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>                 genpd_sd_counter_inc(genpd);
>
>   out:
> -       mutex_unlock(&subdomain->lock);
> -       mutex_unlock(&genpd->lock);
> +       genpd_unlock(subdomain);
> +       genpd_unlock(genpd);
>         if (ret)
>                 kfree(link);
>         return ret;
> @@ -1466,13 +1574,13 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>         if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain))
>                 return -EINVAL;
>
> -       mutex_lock(&genpd->lock);
> +       genpd_lock(genpd);
>
>         list_for_each_entry(link, &genpd->master_links, master_node) {
>                 if (link->slave != subdomain)
>                         continue;
>
> -               mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
> +               genpd_lock_nested(subdomain);
>
>                 list_del(&link->master_node);
>                 list_del(&link->slave_node);
> @@ -1480,13 +1588,13 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>                 if (subdomain->status != GPD_STATE_POWER_OFF)
>                         genpd_sd_counter_dec(genpd);
>
> -               mutex_unlock(&subdomain->lock);
> +               genpd_unlock(subdomain);
>
>                 ret = 0;
>                 break;
>         }
>
> -       mutex_unlock(&genpd->lock);
> +       genpd_unlock(genpd);
>
>         return ret;
>  }
> @@ -1510,11 +1618,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
>         if (IS_ERR_OR_NULL(genpd) || state < 0)
>                 return -EINVAL;
>
> +       if (genpd->irq_safe) {
> +               WARN("Domain %s is IRQ safe, cannot attach to cpuidle\n",
> +                               genpd->name);
> +               return -EINVAL;
> +       }
> +
>         cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
>         if (!cpuidle_data)
>                 return -ENOMEM;
>
> -       mutex_lock(&genpd->lock);
> +       genpd_lock(genpd);
>
>         if (genpd->cpuidle_data) {
>                 ret = -EEXIST;
> @@ -1541,7 +1655,7 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
>         genpd_recalc_cpu_exit_latency(genpd);
>
>   out:
> -       mutex_unlock(&genpd->lock);
> +       genpd_unlock(genpd);
>         return ret;
>
>   err:
> @@ -1578,7 +1692,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
>         if (IS_ERR_OR_NULL(genpd))
>                 return -EINVAL;
>
> -       mutex_lock(&genpd->lock);
> +       genpd_lock(genpd);
>
>         cpuidle_data = genpd->cpuidle_data;
>         if (!cpuidle_data) {
> @@ -1596,7 +1710,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
>         kfree(cpuidle_data);
>
>   out:
> -       mutex_unlock(&genpd->lock);
> +       genpd_unlock(genpd);
>         return ret;
>  }
>
> @@ -1657,6 +1771,17 @@ static int pm_genpd_default_restore_state(struct device *dev)
>         return cb ? cb(dev) : 0;
>  }
>
> +static void genpd_lock_init(struct generic_pm_domain *genpd)
> +{
> +       if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
> +               spin_lock_init(&genpd->slock);
> +               genpd->irq_safe = true;
> +       } else {
> +               mutex_init(&genpd->mlock);
> +               genpd->irq_safe = false;
> +       }

As I stated earlier around having function pointers, this would be the
place where to assign them I guess.

> +}
> +
>  /**
>   * pm_genpd_init - Initialize a generic I/O PM domain object.
>   * @genpd: PM domain object to initialize.
> @@ -1672,7 +1797,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>         INIT_LIST_HEAD(&genpd->master_links);
>         INIT_LIST_HEAD(&genpd->slave_links);
>         INIT_LIST_HEAD(&genpd->dev_list);
> -       mutex_init(&genpd->lock);
> +       genpd_lock_init(genpd);
>         genpd->gov = gov;
>         INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
>         genpd->in_progress = 0;
> @@ -2067,7 +2192,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
>         struct gpd_link *link;
>         int ret;
>
> -       ret = mutex_lock_interruptible(&genpd->lock);
> +       ret = genpd_lock_interruptible(genpd);
>         if (ret)
>                 return -ERESTARTSYS;
>
> @@ -2087,7 +2212,8 @@ static int pm_genpd_summary_one(struct seq_file *s,
>         }
>
>         list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
> -               kobj_path = kobject_get_path(&pm_data->dev->kobj, GFP_KERNEL);
> +               kobj_path = kobject_get_path(&pm_data->dev->kobj,
> +                               genpd->irq_safe ? GFP_ATOMIC : GFP_KERNEL);
>                 if (kobj_path == NULL)
>                         continue;
>
> @@ -2098,7 +2224,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
>
>         seq_puts(s, "\n");
>  exit:
> -       mutex_unlock(&genpd->lock);
> +       genpd_unlock(genpd);
>
>         return 0;
>  }
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index b2725e6..dc7cb53 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -16,9 +16,11 @@
>  #include <linux/of.h>
>  #include <linux/notifier.h>
>  #include <linux/cpuidle.h>
> +#include <linux/spinlock.h>
>
>  /* Defines used for the flags field in the struct generic_pm_domain */
>  #define GENPD_FLAG_PM_CLK      (1U << 0) /* PM domain uses PM clk */
> +#define GENPD_FLAG_IRQ_SAFE    (1U << 1) /* PM domain operates in atomic */
>
>  enum gpd_status {
>         GPD_STATE_ACTIVE = 0,   /* PM domain is active */
> @@ -49,7 +51,6 @@ struct generic_pm_domain {
>         struct list_head master_links;  /* Links with PM domain as a master */
>         struct list_head slave_links;   /* Links with PM domain as a slave */
>         struct list_head dev_list;      /* List of devices */
> -       struct mutex lock;
>         struct dev_power_governor *gov;
>         struct work_struct power_off_work;
>         const char *name;
> @@ -74,6 +75,14 @@ struct generic_pm_domain {
>         void (*detach_dev)(struct generic_pm_domain *domain,
>                            struct device *dev);
>         unsigned int flags;             /* Bit field of configs for genpd */
> +       bool irq_safe;

If you convert to functions pointers, perhaps you can remove the
irq_safe bool here, and instead only use the flags field.

> +       union {
> +               struct mutex mlock;
> +               struct {
> +                       spinlock_t slock;
> +                       unsigned long lock_flags;
> +               };
> +       };
>  };
>
>  static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
> --
> 2.1.4
>

Kind regards
Uffe

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Lina Iyer]

On Fri, Sep 04 2015 at 03:27 -0600, Russell King - ARM Linux wrote:
>On Fri, Sep 04, 2015 at 10:17:35AM +0100, Russell King - ARM Linux wrote:
>> On Thu, Sep 03, 2015 at 01:58:34PM -0600, Lina Iyer wrote:
>> > @@ -401,6 +412,11 @@ asmlinkage void secondary_start_kernel(void)
>> >  	local_irq_enable();
>> >  	local_fiq_enable();
>> >
>> > +	/* We are running, enable runtime PM for the CPU. */
>> > +	cpu_dev = get_cpu_device(cpu);
>> > +	if (cpu_dev)
>> > +		pm_runtime_get_sync(cpu_dev);
>> > +
>>
>> Please no.  This is fragile.
>>
>> pm_runtime_get_sync() can sleep, and this path is used when the system
>> is fully up and running.  Locks may be held, which cause this to sleep.
>> However, we're calling it from a context where we can't sleep - which
>> is the general rule for any part of system initialisation where we
>> have not entered the idle thread yet (the idle thread is what we run
>> when sleeping.)
>>
More explanation below.

Another patch (3/7) in this series defines CPU devices as IRQ safe and
therefore the dev->power.lock would be spinlock'd before calling the
rest of the PM runtime code and the domain. The CPU PM domain would also
be an IRQ safe domain (patch 2/7 makes the genpd framework usable in IRQ
safe context).

>> NAK on this.
>
>There's another issue here.  This is obviously wrong.  If the CPU is
>inside the PM domain, then don't you need the PM domain powered up for
>the CPU to start executing instructions?
>
This would not be the case for CPU PM domains. The CPU PM domains AFAIK,
are triggered by the same wake up interrupt that brings the CPU out of
its power downs state. The domain hardware has to be awake before the
CPU executes its first instruction. Thefore the hw power up is a logical
OR of the wakeup signals to any of the cores.

Similarly, the domain hardware would be the last to power down after all
the CPUs.  Generally tied to the execution of WFI command by the ARM CPU
core. The domain power down signal is a logical AND of all the WFI
signals of all the cores in the domain.

>If the CPU can run instructions with the PM domain that it's allegedly
>inside powered down, then the CPU is _not_ part of the PM domain, and
>this whole thing is total crap.
>
CPU PM domains have to be synchronized in hardware and all these can
only happen when the SoC provides a domain that can power on before any
of the CPU powers up and power down only after the all CPUs have powered
down.

What s/w generally can do in the CPU domain power on/off callbacks from
genpd is to configure which state the domain should be in, when all the
CPUs have powered down. Many SoCs, have PM domains that could be in
retention in anticipation of an early wakeup or could be completely
cache flushed and powered down, when suspending or sleeping for a longer
duration. The power off callbacks for the PM domain does the
configuration and it doesnt take effect until all the CPUs power down.

While powering on the domain in the genpd power on callback, all we do
is to ensure that the domain is reconfigured to remain active (ignore
all WFI instruction from the CPU, which are not tied to CPU power down.
Dont want the domain powering off when all CPUs in the domain are just
clock gated.) until the domain is configured to its idle state by the
last CPU in Linux.

>Sorry, I'm really not impressed by the "design" here, it seems to be
>totally screwed.
>

Thanks,
Lina

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Lina Iyer]

On Thu, Sep 03 2015 at 21:59 -0600, Stephen Boyd wrote:
>On 09/03, Lina Iyer wrote:
>> @@ -137,7 +138,6 @@ int __cpu_up(unsigned int cpu, struct task_struct *idle)
>>  		pr_err("CPU%u: failed to boot: %d\n", cpu, ret);
>>  	}
>>
>> -
>
>Please remove noise.
>
Sorry, I forgot to address it earlier. Thought I did.
Apologize.

>>  	memset(&secondary_data, 0, sizeof(secondary_data));
>>  	return ret;
>>  }
>> @@ -271,6 +271,16 @@ void __cpu_die(unsigned int cpu)
>>  void __ref cpu_die(void)
>>  {
>>  	unsigned int cpu = smp_processor_id();
>> +	struct device *cpu_dev;
>> +
>> +	/*
>> +	 * We dont need the CPU device anymore.
>
>s/dont/don't/
>
>> +	 * Lets do this before IRQs are disabled to allow
>
>s/Lets/Let's/
>
>> +	 * runtime PM to suspend the domain as well.
>> +	 */
>
>-- 
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>a Linux Foundation Collaborative Project

[PATCH v2 2/7] PM / Domains: Support IRQ safe PM domains

[Lina Iyer]

On Fri, Sep 04 2015 at 04:02 -0600, Ulf Hansson wrote:
>On 3 September 2015 at 21:58, Lina Iyer <lina.iyer@linaro.org> wrote:
>> Generic Power Domains currently support turning on/off only in process
>> context. This prevents the usage of PM domains for domains that could be
>> powered on/off in a context where IRQs are disabled. Many such domains
>> exist today and do not get powered off, when the IRQ safe devices in
>> that domain are powered off, because of this limitation.
>>
>> However, not all domains can operate in IRQ safe contexts. Genpd
>> therefore, has to support both cases where the domain may or may not
>> operate in IRQ safe contexts. Configuring genpd to use an appropriate
>> lock for that domain, would allow domains that have IRQ safe devices to
>> runtime suspend and resume, in atomic context.
>>
>> To achieve domain specific locking, set the domain's ->flag to
>> GENPD_FLAG_IRQ_SAFE while defining the domain. This indicates that genpd
>> should use a spinlock instead of a mutex for locking the domain. Locking
>> is abstracted through genpd_lock() and genpd_unlock() functions that use
>> the flag to determine the appropriate lock to be used for that domain.
>> Domains that have lower latency to suspend and resume and can operate
>> with IRQs disabled may now be able to save power, when the component
>> devices and sub-domains are idle at runtime.
>>
>> The restriction this imposes on the domain hierarchy is that sub-domains
>> and all devices in the IRQ safe domain's hierarchy also have to be IRQ
>> safe, so that we dont try to lock a mutex, while holding a spinlock.
>
>Moreover we can't have IRQ safe domains mixed with non-IRQ safe
>subdomains domains, right?
>
>> Non-IRQ safe domains may continue to have devices and sub-domains that
>> may or may not be IRQ safe.
>>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Cc: Kevin Hilman <khilman@linaro.org>
>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
>> Cc: Krzysztof Koz?owski <k.kozlowski@samsung.com>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>>  Documentation/power/devices.txt |  11 ++-
>>  drivers/base/power/domain.c     | 212 ++++++++++++++++++++++++++++++++--------
>>  include/linux/pm_domain.h       |  11 ++-
>>  3 files changed, 189 insertions(+), 45 deletions(-)
>>
>> diff --git a/Documentation/power/devices.txt b/Documentation/power/devices.txt
>> index 8ba6625..bde6141 100644
>> --- a/Documentation/power/devices.txt
>> +++ b/Documentation/power/devices.txt
>> @@ -607,7 +607,16 @@ individually.  Instead, a set of devices sharing a power resource can be put
>>  into a low-power state together at the same time by turning off the shared
>>  power resource.  Of course, they also need to be put into the full-power state
>>  together, by turning the shared power resource on.  A set of devices with this
>> -property is often referred to as a power domain.
>> +property is often referred to as a power domain. A power domain may also be
>> +nested inside another power domain.
>
>This seems like a separate change.
>
>> +
>> +Devices, by default, operate in process context and if a device can operate in
>> +IRQ safe context, has to be explicitly set as IRQ safe. Power domains by
>> +default, operate in process context but could have devices that are IRQ safe.
>> +Such power domains cannot be powered on/off during runtime PM. On the other
>> +hand, an IRQ safe PM domain that can be powered on/off and suspended or resumed
>> +in an atomic context, may contain IRQ safe devices. Such domains may only
>> +contain IRQ safe devices or IRQ safe sub-domains.
>
>This is combination of describing the current behaviour and the
>changed behaviour after @subject patch. Could you please split this
>into separate patches?
>
Sure.

>>
>>  Support for power domains is provided through the pm_domain field of struct
>>  device.  This field is a pointer to an object of type struct dev_pm_domain,
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index ef8d19f..9849338 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -53,6 +53,80 @@
>>  static LIST_HEAD(gpd_list);
>>  static DEFINE_MUTEX(gpd_list_lock);
>>
>> +static inline int genpd_lock_nosleep(struct generic_pm_domain *genpd,
>> +                                       unsigned int subclass)
>> +       __acquires(&genpd->slock)
>> +{
>> +       unsigned long flags;
>> +
>> +       if (subclass > 0)
>> +               spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
>> +       else
>> +               spin_lock_irqsave(&genpd->slock, flags);
>> +
>> +       genpd->lock_flags = flags;
>> +       return 0;
>> +}
>> +
>> +static inline void genpd_unlock_nosleep(struct generic_pm_domain *genpd)
>> +       __releases(&genpd->slock)
>> +{
>> +       spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
>> +}
>> +
>> +static inline int genpd_lock_irq(struct generic_pm_domain *genpd,
>> +                                       unsigned int subclass)
>> +       __acquires(&genpd->mlock)
>> +{
>> +       if (subclass > 0)
>> +               mutex_lock_nested(&genpd->mlock, subclass);
>> +       else
>> +               mutex_lock(&genpd->mlock);
>> +       return 0;
>> +}
>> +
>> +static inline int genpd_lock_interruptible_irq(struct generic_pm_domain *genpd)
>> +       __acquires(&genpd->mlock)
>> +{
>> +       return mutex_lock_interruptible(&genpd->mlock);
>> +}
>> +
>> +static inline void genpd_unlock_irq(struct generic_pm_domain *genpd)
>> +       __releases(&genpd->mlock)
>> +{
>> +       mutex_unlock(&genpd->mlock);
>> +}
>> +
>> +static inline int genpd_lock(struct generic_pm_domain *genpd)
>> +{
>> +       return genpd->irq_safe ? genpd_lock_nosleep(genpd, 0)
>> +                       : genpd_lock_irq(genpd, 0);
>> +}
>> +
>> +static inline int genpd_lock_nested(struct generic_pm_domain *genpd)
>> +{
>> +       return genpd->irq_safe ? genpd_lock_nosleep(genpd, SINGLE_DEPTH_NESTING)
>> +                       : genpd_lock_irq(genpd, SINGLE_DEPTH_NESTING);
>> +}
>> +
>> +static inline int genpd_lock_interruptible(struct generic_pm_domain *genpd)
>> +{
>> +       return genpd->irq_safe ? genpd_lock_nosleep(genpd, 0)
>> +                       : genpd_lock_interruptible_irq(genpd);
>> +}
>> +
>> +static inline void genpd_unlock(struct generic_pm_domain *genpd)
>> +{
>> +       return genpd->irq_safe ? genpd_unlock_nosleep(genpd)
>> +                       : genpd_unlock_irq(genpd);
>> +}
>
>I think all the above functions that check the genpd->irq_safe flag
>can be converted into dealing only with one type of lock, and then
>assigned as functions pointers at initialization, depending on the
>configuration.
>
>This would mean we don't need to check the genpd->irq_safe each time
>we acquire or release a lock.
>
I thought about it. Regmap does that. But that would mean, we carry
around, three function pointers - lock, lock_nested, unlock on each
genpd object.

>> +
>> +static inline bool irq_safe_dev_in_no_sleep_domain(struct device *dev,
>> +               struct generic_pm_domain *genpd)
>> +{
>> +       return dev->power.irq_safe && !genpd->irq_safe;
>> +}
>> +
>>  static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
>>  {
>>         struct generic_pm_domain *genpd = NULL, *gpd;
>> @@ -273,9 +347,9 @@ int pm_genpd_poweron(struct generic_pm_domain *genpd)
>>  {
>>         int ret;
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>         ret = __pm_genpd_poweron(genpd);
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>         return ret;
>>  }
>>
>> @@ -335,9 +409,9 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
>>                 spin_unlock_irq(&dev->power.lock);
>>
>>                 if (!IS_ERR(genpd)) {
>> -                       mutex_lock(&genpd->lock);
>> +                       genpd_lock(genpd);
>>                         genpd->max_off_time_changed = true;
>> -                       mutex_unlock(&genpd->lock);
>> +                       genpd_unlock(genpd);
>>                 }
>>
>>                 dev = dev->parent;
>> @@ -394,8 +468,17 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
>>                 if (stat > PM_QOS_FLAGS_NONE)
>>                         return -EBUSY;
>>
>> -               if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
>
>I don't remember having above line in genpd. I guess you missed to
>include the below patch within in this patchset:
>"PM / Domains: Remove dev->driver check for runtime PM".
>
I seemed to me that the change deserves its own patch.

>> +               /*
>> +                * We do not want to power off the domain if the device is
>> +                * not suspended or an IRQ safe device is part of this
>> +                * non-IRQ safe domain.
>> +                */
>> +               if (!pm_runtime_suspended(pdd->dev) ||
>> +                       irq_safe_dev_in_no_sleep_domain(pdd->dev, genpd))
>>                         not_suspended++;
>> +               WARN_ONCE(irq_safe_dev_in_no_sleep_domain(pdd->dev, genpd),
>> +                               "PM domain %s will not be powered off\n",
>> +                               genpd->name);
>>         }
>>
>>         if (not_suspended > genpd->in_progress)
>> @@ -460,9 +543,9 @@ static void genpd_power_off_work_fn(struct work_struct *work)
>>
>>         genpd = container_of(work, struct generic_pm_domain, power_off_work);
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>         pm_genpd_poweroff(genpd);
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>  }
>>
>>  /**
>> @@ -500,17 +583,21 @@ static int pm_genpd_runtime_suspend(struct device *dev)
>>         }
>>
>>         /*
>> -        * If power.irq_safe is set, this routine will be run with interrupts
>> -        * off, so it can't use mutexes.
>> +        * If power.irq_safe is set, this routine may be run with
>> +        * IRQ disabled, so suspend only if the power domain is
>> +        * irq_safe.
>
>Maybe elaborate on that the locking mechanism differ, which is why we
>can't proceed in some cases...
>
>>          */
>> -       if (dev->power.irq_safe)
>> +       WARN_ONCE(irq_safe_dev_in_no_sleep_domain(dev, genpd),
>> +                       "genpd %s will not be powered off\n", genpd->name);
>
>Perhaps, unless the string becomes too long, some information about *why*.
>
Ok

>> +       if (irq_safe_dev_in_no_sleep_domain(dev, genpd))
>>                 return 0;
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>> +
>>         genpd->in_progress++;
>>         pm_genpd_poweroff(genpd);
>>         genpd->in_progress--;
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         return 0;
>>  }
>> @@ -535,15 +622,18 @@ static int pm_genpd_runtime_resume(struct device *dev)
>>         if (IS_ERR(genpd))
>>                 return -EINVAL;
>>
>> -       /* If power.irq_safe, the PM domain is never powered off. */
>> -       if (dev->power.irq_safe) {
>> +       /*
>> +        * As we dont power off a non IRQ safe domain, which holds
>> +        * an IRQ safe device, we dont need to restore power to it.
>> +        */
>> +       if (dev->power.irq_safe && !genpd->irq_safe) {
>>                 timed = false;
>>                 goto out;
>>         }
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>         ret = __pm_genpd_poweron(genpd);
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         if (ret)
>>                 return ret;
>> @@ -743,14 +833,14 @@ static int pm_genpd_prepare(struct device *dev)
>>         if (resume_needed(dev, genpd))
>>                 pm_runtime_resume(dev);
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>
>>         if (genpd->prepared_count++ == 0) {
>>                 genpd->suspended_count = 0;
>>                 genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
>>         }
>>
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         if (genpd->suspend_power_off) {
>>                 pm_runtime_put_noidle(dev);
>> @@ -768,12 +858,12 @@ static int pm_genpd_prepare(struct device *dev)
>>
>>         ret = pm_generic_prepare(dev);
>>         if (ret) {
>> -               mutex_lock(&genpd->lock);
>> +               genpd_lock(genpd);
>>
>>                 if (--genpd->prepared_count == 0)
>>                         genpd->suspend_power_off = false;
>>
>> -               mutex_unlock(&genpd->lock);
>> +               genpd_unlock(genpd);
>>                 pm_runtime_enable(dev);
>>         }
>>
>> @@ -1130,13 +1220,13 @@ static void pm_genpd_complete(struct device *dev)
>>         if (IS_ERR(genpd))
>>                 return;
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>
>>         run_complete = !genpd->suspend_power_off;
>>         if (--genpd->prepared_count == 0)
>>                 genpd->suspend_power_off = false;
>>
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         if (run_complete) {
>>                 pm_generic_complete(dev);
>> @@ -1280,11 +1370,18 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>>         if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
>>                 return -EINVAL;
>>
>> +       if (genpd->irq_safe && !dev->power.irq_safe) {
>> +               dev_err(dev,
>> +                       "PM Domain %s is IRQ safe; device has to IRQ safe.\n",
>
>/s/; device has to IRQ safe/, but device isn't.
>
>> +                       genpd->name);
>> +               return -EINVAL;
>> +       }
>> +
>>         gpd_data = genpd_alloc_dev_data(dev, genpd, td);
>>         if (IS_ERR(gpd_data))
>>                 return PTR_ERR(gpd_data);
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>
>>         if (genpd->prepared_count > 0) {
>>                 ret = -EAGAIN;
>> @@ -1301,7 +1398,7 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>>         list_add_tail(&gpd_data->base.list_node, &genpd->dev_list);
>>
>>   out:
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         if (ret)
>>                 genpd_free_dev_data(dev, gpd_data);
>> @@ -1345,7 +1442,7 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>>         gpd_data = to_gpd_data(pdd);
>>         dev_pm_qos_remove_notifier(dev, &gpd_data->nb);
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>
>>         if (genpd->prepared_count > 0) {
>>                 ret = -EAGAIN;
>> @@ -1360,14 +1457,14 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
>>
>>         list_del_init(&pdd->list_node);
>>
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         genpd_free_dev_data(dev, gpd_data);
>>
>>         return 0;
>>
>>   out:
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>         dev_pm_qos_add_notifier(dev, &gpd_data->nb);
>>
>>         return ret;
>> @@ -1388,12 +1485,23 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>>             || genpd == subdomain)
>>                 return -EINVAL;
>>
>> +       /*
>> +        * If the domain can be powered on/off in an IRQ safe
>> +        * context, ensure that the subdomain can also be
>> +        * powered on/off in that context.
>> +        */
>> +       if (genpd->irq_safe && !subdomain->irq_safe) {
>
>Is this really enough? What if the subdomain is IRQ safe but its
>parent isn't, that's not allowed either, right?
>
That should be allowable. We would just not power off the parent and
expect it to be powered on always, when the subdomain powers on/off.

>> +               WARN("Sub-domain (%s) in an IRQ-safe domain (%s) has to be IRQ safe\n",
>> +                               subdomain->name, genpd->name);
>> +               return -EINVAL;
>> +       }
>> +
>>         link = kzalloc(sizeof(*link), GFP_KERNEL);
>>         if (!link)
>>                 return -ENOMEM;
>>
>> -       mutex_lock(&genpd->lock);
>> -       mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
>> +       genpd_lock(genpd);
>> +       genpd_lock_nested(subdomain);
>>
>>         if (genpd->status == GPD_STATE_POWER_OFF
>>             &&  subdomain->status != GPD_STATE_POWER_OFF) {
>> @@ -1416,8 +1524,8 @@ int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
>>                 genpd_sd_counter_inc(genpd);
>>
>>   out:
>> -       mutex_unlock(&subdomain->lock);
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(subdomain);
>> +       genpd_unlock(genpd);
>>         if (ret)
>>                 kfree(link);
>>         return ret;
>> @@ -1466,13 +1574,13 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>>         if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(subdomain))
>>                 return -EINVAL;
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>
>>         list_for_each_entry(link, &genpd->master_links, master_node) {
>>                 if (link->slave != subdomain)
>>                         continue;
>>
>> -               mutex_lock_nested(&subdomain->lock, SINGLE_DEPTH_NESTING);
>> +               genpd_lock_nested(subdomain);
>>
>>                 list_del(&link->master_node);
>>                 list_del(&link->slave_node);
>> @@ -1480,13 +1588,13 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>>                 if (subdomain->status != GPD_STATE_POWER_OFF)
>>                         genpd_sd_counter_dec(genpd);
>>
>> -               mutex_unlock(&subdomain->lock);
>> +               genpd_unlock(subdomain);
>>
>>                 ret = 0;
>>                 break;
>>         }
>>
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         return ret;
>>  }
>> @@ -1510,11 +1618,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
>>         if (IS_ERR_OR_NULL(genpd) || state < 0)
>>                 return -EINVAL;
>>
>> +       if (genpd->irq_safe) {
>> +               WARN("Domain %s is IRQ safe, cannot attach to cpuidle\n",
>> +                               genpd->name);
>> +               return -EINVAL;
>> +       }
>> +
>>         cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
>>         if (!cpuidle_data)
>>                 return -ENOMEM;
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>
>>         if (genpd->cpuidle_data) {
>>                 ret = -EEXIST;
>> @@ -1541,7 +1655,7 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
>>         genpd_recalc_cpu_exit_latency(genpd);
>>
>>   out:
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>         return ret;
>>
>>   err:
>> @@ -1578,7 +1692,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
>>         if (IS_ERR_OR_NULL(genpd))
>>                 return -EINVAL;
>>
>> -       mutex_lock(&genpd->lock);
>> +       genpd_lock(genpd);
>>
>>         cpuidle_data = genpd->cpuidle_data;
>>         if (!cpuidle_data) {
>> @@ -1596,7 +1710,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
>>         kfree(cpuidle_data);
>>
>>   out:
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>         return ret;
>>  }
>>
>> @@ -1657,6 +1771,17 @@ static int pm_genpd_default_restore_state(struct device *dev)
>>         return cb ? cb(dev) : 0;
>>  }
>>
>> +static void genpd_lock_init(struct generic_pm_domain *genpd)
>> +{
>> +       if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
>> +               spin_lock_init(&genpd->slock);
>> +               genpd->irq_safe = true;
>> +       } else {
>> +               mutex_init(&genpd->mlock);
>> +               genpd->irq_safe = false;
>> +       }
>
>As I stated earlier around having function pointers, this would be the
>place where to assign them I guess.
>
>> +}
>> +
>>  /**
>>   * pm_genpd_init - Initialize a generic I/O PM domain object.
>>   * @genpd: PM domain object to initialize.
>> @@ -1672,7 +1797,7 @@ void pm_genpd_init(struct generic_pm_domain *genpd,
>>         INIT_LIST_HEAD(&genpd->master_links);
>>         INIT_LIST_HEAD(&genpd->slave_links);
>>         INIT_LIST_HEAD(&genpd->dev_list);
>> -       mutex_init(&genpd->lock);
>> +       genpd_lock_init(genpd);
>>         genpd->gov = gov;
>>         INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
>>         genpd->in_progress = 0;
>> @@ -2067,7 +2192,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
>>         struct gpd_link *link;
>>         int ret;
>>
>> -       ret = mutex_lock_interruptible(&genpd->lock);
>> +       ret = genpd_lock_interruptible(genpd);
>>         if (ret)
>>                 return -ERESTARTSYS;
>>
>> @@ -2087,7 +2212,8 @@ static int pm_genpd_summary_one(struct seq_file *s,
>>         }
>>
>>         list_for_each_entry(pm_data, &genpd->dev_list, list_node) {
>> -               kobj_path = kobject_get_path(&pm_data->dev->kobj, GFP_KERNEL);
>> +               kobj_path = kobject_get_path(&pm_data->dev->kobj,
>> +                               genpd->irq_safe ? GFP_ATOMIC : GFP_KERNEL);
>>                 if (kobj_path == NULL)
>>                         continue;
>>
>> @@ -2098,7 +2224,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
>>
>>         seq_puts(s, "\n");
>>  exit:
>> -       mutex_unlock(&genpd->lock);
>> +       genpd_unlock(genpd);
>>
>>         return 0;
>>  }
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index b2725e6..dc7cb53 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -16,9 +16,11 @@
>>  #include <linux/of.h>
>>  #include <linux/notifier.h>
>>  #include <linux/cpuidle.h>
>> +#include <linux/spinlock.h>
>>
>>  /* Defines used for the flags field in the struct generic_pm_domain */
>>  #define GENPD_FLAG_PM_CLK      (1U << 0) /* PM domain uses PM clk */
>> +#define GENPD_FLAG_IRQ_SAFE    (1U << 1) /* PM domain operates in atomic */
>>
>>  enum gpd_status {
>>         GPD_STATE_ACTIVE = 0,   /* PM domain is active */
>> @@ -49,7 +51,6 @@ struct generic_pm_domain {
>>         struct list_head master_links;  /* Links with PM domain as a master */
>>         struct list_head slave_links;   /* Links with PM domain as a slave */
>>         struct list_head dev_list;      /* List of devices */
>> -       struct mutex lock;
>>         struct dev_power_governor *gov;
>>         struct work_struct power_off_work;
>>         const char *name;
>> @@ -74,6 +75,14 @@ struct generic_pm_domain {
>>         void (*detach_dev)(struct generic_pm_domain *domain,
>>                            struct device *dev);
>>         unsigned int flags;             /* Bit field of configs for genpd */
>> +       bool irq_safe;
>
>If you convert to functions pointers, perhaps you can remove the
>irq_safe bool here, and instead only use the flags field.
>
Answered earlier.

Another point to consider is that, this facilitates the use of
__acquires() and __releases(), which need a lock object and not a
pointer.

>> +       union {
>> +               struct mutex mlock;
>> +               struct {
>> +                       spinlock_t slock;
>> +                       unsigned long lock_flags;
>> +               };
>> +       };
>>  };
>>
>>  static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
>> --
>> 2.1.4
>>
>
>Kind regards
>Uffe

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Russell King - ARM Linux]

On Fri, Sep 04, 2015 at 09:12:02AM -0600, Lina Iyer wrote:
> On Fri, Sep 04 2015 at 03:27 -0600, Russell King - ARM Linux wrote:
> >On Fri, Sep 04, 2015 at 10:17:35AM +0100, Russell King - ARM Linux wrote:
> >>On Thu, Sep 03, 2015 at 01:58:34PM -0600, Lina Iyer wrote:
> >>> @@ -401,6 +412,11 @@ asmlinkage void secondary_start_kernel(void)
> >>>  	local_irq_enable();
> >>>  	local_fiq_enable();
> >>>
> >>> +	/* We are running, enable runtime PM for the CPU. */
> >>> +	cpu_dev = get_cpu_device(cpu);
> >>> +	if (cpu_dev)
> >>> +		pm_runtime_get_sync(cpu_dev);
> >>> +
> >>
> >>Please no.  This is fragile.
> >>
> >>pm_runtime_get_sync() can sleep, and this path is used when the system
> >>is fully up and running.  Locks may be held, which cause this to sleep.
> >>However, we're calling it from a context where we can't sleep - which
> >>is the general rule for any part of system initialisation where we
> >>have not entered the idle thread yet (the idle thread is what we run
> >>when sleeping.)
> >>
> More explanation below.
> 
> Another patch (3/7) in this series defines CPU devices as IRQ safe and
> therefore the dev->power.lock would be spinlock'd before calling the
> rest of the PM runtime code and the domain. The CPU PM domain would also
> be an IRQ safe domain (patch 2/7 makes the genpd framework usable in IRQ
> safe context).
> 
> >>NAK on this.
> >
> >There's another issue here.  This is obviously wrong.  If the CPU is
> >inside the PM domain, then don't you need the PM domain powered up for
> >the CPU to start executing instructions?
> >
> This would not be the case for CPU PM domains. The CPU PM domains AFAIK,
> are triggered by the same wake up interrupt that brings the CPU out of
> its power downs state. The domain hardware has to be awake before the
> CPU executes its first instruction. Thefore the hw power up is a logical
> OR of the wakeup signals to any of the cores.

You're taking the behaviour of the hardware you have in front of you
and claiming that it's true everywhere, and shoving that into generic
code.

I know, for example on OMAP, you have to power up the CPU first before
you can "wake" it.

I wouldn't be surprised if other SoCs are like that: where they require
the CPU core to be powered and held in reset, before releasing the reset
to then allow them to start executing the secondary core bringup.

Relying on hardware to do this sounds really fragile and bad design to
me.

If you want to persue your current design, don't make it generic code,
because it's got no right to be generic with assumptions like that.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Lina Iyer]

On Fri, Sep 04 2015 at 10:23 -0600, Russell King - ARM Linux wrote:
>On Fri, Sep 04, 2015 at 09:12:02AM -0600, Lina Iyer wrote:
>> On Fri, Sep 04 2015 at 03:27 -0600, Russell King - ARM Linux wrote:
>> >On Fri, Sep 04, 2015 at 10:17:35AM +0100, Russell King - ARM Linux wrote:
>> >>On Thu, Sep 03, 2015 at 01:58:34PM -0600, Lina Iyer wrote:
>> >>> @@ -401,6 +412,11 @@ asmlinkage void secondary_start_kernel(void)
>> >>>  	local_irq_enable();
>> >>>  	local_fiq_enable();
>> >>>
>> >>> +	/* We are running, enable runtime PM for the CPU. */
>> >>> +	cpu_dev = get_cpu_device(cpu);
>> >>> +	if (cpu_dev)
>> >>> +		pm_runtime_get_sync(cpu_dev);
>> >>> +
>> >>
>> >>Please no.  This is fragile.
>> >>
>> >>pm_runtime_get_sync() can sleep, and this path is used when the system
>> >>is fully up and running.  Locks may be held, which cause this to sleep.
>> >>However, we're calling it from a context where we can't sleep - which
>> >>is the general rule for any part of system initialisation where we
>> >>have not entered the idle thread yet (the idle thread is what we run
>> >>when sleeping.)
>> >>
>> More explanation below.
>>
>> Another patch (3/7) in this series defines CPU devices as IRQ safe and
>> therefore the dev->power.lock would be spinlock'd before calling the
>> rest of the PM runtime code and the domain. The CPU PM domain would also
>> be an IRQ safe domain (patch 2/7 makes the genpd framework usable in IRQ
>> safe context).
>>
>> >>NAK on this.
>> >
>> >There's another issue here.  This is obviously wrong.  If the CPU is
>> >inside the PM domain, then don't you need the PM domain powered up for
>> >the CPU to start executing instructions?
>> >
>> This would not be the case for CPU PM domains. The CPU PM domains AFAIK,
>> are triggered by the same wake up interrupt that brings the CPU out of
>> its power downs state. The domain hardware has to be awake before the
>> CPU executes its first instruction. Thefore the hw power up is a logical
>> OR of the wakeup signals to any of the cores.
>
>You're taking the behaviour of the hardware you have in front of you
>and claiming that it's true everywhere, and shoving that into generic
>code.
>
>I know, for example on OMAP, you have to power up the CPU first before
>you can "wake" it.
>
>I wouldn't be surprised if other SoCs are like that: where they require
>the CPU core to be powered and held in reset, before releasing the reset
>to then allow them to start executing the secondary core bringup.
>
It would still require that the CPU's domain be powered on in the hw,
before the CPU can run Linux.

>Relying on hardware to do this sounds really fragile and bad design to
>me.
>
>If you want to persue your current design, don't make it generic code,
>because it's got no right to be generic with assumptions like that.
>
Hmm, okay. I can look at alternatives like  hotplug notifiers.

-- Lina

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Russell King - ARM Linux]

On Fri, Sep 04, 2015 at 11:02:11AM -0600, Lina Iyer wrote:
> On Fri, Sep 04 2015 at 10:23 -0600, Russell King - ARM Linux wrote:
> >You're taking the behaviour of the hardware you have in front of you
> >and claiming that it's true everywhere, and shoving that into generic
> >code.
> >
> >I know, for example on OMAP, you have to power up the CPU first before
> >you can "wake" it.
> >
> >I wouldn't be surprised if other SoCs are like that: where they require
> >the CPU core to be powered and held in reset, before releasing the reset
> >to then allow them to start executing the secondary core bringup.
> >
> It would still require that the CPU's domain be powered on in the hw,
> before the CPU can run Linux.
> 
> >Relying on hardware to do this sounds really fragile and bad design to
> >me.
> >
> >If you want to persue your current design, don't make it generic code,
> >because it's got no right to be generic with assumptions like that.
> >
> Hmm, okay. I can look at alternatives like  hotplug notifiers.

How about putting the pm resume in path of the _requesting_ CPU?
IOW, in __cpu_up().

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Grygorii Strashko]

Hi Lina,

On 09/04/2015 06:12 PM, Lina Iyer wrote:
> On Fri, Sep 04 2015 at 03:27 -0600, Russell King - ARM Linux wrote:
>> On Fri, Sep 04, 2015 at 10:17:35AM +0100, Russell King - ARM Linux wrote:
>>> On Thu, Sep 03, 2015 at 01:58:34PM -0600, Lina Iyer wrote:
>>> > @@ -401,6 +412,11 @@ asmlinkage void secondary_start_kernel(void)
>>> >      local_irq_enable();
>>> >      local_fiq_enable();
>>> >
>>> > +    /* We are running, enable runtime PM for the CPU. */
>>> > +    cpu_dev = get_cpu_device(cpu);
>>> > +    if (cpu_dev)
>>> > +        pm_runtime_get_sync(cpu_dev);
>>> > +
>>>
>>> Please no.  This is fragile.
>>>
>>> pm_runtime_get_sync() can sleep, and this path is used when the system
>>> is fully up and running.  Locks may be held, which cause this to sleep.
>>> However, we're calling it from a context where we can't sleep - which
>>> is the general rule for any part of system initialisation where we
>>> have not entered the idle thread yet (the idle thread is what we run
>>> when sleeping.)
>>>
> More explanation below.
> 
> Another patch (3/7) in this series defines CPU devices as IRQ safe and
> therefore the dev->power.lock would be spinlock'd before calling the
> rest of the PM runtime code and the domain. The CPU PM domain would also
> be an IRQ safe domain (patch 2/7 makes the genpd framework usable in IRQ
> safe context).

There is one "small" problem with such approach :(

- It's incompatible with -RT kernel, because PM runtime can't be used
in atomic context on -RT. As result, CPU's hotplug will be broken
(CPUIdle will be broken also, but CPU hotplug is more important at least for me:).

Also, as for me, PM runtime + genpd is a little bit heavy-weight things
to be used for CPUIdle and It could affect on states with small wake-up latencies.

[...]

-- 
regards,
-grygorii

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Alan Stern]

On Fri, 4 Sep 2015, Grygorii Strashko wrote:

> There is one "small" problem with such approach :(
> 
> - It's incompatible with -RT kernel, because PM runtime can't be used
> in atomic context on -RT.

Can you explain this more fully?  Why can't runtime PM be used in 
atomic context in the -rt kernels?

Alan Stern

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Grygorii Strashko]

On 09/04/2015 09:45 PM, Alan Stern wrote:
> On Fri, 4 Sep 2015, Grygorii Strashko wrote:
> 
>> There is one "small" problem with such approach :(
>>
>> - It's incompatible with -RT kernel, because PM runtime can't be used
>> in atomic context on -RT.
> 
> Can you explain this more fully?  Why can't runtime PM be used in
> atomic context in the -rt kernels?
>  

See:
 http://lwn.net/Articles/146861/
 https://rt.wiki.kernel.org/index.php/Frequently_Asked_Questions#How_does_the_CONFIG_PREEMPT_RT_patch_work.3F

spinlock_t
    Critical sections are preemptible. The _irq operations (e.g., spin_lock_irqsave())
 do -not- disable hardware interrupts. Priority inheritance is used to prevent priority
 inversion. An underlying rt_mutex is used to implement spinlock_t in PREEMPT_RT. 

As result, have to do things like:
https://lkml.org/lkml/2015/8/18/161
https://lkml.org/lkml/2015/8/18/162

Sorry for brief reply - Friday/Sat night :)

-- 
regards,
-grygorii

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Alan Stern]

On Sat, 5 Sep 2015, Grygorii Strashko wrote:

> On 09/04/2015 09:45 PM, Alan Stern wrote:
> > On Fri, 4 Sep 2015, Grygorii Strashko wrote:
> > 
> >> There is one "small" problem with such approach :(
> >>
> >> - It's incompatible with -RT kernel, because PM runtime can't be used
> >> in atomic context on -RT.
> > 
> > Can you explain this more fully?  Why can't runtime PM be used in
> > atomic context in the -rt kernels?
> >  
> 
> See:
>  http://lwn.net/Articles/146861/
>  https://rt.wiki.kernel.org/index.php/Frequently_Asked_Questions#How_does_the_CONFIG_PREEMPT_RT_patch_work.3F
> 
> spinlock_t
>     Critical sections are preemptible. The _irq operations (e.g., spin_lock_irqsave())
>  do -not- disable hardware interrupts. Priority inheritance is used to prevent priority
>  inversion. An underlying rt_mutex is used to implement spinlock_t in PREEMPT_RT. 
> 
> As result, have to do things like:
> https://lkml.org/lkml/2015/8/18/161
> https://lkml.org/lkml/2015/8/18/162
> 
> Sorry for brief reply - Friday/Sat night :)

I see.  Although we normally think of interrupt contexts as being
atomic, in an -rt kernel this isn't true any more because things like
spin_lock_irq don't actually disable interrupts.

Therefore it would be correct to say that in -rt kernels, runtime PM
can be used in interrupt context (if the device is marked as irq-safe),
but not in atomic context.  Right?

Alan Stern

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Rafael J. Wysocki]

On Saturday, September 05, 2015 11:39:20 AM Alan Stern wrote:
> On Sat, 5 Sep 2015, Grygorii Strashko wrote:
> 
> > On 09/04/2015 09:45 PM, Alan Stern wrote:
> > > On Fri, 4 Sep 2015, Grygorii Strashko wrote:
> > > 
> > >> There is one "small" problem with such approach :(
> > >>
> > >> - It's incompatible with -RT kernel, because PM runtime can't be used
> > >> in atomic context on -RT.
> > > 
> > > Can you explain this more fully?  Why can't runtime PM be used in
> > > atomic context in the -rt kernels?
> > >  
> > 
> > See:
> >  http://lwn.net/Articles/146861/
> >  https://rt.wiki.kernel.org/index.php/Frequently_Asked_Questions#How_does_the_CONFIG_PREEMPT_RT_patch_work.3F
> > 
> > spinlock_t
> >     Critical sections are preemptible. The _irq operations (e.g., spin_lock_irqsave())
> >  do -not- disable hardware interrupts. Priority inheritance is used to prevent priority
> >  inversion. An underlying rt_mutex is used to implement spinlock_t in PREEMPT_RT. 
> > 
> > As result, have to do things like:
> > https://lkml.org/lkml/2015/8/18/161
> > https://lkml.org/lkml/2015/8/18/162
> > 
> > Sorry for brief reply - Friday/Sat night :)
> 
> I see.  Although we normally think of interrupt contexts as being
> atomic, in an -rt kernel this isn't true any more because things like
> spin_lock_irq don't actually disable interrupts.
> 
> Therefore it would be correct to say that in -rt kernels, runtime PM
> can be used in interrupt context (if the device is marked as irq-safe),
> but not in atomic context.  Right?

Right.

Whatever is suitable for interrupt context in the mainline, will be suitable
for that in -rt kernels too.  However, what is suitable for the idle loop
in the mainline, may not be suitable for that in -rt kernels.

That's why raw_spin_lock/unlock() need to be used within the idle loop.

Thanks,
Rafael

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Grygorii Strashko]

On 09/07/2015 04:04 PM, Rafael J. Wysocki wrote:
> On Saturday, September 05, 2015 11:39:20 AM Alan Stern wrote:
>> On Sat, 5 Sep 2015, Grygorii Strashko wrote:
>>
>>> On 09/04/2015 09:45 PM, Alan Stern wrote:
>>>> On Fri, 4 Sep 2015, Grygorii Strashko wrote:
>>>>
>>>>> There is one "small" problem with such approach :(
>>>>>
>>>>> - It's incompatible with -RT kernel, because PM runtime can't be used
>>>>> in atomic context on -RT.
>>>>
>>>> Can you explain this more fully?  Why can't runtime PM be used in
>>>> atomic context in the -rt kernels?
>>>>   
>>>
>>> See:
>>>   http://lwn.net/Articles/146861/
>>>   https://rt.wiki.kernel.org/index.php/Frequently_Asked_Questions#How_does_the_CONFIG_PREEMPT_RT_patch_work.3F
>>>
>>> spinlock_t
>>>      Critical sections are preemptible. The _irq operations (e.g., spin_lock_irqsave())
>>>   do -not- disable hardware interrupts. Priority inheritance is used to prevent priority
>>>   inversion. An underlying rt_mutex is used to implement spinlock_t in PREEMPT_RT.
>>>
>>> As result, have to do things like:
>>> https://lkml.org/lkml/2015/8/18/161
>>> https://lkml.org/lkml/2015/8/18/162
>>>
>>> Sorry for brief reply - Friday/Sat night :)
>>
>> I see.  Although we normally think of interrupt contexts as being
>> atomic, in an -rt kernel this isn't true any more because things like
>> spin_lock_irq don't actually disable interrupts.
>>
>> Therefore it would be correct to say that in -rt kernels, runtime PM
>> can be used in interrupt context (if the device is marked as irq-safe),
>> but not in atomic context.  Right?
> 
> Right.
> 
> Whatever is suitable for interrupt context in the mainline, will be suitable
> for that in -rt kernels too. 

Not exactly true :(, since spinlock is converted to [rt_] mutex.
Usually, this difference can't be seen because on -RT kernel all or
mostly all HW IRQ handlers will be forced to be threaded.
For the cases, where such automatic conversion is not working,
(like chained irq handlers or HW-handler+Threaded handler) the code
has to be carefully patched to work properly as for non-RT as for -RT.

Also, this triggers some -RT incompatibility issues, like with PM runtime or
CLK framework (http://www.spinics.net/lists/linux-rt-users/msg13653.html).

> However, what is suitable for the idle loop
> in the mainline, may not be suitable for that in -rt kernels.
> 
> That's why raw_spin_lock/unlock() need to be used within the idle loop.

Indeed. CPU hotplug/CPUIdle is guarded by local_irq_disable()/local_irq_enable().
(example of CPU hotplug RT-issue http://www.spinics.net/lists/arm-kernel/msg438963.html).

I don't want to be the final authority here as my experience with -RT is short.
But, I want to point out on potential issues based on what I've already discovered
and tried to fix.

Thanks & regards,
-grygorii

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Rafael J. Wysocki]

On Monday, September 07, 2015 04:37:44 PM Grygorii Strashko wrote:
> On 09/07/2015 04:04 PM, Rafael J. Wysocki wrote:
> > On Saturday, September 05, 2015 11:39:20 AM Alan Stern wrote:
> >> On Sat, 5 Sep 2015, Grygorii Strashko wrote:
> >>
> >>> On 09/04/2015 09:45 PM, Alan Stern wrote:
> >>>> On Fri, 4 Sep 2015, Grygorii Strashko wrote:
> >>>>
> >>>>> There is one "small" problem with such approach :(
> >>>>>
> >>>>> - It's incompatible with -RT kernel, because PM runtime can't be used
> >>>>> in atomic context on -RT.
> >>>>
> >>>> Can you explain this more fully?  Why can't runtime PM be used in
> >>>> atomic context in the -rt kernels?
> >>>>   
> >>>
> >>> See:
> >>>   http://lwn.net/Articles/146861/
> >>>   https://rt.wiki.kernel.org/index.php/Frequently_Asked_Questions#How_does_the_CONFIG_PREEMPT_RT_patch_work.3F
> >>>
> >>> spinlock_t
> >>>      Critical sections are preemptible. The _irq operations (e.g., spin_lock_irqsave())
> >>>   do -not- disable hardware interrupts. Priority inheritance is used to prevent priority
> >>>   inversion. An underlying rt_mutex is used to implement spinlock_t in PREEMPT_RT.
> >>>
> >>> As result, have to do things like:
> >>> https://lkml.org/lkml/2015/8/18/161
> >>> https://lkml.org/lkml/2015/8/18/162
> >>>
> >>> Sorry for brief reply - Friday/Sat night :)
> >>
> >> I see.  Although we normally think of interrupt contexts as being
> >> atomic, in an -rt kernel this isn't true any more because things like
> >> spin_lock_irq don't actually disable interrupts.
> >>
> >> Therefore it would be correct to say that in -rt kernels, runtime PM
> >> can be used in interrupt context (if the device is marked as irq-safe),
> >> but not in atomic context.  Right?
> > 
> > Right.
> > 
> > Whatever is suitable for interrupt context in the mainline, will be suitable
> > for that in -rt kernels too. 
> 
> Not exactly true :(, since spinlock is converted to [rt_] mutex.
> Usually, this difference can't be seen because on -RT kernel all or
> mostly all HW IRQ handlers will be forced to be threaded.

Exactly.  And that's what I'm talking about.

> For the cases, where such automatic conversion is not working,
> (like chained irq handlers or HW-handler+Threaded handler) the code
> has to be carefully patched to work properly as for non-RT as for -RT.

Right.

> Also, this triggers some -RT incompatibility issues, like with PM runtime or

That I'm not sure about.  Why would runtime PM cause problems with -RT (apart
from attempts to use it from the idle loop, but that's not happening in the
mainline anyway)?

> CLK framework (http://www.spinics.net/lists/linux-rt-users/msg13653.html).
>
> > However, what is suitable for the idle loop
> > in the mainline, may not be suitable for that in -rt kernels.
> > 
> > That's why raw_spin_lock/unlock() need to be used within the idle loop.
> 
> Indeed. CPU hotplug/CPUIdle is guarded by local_irq_disable()/local_irq_enable().
> (example of CPU hotplug RT-issue http://www.spinics.net/lists/arm-kernel/msg438963.html).
> 
> I don't want to be the final authority here as my experience with -RT is short.
> But, I want to point out on potential issues based on what I've already discovered
> and tried to fix.

OK

Thanks,
Rafael

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Grygorii Strashko]

On 09/07/2015 11:42 PM, Rafael J. Wysocki wrote:
> On Monday, September 07, 2015 04:37:44 PM Grygorii Strashko wrote:
>> On 09/07/2015 04:04 PM, Rafael J. Wysocki wrote:
>>> On Saturday, September 05, 2015 11:39:20 AM Alan Stern wrote:
>>>> On Sat, 5 Sep 2015, Grygorii Strashko wrote:
>>>>
>>>>> On 09/04/2015 09:45 PM, Alan Stern wrote:
>>>>>> On Fri, 4 Sep 2015, Grygorii Strashko wrote:
>>>>>>
>>>>>>> There is one "small" problem with such approach :(
>>>>>>>
>>>>>>> - It's incompatible with -RT kernel, because PM runtime can't be used
>>>>>>> in atomic context on -RT.
>>>>>>
>>>>>> Can you explain this more fully?  Why can't runtime PM be used in
>>>>>> atomic context in the -rt kernels?
>>>>>>    
>>>>>
>>>>> See:
>>>>>    http://lwn.net/Articles/146861/
>>>>>    https://rt.wiki.kernel.org/index.php/Frequently_Asked_Questions#How_does_the_CONFIG_PREEMPT_RT_patch_work.3F
>>>>>
>>>>> spinlock_t
>>>>>       Critical sections are preemptible. The _irq operations (e.g., spin_lock_irqsave())
>>>>>    do -not- disable hardware interrupts. Priority inheritance is used to prevent priority
>>>>>    inversion. An underlying rt_mutex is used to implement spinlock_t in PREEMPT_RT.
>>>>>
>>>>> As result, have to do things like:
>>>>> https://lkml.org/lkml/2015/8/18/161
>>>>> https://lkml.org/lkml/2015/8/18/162
>>>>>
>>>>> Sorry for brief reply - Friday/Sat night :)
>>>>
>>>> I see.  Although we normally think of interrupt contexts as being
>>>> atomic, in an -rt kernel this isn't true any more because things like
>>>> spin_lock_irq don't actually disable interrupts.
>>>>
>>>> Therefore it would be correct to say that in -rt kernels, runtime PM
>>>> can be used in interrupt context (if the device is marked as irq-safe),
>>>> but not in atomic context.  Right?
>>>
>>> Right.
>>>
>>> Whatever is suitable for interrupt context in the mainline, will be suitable
>>> for that in -rt kernels too.
>>
>> Not exactly true :(, since spinlock is converted to [rt_] mutex.
>> Usually, this difference can't be seen because on -RT kernel all or
>> mostly all HW IRQ handlers will be forced to be threaded.
> 
> Exactly.  And that's what I'm talking about.
> 
>> For the cases, where such automatic conversion is not working,
>> (like chained irq handlers or HW-handler+Threaded handler) the code
>> has to be carefully patched to work properly as for non-RT as for -RT.
> 
> Right.
> 
>> Also, this triggers some -RT incompatibility issues, like with PM runtime or
> 
> That I'm not sure about.  Why would runtime PM cause problems with -RT (apart
> from attempts to use it from the idle loop, but that's not happening in the
> mainline anyway)?


I have to be more specific - sorry. "irq_safe" mode of PM runtime is incompatible with -RT.

Here is an example:
- HW IRQ handler in TI OMAP GPIO driver is implemented as chained IRQ handler and
  contains pm_runtime_get_sync()/pm_runtime_put(). This works properly with vanilla kernel
  because OMAP GPIO devices marked as irq_safe.
  Chained IRQ handlers can't be forced threaded and PM runtime APIs trigger
 "sleeping function called from invalid context" issues there, so corresponding code has to be reworked.


...


-- 
regards,
-grygorii

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Kevin Hilman]

Grygorii Strashko <grygorii.strashko@ti.com> writes:

> On 09/07/2015 11:42 PM, Rafael J. Wysocki wrote:
>> On Monday, September 07, 2015 04:37:44 PM Grygorii Strashko wrote:
>>> On 09/07/2015 04:04 PM, Rafael J. Wysocki wrote:
>>>> On Saturday, September 05, 2015 11:39:20 AM Alan Stern wrote:
>>>>> On Sat, 5 Sep 2015, Grygorii Strashko wrote:
>>>>>
>>>>>> On 09/04/2015 09:45 PM, Alan Stern wrote:
>>>>>>> On Fri, 4 Sep 2015, Grygorii Strashko wrote:
>>>>>>>
>>>>>>>> There is one "small" problem with such approach :(
>>>>>>>>
>>>>>>>> - It's incompatible with -RT kernel, because PM runtime can't be used
>>>>>>>> in atomic context on -RT.
>>>>>>>
>>>>>>> Can you explain this more fully?  Why can't runtime PM be used in
>>>>>>> atomic context in the -rt kernels?
>>>>>>>    
>>>>>>
>>>>>> See:
>>>>>>    http://lwn.net/Articles/146861/
>>>>>>    https://rt.wiki.kernel.org/index.php/Frequently_Asked_Questions#How_does_the_CONFIG_PREEMPT_RT_patch_work.3F
>>>>>>
>>>>>> spinlock_t
>>>>>>       Critical sections are preemptible. The _irq operations (e.g., spin_lock_irqsave())
>>>>>>    do -not- disable hardware interrupts. Priority inheritance is used to prevent priority
>>>>>>    inversion. An underlying rt_mutex is used to implement spinlock_t in PREEMPT_RT.
>>>>>>
>>>>>> As result, have to do things like:
>>>>>> https://lkml.org/lkml/2015/8/18/161
>>>>>> https://lkml.org/lkml/2015/8/18/162
>>>>>>
>>>>>> Sorry for brief reply - Friday/Sat night :)
>>>>>
>>>>> I see.  Although we normally think of interrupt contexts as being
>>>>> atomic, in an -rt kernel this isn't true any more because things like
>>>>> spin_lock_irq don't actually disable interrupts.
>>>>>
>>>>> Therefore it would be correct to say that in -rt kernels, runtime PM
>>>>> can be used in interrupt context (if the device is marked as irq-safe),
>>>>> but not in atomic context.  Right?
>>>>
>>>> Right.
>>>>
>>>> Whatever is suitable for interrupt context in the mainline, will be suitable
>>>> for that in -rt kernels too.
>>>
>>> Not exactly true :(, since spinlock is converted to [rt_] mutex.
>>> Usually, this difference can't be seen because on -RT kernel all or
>>> mostly all HW IRQ handlers will be forced to be threaded.
>> 
>> Exactly.  And that's what I'm talking about.
>> 
>>> For the cases, where such automatic conversion is not working,
>>> (like chained irq handlers or HW-handler+Threaded handler) the code
>>> has to be carefully patched to work properly as for non-RT as for -RT.
>> 
>> Right.
>> 
>>> Also, this triggers some -RT incompatibility issues, like with PM runtime or
>> 
>> That I'm not sure about.  Why would runtime PM cause problems with -RT (apart
>> from attempts to use it from the idle loop, but that's not happening in the
>> mainline anyway)?
>
>
> I have to be more specific - sorry. "irq_safe" mode of PM runtime is incompatible with -RT.
>
> Here is an example:
> - HW IRQ handler in TI OMAP GPIO driver is implemented as chained IRQ handler and
>   contains pm_runtime_get_sync()/pm_runtime_put(). This works properly with vanilla kernel
>   because OMAP GPIO devices marked as irq_safe.
>   Chained IRQ handlers can't be forced threaded and PM runtime APIs trigger
>  "sleeping function called from invalid context" issues there, so corresponding code has to be reworked.

Isn't that why those are being converted to raw_*[1] ?

Kevin

[1] http://marc.info/?l=linux-omap&m=143749603401221&w=2

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Grygorii Strashko]

On 09/09/2015 01:03 AM, Kevin Hilman wrote:
> Grygorii Strashko <grygorii.strashko@ti.com> writes:
> 
>> On 09/07/2015 11:42 PM, Rafael J. Wysocki wrote:
>>> On Monday, September 07, 2015 04:37:44 PM Grygorii Strashko wrote:
>>>> On 09/07/2015 04:04 PM, Rafael J. Wysocki wrote:
>>>>> On Saturday, September 05, 2015 11:39:20 AM Alan Stern wrote:
>>>>>> On Sat, 5 Sep 2015, Grygorii Strashko wrote:
>>>>>>
>>>>>>> On 09/04/2015 09:45 PM, Alan Stern wrote:
>>>>>>>> On Fri, 4 Sep 2015, Grygorii Strashko wrote:
>>>>>>>>
>>>>>>>>> There is one "small" problem with such approach :(
>>>>>>>>>
>>>>>>>>> - It's incompatible with -RT kernel, because PM runtime can't be used
>>>>>>>>> in atomic context on -RT.
>>>>>>>>
>>>>>>>> Can you explain this more fully?  Why can't runtime PM be used in
>>>>>>>> atomic context in the -rt kernels?
>>>>>>>>     
>>>>>>>
>>>>>>> See:
>>>>>>>     http://lwn.net/Articles/146861/
>>>>>>>     https://rt.wiki.kernel.org/index.php/Frequently_Asked_Questions#How_does_the_CONFIG_PREEMPT_RT_patch_work.3F
>>>>>>>
>>>>>>> spinlock_t
>>>>>>>        Critical sections are preemptible. The _irq operations (e.g., spin_lock_irqsave())
>>>>>>>     do -not- disable hardware interrupts. Priority inheritance is used to prevent priority
>>>>>>>     inversion. An underlying rt_mutex is used to implement spinlock_t in PREEMPT_RT.
>>>>>>>
>>>>>>> As result, have to do things like:
>>>>>>> https://lkml.org/lkml/2015/8/18/161
>>>>>>> https://lkml.org/lkml/2015/8/18/162
>>>>>>>
>>>>>>> Sorry for brief reply - Friday/Sat night :)
>>>>>>
>>>>>> I see.  Although we normally think of interrupt contexts as being
>>>>>> atomic, in an -rt kernel this isn't true any more because things like
>>>>>> spin_lock_irq don't actually disable interrupts.
>>>>>>
>>>>>> Therefore it would be correct to say that in -rt kernels, runtime PM
>>>>>> can be used in interrupt context (if the device is marked as irq-safe),
>>>>>> but not in atomic context.  Right?
>>>>>
>>>>> Right.
>>>>>
>>>>> Whatever is suitable for interrupt context in the mainline, will be suitable
>>>>> for that in -rt kernels too.
>>>>
>>>> Not exactly true :(, since spinlock is converted to [rt_] mutex.
>>>> Usually, this difference can't be seen because on -RT kernel all or
>>>> mostly all HW IRQ handlers will be forced to be threaded.
>>>
>>> Exactly.  And that's what I'm talking about.
>>>
>>>> For the cases, where such automatic conversion is not working,
>>>> (like chained irq handlers or HW-handler+Threaded handler) the code
>>>> has to be carefully patched to work properly as for non-RT as for -RT.
>>>
>>> Right.
>>>
>>>> Also, this triggers some -RT incompatibility issues, like with PM runtime or
>>>
>>> That I'm not sure about.  Why would runtime PM cause problems with -RT (apart
>>> from attempts to use it from the idle loop, but that's not happening in the
>>> mainline anyway)?
>>
>>
>> I have to be more specific - sorry. "irq_safe" mode of PM runtime is incompatible with -RT.
>>
>> Here is an example:
>> - HW IRQ handler in TI OMAP GPIO driver is implemented as chained IRQ handler and
>>    contains pm_runtime_get_sync()/pm_runtime_put(). This works properly with vanilla kernel
>>    because OMAP GPIO devices marked as irq_safe.
>>    Chained IRQ handlers can't be forced threaded and PM runtime APIs trigger
>>   "sleeping function called from invalid context" issues there, so corresponding code has to be reworked.
> 
> Isn't that why those are being converted to raw_*[1] ?
> 
> Kevin
> 
> [1] http://marc.info/?l=linux-omap&m=143749603401221&w=2
> 

That's way I've tried to convert those to generic IRQ handler [2] :), 
so on -RT it will be forced threaded. 

raw_* is different kind of problem in gpio-omap - IRQ controllers
have to use raw_* inside irq_chip callbacks, because IRQ core guards those
callbacks using raw_* locks. 
.irq_bus_lock()/irq_bus_sync_unlock() callbacks can be used [3]
for any kind of operations which require non-atomic context.

[2] https://lkml.org/lkml/2015/8/18/162
gpio: omap: convert to use generic irq handler
[3] https://lkml.org/lkml/2015/8/18/161
gpio: omap: move pm runtime in irq_chip.irq_bus_lock/sync_unlock



-- 
regards,
-grygorii

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Lina Iyer]

On Thu, Sep 10 2015 at 04:01 -0700, Grygorii Strashko wrote:
>On 09/09/2015 01:03 AM, Kevin Hilman wrote:
>> Grygorii Strashko <grygorii.strashko@ti.com> writes:
>>
>>> On 09/07/2015 11:42 PM, Rafael J. Wysocki wrote:
>>>> On Monday, September 07, 2015 04:37:44 PM Grygorii Strashko wrote:
>>>>> On 09/07/2015 04:04 PM, Rafael J. Wysocki wrote:
>>>>>> On Saturday, September 05, 2015 11:39:20 AM Alan Stern wrote:
>>>>>>> On Sat, 5 Sep 2015, Grygorii Strashko wrote:
>>>>>>>
>>>>>>>> On 09/04/2015 09:45 PM, Alan Stern wrote:
>>>>>>>>> On Fri, 4 Sep 2015, Grygorii Strashko wrote:
>>>>>>>>>
>>>>>>>>>> There is one "small" problem with such approach :(
>>>>>>>>>>
>>>>>>>>>> - It's incompatible with -RT kernel, because PM runtime can't be used
>>>>>>>>>> in atomic context on -RT.
>>>>>>>>>
>>>>>>>>> Can you explain this more fully?  Why can't runtime PM be used in
>>>>>>>>> atomic context in the -rt kernels?
>>>>>>>>>
>>>>>>>>
>>>>>>>> See:
>>>>>>>>     http://lwn.net/Articles/146861/
>>>>>>>>     https://rt.wiki.kernel.org/index.php/Frequently_Asked_Questions#How_does_the_CONFIG_PREEMPT_RT_patch_work.3F
>>>>>>>>
>>>>>>>> spinlock_t
>>>>>>>>        Critical sections are preemptible. The _irq operations (e.g., spin_lock_irqsave())
>>>>>>>>     do -not- disable hardware interrupts. Priority inheritance is used to prevent priority
>>>>>>>>     inversion. An underlying rt_mutex is used to implement spinlock_t in PREEMPT_RT.
>>>>>>>>
>>>>>>>> As result, have to do things like:
>>>>>>>> https://lkml.org/lkml/2015/8/18/161
>>>>>>>> https://lkml.org/lkml/2015/8/18/162
>>>>>>>>
>>>>>>>> Sorry for brief reply - Friday/Sat night :)
>>>>>>>
>>>>>>> I see.  Although we normally think of interrupt contexts as being
>>>>>>> atomic, in an -rt kernel this isn't true any more because things like
>>>>>>> spin_lock_irq don't actually disable interrupts.
>>>>>>>
>>>>>>> Therefore it would be correct to say that in -rt kernels, runtime PM
>>>>>>> can be used in interrupt context (if the device is marked as irq-safe),
>>>>>>> but not in atomic context.  Right?
>>>>>>
>>>>>> Right.
>>>>>>
>>>>>> Whatever is suitable for interrupt context in the mainline, will be suitable
>>>>>> for that in -rt kernels too.
>>>>>
>>>>> Not exactly true :(, since spinlock is converted to [rt_] mutex.
>>>>> Usually, this difference can't be seen because on -RT kernel all or
>>>>> mostly all HW IRQ handlers will be forced to be threaded.
>>>>
>>>> Exactly.  And that's what I'm talking about.
>>>>
>>>>> For the cases, where such automatic conversion is not working,
>>>>> (like chained irq handlers or HW-handler+Threaded handler) the code
>>>>> has to be carefully patched to work properly as for non-RT as for -RT.
>>>>
>>>> Right.
>>>>
>>>>> Also, this triggers some -RT incompatibility issues, like with PM runtime or
>>>>
>>>> That I'm not sure about.  Why would runtime PM cause problems with -RT (apart
>>>> from attempts to use it from the idle loop, but that's not happening in the
>>>> mainline anyway)?
>>>
>>>
>>> I have to be more specific - sorry. "irq_safe" mode of PM runtime is incompatible with -RT.
>>>
>>> Here is an example:
>>> - HW IRQ handler in TI OMAP GPIO driver is implemented as chained IRQ handler and
>>>    contains pm_runtime_get_sync()/pm_runtime_put(). This works properly with vanilla kernel
>>>    because OMAP GPIO devices marked as irq_safe.
>>>    Chained IRQ handlers can't be forced threaded and PM runtime APIs trigger
>>>   "sleeping function called from invalid context" issues there, so corresponding code has to be reworked.
>>
>> Isn't that why those are being converted to raw_*[1] ?
>>
>> Kevin
>>
>> [1] http://marc.info/?l=linux-omap&m=143749603401221&w=2
>>
>
>That's way I've tried to convert those to generic IRQ handler [2] :),
>so on -RT it will be forced threaded.
>
>raw_* is different kind of problem in gpio-omap - IRQ controllers
>have to use raw_* inside irq_chip callbacks, because IRQ core guards those
>callbacks using raw_* locks.
>.irq_bus_lock()/irq_bus_sync_unlock() callbacks can be used [3]
>for any kind of operations which require non-atomic context.
>
Talking to John Stultz at Linaro Connect: Is cpuidle relevant in
-RT kernel? I dont know much about -RT. I thought this might be point
that we should consider.

As it stands today, on a 800 Mhz ARM quad core, I am seeing a latency of
50-70 usec for the additional runtime PM in the cpuidle. Ofcourse, there
is a definite need to optimize and there are opportunities to do that.

Since each CPU does its own runtime PM, we could probably avoid any kind
of locks in the runtime PM. But locks in genpd may be unavoidable. Will
look more into that.

Thanks,
Lina

>[2] https://lkml.org/lkml/2015/8/18/162
>gpio: omap: convert to use generic irq handler
>[3] https://lkml.org/lkml/2015/8/18/161
>gpio: omap: move pm runtime in irq_chip.irq_bus_lock/sync_unlock
>
>
>
>-- 
>regards,
>-grygorii

[PATCH v2 7/7] ARM: smp: Add runtime PM support for CPU hotplug

[Thomas Gleixner]

On Tue, 22 Sep 2015, Lina Iyer wrote:
> Talking to John Stultz at Linaro Connect: Is cpuidle relevant in
> -RT kernel? I dont know much about -RT. I thought this might be point
> that we should consider.

There are definitely RT systems out there where power consumption is a
concern.
 
> As it stands today, on a 800 Mhz ARM quad core, I am seeing a latency of
> 50-70 usec for the additional runtime PM in the cpuidle. Ofcourse, there
> is a definite need to optimize and there are opportunities to do that.

RT is not about being fast. RT is about being deterministic. So if a
power sensitive RT system can afford the extra latencies induced by PM
it will tolerate them. Of course, optimizing that is not a bad thing
at all :)
 
Thanks,

	tglx

[PATCH v2 6/7] ARM64: smp: Add runtime PM support for CPU hotplug

[Geert Uytterhoeven]

Hi Lina,

On Thu, Sep 3, 2015 at 9:58 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index dbdaacd..4076962 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -135,6 +135,7 @@ asmlinkage void secondary_start_kernel(void)
>  {
>         struct mm_struct *mm = &init_mm;
>         unsigned int cpu = smp_processor_id();
> +       struct device *cpu_dev;
>
>         /*
>          * All kernel threads share the same mm context; grab a
> @@ -185,6 +186,11 @@ asmlinkage void secondary_start_kernel(void)
>         local_irq_enable();
>         local_async_enable();
>
> +       /* We are running, enable runtime PM for the CPU. */
> +       cpu_dev = get_cpu_device(cpu);
> +       if (cpu_dev)
> +               pm_runtime_get_sync(cpu_dev);

arch/arm64/kernel/smp.c:192:3: error: implicit declaration of function
'pm_runtime_get_sync' [-Werror=implicit-function-declaration]

Adding #include <linux/pm_runtime.h> fixes compilation.

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

[PATCH v2 2/7] PM / Domains: Support IRQ safe PM domains

[Geert Uytterhoeven]

Hi Lina,

On Thu, Sep 3, 2015 at 9:58 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> Generic Power Domains currently support turning on/off only in process
> context. This prevents the usage of PM domains for domains that could be
> powered on/off in a context where IRQs are disabled. Many such domains
> exist today and do not get powered off, when the IRQ safe devices in
> that domain are powered off, because of this limitation.
>
> However, not all domains can operate in IRQ safe contexts. Genpd
> therefore, has to support both cases where the domain may or may not
> operate in IRQ safe contexts. Configuring genpd to use an appropriate
> lock for that domain, would allow domains that have IRQ safe devices to
> runtime suspend and resume, in atomic context.
>
> To achieve domain specific locking, set the domain's ->flag to
> GENPD_FLAG_IRQ_SAFE while defining the domain. This indicates that genpd
> should use a spinlock instead of a mutex for locking the domain. Locking
> is abstracted through genpd_lock() and genpd_unlock() functions that use
> the flag to determine the appropriate lock to be used for that domain.
> Domains that have lower latency to suspend and resume and can operate
> with IRQs disabled may now be able to save power, when the component
> devices and sub-domains are idle at runtime.
>
> The restriction this imposes on the domain hierarchy is that sub-domains
> and all devices in the IRQ safe domain's hierarchy also have to be IRQ
> safe, so that we dont try to lock a mutex, while holding a spinlock.
> Non-IRQ safe domains may continue to have devices and sub-domains that
> may or may not be IRQ safe.
>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Kevin Hilman <khilman@linaro.org>
> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Krzysztof Koz?owski <k.kozlowski@samsung.com>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>

> @@ -394,8 +468,17 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
>                 if (stat > PM_QOS_FLAGS_NONE)
>                         return -EBUSY;
>
> -               if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe)
> +               /*
> +                * We do not want to power off the domain if the device is
> +                * not suspended or an IRQ safe device is part of this
> +                * non-IRQ safe domain.
> +                */
> +               if (!pm_runtime_suspended(pdd->dev) ||
> +                       irq_safe_dev_in_no_sleep_domain(pdd->dev, genpd))
>                         not_suspended++;
> +               WARN_ONCE(irq_safe_dev_in_no_sleep_domain(pdd->dev, genpd),
> +                               "PM domain %s will not be powered off\n",

Does this warrant a WARN_ON(), i.e. is this really something that should not
happen?

This prints "PM domain %s" ...

> +                               genpd->name);

Please also print dev_name(pdd->dev), to make debugging easier.

> @@ -500,17 +583,21 @@ static int pm_genpd_runtime_suspend(struct device *dev)
>         }
>
>         /*
> -        * If power.irq_safe is set, this routine will be run with interrupts
> -        * off, so it can't use mutexes.
> +        * If power.irq_safe is set, this routine may be run with
> +        * IRQ disabled, so suspend only if the power domain is
> +        * irq_safe.
>          */
> -       if (dev->power.irq_safe)
> +       WARN_ONCE(irq_safe_dev_in_no_sleep_domain(dev, genpd),
> +                       "genpd %s will not be powered off\n", genpd->name);

Does this warrant a WARN_ON(), i.e. is this really something that should not
happen?

... while this prints "genpd %s".

Please also print dev_name(pdd->dev), to make debugging easier.

> +       if (irq_safe_dev_in_no_sleep_domain(dev, genpd))
>                 return 0;

> @@ -1280,11 +1370,18 @@ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev,
>         if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev))
>                 return -EINVAL;
>
> +       if (genpd->irq_safe && !dev->power.irq_safe) {
> +               dev_err(dev,
> +                       "PM Domain %s is IRQ safe; device has to IRQ safe.\n",

... has to be IRQ safe

> +                       genpd->name);

The reason I'm asking about the WARN_ON()s is that both are triggered on the
two systems I tried your series on:
  - r8a7740/armadillo has real hardware power domains and a clock domain,
    using the renesas,sysc-rmobile PM Domain driver,
  - r8a7791/koelsch has a clock domain, using the renesas,cpg-mstp-clocks
    CPG/MSTP clock domain driver.

Both have renesas,cmt and/or renesas,tmu timers, which are one of the few
existing IRQ safe devices, causing the warnings.

Apart from the warnings, the system seems to work fine.

Making the PM resp. Clock Domain irqsafe by setting GENPD_FLAG_IRQ_SAFE makes
matters worse, as you cannot attach non-irq safe devices to an irq safe genpd,
which causes the system to fail to boot.

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