Re: [PATCH 2/2] PM / devfreq: Use exclusively PM QoS to determine frequency limits
From: Leonard Crestez
Date: Tue Jan 14 2020 - 11:08:19 EST
On 13.01.2020 09:24, Chanwoo Choi wrote:
> Hi,
>
> Any device driver except for devfreq_cooling.c might
> use dev_pm_opp_enable/disable interface.
> So, don't need to remove the devfreq->scaling_max_freq
> and devfreq->scaling_min_freq for supporting OPP interface.
It seems that devfreq_cooling was the only upstream user of
dev_pm_opp_enable and the remaining callers of dev_pm_opp_disable are
probe-time checks.
> Regards,
> Chanwoo Choi
>
> On 1/11/20 2:49 AM, Matthias Kaehlcke wrote:
>> Traditionally devfreq cooling devices dynamically disabled OPPs
>> that shouldn't be used because of thermal pressure. Devfreq cooling
>> devices now use PM QoS to set frequency limits, hence the devfreq
>> code dealing that deals with disabled OPPs can be removed.
>>
>> Signed-off-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>
>> ---
>>
>> drivers/devfreq/devfreq.c | 75 +++++----------------------------------
>> include/linux/devfreq.h | 4 ---
>> 2 files changed, 8 insertions(+), 71 deletions(-)
>>
>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>> index 57f6944d65a6..ec66e2c27cc4 100644
>> --- a/drivers/devfreq/devfreq.c
>> +++ b/drivers/devfreq/devfreq.c
>> @@ -73,34 +73,6 @@ static struct devfreq *find_device_devfreq(struct device *dev)
>> return ERR_PTR(-ENODEV);
>> }
>>
>> -static unsigned long find_available_min_freq(struct devfreq *devfreq)
>> -{
>> - struct dev_pm_opp *opp;
>> - unsigned long min_freq = 0;
>> -
>> - opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, &min_freq);
>> - if (IS_ERR(opp))
>> - min_freq = 0;
>> - else
>> - dev_pm_opp_put(opp);
>> -
>> - return min_freq;
>> -}
>> -
>> -static unsigned long find_available_max_freq(struct devfreq *devfreq)
>> -{
>> - struct dev_pm_opp *opp;
>> - unsigned long max_freq = ULONG_MAX;
>> -
>> - opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent, &max_freq);
>> - if (IS_ERR(opp))
>> - max_freq = 0;
>> - else
>> - dev_pm_opp_put(opp);
>> -
>> - return max_freq;
>> -}
>> -
>> /**
>> * get_freq_range() - Get the current freq range
>> * @devfreq: the devfreq instance
>> @@ -141,10 +113,6 @@ static void get_freq_range(struct devfreq *devfreq,
>> *max_freq = min(*max_freq,
>> (unsigned long)HZ_PER_KHZ * qos_max_freq);
>>
>> - /* Apply constraints from OPP interface */
>> - *min_freq = max(*min_freq, devfreq->scaling_min_freq);
>> - *max_freq = min(*max_freq, devfreq->scaling_max_freq);
>> -
>> if (*min_freq > *max_freq)
>> *min_freq = *max_freq;
>> }
>> @@ -610,23 +578,10 @@ static int devfreq_notifier_call(struct notifier_block *nb, unsigned long type,
>> void *devp)
>> {
>> struct devfreq *devfreq = container_of(nb, struct devfreq, nb);
>> - int err = -EINVAL;
>> + int err;
>>
>> mutex_lock(&devfreq->lock);
>> -
>> - devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>> - if (!devfreq->scaling_min_freq)
>> - goto out;
>> -
>> - devfreq->scaling_max_freq = find_available_max_freq(devfreq);
>> - if (!devfreq->scaling_max_freq) {
>> - devfreq->scaling_max_freq = ULONG_MAX;
>> - goto out;
>> - }
>> -
>> err = update_devfreq(devfreq);
>> -
>> -out:
>> mutex_unlock(&devfreq->lock);
>> if (err)
>> dev_err(devfreq->dev.parent,
>> @@ -781,19 +736,15 @@ struct devfreq *devfreq_add_device(struct device *dev,
>> mutex_lock(&devfreq->lock);
>> }
>>
>> - devfreq->scaling_min_freq = find_available_min_freq(devfreq);
>> - if (!devfreq->scaling_min_freq) {
>> - mutex_unlock(&devfreq->lock);
>> - err = -EINVAL;
>> + err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
>> + DEV_PM_QOS_MIN_FREQUENCY, 0);
>> + if (err < 0)
>> goto err_dev;
>> - }
>> -
>> - devfreq->scaling_max_freq = find_available_max_freq(devfreq);
>> - if (!devfreq->scaling_max_freq) {
>> - mutex_unlock(&devfreq->lock);
>> - err = -EINVAL;
>> + err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
>> + DEV_PM_QOS_MAX_FREQUENCY,
>> + PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
>> + if (err < 0)
>> goto err_dev;
>> - }
>>
>> devfreq->suspend_freq = dev_pm_opp_get_suspend_opp_freq(dev);
>> atomic_set(&devfreq->suspend_count, 0);
>> @@ -834,16 +785,6 @@ struct devfreq *devfreq_add_device(struct device *dev,
>>
>> mutex_unlock(&devfreq->lock);
>>
>> - err = dev_pm_qos_add_request(dev, &devfreq->user_min_freq_req,
>> - DEV_PM_QOS_MIN_FREQUENCY, 0);
>> - if (err < 0)
>> - goto err_devfreq;
>> - err = dev_pm_qos_add_request(dev, &devfreq->user_max_freq_req,
>> - DEV_PM_QOS_MAX_FREQUENCY,
>> - PM_QOS_MAX_FREQUENCY_DEFAULT_VALUE);
>> - if (err < 0)
>> - goto err_devfreq;
>> -
Performing PM QoS initialization under devfreq->lock triggers lockdep
warnings for me. The warnings seem to be legitimate:
1) At init time &dev_pm_qos_mtx is taken under &devfreq->lock;
2) At update time &devfreq->lock is called under &dev_pm_qos_mtx (it's
held while notifiers are called).
It's not clear why you moved dev_pm_qos_add_request higher?
>> devfreq->nb_min.notifier_call = qos_min_notifier_call;
>> err = dev_pm_qos_add_notifier(devfreq->dev.parent, &devfreq->nb_min,
>> DEV_PM_QOS_MIN_FREQUENCY);
>> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
>> index fb376b5b7281..cb75f23ad2f4 100644
>> --- a/include/linux/devfreq.h
>> +++ b/include/linux/devfreq.h
>> @@ -126,8 +126,6 @@ struct devfreq_dev_profile {
>> * touch this.
>> * @user_min_freq_req: PM QoS minimum frequency request from user (via sysfs)
>> * @user_max_freq_req: PM QoS maximum frequency request from user (via sysfs)
>> - * @scaling_min_freq: Limit minimum frequency requested by OPP interface
>> - * @scaling_max_freq: Limit maximum frequency requested by OPP interface
>> * @stop_polling: devfreq polling status of a device.
>> * @suspend_freq: frequency of a device set during suspend phase.
>> * @resume_freq: frequency of a device set in resume phase.
>> @@ -166,8 +164,6 @@ struct devfreq {
>>
>> struct dev_pm_qos_request user_min_freq_req;
>> struct dev_pm_qos_request user_max_freq_req;
>> - unsigned long scaling_min_freq;
>> - unsigned long scaling_max_freq;
>> bool stop_polling;
>>
>> unsigned long suspend_freq;
>>
>