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

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

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

On Fri, Sep 25, 2026 at 07:03:00PM +0200, Herve Codina wrote:
> Hi Bjorn,
> 
> 
> On Wed, 23 Sep 2026 17:50:18 +0000
> Bjorn Andersson <bjorn.andersson@oss.qualcomm.com> 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.
> 
> I am not so sure that this has to checked by dtc.

It doesn't have to be, but that doesn't mean it isn't useful to do so.

> Should dtc have the knowledge of /reserved-memory nodes and
> sub-nodes meaning?

For the core logic, no.  But checks.c is explicitly about making
semantic checks of the tree's contents.  This is at the very least a
reasonable candidate for a check.

> Overlaps on regs and size can appear in many places in the devicetree which
> are not correct from a functional point of view. Should dtc check this kind
> of issue? I am not so sure.

Again, it doesn't have to, but it could still be useful.  What we
check in checks.c is not that extensive, but that's because of policy,
it's just because no-one's implemented it.

[snip]
> > +	for_each_child(node, child) {
> > +		struct property *reg;
> > +		unsigned int cell;
> > +
> > +		if (!node_is_available(child))
> > +			continue;
> 
> Could a "status" property be present in a /reserved-memory subnode?

That's a good question, it certainly seems an odd thing to have in
something that's not an actual device, but I'm not familiar with the
/reserved-memory binding off the top of my head.

> > +
> > +		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);
> 
> Why do you need to convert to a string ?
> 
> 
> > +				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);
> 
> Weird to have '[' and ')'.

[x..y) is a standard notation in maths (it means x is included in the
range, but y is not).  But that doesn't mean it's generally known.


> Also, IHMO, end and other_end don't bring any benefit in the message.
> 
> I think the following message is enough:
> --- 8< ---
>   "reg 0x%PRIx64 size 0x%PRIx64 overlaps with reg 0x%PRIx64 size 0x%PRIx64 from %s",
>      addr, size, ranges[i].addr, ranges[i].size, ranges[i].node->fullpath
> --- 8< ---

This looks better to me.

> or in a more compact way:
>  "reg (0x%PRIx64, 0x%PRIx64) overlaps with reg (0x%PRIx64, 0x%PRIx64) from %s",

I don't really like this format, though.

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