Re: [PATCH 00/10] of/fdt: Some bug fixes and cleanups

[Rob Herring <robh@kernel.org>]

On Wed, Nov 12, 2025 at 10:35:10PM +0800, Yuntao Wang wrote:
> This patch series fixes several bugs related to dt_root_addr_cells and
> dt_root_size_cells, and performs some cleanup.
> 
> Links to the previous related patches:
> 
> https://lore.kernel.org/lkml/CAL_JsqJxar7z+VcBXwPTw5-Et2oC9bQmH_CtMtKhoo_-=zN2XQ@mail.gmail.com/
> 
> Yuntao Wang (10):
>   of/fdt: Introduce dt_root_addr_size_cells() and
>     dt_root_addr_size_bytes()
>   of/reserved_mem: Use dt_root_addr_size_bytes() instead of open-coding
>     it
>   of/reserved_mem: Use dt_root_addr_size_bytes() instead of open-coding
>     it
>   of/reserved_mem: Use dt_root_addr_size_bytes() instead of open-coding
>     it

Your aim in writing subjects should be to write something that is unique 
for every commit in the past or future. Because you can never make the 
same change twice, right? (I'm excluding 'fix typos/spelling' type 
commits). Certainly the same subject in one series is never right.

>   of/fdt: Use dt_root_addr_size_bytes() instead of open-coding it
>   of/fdt: Fix the len check in early_init_dt_check_for_elfcorehdr()
>   of/fdt: Fix the len check in
>     early_init_dt_check_for_usable_mem_range()
>   of/fdt: Use dt_root_addr_size_bytes() instead of open-coding it

This is not what I meant. We have multiple copies of this where only 
the property name changes: 

	prop = of_get_flat_dt_prop(node, "linux,elfcorehdr", &len);
	if (!prop || (len < (dt_root_addr_cells + dt_root_size_cells)))
		return;

	elfcorehdr_addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
	elfcorehdr_size = dt_mem_next_cell(dt_root_size_cells, &prop);

Instead, add a function something like this:

static void early_init_dt_read_address(unsigned long node, const char 
*prop, u64 *addr, u64*size)
{
        prop = of_get_flat_dt_prop(node, prop, &len);
        if (!prop || (len < (dt_root_addr_cells + dt_root_size_cells)))
                return;

        *addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
        *size = dt_mem_next_cell(dt_root_size_cells, &prop);
}

Then we only have the length checks in one place.


That still leaves the cases with more than 1 entry open coded. So 
instead, to cover that case to something like this:

const __be32 *of_get_flat_dt_address_prop(unsigned long node, const char 
*propname, int *len)
{
	prop = of_get_flat_dt_prop(node, propname, &len);
	if (!prop || (*len % (dt_root_addr_cells + dt_root_size_cells))) {
		*len = 0;
		return NULL;
	}

	*len /= (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
	return prop;
}

And then a user would look something like this:

prop = of_get_flat_dt_address(node, "linux,usable-memory-range", &len);
for (i = 0; i < len; i++) {
	of_read_address_idx(prop, i, &addr, &size);
	...
}

Here 'len' is number of addr+size entries.

And the simple case of reading 1 entry could be just:

of_read_address_idx(of_get_flat_dt_address(node, "linux,elfcorehdr", NULL), 0, &addr, &size);

Rob