[RFC PATCH] PM / devfreq: Add policy notifier

[RFC PATCH] PM / devfreq: Add policy notifier

[Matthias Kaehlcke]

Policy notifiers are called before a frequency change and may adjust the
frequency, similar to CPUFREQ_ADJUST. The purpose of policy notifiers is
to support non-thermal throttling of devfreq devices.

As of now notifiers may only select a lower frequency, but not increase it.
The reason for this limitation is that devfreq currently doesn't have an
actual policy for frequencies and OPPs > freq might be disabled by a
devfreq cooling device (see drivers/thermal/devfreq_cooling.c).

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Marked as RFC since I'm aware that this isn't quite a 'policy' notifier,
but I also didn't want to make it too specific. Maybe devfreq devices should
have a policy similar to cpufreq?

Should devfreq core be in charge of disabling/enabling OPPs instead of
the thermal cooling device? Typically we don't want to use higher OPPs
than those whitelisted by thermal, so in practice the current situation
shouldn't be much of an issue.

 drivers/devfreq/devfreq.c | 20 ++++++++++++++++----
 include/linux/devfreq.h   |  6 ++++++
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index fe2af6aa88fc..a7294c056f65 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -273,6 +273,9 @@ int update_devfreq(struct devfreq *devfreq)
 	if (err)
 		return err;
 
+	srcu_notifier_call_chain(&devfreq->policy_notifier_list,
+			DEVFREQ_ADJUST, &freq);
+
 	/*
 	 * Adjust the frequency with user freq, QoS and available freq.
 	 *
@@ -640,6 +643,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
 	devfreq->last_stat_updated = jiffies;
 
 	srcu_init_notifier_head(&devfreq->transition_notifier_list);
+	srcu_init_notifier_head(&devfreq->policy_notifier_list);
 
 	mutex_unlock(&devfreq->lock);
 
@@ -1414,7 +1418,7 @@ EXPORT_SYMBOL(devm_devfreq_unregister_opp_notifier);
  * devfreq_register_notifier() - Register a driver with devfreq
  * @devfreq:	The devfreq object.
  * @nb:		The notifier block to register.
- * @list:	DEVFREQ_TRANSITION_NOTIFIER.
+ * @list:	DEVFREQ_TRANSITION_NOTIFIER or DEVFREQ_POLICY_NOTIFIER.
  */
 int devfreq_register_notifier(struct devfreq *devfreq,
 				struct notifier_block *nb,
@@ -1430,6 +1434,10 @@ int devfreq_register_notifier(struct devfreq *devfreq,
 		ret = srcu_notifier_chain_register(
 				&devfreq->transition_notifier_list, nb);
 		break;
+	case DEVFREQ_POLICY_NOTIFIER:
+		ret = srcu_notifier_chain_register(
+				&devfreq->policy_notifier_list, nb);
+		break;
 	default:
 		ret = -EINVAL;
 	}
@@ -1442,7 +1450,7 @@ EXPORT_SYMBOL(devfreq_register_notifier);
  * devfreq_unregister_notifier() - Unregister a driver with devfreq
  * @devfreq:	The devfreq object.
  * @nb:		The notifier block to be unregistered.
- * @list:	DEVFREQ_TRANSITION_NOTIFIER.
+ * @list:	DEVFREQ_TRANSITION_NOTIFIER or DEVFREQ_POLICY_NOTIFIER.
  */
 int devfreq_unregister_notifier(struct devfreq *devfreq,
 				struct notifier_block *nb,
@@ -1458,6 +1466,10 @@ int devfreq_unregister_notifier(struct devfreq *devfreq,
 		ret = srcu_notifier_chain_unregister(
 				&devfreq->transition_notifier_list, nb);
 		break;
+	case DEVFREQ_POLICY_NOTIFIER:
+		ret = srcu_notifier_chain_unregister(
+				&devfreq->policy_notifier_list, nb);
+		break;
 	default:
 		ret = -EINVAL;
 	}
@@ -1485,7 +1497,7 @@ static void devm_devfreq_notifier_release(struct device *dev, void *res)
  * @dev:	The devfreq user device. (parent of devfreq)
  * @devfreq:	The devfreq object.
  * @nb:		The notifier block to be unregistered.
- * @list:	DEVFREQ_TRANSITION_NOTIFIER.
+ * @list:	DEVFREQ_TRANSITION_NOTIFIER or DEVFREQ_POLICY_NOTIFIER.
  */
 int devm_devfreq_register_notifier(struct device *dev,
 				struct devfreq *devfreq,
@@ -1521,7 +1533,7 @@ EXPORT_SYMBOL(devm_devfreq_register_notifier);
  * @dev:	The devfreq user device. (parent of devfreq)
  * @devfreq:	The devfreq object.
  * @nb:		The notifier block to be unregistered.
- * @list:	DEVFREQ_TRANSITION_NOTIFIER.
+ * @list:	DEVFREQ_TRANSITION_NOTIFIER or DEVFREQ_POLICY_NOTIFIER.
  */
 void devm_devfreq_unregister_notifier(struct device *dev,
 				struct devfreq *devfreq,
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 3aae5b3af87c..8edc538d78f7 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -33,6 +33,10 @@
 #define	DEVFREQ_PRECHANGE		(0)
 #define DEVFREQ_POSTCHANGE		(1)
 
+#define DEVFREQ_POLICY_NOTIFIER		1
+
+#define	DEVFREQ_ADJUST			0
+
 struct devfreq;
 struct devfreq_governor;
 
@@ -136,6 +140,7 @@ struct devfreq_dev_profile {
  * @time_in_state:	Statistics of devfreq states
  * @last_stat_updated:	The last time stat updated
  * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier
+ * @policy_notifier_list: list head of DEVFREQ_POLICY_NOTIFIER notifier
  *
  * This structure stores the devfreq information for a give device.
  *
@@ -174,6 +179,7 @@ struct devfreq {
 	unsigned long last_stat_updated;
 
 	struct srcu_notifier_head transition_notifier_list;
+	struct srcu_notifier_head policy_notifier_list;
 };
 
 struct devfreq_freqs {
-- 
2.17.0.441.gb46fe60e1d-goog

Re: [RFC PATCH] PM / devfreq: Add policy notifier

[Chanwoo Choi]

Hi,

Could you give some use-case of DEVFREQ_POLICY_NOTIFIER
or send use-case patch with this patch?

I already knew the CPUFREQ_POLICY_NOTIFIER.
But, until now, there are no any requirements of DEVFREQ_POLICY_NOTIFIER.
If there are no any use-case, it is not necessary codes.

On 2018년 05월 16일 06:24, Matthias Kaehlcke wrote:
> Policy notifiers are called before a frequency change and may adjust the
> frequency, similar to CPUFREQ_ADJUST. The purpose of policy notifiers is
> to support non-thermal throttling of devfreq devices.
> 
> As of now notifiers may only select a lower frequency, but not increase it.
> The reason for this limitation is that devfreq currently doesn't have an
> actual policy for frequencies and OPPs > freq might be disabled by a
> devfreq cooling device (see drivers/thermal/devfreq_cooling.c).
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Marked as RFC since I'm aware that this isn't quite a 'policy' notifier,
> but I also didn't want to make it too specific. Maybe devfreq devices should
> have a policy similar to cpufreq?
> 
> Should devfreq core be in charge of disabling/enabling OPPs instead of
> the thermal cooling device? Typically we don't want to use higher OPPs
> than those whitelisted by thermal, so in practice the current situation
> shouldn't be much of an issue.
> 
>  drivers/devfreq/devfreq.c | 20 ++++++++++++++++----
>  include/linux/devfreq.h   |  6 ++++++
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index fe2af6aa88fc..a7294c056f65 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -273,6 +273,9 @@ int update_devfreq(struct devfreq *devfreq)
>  	if (err)
>  		return err;
>  
> +	srcu_notifier_call_chain(&devfreq->policy_notifier_list,
> +			DEVFREQ_ADJUST, &freq);

It is not proper to used 'freq' as the passed data.
In current step,'freq' is not adjusted and is not final decided frequency.

> +
>  	/*
>  	 * Adjust the frequency with user freq, QoS and available freq.
>  	 *
> @@ -640,6 +643,7 @@ struct devfreq *devfreq_add_device(struct device *dev,
>  	devfreq->last_stat_updated = jiffies;
>  
>  	srcu_init_notifier_head(&devfreq->transition_notifier_list);
> +	srcu_init_notifier_head(&devfreq->policy_notifier_list);
>  
>  	mutex_unlock(&devfreq->lock);
>  
> @@ -1414,7 +1418,7 @@ EXPORT_SYMBOL(devm_devfreq_unregister_opp_notifier);
>   * devfreq_register_notifier() - Register a driver with devfreq
>   * @devfreq:	The devfreq object.
>   * @nb:		The notifier block to register.
> - * @list:	DEVFREQ_TRANSITION_NOTIFIER.
> + * @list:	DEVFREQ_TRANSITION_NOTIFIER or DEVFREQ_POLICY_NOTIFIER.
>   */
>  int devfreq_register_notifier(struct devfreq *devfreq,
>  				struct notifier_block *nb,
> @@ -1430,6 +1434,10 @@ int devfreq_register_notifier(struct devfreq *devfreq,
>  		ret = srcu_notifier_chain_register(
>  				&devfreq->transition_notifier_list, nb);
>  		break;
> +	case DEVFREQ_POLICY_NOTIFIER:
> +		ret = srcu_notifier_chain_register(
> +				&devfreq->policy_notifier_list, nb);
> +		break;
>  	default:
>  		ret = -EINVAL;
>  	}
> @@ -1442,7 +1450,7 @@ EXPORT_SYMBOL(devfreq_register_notifier);
>   * devfreq_unregister_notifier() - Unregister a driver with devfreq
>   * @devfreq:	The devfreq object.
>   * @nb:		The notifier block to be unregistered.
> - * @list:	DEVFREQ_TRANSITION_NOTIFIER.
> + * @list:	DEVFREQ_TRANSITION_NOTIFIER or DEVFREQ_POLICY_NOTIFIER.
>   */
>  int devfreq_unregister_notifier(struct devfreq *devfreq,
>  				struct notifier_block *nb,
> @@ -1458,6 +1466,10 @@ int devfreq_unregister_notifier(struct devfreq *devfreq,
>  		ret = srcu_notifier_chain_unregister(
>  				&devfreq->transition_notifier_list, nb);
>  		break;
> +	case DEVFREQ_POLICY_NOTIFIER:
> +		ret = srcu_notifier_chain_unregister(
> +				&devfreq->policy_notifier_list, nb);
> +		break;
>  	default:
>  		ret = -EINVAL;
>  	}
> @@ -1485,7 +1497,7 @@ static void devm_devfreq_notifier_release(struct device *dev, void *res)
>   * @dev:	The devfreq user device. (parent of devfreq)
>   * @devfreq:	The devfreq object.
>   * @nb:		The notifier block to be unregistered.
> - * @list:	DEVFREQ_TRANSITION_NOTIFIER.
> + * @list:	DEVFREQ_TRANSITION_NOTIFIER or DEVFREQ_POLICY_NOTIFIER.
>   */
>  int devm_devfreq_register_notifier(struct device *dev,
>  				struct devfreq *devfreq,
> @@ -1521,7 +1533,7 @@ EXPORT_SYMBOL(devm_devfreq_register_notifier);
>   * @dev:	The devfreq user device. (parent of devfreq)
>   * @devfreq:	The devfreq object.
>   * @nb:		The notifier block to be unregistered.
> - * @list:	DEVFREQ_TRANSITION_NOTIFIER.
> + * @list:	DEVFREQ_TRANSITION_NOTIFIER or DEVFREQ_POLICY_NOTIFIER.
>   */
>  void devm_devfreq_unregister_notifier(struct device *dev,
>  				struct devfreq *devfreq,
> diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
> index 3aae5b3af87c..8edc538d78f7 100644
> --- a/include/linux/devfreq.h
> +++ b/include/linux/devfreq.h
> @@ -33,6 +33,10 @@
>  #define	DEVFREQ_PRECHANGE		(0)
>  #define DEVFREQ_POSTCHANGE		(1)
>  
> +#define DEVFREQ_POLICY_NOTIFIER		1
> +
> +#define	DEVFREQ_ADJUST			0
> +
>  struct devfreq;
>  struct devfreq_governor;
>  
> @@ -136,6 +140,7 @@ struct devfreq_dev_profile {
>   * @time_in_state:	Statistics of devfreq states
>   * @last_stat_updated:	The last time stat updated
>   * @transition_notifier_list: list head of DEVFREQ_TRANSITION_NOTIFIER notifier
> + * @policy_notifier_list: list head of DEVFREQ_POLICY_NOTIFIER notifier
>   *
>   * This structure stores the devfreq information for a give device.
>   *
> @@ -174,6 +179,7 @@ struct devfreq {
>  	unsigned long last_stat_updated;
>  
>  	struct srcu_notifier_head transition_notifier_list;
> +	struct srcu_notifier_head policy_notifier_list;
>  };
>  
>  struct devfreq_freqs {
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [RFC PATCH] PM / devfreq: Add policy notifier

[Matthias Kaehlcke]

Hi,

On Thu, May 17, 2018 at 11:01:34AM +0900, Chanwoo Choi wrote:
> Hi,
> 
> Could you give some use-case of DEVFREQ_POLICY_NOTIFIER
> or send use-case patch with this patch?

This is a WIP patch that makes use of the DEVFREQ_POLICY_NOTIFIER:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1065122

> I already knew the CPUFREQ_POLICY_NOTIFIER.
> But, until now, there are no any requirements of DEVFREQ_POLICY_NOTIFIER.
> If there are no any use-case, it is not necessary codes.

Sure, I intend to land the above driver upstream if devfreq can
provide the necessary interfaces.

> On 2018년 05월 16일 06:24, Matthias Kaehlcke wrote:
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index fe2af6aa88fc..a7294c056f65 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -273,6 +273,9 @@ int update_devfreq(struct devfreq *devfreq)
> >  	if (err)
> >  		return err;
> >  
> > +	srcu_notifier_call_chain(&devfreq->policy_notifier_list,
> > +			DEVFREQ_ADJUST, &freq);
> 
> It is not proper to used 'freq' as the passed data.
> In current step,'freq' is not adjusted and is not final decided
> frequency.

Right, the next revision will pass a struct devfreq_policy instead,
where the notifiers can adjust the min/max values, similar to what
cpufreq does.

Thanks

Matthias

Re: [RFC PATCH] PM / devfreq: Add policy notifier

[Chanwoo Choi]

Hi,

On 2018년 05월 18일 08:07, Matthias Kaehlcke wrote:
> Hi,
> 
> On Thu, May 17, 2018 at 11:01:34AM +0900, Chanwoo Choi wrote:
>> Hi,
>>
>> Could you give some use-case of DEVFREQ_POLICY_NOTIFIER
>> or send use-case patch with this patch?
> 
> This is a WIP patch that makes use of the DEVFREQ_POLICY_NOTIFIER:
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1065122
> 
>> I already knew the CPUFREQ_POLICY_NOTIFIER.
>> But, until now, there are no any requirements of DEVFREQ_POLICY_NOTIFIER.
>> If there are no any use-case, it is not necessary codes.
> 
> Sure, I intend to land the above driver upstream if devfreq can
> provide the necessary interfaces.

I recommend that you should send the patch with the use-case patch.

> 
>> On 2018년 05월 16일 06:24, Matthias Kaehlcke wrote:
>>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
>>> index fe2af6aa88fc..a7294c056f65 100644
>>> --- a/drivers/devfreq/devfreq.c
>>> +++ b/drivers/devfreq/devfreq.c
>>> @@ -273,6 +273,9 @@ int update_devfreq(struct devfreq *devfreq)
>>>  	if (err)
>>>  		return err;
>>>  
>>> +	srcu_notifier_call_chain(&devfreq->policy_notifier_list,
>>> +			DEVFREQ_ADJUST, &freq);
>>
>> It is not proper to used 'freq' as the passed data.
>> In current step,'freq' is not adjusted and is not final decided
>> frequency.
> 
> Right, the next revision will pass a struct devfreq_policy instead,
> where the notifiers can adjust the min/max values, similar to what
> cpufreq does.

Actually, I don't know the devfreq_policy(?). As I already commented,
it is not proper to discuss it because there is no any real code and patches.
It is difficult to understand for me.

> 
> Thanks
> 
> Matthias
> 
> 
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [RFC PATCH] PM / devfreq: Add policy notifier

[Matthias Kaehlcke]

On Fri, May 18, 2018 at 08:26:30AM +0900, Chanwoo Choi wrote:
> Hi,
> 
> On 2018년 05월 18일 08:07, Matthias Kaehlcke wrote:
> > Hi,
> > 
> > On Thu, May 17, 2018 at 11:01:34AM +0900, Chanwoo Choi wrote:
> >> Hi,
> >>
> >> Could you give some use-case of DEVFREQ_POLICY_NOTIFIER
> >> or send use-case patch with this patch?
> > 
> > This is a WIP patch that makes use of the DEVFREQ_POLICY_NOTIFIER:
> > 
> > https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1065122
> > 
> >> I already knew the CPUFREQ_POLICY_NOTIFIER.
> >> But, until now, there are no any requirements of DEVFREQ_POLICY_NOTIFIER.
> >> If there are no any use-case, it is not necessary codes.
> > 
> > Sure, I intend to land the above driver upstream if devfreq can
> > provide the necessary interfaces.
> 
> I recommend that you should send the patch with the use-case patch.

One of the reasons this patch was sent as an RFC was to get an idea
whether it would be feasible to add support for
DEVFREQ_POLICY_NOTIFIER before spending much time implementing a
driver that relies on it, and then possibly hear that it's not going
to fly.

However since you didn't voice general concerns about the idea I made
progress with the driver in the last days. I'll polish it a bit more
and send it in a series with related devfreq patches.

> >> On 2018년 05월 16일 06:24, Matthias Kaehlcke wrote:
> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >>> index fe2af6aa88fc..a7294c056f65 100644
> >>> --- a/drivers/devfreq/devfreq.c
> >>> +++ b/drivers/devfreq/devfreq.c
> >>> @@ -273,6 +273,9 @@ int update_devfreq(struct devfreq *devfreq)
> >>>  	if (err)
> >>>  		return err;
> >>>  
> >>> +	srcu_notifier_call_chain(&devfreq->policy_notifier_list,
> >>> +			DEVFREQ_ADJUST, &freq);
> >>
> >> It is not proper to used 'freq' as the passed data.
> >> In current step,'freq' is not adjusted and is not final decided
> >> frequency.
> > 
> > Right, the next revision will pass a struct devfreq_policy instead,
> > where the notifiers can adjust the min/max values, similar to what
> > cpufreq does.
> 
> Actually, I don't know the devfreq_policy(?). As I already commented,
> it is not proper to discuss it because there is no any real code and patches.
> It is difficult to understand for me.