Re: [RFC PATCH v3 4/7] thermal: introduce the Power Actor API

Javi Merino
2014-06-12T13:45:28Z

On Wed, Jun 11, 2014 at 12:32:54PM +0100, Eduardo Valentin wrote:
> Hello Javi,
>
> On Tue, Jun 03, 2014 at 11:18:32AM +0100, Javi Merino wrote:
> > This patch introduces the Power Actor API in the thermal framework.
> > With it, devices that can report their power consumption and control
> > it can be registered. This base interface is meant to be used to
> > derive specific power actors, such as a cpu power actor.
> >
> > Cc: Zhang Rui <rui.zhang@intel.com>
> > Cc: Eduardo Valentin <edubezval@gmail.com>
> > Signed-off-by: Javi Merino <javi.merino@arm.com>
> > ---
> > Documentation/thermal/power_actor.txt | 38 ++++++++++++++++++
> > drivers/thermal/Kconfig | 3 ++
> > drivers/thermal/Makefile | 2 +
> > drivers/thermal/power_actor/Makefile | 5 +++
> > drivers/thermal/power_actor/power_actor.c | 64 +++++++++++++++++++++++++++++++
> > drivers/thermal/power_actor/power_actor.h | 64 +++++++++++++++++++++++++++++++
>
> Do you think this API may have other users other than thermal?
>
> If yes, I propose moving it somewhere else, such as driver/base/power.
>

It could be used by others, but I think we should move it when those
other users appear.

> > 6 files changed, 176 insertions(+)
> > create mode 100644 Documentation/thermal/power_actor.txt
> > create mode 100644 drivers/thermal/power_actor/Makefile
> > create mode 100644 drivers/thermal/power_actor/power_actor.c
> > create mode 100644 drivers/thermal/power_actor/power_actor.h
> >
> > diff --git a/Documentation/thermal/power_actor.txt b/Documentation/thermal/power_actor.txt
> > new file mode 100644
> > index 000000000000..5a61f32ec143
> > --- /dev/null
> > +++ b/Documentation/thermal/power_actor.txt
> > @@ -0,0 +1,38 @@
> > +
> > +Power Actor API
> > +===============
> > +
> > +The base power actor API is meant to be used to derive specific power
>
>
> How about having a deeper explanation? Maybe including one or two
> paragraphs explaning why and when using this API makes sense.
>
> One or two scenario explanation also would help.

Ok, I'll add it.

> An explanation of the difference to cooling API is also good to have.
>
> > +actors, such as a cpu power actor. When registering, they should call
> > +`power_actor_register()` with a unique `enum power_actor_types`. When
>
> Why this enum? Maybe it is a design copy of PM QoS?

To be able to tell different power actors apart. I haven't looked
into PM QoS.

> But to me this is bound to struct device, no?

You're suggesting that we should add this as a field to struct device?
Could do.

> > +unregistering, the power actor should call `power_actor_unregister()`
> > +with the `struct power_actor *` received in the call to
> > +`power_actor_register()`.
> > +
> > +Callbacks
> > +---------
> > +
> > +1. u32 get_req_power(struct power_actor *actor)
> > +@actor: a valid `struct power_actor *` registered with
> > + `power_actor_register()`
> > +
> > +`get_req_power()` returns the current requested power in milliwatts.
> > +
> > +2. u32 get_max_power(struct power_actor *actor)
> > +@actor: a valid `struct power_actor *` registered with
> > + `power_actor_register()`
> > +
> > +`get_max_power()` returns the maximum power that the device could
> > +consume if it was fully utilized. It's a function as some devices'
> > +maximum power consumption can change due to external factors such as
> > +temperature.
> > +
> > +3. int set_power(struct power_actor *actor, u32 power)
> > +@actor: a valid `struct power_actor *` registered with
> > + `power_actor_register()`
> > +@power: power in milliwatts
> > +
> > +`set_power()` should configure the device to consume @power
> > +milliwatts.
> > +
> > +Returns 0 on success, -E* on error.
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > index 2d51912a6e40..47e2f15537ca 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -89,6 +89,9 @@ config THERMAL_GOV_USER_SPACE
> > help
> > Enable this to let the user space manage the platform thermals.
> >
> > +config THERMAL_POWER_ACTOR
> > + bool
> > +
>
> Why empty description/help?

