* [PATCH] ARM/dt: Respect #address-cells when parsing CPUs
@ 2016-09-23 15:51 Robin Murphy
2016-09-23 16:01 ` Mark Rutland
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Robin Murphy @ 2016-09-23 15:51 UTC (permalink / raw)
To: linux-arm-kernel
Whilst MPIDR values themselves are only 32 bits, it is still perfectly
valid for a DT to have #address-cells > 1 in the CPUs node, resulting in
the "reg" property having leading zero cell(s). In that situation, the
big-endian nature of the data conspires with the current behaviour of
just parsing the first cell to cause the kernel to think all CPUs have
ID 0, and become resoundingly unhappy as a consequence.
Take #address-cells into account when parsing CPUs so as to be correct
under any circumstances.
CC: Russell King <linux@armlinux.org.uk>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
arch/arm/kernel/devtree.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
index 40ecd5f514a2..7ab113c5c818 100644
--- a/arch/arm/kernel/devtree.c
+++ b/arch/arm/kernel/devtree.c
@@ -88,6 +88,7 @@ void __init arm_dt_init_cpu_maps(void)
return;
for_each_child_of_node(cpus, cpu) {
+ const u32 *cell;
u32 hwid;
if (of_node_cmp(cpu->type, "cpu"))
@@ -99,12 +100,14 @@ void __init arm_dt_init_cpu_maps(void)
* properties is considered invalid to build the
* cpu_logical_map.
*/
- if (of_property_read_u32(cpu, "reg", &hwid)) {
+ cell = of_get_property(cpu, "reg", NULL);
+ if (!cell) {
pr_debug(" * %s missing reg property\n",
cpu->full_name);
of_node_put(cpu);
return;
}
+ hwid = of_read_number(cell, of_n_addr_cells(cpu));
/*
* 8 MSBs must be set to 0 in the DT since the reg property
--
2.8.1.dirty
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH] ARM/dt: Respect #address-cells when parsing CPUs
2016-09-23 15:51 [PATCH] ARM/dt: Respect #address-cells when parsing CPUs Robin Murphy
@ 2016-09-23 16:01 ` Mark Rutland
2016-09-23 16:57 ` Sudeep Holla
2016-09-26 14:25 ` [PATCH v2] ARM/dt: Respect property size " Robin Murphy
2 siblings, 0 replies; 5+ messages in thread
From: Mark Rutland @ 2016-09-23 16:01 UTC (permalink / raw)
To: linux-arm-kernel
Hi Robin,
On Fri, Sep 23, 2016 at 04:51:48PM +0100, Robin Murphy wrote:
> Whilst MPIDR values themselves are only 32 bits, it is still perfectly
> valid for a DT to have #address-cells > 1 in the CPUs node, resulting in
> the "reg" property having leading zero cell(s). In that situation, the
> big-endian nature of the data conspires with the current behaviour of
> just parsing the first cell to cause the kernel to think all CPUs have
> ID 0, and become resoundingly unhappy as a consequence.
>
> Take #address-cells into account when parsing CPUs so as to be correct
> under any circumstances.
To be correct under any circumstances you'll also need to verify that
the upper bits (e.g. Aff3) are zero, and handle the non-zero case
somewhow.
As it stands, this can have the kernel think that MPIDR.Aff* values
exist which were never in the DT.
Thanks,
Mark.
> CC: Russell King <linux@armlinux.org.uk>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> arch/arm/kernel/devtree.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
> index 40ecd5f514a2..7ab113c5c818 100644
> --- a/arch/arm/kernel/devtree.c
> +++ b/arch/arm/kernel/devtree.c
> @@ -88,6 +88,7 @@ void __init arm_dt_init_cpu_maps(void)
> return;
>
> for_each_child_of_node(cpus, cpu) {
> + const u32 *cell;
> u32 hwid;
>
> if (of_node_cmp(cpu->type, "cpu"))
> @@ -99,12 +100,14 @@ void __init arm_dt_init_cpu_maps(void)
> * properties is considered invalid to build the
> * cpu_logical_map.
> */
> - if (of_property_read_u32(cpu, "reg", &hwid)) {
> + cell = of_get_property(cpu, "reg", NULL);
> + if (!cell) {
> pr_debug(" * %s missing reg property\n",
> cpu->full_name);
> of_node_put(cpu);
> return;
> }
> + hwid = of_read_number(cell, of_n_addr_cells(cpu));
>
> /*
> * 8 MSBs must be set to 0 in the DT since the reg property
> --
> 2.8.1.dirty
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] ARM/dt: Respect #address-cells when parsing CPUs
2016-09-23 15:51 [PATCH] ARM/dt: Respect #address-cells when parsing CPUs Robin Murphy
2016-09-23 16:01 ` Mark Rutland
@ 2016-09-23 16:57 ` Sudeep Holla
2016-09-26 14:25 ` [PATCH v2] ARM/dt: Respect property size " Robin Murphy
2 siblings, 0 replies; 5+ messages in thread
From: Sudeep Holla @ 2016-09-23 16:57 UTC (permalink / raw)
To: linux-arm-kernel
On 23/09/16 16:51, Robin Murphy wrote:
> Whilst MPIDR values themselves are only 32 bits, it is still perfectly
> valid for a DT to have #address-cells > 1 in the CPUs node, resulting in
> the "reg" property having leading zero cell(s). In that situation, the
> big-endian nature of the data conspires with the current behaviour of
> just parsing the first cell to cause the kernel to think all CPUs have
> ID 0, and become resoundingly unhappy as a consequence.
>
> Take #address-cells into account when parsing CPUs so as to be correct
> under any circumstances.
>
Lorenzo had a patch [1] to address this a while ago. I assume all the
other patches in the series have been merged hopefully which fixes the
device trees.
--
Regards,
Sudeep
[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/169093.html
[2]
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-May/169083.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] ARM/dt: Respect property size when parsing CPUs
2016-09-23 15:51 [PATCH] ARM/dt: Respect #address-cells when parsing CPUs Robin Murphy
2016-09-23 16:01 ` Mark Rutland
2016-09-23 16:57 ` Sudeep Holla
@ 2016-09-26 14:25 ` Robin Murphy
2016-09-26 15:19 ` Russell King - ARM Linux
2 siblings, 1 reply; 5+ messages in thread
From: Robin Murphy @ 2016-09-26 14:25 UTC (permalink / raw)
To: linux-arm-kernel
Whilst MPIDR values themselves are less than 32 bits, it is still
perfectly valid for a DT to have #address-cells > 1 in the CPUs node,
resulting in the "reg" property having leading zero cell(s). In that
situation, the big-endian nature of the data conspires with the current
behaviour of only reading the first cell to cause the kernel to think
all CPUs have ID 0, and become resoundingly unhappy as a consequence.
Take the full property length into account when parsing CPUs so as to
be correct under any circumstances.
CC: Russell King <linux@armlinux.org.uk>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
v2: Check that any and all upper cells are zero rather than just
ignoring them.
I think if we want to properly validate the DT as per Lorenzo's old
patch, that can follow on afterwards; here I'm really just concerned
with making sure the existing behaviour is actually robust and correct.
arch/arm/kernel/devtree.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
index 40ecd5f514a2..f676febbb270 100644
--- a/arch/arm/kernel/devtree.c
+++ b/arch/arm/kernel/devtree.c
@@ -88,6 +88,8 @@ void __init arm_dt_init_cpu_maps(void)
return;
for_each_child_of_node(cpus, cpu) {
+ const __be32 *cell;
+ int prop_bytes;
u32 hwid;
if (of_node_cmp(cpu->type, "cpu"))
@@ -99,7 +101,8 @@ void __init arm_dt_init_cpu_maps(void)
* properties is considered invalid to build the
* cpu_logical_map.
*/
- if (of_property_read_u32(cpu, "reg", &hwid)) {
+ cell = of_get_property(cpu, "reg", &prop_bytes);
+ if (!cell || prop_bytes < sizeof(*cell)) {
pr_debug(" * %s missing reg property\n",
cpu->full_name);
of_node_put(cpu);
@@ -107,10 +110,15 @@ void __init arm_dt_init_cpu_maps(void)
}
/*
- * 8 MSBs must be set to 0 in the DT since the reg property
+ * Bits n:24 must be set to 0 in the DT since the reg property
* defines the MPIDR[23:0].
*/
- if (hwid & ~MPIDR_HWID_BITMASK) {
+ do {
+ hwid = be32_to_cpu(*cell++);
+ prop_bytes -= sizeof(*cell);
+ } while (!hwid && prop_bytes > 0);
+
+ if (prop_bytes || (hwid & ~MPIDR_HWID_BITMASK)) {
of_node_put(cpu);
return;
}
--
2.8.1.dirty
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH v2] ARM/dt: Respect property size when parsing CPUs
2016-09-26 14:25 ` [PATCH v2] ARM/dt: Respect property size " Robin Murphy
@ 2016-09-26 15:19 ` Russell King - ARM Linux
0 siblings, 0 replies; 5+ messages in thread
From: Russell King - ARM Linux @ 2016-09-26 15:19 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Sep 26, 2016 at 03:25:24PM +0100, Robin Murphy wrote:
> Whilst MPIDR values themselves are less than 32 bits, it is still
> perfectly valid for a DT to have #address-cells > 1 in the CPUs node,
> resulting in the "reg" property having leading zero cell(s). In that
> situation, the big-endian nature of the data conspires with the current
> behaviour of only reading the first cell to cause the kernel to think
> all CPUs have ID 0, and become resoundingly unhappy as a consequence.
>
> Take the full property length into account when parsing CPUs so as to
> be correct under any circumstances.
Please drop this into the patch system, thanks.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-09-26 15:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-23 15:51 [PATCH] ARM/dt: Respect #address-cells when parsing CPUs Robin Murphy
2016-09-23 16:01 ` Mark Rutland
2016-09-23 16:57 ` Sudeep Holla
2016-09-26 14:25 ` [PATCH v2] ARM/dt: Respect property size " Robin Murphy
2016-09-26 15:19 ` Russell King - ARM Linux
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).