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/[email protected]/ > [1] > Link: > https://lore.kernel.org/imx/alexXTYQHUs76J7v@SMW015318/T/#mb9dc709ff3adc24d9db6d30c973f37b580f6f1f9 > [2] > > Signed-off-by: Laurentiu Mihalcea <[email protected]> > Reviewed-by: Peng Fan <[email protected]> > Signed-off-by: Frank Li <[email protected]> > > 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? 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 <[email protected]>
