Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Leonard Crestez <leonard.crestez@nxp.com>
To:     Chanwoo Choi <cw00.choi@samsung.com>,
        Matthias Kaehlcke <mka@chromium.org>,
        Viresh Kumar <viresh.kumar@linaro.org>
CC:     MyungJoo Ham <myungjoo.ham@samsung.com>,
        Kyungmin Park <kyungmin.park@samsung.com>,
        Zhang Rui <rui.zhang@intel.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Amit Kucheria <amit.kucheria@verdurent.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>
Subject: Re: [PATCH 2/2] PM / devfreq: Use exclusively PM QoS to determine
 frequency limits
Thread-Topic: [PATCH 2/2] PM / devfreq: Use exclusively PM QoS to determine
 frequency limits
Thread-Index: AQHVx95Oo8Zzm3lRTk2yNfld8Mz1Kg==
Date:   Tue, 14 Jan 2020 16:08:11 +0000
Message-ID: <VI1PR04MB702308C23513581F33EFE697EE340@VI1PR04MB7023.eurprd04.prod.outlook.com>
References: <20200110094913.1.I146403d05b9ec82f48b807efd416a57f545b447a@changeid>
 <CGME20200110174932epcas1p345b0e750b48cc9e351dca14e0dd4de86@epcas1p3.samsung.com>
 <20200110094913.2.Ie8eacf976ce7a13e421592f5c1ab8dbdc537da5c@changeid>
 <c793c2e5-dd0e-bff4-9769-26344afe914e@samsung.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 3e651e2b-9786-4e7d-eb82-08d7990bf471
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Jan 2020 16:08:11.9985
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Em3J6l/nczxqDv/pOmEpWHB413Ip63mMohww6jb9b65vq1C4AId7spb92ufsw+fOc1P15eM2tM2k2ibfudUj/A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB5472
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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