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

[Chanwoo Choi <cw00.choi@samsung.com>]

On 19. 9. 24. 오후 4:44, Leonard Crestez wrote:
> On 2019-09-24 5:07 AM, Chanwoo Choi wrote:
>> Hi,
>>
>> On 19. 9. 24. 오전 1:27, 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.
>>>
>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
>>> ---
>>>   drivers/devfreq/devfreq.c | 13 ++++++++++---
>>>   1 file changed, 10 insertions(+), 3 deletions(-)
>>>
>>> Changes since v1:
>>> * Split from series: low-priority bugfix not strictly required for PM QoS
>>> * Only keep lock during update, release before sprintf
>>>
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index 4c58fbf7d4e4..00fc23fea5b2 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -206,10 +206,11 @@ int devfreq_update_status(struct devfreq *devfreq, unsigned long freq)
>>>   {
>>>   	int lev, prev_lev, ret = 0;
>>>   	unsigned long cur_time;
>>>   
>>>   	cur_time = jiffies;
>>> +	lockdep_assert_held(&devfreq->lock);
>>
>> It better to move lock checking before 'cur_time = jiffies'
>> in order to reduce the redundant code execution.
> 
> OK but I don't see how this makes a difference for an assert? It just 
> prints a warning and carries on.

According to the sequence between 'lockdep_assert_held' and 'cur_time = jiffies',
cur_time will be initialized with different jiffies because 'jiffies' is continuously
changed. In order to get the more correct time from 'jiffies',
we better to get 'jiffies' after releasing the lock.

> 
>>>   	/* Immediately exit if previous_freq is not initialized yet. */
>>>   	if (!devfreq->previous_freq)
>>>   		goto out;
>>>   
>>> @@ -1507,16 +1508,22 @@ 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;
>>>   
>>> -	if (!devfreq->stop_polling &&
>>> -			devfreq_update_status(devfreq, devfreq->previous_freq))
>>> -		return 0;
>>>   	if (max_state == 0)
>>>   		return sprintf(buf, "Not Supported.\n");
>>>   
>>> +	/* lock and update */
>>
>> It is not necessary. Anyone can know that this code is related to mutex lock/unlock.
> 
> OK. You're the second person to mention this but it's quite strange to 
> see objections raised against comments.

The comment is very important to understand the code
for everyone. But, in this case, almost people understand
the usage of mutex_lock/mutex_unlock. It is no difficult
to understand the meaning of below codes.

Usually, we would add the comments if some codes are very difficult
without comments or some codes have depend on some call sequence and so on.

> 
>>> +	mutex_lock(&devfreq->lock);
>>> +	if (!devfreq->stop_polling &&
>>> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
>>> +		mutex_unlock(&devfreq->lock);
>>> +		return 0;
>>> +	}
>>> +	mutex_unlock(&devfreq->lock);
>>> +
>>>   	len = sprintf(buf, "     From  :   To\n");
>>>   	len += sprintf(buf + len, "           :");
>>>   	for (i = 0; i < max_state; i++)
>>>   		len += sprintf(buf + len, "%10lu",
>>>   				devfreq->profile->freq_table[i]);
>>>
>>
>> Basically, it is necessary. Please edit it by my comments.
>> Also, you have to add the following 'fixes' as following:
>> and send it stable mailing list.
>>
>> Fixes: 39688ce6facd ("PM / devfreq: account suspend/resume for stats")
>>
>> If you edit it by my comments, feel free to add my tag:
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel