On Sun, Jun 07 2015 at 03:21 -0600, Krzysztof Kozlowski wrote:

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

quoted

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

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.

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

Cc: Ulf Hansson <redacted>
Cc: Rafael J. Wysocki <redacted>
Cc: Kevin Hilman <redacted>
Signed-off-by: Lina Iyer <redacted>
---
 drivers/base/power/domain.c | 200 ++++++++++++++++++++++++++++++++++----------
 include/linux/pm_domain.h   |  11 ++-

Documentation should also be reflected.

Yes, will add.

quoted

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

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index dfd7595..8b89d15 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c

@@ -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)	\
+			: 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.

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

+
 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);

-	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

-	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;

+	/* 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.

Thanks for your review.

-- Lina