PM / Domains: Generic PM domains for cpus

In order for domains to be powered on/off in irq locked context, the domain locks could either be a spinlock or a mutex, depending on the domain. In preparation for atomic support, allocate domain data outside the domain locks, so the allocation calls dont have to be context sensitive.

Cc Ulf Hansson <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Kevin Hilman <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/base/power/domain.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

drivers/base/power/domain.c +7 -8

index 3b3367b..dfd7595 100644
@@ -1367,13 +1367,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);
 

@@ -1383,18 +1387,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;

@@ -1496,17 +1495,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
 	if (IS_ERR_OR_NULL(genpd) || state < 0)
 		return -EINVAL;

drivers/base/power/domain.c +1 -5

 	}
-	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
-	if (!cpuidle_data) {
-		ret = -ENOMEM;
-		goto out;
-	}
+
 	cpuidle_drv = cpuidle_driver_ref();
 	if (!cpuidle_drv) {
 		ret = -ENODEV;

W dniu 05.06.2015 o 07:29, Lina Iyer pisze:

Quoted 68 lines

In order for domains to be powered on/off in irq locked context, the domain locks could either be a spinlock or a mutex, depending on the domain. In preparation for atomic support, allocate domain data outside the domain locks, so the allocation calls dont have to be context sensitive.

Cc Ulf Hansson <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Kevin Hilman <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/base/power/domain.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

drivers/base/power/domain.c +11 -8

index 3b3367b..dfd7595 100644
@@ -1367,13 +1367,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);
 

@@ -1383,18 +1387,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;

@@ -1496,17 +1495,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);
 

drivers/base/power/domain.c int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state) · line 1506

Quoted 3 lines

drivers/base/power/domain.c +0 -0

 	if (genpd->cpuidle_data) {
 		ret = -EEXIST;
 		goto out;

I think you must update the error paths. In this case the kfree() is missing.

Quoted 12 lines

drivers/base/power/domain.c +1 -5

 	}
-	cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
-	if (!cpuidle_data) {
-		ret = -ENOMEM;
-		goto out;
-	}
+
 	cpuidle_drv = cpuidle_driver_ref();
 	if (!cpuidle_drv) {
 		ret = -ENODEV;

In order for domains to be powered on/off in irq locked context, the domain locks could either be a spinlock or a mutex, depending on the domain.

In preparation for atomic support, allocate domain data outside

the domain locks, so the allocation calls dont have to be context sensitive.

Power Domains currently support turning on/off only in process context. This restricts the usage of PM domains to devices and domains that could be powered on/off in irq disabled contexts as the mutexes used in GenPD allows for cpu sleep while waiting for locks.

Genpd inherently provides support for devices, domain hierarchy and can be used to represent cpu clusters like in ARM's big.Little, where, each cpu cluster is in its domain, with supporting caches and other peripheral hardware. Multiple such domains could be part of another domain. Because mutexes are used to protect and synchronize domain operations and cpu idle operations are inherently atomic, the use of genpd is not possible for runtime suspend and resume of the pm domain. Replacing the locks to spinlocks would allow cpu domain to be be powered off to save power, when all the cpus are powered off.

However, not all domains can operate in irq-safe contexts and usually would need to sleep during domain operations. So genpd has to support both the cases, where the domain is or is not irq-safe. The irq-safe attribute is therefore domain specific.

To achieve domain specific locking, set the GENPD_FLAG_IRQ_SAFE flag while defining the domain. This determines if the domain should use a spinlock instead of a mutex. Locking is abstracted through genpd_lock_domain() and genpd_unlock_domain() functions that use the flag to determine the locking to be used for this domain.

The restriction this imposes on the domain hierarchy is that subdomains and all devices in the hierarchy also be irq-safe. Non irq-safe domains may continue to have irq-safe devices, but not the other way around.

Cc Ulf Hansson <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Kevin Hilman <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/base/power/domain.c | 200 ++++++++++++++++++++++++++++++++++----------
 include/linux/pm_domain.h   |  11 ++-

 2 files changed, 164 insertions(+), 47 deletions(-)

drivers/base/power/domain.c +54 -0

index dfd7595..8b89d15 100644
@@ -50,6 +50,71 @@
 static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
 
+static inline int genpd_lock_domain_noirq(struct generic_pm_domain *genpd,
+					unsigned int subclass)
+	__acquires(&genpd->slock)
+{
+	unsigned long flags;
+
+	if (unlikely(subclass > 0))
+		spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
+	else
+		spin_lock_irqsave(&genpd->slock, flags);
+
+	genpd->flags = flags;
+
+	return 0;
+}
+
+static inline int genpd_unlock_domain_noirq(struct generic_pm_domain *genpd)
+	__releases(&genpd->slock)
+{
+	spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
+	return 0;
+}
+
+static inline int genpd_lock_domain_irq(struct generic_pm_domain *genpd,
+					unsigned int subclass)
+	__acquires(&genpd->mlock)
+{
+	if (unlikely(subclass > 0))
+		mutex_lock_nested(&genpd->mlock, subclass);
+	else
+		mutex_lock(&genpd->mlock);
+
+	return 0;
+}
+
+static inline int genpd_lock_domain_interruptible_irq(
+				struct generic_pm_domain *genpd)
+	__acquires(&genpd->mlock)
+{
+	return mutex_lock_interruptible(&genpd->mlock);
+}
+
+static inline int genpd_unlock_domain_irq(struct generic_pm_domain *genpd)
+	__releases(&genpd->mlock)
+{
+	mutex_unlock(&genpd->mlock);
+	return 0;
+}
+
+#define genpd_lock_domain(genpd)				\
+	(genpd->irq_safe ? genpd_lock_domain_noirq(genpd, 0)	\
+			: genpd_lock_domain_irq(genpd, 0))
+
+#define genpd_lock_domain_nested(genpd)				\

drivers/base/power/domain.c +6 -5

+
 static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
 {
 	struct generic_pm_domain *genpd = NULL, *gpd;

@@ -262,9 +327,9 @@ int pm_genpd_poweron(struct generic_pm_domain *genpd)
 {
 	int ret;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock_domain(genpd);
 	ret = __pm_genpd_poweron(genpd);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock_domain(genpd);
 	return ret;
 }
 

@@ -326,9 +391,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_domain(genpd);
 			genpd->max_off_time_changed = true;
-			mutex_unlock(&genpd->lock);
+			genpd_unlock_domain(genpd);
 		}
 
 		dev = dev->parent;

@@ -387,7 +452,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 			return -EBUSY;
 
 		if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
-		    || pdd->dev->power.irq_safe))
+			|| (pdd->dev->power.irq_safe && !genpd->irq_safe)))
 			not_suspended++;
 	}
 

drivers/base/power/domain.c +26 -19

-	mutex_unlock(&genpd->lock);
+	genpd_unlock_domain(genpd);
 }
 
 /**

@@ -478,12 +543,8 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	/*
-	 * We can't allow to power off the PM domain if it holds an irq_safe
-	 * device. That's beacuse we use mutexes to protect data while power
-	 * off and on the PM domain, thus we can't execute in atomic context.
-	 */
-	if (dev->power.irq_safe)
+	/* We can't allow to power off a domain that is also not irq safe. */
+	if (dev->power.irq_safe && !genpd->irq_safe)
 		return -EBUSY;
 
 	stop_ok = genpd->gov ? genpd->gov->stop_ok : NULL;

@@ -500,11 +561,19 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 		return ret;
 	}
 
-	mutex_lock(&genpd->lock);
+	/*
+	 * If power.irq_safe is set, this routine will be run with interrupts
+	 * off, so suspend only if the power domain is irq_safe.
+	 */
+	if (dev->power.irq_safe && !genpd->irq_safe)
+		return 0;
+
+	genpd_lock_domain(genpd);
+
 	genpd->in_progress++;
 	pm_genpd_poweroff(genpd);
 	genpd->in_progress--;
-	mutex_unlock(&genpd->lock);
+	genpd_unlock_domain(genpd);
 
 	return 0;
 }

@@ -528,13 +597,16 @@ 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)
+	/*
+	 * If power.irq_safe and domain is not, then
+	 * the PM domain is never powered off.
+	 */
+	if (dev->power.irq_safe && !genpd->irq_safe)
 		return genpd_start_dev_no_timing(genpd, dev);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock_domain(genpd);
 	ret = __pm_genpd_poweron(genpd);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock_domain(genpd);
 
 	if (ret)
 		return ret;

@@ -729,14 +801,14 @@ static int pm_genpd_prepare(struct device *dev)
 	if (resume_needed(dev, genpd))
 		pm_runtime_resume(dev);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock_domain(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_domain(genpd);
 
 	if (genpd->suspend_power_off) {
 		pm_runtime_put_noidle(dev);

@@ -754,12 +826,12 @@ static int pm_genpd_prepare(struct device *dev)
 
 	ret = pm_generic_prepare(dev);
 	if (ret) {
-		mutex_lock(&genpd->lock);
+		genpd_lock_domain(genpd);
 
 		if (--genpd->prepared_count == 0)
 			genpd->suspend_power_off = false;
 
-		mutex_unlock(&genpd->lock);
+		genpd_unlock_domain(genpd);
 		pm_runtime_enable(dev);
 	}
 

@@ -1116,13 +1188,13 @@ static void pm_genpd_complete(struct device *dev)
 	if (IS_ERR(genpd))
 		return;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock_domain(genpd);
 
 	run_complete = !genpd->suspend_power_off;
 	if (--genpd->prepared_count == 0)
 		genpd->suspend_power_off = false;
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock_domain(genpd);
 
 	if (run_complete) {
 		pm_generic_complete(dev);

W dniu 05.06.2015 o 07:29, Lina Iyer pisze:

Quoted 4 lines

Power Domains currently support turning on/off only in process context. This restricts the usage of PM domains to devices and domains that could be powered on/off in irq disabled contexts as the mutexes used in GenPD allows for cpu sleep while waiting for locks.

I can find also other use case: currently the power domain with irq_safe devices is always powered on. With the patch it could be powered off (of course if the driver/mach code is irq-safe).

Quoted 22 lines

Genpd inherently provides support for devices, domain hierarchy and can be used to represent cpu clusters like in ARM's big.Little, where, each cpu cluster is in its domain, with supporting caches and other peripheral hardware. Multiple such domains could be part of another domain. Because mutexes are used to protect and synchronize domain operations and cpu idle operations are inherently atomic, the use of genpd is not possible for runtime suspend and resume of the pm domain. Replacing the locks to spinlocks would allow cpu domain to be be powered off to save power, when all the cpus are powered off.

However, not all domains can operate in irq-safe contexts and usually would need to sleep during domain operations. So genpd has to support both the cases, where the domain is or is not irq-safe. The irq-safe attribute is therefore domain specific.

To achieve domain specific locking, set the GENPD_FLAG_IRQ_SAFE flag while defining the domain. This determines if the domain should use a spinlock instead of a mutex. Locking is abstracted through genpd_lock_domain() and genpd_unlock_domain() functions that use the flag to determine the locking to be used for this domain.

Quoted 3 lines

The restriction this imposes on the domain hierarchy is that subdomains and all devices in the hierarchy also be irq-safe. Non irq-safe domains may continue to have irq-safe devices, but not the other way around.

So an irq-safe device can be put in irq-safe subdomain which can be a child of non-irq-safe topdomain?

Quoted 8 lines

Cc Ulf Hansson <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Kevin Hilman <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/base/power/domain.c | 200 ++++++++++++++++++++++++++++++++++----------
 include/linux/pm_domain.h   |  11 ++-

Documentation should also be reflected.

Quoted 69 lines

 2 files changed, 164 insertions(+), 47 deletions(-)

drivers/base/power/domain.c +59 -0

index dfd7595..8b89d15 100644
@@ -50,6 +50,71 @@
 static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
 
+static inline int genpd_lock_domain_noirq(struct generic_pm_domain *genpd,
+					unsigned int subclass)
+	__acquires(&genpd->slock)
+{
+	unsigned long flags;
+
+	if (unlikely(subclass > 0))
+		spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
+	else
+		spin_lock_irqsave(&genpd->slock, flags);
+
+	genpd->flags = flags;
+
+	return 0;
+}
+
+static inline int genpd_unlock_domain_noirq(struct generic_pm_domain *genpd)
+	__releases(&genpd->slock)
+{
+	spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
+	return 0;
+}
+
+static inline int genpd_lock_domain_irq(struct generic_pm_domain *genpd,
+					unsigned int subclass)
+	__acquires(&genpd->mlock)
+{
+	if (unlikely(subclass > 0))
+		mutex_lock_nested(&genpd->mlock, subclass);
+	else
+		mutex_lock(&genpd->mlock);
+
+	return 0;
+}
+
+static inline int genpd_lock_domain_interruptible_irq(
+				struct generic_pm_domain *genpd)
+	__acquires(&genpd->mlock)
+{
+	return mutex_lock_interruptible(&genpd->mlock);
+}
+
+static inline int genpd_unlock_domain_irq(struct generic_pm_domain *genpd)
+	__releases(&genpd->mlock)
+{
+	mutex_unlock(&genpd->mlock);
+	return 0;
+}
+
+#define genpd_lock_domain(genpd)				\
+	(genpd->irq_safe ? genpd_lock_domain_noirq(genpd, 0)	\
+			: genpd_lock_domain_irq(genpd, 0))
+
+#define genpd_lock_domain_nested(genpd)				\
+	(genpd->irq_safe ? genpd_lock_domain_noirq(genpd, SINGLE_DEPTH_NESTING)\
+			: genpd_lock_domain_irq(genpd, SINGLE_DEPTH_NESTING))
+
+#define genpd_unlock_domain(genpd)				\
+	(genpd->irq_safe ? genpd_unlock_domain_noirq(genpd)	\

drivers/base/power/domain.c line 116

Quoted 5 lines

drivers/base/power/domain.c +5 -0

+			: genpd_unlock_domain_irq(genpd))
+
+#define genpd_lock_domain_interruptible(genpd)			\
+	(genpd->irq_safe ? genpd_lock_domain_noirq(genpd, 0)	\
+			: genpd_lock_domain_interruptible_irq(genpd))

Why macros? You are not using here benefits of a macro and they are called just like ordinary functions.

You added "domain" prefix but genpd already contains this. genod_lock(), genpd_lock_nested() etc. should be sufficient, unless there is a conflict, similar name planned or you plan to lock something else (genpd_lock_device?).

Quoted 42 lines

drivers/base/power/domain.c +6 -5

+
 static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
 {
 	struct generic_pm_domain *genpd = NULL, *gpd;

@@ -262,9 +327,9 @@ int pm_genpd_poweron(struct generic_pm_domain *genpd)
 {
 	int ret;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock_domain(genpd);
 	ret = __pm_genpd_poweron(genpd);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock_domain(genpd);
 	return ret;
 }
 

@@ -326,9 +391,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_domain(genpd);
 			genpd->max_off_time_changed = true;
-			mutex_unlock(&genpd->lock);
+			genpd_unlock_domain(genpd);
 		}
 
 		dev = dev->parent;

@@ -387,7 +452,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
 			return -EBUSY;
 
 		if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
-		    || pdd->dev->power.irq_safe))
+			|| (pdd->dev->power.irq_safe && !genpd->irq_safe)))
 			not_suspended++;
 	}
 

@@ -453,9 +518,9 @@ static void genpd_power_off_work_fn(struct work_struct *work)
 
 	genpd = container_of(work, struct generic_pm_domain, power_off_work);
 

drivers/base/power/domain.c static void genpd_power_off_work_fn(struct work_struct *work) · line 522

Quoted 3 lines

drivers/base/power/domain.c +1 -1

-	mutex_lock(&genpd->lock);
+	genpd_lock_domain(genpd);
 	pm_genpd_poweroff(genpd);

Ipm_genpd_poweroff() calls __pm_genpd_save_device() which grabs mutex. At least in next-20150604 but maybe the patches, which this depends on, changed it?

Quoted 116 lines

drivers/base/power/domain.c +26 -19

-	mutex_unlock(&genpd->lock);
+	genpd_unlock_domain(genpd);
 }
 
 /**

@@ -478,12 +543,8 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 	if (IS_ERR(genpd))
 		return -EINVAL;
 
-	/*
-	 * We can't allow to power off the PM domain if it holds an irq_safe
-	 * device. That's beacuse we use mutexes to protect data while power
-	 * off and on the PM domain, thus we can't execute in atomic context.
-	 */
-	if (dev->power.irq_safe)
+	/* We can't allow to power off a domain that is also not irq safe. */
+	if (dev->power.irq_safe && !genpd->irq_safe)
 		return -EBUSY;
 
 	stop_ok = genpd->gov ? genpd->gov->stop_ok : NULL;

@@ -500,11 +561,19 @@ static int pm_genpd_runtime_suspend(struct device *dev)
 		return ret;
 	}
 
-	mutex_lock(&genpd->lock);
+	/*
+	 * If power.irq_safe is set, this routine will be run with interrupts
+	 * off, so suspend only if the power domain is irq_safe.
+	 */
+	if (dev->power.irq_safe && !genpd->irq_safe)
+		return 0;
+
+	genpd_lock_domain(genpd);
+
 	genpd->in_progress++;
 	pm_genpd_poweroff(genpd);
 	genpd->in_progress--;
-	mutex_unlock(&genpd->lock);
+	genpd_unlock_domain(genpd);
 
 	return 0;
 }

@@ -528,13 +597,16 @@ 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)
+	/*
+	 * If power.irq_safe and domain is not, then
+	 * the PM domain is never powered off.
+	 */
+	if (dev->power.irq_safe && !genpd->irq_safe)
 		return genpd_start_dev_no_timing(genpd, dev);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock_domain(genpd);
 	ret = __pm_genpd_poweron(genpd);
-	mutex_unlock(&genpd->lock);
+	genpd_unlock_domain(genpd);
 
 	if (ret)
 		return ret;

@@ -729,14 +801,14 @@ static int pm_genpd_prepare(struct device *dev)
 	if (resume_needed(dev, genpd))
 		pm_runtime_resume(dev);
 
-	mutex_lock(&genpd->lock);
+	genpd_lock_domain(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_domain(genpd);
 
 	if (genpd->suspend_power_off) {
 		pm_runtime_put_noidle(dev);

@@ -754,12 +826,12 @@ static int pm_genpd_prepare(struct device *dev)
 
 	ret = pm_generic_prepare(dev);
 	if (ret) {
-		mutex_lock(&genpd->lock);
+		genpd_lock_domain(genpd);
 
 		if (--genpd->prepared_count == 0)
 			genpd->suspend_power_off = false;
 
-		mutex_unlock(&genpd->lock);
+		genpd_unlock_domain(genpd);
 		pm_runtime_enable(dev);
 	}
 

@@ -1116,13 +1188,13 @@ static void pm_genpd_complete(struct device *dev)
 	if (IS_ERR(genpd))
 		return;
 
-	mutex_lock(&genpd->lock);
+	genpd_lock_domain(genpd);
 
 	run_complete = !genpd->suspend_power_off;
 	if (--genpd->prepared_count == 0)
 		genpd->suspend_power_off = false;
 
-	mutex_unlock(&genpd->lock);
+	genpd_unlock_domain(genpd);
 
 	if (run_complete) {
 		pm_generic_complete(dev);

@@ -1266,11 +1338,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;
 

drivers/base/power/domain.c int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, · line 1341

Quoted 1 line

drivers/base/power/domain.c +1 -0

+	/* Devices in an IRQ safe PM Domain have to be irq safe too */

Why? Can you add this information here? Previously there was a reason in case of irq_safe devices which you removed leaving only policy.

Quoted 10 lines · Krzysztof Kozlowski

W dniu 05.06.2015 o 07:29, Lina Iyer pisze:

Quoted 4 lines

Power Domains currently support turning on/off only in process context. This restricts the usage of PM domains to devices and domains that could be powered on/off in irq disabled contexts as the mutexes used in GenPD allows for cpu sleep while waiting for locks.

I can find also other use case: currently the power domain with irq_safe devices is always powered on. With the patch it could be powered off (of course if the driver/mach code is irq-safe).

Yes, absolutely.

Quoted 26 lines

Quoted 25 lines

Genpd inherently provides support for devices, domain hierarchy and can be used to represent cpu clusters like in ARM's big.Little, where, each cpu cluster is in its domain, with supporting caches and other peripheral hardware. Multiple such domains could be part of another domain. Because mutexes are used to protect and synchronize domain operations and cpu idle operations are inherently atomic, the use of genpd is not possible for runtime suspend and resume of the pm domain. Replacing the locks to spinlocks would allow cpu domain to be be powered off to save power, when all the cpus are powered off.

However, not all domains can operate in irq-safe contexts and usually would need to sleep during domain operations. So genpd has to support both the cases, where the domain is or is not irq-safe. The irq-safe attribute is therefore domain specific.

To achieve domain specific locking, set the GENPD_FLAG_IRQ_SAFE flag while defining the domain. This determines if the domain should use a spinlock instead of a mutex. Locking is abstracted through genpd_lock_domain() and genpd_unlock_domain() functions that use the flag to determine the locking to be used for this domain.

The restriction this imposes on the domain hierarchy is that subdomains and all devices in the hierarchy also be irq-safe. Non irq-safe domains may continue to have irq-safe devices, but not the other way around.

Quoted 3 lines

So an irq-safe device can be put in irq-safe subdomain which can be a child of non-irq-safe topdomain?

Yes, the container need not be irq-safe but the contained need to be irqsafe.

Quoted 11 lines

Quoted 10 lines

Cc Ulf Hansson <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Kevin Hilman <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/base/power/domain.c | 200
++++++++++++++++++++++++++++++++++----------
 include/linux/pm_domain.h   |  11 ++-

Quoted 2 lines

Documentation should also be reflected.

Yes, will add.

Quoted 74 lines

Quoted 74 lines

 2 files changed, 164 insertions(+), 47 deletions(-)

drivers/base/power/domain.c +43 -0

index dfd7595..8b89d15 100644
@@ -50,6 +50,71 @@
 static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
 
+static inline int genpd_lock_domain_noirq(struct generic_pm_domain
 *genpd,
+                    unsigned int subclass)
+    __acquires(&genpd->slock)
+{
+    unsigned long flags;
+
+    if (unlikely(subclass > 0))
+        spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
+    else
+        spin_lock_irqsave(&genpd->slock, flags);
+
+    genpd->flags = flags;
+
+    return 0;
+}
+
+static inline int genpd_unlock_domain_noirq(struct generic_pm_domain
 *genpd)
+    __releases(&genpd->slock)
+{
+    spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
+    return 0;
+}
+
+static inline int genpd_lock_domain_irq(struct generic_pm_domain
 *genpd,
+                    unsigned int subclass)
+    __acquires(&genpd->mlock)
+{
+    if (unlikely(subclass > 0))
+        mutex_lock_nested(&genpd->mlock, subclass);
+    else
+        mutex_lock(&genpd->mlock);
+
+    return 0;
+}
+
+static inline int genpd_lock_domain_interruptible_irq(
+                struct generic_pm_domain *genpd)
+    __acquires(&genpd->mlock)
+{
+    return mutex_lock_interruptible(&genpd->mlock);
+}
+
+static inline int genpd_unlock_domain_irq(struct generic_pm_domain

*genpd)

drivers/base/power/domain.c +12 -0

+    __releases(&genpd->mlock)
+{
+    mutex_unlock(&genpd->mlock);
+    return 0;
+}
+
+#define genpd_lock_domain(genpd)                \
+    (genpd->irq_safe ? genpd_lock_domain_noirq(genpd, 0)    \
+            : genpd_lock_domain_irq(genpd, 0))
+
+#define genpd_lock_domain_nested(genpd)                \
+    (genpd->irq_safe ? genpd_lock_domain_noirq(genpd,

SINGLE_DEPTH_NESTING)\

drivers/base/power/domain.c +4 -0

+            : genpd_lock_domain_irq(genpd, SINGLE_DEPTH_NESTING))
+
+#define genpd_unlock_domain(genpd)                \
+    (genpd->irq_safe ? genpd_unlock_domain_noirq(genpd)    \

Quoted 9 lines

drivers/base/power/domain.c line 116

Quoted 5 lines

drivers/base/power/domain.c +5 -0

+            : genpd_unlock_domain_irq(genpd))
+
+#define genpd_lock_domain_interruptible(genpd)            \
+    (genpd->irq_safe ? genpd_lock_domain_noirq(genpd, 0)    \
+            : genpd_lock_domain_interruptible_irq(genpd))

Why macros? You are not using here benefits of a macro and they are called just like ordinary functions.

Well, I didnt see a need for a function that might show up in the stack. But I have no strong preference either way.

Quoted 6 lines

You added "domain" prefix but genpd already contains this. genod_lock(), genpd_lock_nested() etc. should be sufficient, unless there is a conflict, similar name planned or you plan to lock something else (genpd_lock_device?).

Sigh. Yes, you are right. Its redundant. Will remove.

Quoted 48 lines

Quoted 48 lines

+
static struct generic_pm_domain *pm_genpd_lookup_name(const char
*domain_name)

 {
     struct generic_pm_domain *genpd = NULL, *gpd;

drivers/base/power/domain.c +4 -4

@@ -262,9 +327,9 @@ int pm_genpd_poweron(struct generic_pm_domain
 *genpd)
 {
     int ret;
 
-    mutex_lock(&genpd->lock);
+    genpd_lock_domain(genpd);
     ret = __pm_genpd_poweron(genpd);
-    mutex_unlock(&genpd->lock);
+    genpd_unlock_domain(genpd);
     return ret;
 }

@@ -326,9 +391,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_domain(genpd);
             genpd->max_off_time_changed = true;
-            mutex_unlock(&genpd->lock);
+            genpd_unlock_domain(genpd);
         }
 

         dev = dev->parent;

drivers/base/power/domain.c +1 -1

@@ -387,7 +452,7 @@ static int pm_genpd_poweroff(struct
 generic_pm_domain *genpd)
             return -EBUSY;
 
         if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
-            || pdd->dev->power.irq_safe))
+            || (pdd->dev->power.irq_safe && !genpd->irq_safe)))
             not_suspended++;
     }

@@ -453,9 +518,9 @@ static void genpd_power_off_work_fn(struct
 work_struct *work)
 
     genpd = container_of(work, struct generic_pm_domain,
 power_off_work);
 

Quoted 8 lines

drivers/base/power/domain.c static void genpd_power_off_work_fn(struct work_struct *work) · line 522

Quoted 3 lines

drivers/base/power/domain.c +1 -1

-    mutex_lock(&genpd->lock);
+    genpd_lock_domain(genpd);
     pm_genpd_poweroff(genpd);

Ipm_genpd_poweroff() calls __pm_genpd_save_device() which grabs mutex. At least in next-20150604 but maybe the patches, which this depends on, changed it?

Yes. Ulf's patch remvoed that call.

Quoted 128 lines

Quoted 128 lines

drivers/base/power/domain.c +1 -1

-    mutex_unlock(&genpd->lock);
+    genpd_unlock_domain(genpd);
 }

/**

drivers/base/power/domain.c +0 -2

@@ -478,12 +543,8 @@ static int pm_genpd_runtime_suspend(struct
 device *dev)
     if (IS_ERR(genpd))
         return -EINVAL;
 
-    /*
-     * We can't allow to power off the PM domain if it holds an

irq_safe
- * device. That's beacuse we use mutexes to protect data while
power
- * off and on the PM domain, thus we can't execute in atomic
context.

drivers/base/power/domain.c +1 -2

-     */
-    if (dev->power.irq_safe)
+    /* We can't allow to power off a domain that is also not irq

safe. */

drivers/base/power/domain.c +3 -1

+    if (dev->power.irq_safe && !genpd->irq_safe)
         return -EBUSY;
 
     stop_ok = genpd->gov ? genpd->gov->stop_ok : NULL;

@@ -500,11 +561,19 @@ static int pm_genpd_runtime_suspend(struct
 device *dev)
         return ret;
     }
 
-    mutex_lock(&genpd->lock);
+    /*
+     * If power.irq_safe is set, this routine will be run with

interrupts

drivers/base/power/domain.c +15 -5

+     * off, so suspend only if the power domain is irq_safe.
+     */
+    if (dev->power.irq_safe && !genpd->irq_safe)
+        return 0;
+
+    genpd_lock_domain(genpd);
+
     genpd->in_progress++;
     pm_genpd_poweroff(genpd);
     genpd->in_progress--;
-    mutex_unlock(&genpd->lock);
+    genpd_unlock_domain(genpd);
 
     return 0;
 }

@@ -528,13 +597,16 @@ 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)
+    /*
+     * If power.irq_safe and domain is not, then
+     * the PM domain is never powered off.
+     */
+    if (dev->power.irq_safe && !genpd->irq_safe)
         return genpd_start_dev_no_timing(genpd, dev);
 
-    mutex_lock(&genpd->lock);
+    genpd_lock_domain(genpd);
     ret = __pm_genpd_poweron(genpd);
-    mutex_unlock(&genpd->lock);
+    genpd_unlock_domain(genpd);
 
     if (ret)

         return ret;

drivers/base/power/domain.c +2 -2

@@ -729,14 +801,14 @@ static int pm_genpd_prepare(struct device *dev)
     if (resume_needed(dev, genpd))
         pm_runtime_resume(dev);
 
