Re: [PATCH] opp: Prepare for ->set_opp() helper to work without regulators

[Dmitry Osipenko]

20.01.2021 10:39, Viresh Kumar пишет:
> On 19-01-21, 20:16, Dmitry Osipenko wrote:
>> 19.01.2021 09:35, Viresh Kumar пишет:
>>> +	mutex_lock(&opp_table->lock);
>>> +	opp_table->set_opp_data = data;
>>> +	if (opp_table->sod_supplies) {
>>> +		data->old_opp.supplies = opp_table->sod_supplies;
>>> +		data->new_opp.supplies = opp_table->sod_supplies +
>>> +					 opp_table->regulator_count;
>>> +	}
>>> +	mutex_unlock(&opp_table->lock);
>>
>> Why do we need all these locks in this patch?
> 
> In case dev_pm_opp_set_regulators() and
> dev_pm_opp_register_set_opp_helper() get called at the same time.
> Which can actually happen, though is a corner case.
> 
>> The OPP API isn't thread-safe, these locks won't make the API
>> thread-safe.
> 
> I am not sure what you mean by that, can you please explain ?
> 
>> At least both sod_supplies and set_opp() pointers should be
>> set and unset under the lock.
> 
> The ->set_opp pointer isn't getting used for a comparison and so
> putting that inside a lock won't get us anything. We are only using
> set_opp_data and sod_supplies for comparison at both the places and so
> they need to be updated within the lock.
> 
If OPP API was meant to be thread-safe, then the
dev_pm_opp_unregister_set_opp_helper() should unset the
opp_table->set_opp_data under the lock since it races with
dev_pm_opp_set_regulators().

Secondly, functions like dev_pm_opp_set_rate() don't have any locks at all.

It should be better not to add "random" locks into the code because it
only creates an illusion for an oblivious API user that OPP API cares
about thread safety, IMO.

Making OPP API thread-safe will take some effort and a careful review of
every lock will be needed.