Re: [PATCH v5 2/5] powercap/drivers/dtpm: Create a registering system
From: Greg KH <gregkh@linuxfoundation.org>
Date: 2021-03-31 18:05:25
Also in:
lkml
On Wed, Mar 31, 2021 at 01:00:45PM +0200, Daniel Lezcano wrote:
A SoC can be differently structured depending on the platform and the kernel can not be aware of all the combinations, as well as the specific tweaks for a particular board. The creation of the hierarchy must be delegated to userspace.
Isn't that what DT is for?
These changes provide a registering mechanism where the different subsystems will initialize their dtpm backends and register with a name the dtpm node in a list. The next changes will provide an userspace interface to create hierarchically the different nodes. Those will be created by name and found via the list filled by the different subsystem. If a specified name is not found in the list, it is assumed to be a virtual node which will have children and the default is to allocate such node.
There's no userspace portion here, so why talk about it?
quoted hunk ↗ jump to hunk
Cc: Greg KH <gregkh@linuxfoundation.org> Signed-off-by: Daniel Lezcano <redacted> Reviewed-by: Lukasz Luba <lukasz.luba@arm.com> --- V5: - Decrease log level from 'info' to 'debug' - Remove the refcount, it is pointless, lifetime cycle is already handled by the device refcounting. The dtpm node allocator is in charge of freeing it. - Rename the functions to 'dtpm_add, dtpm_del, dtpm_lookup' - Fix missing kfrees when deleting the node from the list V4: - Fixed typo in the commit log V2: - Fixed error code path by dropping lock --- drivers/powercap/dtpm.c | 121 ++++++++++++++++++++++++++++++++++-- drivers/powercap/dtpm_cpu.c | 8 +-- include/linux/dtpm.h | 6 ++ 3 files changed, 127 insertions(+), 8 deletions(-)diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c index 58433b8ef9a1..8df7adeed0cf 100644 --- a/drivers/powercap/dtpm.c +++ b/drivers/powercap/dtpm.c@@ -34,6 +34,14 @@ static DEFINE_MUTEX(dtpm_lock); static struct powercap_control_type *pct; static struct dtpm *root; +struct dtpm_node { + const char *name; + struct dtpm *dtpm; + struct list_head node; +}; + +static LIST_HEAD(dtpm_list); + static int get_time_window_us(struct powercap_zone *pcz, int cid, u64 *window) { return -ENOSYS;@@ -152,6 +160,113 @@ static int __dtpm_update_power(struct dtpm *dtpm) return ret; } +static struct dtpm *__dtpm_lookup(const char *name) +{ + struct dtpm_node *node; + + list_for_each_entry(node, &dtpm_list, node) { + if (!strcmp(name, node->name)) + return node->dtpm; + } + + return NULL; +} + +/** + * dtpm_lookup - Lookup for a registered dtpm node given its name + * @name: the name of the dtpm device + * + * The function looks up in the list of the registered dtpm + * devices. This function must be called to create a dtpm node in the + * powercap hierarchy. + * + * Return: a pointer to a dtpm structure, NULL if not found. + */ +struct dtpm *dtpm_lookup(const char *name) +{ + struct dtpm *dtpm; + + mutex_lock(&dtpm_lock); + dtpm = __dtpm_lookup(name); + mutex_unlock(&dtpm_lock); + + return dtpm; +} + +/** + * dtpm_add - Add the dtpm in the dtpm list + * @name: a name used as an identifier + * @dtpm: the dtpm node to be registered + * + * Stores the dtpm device in a list. The list contains all the devices + * which are power capable in terms of limitation and power + * consumption measurements. Even if conceptually, a power capable + * device won't register itself twice, the function will check if it + * was already registered in order to prevent a misuse of the API. + * + * Return: 0 on success, -EEXIST if the device name is already present + * in the list, -ENOMEM in case of memory allocation failure. + */ +int dtpm_add(const char *name, struct dtpm *dtpm)
Why not just use the name of the dtpm? Where does this "new" name come from? And why would it differ?
+{
+ struct dtpm_node *node;
+ int ret;
+
+ mutex_lock(&dtpm_lock);
+
+ ret = -EEXIST;
+ if (__dtpm_lookup(name))
+ goto out_unlock;Why do you have yet-another-list of these devices? They are already all on a list, why do you need another? And you seem to be ignoring reference count issues here, you add a reference counted pointer to a random list in the kernel and never increment the reference count. That's bad :( So just don't use a new list please... thanks, greg k-h