Re: [Linux-stm32] [PATCH 3/3] remoteproc: Use of_reserved_mem_region_* functions for "memory-region"

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

On 3/20/25 19:02, Rob Herring wrote:
> On Thu, Mar 20, 2025 at 4:23 AM Arnaud POULIQUEN
> <arnaud.pouliquen@foss.st.com> wrote:
>>
>>
>>
>> On 3/20/25 00:04, Rob Herring wrote:
>>> On Wed, Mar 19, 2025 at 10:26 AM Arnaud POULIQUEN
>>> <arnaud.pouliquen@foss.st.com> 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@kernel.org>
>>>>> ---
>>>>> 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) {


That's work on my side.
Could we have an OF helper to retrieve the name from the full name?

Thanks,
Arnaud