linux-kernel - Re: [PATCH] clk: fix clock global name usage.

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <c89ecb6f328014ce22ae5d6c634e5337dbbf3ea2.camel@baylibre.com>
Date:   Fri, 24 May 2019 17:00:08 +0200
From:   Jerome Brunet <jbrunet@...libre.com>
To:     Stephen Boyd <sboyd@...nel.org>,
        Alexandre Mergnat <amergnat@...libre.com>,
        mturquette@...libre.com
Cc:     linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        baylibre-upstreaming@...ups.io
Subject: Re: [PATCH] clk: fix clock global name usage.

On Fri, 2019-05-24 at 07:33 -0700, Stephen Boyd wrote:
> Do you set the index to 0 in this clk's parent_data? We purposefully set
> the index to -1 in clk_core_populate_parent_map() so that the fw_name
> can be NULL but the index can be something >= 0 and then we'll use that
> to lookup the clk from DT. We need to support that combination.
> 
>         fw_name   |   index |  DT lookup?
>         ----------+---------+------------
>         NULL      |    >= 0 |     Y
>         NULL      |    -1   |     N
>         non-NULL  |    -1   |     ?
>         non-NULL  |    >= 0 |     Y
> 
> Maybe we should support the ? case, because right now it will fail to do
> the DT lookup when the index is -1.

Hi Stephen,

We are trying to migrate all meson clocks to the new parent structure.
There is a little quirk which forces us to continue to use legacy names
for a couple of clocks.

We heavily use static data which init everything to 0.
Here is an example:

static struct clk_regmap g12a_aoclk_cts_rtc_oscin = {
[...]
 	.hw.init = &(struct clk_init_data){
 		.name = "g12a_ao_cts_rtc_oscin",
 		.ops = &clk_regmap_mux_ops,
-		.parent_names = (const char *[]){ "g12a_ao_32k_by_oscin",
-						  IN_PREFIX "ext_32k-0" },
+		.parent_data = (const struct clk_parent_data []) {
+			{ .name = "g12a_ao_32k_by_oscin" },
+			{ .fw_name = "ext-32k-0", },
+		},
 		.num_parents = 2,
 		.flags = CLK_SET_RATE_PARENT,
 	},
};

With this, instead of taking name = "g12a_ao_32k_by_oscin" for entry #0
it takes DT names at index 0 which is not what we intended.

If I understand correctly we should put
+			{ .name = "g12a_ao_32k_by_oscin", index = -1, },

And would be alright ?

While I understand it, it is not very obvious or nice to look at.
Plus it is a bit weird that this -1 is required for .name and not .hw.

Do you think we could come up with a priority order which makes the first
example work ?

Something like:

if (hw) {
	/* use pointer */
} else if (name) {
	/* use legacy global names */
} else if (fw_name) {
	/* use DT names */
} else if (index >= 0) 
	/* use DT index */
} else {
	return -EINVAL;
}

The last 2 clause could be removed if we make index an unsigned.

Cheers
Jerome

> 
> So this patch instead?