[2/3] thermal/drivers/devfreq_cooling: Use device name instead of auto-numbering

Message ID 20210310114600.27178-2-daniel.lezcano@linaro.org
State New
Headers show
Series
  • [1/3] thermal/drivers/cpufreq_cooling: Use device name instead of auto-numbering
Related show

Commit Message

Daniel Lezcano March 10, 2021, 11:45 a.m.
Currently the naming of a cooling device is just a cooling technique
followed by a number. When there are multiple cooling devices using
the same technique, it is impossible to clearly identify the related
device as this one is just a number.

For instance:

 thermal-devfreq-0
 thermal-devfreq-1
 etc ...

The 'thermal' prefix is redundant with the subsystem namespace. This
patch removes the 'thermal prefix and changes the number by the device
name. So the naming above becomes:

 devfreq-5000000.gpu
 devfreq-1d84000.ufshc
 etc ...

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

---
 drivers/thermal/devfreq_cooling.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

-- 
2.17.1

Comments

Lukasz Luba March 12, 2021, 11:15 a.m. | #1
On 3/10/21 11:45 AM, Daniel Lezcano wrote:
> Currently the naming of a cooling device is just a cooling technique

> followed by a number. When there are multiple cooling devices using

> the same technique, it is impossible to clearly identify the related

> device as this one is just a number.

> 

> For instance:

> 

>   thermal-devfreq-0

>   thermal-devfreq-1

>   etc ...

> 

> The 'thermal' prefix is redundant with the subsystem namespace. This

> patch removes the 'thermal prefix and changes the number by the device


missing ' after 'thermal

> name. So the naming above becomes:

> 

>   devfreq-5000000.gpu

>   devfreq-1d84000.ufshc

>   etc ...

> 

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

> ---

>   drivers/thermal/devfreq_cooling.c | 21 ++++-----------------

>   1 file changed, 4 insertions(+), 17 deletions(-)

> 

> diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c

> index fed3121ff2a1..62abcffeb422 100644

> --- a/drivers/thermal/devfreq_cooling.c

> +++ b/drivers/thermal/devfreq_cooling.c


same here, you can now remove the idr.h header

> @@ -25,11 +25,8 @@

>   #define HZ_PER_KHZ		1000

>   #define SCALE_ERROR_MITIGATION	100

>   

> -static DEFINE_IDA(devfreq_ida);

> -

>   /**

>    * struct devfreq_cooling_device - Devfreq cooling device

> - * @id:		unique integer value corresponding to each

>    *		devfreq_cooling_device registered.

>    * @cdev:	Pointer to associated thermal cooling device.

>    * @devfreq:	Pointer to associated devfreq device.

> @@ -51,7 +48,6 @@ static DEFINE_IDA(devfreq_ida);

>    * @em_pd:		Energy Model for the associated Devfreq device

>    */