-    mutex_lock(&genpd->lock);
+    genpd_lock_domain(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_domain(genpd);
 
     if (genpd->suspend_power_off) {

         pm_runtime_put_noidle(dev);

drivers/base/power/domain.c +4 -4

@@ -754,12 +826,12 @@ static int pm_genpd_prepare(struct device *dev)
 
     ret = pm_generic_prepare(dev);
     if (ret) {
-        mutex_lock(&genpd->lock);
+        genpd_lock_domain(genpd);
 
         if (--genpd->prepared_count == 0)
             genpd->suspend_power_off = false;
 
-        mutex_unlock(&genpd->lock);
+        genpd_unlock_domain(genpd);
         pm_runtime_enable(dev);
     }
 

@@ -1116,13 +1188,13 @@ static void pm_genpd_complete(struct device
 *dev)
     if (IS_ERR(genpd))
         return;
 
-    mutex_lock(&genpd->lock);
+    genpd_lock_domain(genpd);
 
     run_complete = !genpd->suspend_power_off;
     if (--genpd->prepared_count == 0)
         genpd->suspend_power_off = false;
 
-    mutex_unlock(&genpd->lock);
+    genpd_unlock_domain(genpd);
 
     if (run_complete) {

         pm_generic_complete(dev);

drivers/base/power/domain.c +0 -0

@@ -1266,11 +1338,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;
 

Quoted 5 lines

drivers/base/power/domain.c int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, · line 1341

Quoted 1 line

drivers/base/power/domain.c +1 -0

+    /* Devices in an IRQ safe PM Domain have to be irq safe too */

Why? Can you add this information here? Previously there was a reason in case of irq_safe devices which you removed leaving only policy.

Sorry, your question is not clear to me.
I believe this is a new requirement that enforces the contained devices of an irq-safe domain to be irq-safe as well.

Quoted 2 lines · Lina Iyer

Quoted 1 line · Krzysztof Kozlowski

W dniu 05.06.2015 o 07:29, Lina Iyer pisze:

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/base/power/domain.c +0 -0

@@ -1266,11 +1338,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;
 

Quoted 8 lines

Quoted 5 lines

drivers/base/power/domain.c int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, · line 1341

Quoted 1 line

drivers/base/power/domain.c +1 -0

+    /* Devices in an IRQ safe PM Domain have to be irq safe too */

Why? Can you add this information here? Previously there was a reason in case of irq_safe devices which you removed leaving only policy.

Sorry, your question is not clear to me.
I believe this is a new requirement that enforces the contained devices of an irq-safe domain to be irq-safe as well.

What I wanted to say is that it would be nice if comment explained why domain have to be IRQ safe too. Without this "WHY" answer the comment is quite redundant - the "if" statement is obvious. But the "WHY" is not such obvious.

Previous comments in few places mentioned the answer:
/*
* We can't allow to power off the PM domain if it holds an irq_safe
* device. That's beacuse we use mutexes to protect data while power
* off and on the PM domain, thus we can't execute in atomic context. */

Power Domains currently support turning on/off only in process context.

This restricts the usage of PM domains to devices and domains that could be powered on/off in irq disabled contexts as the mutexes used in GenPD allows for cpu sleep while waiting for locks.

Genpd inherently provides support for devices, domain hierarchy and can

be used to represent cpu clusters like in ARM's big.Little, where, each cpu cluster is in its domain, with supporting caches and other peripheral hardware.

Multiple such domains could be part of another domain.

Because mutexes are used to protect and synchronize domain

operations and cpu idle operations are inherently atomic,

the use of
genpd is not possible for runtime suspend and resume of the pm domain.

Replacing the locks to spinlocks would allow cpu domain to be be powered

off to save power, when all the cpus are powered off.

However, not all domains can operate in irq-safe contexts and usually would need to sleep during domain operations. So genpd has to support both the cases, where the domain is or is not irq-safe. The irq-safe attribute is therefore domain specific.

To achieve domain specific locking, set the GENPD_FLAG_IRQ_SAFE flag while defining the domain. This determines if the domain should use a spinlock instead of a mutex. Locking is abstracted through genpd_lock_domain() and genpd_unlock_domain() functions that use the flag to determine the locking to be used for this domain.

The restriction this imposes on the domain hierarchy is that subdomains and all devices in the hierarchy also be irq-safe. Non irq-safe domains may continue to have irq-safe devices, but not the other way around.

Quoted 1 line · Lina Iyer

Power Domains currently support turning on/off only in process context.

Generic Power Domains...

Also Re: $SUBJECT. s/atomic/IRQ safe/

Quoted 3 lines

This restricts the usage of PM domains to devices and domains that could be powered on/off in irq disabled contexts as the mutexes used in GenPD allows for cpu sleep while waiting for locks.

That sentence reads the opposite of what you mean. Rather than "This restricts X to Y", I think you menant "This prevents X for Y".

Quoted 1 line

Genpd inherently provides support for devices, domain hierarchy and can

s/domain heirarchy/nesting/

Quoted 3 lines

be used to represent cpu clusters like in ARM's big.Little, where, each cpu cluster is in its domain, with supporting caches and other peripheral hardware.

s/domain/power domain/

Quoted 1 line

Multiple such domains could be part of another domain.

OK, but not important to this change IMO.

Quoted 1 line

Because mutexes are used to protect and synchronize domain

s/domain/genpd/

Quoted 1 line

operations and cpu idle operations are inherently atomic,

Be more specific about "CPU idle operations are inherently atomic", as it's not obvious that it's true. I think what you mean is that the CPUidle paths for entering of low-power idle states is inherently atomic because interrupts are disabled.

Quoted 2 lines

the use of
genpd is not possible for runtime suspend and resume of the pm domain.

... so, the use of genpd during the idle path of CPUs is not currently possible because interrups are disabled in the idle path.

Quoted 1 line

Replacing the locks to spinlocks would allow cpu domain to be be powered

                    ^^^^

s/to/with/

Quoted 1 line

off to save power, when all the cpus are powered off.

More accuratly, replacing the locks doesn't allow the domain to be powered off, rather it allows genpd to be used in the idle path, which would allow genpd to be used

Quoted 11 lines

However, not all domains can operate in irq-safe contexts and usually would need to sleep during domain operations. So genpd has to support both the cases, where the domain is or is not irq-safe. The irq-safe attribute is therefore domain specific.

To achieve domain specific locking, set the GENPD_FLAG_IRQ_SAFE flag while defining the domain. This determines if the domain should use a spinlock instead of a mutex. Locking is abstracted through genpd_lock_domain() and genpd_unlock_domain() functions that use the flag to determine the locking to be used for this domain.

Quoted 3 lines

The restriction this imposes on the domain hierarchy is that subdomains and all devices in the hierarchy also be irq-safe. Non irq-safe domains may continue to have irq-safe devices, but not the other way around.

This might need some corresponding updates to Documentation/ as well.

Power Domains currently support turning on/off only in process context. This restricts the usage of PM domains to devices and domains that could be powered on/off in irq disabled contexts as the mutexes used in GenPD allows for cpu sleep while waiting for locks.

Genpd inherently provides support for devices, domain hierarchy and can be used to represent cpu clusters like in ARM's big.Little, where, each cpu cluster is in its domain, with supporting caches and other peripheral hardware. Multiple such domains could be part of another domain. Because mutexes are used to protect and synchronize domain operations and cpu idle operations are inherently atomic, the use of genpd is not possible for runtime suspend and resume of the pm domain. Replacing the locks to spinlocks would allow cpu domain to be be powered off to save power, when all the cpus are powered off.

However, not all domains can operate in irq-safe contexts and usually would need to sleep during domain operations. So genpd has to support both the cases, where the domain is or is not irq-safe. The irq-safe attribute is therefore domain specific.

To achieve domain specific locking, set the GENPD_FLAG_IRQ_SAFE flag while defining the domain. This determines if the domain should use a spinlock instead of a mutex. Locking is abstracted through genpd_lock_domain() and genpd_unlock_domain() functions that use the flag to determine the locking to be used for this domain.

The restriction this imposes on the domain hierarchy is that subdomains and all devices in the hierarchy also be irq-safe. Non irq-safe domains may continue to have irq-safe devices, but not the other way around.

Cc Ulf Hansson <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Kevin Hilman <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/base/power/domain.c | 200 ++++++++++++++++++++++++++++++++++----------
 include/linux/pm_domain.h   |  11 ++-
 2 files changed, 164 insertions(+), 47 deletions(-)

drivers/base/power/domain.c +0 -0

index dfd7595..8b89d15 100644

drivers/base/power/domain.c +42 -0

+
+       return 0;
+}
+
+static inline int genpd_unlock_domain_noirq(struct generic_pm_domain *genpd)
+       __releases(&genpd->slock)
+{
+       spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
+       return 0;
+}
+
+static inline int genpd_lock_domain_irq(struct generic_pm_domain *genpd,
+                                       unsigned int subclass)
+       __acquires(&genpd->mlock)
+{
+       if (unlikely(subclass > 0))
+               mutex_lock_nested(&genpd->mlock, subclass);
+       else
+               mutex_lock(&genpd->mlock);
+
+       return 0;
+}
+
+static inline int genpd_lock_domain_interruptible_irq(
+                               struct generic_pm_domain *genpd)
+       __acquires(&genpd->mlock)
+{
+       return mutex_lock_interruptible(&genpd->mlock);
+}
+
+static inline int genpd_unlock_domain_irq(struct generic_pm_domain *genpd)
+       __releases(&genpd->mlock)
+{
+       mutex_unlock(&genpd->mlock);
+       return 0;
+}
+
+#define genpd_lock_domain(genpd)                               \
+       (genpd->irq_safe ? genpd_lock_domain_noirq(genpd, 0)    \
+                       : genpd_lock_domain_irq(genpd, 0))
+
+#define genpd_lock_domain_nested(genpd)                                \

drivers/base/power/domain.c +5 -4

+
 static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
 {
        struct generic_pm_domain *genpd = NULL, *gpd;

@@ -262,9 +327,9 @@ int pm_genpd_poweron(struct generic_pm_domain *genpd)
 {
        int ret;
 
-       mutex_lock(&genpd->lock);
+       genpd_lock_domain(genpd);
        ret = __pm_genpd_poweron(genpd);
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
        return ret;
 }
 

@@ -326,9 +391,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_domain(genpd);
                        genpd->max_off_time_changed = true;
-                       mutex_unlock(&genpd->lock);
+                       genpd_unlock_domain(genpd);
                }
 
                dev = dev->parent;

drivers/base/power/domain.c +2 -2

                        not_suspended++;
        }
 

@@ -453,9 +518,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_domain(genpd);
        pm_genpd_poweroff(genpd);
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
 }
 
 /**

drivers/base/power/domain.c +0 -0

                return -EBUSY;
 
        stop_ok = genpd->gov ? genpd->gov->stop_ok : NULL;

drivers/base/power/domain.c +4 -1

+
+       genpd_lock_domain(genpd);
+
        genpd->in_progress++;
        pm_genpd_poweroff(genpd);
        genpd->in_progress--;
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
 
        return 0;
 }

drivers/base/power/domain.c +11 -8

+        * the PM domain is never powered off.
+        */
+       if (dev->power.irq_safe && !genpd->irq_safe)
                return genpd_start_dev_no_timing(genpd, dev);
 
