| Message ID | 20220715144712.444104-1-windhl@126.com |
|---|---|
| State | New |
| Headers | show |
| Series | OPP: Fix two refcount bugs | expand |
On 15-07-22, 22:47, Liang He wrote: > In drivers/opp/of.c, there are two refcount bugs: > (1) in _of_init_opp_table(), of_put_node() in the last line is not > needed as the 'opp_np' is escaped out into 'opp_table->np' and > ’opp_table' is an out parameter. > (2) in _opp_add_static_v2(), we need call of_node_get() for the new > reference created when "new_opp->np = np;" as new_opp is escaped out. > Here we should also take care of the of_node_put() when 'new_opp' is > freed based on the function description: "The opp can be controlled > ... and may be removed by dev_pm_opp_remove". > NOTE: _opp_add_static_v2() is called by _of_add_opp_table_v2() in a > for_each_available_child_of_node() which will automatically increase > and decrease the refcount. So we need an additional of_node_get() > for the new reference created in _opp_add_static_v2(). > > Fixes: f06ed90e7051 ("OPP: Parse OPP table's DT properties from _of_init_opp_table()") > Fixes: 274659029c9d ("PM / OPP: Add support to parse "operating-points-v2" bindings") The way I designed the OPP core then was to make sure that np is only used for pointer comparison and nothing else after it is freed by calling of_node_put(). So it isn't a bug. But I do understand that it has become difficult to track now if np can get used later on for other stuff as well or not and it would be better to keep the reference up in such a case. That is, you can drop the Fixes tag as I don't want these to get backported, but yes patches are welcome. Please prepare two separate patches, one for opp_table->np and one for opp->np. It is fine to add multiple patches even for the opp->np case, if the reasoning is different. > Signed-off-by: Liang He <windhl@126.com> > --- > drivers/opp/core.c | 1 + > drivers/opp/of.c | 3 +-- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/opp/core.c b/drivers/opp/core.c > index 84063eaebb91..70775362eb05 100644 > --- a/drivers/opp/core.c > +++ b/drivers/opp/core.c > @@ -1576,6 +1576,7 @@ static void _opp_kref_release(struct kref *kref) > list_del(&opp->node); > mutex_unlock(&opp_table->lock); > > + of_node_put(opp->np); > /* > * Notify the changes in the availability of the operable > * frequency/voltage list. > diff --git a/drivers/opp/of.c b/drivers/opp/of.c > index 30394929d700..0a38fc2c0f05 100644 > --- a/drivers/opp/of.c > +++ b/drivers/opp/of.c > @@ -242,7 +242,6 @@ void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, > opp_table->np = opp_np; > > _opp_table_alloc_required_tables(opp_table, dev, opp_np); > - of_node_put(opp_np); Where does this get dropped now ? > } > > void _of_clear_opp_table(struct opp_table *opp_table) > @@ -902,7 +901,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table, > > new_opp->turbo = of_property_read_bool(np, "turbo-mode"); > > - new_opp->np = np; > + new_opp->np = of_node_get(np); > new_opp->dynamic = false; > new_opp->available = true; > > -- > 2.25.1
On 18-07-22, 12:38, Viresh Kumar wrote: > Please prepare two separate patches, one for opp_table->np and one for opp->np. > It is fine to add multiple patches even for the opp->np case, if the reasoning > is different. Also, please rebase on linux-next/master, in case you haven't.
At 2022-07-18 15:08:48, "Viresh Kumar" <viresh.kumar@linaro.org> wrote: >On 15-07-22, 22:47, Liang He wrote: >> In drivers/opp/of.c, there are two refcount bugs: >> (1) in _of_init_opp_table(), of_put_node() in the last line is not >> needed as the 'opp_np' is escaped out into 'opp_table->np' and >> ’opp_table' is an out parameter. >> (2) in _opp_add_static_v2(), we need call of_node_get() for the new >> reference created when "new_opp->np = np;" as new_opp is escaped out. >> Here we should also take care of the of_node_put() when 'new_opp' is >> freed based on the function description: "The opp can be controlled >> ... and may be removed by dev_pm_opp_remove". >> NOTE: _opp_add_static_v2() is called by _of_add_opp_table_v2() in a >> for_each_available_child_of_node() which will automatically increase >> and decrease the refcount. So we need an additional of_node_get() >> for the new reference created in _opp_add_static_v2(). >> >> Fixes: f06ed90e7051 ("OPP: Parse OPP table's DT properties from _of_init_opp_table()") >> Fixes: 274659029c9d ("PM / OPP: Add support to parse "operating-points-v2" bindings") > >The way I designed the OPP core then was to make sure that np is only used for >pointer comparison and nothing else after it is freed by calling of_node_put(). >So it isn't a bug. > >But I do understand that it has become difficult to track now if np can get used >later on for other stuff as well or not and it would be better to keep the >reference up in such a case. > >That is, you can drop the Fixes tag as I don't want these to get backported, but >yes patches are welcome. > I will drop the fix tags in new patches. >Please prepare two separate patches, one for opp_table->np and one for opp->np. >It is fine to add multiple patches even for the opp->np case, if the reasoning >is different. > Thanks, Viresh, But is it OK if the two patches change the same file at same time？ I wonder if this will trouble you to merge them later. If it is OK, I will begin to prepare the two patches based on same file. >> Signed-off-by: Liang He <windhl@126.com> >> --- >> drivers/opp/core.c | 1 + >> drivers/opp/of.c | 3 +-- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/opp/core.c b/drivers/opp/core.c >> index 84063eaebb91..70775362eb05 100644 >> --- a/drivers/opp/core.c >> +++ b/drivers/opp/core.c >> @@ -1576,6 +1576,7 @@ static void _opp_kref_release(struct kref *kref) >> list_del(&opp->node); >> mutex_unlock(&opp_table->lock); >> >> + of_node_put(opp->np); >> /* >> * Notify the changes in the availability of the operable >> * frequency/voltage list. >> diff --git a/drivers/opp/of.c b/drivers/opp/of.c >> index 30394929d700..0a38fc2c0f05 100644 >> --- a/drivers/opp/of.c >> +++ b/drivers/opp/of.c >> @@ -242,7 +242,6 @@ void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, >> opp_table->np = opp_np; >> >> _opp_table_alloc_required_tables(opp_table, dev, opp_np); >> - of_node_put(opp_np); > >Where does this get dropped now ? > After I read the code, I think it is better to drop opp_table->np in '_opp_table_kref_release' just like we do for the 'opp->np' in '_opp_kref_release'. If it is not OK, please correct me. >[ ... ] >Also, please rebase on linux-next/master, in case you haven't. I will rebase on linux-next/master in future patch work. Thanks, Liang
On 18-07-22, 16:28, Liang He wrote: > At 2022-07-18 15:08:48, "Viresh Kumar" <viresh.kumar@linaro.org> wrote: > But is it OK if the two patches change the same file at same time？ Yes. > I wonder if this will trouble you to merge them later. If I apply the patches in the same order you send them (based on patch numbers in subject), it will never be a problem. > After I read the code, I think it is better to drop opp_table->np in '_opp_table_kref_release' > just like we do for the 'opp->np' in '_opp_kref_release'. Yeah, look fine, though a review after the new version will make it more clear.
diff --git a/drivers/opp/core.c b/drivers/opp/core.c index 84063eaebb91..70775362eb05 100644 --- a/drivers/opp/core.c +++ b/drivers/opp/core.c @@ -1576,6 +1576,7 @@ static void _opp_kref_release(struct kref *kref) list_del(&opp->node); mutex_unlock(&opp_table->lock); + of_node_put(opp->np); /* * Notify the changes in the availability of the operable * frequency/voltage list. diff --git a/drivers/opp/of.c b/drivers/opp/of.c index 30394929d700..0a38fc2c0f05 100644 --- a/drivers/opp/of.c +++ b/drivers/opp/of.c @@ -242,7 +242,6 @@ void _of_init_opp_table(struct opp_table *opp_table, struct device *dev, opp_table->np = opp_np; _opp_table_alloc_required_tables(opp_table, dev, opp_np); - of_node_put(opp_np); } void _of_clear_opp_table(struct opp_table *opp_table) @@ -902,7 +901,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table, new_opp->turbo = of_property_read_bool(np, "turbo-mode"); - new_opp->np = np; + new_opp->np = of_node_get(np); new_opp->dynamic = false; new_opp->available = true;
In drivers/opp/of.c, there are two refcount bugs: (1) in _of_init_opp_table(), of_put_node() in the last line is not needed as the 'opp_np' is escaped out into 'opp_table->np' and ’opp_table' is an out parameter. (2) in _opp_add_static_v2(), we need call of_node_get() for the new reference created when "new_opp->np = np;" as new_opp is escaped out. Here we should also take care of the of_node_put() when 'new_opp' is freed based on the function description: "The opp can be controlled ... and may be removed by dev_pm_opp_remove". NOTE: _opp_add_static_v2() is called by _of_add_opp_table_v2() in a for_each_available_child_of_node() which will automatically increase and decrease the refcount. So we need an additional of_node_get() for the new reference created in _opp_add_static_v2(). Fixes: f06ed90e7051 ("OPP: Parse OPP table's DT properties from _of_init_opp_table()") Fixes: 274659029c9d ("PM / OPP: Add support to parse "operating-points-v2" bindings") Signed-off-by: Liang He <windhl@126.com> --- drivers/opp/core.c | 1 + drivers/opp/of.c | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-)