>   struct devfreq_cooling_device {

> -	int id;

>   	struct thermal_cooling_device *cdev;

>   	struct devfreq *devfreq;

>   	unsigned long cooling_state;

> @@ -363,7 +359,7 @@ of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,

>   	struct thermal_cooling_device *cdev;

>   	struct device *dev = df->dev.parent;

>   	struct devfreq_cooling_device *dfc;

> -	char dev_name[THERMAL_NAME_LENGTH];

> +	char name[THERMAL_NAME_LENGTH];


This is probably too short (20 char) array. For example in my phone's
devfreq dir, there are really long names:

---------------------------------------------------------
redfin:/sys/class/devfreq # for f in `ls ./` ; do echo $f; cat $f/name | 
wc -m ; done 

18321000.qcom,devfreq-l3:qcom,cdsp-cdsp-l3-lat
47
18321000.qcom,devfreq-l3:qcom,cpu0-cpu-l3-lat
46
18321000.qcom,devfreq-l3:qcom,cpu6-cpu-l3-lat
46
18321000.qcom,devfreq-l3:qcom,cpu7-cpu-l3-lat
46
3d00000.qcom,kgsl-3d0
22
soc:qcom,cpu-cpu-llcc-bw
25
soc:qcom,cpu-llcc-ddr-bw
25
soc:qcom,cpu0-cpu-ddr-latfloor
31
soc:qcom,cpu0-cpu-llcc-lat
27
soc:qcom,cpu0-llcc-ddr-lat
27
soc:qcom,cpu6-cpu-ddr-latfloor
31
soc:qcom,cpu6-cpu-llcc-lat
27
soc:qcom,cpu6-llcc-ddr-lat
27
soc:qcom,cpu7-cpu-ddr-latfloor
31
soc:qcom,gpubw
15
soc:qcom,kgsl-busmon
21
soc:qcom,npu-llcc-ddr-bw
25
soc:qcom,npu-npu-llcc-bw
25
soc:qcom,npudsp-npu-ddr-bw
27
soc:qcom,snoc_cnoc_keepalive
29
---------------------------------------------------------

We should allocate tmp buffer for it, to not loose the meaningful part
of that string name or end up with only the same prefix, like for the
first 3 from top:

devfreq-18321000.qco

or for the GPU:
devfreq-3d00000.qcom

This is tricky area and vendors might put any non-meaningful prefix.

The rest of the code looks OK, only this name construction part.
Daniel Lezcano March 12, 2021, 3:53 p.m. | #2
On 12/03/2021 12:15, Lukasz Luba wrote:
> 

> 

> On 3/10/21 11:45 AM, Daniel Lezcano wrote:

>> Currently the naming of a cooling device is just a cooling technique

>> followed by a number. When there are multiple cooling devices using

>> the same technique, it is impossible to clearly identify the related

>> device as this one is just a number.

>>

>> For instance:

>>

>>   thermal-devfreq-0

>>   thermal-devfreq-1

>>   etc ...

>>

>> The 'thermal' prefix is redundant with the subsystem namespace. This

>> patch removes the 'thermal prefix and changes the number by the device

> 

> missing ' after 'thermal

> 

>> name. So the naming above becomes:

>>

>>   devfreq-5000000.gpu

>>   devfreq-1d84000.ufshc

>>   etc ...

>>

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

>> ---


[ ... ]

> ---------------------------------------------------------

> 

> We should allocate tmp buffer for it, to not loose the meaningful part

> of that string name or end up with only the same prefix, like for the

> first 3 from top:

> 

> devfreq-18321000.qco

> 

> or for the GPU:

> devfreq-3d00000.qcom

> 

> This is tricky area and vendors might put any non-meaningful prefix.

> 

> The rest of the code looks OK, only this name construction part.


That requires a change in the thermal_core code to replace the strlcpy
into the cdev->type by a kstrdup.

Otherwise the name will be truncated in any case by the underlying
thermal_cooling_device_register() function.


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

Patch

diff --git a/drivers/thermal/devfreq_cooling.c b/drivers/thermal/devfreq_cooling.c
index fed3121ff2a1..62abcffeb422 100644
--- a/drivers/thermal/devfreq_cooling.c
+++ b/drivers/thermal/devfreq_cooling.c
@@ -25,11 +25,8 @@ 
 #define HZ_PER_KHZ		1000
 #define SCALE_ERROR_MITIGATION	100
 
-static DEFINE_IDA(devfreq_ida);
-
 /**
  * struct devfreq_cooling_device - Devfreq cooling device
- * @id:		unique integer value corresponding to each
  *		devfreq_cooling_device registered.
  * @cdev:	Pointer to associated thermal cooling device.
  * @devfreq:	Pointer to associated devfreq device.
@@ -51,7 +48,6 @@  static DEFINE_IDA(devfreq_ida);
  * @em_pd:		Energy Model for the associated Devfreq device
  */
 struct devfreq_cooling_device {
-	int id;
 	struct thermal_cooling_device *cdev;
 	struct devfreq *devfreq;
 	unsigned long cooling_state;
@@ -363,7 +359,7 @@  of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
 	struct thermal_cooling_device *cdev;
 	struct device *dev = df->dev.parent;
 	struct devfreq_cooling_device *dfc;
-	char dev_name[THERMAL_NAME_LENGTH];
+	char name[THERMAL_NAME_LENGTH];
 	int err, num_opps;
 
 	dfc = kzalloc(sizeof(*dfc), GFP_KERNEL);
@@ -407,30 +403,22 @@  of_devfreq_cooling_register_power(struct device_node *np, struct devfreq *df,
 	if (err < 0)
 		goto free_table;
 
-	err = ida_simple_get(&devfreq_ida, 0, 0, GFP_KERNEL);
-	if (err < 0)
-		goto remove_qos_req;
-
-	dfc->id = err;
-
-	snprintf(dev_name, sizeof(dev_name), "thermal-devfreq-%d", dfc->id);
+	snprintf(name, sizeof(name), "devfreq-%s", dev_name(dev));
 
-	cdev = thermal_of_cooling_device_register(np, dev_name, dfc,
+	cdev = thermal_of_cooling_device_register(np, name, dfc,
 						  &devfreq_cooling_ops);
 	if (IS_ERR(cdev)) {
 		err = PTR_ERR(cdev);
 		dev_err(dev,
 			"Failed to register devfreq cooling device (%d)\n",
 			err);
-		goto release_ida;
+		goto remove_qos_req;
 	}
 
 	dfc->cdev = cdev;
 
 	return cdev;
 
-release_ida:
-	ida_simple_remove(&devfreq_ida, dfc->id);
 remove_qos_req:
 	dev_pm_qos_remove_request(&dfc->req_max_freq);
 free_table:
@@ -527,7 +515,6 @@  void devfreq_cooling_unregister(struct thermal_cooling_device *cdev)
 	dev = dfc->devfreq->dev.parent;
 
 	thermal_cooling_device_unregister(dfc->cdev);
-	ida_simple_remove(&devfreq_ida, dfc->id);
 	dev_pm_qos_remove_request(&dfc->req_max_freq);
 
 	em_dev_unregister_perf_domain(dev);