Re: [PATCH 1/8] PM / devfreq: Lock devfreq in trans_stat_show

[Leonard Crestez <leonard.crestez@nxp.com>]

On 19.09.2019 00:28, Matthias Kaehlcke wrote:
> Hi Leonard,
> 
> this series doesn't indicate the version, from the change history in
> the cover letter I suppose it is v5.

Sorry about that, I forgot --subject-prefix. It is indeed v5

> On Wed, Sep 18, 2019 at 03:18:20AM +0300, Leonard Crestez wrote:
>> There is no locking in this sysfs show function so stats printing can
>> race with a devfreq_update_status called as part of freq switching or
>> with initialization.
>>
>> Also add an assert in devfreq_update_status to make it clear that lock
>> must be held by caller.
> 
> This and some other patches look like generic improvements and not
> directly related to the series "PM / devfreq: Add dev_pm_qos
> support". If there are no dependencies I think it is usually better to
> send the improvements separately, it keeps the series more focussed
> and might reduce version churn. Just my POV though ;-)

The locking cleanups are required in order to initialize pm_qos request 
and notifiers without introducing lockdep warnings.

pm_qos calls notifiers under dev_pm_qos_mtx and those notifiers needs to 
take &devfreq->lock. This means initializing pm_qos notifiers and 
requests must be done outside &devfreq->lock which needs some cleanups 
in devfreq_add_device.

This particular patch is a more loosely related bugfix. Devfreq 
maintainers: would it help to post it separately?

>> @@ -1415,15 +1416,20 @@ static ssize_t trans_stat_show(struct device *dev,
>>   	struct devfreq *devfreq = to_devfreq(dev);
>>   	ssize_t len;
>>   	int i, j;
>>   	unsigned int max_state = devfreq->profile->max_state;
>>   
>> +	mutex_lock(&devfreq->lock);
>>   	if (!devfreq->stop_polling &&
>> -			devfreq_update_status(devfreq, devfreq->previous_freq))
>> -		return 0;
>> -	if (max_state == 0)
>> -		return sprintf(buf, "Not Supported.\n");
>> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
>> +		len = 0;
> 
> you could assign 'len' in the declaration instead, but it's just
> another option, it'ss fine as is
>> +		goto out;
>> +	}
>> +	if (max_state == 0) {
>> +		len = sprintf(buf, "Not Supported.\n");
>> +		goto out;
>> +	}
> 
> This leaves the general structure of the code as is, which is great,
> but since you are already touching this part you can consider to
> improve it: 'max_state' is constant after device creation, hence the
> check could be done at the beginning, which IMO would be clearer, it
> could also save an unnecessary devfreq_update_status() call and it
> wouldn't be necessary to hold the lock (one goto less).

Now that I look at this more closely &devfreq->lock only really needs to 
be held during the stats update, it can be released during sprintf.

--
Regards,
Leonard