| Message ID | 20181204192440.12125-2-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add device links to clocks | expand |
Quoting Miquel Raynal (2018-12-04 11:24:37) > Currently, the core->dev entry is populated only if runtime PM is > enabled. Doing so prevents accessing the device structure in any > case. > > Keep the same logic but instead of using the presence of core->dev as > the only condition, also check the status of > pm_runtime_enabled(). Then, we can set the core->dev pointer at any > time as long as a device structure is available. > > This change will help supporting device links in the clock subsystem. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/clk/clk.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > index af011974d4ec..b799347c5fd6 100644 > --- a/drivers/clk/clk.c > +++ b/drivers/clk/clk.c > @@ -97,7 +97,7 @@ static int clk_pm_runtime_get(struct clk_core *core) > { > int ret = 0; > > - if (!core->dev) > + if (!core->dev || !pm_runtime_enabled(core->dev)) This looks potentially dangerous. What if runtime PM is disabled for the clk when this function is called? Shouldn't we just stash a bool away in the clk_core structure when it's registered? And then we can replace the check for !core->dev with a check for 'core->rpm_enabled' instead. That would be a more exact transformation.
Hi Stephen, Stephen Boyd <sboyd@kernel.org> wrote on Tue, 18 Dec 2018 16:03:29 -0800: > Quoting Miquel Raynal (2018-12-04 11:24:37) > > Currently, the core->dev entry is populated only if runtime PM is > > enabled. Doing so prevents accessing the device structure in any > > case. > > > > Keep the same logic but instead of using the presence of core->dev as > > the only condition, also check the status of > > pm_runtime_enabled(). Then, we can set the core->dev pointer at any > > time as long as a device structure is available. > > > > This change will help supporting device links in the clock subsystem. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/clk/clk.c | 11 +++++------ > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > index af011974d4ec..b799347c5fd6 100644 > > --- a/drivers/clk/clk.c > > +++ b/drivers/clk/clk.c > > @@ -97,7 +97,7 @@ static int clk_pm_runtime_get(struct clk_core *core) > > { > > int ret = 0; > > > > - if (!core->dev) > > + if (!core->dev || !pm_runtime_enabled(core->dev)) > > This looks potentially dangerous. What if runtime PM is disabled for the > clk when this function is called? Shouldn't we just stash a bool away in > the clk_core structure when it's registered? And then we can replace the > check for !core->dev with a check for 'core->rpm_enabled' instead. That > would be a more exact transformation. Sure, I'll do that if you think there is a danger. Thanks, Miquèl
Quoting Miquel Raynal (2018-12-19 00:03:31) > Hi Stephen, > > Stephen Boyd <sboyd@kernel.org> wrote on Tue, 18 Dec 2018 16:03:29 > -0800: > > > Quoting Miquel Raynal (2018-12-04 11:24:37) > > > Currently, the core->dev entry is populated only if runtime PM is > > > enabled. Doing so prevents accessing the device structure in any > > > case. > > > > > > Keep the same logic but instead of using the presence of core->dev as > > > the only condition, also check the status of > > > pm_runtime_enabled(). Then, we can set the core->dev pointer at any > > > time as long as a device structure is available. > > > > > > This change will help supporting device links in the clock subsystem. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/clk/clk.c | 11 +++++------ > > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > index af011974d4ec..b799347c5fd6 100644 > > > --- a/drivers/clk/clk.c > > > +++ b/drivers/clk/clk.c > > > @@ -97,7 +97,7 @@ static int clk_pm_runtime_get(struct clk_core *core) > > > { > > > int ret = 0; > > > > > > - if (!core->dev) > > > + if (!core->dev || !pm_runtime_enabled(core->dev)) > > > > This looks potentially dangerous. What if runtime PM is disabled for the > > clk when this function is called? Shouldn't we just stash a bool away in > > the clk_core structure when it's registered? And then we can replace the > > check for !core->dev with a check for 'core->rpm_enabled' instead. That > > would be a more exact transformation. > > Sure, I'll do that if you think there is a danger. I've made the change and pushed it out to the clk tree. I'm working on some patches to change how we do parent lookups and reworking clk_get() in the process. Take a look so we don't duplicate efforts. The code isn't complete, but I hope to finish soon. Your device links patches can build upon this. git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-parent-rewrite
Hi Stephen, Stephen Boyd <sboyd@kernel.org> wrote on Thu, 20 Dec 2018 13:09:19 -0800: > Quoting Miquel Raynal (2018-12-19 00:03:31) > > Hi Stephen, > > > > Stephen Boyd <sboyd@kernel.org> wrote on Tue, 18 Dec 2018 16:03:29 > > -0800: > > > > > Quoting Miquel Raynal (2018-12-04 11:24:37) > > > > Currently, the core->dev entry is populated only if runtime PM is > > > > enabled. Doing so prevents accessing the device structure in any > > > > case. > > > > > > > > Keep the same logic but instead of using the presence of core->dev as > > > > the only condition, also check the status of > > > > pm_runtime_enabled(). Then, we can set the core->dev pointer at any > > > > time as long as a device structure is available. > > > > > > > > This change will help supporting device links in the clock subsystem. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > drivers/clk/clk.c | 11 +++++------ > > > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c > > > > index af011974d4ec..b799347c5fd6 100644 > > > > --- a/drivers/clk/clk.c > > > > +++ b/drivers/clk/clk.c > > > > @@ -97,7 +97,7 @@ static int clk_pm_runtime_get(struct clk_core *core) > > > > { > > > > int ret = 0; > > > > > > > > - if (!core->dev) > > > > + if (!core->dev || !pm_runtime_enabled(core->dev)) > > > > > > This looks potentially dangerous. What if runtime PM is disabled for the > > > clk when this function is called? Shouldn't we just stash a bool away in > > > the clk_core structure when it's registered? And then we can replace the > > > check for !core->dev with a check for 'core->rpm_enabled' instead. That > > > would be a more exact transformation. > > > > Sure, I'll do that if you think there is a danger. > > I've made the change and pushed it out to the clk tree. I'm working on > some patches to change how we do parent lookups and reworking clk_get() > in the process. Take a look so we don't duplicate efforts. The code > isn't complete, but I hope to finish soon. Your device links patches can > build upon this. > > git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-parent-rewrite > Sorry for the delay and thanks for the update. I have been preempted by another issue, I will switch to clocks and propose something ASAP. Thanks, Miquèl
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index af011974d4ec..b799347c5fd6 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -97,7 +97,7 @@ static int clk_pm_runtime_get(struct clk_core *core) { int ret = 0; - if (!core->dev) + if (!core->dev || !pm_runtime_enabled(core->dev)) return 0; ret = pm_runtime_get_sync(core->dev); @@ -106,7 +106,7 @@ static int clk_pm_runtime_get(struct clk_core *core) static void clk_pm_runtime_put(struct clk_core *core) { - if (!core->dev) + if (!core->dev || !pm_runtime_enabled(core->dev)) return; pm_runtime_put_sync(core->dev); @@ -226,7 +226,7 @@ static bool clk_core_is_enabled(struct clk_core *core) * taking enable spinlock, but the below check is needed if one tries * to call it from other places. */ - if (core->dev) { + if (core->dev && pm_runtime_enabled(core->dev)) { pm_runtime_get_noresume(core->dev); if (!pm_runtime_active(core->dev)) { ret = false; @@ -236,7 +236,7 @@ static bool clk_core_is_enabled(struct clk_core *core) ret = core->ops->is_enabled(core->hw); done: - if (core->dev) + if (core->dev && pm_runtime_enabled(core->dev)) pm_runtime_put(core->dev); return ret; @@ -3272,8 +3272,7 @@ struct clk *clk_register(struct device *dev, struct clk_hw *hw) } core->ops = hw->init->ops; - if (dev && pm_runtime_enabled(dev)) - core->dev = dev; + core->dev = dev; if (dev && dev->driver) core->owner = dev->driver->owner; core->hw = hw;
Currently, the core->dev entry is populated only if runtime PM is enabled. Doing so prevents accessing the device structure in any case. Keep the same logic but instead of using the presence of core->dev as the only condition, also check the status of pm_runtime_enabled(). Then, we can set the core->dev pointer at any time as long as a device structure is available. This change will help supporting device links in the clock subsystem. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/clk/clk.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)