On Fri, Feb 16, 2024 at 10:06:37PM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Feb 16, 2024 at 07:19:20PM +0100, Uwe Kleine-König wrote:
> > A phandle in an overlay is not supposed to overwrite a phandle that
> > already exists in the base dtb as this breaks references to the
> > respective node in the base.
> > 
> > So add another iteration over the fdto that checks for such overwrites
> > and fixes the fdto phandle's value to match the fdt's.
> > 
> > A test is added that checks that newly added phandles and existing
> > phandles work as expected.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> After sending the patch out I had a nice idea for an optimisation here.
> It doesn't make the process faster, but improves the result.
> 
> If phandle with the (shifted) value 17 is changed to 5, there is no
> phandle with the value 17 in the end. So while iterating over the fdto
> to replace all phandles with value 17 by 5 all values > 17 can be
> reduced by one. This way the phandle allocation in the resulting dtb is
> continuous if both inputs have continuous numbers.

Honestly, I don't think the marginal advantage of improving the
chances of contiguous phandles is worth the extra complexity.

> 
> This can be implemented by the patch below on top of the patch I'm
> replying to.
> 
> Best regards
> Uwe
> 
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index 914acc5b14a6..bfdba50dee50 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -529,15 +529,25 @@ static int overlay_adjust_node_conflicting_phandle(void *fdto, int node,
>  	int child;
>  
>  	php = fdt_getprop(fdto, node, "phandle", &len);
> -	if (php && len == sizeof(*php) && fdt32_to_cpu(*php) == fdto_phandle) {
> -		ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt_phandle);
> +	if (php && len == sizeof(*php)) {
> +		if (fdt32_to_cpu(*php) == fdto_phandle)
> +			ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt_phandle);
> +		else if (fdt32_to_cpu(*php) > fdto_phandle)
> +			ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt32_to_cpu(*php) - 1);
> +		else
> +			ret = 0;
>  		if (ret)
>  			return ret;
>  	}
>  
>  	php = fdt_getprop(fdto, node, "linux,phandle", &len);
> -	if (php && len == sizeof(*php) && fdt32_to_cpu(*php) == fdto_phandle) {
> -		ret = fdt_setprop_inplace_u32(fdto, node, "linux,phandle", fdt_phandle);
> +	if (php && len == sizeof(*php)) {
> +		if (fdt32_to_cpu(*php) == fdto_phandle)
> +			ret = fdt_setprop_inplace_u32(fdto, node, "linux,phandle", fdt_phandle);
> +		else if (fdt32_to_cpu(*php) > fdto_phandle)
> +			ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt32_to_cpu(*php) - 1);
> +		else
> +			ret = 0;
>  		if (ret)
>  			return ret;
>  	}
> @@ -609,23 +619,27 @@ static int overlay_update_node_conflicting_references(void *fdto, int tree_node,
>  			 */
>  			memcpy(&adj_val, tree_val + poffset, sizeof(adj_val));
>  
> -			if (fdt32_to_cpu(adj_val) == fdto_phandle) {
> +			if (fdt32_to_cpu(adj_val) < fdto_phandle)
> +				continue;
>  
> +			if (fdt32_to_cpu(adj_val) == fdto_phandle)
>  				adj_val = cpu_to_fdt32(fdt_phandle);
> +			else
> +				adj_val = cpu_to_fdt32(fdt32_to_cpu(adj_val) - 1);
>  
> -				ret = fdt_setprop_inplace_namelen_partial(fdto,
> -									  tree_node,
> -									  name,
> -									  strlen(name),
> -									  poffset,
> -									  &adj_val,
> -									  sizeof(adj_val));
> -				if (ret == -FDT_ERR_NOSPACE)
> -					return -FDT_ERR_BADOVERLAY;
>  
> -				if (ret)
> -					return ret;
> -			}
> +			ret = fdt_setprop_inplace_namelen_partial(fdto,
> +								  tree_node,
> +								  name,
> +								  strlen(name),
> +								  poffset,
> +								  &adj_val,
> +								  sizeof(adj_val));
> +			if (ret == -FDT_ERR_NOSPACE)
> +				return -FDT_ERR_BADOVERLAY;
> +
> +			if (ret)
> +				return ret;
>  		}
>  	}
>  



-- 
David Gibson			| 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