* [PATCH v2] of: Check for overlap in reserved memory regions
@ 2015-09-16 1:30 ` Mitchel Humpherys
0 siblings, 0 replies; 13+ messages in thread
From: Mitchel Humpherys @ 2015-09-16 1:30 UTC (permalink / raw)
To: Rob Herring, Frank Rowand, Grant Likely,
devicetree-u79uwXL29TY76Z2rM5mHXA, Marek Szyprowski
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Mitchel Humpherys
Any overlap in the reserved memory regions (those specified in the
reserved-memory DT node) is a bug. These bugs might go undetected as
long as the contested region isn't used simultaneously by multiple
software agents, which makes such bugs hard to debug. Fix this by
printing a scary warning during boot if overlap is detected.
Signed-off-by: Mitchel Humpherys <mitchelh-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
v1..v2:
- Suggestions from Rob Herring (remove superfluous array and
print statement)
---
drivers/of/of_reserved_mem.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 42 insertions(+), 1 deletion(-)
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 726ebe792813..62f467b8ccae 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -1,7 +1,7 @@
/*
* Device tree based initialization code for reserved memory.
*
- * Copyright (c) 2013, The Linux Foundation. All Rights Reserved.
+ * Copyright (c) 2013, 2015 The Linux Foundation. All Rights Reserved.
* Copyright (c) 2013,2014 Samsung Electronics Co., Ltd.
* http://www.samsung.com
* Author: Marek Szyprowski <m.szyprowski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
@@ -20,6 +20,7 @@
#include <linux/mm.h>
#include <linux/sizes.h>
#include <linux/of_reserved_mem.h>
+#include <linux/sort.h>
#define MAX_RESERVED_REGIONS 16
static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
@@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
return -ENOENT;
}
+static int __init __rmem_cmp(const void *a, const void *b)
+{
+ const struct reserved_mem *ra = a, *rb = b;
+
+ return ra->base - rb->base;
+}
+
+static void __init __rmem_check_for_overlap(void)
+{
+ int i;
+
+ if (reserved_mem_count < 2)
+ return;
+
+ sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
+ __rmem_cmp, NULL);
+ for (i = 0; i < reserved_mem_count - 1; i++) {
+ struct reserved_mem *this, *next;
+
+ this = &reserved_mem[i];
+ next = &reserved_mem[i + 1];
+ if (!(this->base && next->base))
+ continue;
+ if (this->base + this->size > next->base) {
+ phys_addr_t this_end, next_end;
+
+ this_end = this->base + this->size;
+ next_end = next->base + next->size;
+ WARN(1,
+ "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
+ this->name, &this->base, &this_end,
+ next->name, &next->base, &next_end);
+ }
+ }
+}
+
/**
* fdt_init_reserved_mem - allocate and init all saved reserved memory regions
*/
void __init fdt_init_reserved_mem(void)
{
int i;
+
+ /* check for overlapping reserved regions */
+ __rmem_check_for_overlap();
+
for (i = 0; i < reserved_mem_count; i++) {
struct reserved_mem *rmem = &reserved_mem[i];
unsigned long node = rmem->fdt_node;
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2] of: Check for overlap in reserved memory regions
2015-09-16 1:30 ` Mitchel Humpherys
(?)
@ 2015-10-13 20:13 ` Rob Herring
-1 siblings, 0 replies; 13+ messages in thread
From: Rob Herring @ 2015-10-13 20:13 UTC (permalink / raw)
To: Mitchel Humpherys
Cc: Frank Rowand, Grant Likely, devicetree, Marek Szyprowski, linux-kernel
On Tue, Sep 15, 2015 at 8:30 PM, Mitchel Humpherys
<mitchelh@codeaurora.org> wrote:
> Any overlap in the reserved memory regions (those specified in the
> reserved-memory DT node) is a bug. These bugs might go undetected as
> long as the contested region isn't used simultaneously by multiple
> software agents, which makes such bugs hard to debug. Fix this by
> printing a scary warning during boot if overlap is detected.
>
> Signed-off-by: Mitchel Humpherys <mitchelh@codeaurora.org>
Applied this a while ago, but didn't reply here.
Rob
> ---
> v1..v2:
> - Suggestions from Rob Herring (remove superfluous array and
> print statement)
> ---
> drivers/of/of_reserved_mem.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 726ebe792813..62f467b8ccae 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -1,7 +1,7 @@
> /*
> * Device tree based initialization code for reserved memory.
> *
> - * Copyright (c) 2013, The Linux Foundation. All Rights Reserved.
> + * Copyright (c) 2013, 2015 The Linux Foundation. All Rights Reserved.
> * Copyright (c) 2013,2014 Samsung Electronics Co., Ltd.
> * http://www.samsung.com
> * Author: Marek Szyprowski <m.szyprowski@samsung.com>
> @@ -20,6 +20,7 @@
> #include <linux/mm.h>
> #include <linux/sizes.h>
> #include <linux/of_reserved_mem.h>
> +#include <linux/sort.h>
>
> #define MAX_RESERVED_REGIONS 16
> static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
> @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
> return -ENOENT;
> }
>
> +static int __init __rmem_cmp(const void *a, const void *b)
> +{
> + const struct reserved_mem *ra = a, *rb = b;
> +
> + return ra->base - rb->base;
> +}
> +
> +static void __init __rmem_check_for_overlap(void)
> +{
> + int i;
> +
> + if (reserved_mem_count < 2)
> + return;
> +
> + sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
> + __rmem_cmp, NULL);
> + for (i = 0; i < reserved_mem_count - 1; i++) {
> + struct reserved_mem *this, *next;
> +
> + this = &reserved_mem[i];
> + next = &reserved_mem[i + 1];
> + if (!(this->base && next->base))
> + continue;
> + if (this->base + this->size > next->base) {
> + phys_addr_t this_end, next_end;
> +
> + this_end = this->base + this->size;
> + next_end = next->base + next->size;
> + WARN(1,
> + "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
> + this->name, &this->base, &this_end,
> + next->name, &next->base, &next_end);
> + }
> + }
> +}
> +
> /**
> * fdt_init_reserved_mem - allocate and init all saved reserved memory regions
> */
> void __init fdt_init_reserved_mem(void)
> {
> int i;
> +
> + /* check for overlapping reserved regions */
> + __rmem_check_for_overlap();
> +
> for (i = 0; i < reserved_mem_count; i++) {
> struct reserved_mem *rmem = &reserved_mem[i];
> unsigned long node = rmem->fdt_node;
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] of: Check for overlap in reserved memory regions
@ 2015-11-10 4:29 ` Michael Ellerman
0 siblings, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2015-11-10 4:29 UTC (permalink / raw)
To: Mitchel Humpherys, Rob Herring, Frank Rowand, Grant Likely,
devicetree, Marek Szyprowski
Cc: linux-kernel, linuxppc-dev
On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
> Any overlap in the reserved memory regions (those specified in the
> reserved-memory DT node) is a bug. These bugs might go undetected as
> long as the contested region isn't used simultaneously by multiple
> software agents, which makes such bugs hard to debug. Fix this by
> printing a scary warning during boot if overlap is detected.
>
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 726ebe792813..62f467b8ccae 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
> return -ENOENT;
> }
>
> +static int __init __rmem_cmp(const void *a, const void *b)
> +{
> + const struct reserved_mem *ra = a, *rb = b;
> +
> + return ra->base - rb->base;
> +}
> +
> +static void __init __rmem_check_for_overlap(void)
> +{
> + int i;
> +
> + if (reserved_mem_count < 2)
> + return;
> +
> + sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
> + __rmem_cmp, NULL);
> + for (i = 0; i < reserved_mem_count - 1; i++) {
> + struct reserved_mem *this, *next;
> +
> + this = &reserved_mem[i];
> + next = &reserved_mem[i + 1];
> + if (!(this->base && next->base))
> + continue;
> + if (this->base + this->size > next->base) {
> + phys_addr_t this_end, next_end;
> +
> + this_end = this->base + this->size;
> + next_end = next->base + next->size;
> + WARN(1,
> + "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
> + this->name, &this->base, &this_end,
> + next->name, &next->base, &next_end);
This is blowing up on some powerpc machines.
It's too early in boot to call WARN() on these systems.
Can we turn it into a pr_err() for now?
I'll send a patch?
cheers
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] of: Check for overlap in reserved memory regions
@ 2015-11-10 4:29 ` Michael Ellerman
0 siblings, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2015-11-10 4:29 UTC (permalink / raw)
To: Mitchel Humpherys, Rob Herring, Frank Rowand, Grant Likely,
devicetree-u79uwXL29TY76Z2rM5mHXA, Marek Szyprowski
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ
On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
> Any overlap in the reserved memory regions (those specified in the
> reserved-memory DT node) is a bug. These bugs might go undetected as
> long as the contested region isn't used simultaneously by multiple
> software agents, which makes such bugs hard to debug. Fix this by
> printing a scary warning during boot if overlap is detected.
>
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 726ebe792813..62f467b8ccae 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
> return -ENOENT;
> }
>
> +static int __init __rmem_cmp(const void *a, const void *b)
> +{
> + const struct reserved_mem *ra = a, *rb = b;
> +
> + return ra->base - rb->base;
> +}
> +
> +static void __init __rmem_check_for_overlap(void)
> +{
> + int i;
> +
> + if (reserved_mem_count < 2)
> + return;
> +
> + sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
> + __rmem_cmp, NULL);
> + for (i = 0; i < reserved_mem_count - 1; i++) {
> + struct reserved_mem *this, *next;
> +
> + this = &reserved_mem[i];
> + next = &reserved_mem[i + 1];
> + if (!(this->base && next->base))
> + continue;
> + if (this->base + this->size > next->base) {
> + phys_addr_t this_end, next_end;
> +
> + this_end = this->base + this->size;
> + next_end = next->base + next->size;
> + WARN(1,
> + "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
> + this->name, &this->base, &this_end,
> + next->name, &next->base, &next_end);
This is blowing up on some powerpc machines.
It's too early in boot to call WARN() on these systems.
Can we turn it into a pr_err() for now?
I'll send a patch?
cheers
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] of: Check for overlap in reserved memory regions
2015-11-10 4:29 ` Michael Ellerman
@ 2015-11-10 4:41 ` Rob Herring
-1 siblings, 0 replies; 13+ messages in thread
From: Rob Herring @ 2015-11-10 4:41 UTC (permalink / raw)
To: Michael Ellerman
Cc: Mitchel Humpherys, Frank Rowand, Grant Likely, devicetree,
Marek Szyprowski, linux-kernel, linuxppc-dev
On Mon, Nov 9, 2015 at 10:29 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
>
>> Any overlap in the reserved memory regions (those specified in the
>> reserved-memory DT node) is a bug. These bugs might go undetected as
>> long as the contested region isn't used simultaneously by multiple
>> software agents, which makes such bugs hard to debug. Fix this by
>> printing a scary warning during boot if overlap is detected.
>>
>> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
>> index 726ebe792813..62f467b8ccae 100644
>> --- a/drivers/of/of_reserved_mem.c
>> +++ b/drivers/of/of_reserved_mem.c
>> @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
>> return -ENOENT;
>> }
>>
>> +static int __init __rmem_cmp(const void *a, const void *b)
>> +{
>> + const struct reserved_mem *ra = a, *rb = b;
>> +
>> + return ra->base - rb->base;
>> +}
>> +
>> +static void __init __rmem_check_for_overlap(void)
>> +{
>> + int i;
>> +
>> + if (reserved_mem_count < 2)
>> + return;
>> +
>> + sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
>> + __rmem_cmp, NULL);
>> + for (i = 0; i < reserved_mem_count - 1; i++) {
>> + struct reserved_mem *this, *next;
>> +
>> + this = &reserved_mem[i];
>> + next = &reserved_mem[i + 1];
>> + if (!(this->base && next->base))
>> + continue;
>> + if (this->base + this->size > next->base) {
>> + phys_addr_t this_end, next_end;
>> +
>> + this_end = this->base + this->size;
>> + next_end = next->base + next->size;
>> + WARN(1,
>> + "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
>> + this->name, &this->base, &this_end,
>> + next->name, &next->base, &next_end);
>
> This is blowing up on some powerpc machines.
>
> It's too early in boot to call WARN() on these systems.
I didn't realize WARN could not be used early. Good to know.
> Can we turn it into a pr_err() for now?
Sounds fine.
> I'll send a patch?
Great.
Rob
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] of: Check for overlap in reserved memory regions
@ 2015-11-10 4:41 ` Rob Herring
0 siblings, 0 replies; 13+ messages in thread
From: Rob Herring @ 2015-11-10 4:41 UTC (permalink / raw)
To: Michael Ellerman
Cc: devicetree, Mitchel Humpherys, linuxppc-dev, linux-kernel,
Grant Likely, Frank Rowand, Marek Szyprowski
On Mon, Nov 9, 2015 at 10:29 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
>
>> Any overlap in the reserved memory regions (those specified in the
>> reserved-memory DT node) is a bug. These bugs might go undetected as
>> long as the contested region isn't used simultaneously by multiple
>> software agents, which makes such bugs hard to debug. Fix this by
>> printing a scary warning during boot if overlap is detected.
>>
>> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
>> index 726ebe792813..62f467b8ccae 100644
>> --- a/drivers/of/of_reserved_mem.c
>> +++ b/drivers/of/of_reserved_mem.c
>> @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
>> return -ENOENT;
>> }
>>
>> +static int __init __rmem_cmp(const void *a, const void *b)
>> +{
>> + const struct reserved_mem *ra = a, *rb = b;
>> +
>> + return ra->base - rb->base;
>> +}
>> +
>> +static void __init __rmem_check_for_overlap(void)
>> +{
>> + int i;
>> +
>> + if (reserved_mem_count < 2)
>> + return;
>> +
>> + sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
>> + __rmem_cmp, NULL);
>> + for (i = 0; i < reserved_mem_count - 1; i++) {
>> + struct reserved_mem *this, *next;
>> +
>> + this = &reserved_mem[i];
>> + next = &reserved_mem[i + 1];
>> + if (!(this->base && next->base))
>> + continue;
>> + if (this->base + this->size > next->base) {
>> + phys_addr_t this_end, next_end;
>> +
>> + this_end = this->base + this->size;
>> + next_end = next->base + next->size;
>> + WARN(1,
>> + "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
>> + this->name, &this->base, &this_end,
>> + next->name, &next->base, &next_end);
>
> This is blowing up on some powerpc machines.
>
> It's too early in boot to call WARN() on these systems.
I didn't realize WARN could not be used early. Good to know.
> Can we turn it into a pr_err() for now?
Sounds fine.
> I'll send a patch?
Great.
Rob
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] of: Check for overlap in reserved memory regions
2015-11-10 4:41 ` Rob Herring
@ 2015-11-10 4:57 ` Michael Ellerman
-1 siblings, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2015-11-10 4:57 UTC (permalink / raw)
To: Rob Herring
Cc: Mitchel Humpherys, Frank Rowand, Grant Likely, devicetree,
Marek Szyprowski, linux-kernel, linuxppc-dev
On Mon, 2015-11-09 at 22:41 -0600, Rob Herring wrote:
> On Mon, Nov 9, 2015 at 10:29 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> > On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
> > > Any overlap in the reserved memory regions (those specified in the
> > > reserved-memory DT node) is a bug. These bugs might go undetected as
> > > long as the contested region isn't used simultaneously by multiple
> > > software agents, which makes such bugs hard to debug. Fix this by
> > > printing a scary warning during boot if overlap is detected.
> > >
> > > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> > > index 726ebe792813..62f467b8ccae 100644
> > > --- a/drivers/of/of_reserved_mem.c
> > > +++ b/drivers/of/of_reserved_mem.c
> > > @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
> > > return -ENOENT;
> > > }
> > >
> > > +static int __init __rmem_cmp(const void *a, const void *b)
> > > +{
> > > + const struct reserved_mem *ra = a, *rb = b;
> > > +
> > > + return ra->base - rb->base;
> > > +}
> > > +
> > > +static void __init __rmem_check_for_overlap(void)
> > > +{
> > > + int i;
> > > +
> > > + if (reserved_mem_count < 2)
> > > + return;
> > > +
> > > + sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
> > > + __rmem_cmp, NULL);
> > > + for (i = 0; i < reserved_mem_count - 1; i++) {
> > > + struct reserved_mem *this, *next;
> > > +
> > > + this = &reserved_mem[i];
> > > + next = &reserved_mem[i + 1];
> > > + if (!(this->base && next->base))
> > > + continue;
> > > + if (this->base + this->size > next->base) {
> > > + phys_addr_t this_end, next_end;
> > > +
> > > + this_end = this->base + this->size;
> > > + next_end = next->base + next->size;
> > > + WARN(1,
> > > + "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
> > > + this->name, &this->base, &this_end,
> > > + next->name, &next->base, &next_end);
> >
> > This is blowing up on some powerpc machines.
> >
> > It's too early in boot to call WARN() on these systems.
>
> I didn't realize WARN could not be used early. Good to know.
Yeah, it's a bit horrible.
It used to be even worse, we'd take a recursive trap and you'd get nothing
useful at all. Ben fixed that, which makes BUG work but not WARN, because WARN
requires you to take the trap _and return_. We should be able to fix it in the
medium term, but not immediately.
> > Can we turn it into a pr_err() for now?
>
> Sounds fine.
> > I'll send a patch?
>
> Great.
Thanks. I'll just test the final version and then send.
cheers
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] of: Check for overlap in reserved memory regions
@ 2015-11-10 4:57 ` Michael Ellerman
0 siblings, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2015-11-10 4:57 UTC (permalink / raw)
To: Rob Herring
Cc: Mitchel Humpherys, Frank Rowand, Grant Likely, devicetree,
Marek Szyprowski, linux-kernel, linuxppc-dev
On Mon, 2015-11-09 at 22:41 -0600, Rob Herring wrote:
> On Mon, Nov 9, 2015 at 10:29 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> > On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
> > > Any overlap in the reserved memory regions (those specified in the
> > > reserved-memory DT node) is a bug. These bugs might go undetected as
> > > long as the contested region isn't used simultaneously by multiple
> > > software agents, which makes such bugs hard to debug. Fix this by
> > > printing a scary warning during boot if overlap is detected.
> > >
> > > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> > > index 726ebe792813..62f467b8ccae 100644
> > > --- a/drivers/of/of_reserved_mem.c
> > > +++ b/drivers/of/of_reserved_mem.c
> > > @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
> > > return -ENOENT;
> > > }
> > >
> > > +static int __init __rmem_cmp(const void *a, const void *b)
> > > +{
> > > + const struct reserved_mem *ra = a, *rb = b;
> > > +
> > > + return ra->base - rb->base;
> > > +}
> > > +
> > > +static void __init __rmem_check_for_overlap(void)
> > > +{
> > > + int i;
> > > +
> > > + if (reserved_mem_count < 2)
> > > + return;
> > > +
> > > + sort(reserved_mem, reserved_mem_count, sizeof(reserved_mem[0]),
> > > + __rmem_cmp, NULL);
> > > + for (i = 0; i < reserved_mem_count - 1; i++) {
> > > + struct reserved_mem *this, *next;
> > > +
> > > + this = &reserved_mem[i];
> > > + next = &reserved_mem[i + 1];
> > > + if (!(this->base && next->base))
> > > + continue;
> > > + if (this->base + this->size > next->base) {
> > > + phys_addr_t this_end, next_end;
> > > +
> > > + this_end = this->base + this->size;
> > > + next_end = next->base + next->size;
> > > + WARN(1,
> > > + "Reserved memory: OVERLAP DETECTED!\n%s (%pa--%pa) overlaps with %s (%pa--%pa)\n",
> > > + this->name, &this->base, &this_end,
> > > + next->name, &next->base, &next_end);
> >
> > This is blowing up on some powerpc machines.
> >
> > It's too early in boot to call WARN() on these systems.
>
> I didn't realize WARN could not be used early. Good to know.
Yeah, it's a bit horrible.
It used to be even worse, we'd take a recursive trap and you'd get nothing
useful at all. Ben fixed that, which makes BUG work but not WARN, because WARN
requires you to take the trap _and return_. We should be able to fix it in the
medium term, but not immediately.
> > Can we turn it into a pr_err() for now?
>
> Sounds fine.
> > I'll send a patch?
>
> Great.
Thanks. I'll just test the final version and then send.
cheers
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] of: Check for overlap in reserved memory regions
2015-09-16 1:30 ` Mitchel Humpherys
` (2 preceding siblings ...)
(?)
@ 2015-11-12 2:19 ` Michael Ellerman
2015-12-04 17:27 ` Mitchel Humpherys
-1 siblings, 1 reply; 13+ messages in thread
From: Michael Ellerman @ 2015-11-12 2:19 UTC (permalink / raw)
To: Mitchel Humpherys, Rob Herring, Frank Rowand, Grant Likely,
devicetree, Marek Szyprowski
Cc: linux-kernel
On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
> Any overlap in the reserved memory regions (those specified in the
> reserved-memory DT node) is a bug.
Can you expand a bit on why you think it's a bug? I assume it was discussed at
some point on the list but I didn't see it sorry.
There's nothing I can see in the binding document[1] about whether regions can
overlap, or what it would mean if they did.
If we want to declare that overlapping regions are always a bug then there
should be some text in the binding explaining that. There's also the
possibility that we have existing device trees in the wild that contain
overlapping regions, and whether we think it's OK to retrospectively declare
that they're incorrect.
cheers
[1]: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] of: Check for overlap in reserved memory regions
2015-11-12 2:19 ` Michael Ellerman
@ 2015-12-04 17:27 ` Mitchel Humpherys
0 siblings, 0 replies; 13+ messages in thread
From: Mitchel Humpherys @ 2015-12-04 17:27 UTC (permalink / raw)
To: Michael Ellerman
Cc: Rob Herring, Frank Rowand, Grant Likely, devicetree,
Marek Szyprowski, linux-kernel
On Thu, Nov 12 2015 at 01:19:59 PM, Michael Ellerman <mpe@ellerman.id.au> wrote:
> On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
>
>> Any overlap in the reserved memory regions (those specified in the
>> reserved-memory DT node) is a bug.
>
> Can you expand a bit on why you think it's a bug? I assume it was discussed at
> some point on the list but I didn't see it sorry.
The reason I think it's a bug is because the overlapping memory could be
handed out to multiple firmwares, which generally ends in "random"
firmware crashes. We've found by sad experience that root-causing such
a crash can be quite difficult.
Is there a valid use case for overlapping regions? I can't think of
one...
> There's nothing I can see in the binding document[1] about whether regions can
> overlap, or what it would mean if they did.
You're right, the bindings document doesn't say anything about
overlapping memory regions. I can submit something unless someone comes
up with a reason why we should allow overlapping memory regions.
> If we want to declare that overlapping regions are always a bug then there
> should be some text in the binding explaining that. There's also the
> possibility that we have existing device trees in the wild that contain
> overlapping regions, and whether we think it's OK to retrospectively declare
> that they're incorrect.
I did a quick survey of in-tree users of reserved-memory and couldn't
find any overlapping regions.
-Mitch
--
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] of: Check for overlap in reserved memory regions
@ 2015-11-12 8:06 ` Michael Ellerman
0 siblings, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2015-11-12 8:06 UTC (permalink / raw)
To: Mitchel Humpherys, Rob Herring, Frank Rowand, Grant Likely,
devicetree, Marek Szyprowski
Cc: linux-kernel
On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
> Any overlap in the reserved memory regions (those specified in the
> reserved-memory DT node) is a bug. These bugs might go undetected as
> long as the contested region isn't used simultaneously by multiple
> software agents, which makes such bugs hard to debug. Fix this by
> printing a scary warning during boot if overlap is detected.
>
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 726ebe792813..62f467b8ccae 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
> return -ENOENT;
> }
>
> +static int __init __rmem_cmp(const void *a, const void *b)
> +{
> + const struct reserved_mem *ra = a, *rb = b;
> +
> + return ra->base - rb->base;
> +}
So on further inspection it seems this is really our problem.
The reserved_mem->base values are u64, so when the unsigned result of the
substraction is converted to int we may or may not get the right value. That
means the sort fails and the reserved mem check gives false positives.
eg:
Reserved memory: OVERLAP DETECTED!
ibm,slw-image@1ffe600000 (0x0000001ffe600000--0x0000001ffe700000) overlaps with ibm,firmware-allocs-memory@1000000000 (0x0000001000000000--0x0000001000dc0200)
Note there is no overlap:
0x0000001ffe600000--0x0000001ffe700000
vs
0x0000001000000000--0x0000001000dc0200
cheers
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2] of: Check for overlap in reserved memory regions
@ 2015-11-12 8:06 ` Michael Ellerman
0 siblings, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2015-11-12 8:06 UTC (permalink / raw)
To: Mitchel Humpherys, Rob Herring, Frank Rowand, Grant Likely,
devicetree-u79uwXL29TY76Z2rM5mHXA, Marek Szyprowski
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA
On Tue, 2015-09-15 at 18:30 -0700, Mitchel Humpherys wrote:
> Any overlap in the reserved memory regions (those specified in the
> reserved-memory DT node) is a bug. These bugs might go undetected as
> long as the contested region isn't used simultaneously by multiple
> software agents, which makes such bugs hard to debug. Fix this by
> printing a scary warning during boot if overlap is detected.
>
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 726ebe792813..62f467b8ccae 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -197,12 +198,52 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
> return -ENOENT;
> }
>
> +static int __init __rmem_cmp(const void *a, const void *b)
> +{
> + const struct reserved_mem *ra = a, *rb = b;
> +
> + return ra->base - rb->base;
> +}
So on further inspection it seems this is really our problem.
The reserved_mem->base values are u64, so when the unsigned result of the
substraction is converted to int we may or may not get the right value. That
means the sort fails and the reserved mem check gives false positives.
eg:
Reserved memory: OVERLAP DETECTED!
ibm,slw-image@1ffe600000 (0x0000001ffe600000--0x0000001ffe700000) overlaps with ibm,firmware-allocs-memory@1000000000 (0x0000001000000000--0x0000001000dc0200)
Note there is no overlap:
0x0000001ffe600000--0x0000001ffe700000
vs
0x0000001000000000--0x0000001000dc0200
cheers
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread