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

On 19. 8. 21. 오후 10:00, Leonard Crestez wrote:

On 21.08.2019 04:40, Chanwoo Choi wrote:

quoted

On 19. 8. 21. 오전 12:24, Leonard Crestez wrote:

quoted

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

quoted

quoted

@@ -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

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