-       mutex_lock(&genpd->lock);
+       genpd_lock_domain(genpd);
        ret = __pm_genpd_poweron(genpd);
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
 
        if (ret)
                return ret;

@@ -729,14 +801,14 @@ static int pm_genpd_prepare(struct device *dev)
        if (resume_needed(dev, genpd))
                pm_runtime_resume(dev);
 
-       mutex_lock(&genpd->lock);
+       genpd_lock_domain(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_domain(genpd);
 
        if (genpd->suspend_power_off) {
                pm_runtime_put_noidle(dev);

@@ -754,12 +826,12 @@ static int pm_genpd_prepare(struct device *dev)
 
        ret = pm_generic_prepare(dev);
        if (ret) {
-               mutex_lock(&genpd->lock);
+               genpd_lock_domain(genpd);
 
                if (--genpd->prepared_count == 0)
                        genpd->suspend_power_off = false;
 
-               mutex_unlock(&genpd->lock);
+               genpd_unlock_domain(genpd);
                pm_runtime_enable(dev);
        }
 

@@ -1116,13 +1188,13 @@ static void pm_genpd_complete(struct device *dev)
        if (IS_ERR(genpd))
                return;
 
-       mutex_lock(&genpd->lock);
+       genpd_lock_domain(genpd);
 
        run_complete = !genpd->suspend_power_off;
        if (--genpd->prepared_count == 0)
                genpd->suspend_power_off = false;
 
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
 
        if (run_complete) {
                pm_generic_complete(dev);

drivers/base/power/domain.c +9 -5

+                       "Devices in an irq-safe domain have to be irq safe.\n");
+               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_domain(genpd);
 
        if (genpd->prepared_count > 0) {
                ret = -EAGAIN;

@@ -1287,7 +1366,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_domain(genpd);
 
        if (ret)
                genpd_free_dev_data(dev, gpd_data);

@@ -1331,7 +1410,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_domain(genpd);
 
        if (genpd->prepared_count > 0) {
                ret = -EAGAIN;

@@ -1346,14 +1425,14 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
        list_del_init(&pdd->list_node);
 
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
 
        genpd_free_dev_data(dev, gpd_data);
 
        return 0;
 
  out:
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
        dev_pm_qos_add_notifier(dev, &gpd_data->nb);
 
        return ret;

drivers/base/power/domain.c +11 -8

+               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_domain(genpd);
+       genpd_lock_domain_nested(subdomain);
 
        if (genpd->status == GPD_STATE_POWER_OFF
            &&  subdomain->status != GPD_STATE_POWER_OFF) {

@@ -1402,8 +1492,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_domain(subdomain);
+       genpd_unlock_domain(genpd);
 
        return ret;
 }

@@ -1451,13 +1541,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_domain(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_domain_nested(subdomain);
 
                list_del(&link->master_node);
                list_del(&link->slave_node);

@@ -1465,13 +1555,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_domain(subdomain);
 
                ret = 0;
                break;
        }
 
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
 
        return ret;
 }

drivers/base/power/domain.c +7 -4

+               return -EINVAL;
+       }
+
        cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
        if (!cpuidle_data)
                return -ENOMEM;
 
-       mutex_lock(&genpd->lock);
+       genpd_lock_domain(genpd);
 
        if (genpd->cpuidle_data) {
                ret = -EEXIST;

@@ -1526,7 +1622,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_domain(genpd);
        return ret;
 
  err:

@@ -1563,7 +1659,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_domain(genpd);
 
        cpuidle_data = genpd->cpuidle_data;
        if (!cpuidle_data) {

@@ -1581,7 +1677,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
        kfree(cpuidle_data);
 
  out:
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
        return ret;
 }
 

drivers/base/power/domain.c +11 -4

+       } 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.

@@ -1657,7 +1764,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_domain_init(genpd);
        genpd->gov = gov;
        INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
        genpd->in_progress = 0;

@@ -2042,7 +2149,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_domain_interruptible(genpd);
        if (ret)
                return -ERESTARTSYS;
 

@@ -2062,7 +2169,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;
 

@@ -2073,7 +2181,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
 
        seq_puts(s, "\n");
 exit:
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
 
        return 0;
 }

include/linux/pm_domain.h +10 -2

index b2725e6..dc7cb53 100644
@@ -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

Quoted 47 lines · Lina Iyer

Power Domains currently support turning on/off only in process context. This restricts the usage of PM domains to devices and domains that could be powered on/off in irq disabled contexts as the mutexes used in GenPD allows for cpu sleep while waiting for locks.

Genpd inherently provides support for devices, domain hierarchy and can be used to represent cpu clusters like in ARM's big.Little, where, each cpu cluster is in its domain, with supporting caches and other peripheral hardware. Multiple such domains could be part of another domain. Because mutexes are used to protect and synchronize domain operations and cpu idle operations are inherently atomic, the use of genpd is not possible for runtime suspend and resume of the pm domain. Replacing the locks to spinlocks would allow cpu domain to be be powered off to save power, when all the cpus are powered off.

However, not all domains can operate in irq-safe contexts and usually would need to sleep during domain operations. So genpd has to support both the cases, where the domain is or is not irq-safe. The irq-safe attribute is therefore domain specific.

To achieve domain specific locking, set the GENPD_FLAG_IRQ_SAFE flag while defining the domain. This determines if the domain should use a spinlock instead of a mutex. Locking is abstracted through genpd_lock_domain() and genpd_unlock_domain() functions that use the flag to determine the locking to be used for this domain.

The restriction this imposes on the domain hierarchy is that subdomains and all devices in the hierarchy also be irq-safe. Non irq-safe domains may continue to have irq-safe devices, but not the other way around.

Cc Ulf Hansson <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Kevin Hilman <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---
 drivers/base/power/domain.c | 200 ++++++++++++++++++++++++++++++++++----------
 include/linux/pm_domain.h   |  11 ++-
 2 files changed, 164 insertions(+), 47 deletions(-)

drivers/base/power/domain.c +0 -0

index dfd7595..8b89d15 100644
@@ -50,6 +50,71 @@
 static LIST_HEAD(gpd_list);
 static DEFINE_MUTEX(gpd_list_lock);
 

drivers/base/power/domain.c line 53

Quoted 1 line

drivers/base/power/domain.c +1 -0

+static inline int genpd_lock_domain_noirq(struct generic_pm_domain *genpd,

Nitpick:

I would prefer a bit shorter names of all these new locking functions. In general, I think you can remove "domain" from the names. In this case it would mean genpd_lock_noirq().

Quoted 5 lines

drivers/base/power/domain.c +5 -0

+                                       unsigned int subclass)
+       __acquires(&genpd->slock)
+{
+       unsigned long flags;
+

drivers/base/power/domain.c line 64

Quoted 6 lines

drivers/base/power/domain.c +6 -0

+       if (unlikely(subclass > 0))
+               spin_lock_irqsave_nested(&genpd->slock, flags, subclass);
+       else
+               spin_lock_irqsave(&genpd->slock, flags);
+
+       genpd->flags = flags;

This is wrong, it should be genpd->lock_flags that you assign.

BTW, can't you assign the genpd->lock_flags immediately when call spin_lock_irqsave*(), instead of keeping a local copy?

Quoted 42 lines

drivers/base/power/domain.c +42 -0

+
+       return 0;
+}
+
+static inline int genpd_unlock_domain_noirq(struct generic_pm_domain *genpd)
+       __releases(&genpd->slock)
+{
+       spin_unlock_irqrestore(&genpd->slock, genpd->lock_flags);
+       return 0;
+}
+
+static inline int genpd_lock_domain_irq(struct generic_pm_domain *genpd,
+                                       unsigned int subclass)
+       __acquires(&genpd->mlock)
+{
+       if (unlikely(subclass > 0))
+               mutex_lock_nested(&genpd->mlock, subclass);
+       else
+               mutex_lock(&genpd->mlock);
+
+       return 0;
+}
+
+static inline int genpd_lock_domain_interruptible_irq(
+                               struct generic_pm_domain *genpd)
+       __acquires(&genpd->mlock)
+{
+       return mutex_lock_interruptible(&genpd->mlock);
+}
+
+static inline int genpd_unlock_domain_irq(struct generic_pm_domain *genpd)
+       __releases(&genpd->mlock)
+{
+       mutex_unlock(&genpd->mlock);
+       return 0;
+}
+
+#define genpd_lock_domain(genpd)                               \
+       (genpd->irq_safe ? genpd_lock_domain_noirq(genpd, 0)    \
+                       : genpd_lock_domain_irq(genpd, 0))
+
+#define genpd_lock_domain_nested(genpd)                                \

drivers/base/power/domain.c line 116

Quoted 10 lines

drivers/base/power/domain.c +10 -0

+       (genpd->irq_safe ? genpd_lock_domain_noirq(genpd, SINGLE_DEPTH_NESTING)\
+                       : genpd_lock_domain_irq(genpd, SINGLE_DEPTH_NESTING))
+
+#define genpd_unlock_domain(genpd)                             \
+       (genpd->irq_safe ? genpd_unlock_domain_noirq(genpd)     \
+                       : genpd_unlock_domain_irq(genpd))
+
+#define genpd_lock_domain_interruptible(genpd)                 \
+       (genpd->irq_safe ? genpd_lock_domain_noirq(genpd, 0)    \
+                       : genpd_lock_domain_interruptible_irq(genpd))

In general I don't like macros (as Krzysztof).

In this case I also don't see the benfit, could you consider to convert to in-line functions instead?

Quoted 31 lines

drivers/base/power/domain.c +5 -4

+
 static struct generic_pm_domain *pm_genpd_lookup_name(const char *domain_name)
 {
        struct generic_pm_domain *genpd = NULL, *gpd;

@@ -262,9 +327,9 @@ int pm_genpd_poweron(struct generic_pm_domain *genpd)
 {
        int ret;
 
-       mutex_lock(&genpd->lock);
+       genpd_lock_domain(genpd);
        ret = __pm_genpd_poweron(genpd);
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
        return ret;
 }
 

@@ -326,9 +391,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_domain(genpd);
                        genpd->max_off_time_changed = true;
-                       mutex_unlock(&genpd->lock);
+                       genpd_unlock_domain(genpd);
                }
 
                dev = dev->parent;

@@ -387,7 +452,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd)
                        return -EBUSY;
 

drivers/base/power/domain.c static int pm_genpd_poweroff(struct generic_pm_domain *genpd) · line 455

Quoted 3 lines

drivers/base/power/domain.c +1 -1

                if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
-                   || pdd->dev->power.irq_safe))
+                       || (pdd->dev->power.irq_safe && !genpd->irq_safe)))

This deserves a comment in the code.

Quoted 16 lines

drivers/base/power/domain.c +2 -2

                        not_suspended++;
        }
 

@@ -453,9 +518,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_domain(genpd);
        pm_genpd_poweroff(genpd);
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
 }
 
 /**

drivers/base/power/domain.c static int pm_genpd_runtime_suspend(struct device *dev) · line 546

Quoted 10 lines

drivers/base/power/domain.c +1 -6

@@ -478,12 +543,8 @@ static int pm_genpd_runtime_suspend(struct device *dev)
        if (IS_ERR(genpd))
                return -EINVAL;
 
-       /*
-        * We can't allow to power off the PM domain if it holds an irq_safe
-        * device. That's beacuse we use mutexes to protect data while power
-        * off and on the PM domain, thus we can't execute in atomic context.
-        */
-       if (dev->power.irq_safe)
+       /* We can't allow to power off a domain that is also not irq safe. */

I wouldn't mind to keep some more pieces of the earlier comment, since it explains a bit more of the *why*. Could you try to rephrase this comment in that regard?

drivers/base/power/domain.c static int pm_genpd_runtime_suspend(struct device *dev) · line 547

Quoted 1 line

drivers/base/power/domain.c +1 -0

+       if (dev->power.irq_safe && !genpd->irq_safe)

This looks correct...

Quoted 7 lines

drivers/base/power/domain.c +0 -0

                return -EBUSY;
 
        stop_ok = genpd->gov ? genpd->gov->stop_ok : NULL;

@@ -500,11 +561,19 @@ static int pm_genpd_runtime_suspend(struct device *dev)
                return ret;
        }
 

drivers/base/power/domain.c static int pm_genpd_runtime_suspend(struct device *dev) · line 569

Quoted 7 lines

drivers/base/power/domain.c +6 -1

-       mutex_lock(&genpd->lock);
+       /*
+        * If power.irq_safe is set, this routine will be run with interrupts
+        * off, so suspend only if the power domain is irq_safe.
+        */
+       if (dev->power.irq_safe && !genpd->irq_safe)
+               return 0;

... but this doesn't. You have already returned -EBUSY above for this case.

Quoted 12 lines

drivers/base/power/domain.c +4 -1

+
+       genpd_lock_domain(genpd);
+
        genpd->in_progress++;
        pm_genpd_poweroff(genpd);
        genpd->in_progress--;
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
 
        return 0;
 }

drivers/base/power/domain.c static int pm_genpd_runtime_resume(struct device *dev) · line 601

Quoted 7 lines

drivers/base/power/domain.c +2 -2

@@ -528,13 +597,16 @@ 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)
+       /*
+        * If power.irq_safe and domain is not, then

Let's take the opportunity to rephrase this comment a bit.

Perhaps something along the lines: "As we don't power off a non IRQ safe PM domain which holds an IRQ safe device, we don't need to restore the power to it."

Quoted 65 lines

drivers/base/power/domain.c +11 -8

+        * the PM domain is never powered off.
+        */
+       if (dev->power.irq_safe && !genpd->irq_safe)
                return genpd_start_dev_no_timing(genpd, dev);
 
-       mutex_lock(&genpd->lock);
+       genpd_lock_domain(genpd);
        ret = __pm_genpd_poweron(genpd);
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
 
        if (ret)
                return ret;

@@ -729,14 +801,14 @@ static int pm_genpd_prepare(struct device *dev)
        if (resume_needed(dev, genpd))
                pm_runtime_resume(dev);
 
