Re: [PATCH] libfdt: overlay: Add fdt_ prefix to overlay_get_target()

[Vikram Garhwal <fnu.vikram-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>]

On Thu, Nov 18, 2021 at 10:44:00AM +1100, David Gibson wrote:
> On Mon, Nov 15, 2021 at 03:30:43PM -0800, Vikram Garhwal wrote:
> > Add fdt_ prefix to overlay_get_target() and remove static type.
> 
> The short description is burying the lede a bit, the key point is that
> you're making this function public.
> 
> > This is done to get the target path for the overlay nodes which is very useful
> > in many cases. For example, Xen hypervisor needs it when applying overlays
> > because Xen needs to do further processing of the overlay nodes, e.g. mapping of
> > resources(IRQs and IOMMUs) to other VMs, creation of SMMU pagetables, etc.
> > 
> > Signed-off-by: Vikram Garhwal <fnu.vikram-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> 
> The concept looks fine to me.  A few details to polish noted below:
> 
> > ---
> >  libfdt/fdt_overlay.c | 23 ++++-------------------
> >  libfdt/libfdt.h      | 19 +++++++++++++++++++
> >  libfdt/version.lds   |  1 +
> >  3 files changed, 24 insertions(+), 19 deletions(-)
> > 
> > diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> > index d217e79..e141053 100644
> > --- a/libfdt/fdt_overlay.c
> > +++ b/libfdt/fdt_overlay.c
> > @@ -40,22 +40,7 @@ static uint32_t overlay_get_target_phandle(const void *fdto, int fragment)
> >  	return fdt32_to_cpu(*val);
> >  }
> >  
> > -/**
> > - * overlay_get_target - retrieves the offset of a fragment's target
> > - * @fdt: Base device tree blob
> > - * @fdto: Device tree overlay blob
> > - * @fragment: node offset of the fragment in the overlay
> > - * @pathp: pointer which receives the path of the target (or NULL)
> > - *
> > - * overlay_get_target() retrieves the target offset in the base
> > - * device tree of a fragment, no matter how the actual targeting is
> > - * done (through a phandle or a path)
> > - *
> > - * returns:
> > - *      the targeted node offset in the base device tree
> > - *      Negative error code on error
> > - */
> > -static int overlay_get_target(const void *fdt, const void *fdto,
> > +int fdt_overlay_get_target(const void *fdt, const void *fdto,
> >  			      int fragment, char const **pathp)
> 
> "overlay_get_target" was fine as an internal name.  When published, I
> think "fdt_overlay_target_offset" would better fit the naming
> conventions for the rest of the interface.
> 
> Similarly, I'd like the @fragment parameter renamed @fragment_offset.
> 
> [The insistent use of explicit "offset" throughout the interface is
>  deliberate - it's an attempt to constantly remind users that these
>  quantities are explicit offsets in the blob, which can be invalidated
>  by other changes, rather than some sort of safe, stable handle on a
>  node]
> 
Understood, i will make the changes and send v2 soon.
Thanks for reviewing the patch. 
> >  {
> >  	uint32_t phandle;
> > @@ -636,7 +621,7 @@ static int overlay_merge(void *fdt, void *fdto)
> >  		if (overlay < 0)
> >  			return overlay;
> >  
> > -		target = overlay_get_target(fdt, fdto, fragment, NULL);
> > +		target = fdt_overlay_get_target(fdt, fdto, fragment, NULL);
> >  		if (target < 0)
> >  			return target;
> >  
> > @@ -779,7 +764,7 @@ static int overlay_symbol_update(void *fdt, void *fdto)
> >  			return -FDT_ERR_BADOVERLAY;
> >  
> >  		/* get the target of the fragment */
> > -		ret = overlay_get_target(fdt, fdto, fragment, &target_path);
> > +		ret = fdt_overlay_get_target(fdt, fdto, fragment, &target_path);
> >  		if (ret < 0)
> >  			return ret;
> >  		target = ret;
> > @@ -801,7 +786,7 @@ static int overlay_symbol_update(void *fdt, void *fdto)
> >  
> >  		if (!target_path) {
> >  			/* again in case setprop_placeholder changed it */
> > -			ret = overlay_get_target(fdt, fdto, fragment, &target_path);
> > +			ret = fdt_overlay_get_target(fdt, fdto, fragment, &target_path);
> >  			if (ret < 0)
> >  				return ret;
> >  			target = ret;
> > diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> > index 7f117e8..5d42533 100644
> > --- a/libfdt/libfdt.h
> > +++ b/libfdt/libfdt.h
> > @@ -2116,6 +2116,25 @@ int fdt_del_node(void *fdt, int nodeoffset);
> >   */
> >  int fdt_overlay_apply(void *fdt, void *fdto);
> >  
> > +/**
> > + * fdt_overlay_get_target - retrieves the offset of a fragment's target
> > + * @fdt: Base device tree blob
> > + * @fdto: Device tree overlay blob
> > + * @fragment: node offset of the fragment in the overlay
> > + * @pathp: pointer which receives the path of the target (or NULL)
> > + *
> > + * fdt_overlay_get_target() retrieves the target offset in the base
> > + * device tree of a fragment, no matter how the actual targeting is
> > + * done (through a phandle or a path)
> > + *
> > + * returns:
> > + *      the targeted node offset in the base device tree
> > + *      Negative error code on error
> > + */
> > +
> > +int fdt_overlay_get_target(const void *fdt, const void *fdto, int fragment,
> > +                           char const **pathp);
> > +
> >  /**********************************************************************/
> >  /* Debugging / informational functions                                */
> >  /**********************************************************************/
> > diff --git a/libfdt/version.lds b/libfdt/version.lds
> > index 7ab85f1..9e0daa1 100644
> > --- a/libfdt/version.lds
> > +++ b/libfdt/version.lds
> > @@ -77,6 +77,7 @@ LIBFDT_1.2 {
> >  		fdt_appendprop_addrrange;
> >  		fdt_setprop_inplace_namelen_partial;
> >  		fdt_create_with_flags;
> > +		fdt_overlay_get_target;
> >  	local:
> >  		*;
> >  };
> 
> -- 
> 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