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

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

On 19. 8. 21. 오후 10:00, Leonard Crestez wrote:
> 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?

I think that it is more clear to add notifier
after mutex_unlock(&devfreq->lock) if there are no any issue.

> 
> --
> Regards,
> Leonard
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics