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

["Liang He" <windhl@126.com>]

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