RE: [PATCH v10 1/2] of: reserved_mem: Restruture how the reserved memory regions are processed

[William Zhang <william.zhang@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 31739 bytes --]

> -----Original Message-----
> From: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
> Sent: Friday, June 27, 2025 11:02 AM
> To: William Zhang <william.zhang@broadcom.com>; robh@kernel.org
> Cc: aisheng.dong@nxp.com; andy@black.fi.intel.com;
> catalin.marinas@arm.com; devicetree@vger.kernel.org; hch@lst.de;
> iommu@lists.linux.dev; kernel@quicinc.com; klarasmodin@gmail.com; linux-
> kernel@vger.kernel.org; m.szyprowski@samsung.com;
> quic_ninanaik@quicinc.com; robin.murphy@arm.com; saravanak@google.com;
> will@kernel.org; oreoluwa.babatunde@oss.qualcomm.com
> Subject: Re: [PATCH v10 1/2] of: reserved_mem: Restruture how the reserved
> memory regions are processed
>
>
>
> On 6/22/2025 6:24 PM, William Zhang wrote:
> > On Tue, Jun 17, 2025 at 10:15 AM William Zhang
> > <william.zhang@broadcom.com> wrote:
> >>
> >> Hi Oreoluwa,
> >>
> >> On 10/8/2024 3:06 PM, Oreoluwa Babatunde wrote:
> >>> Reserved memory regions defined in the devicetree can be broken up
> >>> into
> >>> two groups:
> >>> i) Statically-placed reserved memory regions
> >>> i.e. regions defined with a static start address and size using the
> >>>       "reg" property.
> >>> ii) Dynamically-placed reserved memory regions.
> >>> i.e. regions defined by specifying an address range where they can be
> >>>       placed in memory using the "alloc_ranges" and "size" properties.
> >>>
> >>> These regions are processed and set aside at boot time.
> >>> This is done in two stages as seen below:
> >>>
> >>> Stage 1:
> >>> At this stage, fdt_scan_reserved_mem() scans through the child nodes
> >>> of
> >>> the reserved_memory node using the flattened devicetree and does the
> >>> following:
> >>>
> >>> 1) If the node represents a statically-placed reserved memory region,
> >>>     i.e. if it is defined using the "reg" property:
> >>>     - Call memblock_reserve() or memblock_mark_nomap() as needed.
> >>>     - Add the information for that region into the reserved_mem array
> >>>       using fdt_reserved_mem_save_node().
> >>>       i.e. fdt_reserved_mem_save_node(node, name, base, size).
> >>>
> >>> 2) If the node represents a dynamically-placed reserved memory region,
> >>>     i.e. if it is defined using "alloc-ranges" and "size" properties:
> >>>     - Add the information for that region to the reserved_mem array
> >>> with
> >>>       the starting address and size set to 0.
> >>>       i.e. fdt_reserved_mem_save_node(node, name, 0, 0).
> >>>     Note: This region is saved to the array with a starting address of
> >>> 0
> >>>     because a starting address is not yet allocated for it.
> >>>
> >>> Stage 2:
> >>> After iterating through all the reserved memory nodes and storing
> >>> their
> >>> relevant information in the reserved_mem array,fdt_init_reserved_mem()
> >>> is
> >>> called and does the following:
> >>>
> >>> 1) For statically-placed reserved memory regions:
> >>>     - Call the region specific init function using
> >>>       __reserved_mem_init_node().
> >>> 2) For dynamically-placed reserved memory regions:
> >>>     - Call __reserved_mem_alloc_size() which is used to allocate
> >>> memory
> >>>       for each of these regions, and mark them as nomap if they have
> >>> the
> >>>       nomap property specified in the DT.
> >>>     - Call the region specific init function.
> >>>
> >>> The current size of the resvered_mem array is 64 as is defined by
> >>> MAX_RESERVED_REGIONS. This means that there is a limitation of 64 for
> >>> how many reserved memory regions can be specified on a system.
> >>> As systems continue to grow more and more complex, the number of
> >>> reserved memory regions needed are also growing and are starting to
> >>> hit
> >>> this 64 count limit, hence the need to make the reserved_mem array
> >>> dynamically sized (i.e. dynamically allocating memory for the
> >>> reserved_mem array using membock_alloc_*).
> >>>
> >>> On architectures such as arm64, memory allocated using memblock is
> >>> writable only after the page tables have been setup. This means that
> >>> if
> >>> the reserved_mem array is going to be dynamically allocated, it needs
> >>> to
> >>> happen after the page tables have been setup, not before.
> >>>
> >>> Since the reserved memory regions are currently being processed and
> >>> added to the array before the page tables are setup, there is a need
> >>> to
> >>> change the order in which some of the processing is done to allow for
> >>> the reserved_mem array to be dynamically sized.
> >>>
> >>> It is possible to process the statically-placed reserved memory
> >>> regions
> >>> without needing to store them in the reserved_mem array until after
> >>> the
> >>> page tables have been setup because all the information stored in the
> >>> array is readily available in the devicetree and can be referenced at
> >>> any time.
> >>> Dynamically-placed reserved memory regions on the other hand get
> >>> assigned a start address only at runtime, and hence need a place to be
> >>> stored once they are allocated since there is no other referrence to
> >>> the
> >>> start address for these regions.
> >>>
> >>> Hence this patch changes the processing order of the reserved memory
> >>> regions in the following ways:
> >>>
> >>> Step 1:
> >>> fdt_scan_reserved_mem() scans through the child nodes of
> >>> the reserved_memory node using the flattened devicetree and does the
> >>> following:
> >>>
> >>> 1) If the node represents a statically-placed reserved memory region,
> >>>     i.e. if it is defined using the "reg" property:
> >>>     - Call memblock_reserve() or memblock_mark_nomap() as needed.
> >>>
> >>> 2) If the node represents a dynamically-placed reserved memory region,
> >>>     i.e. if it is defined using "alloc-ranges" and "size" properties:
> >>>     - Call __reserved_mem_alloc_size() which will:
> >>>       i) Allocate memory for the reserved region and call
> >>>       memblock_mark_nomap() as needed.
> >>>       ii) Call the region specific initialization function using
> >>>       fdt_init_reserved_mem_node().
> >>>       iii) Save the region information in the reserved_mem array using
> >>>       fdt_reserved_mem_save_node().
> >>>
> >>> Step 2:
> >>> 1) This stage of the reserved memory processing is now only used to
> >>> add
> >>>     the statically-placed reserved memory regions into the
> >>> reserved_mem
> >>>     array using fdt_scan_reserved_mem_reg_nodes(), as well as call
> >>> their
> >>>     region specific initialization functions.
> >>>
> >>> 2) This step has also been moved to be after the page tables are
> >>>     setup. Moving this will allow us to replace the reserved_mem
> >>>     array with a dynamically sized array before storing the rest of
> >>>     these regions.
> >> I am running into a call trace with this order change on armv7 chip
> >> when
> >> I tried to allocate dma coherent memory from the device reserved
> >> memory.
> >> The issue does not happen on armv8 chips.
> >>
> >> [    0.000000] Reserved memory: created CMA memory pool at 0x1e000000,
> >> size 32 MiB
> >> [    0.000000] OF: reserved mem: initialized node dt_reserved_cma,
> >> compatible id shared-dma-pool
> >> [    0.000000] OF: reserved mem: 0x1e000000..0x1fffffff (32768 KiB) map
> >> reusable dt_reserved_cma
> >> ....
> >>
> >> [    0.445322] ------------[ cut here ]------------
> >> [    0.445353] WARNING: CPU: 0 PID: 1 at mm/memory.c:3069
> >> __apply_to_page_range+0x380/0x388
> >> [    0.488911] Modules linked in:
> >> [    0.492027] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted
> >> 6.16.0-rc1-g27605c8c0f69-dirty #3 NONE
> >> [    0.501174] Hardware name: Generic DT based system
> >> [    0.505965] Call trace:
> >> [    0.505985]  unwind_backtrace from show_stack+0x10/0x14
> >> [    0.513764]  show_stack from dump_stack_lvl+0x54/0x68
> >> [    0.518834]  dump_stack_lvl from __warn+0x7c/0x128
> >> [    0.523639]  __warn from warn_slowpath_fmt+0x184/0x18c
> >> [    0.527676] Freeing initrd memory: 65536K
> >> [    0.532788]  warn_slowpath_fmt from
> __apply_to_page_range+0x380/0x388
> >> [    0.539242]  __apply_to_page_range from
> apply_to_page_range+0x1c/0x24
> >> [    0.545689]  apply_to_page_range from
> __alloc_from_contiguous+0xc0/0x14c
> >> [    0.552398]  __alloc_from_contiguous from
> cma_allocator_alloc+0x34/0x3c
> >> [    0.559016]  cma_allocator_alloc from arch_dma_alloc+0x11c/0x2ac
> >> [    0.565025]  arch_dma_alloc from dma_alloc_attrs+0x90/0x2e8
> >> [    0.570603]  dma_alloc_attrs from dmydev_probe+0x8c/0xe8
> >> [    0.575919]  dmydev_probe from platform_probe+0x5c/0xb0
> >> [    0.581152]  platform_probe from really_probe+0xc8/0x2c8
> >> [    0.586467]  really_probe from __driver_probe_device+0x88/0x19c
> >> [    0.592387]  __driver_probe_device from
> >> driver_probe_device+0x30/0x104
> >> [    0.598915]  driver_probe_device from __driver_attach+0x90/0x178
> >> [    0.604921]  __driver_attach from bus_for_each_dev+0x7c/0xcc
> >> [    0.610582]  bus_for_each_dev from bus_add_driver+0xcc/0x1ec
> >> [    0.616241]  bus_add_driver from driver_register+0x7c/0x114
> >> [    0.621814]  driver_register from dmydev_init+0x20/0x28
> >> [    0.627045]  dmydev_init from do_one_initcall+0x58/0x200
> >> [    0.632363]  do_one_initcall from kernel_init_freeable+0x1cc/0x228
> >> [    0.638550]  kernel_init_freeable from kernel_init+0x1c/0x12c
> >> [    0.644299]  kernel_init from ret_from_fork+0x14/0x28
> >> [    0.649351] Exception stack(0xe0819fb0 to 0xe0819ff8)
> >> [    0.654401] 9fa0:                                     00000000
> >> 00000000 00000000 00000000
> >> [    0.662575] 9fc0: 00000000 00000000 00000000 00000000 00000000
> >> 00000000 00000000 00000000
> >> [    0.670747] 9fe0: 00000000 00000000 00000000 00000000 00000013
> 00000000
> >> [    0.677403] ---[ end trace 0000000000000000 ]---
> >> [    0.682083] dmydev dmy_device: Allocate dma memory at 0xde000000 dma
> >> addr 0x1e000000
> >>
> >> The reason is that now reserved memory's fixup function
> >> dma_contiguous_early_fixup is called after the page table is
> >> initialized. This fixup function increases the dma_mmu_remap count for
> >> each reserved memory. And the dma_contiguous_remap function depends
> on
> >> it to properly set up the reserved memory mmu table entry. Before this
> >> change, the paging_init function calls dma_contiguous_remap and it
> >> founds the reserved memory and set it up properly.  After the change,
> >> this function found there is no reserved memory so skip any
> >> initialization hence causes the crash later on when my driver tries to
> >> allocate dma memory from the reserved memory.
> >>
> >> My workaround below is to move the dma_contiguous_remap out from the
> >> paging_init function to the place right after unflatten_device_tree
> >> where the dma_mmu_remap count is correctly set. But this is not ideal
> >> solution and would like to see if you have any better way to solve the
> >> issue.
> >>
> >> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> >> index a41c93988d2c..535d1bf44529 100644
> >> --- a/arch/arm/kernel/setup.c
> >> +++ b/arch/arm/kernel/setup.c
> >> @@ -1079,6 +1079,7 @@ void __init hyp_mode_check(void)
> >>   #endif
> >>   }
> >>
> >> +void __init dma_contiguous_remap(void);
> >>   static void (*__arm_pm_restart)(enum reboot_mode reboot_mode, const
> >> char *cmd);
> >>
> >>   static int arm_restart(struct notifier_block *nb, unsigned long
> >> action,
> >> @@ -1164,6 +1165,7 @@ void __init setup_arch(char **cmdline_p)
> >>          }
> >>
> >>          unflatten_device_tree();
> >> +       dma_contiguous_remap();
> >>
> >>          arm_dt_init_cpu_maps();
> >>          psci_dt_init();
> >> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> >> index edb7f56b7c91..1828c8737d70 100644
> >> --- a/arch/arm/mm/mmu.c
> >> +++ b/arch/arm/mm/mmu.c
> >> @@ -1773,7 +1773,6 @@ void __init paging_init(const struct machine_desc
> >> *mdesc)
> >>           * be used
> >>           */
> >>          map_kernel();
> >> -       dma_contiguous_remap();
> >>          early_fixmap_shutdown();
> >>          devicemaps_init(mdesc);
> >>          kmap_init();
> >>
> >> You can reproduce the issue on any v7 devices by adding these nodes to
> >> the device tree
> >> +       reserved-memory {
> >> +               #address-cells = <1>;
> >> +               #size-cells = <1>;
> >> +               ranges;
> >> +
> >> +               dt_reserved_cma: dt_reserved_cma {
> >> +                       compatible = "shared-dma-pool";
> >> +                       reusable;
> >> +
> >> +                       reg = <0x1e000000 0x2000000>;
> >> +               };
> >> +       };
> >> +
> >> +       dmy_device {
> >> +               compatible = "xyz,dmydev";
> >> +               memory-region = <&dt_reserved_cma>;
> >> +       };
> >>
> >> And use this test driver to trigger the call stack:
> >> diff --git a/drivers/char/dmydev.c b/drivers/char/dmydev.c
> >> new file mode 100644
> >> index 000000000000..1dd52ec492eb
> >> --- /dev/null
> >> +++ b/drivers/char/dmydev.c
> >> @@ -0,0 +1,67 @@
> >> +#include<linux/module.h>
> >> +#include<linux/kernel.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_reserved_mem.h>
> >> +#include <linux/dma-mapping.h>
> >> +
> >> +static int dmydev_probe(struct platform_device *pdev)
> >> +{
> >> +       void* virt_addr;
> >> +       dma_addr_t dma_addr;
> >> +       int ret;
> >> +
> >> +       printk(KERN_ALERT "dmydev_probe called\n");
> >> +
> >> +       ret = of_reserved_mem_device_init(&pdev->dev);
> >> +       if (ret && ret != -ENODEV) {
> >> +               dev_err(&pdev->dev, "Couldn't assign reserve memory to
> >> device ret = %d\n", ret);
> >> +                       return ret;
> >> +       }
> >> +
> >> +       virt_addr = dma_alloc_coherent(&pdev->dev, 0x800000, &dma_addr,
> >> GFP_KERNEL);
> >> +       if (virt_addr == NULL) {
> >> +               dev_err(&pdev->dev,"Failed to allocated cma memory\n");
> >> +               ret = -ENOMEM;
> >> +       }
> >> +       else
> >> +               dev_err(&pdev->dev,"Allocate dma memory at 0x%px dma
> >> addr %pad\n", virt_addr, &dma_addr);
> >> +
> >> +       return ret;
> >> +}
> >> +
> >> +static void dmydev_remove(struct platform_device *pdev)
> >> +{
> >> +}
> >> +
> >> +static const struct of_device_id dmydev_of_match[] = {
> >> +       {.compatible = "xyz,dmydev"},
> >> +       {}
> >> +};
> >> +MODULE_DEVICE_TABLE(of, dmydev_of_match);
> >>
> >> Let me know if you need more info.
> >>
> >>>
> >>> Signed-off-by: Oreoluwa Babatunde <quic_obabatun@quicinc.com>
> >>> ---
> >>>   drivers/of/fdt.c             |   5 +-
> >>>   drivers/of/of_private.h      |   3 +-
> >>>   drivers/of/of_reserved_mem.c | 168
> >>> ++++++++++++++++++++++++----------
> -
> >>>   3 files changed, 122 insertions(+), 54 deletions(-)
> >>>
> >>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> >>> index 4d528c10df3a..d0dbc8183ac4 100644
> >>> --- a/drivers/of/fdt.c
> >>> +++ b/drivers/of/fdt.c
> >>> @@ -511,8 +511,6 @@ void __init early_init_fdt_scan_reserved_mem(void)
> >>>                       break;
> >>>               memblock_reserve(base, size);
> >>>       }
> >>> -
> >>> -     fdt_init_reserved_mem();
> >>>   }
> >>>
> >>>   /**
> >>> @@ -1212,6 +1210,9 @@ void __init unflatten_device_tree(void)
> >>>   {
> >>>       void *fdt = initial_boot_params;
> >>>
> >>> +     /* Save the statically-placed regions in the reserved_mem array
> >>> */
> >>> +     fdt_scan_reserved_mem_reg_nodes();
> >>> +
> >>>       /* Don't use the bootloader provided DTB if ACPI is enabled */
> >>>       if (!acpi_disabled)
> >>>               fdt = NULL;
> >>> diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h
> >>> index 04aa2a91f851..29525c0b9939 100644
> >>> --- a/drivers/of/of_private.h
> >>> +++ b/drivers/of/of_private.h
> >>> @@ -9,6 +9,7 @@
> >>>    */
> >>>
> >>>   #define FDT_ALIGN_SIZE 8
> >>> +#define MAX_RESERVED_REGIONS    64
> >>>
> >>>   /**
> >>>    * struct alias_prop - Alias property in 'aliases' node
> >>> @@ -180,7 +181,7 @@ static inline struct device_node
> *__of_get_dma_parent(const struct device_node *
> >>>   #endif
> >>>
> >>>   int fdt_scan_reserved_mem(void);
> >>> -void fdt_init_reserved_mem(void);
> >>> +void __init fdt_scan_reserved_mem_reg_nodes(void);
> >>>
> >>>   bool of_fdt_device_is_available(const void *blob, unsigned long
> >>> node);
> >>>
> >>> diff --git a/drivers/of/of_reserved_mem.c
> >>> b/drivers/of/of_reserved_mem.c
> >>> index 46e1c3fbc769..2011174211f9 100644
> >>> --- a/drivers/of/of_reserved_mem.c
> >>> +++ b/drivers/of/of_reserved_mem.c
> >>> @@ -27,7 +27,6 @@
> >>>
> >>>   #include "of_private.h"
> >>>
> >>> -#define MAX_RESERVED_REGIONS 64
> >>>   static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
> >>>   static int reserved_mem_count;
> >>>
> >>> @@ -56,6 +55,7 @@ static int __init
> early_init_dt_alloc_reserved_memory_arch(phys_addr_t size,
> >>>       return err;
> >>>   }
> >>>
> >>> +static void __init fdt_init_reserved_mem_node(struct reserved_mem
> *rmem);
> >>>   /*
> >>>    * fdt_reserved_mem_save_node() - save fdt node for second pass
> initialization
> >>>    */
> >>> @@ -74,6 +74,9 @@ static void __init
> fdt_reserved_mem_save_node(unsigned long node, const char *un
> >>>       rmem->base = base;
> >>>       rmem->size = size;
> >>>
> >>> +     /* Call the region specific initialization function */
> >>> +     fdt_init_reserved_mem_node(rmem);
> >>> +
> >>>       reserved_mem_count++;
> >>>       return;
> >>>   }
> >>> @@ -106,7 +109,6 @@ static int __init
> __reserved_mem_reserve_reg(unsigned long node,
> >>>       phys_addr_t base, size;
> >>>       int len;
> >>>       const __be32 *prop;
> >>> -     int first = 1;
> >>>       bool nomap;
> >>>
> >>>       prop = of_get_flat_dt_prop(node, "reg", &len);
> >>> @@ -134,10 +136,6 @@ static int __init
> __reserved_mem_reserve_reg(unsigned long node,
> >>>                              uname, &base, (unsigned long)(size /
> >>> SZ_1M));
> >>>
> >>>               len -= t_len;
> >>> -             if (first) {
> >>> -                     fdt_reserved_mem_save_node(node, uname, base,
> >>> size);
> >>> -                     first = 0;
> >>> -             }
> >>>       }
> >>>       return 0;
> >>>   }
> >>> @@ -165,12 +163,77 @@ static int __init
> __reserved_mem_check_root(unsigned long node)
> >>>       return 0;
> >>>   }
> >>>
> >>> +static void __init __rmem_check_for_overlap(void);
> >>> +
> >>> +/**
> >>> + * fdt_scan_reserved_mem_reg_nodes() - Store info for the "reg"
> >>> defined
> >>> + * reserved memory regions.
> >>> + *
> >>> + * This function is used to scan through the DT and store the
> >>> + * information for the reserved memory regions that are defined using
> >>> + * the "reg" property. The region node number, name, base address,
> >>> and
> >>> + * size are all stored in the reserved_mem array by calling the
> >>> + * fdt_reserved_mem_save_node() function.
> >>> + */
> >>> +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;
> >>> +     phys_addr_t base, size;
> >>> +     const __be32 *prop;
> >>> +     int node, child;
> >>> +     int len;
> >>> +
> >>> +     if (!fdt)
> >>> +             return;
> >>> +
> >>> +     node = fdt_path_offset(fdt, "/reserved-memory");
> >>> +     if (node < 0) {
> >>> +             pr_info("Reserved memory: No reserved-memory node in the
> DT\n");
> >>> +             return;
> >>> +     }
> >>> +
> >>> +     if (__reserved_mem_check_root(node)) {
> >>> +             pr_err("Reserved memory: unsupported node format,
> >>> ignoring\n");
> >>> +             return;
> >>> +     }
> >>> +
> >>> +     fdt_for_each_subnode(child, fdt, node) {
> >>> +             const char *uname;
> >>> +
> >>> +             prop = of_get_flat_dt_prop(child, "reg", &len);
> >>> +             if (!prop)
> >>> +                     continue;
> >>> +             if (!of_fdt_device_is_available(fdt, child))
> >>> +                     continue;
> >>> +
> >>> +             uname = fdt_get_name(fdt, child, NULL);
> >>> +             if (len && len % t_len != 0) {
> >>> +                     pr_err("Reserved memory: invalid reg property in
> >>> '%s', skipping
> node.\n",
> >>> +                            uname);
> >>> +                     continue;
> >>> +             }
> >>> +             base = dt_mem_next_cell(dt_root_addr_cells, &prop);
> >>> +             size = dt_mem_next_cell(dt_root_size_cells, &prop);
> >>> +
> >>> +             if (size)
> >>> +                     fdt_reserved_mem_save_node(child, uname, base,
> >>> size);
> >>> +     }
> >>> +
> >>> +     /* check for overlapping reserved regions */
> >>> +     __rmem_check_for_overlap();
> >>> +}
> >>> +
> >>> +static int __init __reserved_mem_alloc_size(unsigned long node, const
> char *uname);
> >>> +
> >>>   /*
> >>>    * fdt_scan_reserved_mem() - scan a single FDT node for reserved
> >>> memory
> >>>    */
> >>>   int __init fdt_scan_reserved_mem(void)
> >>>   {
> >>>       int node, child;
> >>> +     int dynamic_nodes_cnt = 0;
> >>> +     int dynamic_nodes[MAX_RESERVED_REGIONS];
> >>>       const void *fdt = initial_boot_params;
> >>>
> >>>       node = fdt_path_offset(fdt, "/reserved-memory");
> >>> @@ -192,8 +255,24 @@ int __init fdt_scan_reserved_mem(void)
> >>>               uname = fdt_get_name(fdt, child, NULL);
> >>>
> >>>               err = __reserved_mem_reserve_reg(child, uname);
> >>> -             if (err == -ENOENT && of_get_flat_dt_prop(child, "size",
> >>> NULL))
> >>> -                     fdt_reserved_mem_save_node(child, uname, 0, 0);
> >>> +             /*
> >>> +              * Save the nodes for the dynamically-placed regions
> >>> +              * into an array which will be used for allocation right
> >>> +              * after all the statically-placed regions are reserved
> >>> +              * or marked as no-map. This is done to avoid
> >>> dynamically
> >>> +              * allocating from one of the statically-placed regions.
> >>> +              */
> >>> +             if (err == -ENOENT && of_get_flat_dt_prop(child, "size",
> >>> NULL)) {
> >>> +                     dynamic_nodes[dynamic_nodes_cnt] = child;
> >>> +                     dynamic_nodes_cnt++;
> >>> +             }
> >>> +     }
> >>> +     for (int i = 0; i < dynamic_nodes_cnt; i++) {
> >>> +             const char *uname;
> >>> +
> >>> +             child = dynamic_nodes[i];
> >>> +             uname = fdt_get_name(fdt, child, NULL);
> >>> +             __reserved_mem_alloc_size(child, uname);
> >>>       }
> >>>       return 0;
> >>>   }
> >>> @@ -253,8 +332,7 @@ static int __init
> __reserved_mem_alloc_in_range(phys_addr_t size,
> >>>    * __reserved_mem_alloc_size() - allocate reserved memory described
> >>> by
> >>>    *  'size', 'alignment'  and 'alloc-ranges' properties.
> >>>    */
> >>> -static int __init __reserved_mem_alloc_size(unsigned long node,
> >>> -     const char *uname, phys_addr_t *res_base, phys_addr_t *res_size)
> >>> +static int __init __reserved_mem_alloc_size(unsigned long node, const
> char *uname)
> >>>   {
> >>>       int t_len = (dt_root_addr_cells + dt_root_size_cells) *
> >>> sizeof(__be32);
> >>>       phys_addr_t start = 0, end = 0;
> >>> @@ -334,9 +412,8 @@ static int __init
> __reserved_mem_alloc_size(unsigned long node,
> >>>               return -ENOMEM;
> >>>       }
> >>>
> >>> -     *res_base = base;
> >>> -     *res_size = size;
> >>> -
> >>> +     /* Save region in the reserved_mem array */
> >>> +     fdt_reserved_mem_save_node(node, uname, base, size);
> >>>       return 0;
> >>>   }
> >>>
> >>> @@ -425,48 +502,37 @@ static void __init
> __rmem_check_for_overlap(void)
> >>>   }
> >>>
> >>>   /**
> >>> - * fdt_init_reserved_mem() - allocate and init all saved reserved
> >>> memory
> regions
> >>> + * fdt_init_reserved_mem_node() - Initialize a reserved memory region
> >>> + * @rmem: reserved_mem struct of the memory region to be initialized.
> >>> + *
> >>> + * This function is used to call the region specific initialization
> >>> + * function for a reserved memory region.
> >>>    */
> >>> -void __init fdt_init_reserved_mem(void)
> >>> +static void __init fdt_init_reserved_mem_node(struct reserved_mem
> *rmem)
> >>>   {
> >>> -     int i;
> >>> -
> >>> -     /* check for overlapping reserved regions */
> >>> -     __rmem_check_for_overlap();
> >>> -
> >>> -     for (i = 0; i < reserved_mem_count; i++) {
> >>> -             struct reserved_mem *rmem = &reserved_mem[i];
> >>> -             unsigned long node = rmem->fdt_node;
> >>> -             int err = 0;
> >>> -             bool nomap;
> >>> +     unsigned long node = rmem->fdt_node;
> >>> +     int err = 0;
> >>> +     bool nomap;
> >>>
> >>> -             nomap = of_get_flat_dt_prop(node, "no-map", NULL) !=
> >>> NULL;
> >>> +     nomap = of_get_flat_dt_prop(node, "no-map", NULL) != NULL;
> >>>
> >>> -             if (rmem->size == 0)
> >>> -                     err = __reserved_mem_alloc_size(node,
> >>> rmem->name,
> >>> -                                              &rmem->base,
> >>> &rmem->size);
> >>> -             if (err == 0) {
> >>> -                     err = __reserved_mem_init_node(rmem);
> >>> -                     if (err != 0 && err != -ENOENT) {
> >>> -                             pr_info("node %s compatible matching
> >>> fail\n",
> >>> -                                     rmem->name);
> >>> -                             if (nomap)
> >>> -                                     memblock_clear_nomap(rmem->base,
> >>> rmem->size);
> >>> -                             else
> >>> -                                     memblock_phys_free(rmem->base,
> >>> -                                                        rmem->size);
> >>> -                     } else {
> >>> -                             phys_addr_t end = rmem->base +
> >>> rmem->size - 1;
> >>> -                             bool reusable =
> >>> -                                     (of_get_flat_dt_prop(node,
> >>> "reusable", NULL)) !=
> NULL;
> >>> -
> >>> -                             pr_info("%pa..%pa (%lu KiB) %s %s %s\n",
> >>> -                                     &rmem->base, &end, (unsigned
> >>> long)(rmem->size /
> SZ_1K),
> >>> -                                     nomap ? "nomap" : "map",
> >>> -                                     reusable ? "reusable" :
> >>> "non-reusable",
> >>> -                                     rmem->name ? rmem->name :
> >>> "unknown");
> >>> -                     }
> >>> -             }
> >>> +     err = __reserved_mem_init_node(rmem);
> >>> +     if (err != 0 && err != -ENOENT) {
> >>> +             pr_info("node %s compatible matching fail\n",
> >>> rmem->name);
> >>> +             if (nomap)
> >>> +                     memblock_clear_nomap(rmem->base, rmem->size);
> >>> +             else
> >>> +                     memblock_phys_free(rmem->base, rmem->size);
> >>> +     } else {
> >>> +             phys_addr_t end = rmem->base + rmem->size - 1;
> >>> +             bool reusable =
> >>> +                     (of_get_flat_dt_prop(node, "reusable", NULL)) !=
> >>> NULL;
> >>> +
> >>> +             pr_info("%pa..%pa (%lu KiB) %s %s %s\n",
> >>> +                     &rmem->base, &end, (unsigned long)(rmem->size /
> >>> SZ_1K),
> >>> +                     nomap ? "nomap" : "map",
> >>> +                     reusable ? "reusable" : "non-reusable",
> >>> +                     rmem->name ? rmem->name : "unknown");
> >>>       }
> >>>   }
> >>>
> >>
> > Just want to follow up on this issue.  Do you need any further detail
> > or clarification?
> > Any ARM memory manage guru on this thread can comment?
> > Or is my workaround acceptable as a patch?
>
>
> Hi William,
>
> Sorry about the delay in getting back to you.
>
> Instead of moving dma_contiguous_remap(), I suggest moving
> dma_contiguous_early_fixup()
> to the function that parses the reserved regions so that it is done before
> paging_init.
>
> Here is what that could look like. Can you please give this a try?
>
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 77016c0cc296..132d2c66cafc 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -25,6 +25,7 @@
>  #include <linux/memblock.h>
>  #include <linux/kmemleak.h>
>  #include <linux/cma.h>
> +#include <linux/dma-map-ops.h>
>
>  #include "of_private.h"
>
> @@ -175,13 +176,17 @@ static int __init
> __reserved_mem_reserve_reg(unsigned long node,
>                 base = dt_mem_next_cell(dt_root_addr_cells, &prop);
>                 size = dt_mem_next_cell(dt_root_size_cells, &prop);
>
> -               if (size &&
> -                   early_init_dt_reserve_memory(base, size, nomap) == 0)
> +               if (size && early_init_dt_reserve_memory(base, size,
> nomap) == 0) {
> +                       /* Architecture specific contiguous memory fixup.
> */
> +                       if (of_flat_dt_is_compatible(node,
> "shared-dma-pool"))
> +                               dma_contiguous_early_fixup(base, size);
> +
>                         pr_debug("Reserved memory: reserved region for
> node '%s':
> base %pa, size %lu MiB\n",
>                                 uname, &base, (unsigned long)(size /
> SZ_1M));
> -               else
> +               } else {
>                         pr_err("Reserved memory: failed to reserve memory
> for node '%s':
> base %pa, size %lu MiB\n",
>                                uname, &base, (unsigned long)(size /
> SZ_1M));
> +               }
>
>                 len -= t_len;
>         }
> @@ -472,6 +477,9 @@ static int __init __reserved_mem_alloc_size(unsigned
> long node, const char *unam
>                        uname, (unsigned long)(size / SZ_1M));
>                 return -ENOMEM;
>         }
> +       /* Architecture specific contiguous memory fixup. */
> +       if (of_flat_dt_is_compatible(node, "shared-dma-pool"))
> +               dma_contiguous_early_fixup(base, size);
>         /* Save region in the reserved_mem array */
>         fdt_reserved_mem_save_node(node, uname, base, size);
> diff --git a/kernel/dma/contiguous.c b/kernel/dma/contiguous.c
> index 8df0dfaaca18..9e5d63efe7c5 100644
> --- a/kernel/dma/contiguous.c
> +++ b/kernel/dma/contiguous.c
> @@ -480,8 +480,6 @@ static int __init rmem_cma_setup(struct reserved_mem
> *rmem)
>                 pr_err("Reserved memory: unable to setup CMA region\n");
>                 return err;
>         }
> -       /* Architecture specific contiguous memory fixup. */
> -       dma_contiguous_early_fixup(rmem->base, rmem->size);
>
>         if (default_cma)
>                 dma_contiguous_default_area = cma;
>
> Regards,
> Oreoluwa

Thank you Oreoluwa!  Your patch fixed the issue too and it looks a more
localized and better fix!

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4199 bytes --]