linux-kernel - Re: [PATCH v2 1/7] of/fdt: Consolidate duplicate code into helper functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251113223859.GB800052-robh@kernel.org>
Date: Thu, 13 Nov 2025 16:38:59 -0600
From: Rob Herring <robh@...nel.org>
To: Yuntao Wang <yuntao.wang@...ux.dev>
Cc: Saravana Kannan <saravanak@...gle.com>,
	Geert Uytterhoeven <geert+renesas@...der.be>,
	Catalin Marinas <catalin.marinas@....com>,
	James Morse <james.morse@....com>, Baoquan He <bhe@...hat.com>,
	Zhen Lei <thunder.leizhen@...wei.com>,
	Ard Biesheuvel <ardb@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Geoff Levand <geoff@...radead.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Changyuan Lyu <changyuanl@...gle.com>,
	Alexander Graf <graf@...zon.com>,
	"Mike Rapoport (Microsoft)" <rppt@...nel.org>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/7] of/fdt: Consolidate duplicate code into helper
 functions

On Thu, Nov 13, 2025 at 11:50:58PM +0800, Yuntao Wang wrote:
> Currently, there are many pieces of nearly identical code scattered across
> different places. Consolidate the duplicate code into helper functions to
> improve maintainability and reduce the likelihood of errors.
> 
> Signed-off-by: Yuntao Wang <yuntao.wang@...ux.dev>
> ---
>  drivers/of/fdt.c       | 41 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/of_fdt.h |  5 +++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 0edd639898a6..5e0eabc1449f 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -625,6 +625,47 @@ const void *__init of_get_flat_dt_prop(unsigned long node, const char *name,
>  	return fdt_getprop(initial_boot_params, node, name, size);
>  }
>  
> +const __be32 *__init of_fdt_get_addr_size_prop(unsigned long node,
> +                                               const char *name, int *entries)
> +{
> +	const __be32 *prop;
> +	int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
> +
> +	prop = of_get_flat_dt_prop(node, name, &len);
> +	if (!prop) {
> +		*entries = 0;
> +		return NULL;
> +	}
> +
> +	if (len % elen) {
> +		*entries = -1;

I don't think it's really important to distinguish a length error from 
any other error. Either we can read the property or we can't.

> +		return NULL;
> +	}
> +
> +	*entries = len / elen;
> +	return prop;
> +}
> +
> +bool __init of_fdt_get_addr_size(unsigned long node, const char *name,
> +                                 u64 *addr, u64 *size)
> +{
> +	const __be32 *prop;
> +	int len, elen = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);

Still have 2 locations to get the same calculation wrong...

> +
> +	prop = of_get_flat_dt_prop(node, name, &len);
> +	if (!prop || len < elen)
> +		return false;

Why doesn't calling of_fdt_get_addr_size_prop() work here? If 'len < 
elen', then 'len % elen' will also be true except in the 0 length case. 
For that case, of_fdt_get_addr_size_prop() needs to handle it too.

> +
> +	of_fdt_read_addr_size(prop, addr, size);
> +	return true;
> +}
> +
> +void __init of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size)
> +{
> +	*addr = dt_mem_next_cell(dt_root_addr_cells, &prop);
> +	*size = dt_mem_next_cell(dt_root_size_cells, &prop);
> +}
> +
>  /**
>   * of_fdt_is_compatible - Return true if given node from the given blob has
>   * compat in its compatible list
> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> index b8d6c0c20876..3a0805ff6c7b 100644
> --- a/include/linux/of_fdt.h
> +++ b/include/linux/of_fdt.h
> @@ -55,6 +55,11 @@ extern int of_get_flat_dt_subnode_by_name(unsigned long node,
>  					  const char *uname);
>  extern const void *of_get_flat_dt_prop(unsigned long node, const char *name,
>  				       int *size);
> +extern const __be32 *of_fdt_get_addr_size_prop(unsigned long node,
> +                                               const char *name, int *entries);
> +extern bool of_fdt_get_addr_size(unsigned long node, const char *name,
> +                                 u64 *addr, u64 *size);
> +extern void of_fdt_read_addr_size(const __be32 *prop, u64 *addr, u64 *size);
>  extern int of_flat_dt_is_compatible(unsigned long node, const char *name);

Looks like of_flat_dt_* would be more consistent with existing naming.

>  extern unsigned long of_get_flat_dt_root(void);
>  extern uint32_t of_get_flat_dt_phandle(unsigned long node);
> -- 
> 2.51.0
>