Because it's not an option that users can select.

> > config CPU_THERMAL
> > bool "generic cpu cooling support"
> > depends on CPU_FREQ
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> > index 54e4ec9eb5df..878a02cab7d1 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -14,6 +14,8 @@ thermal_sys-$(CONFIG_THERMAL_GOV_FAIR_SHARE) += fair_share.o
> > thermal_sys-$(CONFIG_THERMAL_GOV_STEP_WISE) += step_wise.o
> > thermal_sys-$(CONFIG_THERMAL_GOV_USER_SPACE) += user_space.o
> >
> > +obj-$(CONFIG_THERMAL_POWER_ACTOR) += power_actor/
> > +
> > # cpufreq cooling
> > thermal_sys-$(CONFIG_CPU_THERMAL) += cpu_cooling.o
> >
> > diff --git a/drivers/thermal/power_actor/Makefile b/drivers/thermal/power_actor/Makefile
> > new file mode 100644
> > index 000000000000..46478f4928be
> > --- /dev/null
> > +++ b/drivers/thermal/power_actor/Makefile
> > @@ -0,0 +1,5 @@
> > +#
> > +# Makefile for the power actors
> > +#
> > +
> > +obj-y += power_actor.o
>
> Why a new directory? Maybe using parent dir is enough?

Ok, I'll move it to the parent directory.

> > diff --git a/drivers/thermal/power_actor/power_actor.c b/drivers/thermal/power_actor/power_actor.c
> > new file mode 100644
> > index 000000000000..d891deb0e2a1
> > --- /dev/null
> > +++ b/drivers/thermal/power_actor/power_actor.c
> > @@ -0,0 +1,64 @@
> > +/*
> > + * Basic interface for power actors
> > + *
> > + * Copyright (C) 2014 ARM Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#define pr_fmt(fmt) "Power actor: " fmt
> > +
> > +#include <linux/err.h>
> > +#include <linux/list.h>
> > +#include <linux/slab.h>
> > +
> > +#include "power_actor.h"
> > +
> > +LIST_HEAD(actor_list);
> > +
> > +/**
> > + * power_actor_register - Register an actor in the power actor API
> > + * @type: actor type
> > + * @ops: struct power_actor_ops for this actor
> > + * @privdata: pointer to private data related to the actor
> > + *
> > + * Returns the struct power_actor * on success, ERR_PTR() on failure
> > + */
> > +struct power_actor *power_actor_register(enum power_actor_types type,
> > + struct power_actor_ops *ops,
> > + void *privdata)
> > +{
> > + struct power_actor *actor;
> > +
> > + if (!ops->get_req_power || !ops->get_max_power || !ops->set_power)
> > + return ERR_PTR(-EINVAL);
> > +
> > + actor = kzalloc(sizeof(*actor), GFP_KERNEL);
>
> devm_?

I can't, there's no device.

> > + if (!actor)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + actor->type = type;
> > + actor->ops = ops;
> > + actor->data = privdata;
> > +
> > + list_add(&actor->actor_node, &actor_list);
> > +
>
> Do you need to protect this one?

Yes, it needs protection.

> > + return actor;
> > +}
> > +
> > +/**
> > + * power_actor_unregister - Unregister an actor
> > + * @actor: the actor to unregister
> > + */
> > +void power_actor_unregister(struct power_actor *actor)
> > +{
> > + list_del(&actor->actor_node);
> > + kfree(actor);
> > +}
> > diff --git a/drivers/thermal/power_actor/power_actor.h b/drivers/thermal/power_actor/power_actor.h
> > new file mode 100644
> > index 000000000000..28098f43630b
> > --- /dev/null
> > +++ b/drivers/thermal/power_actor/power_actor.h
> > @@ -0,0 +1,64 @@
> > +/*
> > + * Copyright (C) 2014 ARM Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#ifndef __POWER_ACTOR_H__
> > +#define __POWER_ACTOR_H__
> > +
> > +#include <linux/list.h>
> > +
> > +#define MAX_NUM_ACTORS 8
> > +
>
> unused define

True, it's a left over, I'll delete it.

> > +enum power_actor_types {
> > +};
>
> I still don't get this enum.

You will when you look at the next patch ;)

Cheers,
Javi