Re: [PATCH v5 3/4] of: reserved_mem: Use the unflatten_devicetree APIs to scan reserved mem. nodes

[Oreoluwa Babatunde]

On 4/11/2024 11:59 AM, Rob Herring wrote:
> On Thu, Mar 28, 2024 at 4:16 PM Oreoluwa Babatunde
> <quic_obabatun@xxxxxxxxxxx> wrote:
>> The unflatten_devicetree APIs have been setup and are available to be
>> used by the time the fdt_init_reserved_mem() function is called.
>> Since the unflatten_devicetree APIs are a more efficient way of scanning
>> through the DT nodes, switch to using these APIs to facilitate the rest
>> of the reserved memory processing.
> Please use get_maintainers.pl. Specifically, you missed maintainers
> for kernel/dma/.

Sorry about that, Will include them in the next version.

>
>> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@xxxxxxxxxxx>
>> ---
>>  drivers/of/of_reserved_mem.c    | 77 +++++++++++++++++++++------------
>>  include/linux/of_reserved_mem.h |  2 +-
>>  kernel/dma/coherent.c           |  8 ++--
>>  kernel/dma/contiguous.c         |  8 ++--
>>  kernel/dma/swiotlb.c            | 10 ++---
>>  5 files changed, 64 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
>> index 0aba366eba59..68d1f4cca4bb 100644
>> --- a/drivers/of/of_reserved_mem.c
>> +++ b/drivers/of/of_reserved_mem.c
>> @@ -99,7 +99,7 @@ static void __init alloc_reserved_mem_array(void)
>>  /*
>>   * fdt_reserved_mem_save_node() - save fdt node for second pass initialization
>>   */
>> -static void __init fdt_reserved_mem_save_node(unsigned long node, const char *uname,
>> +static void __init fdt_reserved_mem_save_node(struct device_node *node, const char *uname,
>>                                               phys_addr_t base, phys_addr_t size)
>>  {
>>         struct reserved_mem *rmem = &reserved_mem[reserved_mem_count];
>> @@ -109,7 +109,7 @@ static void __init fdt_reserved_mem_save_node(unsigned long node, const char *un
>>                 return;
>>         }
>>
>> -       rmem->fdt_node = node;
>> +       rmem->dev_node = node;
>>         rmem->name = uname;
>>         rmem->base = base;
>>         rmem->size = size;
>> @@ -178,11 +178,11 @@ static int __init __reserved_mem_reserve_reg(unsigned long node,
>>  }
>>
>>  /*
>> - * __reserved_mem_check_root() - check if #size-cells, #address-cells provided
>> + * __fdt_reserved_mem_check_root() - check if #size-cells, #address-cells provided
>>   * in /reserved-memory matches the values supported by the current implementation,
>>   * also check if ranges property has been provided
>>   */
>> -static int __init __reserved_mem_check_root(unsigned long node)
>> +static int __init __fdt_reserved_mem_check_root(unsigned long node)
>>  {
>>         const __be32 *prop;
>>
>> @@ -200,6 +200,29 @@ static int __init __reserved_mem_check_root(unsigned long node)
>>         return 0;
>>  }
>>
>> +/*
>> + * __dt_reserved_mem_check_root() - check if #size-cells, #address-cells provided
>> + * in /reserved-memory matches the values supported by the current implementation,
>> + * also check if ranges property has been provided
>> + */
>> +static int __init __dt_reserved_mem_check_root(struct device_node *node)
> The normal prefix is 'of', not 'dt'. Weird, right?, but it dates back
> to OpenFirmware.
Got it! I will follow the same for the functions that I renamed in Patch 04 as well.
>
>> +{
>> +       const __be32 *prop;
>> +
>> +       prop = of_get_property(node, "#size-cells", NULL);
> Throughout, use the appropriate typed function. Here for example,
> of_property_read_u32().
ack.
>
>> +       if (!prop || be32_to_cpup(prop) != dt_root_size_cells)
>> +               return -EINVAL;
>> +
>> +       prop = of_get_property(node, "#address-cells", NULL);
>> +       if (!prop || be32_to_cpup(prop) != dt_root_addr_cells)
>> +               return -EINVAL;
>> +
>> +       prop = of_get_property(node, "ranges", NULL);
> Or for presence, just of_property_present().
ack.
>
>> +       if (!prop)
>> +               return -EINVAL;
>> +       return 0;
>> +}
>> +
>>  /**
>>   * fdt_scan_reserved_mem_reg_nodes() - Store info for the "reg" defined
>>   * reserved memory regions.
>> @@ -213,33 +236,38 @@ static int __init __reserved_mem_check_root(unsigned long node)
>>  static void __init fdt_scan_reserved_mem_reg_nodes(void)
>>  {
>>         int t_len = (dt_root_addr_cells + dt_root_size_cells) * sizeof(__be32);
>> -       const void *fdt = initial_boot_params;
>> +       struct device_node *node, *child;
>>         phys_addr_t base, size;
>>         const __be32 *prop;
>> -       int node, child;
>>         int len;
>>
>> -       node = fdt_path_offset(fdt, "/reserved-memory");
>> -       if (node < 0) {
>> +       node = of_find_node_by_path("/reserved-memory");
>> +       if (!node) {
>>                 pr_info("Reserved memory: No reserved-memory node in the DT\n");
>>                 return;
>>         }
>>
>> -       if (__reserved_mem_check_root(node)) {
>> +       if (__dt_reserved_mem_check_root(node)) {
>>                 pr_err("Reserved memory: unsupported node format, ignoring\n");
>>                 return;
>>         }
>>
>> -       fdt_for_each_subnode(child, fdt, node) {
>> +       for_each_child_of_node(node, child) {
>>                 const char *uname;
>> +               struct reserved_mem *rmem;
>>
>> -               prop = of_get_flat_dt_prop(child, "reg", &len);
>> -               if (!prop)
>> +               if (!of_device_is_available(child))
>>                         continue;
>> -               if (!of_fdt_device_is_available(fdt, child))
>> +
>> +               prop = of_get_property(child, "reg", &len);
> We have specific unflattened functions for reading 'reg'. Note that
> you should use the 'translated' ones even though we have a check to
> disallow any real translation. That restriction should be fixed at
> some point.
"of_address_to_resource()" seems to be a function that reads the 'reg' property
the way you are describing here. I'll switch to that function and see how it works!

Please let me know if you had another function in mind for me to use here.
>
>> +               if (!prop) {
>> +                       rmem = of_reserved_mem_lookup(child);
>> +                       if (rmem)
>> +                               rmem->dev_node = child;
>>                         continue;
>> +               }
>>
>> -               uname = fdt_get_name(fdt, child, NULL);
>> +               uname = of_node_full_name(child);
>>                 if (len && len % t_len != 0) {
>>                         pr_err("Reserved memory: invalid reg property in '%s', skipping node.\n",
>>                                uname);
>> @@ -269,7 +297,7 @@ int __init fdt_scan_reserved_mem(void)
>>         if (node < 0)
>>                 return -ENODEV;
>>
>> -       if (__reserved_mem_check_root(node) != 0) {
>> +       if (__fdt_reserved_mem_check_root(node) != 0) {
>>                 pr_err("Reserved memory: unsupported node format, ignoring\n");
>>                 return -EINVAL;
>>         }
>> @@ -447,7 +475,7 @@ static int __init __reserved_mem_alloc_size(unsigned long node, const char *unam
>>                        uname, (unsigned long)(size / SZ_1M));
>>                 return -ENOMEM;
>>         }
>> -       fdt_reserved_mem_save_node(node, uname, base, size);
>> +       fdt_reserved_mem_save_node(NULL, uname, base, size);
>>         return 0;
>>  }
>>
>> @@ -467,7 +495,7 @@ static int __init __reserved_mem_init_node(struct reserved_mem *rmem)
>>                 reservedmem_of_init_fn initfn = i->data;
>>                 const char *compat = i->compatible;
>>
>> -               if (!of_flat_dt_is_compatible(rmem->fdt_node, compat))
>> +               if (!of_device_is_compatible(rmem->dev_node, compat))
>>                         continue;
>>
>>                 ret = initfn(rmem);
>> @@ -500,11 +528,6 @@ static int __init __rmem_cmp(const void *a, const void *b)
>>         if (ra->size > rb->size)
>>                 return 1;
>>
>> -       if (ra->fdt_node < rb->fdt_node)
>> -               return -1;
>> -       if (ra->fdt_node > rb->fdt_node)
>> -               return 1;
>> -
>>         return 0;
>>  }
>>
>> @@ -551,16 +574,16 @@ void __init fdt_init_reserved_mem(void)
>>
>>         for (i = 0; i < reserved_mem_count; i++) {
>>                 struct reserved_mem *rmem = &reserved_mem[i];
>> -               unsigned long node = rmem->fdt_node;
>> +               struct device_node *node = rmem->dev_node;
>>                 int len;
>>                 const __be32 *prop;
>>                 int err = 0;
>>                 bool nomap;
>>
>> -               nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
>> -               prop = of_get_flat_dt_prop(node, "phandle", &len);
>> +               nomap = of_get_property(node, "no-map", NULL) != NULL;
>> +               prop = of_get_property(node, "phandle", &len);
> We store the phandle in struct device_node, so reading and storing it
> here shouldn't be needed I think.
ack.
>
>>                 if (!prop)
>> -                       prop = of_get_flat_dt_prop(node, "linux,phandle", &len);
>> +                       prop = of_get_property(node, "linux,phandle", &len);
>>                 if (prop)
>>                         rmem->phandle = of_read_number(prop, len/4);
>>
>> @@ -574,7 +597,7 @@ void __init fdt_init_reserved_mem(void)
>>                 } else {
>>                         phys_addr_t end = rmem->base + rmem->size - 1;
>>                         bool reusable =
>> -                               (of_get_flat_dt_prop(node, "reusable", NULL)) != NULL;
>> +                               (of_get_property(node, "reusable", NULL)) != NULL;
>>
>>                         pr_info("%pa..%pa (%lu KiB) %s %s %s\n",
>>                                 &rmem->base, &end, (unsigned long)(rmem->size / SZ_1K),
>> diff --git a/include/linux/of_reserved_mem.h b/include/linux/of_reserved_mem.h
>> index 4de2a24cadc9..b6107a18d170 100644
>> --- a/include/linux/of_reserved_mem.h
>> +++ b/include/linux/of_reserved_mem.h
>> @@ -10,7 +10,7 @@ struct reserved_mem_ops;
>>
>>  struct reserved_mem {
>>         const char                      *name;
>> -       unsigned long                   fdt_node;
>> +       struct device_node              *dev_node;
>>         unsigned long                   phandle;
>>         const struct reserved_mem_ops   *ops;
>>         phys_addr_t                     base;
>> diff --git a/kernel/dma/coherent.c b/kernel/dma/coherent.c
>> index ff5683a57f77..0db0aae83102 100644
>> --- a/kernel/dma/coherent.c
>> +++ b/kernel/dma/coherent.c
>> @@ -362,20 +362,20 @@ static const struct reserved_mem_ops rmem_dma_ops = {
>>
>>  static int __init rmem_dma_setup(struct reserved_mem *rmem)
>>  {
>> -       unsigned long node = rmem->fdt_node;
>> +       struct device_node *node = rmem->dev_node;
>>
>> -       if (of_get_flat_dt_prop(node, "reusable", NULL))
>> +       if (of_get_property(node, "reusable", NULL))
>>                 return -EINVAL;
>>
>>  #ifdef CONFIG_ARM
>> -       if (!of_get_flat_dt_prop(node, "no-map", NULL)) {
>> +       if (!of_get_property(node, "no-map", NULL)) {
> While you are here, convert this to IS_ENABLED():
>
> if (IS_ENABLED(CONFIG_ARM) && !of_property_read_bool(node)) {
>   ...
> }
ack.


Thank you for your feedback!

Oreoluwa