Re: [PATCH v3 1/2] PM / devfreq: Add dev_pm_qos support

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

On 21.08.2019 04:40, Chanwoo Choi wrote:

> On 19. 8. 21. 오전 12:24, Leonard Crestez wrote:
>> Add dev_pm_qos notifies to devfreq core in order to support frequency
>> limits via the dev_pm_qos_add_request.
>>
>> +static unsigned long get_effective_min_freq(struct devfreq *devfreq)
> 
> I'm not sure that 'effective' expression is correct.
>  From this function, the devfreq can get the final target frequency.
> 
> I think that we need to use the more correct expression
> to give the meaning of function as following:
> 
> get_min_freq
> get_max_freq

OK, will rename to get_min_freq and get_max_freq

>> @@ -636,21 +688,40 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>   		err = -ENOMEM;
>>   		goto err_out;
>>   	}
>>   
>>   	mutex_init(&devfreq->lock);
>> -	mutex_lock(&devfreq->lock);
> 
> Basically, I think that it is safe to lock when touch
> the variable of the devfreq.
> 
> it is not proper way for the dev_pm_qos because
> it breaks the existing locking reason of devfreq's variables.

I don't understand what you mean. I'm initializing some stuff outside 
the lock to avoid circular lock warning between:

(&devfreq->lock){+.+.}, at: devfreq_qos_min_notifier_call+0x24/0x48
(&(n)->rwsem){++++}, at: blocking_notifier_call_chain+0x3c/0x78

In general you don't need to lock an object while initializing except 
after it becomes accessible from the outside so devfreq_add_device 
doesn't need to take the lock so early.

The QOS notifiers are registered on the parent device so in theory it's 
possible for somebody to add QOS requests while devfreq_add_device is 
executing. Maybe notifier registration should be moved at the end after 
unlock?

--
Regards,
Leonard