Re: [PATCH] checks: Warn about overlapping reserved-memory ranges

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 10232 bytes --]

On Wed, Sep 23, 2026 at 05:50:18PM +0000, Bjorn Andersson wrote:
> Static /reserved-memory entries associate physical address ranges with
> particular purposes and properties. While not explicitly prohibited,
> overlapping ranges commonly indicate an incorrect address or size and
> may assign the same memory conflicting attributes. Dtc currently
> provides no diagnostic for this type of mistake.
> 
> Check every non-empty reg range under available reserved-memory children
> and report overlaps. Add tests covering overlapping and adjacent ranges,
> as well as unavailable, zero-sized, and dynamically allocated regions.

Hm.  It occurs to me that overlapping reg values are generally bad on
*any* bus, not just /reserved-memory.  It might be worth generalising
that part of the test.

> Assisted-by: pi:gpt-5.6-sol
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
>  checks.c                             | 131 +++++++++++++++++++++++++++++++++++
>  tests/reserved-memory-no-overlap.dts |  43 ++++++++++++
>  tests/reserved-memory-overlap.dts    |  21 ++++++
>  tests/run_tests.sh                   |   2 +
>  4 files changed, 197 insertions(+)
> 
> diff --git a/checks.c b/checks.c
> index a6037ed08000..baa0914e37a5 100644
> --- a/checks.c
> +++ b/checks.c
> @@ -1266,6 +1266,136 @@ static bool node_is_disabled(struct node *node)
>  	return false;
>  }
>  
> +static bool node_is_available(struct node *node)

Do we really need a new function, rather than just
!node_is_disabled()?

> +{
> +	struct property *prop;
> +	const char *status;
> +
> +	prop = get_property(node, "status");
> +	if (!prop)
> +		return true;
> +
> +	if (!data_is_one_string(prop->val))
> +		return false;
> +
> +	status = prop->val.val;
> +	return streq(status, "okay") || streq(status, "ok");

I'm pretty sure only "okay" is acceptable according to IEEE1275 - I'm
not sure if the flat tree specs expanded on that.

Which does suggest that a check for valid values of the 'status'
property (not just that it's a string) might be a good idea.

> +}
> +
> +static bool get_reg_value(struct property *reg, unsigned int cell,
> +			  int cell_count, uint64_t *value)
> +{
> +	uint64_t val = 0;
> +	int i;
> +
> +	if (cell_count < 0 || cell_count > 2)

cell_count < 0 is nonsensical, it should be an unsigned parameter.

> +		return false;
> +
> +	for (i = 0; i < cell_count; i++)
> +		val = (val << 32) | propval_cell_n(reg, cell + i);
> +
> +	*value = val;
> +	return true;

I don't love the fact that this is limited to cases with either 1 or 2
cells.  I'd kind of prefer a general 'reg' overlap checker, although I
recognize that's significantly more complex.


> +}
> +
> +static bool ranges_overlap(uint64_t addr_a, uint64_t size_a,
> +			   uint64_t addr_b, uint64_t size_b)
> +{
> +	if (!size_a || !size_b)
> +		return false;

I'm not sure if this makes sense.  If there's a zero-length range in
the middle of another range it's not technically an overlap, but it
still seems a dubious looking situation that we should probably warn
about.

> +	if (addr_a <= addr_b)
> +		return addr_b - addr_a < size_a;
> +
> +	return addr_a - addr_b < size_b;
> +}
> +
> +static void format_range_end(char end_str[20], uint64_t addr, uint64_t size)

Arbitrary open-coded string lengths are not ok.

> +{
> +	uint64_t end = addr + size;
> +
> +	if (end < addr)
> +		snprintf(end_str, 20, "0x1%016" PRIx64, end);
> +	else
> +		snprintf(end_str, 20, "0x%" PRIx64, end);

This seems a very convoluted way of going about things.  You could
instead express memory ranges in inclusive format
(e.g. 0x1000..0x1fff), to avoid the overflow occuring in the first
place.

> +}
> +
> +static void check_reserved_memory_overlap(struct check *c,
> +					  struct dt_info *dti,
> +					  struct node *node)
> +{
> +	struct reserve_range {
> +		struct node *node;
> +		uint64_t addr;
> +		uint64_t size;
> +	} *ranges = NULL;
> +	struct node *child;
> +	unsigned int range_count = 0;
> +	int addr_cells, size_cells, entry_cells;

These should be unsigned.

> +
> +	if (!streq(node->fullpath, "/reserved-memory"))
> +		return;
> +
> +	addr_cells = node_addr_cells(node);
> +	size_cells = node_size_cells(node);
> +	entry_cells = addr_cells + size_cells;

You try to check for overflow below, but *signed* integer overflow is
UB the moment it happens, it's too late to check it below.  But this
should be unsigned anyway.

> +
> +	/* Physical addresses and sizes wider than 64 bits cannot be compared. */
> +	if (addr_cells > 2 || size_cells > 2 || entry_cells <= 0)

Hmm.  If addr_cells + size_cells overflows we have much bigger
problems than this test.  Maybe we should expand sanity checks for
#address-cells and #size-cells and make that a prerequisite for this
check.

> +		return;
> +
> +	for_each_child(node, child) {
> +		struct property *reg;
> +		unsigned int cell;
> +
> +		if (!node_is_available(child))
> +			continue;

Is the 'status' property used in /reserved-memory?  That seems... weird.

> +
> +		reg = get_property(child, "reg");
> +		if (!reg)
> +			continue;
> +
> +		for (cell = 0; cell < reg->val.len / sizeof(cell_t);
> +		     cell += entry_cells) {
> +			uint64_t addr, size;
> +			unsigned int i;
> +
> +			if (!get_reg_value(reg, cell, addr_cells, &addr) ||
> +			    !get_reg_value(reg, cell + addr_cells,
> +					   size_cells, &size))
> +				continue;
> +
> +			for (i = 0; i < range_count; i++) {
> +				char end[20], other_end[20];
> +
> +				if (!ranges_overlap(addr, size, ranges[i].addr,
> +						    ranges[i].size))
> +					continue;
> +
> +				format_range_end(end, addr, size);
> +				format_range_end(other_end, ranges[i].addr,
> +						 ranges[i].size);
> +				FAIL(c, dti, child,
> +				     "reg range [0x%" PRIx64 ", %s) overlaps with %s "
> +				     "[0x%" PRIx64 ", %s)",
> +				     addr, end, ranges[i].node->fullpath,
> +				     ranges[i].addr, other_end);
> +			}
> +
> +			ranges = xrealloc(ranges,
> +					  (range_count + 1) * sizeof(*ranges));
> +			ranges[range_count].node = child;
> +			ranges[range_count].addr = addr;
> +			ranges[range_count].size = size;
> +			range_count++;
> +		}
> +	}
> +
> +	free(ranges);
> +}
> +WARNING(reserved_memory_overlap, check_reserved_memory_overlap, NULL,
> +	&addr_size_cells, &reg_format);
> +
>  static void check_unique_unit_address_common(struct check *c,
>  						struct dt_info *dti,
>  						struct node *node,
> @@ -1945,6 +2075,7 @@ static struct check *check_table[] = {
>  
>  	&avoid_default_addr_size,
>  	&avoid_unnecessary_addr_size,
> +	&reserved_memory_overlap,
>  	&unique_unit_address,
>  	&unique_unit_address_if_enabled,
>  	&obsolete_chosen_interrupt_controller,
> diff --git a/tests/reserved-memory-no-overlap.dts b/tests/reserved-memory-no-overlap.dts
> new file mode 100644
> index 000000000000..0293986ef746
> --- /dev/null
> +++ b/tests/reserved-memory-no-overlap.dts
> @@ -0,0 +1,43 @@
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +

Hrm.  Is 'ranges' really supposed to be there according to the
/reserved-memory binding?  That indicates this is a bridge with
identity mapping, which doesn't make a lot of sense for an
informational node like this.

Also, is there no 'compatible' that should be resent for /reserved-memory?

> +		first@100000000 {
> +			reg = <0x1 0x00000000 0x0 0x00100000>;
> +		};
> +
> +		adjacent@100100000 {
> +			reg = <0x1 0x00100000 0x0 0x00100000>;
> +		};
> +
> +		disabled@100020000 {
> +			reg = <0x1 0x00020000 0x0 0x00100000>;
> +			status = "disabled";
> +		};
> +
> +		reserved@100040000 {
> +			reg = <0x1 0x00040000 0x0 0x00100000>;
> +			status = "reserved";

"reserved" is not a valid value for 'status' according to IEEE 1275
(p188).

> +		};
> +
> +		failed@100060000 {
> +			reg = <0x1 0x00060000 0x0 0x00100000>;
> +			status = "fail";
> +		};
> +
> +		zero-size@100080000 {
> +			reg = <0x1 0x00080000 0x0 0x0>;
> +		};
> +
> +		dynamic {
> +			size = <0x0 0x00100000>;
> +		};
> +	};
> +};
> diff --git a/tests/reserved-memory-overlap.dts b/tests/reserved-memory-overlap.dts
> new file mode 100644
> index 000000000000..fec7c3a33b1f
> --- /dev/null
> +++ b/tests/reserved-memory-overlap.dts
> @@ -0,0 +1,21 @@
> +/dts-v1/;
> +
> +/ {
> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	reserved-memory {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		first@100000000 {
> +			reg = <0x1 0x00000000 0x0 0x00100000>,
> +			      <0x2 0x00000000 0x0 0x00200000>;
> +		};
> +
> +		second@200100000 {
> +			reg = <0x2 0x00100000 0x0 0x00100000>;
> +		};
> +	};
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 842b543059bb..27c3ebfed338 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -746,6 +746,8 @@ dtc_tests () {
>      check_tests "$SRCDIR/reg-ranges-root.dts" reg_format ranges_format
>      check_tests "$SRCDIR/bad-dma-ranges.dts" dma_ranges_format
>      check_tests "$SRCDIR/default-addr-size.dts" avoid_default_addr_size
> +    check_tests "$SRCDIR/reserved-memory-overlap.dts" reserved_memory_overlap
> +    check_tests "$SRCDIR/reserved-memory-no-overlap.dts" -n reserved_memory_overlap
>      check_tests "$SRCDIR/obsolete-chosen-interrupt-controller.dts" obsolete_chosen_interrupt_controller
>      check_tests "$SRCDIR/reg-without-unit-addr.dts" unit_address_vs_reg
>      check_tests "$SRCDIR/unit-addr-without-reg.dts" unit_address_vs_reg
> 
> ---
> base-commit: 7a1e017926004ecff5fce62d62d42ce9f3e00082
> change-id: 20260923-check-reserved-overlap-6c5fc2214ea4
> 
> Best regards,
> --  
> Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> 
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]