-       mutex_lock(&genpd->lock);
+       genpd_lock_domain(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_domain(genpd);
 
        if (genpd->suspend_power_off) {
                pm_runtime_put_noidle(dev);

@@ -754,12 +826,12 @@ static int pm_genpd_prepare(struct device *dev)
 
        ret = pm_generic_prepare(dev);
        if (ret) {
-               mutex_lock(&genpd->lock);
+               genpd_lock_domain(genpd);
 
                if (--genpd->prepared_count == 0)
                        genpd->suspend_power_off = false;
 
-               mutex_unlock(&genpd->lock);
+               genpd_unlock_domain(genpd);
                pm_runtime_enable(dev);
        }
 

@@ -1116,13 +1188,13 @@ static void pm_genpd_complete(struct device *dev)
        if (IS_ERR(genpd))
                return;
 
-       mutex_lock(&genpd->lock);
+       genpd_lock_domain(genpd);
 
        run_complete = !genpd->suspend_power_off;
        if (--genpd->prepared_count == 0)
                genpd->suspend_power_off = false;
 
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
 
        if (run_complete) {
                pm_generic_complete(dev);

@@ -1266,11 +1338,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;
 

drivers/base/power/domain.c int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, · line 1341

Quoted 1 line

drivers/base/power/domain.c +1 -0

+       /* Devices in an IRQ safe PM Domain have to be irq safe too */

Let's be consistent and stick with IRQ in upper case letters for the comments. I think there are other places in the patch which needs update as well.

drivers/base/power/domain.c int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, · line 1343

Quoted 2 lines

drivers/base/power/domain.c +2 -0

+       if (genpd->irq_safe && !dev->power.irq_safe) {
+               dev_warn(dev,

dev_err(), or perhaps even WARN() ?

Quoted 49 lines

drivers/base/power/domain.c +9 -5

+                       "Devices in an irq-safe domain have to be irq safe.\n");
+               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_domain(genpd);
 
        if (genpd->prepared_count > 0) {
                ret = -EAGAIN;

@@ -1287,7 +1366,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_domain(genpd);
 
        if (ret)
                genpd_free_dev_data(dev, gpd_data);

@@ -1331,7 +1410,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_domain(genpd);
 
        if (genpd->prepared_count > 0) {
                ret = -EAGAIN;

@@ -1346,14 +1425,14 @@ int pm_genpd_remove_device(struct generic_pm_domain *genpd,
 
        list_del_init(&pdd->list_node);
 
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
 
        genpd_free_dev_data(dev, gpd_data);
 
        return 0;
 
  out:
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
        dev_pm_qos_add_notifier(dev, &gpd_data->nb);
 
        return ret;

drivers/base/power/domain.c int pm_genpd_add_subdomain(struct generic_pm_domain *genpd, · line 1460

Quoted 8 lines

drivers/base/power/domain.c +5 -0

@@ -1374,12 +1453,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.
+        */

Have you considered if/how to address this limitation?

Just to be clear, for now I think this limitation is perfectly okay, but going forward we might see a need to mix non IRQ safe domains with IRQ safe domains, via sub-domains.

drivers/base/power/domain.c int pm_genpd_add_subdomain(struct generic_pm_domain *genpd, · line 1463

Quoted 3 lines

drivers/base/power/domain.c +3 -0

+       if (genpd->irq_safe && !subdomain->irq_safe) {
+               pr_warn("Incompatible sub-domain %s of irq-safe domain %s\n",
+                               subdomain->name, genpd->name);

Similar comment as earlier. Perhaps pr_err() or WARN() instead.

Quoted 61 lines

drivers/base/power/domain.c +11 -8

+               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_domain(genpd);
+       genpd_lock_domain_nested(subdomain);
 
        if (genpd->status == GPD_STATE_POWER_OFF
            &&  subdomain->status != GPD_STATE_POWER_OFF) {

@@ -1402,8 +1492,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_domain(subdomain);
+       genpd_unlock_domain(genpd);
 
        return ret;
 }

@@ -1451,13 +1541,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_domain(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_domain_nested(subdomain);
 
                list_del(&link->master_node);
                list_del(&link->slave_node);

@@ -1465,13 +1555,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_domain(subdomain);
 
                ret = 0;
                break;
        }
 
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
 
        return ret;
 }

@@ -1495,11 +1585,17 @@ int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state)
        if (IS_ERR_OR_NULL(genpd) || state < 0)
                return -EINVAL;
 

drivers/base/power/domain.c int pm_genpd_attach_cpuidle(struct generic_pm_domain *genpd, int state) · line 1590

Quoted 3 lines

drivers/base/power/domain.c +3 -0

+       if (genpd->irq_safe) {
+               pr_err("Domain %s is irq safe, cannot attach to cpuidle\n",
+                               genpd->name);

Here you have pr_err(), which is okay but not consistent. Also, I wonder if we should use a WARN() instead?

Quoted 43 lines

drivers/base/power/domain.c +7 -4

+               return -EINVAL;
+       }
+
        cpuidle_data = kzalloc(sizeof(*cpuidle_data), GFP_KERNEL);
        if (!cpuidle_data)
                return -ENOMEM;
 
-       mutex_lock(&genpd->lock);
+       genpd_lock_domain(genpd);
 
        if (genpd->cpuidle_data) {
                ret = -EEXIST;

@@ -1526,7 +1622,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_domain(genpd);
        return ret;
 
  err:

@@ -1563,7 +1659,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_domain(genpd);
 
        cpuidle_data = genpd->cpuidle_data;
        if (!cpuidle_data) {

@@ -1581,7 +1677,7 @@ int pm_genpd_detach_cpuidle(struct generic_pm_domain *genpd)
        kfree(cpuidle_data);
 
  out:
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
        return ret;
 }
 

@@ -1642,6 +1738,17 @@ static int pm_genpd_default_restore_state(struct device *dev)
        return cb ? cb(dev) : 0;
 }
 

drivers/base/power/domain.c static int pm_genpd_default_restore_state(struct device *dev) · line 1745

Quoted 5 lines

drivers/base/power/domain.c +5 -0

+static void genpd_lock_domain_init(struct generic_pm_domain *genpd)
+{
+       if (genpd->flags & GENPD_FLAG_IRQ_SAFE) {
+               spin_lock_init(&genpd->slock);
+               genpd->irq_safe = true;

Copying the result from "genpd->flags & GENPD_FLAG_IRQ_SAFE" into the irq_safe bool, seems like a "micro" optimization. How about only using the genpd->flags instead?

Quoted 84 lines

drivers/base/power/domain.c +11 -4

+       } 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.

@@ -1657,7 +1764,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_domain_init(genpd);
        genpd->gov = gov;
        INIT_WORK(&genpd->power_off_work, genpd_power_off_work_fn);
        genpd->in_progress = 0;

@@ -2042,7 +2149,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_domain_interruptible(genpd);
        if (ret)
                return -ERESTARTSYS;
 

@@ -2062,7 +2169,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;
 

@@ -2073,7 +2181,7 @@ static int pm_genpd_summary_one(struct seq_file *s,
 
        seq_puts(s, "\n");
 exit:
-       mutex_unlock(&genpd->lock);
+       genpd_unlock_domain(genpd);
 
        return 0;
 }

include/linux/pm_domain.h +10 -1

index b2725e6..dc7cb53 100644
@@ -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;
+               };
+       };
 };
 

Quoted 4 lines

include/linux/pm_domain.h +0 -1

 static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd)
--

2.1.4

Quoted 4 lines

Quoted 4 lines

drivers/base/power/domain.c +0 -0

@@ -387,7 +452,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain
 *genpd)
                        return -EBUSY;
 

Quoted 8 lines

drivers/base/power/domain.c static int pm_genpd_poweroff(struct generic_pm_domain *genpd)

Quoted 4 lines

drivers/base/power/domain.c +1 -1

                if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev)
-                   || pdd->dev->power.irq_safe))
+                       || (pdd->dev->power.irq_safe &&
 !genpd->irq_safe)))

This deserves a comment in the code.

This check should simplify to

                if (!pm_runtime_suspended(pdd->dev))

The other connditions seem unnecessary just to determine if the devices are suspended.

Do you see any problem with that?

Quoted 22 lines

Quoted 15 lines

drivers/base/power/domain.c +0 -2

@@ -478,12 +543,8 @@ static int pm_genpd_runtime_suspend(struct device
 *dev)
        if (IS_ERR(genpd))
                return -EINVAL;
 
-       /*
-        * We can't allow to power off the PM domain if it holds an

irq_safe
- * device. That's beacuse we use mutexes to protect data while
power
- * off and on the PM domain, thus we can't execute in atomic
context.

drivers/base/power/domain.c +1 -2

-        */
-       if (dev->power.irq_safe)
+       /* We can't allow to power off a domain that is also not irq

Quoted 1 line

safe. */

I wouldn't mind to keep some more pieces of the earlier comment, since it explains a bit more of the *why*. Could you try to rephrase this comment in that regard?

Quoted 5 lines

Quoted 1 line

+ if (dev->power.irq_safe && !genpd->irq_safe)

This looks correct...

Apparently not the best place to check this. We should still allow runtime suspend of the device, irrespective of whether the domain is IRQ safe or not and this check here is incorrect.

Quoted 9 lines

Quoted 9 lines

                return -EBUSY;

        stop_ok = genpd->gov ? genpd->gov->stop_ok : NULL;

drivers/base/power/domain.c +0 -0

@@ -500,11 +561,19 @@ static int pm_genpd_runtime_suspend(struct device
 *dev)
                return ret;
        }
 

Quoted 13 lines

drivers/base/power/domain.c static int pm_genpd_runtime_suspend(struct device *dev) · line 569

Quoted 8 lines

drivers/base/power/domain.c +2 -1

-       mutex_lock(&genpd->lock);
+       /*
+        * If power.irq_safe is set, this routine will be run with

interrupts

drivers/base/power/domain.c +4 -0

+        * off, so suspend only if the power domain is irq_safe.
+        */
+       if (dev->power.irq_safe && !genpd->irq_safe)
+               return 0;

... but this doesn't. You have already returned -EBUSY above for this case.

This is probably more appropriate, as we want to skip locking the domain, because it may be in atomic context and the domain may use mutexes to lock.

I feel like, we miss out an opportunity here to power off the domain, if device is IRQ safe while the domain is not, but the function is called from a process context. I am not sure how to check that correctly, yet.