[PATCH v2 1/2] thermal/core: Make the userspace governor deprecated

[PATCH v2 1/2] thermal/core: Make the userspace governor deprecated

[Daniel Lezcano]

The userspace governor is sending temperature when polling is active
and trip point crossed events. Nothing else.

AFAICT, this governor is used with custom kernels making the userspace
governor co-existing with another governor on the same thermal zone
because there was no notification mechanism, implying a hack in the
framework to support this configuration.

The new netlink thermal notification is able to provide more
information than the userspace governor and give the opportunity to
the users of this governor to replace it by a dedicated notification
framework.

The userspace governor will be removed as its usage is no longer
needed.

Add a warning message to tell the userspace governor is deprecated.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/gov_user_space.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/thermal/gov_user_space.c b/drivers/thermal/gov_user_space.c
index 82a7198bbe71..f4fe050e1cbc 100644
--- a/drivers/thermal/gov_user_space.c
+++ b/drivers/thermal/gov_user_space.c
@@ -15,6 +15,14 @@
 
 #include "thermal_core.h"
 
+static int user_space_bind(struct thermal_zone_device *tz)
+{
+	pr_warn("Userspace governor deprecated: use thermal netlink "	\
+		"notification instead\n");
+
+	return 0;
+}
+
 /**
  * notify_user_space - Notifies user space about thermal events
  * @tz: thermal_zone_device
@@ -43,5 +51,6 @@ static int notify_user_space(struct thermal_zone_device *tz, int trip)
 static struct thermal_governor thermal_gov_user_space = {
 	.name		= "user_space",
 	.throttle	= notify_user_space,
+	.bind_to_tz	= user_space_bind,
 };
 THERMAL_GOVERNOR_DECLARE(thermal_gov_user_space);
-- 
2.25.1

[PATCH v2 2/2] thermal/core: Make deprecated cooling device state change from userspace

[Daniel Lezcano]

The cooling devices have their cooling device set_cur_state
read-writable all the time in the sysfs directory, thus allowing the
userspace to act on it.

The thermal framework is wrongly used by userspace as a power capping
framework by acting on the cooling device opaque state. This one then
competes with the in-kernel governor decision.

We have seen in out-of-tree kernels, a big number of devices which are
abusely declaring themselves as cooling device just to act on their
power.

The role of the thermal framework is to protect the junction
temperature of the silicon. Letting the userspace to play with a
cooling device is invalid and potentially dangerous.

The powercap framework is the right framework to do power capping and
moreover it deals with the aggregation via the dev pm qos.

As the userspace governor is marked deprecated and about to be
removed, there is no point to keep this file writable also in the
future.

Emit a warning and deprecate the interface.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/thermal/thermal_sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index 1c4aac8464a7..730f1361dbef 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -610,6 +610,8 @@ cur_state_store(struct device *dev, struct device_attribute *attr,
 	unsigned long state;
 	int result;
 
+	pr_warn_once("Setting cooling device state is deprecated\n");
+	
 	if (sscanf(buf, "%ld\n", &state) != 1)
 		return -EINVAL;
 
-- 
2.25.1

Re: [PATCH v2 1/2] thermal/core: Make the userspace governor deprecated

[Rafael J. Wysocki]

On Tue, Oct 19, 2021 at 5:02 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> The userspace governor is sending temperature when polling is active
> and trip point crossed events. Nothing else.
>
> AFAICT, this governor is used with custom kernels making the userspace
> governor co-existing with another governor on the same thermal zone
> because there was no notification mechanism, implying a hack in the
> framework to support this configuration.
>
> The new netlink thermal notification is able to provide more
> information than the userspace governor and give the opportunity to
> the users of this governor to replace it by a dedicated notification
> framework.
>
> The userspace governor will be removed as its usage is no longer
> needed.
>
> Add a warning message to tell the userspace governor is deprecated.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/thermal/gov_user_space.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/thermal/gov_user_space.c b/drivers/thermal/gov_user_space.c
> index 82a7198bbe71..f4fe050e1cbc 100644
> --- a/drivers/thermal/gov_user_space.c
> +++ b/drivers/thermal/gov_user_space.c
> @@ -15,6 +15,14 @@
>
>  #include "thermal_core.h"
>
> +static int user_space_bind(struct thermal_zone_device *tz)
> +{
> +       pr_warn("Userspace governor deprecated: use thermal netlink "   \
> +               "notification instead\n");
> +
> +       return 0;
> +}
> +
>  /**
>   * notify_user_space - Notifies user space about thermal events
>   * @tz: thermal_zone_device
> @@ -43,5 +51,6 @@ static int notify_user_space(struct thermal_zone_device *tz, int trip)
>  static struct thermal_governor thermal_gov_user_space = {
>         .name           = "user_space",
>         .throttle       = notify_user_space,
> +       .bind_to_tz     = user_space_bind,
>  };
>  THERMAL_GOVERNOR_DECLARE(thermal_gov_user_space);
> --
> 2.25.1
>

Re: [PATCH v2 2/2] thermal/core: Make deprecated cooling device state change from userspace

[Rafael J. Wysocki]

On Tue, Oct 19, 2021 at 5:02 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> The cooling devices have their cooling device set_cur_state
> read-writable all the time in the sysfs directory, thus allowing the
> userspace to act on it.
>
> The thermal framework is wrongly used by userspace as a power capping
> framework by acting on the cooling device opaque state. This one then
> competes with the in-kernel governor decision.
>
> We have seen in out-of-tree kernels, a big number of devices which are
> abusely declaring themselves as cooling device just to act on their
> power.
>
> The role of the thermal framework is to protect the junction
> temperature of the silicon. Letting the userspace to play with a
> cooling device is invalid and potentially dangerous.
>
> The powercap framework is the right framework to do power capping and
> moreover it deals with the aggregation via the dev pm qos.
>
> As the userspace governor is marked deprecated and about to be
> removed, there is no point to keep this file writable also in the
> future.
>
> Emit a warning and deprecate the interface.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/thermal/thermal_sysfs.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> index 1c4aac8464a7..730f1361dbef 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -610,6 +610,8 @@ cur_state_store(struct device *dev, struct device_attribute *attr,
>         unsigned long state;
>         int result;
>
> +       pr_warn_once("Setting cooling device state is deprecated\n");

Maybe dev_warn_once() even?  I guess it won\t hurt to print it once
per the affected device?

> +
>         if (sscanf(buf, "%ld\n", &state) != 1)
>                 return -EINVAL;
>
> --

Re: [PATCH v2 2/2] thermal/core: Make deprecated cooling device state change from userspace

[Rafael J. Wysocki]

On Tue, Oct 19, 2021 at 5:40 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Oct 19, 2021 at 5:02 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> >
> > The cooling devices have their cooling device set_cur_state
> > read-writable all the time in the sysfs directory, thus allowing the
> > userspace to act on it.
> >
> > The thermal framework is wrongly used by userspace as a power capping
> > framework by acting on the cooling device opaque state. This one then
> > competes with the in-kernel governor decision.
> >
> > We have seen in out-of-tree kernels, a big number of devices which are
> > abusely declaring themselves as cooling device just to act on their
> > power.
> >
> > The role of the thermal framework is to protect the junction
> > temperature of the silicon. Letting the userspace to play with a
> > cooling device is invalid and potentially dangerous.
> >
> > The powercap framework is the right framework to do power capping and
> > moreover it deals with the aggregation via the dev pm qos.
> >
> > As the userspace governor is marked deprecated and about to be
> > removed, there is no point to keep this file writable also in the
> > future.
> >
> > Emit a warning and deprecate the interface.
> >
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > ---
> >  drivers/thermal/thermal_sysfs.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
> > index 1c4aac8464a7..730f1361dbef 100644
> > --- a/drivers/thermal/thermal_sysfs.c
> > +++ b/drivers/thermal/thermal_sysfs.c
> > @@ -610,6 +610,8 @@ cur_state_store(struct device *dev, struct device_attribute *attr,
> >         unsigned long state;
> >         int result;
> >
> > +       pr_warn_once("Setting cooling device state is deprecated\n");
>
> Maybe dev_warn_once() even?  I guess it won\t hurt to print it once
> per the affected device?
>
> > +
> >         if (sscanf(buf, "%ld\n", &state) != 1)
> >                 return -EINVAL;
> >
> > --

Also I think that changing the subject to something like "Deprecate
changing cooling device state from userspace" would help.

Re: [PATCH v2 2/2] thermal/core: Make deprecated cooling device state change from userspace

[Daniel Lezcano]

On 19/10/2021 17:43, Rafael J. Wysocki wrote:
> On Tue, Oct 19, 2021 at 5:40 PM Rafael J. Wysocki <rafael@kernel.org>
> wrote:
>> 
>> On Tue, Oct 19, 2021 at 5:02 PM Daniel Lezcano 
>> <daniel.lezcano@linaro.org> wrote:
>>> 
>>> The cooling devices have their cooling device set_cur_state 
>>> read-writable all the time in the sysfs directory, thus allowing
>>> the userspace to act on it.
>>> 
>>> The thermal framework is wrongly used by userspace as a power
>>> capping framework by acting on the cooling device opaque state.
>>> This one then competes with the in-kernel governor decision.
>>> 
>>> We have seen in out-of-tree kernels, a big number of devices
>>> which are abusely declaring themselves as cooling device just to
>>> act on their power.
>>> 
>>> The role of the thermal framework is to protect the junction 
>>> temperature of the silicon. Letting the userspace to play with a 
>>> cooling device is invalid and potentially dangerous.
>>> 
>>> The powercap framework is the right framework to do power capping
>>> and moreover it deals with the aggregation via the dev pm qos.
>>> 
>>> As the userspace governor is marked deprecated and about to be 
>>> removed, there is no point to keep this file writable also in
>>> the future.
>>> 
>>> Emit a warning and deprecate the interface.
>>> 
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> --- 
>>> drivers/thermal/thermal_sysfs.c | 2 ++ 1 file changed, 2
>>> insertions(+)
>>> 
>>> diff --git a/drivers/thermal/thermal_sysfs.c
>>> b/drivers/thermal/thermal_sysfs.c index
>>> 1c4aac8464a7..730f1361dbef 100644 ---
>>> a/drivers/thermal/thermal_sysfs.c +++
>>> b/drivers/thermal/thermal_sysfs.c @@ -610,6 +610,8 @@
>>> cur_state_store(struct device *dev, struct device_attribute
>>> *attr, unsigned long state; int result;
>>> 
>>> +       pr_warn_once("Setting cooling device state is
>>> deprecated\n");
>> 
>> Maybe dev_warn_once() even?  I guess it won\t hurt to print it
>> once per the affected device?

Actually, there is no difference except it will be prefixed by the
device name.

As the static variable to write once the message is local to the
function, not the dev, the warning will appear only once even if we use
dev, or cdev->device.

The other calls in the file are pr_warn. I suggest to stay consistent in
this case. Is that fine ?

>>> + if (sscanf(buf, "%ld\n", &state) != 1) return -EINVAL;
>>> 
>>> --
> 
> Also I think that changing the subject to something like "Deprecate 
> changing cooling device state from userspace" would help.

Ok


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH v2 2/2] thermal/core: Make deprecated cooling device state change from userspace

[Rafael J. Wysocki]

On Tue, Oct 19, 2021 at 5:59 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 19/10/2021 17:43, Rafael J. Wysocki wrote:
> > On Tue, Oct 19, 2021 at 5:40 PM Rafael J. Wysocki <rafael@kernel.org>
> > wrote:
> >>
> >> On Tue, Oct 19, 2021 at 5:02 PM Daniel Lezcano
> >> <daniel.lezcano@linaro.org> wrote:
> >>>
> >>> The cooling devices have their cooling device set_cur_state
> >>> read-writable all the time in the sysfs directory, thus allowing
> >>> the userspace to act on it.
> >>>
> >>> The thermal framework is wrongly used by userspace as a power
> >>> capping framework by acting on the cooling device opaque state.
> >>> This one then competes with the in-kernel governor decision.
> >>>
> >>> We have seen in out-of-tree kernels, a big number of devices
> >>> which are abusely declaring themselves as cooling device just to
> >>> act on their power.
> >>>
> >>> The role of the thermal framework is to protect the junction
> >>> temperature of the silicon. Letting the userspace to play with a
> >>> cooling device is invalid and potentially dangerous.
> >>>
> >>> The powercap framework is the right framework to do power capping
> >>> and moreover it deals with the aggregation via the dev pm qos.
> >>>
> >>> As the userspace governor is marked deprecated and about to be
> >>> removed, there is no point to keep this file writable also in
> >>> the future.
> >>>
> >>> Emit a warning and deprecate the interface.
> >>>
> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> ---
> >>> drivers/thermal/thermal_sysfs.c | 2 ++ 1 file changed, 2
> >>> insertions(+)
> >>>
> >>> diff --git a/drivers/thermal/thermal_sysfs.c
> >>> b/drivers/thermal/thermal_sysfs.c index
> >>> 1c4aac8464a7..730f1361dbef 100644 ---
> >>> a/drivers/thermal/thermal_sysfs.c +++
> >>> b/drivers/thermal/thermal_sysfs.c @@ -610,6 +610,8 @@
> >>> cur_state_store(struct device *dev, struct device_attribute
> >>> *attr, unsigned long state; int result;
> >>>
> >>> +       pr_warn_once("Setting cooling device state is
> >>> deprecated\n");
> >>
> >> Maybe dev_warn_once() even?  I guess it won\t hurt to print it
> >> once per the affected device?
>
> Actually, there is no difference except it will be prefixed by the
> device name.
>
> As the static variable to write once the message is local to the
> function, not the dev, the warning will appear only once even if we use
> dev, or cdev->device.
>
> The other calls in the file are pr_warn. I suggest to stay consistent in
> this case. Is that fine ?

It is, except that it may not be straightforward to connect the
message to the specific device in question.