Messages in this thread | | | From | Leonard Crestez <> | Subject | Re: [PATCH 2/2] PM / devfreq: Use exclusively PM QoS to determine frequency limits | Date | Tue, 14 Jan 2020 16:08:11 +0000 |
| |
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@chromium.org> >> --- >> >> 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; >> >
| |