| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 12 Jun 2014 14:45:20 +0100
From: "Javi Merino" <javi.merino@....com>
To: Eduardo Valentin <edubezval@...il.com>
Cc: "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Punit Agrawal <Punit.Agrawal@....com>,
Zhang Rui <rui.zhang@...el.com>
Subject: Re: [RFC PATCH v3 4/7] thermal: introduce the Power Actor API
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@...el.com>
> > Cc: Eduardo Valentin <edubezval@...il.com>
> > Signed-off-by: Javi Merino <javi.merino@....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)
> > +@...or: 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)
> > +@...or: 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)
> > +@...or: a valid `struct power_actor *` registered with
> > + `power_actor_register()`
> > +@...er: 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists