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

[Yuntao Wang <yuntao.wang@linux.dev>]

On Wed, 12 Nov 2025 14:55:51 -0600, Rob Herring <robh@kernel.org> wrote:

> 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

Hi Rob,

The link to the new patch series:

https://lore.kernel.org/linux-devicetree/20251113155104.226617-1-yuntao.wang@linux.dev/t/

Thanks,
Yuntao