| Message ID | 20221108013517.749665-1-marex@denx.de (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [1/3,RFC] PM: domains: Introduce .power_pre/post_on/off callbacks | expand |
Hi Marek, Thank you for the patch. On Tue, Nov 08, 2022 at 02:35:15AM +0100, Marek Vasut wrote: > Currently it is possible that a power domain power on or off would claim > the genpd lock first and clock core prepare_lock second, while another > thread could do the reverse, and this would trigger lockdep warning. > > Introduce new callbacks, .power_pre/post_on() and .power_off_pre/post(), which > are triggered before the genpd_lock() and after genpd_unlock() respectively in > case the domain is powered on and off. Those are meant to let drivers claim > clock core prepare_lock via clk_*prepare() call and release the lock via > clk_*unprepare() call to always assure that the clock and genpd lock ordering > is correct. > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Adam Ford <aford173@gmail.com> > Cc: Fabio Estevam <festevam@denx.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Jacky Bai <ping.bai@nxp.com> > Cc: Kevin Hilman <khilman@kernel.org> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Len Brown <len.brown@intel.com> > Cc: Liam Girdwood <lgirdwood@gmail.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Marek Vasut <marex@denx.de> > Cc: Mark Brown <broonie@kernel.org> > Cc: Martin Kepplinger <martink@posteo.de> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Peng Fan <peng.fan@nxp.com> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > Cc: Philipp Zabel <p.zabel@pengutronix.de> > Cc: Rafael J. Wysocki <rafael@kernel.org> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Shengjiu Wang <shengjiu.wang@nxp.com> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: linux-clk@vger.kernel.org > Cc: linux-imx@nxp.com > Cc: linux-pm@vger.kernel.org > To: linux-arm-kernel@lists.infradead.org > --- > drivers/base/power/domain.c | 103 ++++++++++++++++++++++++++++++++---- > include/linux/pm_domain.h | 4 ++ > 2 files changed, 97 insertions(+), 10 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 6471b559230e9..df2a93d0674e4 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -494,6 +494,22 @@ void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next) > } > EXPORT_SYMBOL_GPL(dev_pm_genpd_set_next_wakeup); > > +static int genpd_power_pre_on(struct generic_pm_domain *genpd) > +{ > + if (!genpd->power_pre_on) > + return 0; > + > + return genpd->power_pre_on(genpd); > +} > + > +static int genpd_power_post_on(struct generic_pm_domain *genpd) > +{ > + if (!genpd->power_post_on) > + return 0; > + > + return genpd->power_post_on(genpd); > +} > + > static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) > { > unsigned int state_idx = genpd->state_idx; > @@ -544,6 +560,22 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) > return ret; > } > > +static int genpd_power_off_pre(struct generic_pm_domain *genpd) > +{ > + if (!genpd->power_off_pre) > + return 0; > + > + return genpd->power_off_pre(genpd); > +} > + > +static int genpd_power_off_post(struct generic_pm_domain *genpd) > +{ > + if (!genpd->power_off_post) > + return 0; > + > + return genpd->power_off_post(genpd); > +} > + > static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed) > { > unsigned int state_idx = genpd->state_idx; > @@ -816,12 +848,18 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb, > static void genpd_power_off_work_fn(struct work_struct *work) > { > struct generic_pm_domain *genpd; > + int ret; > > genpd = container_of(work, struct generic_pm_domain, power_off_work); > > + ret = genpd_power_off_pre(genpd); > + if (ret) > + return; > genpd_lock(genpd); > genpd_power_off(genpd, false, 0); > genpd_unlock(genpd); > + ret = genpd_power_off_post(genpd); > + WARN_ON_ONCE(ret); > } > > /** > @@ -938,12 +976,14 @@ static int genpd_runtime_suspend(struct device *dev) > if (irq_safe_dev_in_sleep_domain(dev, genpd)) > return 0; > > + ret = genpd_power_off_pre(genpd); > + if (ret) > + return ret; > genpd_lock(genpd); > gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > genpd_power_off(genpd, true, 0); > genpd_unlock(genpd); > - > - return 0; > + return genpd_power_off_post(genpd); > } > > /** > @@ -977,12 +1017,21 @@ static int genpd_runtime_resume(struct device *dev) > if (irq_safe_dev_in_sleep_domain(dev, genpd)) > goto out; > > + ret = genpd_power_pre_on(genpd); > + if (ret) > + return ret; > genpd_lock(genpd); > ret = genpd_power_on(genpd, 0); > if (!ret) > genpd_restore_performance_state(dev, gpd_data->rpm_pstate); > genpd_unlock(genpd); > > + if (ret) { > + genpd_power_post_on(genpd); > + return ret; > + } > + > + ret = genpd_power_post_on(genpd); > if (ret) > return ret; > > @@ -1017,10 +1066,13 @@ static int genpd_runtime_resume(struct device *dev) > genpd_stop_dev(genpd, dev); > err_poweroff: > if (!pm_runtime_is_irq_safe(dev) || genpd_is_irq_safe(genpd)) { > - genpd_lock(genpd); > - gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > - genpd_power_off(genpd, true, 0); > - genpd_unlock(genpd); > + if (!genpd_power_off_pre(genpd)) { > + genpd_lock(genpd); > + gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > + genpd_power_off(genpd, true, 0); > + genpd_unlock(genpd); > + genpd_power_off_post(genpd); > + } > } > > return ret; > @@ -1225,12 +1277,14 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff) > } > } > > + ret = genpd_power_off_pre(genpd); > + if (ret) > + return ret; > genpd_lock(genpd); > genpd->suspended_count++; > genpd_sync_power_off(genpd, true, 0); > genpd_unlock(genpd); > - > - return 0; > + return genpd_power_off_post(genpd); > } > > /** > @@ -1267,10 +1321,16 @@ static int genpd_resume_noirq(struct device *dev) > if (device_wakeup_path(dev) && genpd_is_active_wakeup(genpd)) > return pm_generic_resume_noirq(dev); > > + ret = genpd_power_pre_on(genpd); > + if (ret) > + return ret; > genpd_lock(genpd); > genpd_sync_power_on(genpd, true, 0); > genpd->suspended_count--; > genpd_unlock(genpd); > + ret = genpd_power_post_on(genpd); > + if (ret) > + return ret; > > if (genpd->dev_ops.stop && genpd->dev_ops.start && > !pm_runtime_status_suspended(dev)) { > @@ -1378,6 +1438,9 @@ static int genpd_restore_noirq(struct device *dev) > * At this point suspended_count == 0 means we are being run for the > * first time for the given domain in the present cycle. > */ > + ret = genpd_power_pre_on(genpd); > + if (ret) > + return ret; > genpd_lock(genpd); > if (genpd->suspended_count++ == 0) { > /* > @@ -1390,6 +1453,9 @@ static int genpd_restore_noirq(struct device *dev) > > genpd_sync_power_on(genpd, true, 0); > genpd_unlock(genpd); > + ret = genpd_power_post_on(genpd); > + if (ret) > + return ret; > > if (genpd->dev_ops.stop && genpd->dev_ops.start && > !pm_runtime_status_suspended(dev)) { > @@ -1413,6 +1479,7 @@ static int genpd_restore_noirq(struct device *dev) > static void genpd_complete(struct device *dev) > { > struct generic_pm_domain *genpd; > + int ret; This variable is unused, causing a compilation error. > > dev_dbg(dev, "%s()\n", __func__); > > @@ -1435,6 +1502,7 @@ static void genpd_switch_state(struct device *dev, bool suspend) > { > struct generic_pm_domain *genpd; > bool use_lock; > + int ret; > > genpd = dev_to_genpd_safe(dev); > if (!genpd) > @@ -1442,8 +1510,13 @@ static void genpd_switch_state(struct device *dev, bool suspend) > > use_lock = genpd_is_irq_safe(genpd); > > - if (use_lock) > + if (use_lock) { > + ret = suspend ? genpd_power_off_pre(genpd) : > + genpd_power_pre_on(genpd); > + if (ret) > + return; > genpd_lock(genpd); > + } > > if (suspend) { > genpd->suspended_count++; > @@ -1453,8 +1526,12 @@ static void genpd_switch_state(struct device *dev, bool suspend) > genpd->suspended_count--; > } > > - if (use_lock) > + if (use_lock) { > genpd_unlock(genpd); > + ret = suspend ? genpd_power_off_post(genpd) : > + genpd_power_post_on(genpd); > + WARN_ON_ONCE(ret); > + } > } > > /** > @@ -2750,9 +2827,15 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, > dev->pm_domain->sync = genpd_dev_pm_sync; > > if (power_on) { > + ret = genpd_power_pre_on(pd); > + if (ret) > + return ret; > genpd_lock(pd); > ret = genpd_power_on(pd, 0); > genpd_unlock(pd); > + ret = genpd_power_post_on(pd); > + if (ret) > + return ret; > } > > if (ret) { > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index ebc3516980907..3cf231a27cb1b 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -134,8 +134,12 @@ struct generic_pm_domain { > unsigned int prepared_count; /* Suspend counter of prepared devices */ > unsigned int performance_state; /* Aggregated max performance state */ > cpumask_var_t cpus; /* A cpumask of the attached CPUs */ > + int (*power_off_pre)(struct generic_pm_domain *domain); > int (*power_off)(struct generic_pm_domain *domain); > + int (*power_off_post)(struct generic_pm_domain *domain); > + int (*power_pre_on)(struct generic_pm_domain *domain); > int (*power_on)(struct generic_pm_domain *domain); > + int (*power_post_on)(struct generic_pm_domain *domain); > struct raw_notifier_head power_notifiers; /* Power on/off notifiers */ > struct opp_table *opp_table; /* OPP table of the genpd */ > unsigned int (*opp_to_performance_state)(struct generic_pm_domain *genpd,
On 11/9/22 14:19, Laurent Pinchart wrote: Hi, [...] >> @@ -1413,6 +1479,7 @@ static int genpd_restore_noirq(struct device *dev) >> static void genpd_complete(struct device *dev) >> { >> struct generic_pm_domain *genpd; >> + int ret; > > This variable is unused, causing a compilation error. I suspect only with -Werror, but anyway, already fixed locally, it's a rebase on latest next artifact. [...] >> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h >> index ebc3516980907..3cf231a27cb1b 100644 >> --- a/include/linux/pm_domain.h >> +++ b/include/linux/pm_domain.h >> @@ -134,8 +134,12 @@ struct generic_pm_domain { >> unsigned int prepared_count; /* Suspend counter of prepared devices */ >> unsigned int performance_state; /* Aggregated max performance state */ >> cpumask_var_t cpus; /* A cpumask of the attached CPUs */ >> + int (*power_off_pre)(struct generic_pm_domain *domain); >> int (*power_off)(struct generic_pm_domain *domain); >> + int (*power_off_post)(struct generic_pm_domain *domain); >> + int (*power_pre_on)(struct generic_pm_domain *domain); >> int (*power_on)(struct generic_pm_domain *domain); >> + int (*power_post_on)(struct generic_pm_domain *domain); >> struct raw_notifier_head power_notifiers; /* Power on/off notifiers */ >> struct opp_table *opp_table; /* OPP table of the genpd */ >> unsigned int (*opp_to_performance_state)(struct generic_pm_domain *genpd, I am looking more for a feedback on this extension of the callbacks, and on the overall approach. Is this something which looks OK, or would there be better way to handle this ?
On Tue, 8 Nov 2022 at 02:35, Marek Vasut <marex@denx.de> wrote: > > Currently it is possible that a power domain power on or off would claim > the genpd lock first and clock core prepare_lock second, while another > thread could do the reverse, and this would trigger lockdep warning. I am not quite sure I fully understand. In this case is the lockdep warning relevant or just something that we want to silence? > > Introduce new callbacks, .power_pre/post_on() and .power_off_pre/post(), which > are triggered before the genpd_lock() and after genpd_unlock() respectively in > case the domain is powered on and off. Those are meant to let drivers claim > clock core prepare_lock via clk_*prepare() call and release the lock via > clk_*unprepare() call to always assure that the clock and genpd lock ordering > is correct. To me, this sounds like a problem that may be better fixed by trying to model the parent/child-domains in a more strict way, through genpd. There is a comment in the code in imx8mp_blk_ctrl_probe() that seems to be pointing in this direction too. "* We use runtime PM to trigger power on/off of the upstream GPC * domain, as a strict hierarchical parent/child power domain * setup doesn't allow us to meet the sequencing requirements......" I am wondering about what those "sequencing requirements" are - and whether it could make better sense to fix these issues instead? Kind regards Uffe > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Adam Ford <aford173@gmail.com> > Cc: Fabio Estevam <festevam@denx.de> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Jacky Bai <ping.bai@nxp.com> > Cc: Kevin Hilman <khilman@kernel.org> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Cc: Len Brown <len.brown@intel.com> > Cc: Liam Girdwood <lgirdwood@gmail.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Marek Vasut <marex@denx.de> > Cc: Mark Brown <broonie@kernel.org> > Cc: Martin Kepplinger <martink@posteo.de> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Peng Fan <peng.fan@nxp.com> > Cc: Pengutronix Kernel Team <kernel@pengutronix.de> > Cc: Philipp Zabel <p.zabel@pengutronix.de> > Cc: Rafael J. Wysocki <rafael@kernel.org> > Cc: Sascha Hauer <s.hauer@pengutronix.de> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Shengjiu Wang <shengjiu.wang@nxp.com> > Cc: Stephen Boyd <sboyd@kernel.org> > Cc: Ulf Hansson <ulf.hansson@linaro.org> > Cc: linux-clk@vger.kernel.org > Cc: linux-imx@nxp.com > Cc: linux-pm@vger.kernel.org > To: linux-arm-kernel@lists.infradead.org > --- > drivers/base/power/domain.c | 103 ++++++++++++++++++++++++++++++++---- > include/linux/pm_domain.h | 4 ++ > 2 files changed, 97 insertions(+), 10 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 6471b559230e9..df2a93d0674e4 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -494,6 +494,22 @@ void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next) > } > EXPORT_SYMBOL_GPL(dev_pm_genpd_set_next_wakeup); > > +static int genpd_power_pre_on(struct generic_pm_domain *genpd) > +{ > + if (!genpd->power_pre_on) > + return 0; > + > + return genpd->power_pre_on(genpd); > +} > + > +static int genpd_power_post_on(struct generic_pm_domain *genpd) > +{ > + if (!genpd->power_post_on) > + return 0; > + > + return genpd->power_post_on(genpd); > +} > + > static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) > { > unsigned int state_idx = genpd->state_idx; > @@ -544,6 +560,22 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) > return ret; > } > > +static int genpd_power_off_pre(struct generic_pm_domain *genpd) > +{ > + if (!genpd->power_off_pre) > + return 0; > + > + return genpd->power_off_pre(genpd); > +} > + > +static int genpd_power_off_post(struct generic_pm_domain *genpd) > +{ > + if (!genpd->power_off_post) > + return 0; > + > + return genpd->power_off_post(genpd); > +} > + > static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed) > { > unsigned int state_idx = genpd->state_idx; > @@ -816,12 +848,18 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb, > static void genpd_power_off_work_fn(struct work_struct *work) > { > struct generic_pm_domain *genpd; > + int ret; > > genpd = container_of(work, struct generic_pm_domain, power_off_work); > > + ret = genpd_power_off_pre(genpd); > + if (ret) > + return; > genpd_lock(genpd); > genpd_power_off(genpd, false, 0); > genpd_unlock(genpd); > + ret = genpd_power_off_post(genpd); > + WARN_ON_ONCE(ret); > } > > /** > @@ -938,12 +976,14 @@ static int genpd_runtime_suspend(struct device *dev) > if (irq_safe_dev_in_sleep_domain(dev, genpd)) > return 0; > > + ret = genpd_power_off_pre(genpd); > + if (ret) > + return ret; > genpd_lock(genpd); > gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > genpd_power_off(genpd, true, 0); > genpd_unlock(genpd); > - > - return 0; > + return genpd_power_off_post(genpd); > } > > /** > @@ -977,12 +1017,21 @@ static int genpd_runtime_resume(struct device *dev) > if (irq_safe_dev_in_sleep_domain(dev, genpd)) > goto out; > > + ret = genpd_power_pre_on(genpd); > + if (ret) > + return ret; > genpd_lock(genpd); > ret = genpd_power_on(genpd, 0); > if (!ret) > genpd_restore_performance_state(dev, gpd_data->rpm_pstate); > genpd_unlock(genpd); > > + if (ret) { > + genpd_power_post_on(genpd); > + return ret; > + } > + > + ret = genpd_power_post_on(genpd); > if (ret) > return ret; > > @@ -1017,10 +1066,13 @@ static int genpd_runtime_resume(struct device *dev) > genpd_stop_dev(genpd, dev); > err_poweroff: > if (!pm_runtime_is_irq_safe(dev) || genpd_is_irq_safe(genpd)) { > - genpd_lock(genpd); > - gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > - genpd_power_off(genpd, true, 0); > - genpd_unlock(genpd); > + if (!genpd_power_off_pre(genpd)) { > + genpd_lock(genpd); > + gpd_data->rpm_pstate = genpd_drop_performance_state(dev); > + genpd_power_off(genpd, true, 0); > + genpd_unlock(genpd); > + genpd_power_off_post(genpd); > + } > } > > return ret; > @@ -1225,12 +1277,14 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff) > } > } > > + ret = genpd_power_off_pre(genpd); > + if (ret) > + return ret; > genpd_lock(genpd); > genpd->suspended_count++; > genpd_sync_power_off(genpd, true, 0); > genpd_unlock(genpd); > - > - return 0; > + return genpd_power_off_post(genpd); > } > > /** > @@ -1267,10 +1321,16 @@ static int genpd_resume_noirq(struct device *dev) > if (device_wakeup_path(dev) && genpd_is_active_wakeup(genpd)) > return pm_generic_resume_noirq(dev); > > + ret = genpd_power_pre_on(genpd); > + if (ret) > + return ret; > genpd_lock(genpd); > genpd_sync_power_on(genpd, true, 0); > genpd->suspended_count--; > genpd_unlock(genpd); > + ret = genpd_power_post_on(genpd); > + if (ret) > + return ret; > > if (genpd->dev_ops.stop && genpd->dev_ops.start && > !pm_runtime_status_suspended(dev)) { > @@ -1378,6 +1438,9 @@ static int genpd_restore_noirq(struct device *dev) > * At this point suspended_count == 0 means we are being run for the > * first time for the given domain in the present cycle. > */ > + ret = genpd_power_pre_on(genpd); > + if (ret) > + return ret; > genpd_lock(genpd); > if (genpd->suspended_count++ == 0) { > /* > @@ -1390,6 +1453,9 @@ static int genpd_restore_noirq(struct device *dev) > > genpd_sync_power_on(genpd, true, 0); > genpd_unlock(genpd); > + ret = genpd_power_post_on(genpd); > + if (ret) > + return ret; > > if (genpd->dev_ops.stop && genpd->dev_ops.start && > !pm_runtime_status_suspended(dev)) { > @@ -1413,6 +1479,7 @@ static int genpd_restore_noirq(struct device *dev) > static void genpd_complete(struct device *dev) > { > struct generic_pm_domain *genpd; > + int ret; > > dev_dbg(dev, "%s()\n", __func__); > > @@ -1435,6 +1502,7 @@ static void genpd_switch_state(struct device *dev, bool suspend) > { > struct generic_pm_domain *genpd; > bool use_lock; > + int ret; > > genpd = dev_to_genpd_safe(dev); > if (!genpd) > @@ -1442,8 +1510,13 @@ static void genpd_switch_state(struct device *dev, bool suspend) > > use_lock = genpd_is_irq_safe(genpd); > > - if (use_lock) > + if (use_lock) { > + ret = suspend ? genpd_power_off_pre(genpd) : > + genpd_power_pre_on(genpd); > + if (ret) > + return; > genpd_lock(genpd); > + } > > if (suspend) { > genpd->suspended_count++; > @@ -1453,8 +1526,12 @@ static void genpd_switch_state(struct device *dev, bool suspend) > genpd->suspended_count--; > } > > - if (use_lock) > + if (use_lock) { > genpd_unlock(genpd); > + ret = suspend ? genpd_power_off_post(genpd) : > + genpd_power_post_on(genpd); > + WARN_ON_ONCE(ret); > + } > } > > /** > @@ -2750,9 +2827,15 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, > dev->pm_domain->sync = genpd_dev_pm_sync; > > if (power_on) { > + ret = genpd_power_pre_on(pd); > + if (ret) > + return ret; > genpd_lock(pd); > ret = genpd_power_on(pd, 0); > genpd_unlock(pd); > + ret = genpd_power_post_on(pd); > + if (ret) > + return ret; > } > > if (ret) { > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index ebc3516980907..3cf231a27cb1b 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -134,8 +134,12 @@ struct generic_pm_domain { > unsigned int prepared_count; /* Suspend counter of prepared devices */ > unsigned int performance_state; /* Aggregated max performance state */ > cpumask_var_t cpus; /* A cpumask of the attached CPUs */ > + int (*power_off_pre)(struct generic_pm_domain *domain); > int (*power_off)(struct generic_pm_domain *domain); > + int (*power_off_post)(struct generic_pm_domain *domain); > + int (*power_pre_on)(struct generic_pm_domain *domain); > int (*power_on)(struct generic_pm_domain *domain); > + int (*power_post_on)(struct generic_pm_domain *domain); > struct raw_notifier_head power_notifiers; /* Power on/off notifiers */ > struct opp_table *opp_table; /* OPP table of the genpd */ > unsigned int (*opp_to_performance_state)(struct generic_pm_domain *genpd, > -- > 2.35.1 >
On 11/14/22 20:40, Ulf Hansson wrote: > On Tue, 8 Nov 2022 at 02:35, Marek Vasut <marex@denx.de> wrote: >> >> Currently it is possible that a power domain power on or off would claim >> the genpd lock first and clock core prepare_lock second, while another >> thread could do the reverse, and this would trigger lockdep warning. > > I am not quite sure I fully understand. In this case is the lockdep > warning relevant or just something that we want to silence? This is a valid problem, see patches 2/3 and 3/3 for details too. >> Introduce new callbacks, .power_pre/post_on() and .power_off_pre/post(), which >> are triggered before the genpd_lock() and after genpd_unlock() respectively in >> case the domain is powered on and off. Those are meant to let drivers claim >> clock core prepare_lock via clk_*prepare() call and release the lock via >> clk_*unprepare() call to always assure that the clock and genpd lock ordering >> is correct. > > To me, this sounds like a problem that may be better fixed by trying > to model the parent/child-domains in a more strict way, through genpd. > > There is a comment in the code in imx8mp_blk_ctrl_probe() that seems > to be pointing in this direction too. > > "* We use runtime PM to trigger power on/off of the upstream GPC > * domain, as a strict hierarchical parent/child power domain > * setup doesn't allow us to meet the sequencing requirements......" > > I am wondering about what those "sequencing requirements" are - and > whether it could make better sense to fix these issues instead? Here is the lockdep splat: https://lore.kernel.org/all/Y1cs++TV2GCuh4tS@pendragon.ideasonboard.com/ It really is a problem between the clock and genpd subsystem locks, they can be claimed in arbitrary order, see patch 2/3 and 3/3. I think that might clarify what I am attempting to solve here. [...]
+ Stephen Boyd On Mon, 14 Nov 2022 at 21:32, Marek Vasut <marex@denx.de> wrote: > > On 11/14/22 20:40, Ulf Hansson wrote: > > On Tue, 8 Nov 2022 at 02:35, Marek Vasut <marex@denx.de> wrote: > >> > >> Currently it is possible that a power domain power on or off would claim > >> the genpd lock first and clock core prepare_lock second, while another > >> thread could do the reverse, and this would trigger lockdep warning. > > > > I am not quite sure I fully understand. In this case is the lockdep > > warning relevant or just something that we want to silence? > > This is a valid problem, see patches 2/3 and 3/3 for details too. > > >> Introduce new callbacks, .power_pre/post_on() and .power_off_pre/post(), which > >> are triggered before the genpd_lock() and after genpd_unlock() respectively in > >> case the domain is powered on and off. Those are meant to let drivers claim > >> clock core prepare_lock via clk_*prepare() call and release the lock via > >> clk_*unprepare() call to always assure that the clock and genpd lock ordering > >> is correct. > > > > To me, this sounds like a problem that may be better fixed by trying > > to model the parent/child-domains in a more strict way, through genpd. > > > > There is a comment in the code in imx8mp_blk_ctrl_probe() that seems > > to be pointing in this direction too. > > > > "* We use runtime PM to trigger power on/off of the upstream GPC > > * domain, as a strict hierarchical parent/child power domain > > * setup doesn't allow us to meet the sequencing requirements......" > > > > I am wondering about what those "sequencing requirements" are - and > > whether it could make better sense to fix these issues instead? > > Here is the lockdep splat: > > https://lore.kernel.org/all/Y1cs++TV2GCuh4tS@pendragon.ideasonboard.com/ Yes, that certainly helped! > > It really is a problem between the clock and genpd subsystem locks, they > can be claimed in arbitrary order, see patch 2/3 and 3/3. > > I think that might clarify what I am attempting to solve here. Let me try to put some more words behind this, to make sure I have understood correctly, but also to easier allow more people to chim in. Note that, in your commit messages in patch2 and patch3, you are mentioning clk_disable_unused(), but that's not what the lockdep splat above is pointing at. Although, it seems the clk_disable_unused() thingy, would trigger a similar problem for this configuration for the imx8mp platform. Case #1: Triggered from the workqueue, the genpd_power_off_work_fn() ends up calling clk_bulk_unprepare(), from a genpd's ->power_off() callback(), which has been assigned to imx8mp_blk_ctrl_power_off(). Before genpd's ->power_off() is called, the genpd-lock(s) have been acquired, thus we are trying to acquire the global clk-prepare lock via clk_bulk_unprepare() while holding the genpd-lock(s). Case #0: The "drm driver" calls clk_set_rate(), thus we start by acquiring the global clk-prepare lock. Internally in the clock frameworks, the clk_set_rate() path continues to call clk_pm_runtime_get(). In this case, the corresponding clock provider's struct *device, seems to be attached to a genpd too. This means the call to clk_pm_runtime_get() ends up in genpd_runtime_resume(), which needs to acquire the genpd-lock(s) before it continues to call genpd_power_on() to power-on the PM domain. In other words, we try to acquire genpd-lock(s) while holding the global clk-prepare lock. The solution to fix these problems that you suggest in the $subject patch, isn't the direction I want this to take. The new callbacks are prone to be abused and it would also require genpd provider specific code to fix the problems. Of course, we need things to work, but let's look at a couple of options first. See below. 1) In a way, it looks like we have a circular description in DT of the hierarchy of the clock- and genpd providers, which is a bit awkward in my opinion. I was browsing the imx8mp DTS files to find this, but I couldn't. Can you perhaps point me to the DTS file(s) you are using? I can try to have a look so see if this could be arranged differently. 2) What we have seen from other use cases [1], is that calling pm_runtime_get|put*(), while holding subsystem specific locks (like the genpd-lock(s) and clk-prepare lock), isn't working very well. So, I am thinking that we could have a look at the runtime PM deployment in the clock framework, to see if we can avoid holding the global clk-prepare lock, while calling into runtime PM. I believe that should fix these problems too. Kind regards Uffe [1] https://lore.kernel.org/all/20221103183030.3594899-1-swboyd@chromium.org/ https://lore.kernel.org/all/CAE-0n52xbZeJ66RaKwggeRB57fUAwjvxGxfFMKOKJMKVyFTe+w@mail.gmail.com/
Hi Ulf, Am Mittwoch, dem 16.11.2022 um 13:41 +0100 schrieb Ulf Hansson: > + Stephen Boyd > > On Mon, 14 Nov 2022 at 21:32, Marek Vasut <marex@denx.de> wrote: > > > > On 11/14/22 20:40, Ulf Hansson wrote: > > > On Tue, 8 Nov 2022 at 02:35, Marek Vasut <marex@denx.de> wrote: > > > > > > > > Currently it is possible that a power domain power on or off would claim > > > > the genpd lock first and clock core prepare_lock second, while another > > > > thread could do the reverse, and this would trigger lockdep warning. > > > > > > I am not quite sure I fully understand. In this case is the lockdep > > > warning relevant or just something that we want to silence? > > > > This is a valid problem, see patches 2/3 and 3/3 for details too. > > > > > > Introduce new callbacks, .power_pre/post_on() and .power_off_pre/post(), which > > > > are triggered before the genpd_lock() and after genpd_unlock() respectively in > > > > case the domain is powered on and off. Those are meant to let drivers claim > > > > clock core prepare_lock via clk_*prepare() call and release the lock via > > > > clk_*unprepare() call to always assure that the clock and genpd lock ordering > > > > is correct. > > > > > > To me, this sounds like a problem that may be better fixed by trying > > > to model the parent/child-domains in a more strict way, through genpd. > > > > > > There is a comment in the code in imx8mp_blk_ctrl_probe() that seems > > > to be pointing in this direction too. > > > > > > "* We use runtime PM to trigger power on/off of the upstream GPC > > > * domain, as a strict hierarchical parent/child power domain > > > * setup doesn't allow us to meet the sequencing requirements......" > > > > > > I am wondering about what those "sequencing requirements" are - and > > > whether it could make better sense to fix these issues instead? > > > > Here is the lockdep splat: > > > > https://lore.kernel.org/all/Y1cs++TV2GCuh4tS@pendragon.ideasonboard.com/ > > Yes, that certainly helped! > > > > > It really is a problem between the clock and genpd subsystem locks, they > > can be claimed in arbitrary order, see patch 2/3 and 3/3. > > > > I think that might clarify what I am attempting to solve here. > > Let me try to put some more words behind this, to make sure I have > understood correctly, but also to easier allow more people to chim in. > > Note that, in your commit messages in patch2 and patch3, you are > mentioning clk_disable_unused(), but that's not what the lockdep splat > above is pointing at. Although, it seems the clk_disable_unused() > thingy, would trigger a similar problem for this configuration for the > imx8mp platform. > > Case #1: > Triggered from the workqueue, the genpd_power_off_work_fn() ends up > calling clk_bulk_unprepare(), from a genpd's ->power_off() callback(), > which has been assigned to imx8mp_blk_ctrl_power_off(). Before genpd's > ->power_off() is called, the genpd-lock(s) have been acquired, thus we > are trying to acquire the global clk-prepare lock via > clk_bulk_unprepare() while holding the genpd-lock(s). > > Case #0: > The "drm driver" calls clk_set_rate(), thus we start by acquiring the > global clk-prepare lock. Internally in the clock frameworks, the > clk_set_rate() path continues to call clk_pm_runtime_get(). In this > case, the corresponding clock provider's struct *device, seems to be > attached to a genpd too. This means the call to clk_pm_runtime_get() > ends up in genpd_runtime_resume(), which needs to acquire the > genpd-lock(s) before it continues to call genpd_power_on() to power-on > the PM domain. In other words, we try to acquire genpd-lock(s) while > holding the global clk-prepare lock. > > The solution to fix these problems that you suggest in the $subject > patch, isn't the direction I want this to take. The new callbacks are > prone to be abused and it would also require genpd provider specific > code to fix the problems. Of course, we need things to work, but let's > look at a couple of options first. See below. > > 1) > In a way, it looks like we have a circular description in DT of the > hierarchy of the clock- and genpd providers, which is a bit awkward in > my opinion. I was browsing the imx8mp DTS files to find this, but I > couldn't. Can you perhaps point me to the DTS file(s) you are using? I > can try to have a look so see if this could be arranged differently. The dependency chain isn't circular, it just happens to converge in the clock framework and its single big hammer lock. The chain looks some thing like this: 1. DRM driver request pixel clock (clk_prepare_enable -> clk_prepare_mutex) 2. Pixel clock is supplied from the PHY, which is in a power domain, so in order to supply the clock it needs to runtime resume 3. genpd powers up the PHY blk-ctrl domain, which again is inside a GPCv2 power domain 4. genpd powers up GPCv2 domain, which needs a specific clock to be running in order to power up the domain, so it does a clk_prepare_enable, which now happens to hit the clk_prepare_mutex taken in step 1. As the runtime resume/suspend for the PHY may go through a workqueue we have two different contexts trying to take the clk_prepare_mutex, which is what lockdep complains about. > > 2) > What we have seen from other use cases [1], is that calling > pm_runtime_get|put*(), while holding subsystem specific locks (like > the genpd-lock(s) and clk-prepare lock), isn't working very well. So, > I am thinking that we could have a look at the runtime PM deployment > in the clock framework, to see if we can avoid holding the global > clk-prepare lock, while calling into runtime PM. I believe that should > fix these problems too. I don't see any straight forward way to avoid the clock framework calls in the chain laid out above. I would be happy if anyone has some good suggestions. Regards, Lucas
On Wed, 16 Nov 2022 at 14:25, Lucas Stach <l.stach@pengutronix.de> wrote: > > Hi Ulf, > > Am Mittwoch, dem 16.11.2022 um 13:41 +0100 schrieb Ulf Hansson: > > + Stephen Boyd > > > > On Mon, 14 Nov 2022 at 21:32, Marek Vasut <marex@denx.de> wrote: > > > > > > On 11/14/22 20:40, Ulf Hansson wrote: > > > > On Tue, 8 Nov 2022 at 02:35, Marek Vasut <marex@denx.de> wrote: > > > > > > > > > > Currently it is possible that a power domain power on or off would claim > > > > > the genpd lock first and clock core prepare_lock second, while another > > > > > thread could do the reverse, and this would trigger lockdep warning. > > > > > > > > I am not quite sure I fully understand. In this case is the lockdep > > > > warning relevant or just something that we want to silence? > > > > > > This is a valid problem, see patches 2/3 and 3/3 for details too. > > > > > > > > Introduce new callbacks, .power_pre/post_on() and .power_off_pre/post(), which > > > > > are triggered before the genpd_lock() and after genpd_unlock() respectively in > > > > > case the domain is powered on and off. Those are meant to let drivers claim > > > > > clock core prepare_lock via clk_*prepare() call and release the lock via > > > > > clk_*unprepare() call to always assure that the clock and genpd lock ordering > > > > > is correct. > > > > > > > > To me, this sounds like a problem that may be better fixed by trying > > > > to model the parent/child-domains in a more strict way, through genpd. > > > > > > > > There is a comment in the code in imx8mp_blk_ctrl_probe() that seems > > > > to be pointing in this direction too. > > > > > > > > "* We use runtime PM to trigger power on/off of the upstream GPC > > > > * domain, as a strict hierarchical parent/child power domain > > > > * setup doesn't allow us to meet the sequencing requirements......" > > > > > > > > I am wondering about what those "sequencing requirements" are - and > > > > whether it could make better sense to fix these issues instead? > > > > > > Here is the lockdep splat: > > > > > > https://lore.kernel.org/all/Y1cs++TV2GCuh4tS@pendragon.ideasonboard.com/ > > > > Yes, that certainly helped! > > > > > > > > It really is a problem between the clock and genpd subsystem locks, they > > > can be claimed in arbitrary order, see patch 2/3 and 3/3. > > > > > > I think that might clarify what I am attempting to solve here. > > > > Let me try to put some more words behind this, to make sure I have > > understood correctly, but also to easier allow more people to chim in. > > > > Note that, in your commit messages in patch2 and patch3, you are > > mentioning clk_disable_unused(), but that's not what the lockdep splat > > above is pointing at. Although, it seems the clk_disable_unused() > > thingy, would trigger a similar problem for this configuration for the > > imx8mp platform. > > > > Case #1: > > Triggered from the workqueue, the genpd_power_off_work_fn() ends up > > calling clk_bulk_unprepare(), from a genpd's ->power_off() callback(), > > which has been assigned to imx8mp_blk_ctrl_power_off(). Before genpd's > > ->power_off() is called, the genpd-lock(s) have been acquired, thus we > > are trying to acquire the global clk-prepare lock via > > clk_bulk_unprepare() while holding the genpd-lock(s). > > > > Case #0: > > The "drm driver" calls clk_set_rate(), thus we start by acquiring the > > global clk-prepare lock. Internally in the clock frameworks, the > > clk_set_rate() path continues to call clk_pm_runtime_get(). In this > > case, the corresponding clock provider's struct *device, seems to be > > attached to a genpd too. This means the call to clk_pm_runtime_get() > > ends up in genpd_runtime_resume(), which needs to acquire the > > genpd-lock(s) before it continues to call genpd_power_on() to power-on > > the PM domain. In other words, we try to acquire genpd-lock(s) while > > holding the global clk-prepare lock. > > > > The solution to fix these problems that you suggest in the $subject > > patch, isn't the direction I want this to take. The new callbacks are > > prone to be abused and it would also require genpd provider specific > > code to fix the problems. Of course, we need things to work, but let's > > look at a couple of options first. See below. > > > > 1) > > In a way, it looks like we have a circular description in DT of the > > hierarchy of the clock- and genpd providers, which is a bit awkward in > > my opinion. I was browsing the imx8mp DTS files to find this, but I > > couldn't. Can you perhaps point me to the DTS file(s) you are using? I > > can try to have a look so see if this could be arranged differently. > > The dependency chain isn't circular, it just happens to converge in the > clock framework and its single big hammer lock. The chain looks some > thing like this: > > 1. DRM driver request pixel clock (clk_prepare_enable -> > clk_prepare_mutex) > 2. Pixel clock is supplied from the PHY, which is in a power domain, so > in order to supply the clock it needs to runtime resume > 3. genpd powers up the PHY blk-ctrl domain, which again is inside a > GPCv2 power domain > 4. genpd powers up GPCv2 domain, which needs a specific clock to be > running in order to power up the domain, so it does a > clk_prepare_enable, which now happens to hit the clk_prepare_mutex > taken in step 1. > > As the runtime resume/suspend for the PHY may go through a workqueue we > have two different contexts trying to take the clk_prepare_mutex, which > is what lockdep complains about. I see. Thanks for bringing some more clarity in this. So the above is described in some of the in-kernel DTS(i) files too? > > > > > 2) > > What we have seen from other use cases [1], is that calling > > pm_runtime_get|put*(), while holding subsystem specific locks (like > > the genpd-lock(s) and clk-prepare lock), isn't working very well. So, > > I am thinking that we could have a look at the runtime PM deployment > > in the clock framework, to see if we can avoid holding the global > > clk-prepare lock, while calling into runtime PM. I believe that should > > fix these problems too. > > I don't see any straight forward way to avoid the clock framework calls > in the chain laid out above. I would be happy if anyone has some good > suggestions. I think you misunderstood what I proposed above. What I was suggesting is that we could re-work the runtime PM deployment in the clock framework. In this way, we would not be holding the global clk-prepare lock while trying to power-on the phy and its PM domain. Wouldn't that work? Kind regards Uffe
Hi Ulf, On 11/17/2022 12:30 AM, Ulf Hansson wrote: > On Wed, 16 Nov 2022 at 14:25, Lucas Stach <l.stach@pengutronix.de> wrote: >> >> Hi Ulf, >> >> Am Mittwoch, dem 16.11.2022 um 13:41 +0100 schrieb Ulf Hansson: >>> + Stephen Boyd >>> >>> On Mon, 14 Nov 2022 at 21:32, Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 11/14/22 20:40, Ulf Hansson wrote: >>>>> On Tue, 8 Nov 2022 at 02:35, Marek Vasut <marex@denx.de> wrote: >>>>>> >>>>>> Currently it is possible that a power domain power on or off would claim >>>>>> the genpd lock first and clock core prepare_lock second, while another >>>>>> thread could do the reverse, and this would trigger lockdep warning. >>>>> >>>>> I am not quite sure I fully understand. In this case is the lockdep >>>>> warning relevant or just something that we want to silence? >>>> >>>> This is a valid problem, see patches 2/3 and 3/3 for details too. >>>> >>>>>> Introduce new callbacks, .power_pre/post_on() and .power_off_pre/post(), which >>>>>> are triggered before the genpd_lock() and after genpd_unlock() respectively in >>>>>> case the domain is powered on and off. Those are meant to let drivers claim >>>>>> clock core prepare_lock via clk_*prepare() call and release the lock via >>>>>> clk_*unprepare() call to always assure that the clock and genpd lock ordering >>>>>> is correct. >>>>> >>>>> To me, this sounds like a problem that may be better fixed by trying >>>>> to model the parent/child-domains in a more strict way, through genpd. >>>>> >>>>> There is a comment in the code in imx8mp_blk_ctrl_probe() that seems >>>>> to be pointing in this direction too. >>>>> >>>>> "* We use runtime PM to trigger power on/off of the upstream GPC >>>>> * domain, as a strict hierarchical parent/child power domain >>>>> * setup doesn't allow us to meet the sequencing requirements......" >>>>> >>>>> I am wondering about what those "sequencing requirements" are - and >>>>> whether it could make better sense to fix these issues instead? >>>> >>>> Here is the lockdep splat: >>>> >>>> https://lore.kernel.org/all/Y1cs++TV2GCuh4tS@pendragon.ideasonboard.com/ >>> >>> Yes, that certainly helped! >>> >>>> >>>> It really is a problem between the clock and genpd subsystem locks, they >>>> can be claimed in arbitrary order, see patch 2/3 and 3/3. >>>> >>>> I think that might clarify what I am attempting to solve here. >>> >>> Let me try to put some more words behind this, to make sure I have >>> understood correctly, but also to easier allow more people to chim in. >>> >>> Note that, in your commit messages in patch2 and patch3, you are >>> mentioning clk_disable_unused(), but that's not what the lockdep splat >>> above is pointing at. Although, it seems the clk_disable_unused() >>> thingy, would trigger a similar problem for this configuration for the >>> imx8mp platform. >>> >>> Case #1: >>> Triggered from the workqueue, the genpd_power_off_work_fn() ends up >>> calling clk_bulk_unprepare(), from a genpd's ->power_off() callback(), >>> which has been assigned to imx8mp_blk_ctrl_power_off(). Before genpd's >>> ->power_off() is called, the genpd-lock(s) have been acquired, thus we >>> are trying to acquire the global clk-prepare lock via >>> clk_bulk_unprepare() while holding the genpd-lock(s). >>> >>> Case #0: >>> The "drm driver" calls clk_set_rate(), thus we start by acquiring the >>> global clk-prepare lock. Internally in the clock frameworks, the >>> clk_set_rate() path continues to call clk_pm_runtime_get(). In this >>> case, the corresponding clock provider's struct *device, seems to be >>> attached to a genpd too. This means the call to clk_pm_runtime_get() >>> ends up in genpd_runtime_resume(), which needs to acquire the >>> genpd-lock(s) before it continues to call genpd_power_on() to power-on >>> the PM domain. In other words, we try to acquire genpd-lock(s) while >>> holding the global clk-prepare lock. >>> >>> The solution to fix these problems that you suggest in the $subject >>> patch, isn't the direction I want this to take. The new callbacks are >>> prone to be abused and it would also require genpd provider specific >>> code to fix the problems. Of course, we need things to work, but let's >>> look at a couple of options first. See below. >>> >>> 1) >>> In a way, it looks like we have a circular description in DT of the >>> hierarchy of the clock- and genpd providers, which is a bit awkward in >>> my opinion. I was browsing the imx8mp DTS files to find this, but I >>> couldn't. Can you perhaps point me to the DTS file(s) you are using? I >>> can try to have a look so see if this could be arranged differently. >> >> The dependency chain isn't circular, it just happens to converge in the >> clock framework and its single big hammer lock. The chain looks some >> thing like this: >> >> 1. DRM driver request pixel clock (clk_prepare_enable -> >> clk_prepare_mutex) >> 2. Pixel clock is supplied from the PHY, which is in a power domain, so >> in order to supply the clock it needs to runtime resume >> 3. genpd powers up the PHY blk-ctrl domain, which again is inside a >> GPCv2 power domain >> 4. genpd powers up GPCv2 domain, which needs a specific clock to be >> running in order to power up the domain, so it does a >> clk_prepare_enable, which now happens to hit the clk_prepare_mutex >> taken in step 1. >> >> As the runtime resume/suspend for the PHY may go through a workqueue we >> have two different contexts trying to take the clk_prepare_mutex, which >> is what lockdep complains about. > > I see. Thanks for bringing some more clarity in this. > > So the above is described in some of the in-kernel DTS(i) files too? > >> >>> >>> 2) >>> What we have seen from other use cases [1], is that calling >>> pm_runtime_get|put*(), while holding subsystem specific locks (like >>> the genpd-lock(s) and clk-prepare lock), isn't working very well. So, >>> I am thinking that we could have a look at the runtime PM deployment >>> in the clock framework, to see if we can avoid holding the global >>> clk-prepare lock, while calling into runtime PM. I believe that should >>> fix these problems too. >> >> I don't see any straight forward way to avoid the clock framework calls >> in the chain laid out above. I would be happy if anyone has some good >> suggestions. > > I think you misunderstood what I proposed above. What I was suggesting > is that we could re-work the runtime PM deployment in the clock > framework. > > In this way, we would not be holding the global clk-prepare lock while > trying to power-on the phy and its PM domain. Wouldn't that work? Do you have time to give a look at NXP downstream patch to address the lock issue in genpd side [1]? Honestly I not have idea on how to rework the clk part to avoid holding the clk-prepare lock when power on the phy and its power domain. [1]https://github.com/nxp-imx/linux-imx/commit/c85126180a10439a8c7db1800529b4857d91c609 Thanks, Peng. > > Kind regards > Uffe
On Wed, 4 Jan 2023 at 09:38, Peng Fan <peng.fan@oss.nxp.com> wrote: > > Hi Ulf, > > On 11/17/2022 12:30 AM, Ulf Hansson wrote: > > On Wed, 16 Nov 2022 at 14:25, Lucas Stach <l.stach@pengutronix.de> wrote: > >> > >> Hi Ulf, > >> > >> Am Mittwoch, dem 16.11.2022 um 13:41 +0100 schrieb Ulf Hansson: > >>> + Stephen Boyd > >>> > >>> On Mon, 14 Nov 2022 at 21:32, Marek Vasut <marex@denx.de> wrote: > >>>> > >>>> On 11/14/22 20:40, Ulf Hansson wrote: > >>>>> On Tue, 8 Nov 2022 at 02:35, Marek Vasut <marex@denx.de> wrote: > >>>>>> > >>>>>> Currently it is possible that a power domain power on or off would claim > >>>>>> the genpd lock first and clock core prepare_lock second, while another > >>>>>> thread could do the reverse, and this would trigger lockdep warning. > >>>>> > >>>>> I am not quite sure I fully understand. In this case is the lockdep > >>>>> warning relevant or just something that we want to silence? > >>>> > >>>> This is a valid problem, see patches 2/3 and 3/3 for details too. > >>>> > >>>>>> Introduce new callbacks, .power_pre/post_on() and .power_off_pre/post(), which > >>>>>> are triggered before the genpd_lock() and after genpd_unlock() respectively in > >>>>>> case the domain is powered on and off. Those are meant to let drivers claim > >>>>>> clock core prepare_lock via clk_*prepare() call and release the lock via > >>>>>> clk_*unprepare() call to always assure that the clock and genpd lock ordering > >>>>>> is correct. > >>>>> > >>>>> To me, this sounds like a problem that may be better fixed by trying > >>>>> to model the parent/child-domains in a more strict way, through genpd. > >>>>> > >>>>> There is a comment in the code in imx8mp_blk_ctrl_probe() that seems > >>>>> to be pointing in this direction too. > >>>>> > >>>>> "* We use runtime PM to trigger power on/off of the upstream GPC > >>>>> * domain, as a strict hierarchical parent/child power domain > >>>>> * setup doesn't allow us to meet the sequencing requirements......" > >>>>> > >>>>> I am wondering about what those "sequencing requirements" are - and > >>>>> whether it could make better sense to fix these issues instead? > >>>> > >>>> Here is the lockdep splat: > >>>> > >>>> https://lore.kernel.org/all/Y1cs++TV2GCuh4tS@pendragon.ideasonboard.com/ > >>> > >>> Yes, that certainly helped! > >>> > >>>> > >>>> It really is a problem between the clock and genpd subsystem locks, they > >>>> can be claimed in arbitrary order, see patch 2/3 and 3/3. > >>>> > >>>> I think that might clarify what I am attempting to solve here. > >>> > >>> Let me try to put some more words behind this, to make sure I have > >>> understood correctly, but also to easier allow more people to chim in. > >>> > >>> Note that, in your commit messages in patch2 and patch3, you are > >>> mentioning clk_disable_unused(), but that's not what the lockdep splat > >>> above is pointing at. Although, it seems the clk_disable_unused() > >>> thingy, would trigger a similar problem for this configuration for the > >>> imx8mp platform. > >>> > >>> Case #1: > >>> Triggered from the workqueue, the genpd_power_off_work_fn() ends up > >>> calling clk_bulk_unprepare(), from a genpd's ->power_off() callback(), > >>> which has been assigned to imx8mp_blk_ctrl_power_off(). Before genpd's > >>> ->power_off() is called, the genpd-lock(s) have been acquired, thus we > >>> are trying to acquire the global clk-prepare lock via > >>> clk_bulk_unprepare() while holding the genpd-lock(s). > >>> > >>> Case #0: > >>> The "drm driver" calls clk_set_rate(), thus we start by acquiring the > >>> global clk-prepare lock. Internally in the clock frameworks, the > >>> clk_set_rate() path continues to call clk_pm_runtime_get(). In this > >>> case, the corresponding clock provider's struct *device, seems to be > >>> attached to a genpd too. This means the call to clk_pm_runtime_get() > >>> ends up in genpd_runtime_resume(), which needs to acquire the > >>> genpd-lock(s) before it continues to call genpd_power_on() to power-on > >>> the PM domain. In other words, we try to acquire genpd-lock(s) while > >>> holding the global clk-prepare lock. > >>> > >>> The solution to fix these problems that you suggest in the $subject > >>> patch, isn't the direction I want this to take. The new callbacks are > >>> prone to be abused and it would also require genpd provider specific > >>> code to fix the problems. Of course, we need things to work, but let's > >>> look at a couple of options first. See below. > >>> > >>> 1) > >>> In a way, it looks like we have a circular description in DT of the > >>> hierarchy of the clock- and genpd providers, which is a bit awkward in > >>> my opinion. I was browsing the imx8mp DTS files to find this, but I > >>> couldn't. Can you perhaps point me to the DTS file(s) you are using? I > >>> can try to have a look so see if this could be arranged differently. > >> > >> The dependency chain isn't circular, it just happens to converge in the > >> clock framework and its single big hammer lock. The chain looks some > >> thing like this: > >> > >> 1. DRM driver request pixel clock (clk_prepare_enable -> > >> clk_prepare_mutex) > >> 2. Pixel clock is supplied from the PHY, which is in a power domain, so > >> in order to supply the clock it needs to runtime resume > >> 3. genpd powers up the PHY blk-ctrl domain, which again is inside a > >> GPCv2 power domain > >> 4. genpd powers up GPCv2 domain, which needs a specific clock to be > >> running in order to power up the domain, so it does a > >> clk_prepare_enable, which now happens to hit the clk_prepare_mutex > >> taken in step 1. > >> > >> As the runtime resume/suspend for the PHY may go through a workqueue we > >> have two different contexts trying to take the clk_prepare_mutex, which > >> is what lockdep complains about. > > > > I see. Thanks for bringing some more clarity in this. > > > > So the above is described in some of the in-kernel DTS(i) files too? > > > >> > >>> > >>> 2) > >>> What we have seen from other use cases [1], is that calling > >>> pm_runtime_get|put*(), while holding subsystem specific locks (like > >>> the genpd-lock(s) and clk-prepare lock), isn't working very well. So, > >>> I am thinking that we could have a look at the runtime PM deployment > >>> in the clock framework, to see if we can avoid holding the global > >>> clk-prepare lock, while calling into runtime PM. I believe that should > >>> fix these problems too. > >> > >> I don't see any straight forward way to avoid the clock framework calls > >> in the chain laid out above. I would be happy if anyone has some good > >> suggestions. > > > > I think you misunderstood what I proposed above. What I was suggesting > > is that we could re-work the runtime PM deployment in the clock > > framework. > > > > In this way, we would not be holding the global clk-prepare lock while > > trying to power-on the phy and its PM domain. Wouldn't that work? > > > Do you have time to give a look at NXP downstream patch to address the > lock issue in genpd side [1]? Thanks for sharing this! I had a look and the approach seems like it could work for you. However, I am still not sure it's the correct thing to do. > > Honestly I not have idea on how to rework the clk part to avoid holding > the clk-prepare lock when power on the phy and its power domain. I understand. Let me try to prepare a hackish patch that we can use to run some tests. Unfortunately, I can't make any promises for when, but I will do my best. > > [1]https://github.com/nxp-imx/linux-imx/commit/c85126180a10439a8c7db1800529b4857d91c609 > > Thanks, > Peng. > > > > Kind regards > > Uffe Kind regards Uffe
On 1/18/23 13:55, Ulf Hansson wrote: > On Wed, 4 Jan 2023 at 09:38, Peng Fan <peng.fan@oss.nxp.com> wrote: >> >> Hi Ulf, >> >> On 11/17/2022 12:30 AM, Ulf Hansson wrote: >>> On Wed, 16 Nov 2022 at 14:25, Lucas Stach <l.stach@pengutronix.de> wrote: >>>> >>>> Hi Ulf, >>>> >>>> Am Mittwoch, dem 16.11.2022 um 13:41 +0100 schrieb Ulf Hansson: >>>>> + Stephen Boyd >>>>> >>>>> On Mon, 14 Nov 2022 at 21:32, Marek Vasut <marex@denx.de> wrote: >>>>>> >>>>>> On 11/14/22 20:40, Ulf Hansson wrote: >>>>>>> On Tue, 8 Nov 2022 at 02:35, Marek Vasut <marex@denx.de> wrote: >>>>>>>> >>>>>>>> Currently it is possible that a power domain power on or off would claim >>>>>>>> the genpd lock first and clock core prepare_lock second, while another >>>>>>>> thread could do the reverse, and this would trigger lockdep warning. >>>>>>> >>>>>>> I am not quite sure I fully understand. In this case is the lockdep >>>>>>> warning relevant or just something that we want to silence? >>>>>> >>>>>> This is a valid problem, see patches 2/3 and 3/3 for details too. >>>>>> >>>>>>>> Introduce new callbacks, .power_pre/post_on() and .power_off_pre/post(), which >>>>>>>> are triggered before the genpd_lock() and after genpd_unlock() respectively in >>>>>>>> case the domain is powered on and off. Those are meant to let drivers claim >>>>>>>> clock core prepare_lock via clk_*prepare() call and release the lock via >>>>>>>> clk_*unprepare() call to always assure that the clock and genpd lock ordering >>>>>>>> is correct. >>>>>>> >>>>>>> To me, this sounds like a problem that may be better fixed by trying >>>>>>> to model the parent/child-domains in a more strict way, through genpd. >>>>>>> >>>>>>> There is a comment in the code in imx8mp_blk_ctrl_probe() that seems >>>>>>> to be pointing in this direction too. >>>>>>> >>>>>>> "* We use runtime PM to trigger power on/off of the upstream GPC >>>>>>> * domain, as a strict hierarchical parent/child power domain >>>>>>> * setup doesn't allow us to meet the sequencing requirements......" >>>>>>> >>>>>>> I am wondering about what those "sequencing requirements" are - and >>>>>>> whether it could make better sense to fix these issues instead? >>>>>> >>>>>> Here is the lockdep splat: >>>>>> >>>>>> https://lore.kernel.org/all/Y1cs++TV2GCuh4tS@pendragon.ideasonboard.com/ >>>>> >>>>> Yes, that certainly helped! >>>>> >>>>>> >>>>>> It really is a problem between the clock and genpd subsystem locks, they >>>>>> can be claimed in arbitrary order, see patch 2/3 and 3/3. >>>>>> >>>>>> I think that might clarify what I am attempting to solve here. >>>>> >>>>> Let me try to put some more words behind this, to make sure I have >>>>> understood correctly, but also to easier allow more people to chim in. >>>>> >>>>> Note that, in your commit messages in patch2 and patch3, you are >>>>> mentioning clk_disable_unused(), but that's not what the lockdep splat >>>>> above is pointing at. Although, it seems the clk_disable_unused() >>>>> thingy, would trigger a similar problem for this configuration for the >>>>> imx8mp platform. >>>>> >>>>> Case #1: >>>>> Triggered from the workqueue, the genpd_power_off_work_fn() ends up >>>>> calling clk_bulk_unprepare(), from a genpd's ->power_off() callback(), >>>>> which has been assigned to imx8mp_blk_ctrl_power_off(). Before genpd's >>>>> ->power_off() is called, the genpd-lock(s) have been acquired, thus we >>>>> are trying to acquire the global clk-prepare lock via >>>>> clk_bulk_unprepare() while holding the genpd-lock(s). >>>>> >>>>> Case #0: >>>>> The "drm driver" calls clk_set_rate(), thus we start by acquiring the >>>>> global clk-prepare lock. Internally in the clock frameworks, the >>>>> clk_set_rate() path continues to call clk_pm_runtime_get(). In this >>>>> case, the corresponding clock provider's struct *device, seems to be >>>>> attached to a genpd too. This means the call to clk_pm_runtime_get() >>>>> ends up in genpd_runtime_resume(), which needs to acquire the >>>>> genpd-lock(s) before it continues to call genpd_power_on() to power-on >>>>> the PM domain. In other words, we try to acquire genpd-lock(s) while >>>>> holding the global clk-prepare lock. >>>>> >>>>> The solution to fix these problems that you suggest in the $subject >>>>> patch, isn't the direction I want this to take. The new callbacks are >>>>> prone to be abused and it would also require genpd provider specific >>>>> code to fix the problems. Of course, we need things to work, but let's >>>>> look at a couple of options first. See below. >>>>> >>>>> 1) >>>>> In a way, it looks like we have a circular description in DT of the >>>>> hierarchy of the clock- and genpd providers, which is a bit awkward in >>>>> my opinion. I was browsing the imx8mp DTS files to find this, but I >>>>> couldn't. Can you perhaps point me to the DTS file(s) you are using? I >>>>> can try to have a look so see if this could be arranged differently. >>>> >>>> The dependency chain isn't circular, it just happens to converge in the >>>> clock framework and its single big hammer lock. The chain looks some >>>> thing like this: >>>> >>>> 1. DRM driver request pixel clock (clk_prepare_enable -> >>>> clk_prepare_mutex) >>>> 2. Pixel clock is supplied from the PHY, which is in a power domain, so >>>> in order to supply the clock it needs to runtime resume >>>> 3. genpd powers up the PHY blk-ctrl domain, which again is inside a >>>> GPCv2 power domain >>>> 4. genpd powers up GPCv2 domain, which needs a specific clock to be >>>> running in order to power up the domain, so it does a >>>> clk_prepare_enable, which now happens to hit the clk_prepare_mutex >>>> taken in step 1. >>>> >>>> As the runtime resume/suspend for the PHY may go through a workqueue we >>>> have two different contexts trying to take the clk_prepare_mutex, which >>>> is what lockdep complains about. >>> >>> I see. Thanks for bringing some more clarity in this. >>> >>> So the above is described in some of the in-kernel DTS(i) files too? >>> >>>> >>>>> >>>>> 2) >>>>> What we have seen from other use cases [1], is that calling >>>>> pm_runtime_get|put*(), while holding subsystem specific locks (like >>>>> the genpd-lock(s) and clk-prepare lock), isn't working very well. So, >>>>> I am thinking that we could have a look at the runtime PM deployment >>>>> in the clock framework, to see if we can avoid holding the global >>>>> clk-prepare lock, while calling into runtime PM. I believe that should >>>>> fix these problems too. >>>> >>>> I don't see any straight forward way to avoid the clock framework calls >>>> in the chain laid out above. I would be happy if anyone has some good >>>> suggestions. >>> >>> I think you misunderstood what I proposed above. What I was suggesting >>> is that we could re-work the runtime PM deployment in the clock >>> framework. >>> >>> In this way, we would not be holding the global clk-prepare lock while >>> trying to power-on the phy and its PM domain. Wouldn't that work? >> >> >> Do you have time to give a look at NXP downstream patch to address the >> lock issue in genpd side [1]? > > Thanks for sharing this! I had a look and the approach seems like it > could work for you. However, I am still not sure it's the correct > thing to do. > >> >> Honestly I not have idea on how to rework the clk part to avoid holding >> the clk-prepare lock when power on the phy and its power domain. > > I understand. Let me try to prepare a hackish patch that we can use to > run some tests. > > Unfortunately, I can't make any promises for when, but I will do my best. Thanks !
Hi Ulf, On 1/18/2023 8:55 PM, Ulf Hansson wrote: ... >> >> Honestly I not have idea on how to rework the clk part to avoid holding >> the clk-prepare lock when power on the phy and its power domain. > > I understand. Let me try to prepare a hackish patch that we can use to > run some tests. > > Unfortunately, I can't make any promises for when, but I will do my best. > Have you got time to give a look on this part? Or do you have any suggestions that I could help with? Thanks, Peng. >> >> [1]https://github.com/nxp-imx/linux-imx/commit/c85126180a10439a8c7db1800529b4857d91c609 >> >> Thanks, >> Peng. >>> >>> Kind regards >>> Uffe > > Kind regards > Uffe
On Thu, 16 Feb 2023 at 02:47, Peng Fan <peng.fan@oss.nxp.com> wrote: > > Hi Ulf, > > On 1/18/2023 8:55 PM, Ulf Hansson wrote: > ... > >> > >> Honestly I not have idea on how to rework the clk part to avoid holding > >> the clk-prepare lock when power on the phy and its power domain. > > > > I understand. Let me try to prepare a hackish patch that we can use to > > run some tests. > > > > Unfortunately, I can't make any promises for when, but I will do my best. > > > > Have you got time to give a look on this part? Or do you have any > suggestions that I could help with? I have looked closer at this, but haven't yet got the time to actually post a patch. Let me see if I can do some re-prioritization of things at my side... Anyway, my plan is to post a "hackish" patch that we should use solely for testing, to make sure that the approach works. Then, I intend to continue to work on the complete solution, which seems to be requiring some reworks of locking parts in the clock framework. Although, it's too early for me, to say exactly how, so I can't really give you any pointers in this regard, sorry. > > Thanks, > Peng. > > >> > >> [1]https://github.com/nxp-imx/linux-imx/commit/c85126180a10439a8c7db1800529b4857d91c609 > >> Kind regards Uffe
On 2/16/2023 6:48 PM, Ulf Hansson wrote: > On Thu, 16 Feb 2023 at 02:47, Peng Fan <peng.fan@oss.nxp.com> wrote: >> >> Hi Ulf, >> >> On 1/18/2023 8:55 PM, Ulf Hansson wrote: >> ... >>>> >>>> Honestly I not have idea on how to rework the clk part to avoid holding >>>> the clk-prepare lock when power on the phy and its power domain. >>> >>> I understand. Let me try to prepare a hackish patch that we can use to >>> run some tests. >>> >>> Unfortunately, I can't make any promises for when, but I will do my best. >>> >> >> Have you got time to give a look on this part? Or do you have any >> suggestions that I could help with? > > I have looked closer at this, but haven't yet got the time to actually > post a patch. Let me see if I can do some re-prioritization of things > at my side... > > Anyway, my plan is to post a "hackish" patch that we should use solely > for testing, to make sure that the approach works. Then, I intend to > continue to work on the complete solution, which seems to be requiring > some reworks of locking parts in the clock framework. Although, it's > too early for me, to say exactly how, so I can't really give you any > pointers in this regard, sorry. I overlooked your reply, sorry. Thanks for still working on this. We have a new SoC has similar design that use blk ctrl which will need to work as clock controller, while it depends on power domain. So we will run into same deadlock issue. So I hope the deadlock issue could be resolved. Thanks, Peng. > >> >> Thanks, >> Peng. >> >>>> >>>> [1]https://github.com/nxp-imx/linux-imx/commit/c85126180a10439a8c7db1800529b4857d91c609 >>>> > > Kind regards > Uffe
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 6471b559230e9..df2a93d0674e4 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -494,6 +494,22 @@ void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next) } EXPORT_SYMBOL_GPL(dev_pm_genpd_set_next_wakeup); +static int genpd_power_pre_on(struct generic_pm_domain *genpd) +{ + if (!genpd->power_pre_on) + return 0; + + return genpd->power_pre_on(genpd); +} + +static int genpd_power_post_on(struct generic_pm_domain *genpd) +{ + if (!genpd->power_post_on) + return 0; + + return genpd->power_post_on(genpd); +} + static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) { unsigned int state_idx = genpd->state_idx; @@ -544,6 +560,22 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed) return ret; } +static int genpd_power_off_pre(struct generic_pm_domain *genpd) +{ + if (!genpd->power_off_pre) + return 0; + + return genpd->power_off_pre(genpd); +} + +static int genpd_power_off_post(struct generic_pm_domain *genpd) +{ + if (!genpd->power_off_post) + return 0; + + return genpd->power_off_post(genpd); +} + static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed) { unsigned int state_idx = genpd->state_idx; @@ -816,12 +848,18 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb, static void genpd_power_off_work_fn(struct work_struct *work) { struct generic_pm_domain *genpd; + int ret; genpd = container_of(work, struct generic_pm_domain, power_off_work); + ret = genpd_power_off_pre(genpd); + if (ret) + return; genpd_lock(genpd); genpd_power_off(genpd, false, 0); genpd_unlock(genpd); + ret = genpd_power_off_post(genpd); + WARN_ON_ONCE(ret); } /** @@ -938,12 +976,14 @@ static int genpd_runtime_suspend(struct device *dev) if (irq_safe_dev_in_sleep_domain(dev, genpd)) return 0; + ret = genpd_power_off_pre(genpd); + if (ret) + return ret; genpd_lock(genpd); gpd_data->rpm_pstate = genpd_drop_performance_state(dev); genpd_power_off(genpd, true, 0); genpd_unlock(genpd); - - return 0; + return genpd_power_off_post(genpd); } /** @@ -977,12 +1017,21 @@ static int genpd_runtime_resume(struct device *dev) if (irq_safe_dev_in_sleep_domain(dev, genpd)) goto out; + ret = genpd_power_pre_on(genpd); + if (ret) + return ret; genpd_lock(genpd); ret = genpd_power_on(genpd, 0); if (!ret) genpd_restore_performance_state(dev, gpd_data->rpm_pstate); genpd_unlock(genpd); + if (ret) { + genpd_power_post_on(genpd); + return ret; + } + + ret = genpd_power_post_on(genpd); if (ret) return ret; @@ -1017,10 +1066,13 @@ static int genpd_runtime_resume(struct device *dev) genpd_stop_dev(genpd, dev); err_poweroff: if (!pm_runtime_is_irq_safe(dev) || genpd_is_irq_safe(genpd)) { - genpd_lock(genpd); - gpd_data->rpm_pstate = genpd_drop_performance_state(dev); - genpd_power_off(genpd, true, 0); - genpd_unlock(genpd); + if (!genpd_power_off_pre(genpd)) { + genpd_lock(genpd); + gpd_data->rpm_pstate = genpd_drop_performance_state(dev); + genpd_power_off(genpd, true, 0); + genpd_unlock(genpd); + genpd_power_off_post(genpd); + } } return ret; @@ -1225,12 +1277,14 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff) } } + ret = genpd_power_off_pre(genpd); + if (ret) + return ret; genpd_lock(genpd); genpd->suspended_count++; genpd_sync_power_off(genpd, true, 0); genpd_unlock(genpd); - - return 0; + return genpd_power_off_post(genpd); } /** @@ -1267,10 +1321,16 @@ static int genpd_resume_noirq(struct device *dev) if (device_wakeup_path(dev) && genpd_is_active_wakeup(genpd)) return pm_generic_resume_noirq(dev); + ret = genpd_power_pre_on(genpd); + if (ret) + return ret; genpd_lock(genpd); genpd_sync_power_on(genpd, true, 0); genpd->suspended_count--; genpd_unlock(genpd); + ret = genpd_power_post_on(genpd); + if (ret) + return ret; if (genpd->dev_ops.stop && genpd->dev_ops.start && !pm_runtime_status_suspended(dev)) { @@ -1378,6 +1438,9 @@ static int genpd_restore_noirq(struct device *dev) * At this point suspended_count == 0 means we are being run for the * first time for the given domain in the present cycle. */ + ret = genpd_power_pre_on(genpd); + if (ret) + return ret; genpd_lock(genpd); if (genpd->suspended_count++ == 0) { /* @@ -1390,6 +1453,9 @@ static int genpd_restore_noirq(struct device *dev) genpd_sync_power_on(genpd, true, 0); genpd_unlock(genpd); + ret = genpd_power_post_on(genpd); + if (ret) + return ret; if (genpd->dev_ops.stop && genpd->dev_ops.start && !pm_runtime_status_suspended(dev)) { @@ -1413,6 +1479,7 @@ static int genpd_restore_noirq(struct device *dev) static void genpd_complete(struct device *dev) { struct generic_pm_domain *genpd; + int ret; dev_dbg(dev, "%s()\n", __func__); @@ -1435,6 +1502,7 @@ static void genpd_switch_state(struct device *dev, bool suspend) { struct generic_pm_domain *genpd; bool use_lock; + int ret; genpd = dev_to_genpd_safe(dev); if (!genpd) @@ -1442,8 +1510,13 @@ static void genpd_switch_state(struct device *dev, bool suspend) use_lock = genpd_is_irq_safe(genpd); - if (use_lock) + if (use_lock) { + ret = suspend ? genpd_power_off_pre(genpd) : + genpd_power_pre_on(genpd); + if (ret) + return; genpd_lock(genpd); + } if (suspend) { genpd->suspended_count++; @@ -1453,8 +1526,12 @@ static void genpd_switch_state(struct device *dev, bool suspend) genpd->suspended_count--; } - if (use_lock) + if (use_lock) { genpd_unlock(genpd); + ret = suspend ? genpd_power_off_post(genpd) : + genpd_power_post_on(genpd); + WARN_ON_ONCE(ret); + } } /** @@ -2750,9 +2827,15 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev, dev->pm_domain->sync = genpd_dev_pm_sync; if (power_on) { + ret = genpd_power_pre_on(pd); + if (ret) + return ret; genpd_lock(pd); ret = genpd_power_on(pd, 0); genpd_unlock(pd); + ret = genpd_power_post_on(pd); + if (ret) + return ret; } if (ret) { diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index ebc3516980907..3cf231a27cb1b 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -134,8 +134,12 @@ struct generic_pm_domain { unsigned int prepared_count; /* Suspend counter of prepared devices */ unsigned int performance_state; /* Aggregated max performance state */ cpumask_var_t cpus; /* A cpumask of the attached CPUs */ + int (*power_off_pre)(struct generic_pm_domain *domain); int (*power_off)(struct generic_pm_domain *domain); + int (*power_off_post)(struct generic_pm_domain *domain); + int (*power_pre_on)(struct generic_pm_domain *domain); int (*power_on)(struct generic_pm_domain *domain); + int (*power_post_on)(struct generic_pm_domain *domain); struct raw_notifier_head power_notifiers; /* Power on/off notifiers */ struct opp_table *opp_table; /* OPP table of the genpd */ unsigned int (*opp_to_performance_state)(struct generic_pm_domain *genpd,
Currently it is possible that a power domain power on or off would claim the genpd lock first and clock core prepare_lock second, while another thread could do the reverse, and this would trigger lockdep warning. Introduce new callbacks, .power_pre/post_on() and .power_off_pre/post(), which are triggered before the genpd_lock() and after genpd_unlock() respectively in case the domain is powered on and off. Those are meant to let drivers claim clock core prepare_lock via clk_*prepare() call and release the lock via clk_*unprepare() call to always assure that the clock and genpd lock ordering is correct. Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Adam Ford <aford173@gmail.com> Cc: Fabio Estevam <festevam@denx.de> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Jacky Bai <ping.bai@nxp.com> Cc: Kevin Hilman <khilman@kernel.org> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Cc: Len Brown <len.brown@intel.com> Cc: Liam Girdwood <lgirdwood@gmail.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Marek Vasut <marex@denx.de> Cc: Mark Brown <broonie@kernel.org> Cc: Martin Kepplinger <martink@posteo.de> Cc: Pavel Machek <pavel@ucw.cz> Cc: Peng Fan <peng.fan@nxp.com> Cc: Pengutronix Kernel Team <kernel@pengutronix.de> Cc: Philipp Zabel <p.zabel@pengutronix.de> Cc: Rafael J. Wysocki <rafael@kernel.org> Cc: Sascha Hauer <s.hauer@pengutronix.de> Cc: Shawn Guo <shawnguo@kernel.org> Cc: Shengjiu Wang <shengjiu.wang@nxp.com> Cc: Stephen Boyd <sboyd@kernel.org> Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: linux-clk@vger.kernel.org Cc: linux-imx@nxp.com Cc: linux-pm@vger.kernel.org To: linux-arm-kernel@lists.infradead.org --- drivers/base/power/domain.c | 103 ++++++++++++++++++++++++++++++++---- include/linux/pm_domain.h | 4 ++ 2 files changed, 97 insertions(+), 10 deletions(-)