Re: [PATCH v5 2/3] remoteproc: imx_rpoc: use memory-region-names to avoid hidden ABI

[Frank Li]

On Thu, Oct 15, 2026 at 10:26:51PM +0200, Francesco Valla wrote:
> Hello Frank,
>
> > The driver currently derives the memory region type from the referenced
> > memory node name, creating a hidden DT ABI. Since memory node names are
> > not constrained by the binding, incorrect names can silently pass DT
> > validation, as seen in examples using "vdevbuffer" instead of
> > "vdev0buffer" ([1] and [2]).
> >
> > Parse memory-region-names to identify the expected memory regions and avoid
> > relying on memory node names as ABI.
> >
> > Link: https://lore.kernel.org/imx/a3602ef8-2b77-4cc3-adef-2c4cba08a5d9@xxxxxxxxxxxx/ [1]
> > Link: https://lore.kernel.org/imx/alexXTYQHUs76J7v@SMW015318/T/#mb9dc709ff3adc24d9db6d30c973f37b580f6f1f9 [2]
> >
> > Signed-off-by: Laurentiu Mihalcea <laurentiu.mihalcea@xxxxxxx>
> > Reviewed-by: Peng Fan <peng.fan@xxxxxxx>
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> >
> > diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> > index 745ce52cd822..ce9bdfc1fa98 100644
> > --- a/drivers/remoteproc/imx_rproc.c
> > +++ b/drivers/remoteproc/imx_rproc.c
> > @@ -643,7 +643,7 @@ static int imx_rproc_prepare(struct rproc *rproc)
> >  		int err;
> >  		struct resource res;
> >
> > -		err = of_reserved_mem_region_to_resource(np, i++, &res);
> > +		err = imx_rproc_rmem_to_resource(np, i++, &res);
> >  		if (err)
> >  			break;
> >
> > @@ -818,11 +818,20 @@ static int imx_rproc_addr_init(struct imx_rproc *priv,
> >  	if (nph <= 0)
> >  		return 0;
> >
> > +	if (!of_property_present(np, "memory-region-names")) {
> > +		dev_warn(dev, "using node names for carveouts should be avoided\n");
> > +	} else {
> > +		if (nph != of_property_count_strings(np, "memory-region-names")) {
> > +			dev_err(dev, "invalid reserved memory name count\n");
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> >  	/* remap optional addresses */
> >  	for (a = 0; a < nph; a++) {
> >  		struct resource res;
> >
> > -		err = of_reserved_mem_region_to_resource(np, a, &res);
> > +		err = imx_rproc_rmem_to_resource(np, a, &res);
> >  		if (err) {
> >  			dev_err(dev, "unable to resolve memory region\n");
> >  			return err;
> > diff --git a/drivers/remoteproc/imx_rproc.h b/drivers/remoteproc/imx_rproc.h
> > index 0d7d48352a10..3632bc375c71 100644
> > --- a/drivers/remoteproc/imx_rproc.h
> > +++ b/drivers/remoteproc/imx_rproc.h
> > @@ -7,6 +7,8 @@
> >  #ifndef _IMX_RPROC_H
> >  #define _IMX_RPROC_H
> >
> > +#include <linux/of_reserved_mem.h>
> > +
> >  /* address translation table */
> >  struct imx_rproc_att {
> >  	u32 da;	/* device address (From Cortex M4 view)*/
> > @@ -45,4 +47,23 @@ struct imx_rproc_dcfg {
> >  	u32				reset_vector_mask;
> >  };
> >
> > +static inline int imx_rproc_rmem_to_resource(struct device_node *np,
> > +					     int index,
> > +					     struct resource *res)
> > +{
> > +	int ret;
> > +
> > +	ret = of_reserved_mem_region_to_resource(np, index, res);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* "memory-region-names" is optional */
> > +	ret = of_property_read_string_index(np, "memory-region-names",
> > +					    index, &res->name);
> > +	if (ret == -EINVAL)
> > +		return 0;
> > +
> > +	return ret;
> > +}
> > +
>
> Shouldn't this be a regular function in imx_rproc.c instead of being
> defined as inline here?

Yes, will fix at next version.

Frank

> Keeping it in the driver source file would
> probably make the logic easier to follow, also considering that the
> presence of memory-region-names is evaluated there in
> imx_rproc_addr_init().
>
> >  #endif /* _IMX_RPROC_H */
>
> Regards,
> Francesco
>
> --
> Francesco Valla <francesco@xxxxxxxx>