| Message ID | 20201020073558.3582-1-vincent.whitchurch@axis.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | of: Fix reserved-memory overlap detection | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 9 lines checked |
On Tue, Oct 20, 2020 at 2:36 AM Vincent Whitchurch <vincent.whitchurch@axis.com> wrote: > > The reserved-memory overlap detection code fails to detect overlaps if > either of the regions starts at address 0x0. For some reason the code > explicitly checks for and ignores such regions, but this check looks > invalid. Remove the check and fix this detection. Wouldn't 'base' be 0 for nodes that have a 'size' and no address? The base in those cases isn't set until later when __reserved_mem_alloc_size() is called. > > For example, no overlap is currently reported for this case: > > foo@0 { > reg = <0x0000 0x2000>; > }; > > bar@1000 { > reg = <0x1000 0x1000>; > }; > > but it is after this patch: > > OF: reserved mem: OVERLAP DETECTED! > foo@0 (0x00000000--0x00002000) overlaps with bar@1000 (0x00001000--0x00002000) > > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> > --- > drivers/of/of_reserved_mem.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c > index 46b9371c8a33..1c5259e3e81f 100644 > --- a/drivers/of/of_reserved_mem.c > +++ b/drivers/of/of_reserved_mem.c > @@ -217,8 +217,7 @@ static void __init __rmem_check_for_overlap(void) > > 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; > > -- > 2.28.0 >
On Tue, Oct 20, 2020 at 03:00:14PM +0200, Rob Herring wrote: > On Tue, Oct 20, 2020 at 2:36 AM Vincent Whitchurch > <vincent.whitchurch@axis.com> wrote: > > > > The reserved-memory overlap detection code fails to detect overlaps if > > either of the regions starts at address 0x0. For some reason the code > > explicitly checks for and ignores such regions, but this check looks > > invalid. Remove the check and fix this detection. > > Wouldn't 'base' be 0 for nodes that have a 'size' and no address? The > base in those cases isn't set until later when > __reserved_mem_alloc_size() is called. Ah, yes, I guess that's why the check was there. I see that those entries have both a zero address and a zero size, so this seems to work: diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts index 623246f37448..6627e71c7283 100644 --- a/arch/arm/boot/dts/vexpress-v2p-ca9.dts +++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts @@ -81,6 +81,18 @@ vram: vram@4c000000 { reg = <0x4c000000 0x00800000>; no-map; }; + + foo@0 { + reg = <0x0 0x2000>; + }; + + bar@1000 { + reg = <0x0 0x1000>; + }; + + baz { + size = <0x1000>; + }; }; clcd@10020000 { diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c index 46b9371c8a33..fea9433d942a 100644 --- a/drivers/of/of_reserved_mem.c +++ b/drivers/of/of_reserved_mem.c @@ -200,6 +200,16 @@ static int __init __rmem_cmp(const void *a, const void *b) if (ra->base > rb->base) return 1; + /* + * Put the dynamic allocations (address == 0, size == 0) before static + * allocations at address 0x0 so that overlap detection works + * correctly. + */ + if (ra->size < rb->size) + return -1; + if (ra->size > rb->size) + return 1; + return 0; } @@ -212,13 +222,19 @@ static void __init __rmem_check_for_overlap(void) 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 = &reserved_mem[i]; + + pr_info("i %d base %x size %x\n", i, this->base, this->size); + } + 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;
On Tue, Oct 20, 2020 at 8:46 AM Vincent Whitchurch <vincent.whitchurch@axis.com> wrote: > > On Tue, Oct 20, 2020 at 03:00:14PM +0200, Rob Herring wrote: > > On Tue, Oct 20, 2020 at 2:36 AM Vincent Whitchurch > > <vincent.whitchurch@axis.com> wrote: > > > > > > The reserved-memory overlap detection code fails to detect overlaps if > > > either of the regions starts at address 0x0. For some reason the code > > > explicitly checks for and ignores such regions, but this check looks > > > invalid. Remove the check and fix this detection. > > > > Wouldn't 'base' be 0 for nodes that have a 'size' and no address? The > > base in those cases isn't set until later when > > __reserved_mem_alloc_size() is called. > > Ah, yes, I guess that's why the check was there. I see that those > entries have both a zero address and a zero size, so this seems to work: Yes, I think it should work. > > diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts > index 623246f37448..6627e71c7283 100644 > --- a/arch/arm/boot/dts/vexpress-v2p-ca9.dts > +++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts > @@ -81,6 +81,18 @@ vram: vram@4c000000 { > reg = <0x4c000000 0x00800000>; > no-map; > }; > + > + foo@0 { > + reg = <0x0 0x2000>; > + }; > + > + bar@1000 { > + reg = <0x0 0x1000>; 0x1000 base? > + }; > + > + baz { > + size = <0x1000>; > + }; > }; > > clcd@10020000 { > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c > index 46b9371c8a33..fea9433d942a 100644 > --- a/drivers/of/of_reserved_mem.c > +++ b/drivers/of/of_reserved_mem.c > @@ -200,6 +200,16 @@ static int __init __rmem_cmp(const void *a, const void *b) > if (ra->base > rb->base) > return 1; > > + /* > + * Put the dynamic allocations (address == 0, size == 0) before static > + * allocations at address 0x0 so that overlap detection works > + * correctly. > + */ > + if (ra->size < rb->size) > + return -1; > + if (ra->size > rb->size) > + return 1; > + > return 0; > } > > @@ -212,13 +222,19 @@ static void __init __rmem_check_for_overlap(void) > > 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 = &reserved_mem[i]; > + > + pr_info("i %d base %x size %x\n", i, this->base, this->size); > + } > + > 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; >
On Tue, Oct 20, 2020 at 04:17:27PM +0200, Rob Herring wrote: > On Tue, Oct 20, 2020 at 8:46 AM Vincent Whitchurch > <vincent.whitchurch@axis.com> wrote: > > On Tue, Oct 20, 2020 at 03:00:14PM +0200, Rob Herring wrote: > > > On Tue, Oct 20, 2020 at 2:36 AM Vincent Whitchurch > > > <vincent.whitchurch@axis.com> wrote: > > > > > > > > The reserved-memory overlap detection code fails to detect overlaps if > > > > either of the regions starts at address 0x0. For some reason the code > > > > explicitly checks for and ignores such regions, but this check looks > > > > invalid. Remove the check and fix this detection. > > > > > > Wouldn't 'base' be 0 for nodes that have a 'size' and no address? The > > > base in those cases isn't set until later when > > > __reserved_mem_alloc_size() is called. > > > > Ah, yes, I guess that's why the check was there. I see that those > > entries have both a zero address and a zero size, so this seems to work: > > Yes, I think it should work. Thanks, I've tested it a bit more and sent it out as a v2 now. > > diff --git a/arch/arm/boot/dts/vexpress-v2p-ca9.dts b/arch/arm/boot/dts/vexpress-v2p-ca9.dts > > index 623246f37448..6627e71c7283 100644 > > --- a/arch/arm/boot/dts/vexpress-v2p-ca9.dts > > +++ b/arch/arm/boot/dts/vexpress-v2p-ca9.dts > > @@ -81,6 +81,18 @@ vram: vram@4c000000 { > > reg = <0x4c000000 0x00800000>; > > no-map; > > }; > > + > > + foo@0 { > > + reg = <0x0 0x2000>; > > + }; > > + > > + bar@1000 { > > + reg = <0x0 0x1000>; > > 0x1000 base? I've corrected this in the example in the commit message for v2.
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c index 46b9371c8a33..1c5259e3e81f 100644 --- a/drivers/of/of_reserved_mem.c +++ b/drivers/of/of_reserved_mem.c @@ -217,8 +217,7 @@ static void __init __rmem_check_for_overlap(void) 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;
The reserved-memory overlap detection code fails to detect overlaps if either of the regions starts at address 0x0. For some reason the code explicitly checks for and ignores such regions, but this check looks invalid. Remove the check and fix this detection. For example, no overlap is currently reported for this case: foo@0 { reg = <0x0000 0x2000>; }; bar@1000 { reg = <0x1000 0x1000>; }; but it is after this patch: OF: reserved mem: OVERLAP DETECTED! foo@0 (0x00000000--0x00002000) overlaps with bar@1000 (0x00001000--0x00002000) Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com> --- drivers/of/of_reserved_mem.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)