[PATCH] OPP: Fix two refcount bugs

[PATCH] OPP: Fix two refcount bugs

[Liang He]

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(-)

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;
 
-- 
2.25.1

Re: [PATCH] OPP: Fix two refcount bugs

[Viresh Kumar]

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

-- 
viresh

Re: [PATCH] OPP: Fix two refcount bugs

[Viresh Kumar]

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.

-- 
viresh

Re:Re: [PATCH] OPP: Fix two refcount bugs

[Liang He]

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

Re: [PATCH] OPP: Fix two refcount bugs

[Viresh Kumar]

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.

-- 
viresh