On Thu, Mar 20, 2025 at 4:23 AM Arnaud POULIQUEN
<arnaud.pouliquen@xxxxxxxxxxx> wrote:
>
>
>
> On 3/20/25 00:04, Rob Herring wrote:
> > On Wed, Mar 19, 2025 at 10:26 AM Arnaud POULIQUEN
> > <arnaud.pouliquen@xxxxxxxxxxx> wrote:
> >>
> >> Hello Rob,
> >>
> >> On 3/18/25 00:24, Rob Herring (Arm) wrote:
> >>> Use the newly added of_reserved_mem_region_to_resource() and
> >>> of_reserved_mem_region_count() functions to handle "memory-region"
> >>> properties.
> >>>
> >>> The error handling is a bit different in some cases. Often
> >>> "memory-region" is optional, so failed lookup is not an error. But then
> >>> an error in of_reserved_mem_lookup() is treated as an error. However,
> >>> that distinction is not really important. Either the region is available
> >>> and usable or it is not. So now, it is just
> >>> of_reserved_mem_region_to_resource() which is checked for an error.
> >>>
> >>> Signed-off-by: Rob Herring (Arm) <robh@xxxxxxxxxx>
> >>> ---
> >>> For v6.16
> >>>
> >
> > [...]
> >
> >>> diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
> >>> index b02b36a3f515..9d2bd8904c49 100644
> >>> --- a/drivers/remoteproc/stm32_rproc.c
> >>> +++ b/drivers/remoteproc/stm32_rproc.c
> >>> @@ -213,52 +213,46 @@ static int stm32_rproc_prepare(struct rproc *rproc)
> >>> {
> >>> struct device *dev = rproc->dev.parent;
> >>> struct device_node *np = dev->of_node;
> >>> - struct of_phandle_iterator it;
> >>> struct rproc_mem_entry *mem;
> >>> - struct reserved_mem *rmem;
> >>> u64 da;
> >>> - int index = 0;
> >>> + int index = 0, mr = 0;
> >>>
> >>> /* Register associated reserved memory regions */
> >>> - of_phandle_iterator_init(&it, np, "memory-region", NULL, 0);
> >>> - while (of_phandle_iterator_next(&it) == 0) {
> >>> - rmem = of_reserved_mem_lookup(it.node);
> >>> - if (!rmem) {
> >>> - of_node_put(it.node);
> >>> - dev_err(dev, "unable to acquire memory-region\n");
> >>> - return -EINVAL;
> >>> - }
> >>> + while (1) {
> >>> + struct resource res;
> >>> + int ret;
> >>> +
> >>> + ret = of_reserved_mem_region_to_resource(np, mr++, &res);
> >>> + if (ret)
> >>> + return 0;
> >>>
> >>> - if (stm32_rproc_pa_to_da(rproc, rmem->base, &da) < 0) {
> >>> - of_node_put(it.node);
> >>> - dev_err(dev, "memory region not valid %pa\n",
> >>> - &rmem->base);
> >>> + if (stm32_rproc_pa_to_da(rproc, res.start, &da) < 0) {
> >>> + dev_err(dev, "memory region not valid %pR\n", &res);
> >>> return -EINVAL;
> >>> }
> >>>
> >>> /* No need to map vdev buffer */
> >>> - if (strcmp(it.node->name, "vdev0buffer")) {
> >>> + if (strcmp(res.name, "vdev0buffer")) {
> >>
> >> I tested your patches
> >
> > Thank you.
> >
> >> The update introduces a regression here. The strcmp function never returns 0.
> >> Indeed, it.node->name stores the memory region label "vdev0buffer," while
> >> res.name stores the memory region name "vdev0buffer@10042000."
> >>
> >> Several remoteproc drivers may face the same issue as they embed similar code.
> >
> > Indeed. I confused myself because node 'name' is without the
> > unit-address, but this is using the full name. I've replaced the
> > strcmp's with strstarts() to address this. I've updated my branch with
> > the changes.
>
> This is not enough as the remoteproc core function rproc_find_carveout_by_name()
> also compares the memory names. With the following additional fix, it is working
> on my STM32MP15-DK board.
>
> @@ -309,11 +309,11 @@ rproc_find_carveout_by_name(struct rproc *rproc, const
> char *name, ...)
> vsnprintf(_name, sizeof(_name), name, args);
> va_end(args);
>
> list_for_each_entry(carveout, &rproc->carveouts, node) {
> /* Compare carveout and requested names */
> - if (!strcmp(carveout->name, _name)) {
> + if (strstarts(carveout->name, _name)) {
> mem = carveout;
> break;
> }
> }
>
> I just wonder if would not be more suitable to address this using the
> "memory-region-names" field.
That would be better as you shouldn't really care what a provider node
name is where-as "memory-region-names" is meaningful to the driver.
>
> The drawback is that we would break compatibility with legacy boards...
So not an option.
I think I'll have to fix this within the reserved mem code storing the
name or do something like the diff below. I'd like to avoid the
former. Using the original device_node.name is also problematic
because I want to get rid of it. We redundantly store the node name
with and without the unit-address. There's a lot of places like this
one where we hand out the pointer with no lifetime.
diff --git a/drivers/remoteproc/stm32_rproc.c b/drivers/remoteproc/stm32_rproc.c
index 1e949694d365..cdee87c6ffe0 100644
--- a/drivers/remoteproc/stm32_rproc.c
+++ b/drivers/remoteproc/stm32_rproc.c
@@ -239,7 +239,7 @@ static int stm32_rproc_prepare(struct rproc *rproc)
resource_size(&res), da,
stm32_rproc_mem_alloc,
stm32_rproc_mem_release,
- res.name);
+ "%.*s",
strchrnul(res.name, '@') - res.name, res.name);
if (mem)
rproc_coredump_add_segment(rproc, da,
@@ -249,7 +249,7 @@ static int stm32_rproc_prepare(struct rproc *rproc)
mem = rproc_of_resm_mem_entry_init(dev, index,
resource_size(&res),
res.start,
- res.name);
+ "vdev0buffer");
}
if (!mem) {
