linux-kernel - Re: [PATCH v5 6/6] qcom/soc/drivers: Add DTPM description for sdm845

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: <8e2fa6b6-4f95-9381-4d7e-810afe98fcea@linaro.org>
Date:   Fri, 7 Jan 2022 23:07:47 +0100
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     rjw@...ysocki.net, lukasz.luba@....com, robh@...nel.org,
        heiko@...ech.de, arnd@...aro.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, ulf.hansson@...aro.org,
        Andy Gross <agross@...nel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v5 6/6] qcom/soc/drivers: Add DTPM description for sdm845


Hi Bjorn,

On 07/01/2022 20:27, Bjorn Andersson wrote:

[ ... ]

>> +#include <linux/dtpm.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +
>> +static struct dtpm_node __initdata sdm845_hierarchy[] = {
>> +	[0]{ .name = "sdm845" },
> 
> Why is the index signifiant here?
> Doesn't this imply risk that we forget one element, which will be
> thereby implicitly be left initialized as {} and hence denote
> termination of the list?

Yes, that is possible. The other annotation is also possible. The index
helps to refer from the .parent field.

That said nothing forces to use the index, so it is a matter of taste.

>> +	[1]{ .name = "package",
>> +	     .parent = &sdm845_hierarchy[0] },
>> +	[2]{ .name = "/cpus/cpu@0",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[3]{ .name = "/cpus/cpu@100",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[4]{ .name = "/cpus/cpu@200",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[5]{ .name = "/cpus/cpu@300",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[6]{ .name = "/cpus/cpu@400",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[7]{ .name = "/cpus/cpu@500",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[8]{ .name = "/cpus/cpu@600",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[9]{ .name = "/cpus/cpu@700",
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[10]{ .name = "/soc@...pu@...0000",
> 
> It worries me that we encode the textual structure of the dts in the
> kernel. E.g. for quite a while this was "/soc/gpu@...0000", so if this
> landed a year ago this driver would have prevented us from correcting
> the dts.

Why ? The change should be reflected in the driver also, no ?

> Another concern is that not all busses in the system are capable of
> 36-bit wide addresses, so it's plausible that we might one day have to
> create a more accurate representation of the address space. Maybe not on
> SDM845, but this would force us to be inconsistent.

Sorry, I'm missing the point :/

If a change is done in the DT, the code using the description must be
changed accordingly, no?


> Regards,
> Bjorn
> 
>> +	     .type = DTPM_NODE_DT,
>> +	     .parent = &sdm845_hierarchy[1] },
>> +	[11]{ },
>> +};
>> +
>> +static struct of_device_id __initdata sdm845_dtpm_match_table[] = {
>> +        { .compatible = "qcom,sdm845", .data = sdm845_hierarchy },
>> +        {},
>> +};
>> +
>> +static int __init sdm845_dtpm_init(void)
>> +{
>> +	return dtpm_create_hierarchy(sdm845_dtpm_match_table);
>> +}
>> +late_initcall(sdm845_dtpm_init);
>> +
>> +MODULE_DESCRIPTION("Qualcomm DTPM driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:dtpm");
>> +MODULE_AUTHOR("Daniel Lezcano <daniel.lezcano@...nel.org");
>> +
>> -- 
>> 2.25.1
>>


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