| Message ID | 1486571692-33212-4-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 8 February 2017 at 17:34, Lina Iyer <lina.iyer@linaro.org> wrote: > Powering off a domain schedules a work to opportunistically power off > the parent domains. Domains that are IRQ safe may have parents that are > also IRQ safe. It would be beneficial to power off such IRQ safe parents > in the same context as well. > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > --- > drivers/base/power/domain.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 3825bb9..51e2254 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -375,7 +375,8 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb, > * If all of the @genpd's devices have been suspended and all of its subdomains > * have been powered down, remove power from @genpd. > */ > -static int genpd_power_off(struct generic_pm_domain *genpd, bool is_async) > +static int genpd_power_off(struct generic_pm_domain *genpd, bool is_async, > + unsigned int depth) > { > struct pm_domain_data *pdd; > struct gpd_link *link; > @@ -442,7 +443,17 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool is_async) > > list_for_each_entry(link, &genpd->slave_links, slave_node) { > genpd_sd_counter_dec(link->master); > - genpd_queue_power_off_work(link->master); > + /* > + * Power off the parent in the same context if the parent > + * domain is also IRQ safe. > + */ > + if (genpd_is_irq_safe(genpd) && > + genpd_is_irq_safe(link->master)) { > + genpd_lock_nested(link->master, depth + 1); > + genpd_power_off(link->master, false, depth + 1); This doesn't work. You must not call genpd_power_off() using "false" here. That's because "true" in the recursive call, for the master domain tells genpd_power_off() that is has been called from genpd's ->runtime_suspend() callback. That means genpd_power_off() thinks it's okay to allow *one* device in the domain to not be runtime suspended when allowing a power off to be done. This assumption is not correct for the master domain. > + genpd_unlock(link->master); > + } else > + genpd_queue_power_off_work(link->master); > } > > return 0; > @@ -459,7 +470,7 @@ static void genpd_power_off_work_fn(struct work_struct *work) > genpd = container_of(work, struct generic_pm_domain, power_off_work); > > genpd_lock(genpd); > - genpd_power_off(genpd, true); > + genpd_power_off(genpd, true, 0); > genpd_unlock(genpd); > } > > @@ -578,7 +589,7 @@ static int genpd_runtime_suspend(struct device *dev) > return 0; > > genpd_lock(genpd); > - genpd_power_off(genpd, false); > + genpd_power_off(genpd, false, 0); > genpd_unlock(genpd); > > return 0; > @@ -658,7 +669,7 @@ static int genpd_runtime_resume(struct device *dev) > if (!pm_runtime_is_irq_safe(dev) || > (pm_runtime_is_irq_safe(dev) && genpd_is_irq_safe(genpd))) { > genpd_lock(genpd); > - genpd_power_off(genpd, 0); > + genpd_power_off(genpd, 0, 0); > genpd_unlock(genpd); > } > > -- > 2.7.4 > Some more thoughts.. Actually, I have been thinking of changing genpd to avoid queuing power off works, no matter if the PM domain are IRQ safe or not. There are several reasons, but primarily it helps to avoid wasting power. Currently I don't see any reasons to why such change shouldn't be feasible. As a matter of fact, changing this became possible while we removed the intermediate states in genpd in commit ba2bbfbf6307 ("PM / Domains: Remove intermediate states from the power off sequence"). Allow me to help out and cook a patch for this, it's already in the pipe. :-) Kind regards Uffe
On 9 February 2017 at 10:02, Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 8 February 2017 at 17:34, Lina Iyer <lina.iyer@linaro.org> wrote: >> Powering off a domain schedules a work to opportunistically power off >> the parent domains. Domains that are IRQ safe may have parents that are >> also IRQ safe. It would be beneficial to power off such IRQ safe parents >> in the same context as well. >> >> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >> --- >> drivers/base/power/domain.c | 21 ++++++++++++++++----- >> 1 file changed, 16 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c >> index 3825bb9..51e2254 100644 >> --- a/drivers/base/power/domain.c >> +++ b/drivers/base/power/domain.c >> @@ -375,7 +375,8 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb, >> * If all of the @genpd's devices have been suspended and all of its subdomains >> * have been powered down, remove power from @genpd. >> */ >> -static int genpd_power_off(struct generic_pm_domain *genpd, bool is_async) >> +static int genpd_power_off(struct generic_pm_domain *genpd, bool is_async, >> + unsigned int depth) >> { >> struct pm_domain_data *pdd; >> struct gpd_link *link; >> @@ -442,7 +443,17 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool is_async) >> >> list_for_each_entry(link, &genpd->slave_links, slave_node) { >> genpd_sd_counter_dec(link->master); >> - genpd_queue_power_off_work(link->master); >> + /* >> + * Power off the parent in the same context if the parent >> + * domain is also IRQ safe. >> + */ >> + if (genpd_is_irq_safe(genpd) && >> + genpd_is_irq_safe(link->master)) { >> + genpd_lock_nested(link->master, depth + 1); >> + genpd_power_off(link->master, false, depth + 1); > > This doesn't work. You must not call genpd_power_off() using "false" here. > > That's because "true" in the recursive call, for the master domain /s/true/false > tells genpd_power_off() that is has been called from genpd's > ->runtime_suspend() callback. That means genpd_power_off() thinks it's > okay to allow *one* device in the domain to not be runtime suspended > when allowing a power off to be done. This assumption is not correct > for the master domain. > >> + genpd_unlock(link->master); >> + } else >> + genpd_queue_power_off_work(link->master); >> } >> >> return 0; >> @@ -459,7 +470,7 @@ static void genpd_power_off_work_fn(struct work_struct *work) >> genpd = container_of(work, struct generic_pm_domain, power_off_work); >> >> genpd_lock(genpd); >> - genpd_power_off(genpd, true); >> + genpd_power_off(genpd, true, 0); >> genpd_unlock(genpd); >> } >> >> @@ -578,7 +589,7 @@ static int genpd_runtime_suspend(struct device *dev) >> return 0; >> >> genpd_lock(genpd); >> - genpd_power_off(genpd, false); >> + genpd_power_off(genpd, false, 0); >> genpd_unlock(genpd); >> >> return 0; >> @@ -658,7 +669,7 @@ static int genpd_runtime_resume(struct device *dev) >> if (!pm_runtime_is_irq_safe(dev) || >> (pm_runtime_is_irq_safe(dev) && genpd_is_irq_safe(genpd))) { >> genpd_lock(genpd); >> - genpd_power_off(genpd, 0); >> + genpd_power_off(genpd, 0, 0); >> genpd_unlock(genpd); >> } >> >> -- >> 2.7.4 >> > > Some more thoughts.. > > Actually, I have been thinking of changing genpd to avoid queuing > power off works, no matter if the PM domain are IRQ safe or not. There > are several reasons, but primarily it helps to avoid wasting power. > > Currently I don't see any reasons to why such change shouldn't be > feasible. As a matter of fact, changing this became possible while we > removed the intermediate states in genpd in commit ba2bbfbf6307 ("PM / > Domains: Remove intermediate states from the power off sequence"). > > Allow me to help out and cook a patch for this, it's already in the pipe. :-) > > Kind regards > Uffe
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 3825bb9..51e2254 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -375,7 +375,8 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb, * If all of the @genpd's devices have been suspended and all of its subdomains * have been powered down, remove power from @genpd. */ -static int genpd_power_off(struct generic_pm_domain *genpd, bool is_async) +static int genpd_power_off(struct generic_pm_domain *genpd, bool is_async, + unsigned int depth) { struct pm_domain_data *pdd; struct gpd_link *link; @@ -442,7 +443,17 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool is_async) list_for_each_entry(link, &genpd->slave_links, slave_node) { genpd_sd_counter_dec(link->master); - genpd_queue_power_off_work(link->master); + /* + * Power off the parent in the same context if the parent + * domain is also IRQ safe. + */ + if (genpd_is_irq_safe(genpd) && + genpd_is_irq_safe(link->master)) { + genpd_lock_nested(link->master, depth + 1); + genpd_power_off(link->master, false, depth + 1); + genpd_unlock(link->master); + } else + genpd_queue_power_off_work(link->master); } return 0; @@ -459,7 +470,7 @@ static void genpd_power_off_work_fn(struct work_struct *work) genpd = container_of(work, struct generic_pm_domain, power_off_work); genpd_lock(genpd); - genpd_power_off(genpd, true); + genpd_power_off(genpd, true, 0); genpd_unlock(genpd); } @@ -578,7 +589,7 @@ static int genpd_runtime_suspend(struct device *dev) return 0; genpd_lock(genpd); - genpd_power_off(genpd, false); + genpd_power_off(genpd, false, 0); genpd_unlock(genpd); return 0; @@ -658,7 +669,7 @@ static int genpd_runtime_resume(struct device *dev) if (!pm_runtime_is_irq_safe(dev) || (pm_runtime_is_irq_safe(dev) && genpd_is_irq_safe(genpd))) { genpd_lock(genpd); - genpd_power_off(genpd, 0); + genpd_power_off(genpd, 0, 0); genpd_unlock(genpd); }
Powering off a domain schedules a work to opportunistically power off the parent domains. Domains that are IRQ safe may have parents that are also IRQ safe. It would be beneficial to power off such IRQ safe parents in the same context as well. Signed-off-by: Lina Iyer <lina.iyer@linaro.org> --- drivers/base/power/domain.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-)