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

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8f17dc8b-3259-8e6a-f46b-b97495ecd550@arm.com>
Date:   Fri, 12 Mar 2021 11:15:35 +0000
From:   Lukasz Luba <lukasz.luba@....com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     rui.zhang@...el.com, amitk@...nel.org, linux-pm@...r.kernel.org,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] thermal/drivers/devfreq_cooling: Use device name
 instead of auto-numbering



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@...aro.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.