| Message ID | 1586848508-1320-1-git-send-email-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] opp: Fixup release of clock when using set/put clkname | expand |
On 14-04-20, 12:45, Rajendra Nayak wrote: > Fixup dev_pm_opp_put_clkname() to check for a valid clock pointer > before it does a clk_put, since its likely that > _opp_table_kref_release() has already done a clk_put. Also fixup kref release is the last thing that happens on the table, it can't get called after dev_pm_opp_put_clkname(). > _opp_table_kref_release() to set the clock pointer to ERR_PTR(-EINVAL) > after its done doing a clk_put so dev_pm_opp_put_clkname() can then > catch it. > > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> > --- > drivers/opp/core.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index e4f01e7..6d064a8 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1061,8 +1061,10 @@ static void _opp_table_kref_release(struct kref *kref) > _of_clear_opp_table(opp_table); > > /* Release clk */ > - if (!IS_ERR(opp_table->clk)) > + if (!IS_ERR(opp_table->clk)) { > clk_put(opp_table->clk); > + opp_table->clk = ERR_PTR(-EINVAL); > + } > > WARN_ON(!list_empty(&opp_table->opp_list)); > > @@ -1744,8 +1746,10 @@ void dev_pm_opp_put_clkname(struct opp_table *opp_table) > /* Make sure there are no concurrent readers while updating opp_table */ > WARN_ON(!list_empty(&opp_table->opp_list)); > > - clk_put(opp_table->clk); > - opp_table->clk = ERR_PTR(-EINVAL); > + if (!IS_ERR(opp_table->clk)) { > + clk_put(opp_table->clk); > + opp_table->clk = ERR_PTR(-EINVAL); > + } > > dev_pm_opp_put_opp_table(opp_table); > } > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation
On 4/14/2020 12:48 PM, Viresh Kumar wrote: > On 14-04-20, 12:45, Rajendra Nayak wrote: >> Fixup dev_pm_opp_put_clkname() to check for a valid clock pointer >> before it does a clk_put, since its likely that >> _opp_table_kref_release() has already done a clk_put. Also fixup > > kref release is the last thing that happens on the table, it can't get > called after dev_pm_opp_put_clkname(). so in cases where I do have an OPP table for a device in DT and I do dev_pm_opp_set_clkname() dev_pm_opp_of_add_table() and clean it up with a dev_pm_opp_of_remove_table() dev_pm_opp_put_clkname() things seem to work fine as expected. However, given I call these unconditionally in common drivers and on some (old) platforms we really don't have an OPP table (only scale clocks) things get a little tricky. So looks like the ref counting in such cases gets messed up, and we end up with dev_pm_opp_of_remove_table() calling _opp_table_kref_release() and releasing the clock, and then the subsequent call to dev_pm_opp_put_clkname() crashes. >> _opp_table_kref_release() to set the clock pointer to ERR_PTR(-EINVAL) >> after its done doing a clk_put so dev_pm_opp_put_clkname() can then >> catch it. >> >> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> >> --- >> drivers/opp/core.c | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c >> index e4f01e7..6d064a8 100644 >> --- a/drivers/opp/core.c >> +++ b/drivers/opp/core.c >> @@ -1061,8 +1061,10 @@ static void _opp_table_kref_release(struct kref *kref) >> _of_clear_opp_table(opp_table); >> >> /* Release clk */ >> - if (!IS_ERR(opp_table->clk)) >> + if (!IS_ERR(opp_table->clk)) { >> clk_put(opp_table->clk); >> + opp_table->clk = ERR_PTR(-EINVAL); >> + } >> >> WARN_ON(!list_empty(&opp_table->opp_list)); >> >> @@ -1744,8 +1746,10 @@ void dev_pm_opp_put_clkname(struct opp_table *opp_table) >> /* Make sure there are no concurrent readers while updating opp_table */ >> WARN_ON(!list_empty(&opp_table->opp_list)); >> >> - clk_put(opp_table->clk); >> - opp_table->clk = ERR_PTR(-EINVAL); >> + if (!IS_ERR(opp_table->clk)) { >> + clk_put(opp_table->clk); >> + opp_table->clk = ERR_PTR(-EINVAL); >> + } >> >> dev_pm_opp_put_opp_table(opp_table); >> } >> -- >> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member >> of Code Aurora Forum, hosted by The Linux Foundation >
On 14-04-20, 20:35, Rajendra Nayak wrote: > However, given I call these unconditionally in common drivers and on > some (old) platforms we really don't have an OPP table (only scale clocks) > things get a little tricky. So looks like the ref counting in such cases > gets messed up, and we end up with dev_pm_opp_of_remove_table() calling > _opp_table_kref_release() and releasing the clock, and then the subsequent > call to dev_pm_opp_put_clkname() crashes. We aren't supposed to call dev_pm_opp_of_remove_table() if dev_pm_opp_of_add_table() isn't called or it failed. Either we can handle this in core, which I would like to avoid, or just keep a variable to track it in the driver, which will be easy enough I guess.
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index e4f01e7..6d064a8 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1061,8 +1061,10 @@ static void _opp_table_kref_release(struct kref *kref) _of_clear_opp_table(opp_table); /* Release clk */ - if (!IS_ERR(opp_table->clk)) + if (!IS_ERR(opp_table->clk)) { clk_put(opp_table->clk); + opp_table->clk = ERR_PTR(-EINVAL); + } WARN_ON(!list_empty(&opp_table->opp_list)); @@ -1744,8 +1746,10 @@ void dev_pm_opp_put_clkname(struct opp_table *opp_table) /* Make sure there are no concurrent readers while updating opp_table */ WARN_ON(!list_empty(&opp_table->opp_list)); - clk_put(opp_table->clk); - opp_table->clk = ERR_PTR(-EINVAL); + if (!IS_ERR(opp_table->clk)) { + clk_put(opp_table->clk); + opp_table->clk = ERR_PTR(-EINVAL); + } dev_pm_opp_put_opp_table(opp_table); }
Fixup dev_pm_opp_put_clkname() to check for a valid clock pointer before it does a clk_put, since its likely that _opp_table_kref_release() has already done a clk_put. Also fixup _opp_table_kref_release() to set the clock pointer to ERR_PTR(-EINVAL) after its done doing a clk_put so dev_pm_opp_put_clkname() can then catch it. Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> --- drivers/opp/core.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)