diff mbox series

[v5,2/5] powercap/drivers/dtpm: Create a registering system

Message ID 20210331110048.24956-2-daniel.lezcano@linaro.org
State Superseded
Headers show
Series [v5,1/5] powercap/drivers/dtpm: Encapsulate even more the code | expand

Commit Message

Daniel Lezcano March 31, 2021, 11 a.m. UTC 
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.

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.

Cc: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

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(-)

-- 
2.17.1

Comments

Greg Kroah-Hartman March 31, 2021, 6:04 p.m. UTC | #1 
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?

> 

> Cc: Greg KH <gregkh@linuxfoundation.org>

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> 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
Greg Kroah-Hartman March 31, 2021, 6:06 p.m. UTC | #2 
On Wed, Mar 31, 2021 at 01:00:45PM +0200, Daniel Lezcano wrote:
> +struct dtpm *dtpm_lookup(const char *name);

> +

> +int dtpm_add(const char *name, struct dtpm *dtpm);

> +

> +void dtpm_del(const char *name);


You can not add new kernel apis that have no user.  How do you know if
they actually work or not?  We have no idea as we do not see anyone
using them :(

So no need to add things with no user, feel free to just drop this patch
until you have one.

thanks,

greg k-h
Daniel Lezcano March 31, 2021, 8:46 p.m. UTC | #3 
Hi Greg,

On 31/03/2021 20:06, Greg KH wrote:
> On Wed, Mar 31, 2021 at 01:00:45PM +0200, Daniel Lezcano wrote:

>> +struct dtpm *dtpm_lookup(const char *name);

>> +

>> +int dtpm_add(const char *name, struct dtpm *dtpm);

>> +

>> +void dtpm_del(const char *name);

> 

> You can not add new kernel apis that have no user.  How do you know if

> they actually work or not?  We have no idea as we do not see anyone

> using them :(

> 

> So no need to add things with no user, feel free to just drop this patch

> until you have one.


I've sent a couple of patches [1] on top of the previous series. I'm
finishing to respin it against this new one.

  -- Daniel

[1] https://lkml.org/lkml/2021/3/12/1514


-- 
<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
Greg Kroah-Hartman April 1, 2021, 6:02 a.m. UTC | #4 
On Wed, Mar 31, 2021 at 10:46:48PM +0200, Daniel Lezcano wrote:
> 

> Hi Greg,

> 

> On 31/03/2021 20:06, Greg KH wrote:

> > On Wed, Mar 31, 2021 at 01:00:45PM +0200, Daniel Lezcano wrote:

> >> +struct dtpm *dtpm_lookup(const char *name);

> >> +

> >> +int dtpm_add(const char *name, struct dtpm *dtpm);

> >> +

> >> +void dtpm_del(const char *name);

> > 

> > You can not add new kernel apis that have no user.  How do you know if

> > they actually work or not?  We have no idea as we do not see anyone

> > using them :(

> > 

> > So no need to add things with no user, feel free to just drop this patch

> > until you have one.

> 

> I've sent a couple of patches [1] on top of the previous series. I'm

> finishing to respin it against this new one.

> 

>   -- Daniel

> 

> [1] https://lkml.org/lkml/2021/3/12/1514


Please use lore.kernel.org, we do not control lkml and it has been down
in the past and it's impossible to reply from.

Please always provide a user of a function in the patch series,
otherwise you will end up with comments like mine above.

thanks,

greg k-h
Daniel Lezcano April 1, 2021, 7:01 a.m. UTC | #5 
On 01/04/2021 08:02, Greg KH wrote:
> On Wed, Mar 31, 2021 at 10:46:48PM +0200, Daniel Lezcano wrote:

>>

>> Hi Greg,

>>

>> On 31/03/2021 20:06, Greg KH wrote:

>>> On Wed, Mar 31, 2021 at 01:00:45PM +0200, Daniel Lezcano wrote:

>>>> +struct dtpm *dtpm_lookup(const char *name);

>>>> +

>>>> +int dtpm_add(const char *name, struct dtpm *dtpm);

>>>> +

>>>> +void dtpm_del(const char *name);

>>>

>>> You can not add new kernel apis that have no user.  How do you know if

>>> they actually work or not?  We have no idea as we do not see anyone

>>> using them :(

>>>

>>> So no need to add things with no user, feel free to just drop this patch

>>> until you have one.

>>

>> I've sent a couple of patches [1] on top of the previous series. I'm

>> finishing to respin it against this new one.

>>

>>   -- Daniel

>>

>> [1] https://lkml.org/lkml/2021/3/12/1514

> 

> Please use lore.kernel.org, we do not control lkml and it has been down

> in the past and it's impossible to reply from.

> 

> Please always provide a user of a function in the patch series,

> otherwise you will end up with comments like mine above.


Ok, will do.

Thanks

  -- Daniel


-- 
<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
diff mbox series

Patch

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)
+{
+	struct dtpm_node *node;
+	int ret;
+
+	mutex_lock(&dtpm_lock);
+
+	ret = -EEXIST;
+	if (__dtpm_lookup(name))
+		goto out_unlock;
+
+	ret = -ENOMEM;
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		goto out_unlock;
+
+	node->name = kstrdup(name, GFP_KERNEL);
+	if (!node->name) {
+		kfree(node);
+		goto out_unlock;
+	}
+
+	node->dtpm = dtpm;
+
+	list_add(&node->node, &dtpm_list);
+
+	ret = 0;
+out_unlock:
+	mutex_unlock(&dtpm_lock);
+
+	return ret;
+}
+
+/**
+ * dtpm_del - Remove the dtpm device from the list
+ * @name: the dtpm device name to be removed
+ *
+ * Remove the dtpm device from the list of the registered devices.
+ */
+void dtpm_del(const char *name)
+{
+	struct dtpm_node *node;
+
+	mutex_lock(&dtpm_lock);
+
+	list_for_each_entry(node, &dtpm_list, node) {
+
+		if (strcmp(name, node->name))
+			continue;
+
+		list_del(&node->node);
+		kfree(node->name);
+		kfree(node);
+
+		break;
+	}
+
+	mutex_unlock(&dtpm_lock);
+}
+
 /**
  * dtpm_update_power - Update the power on the dtpm
  * @dtpm: a pointer to a dtpm structure to update
@@ -208,8 +323,6 @@  int dtpm_release_zone(struct powercap_zone *pcz)
 	if (root == dtpm)
 		root = NULL;
 
-	kfree(dtpm);
-
 	return 0;
 }
 
@@ -388,7 +501,7 @@  void dtpm_unregister(struct dtpm *dtpm)
 {
 	powercap_unregister_zone(pct, &dtpm->zone);
 
-	pr_info("Unregistered dtpm node '%s'\n", dtpm->zone.name);
+	pr_debug("Unregistered dtpm node '%s'\n", dtpm->zone.name);
 }
 
 /**
@@ -457,7 +570,7 @@  int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent)
 	if (dtpm->ops && !dtpm->ops->update_power_uw(dtpm))
 		__dtpm_add_power(dtpm);
 
-	pr_info("Registered dtpm node '%s' / %llu-%llu uW, \n",
+	pr_debug("Created dtpm node '%s' / %llu-%llu uW, \n",
 		dtpm->zone.name, dtpm->power_min, dtpm->power_max);
 
 	mutex_unlock(&dtpm_lock);
diff --git a/drivers/powercap/dtpm_cpu.c b/drivers/powercap/dtpm_cpu.c
index f6076de39540..9deafd16a197 100644
--- a/drivers/powercap/dtpm_cpu.c
+++ b/drivers/powercap/dtpm_cpu.c
@@ -177,7 +177,7 @@  static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 
 	snprintf(name, sizeof(name), "cpu%d-cpufreq", dtpm_cpu->cpu);
 
-	ret = dtpm_register(name, dtpm, NULL);
+	ret = dtpm_add(name, dtpm);
 	if (ret)
 		goto out_kfree_dtpm_cpu;
 
@@ -185,12 +185,12 @@  static int cpuhp_dtpm_cpu_online(unsigned int cpu)
 				   &dtpm_cpu->qos_req, FREQ_QOS_MAX,
 				   pd->table[pd->nr_perf_states - 1].frequency);
 	if (ret)
-		goto out_dtpm_unregister;
+		goto out_dtpm_del;
 
 	return 0;
 
-out_dtpm_unregister:
-	dtpm_unregister(dtpm);
+out_dtpm_del:
+	dtpm_del(name);
 	dtpm_cpu = NULL;
 	dtpm = NULL;
 
diff --git a/include/linux/dtpm.h b/include/linux/dtpm.h
index acf8d3638988..577c71d4e098 100644
--- a/include/linux/dtpm.h
+++ b/include/linux/dtpm.h
@@ -75,4 +75,10 @@  int dtpm_register(const char *name, struct dtpm *dtpm, struct dtpm *parent);
 
 int dtpm_register_cpu(struct dtpm *parent);
 
+struct dtpm *dtpm_lookup(const char *name);
+
+int dtpm_add(const char *name, struct dtpm *dtpm);
+
+void dtpm_del(const char *name);
+
 #endif