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@xxxxxxxxxx> wrote:
>
> On 19/10/2021 17:43, Rafael J. Wysocki wrote:
> > On Tue, Oct 19, 2021 at 5:40 PM Rafael J. Wysocki <rafael@xxxxxxxxxx>
> > wrote:
> >>
> >> On Tue, Oct 19, 2021 at 5:02 PM Daniel Lezcano
> >> <daniel.lezcano@xxxxxxxxxx> 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@xxxxxxxxxx> ---
> >>> 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.