[RFC PATCH] of: make MAX_RESERVED_REGIONS configurable

[RFC PATCH] of: make MAX_RESERVED_REGIONS configurable

[miles.chen]

From: Miles Chen <miles.chen@mediatek.com>

When we use more than 32 entries in /resered-memory,
there will be an error message: "not enough space all defined regions.".
We can increase MAX_RESERVED_REGIONS to fix this.

commit 22f8cc6e3373 ("drivers: of: increase MAX_RESERVED_REGIONS to 32")
increased MAX_RESERVED_REGIONS to 32 but I'm not sure if increasing
MAX_RESERVED_REGIONS to 64 is suitable for everyone.

In this RFC patch, CONFIG_MAX_OF_RESERVED_REGIONS is added and used as
MAX_RESERVED_REGIONS. Add a sanity test to make sure that
MAX_RESERVED_REGIONS is less than INIT_MEMBLOCK_REGIONS.
Users can configure CONFIG_MAX_OF_RESERVED_REGIONS according to their
need.

Signed-off-by: Miles Chen <miles.chen@mediatek.com>
---
 drivers/of/Kconfig           | 9 +++++++++
 drivers/of/of_reserved_mem.c | 5 ++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index ad3fcad..63fff2e 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -86,6 +86,15 @@ config OF_RESERVED_MEM
 	help
 	  Helpers to allow for reservation of memory regions
 
+config MAX_OF_RESERVED_REGIONS
+	int "the max number of devicetree reserved memory regions"
+	depends on OF_RESERVED_MEM
+	default 32
+	help
+	  Setup the max number of devicetree reserved memory regions
+	  If unsure, leave the default value "32".
+
+
 config OF_RESOLVE
 	bool
 
diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 1977ee0..18a7729 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -22,7 +22,7 @@
 #include <linux/slab.h>
 #include <linux/memblock.h>
 
-#define MAX_RESERVED_REGIONS	32
+#define MAX_RESERVED_REGIONS	CONFIG_MAX_OF_RESERVED_REGIONS
 static struct reserved_mem reserved_mem[MAX_RESERVED_REGIONS];
 static int reserved_mem_count;
 
@@ -246,6 +246,9 @@ void __init fdt_init_reserved_mem(void)
 {
 	int i;
 
+	/* MAX_RESERVED_REGIONS should be bounded by INIT_MEMBLOCK_REGIONS */
+	BUILD_BUG_ON(MAX_RESERVED_REGIONS > INIT_MEMBLOCK_REGIONS);
+
 	/* check for overlapping reserved regions */
 	__rmem_check_for_overlap();
 
-- 
1.9.1

Re: [RFC PATCH] of: make MAX_RESERVED_REGIONS configurable

[Rob Herring]

On Wed, Nov 21, 2018 at 2:11 AM <miles.chen@mediatek.com> wrote:
>
> From: Miles Chen <miles.chen@mediatek.com>
>
> When we use more than 32 entries in /resered-memory,
> there will be an error message: "not enough space all defined regions.".
> We can increase MAX_RESERVED_REGIONS to fix this.
>
> commit 22f8cc6e3373 ("drivers: of: increase MAX_RESERVED_REGIONS to 32")
> increased MAX_RESERVED_REGIONS to 32 but I'm not sure if increasing
> MAX_RESERVED_REGIONS to 64 is suitable for everyone.
>
> In this RFC patch, CONFIG_MAX_OF_RESERVED_REGIONS is added and used as
> MAX_RESERVED_REGIONS. Add a sanity test to make sure that
> MAX_RESERVED_REGIONS is less than INIT_MEMBLOCK_REGIONS.
> Users can configure CONFIG_MAX_OF_RESERVED_REGIONS according to their
> need.

I don't want a kconfig option for this. I think it should be dynamic instead.

The current flow is like this:

for each reserved node:
  - call memblock_reserve
  - Add info to reserved_mem array

I think we should change it to:

for each reserved node:
  - call memblock_reserve
  - count number of nodes

Alloc array using memblock_alloc

for each reserved node:
   - Add info to reserved_mem array

Rob

Re: [RFC PATCH] of: make MAX_RESERVED_REGIONS configurable

[Miles Chen]

On Wed, 2018-11-21 at 10:39 -0600, Rob Herring wrote:
> On Wed, Nov 21, 2018 at 2:11 AM <miles.chen@mediatek.com> wrote:
> >
> > From: Miles Chen <miles.chen@mediatek.com>
> >
> > When we use more than 32 entries in /resered-memory,
> > there will be an error message: "not enough space all defined regions.".
> > We can increase MAX_RESERVED_REGIONS to fix this.
> >
> > commit 22f8cc6e3373 ("drivers: of: increase MAX_RESERVED_REGIONS to 32")
> > increased MAX_RESERVED_REGIONS to 32 but I'm not sure if increasing
> > MAX_RESERVED_REGIONS to 64 is suitable for everyone.
> >
> > In this RFC patch, CONFIG_MAX_OF_RESERVED_REGIONS is added and used as
> > MAX_RESERVED_REGIONS. Add a sanity test to make sure that
> > MAX_RESERVED_REGIONS is less than INIT_MEMBLOCK_REGIONS.
> > Users can configure CONFIG_MAX_OF_RESERVED_REGIONS according to their
> > need.
> 
> I don't want a kconfig option for this. I think it should be dynamic instead.
> 
> The current flow is like this:
> 
> for each reserved node:
>   - call memblock_reserve
>   - Add info to reserved_mem array
> 
> I think we should change it to:
> 
> for each reserved node:
>   - call memblock_reserve
>   - count number of nodes
> 
> Alloc array using memblock_alloc
> 
> for each reserved node:
>    - Add info to reserved_mem array
> 

thanks for your comment.

I reviewed the flow and it might be easier to count the
nodes and setup array first:

for each reserved node:
    - count number of nodes

Alloc array using memblock_alloc


for each reserved node:
   - call memblock_reserve
   - Add info to reserved_mem array

What do you think?

> Rob

Re: [RFC PATCH] of: make MAX_RESERVED_REGIONS configurable

[Rob Herring]

On Wed, Nov 21, 2018 at 8:51 PM Miles Chen <miles.chen@mediatek.com> wrote:
>
> On Wed, 2018-11-21 at 10:39 -0600, Rob Herring wrote:
> > On Wed, Nov 21, 2018 at 2:11 AM <miles.chen@mediatek.com> wrote:
> > >
> > > From: Miles Chen <miles.chen@mediatek.com>
> > >
> > > When we use more than 32 entries in /resered-memory,
> > > there will be an error message: "not enough space all defined regions.".
> > > We can increase MAX_RESERVED_REGIONS to fix this.
> > >
> > > commit 22f8cc6e3373 ("drivers: of: increase MAX_RESERVED_REGIONS to 32")
> > > increased MAX_RESERVED_REGIONS to 32 but I'm not sure if increasing
> > > MAX_RESERVED_REGIONS to 64 is suitable for everyone.
> > >
> > > In this RFC patch, CONFIG_MAX_OF_RESERVED_REGIONS is added and used as
> > > MAX_RESERVED_REGIONS. Add a sanity test to make sure that
> > > MAX_RESERVED_REGIONS is less than INIT_MEMBLOCK_REGIONS.
> > > Users can configure CONFIG_MAX_OF_RESERVED_REGIONS according to their
> > > need.
> >
> > I don't want a kconfig option for this. I think it should be dynamic instead.
> >
> > The current flow is like this:
> >
> > for each reserved node:
> >   - call memblock_reserve
> >   - Add info to reserved_mem array
> >
> > I think we should change it to:
> >
> > for each reserved node:
> >   - call memblock_reserve
> >   - count number of nodes
> >
> > Alloc array using memblock_alloc
> >
> > for each reserved node:
> >    - Add info to reserved_mem array
> >
>
> thanks for your comment.
>
> I reviewed the flow and it might be easier to count the
> nodes and setup array first:
>
> for each reserved node:
>     - count number of nodes
>
> Alloc array using memblock_alloc
>
>
> for each reserved node:
>    - call memblock_reserve

The order here is wrong. It is important that you reserve the memory
blocks before doing any allocations.

>    - Add info to reserved_mem array
>
> What do you think?
>
> > Rob
>
>

Re: [RFC PATCH] of: make MAX_RESERVED_REGIONS configurable

[Miles Chen]

On Sat, 2018-11-24 at 14:56 -0600, Rob Herring wrote:
> On Wed, Nov 21, 2018 at 8:51 PM Miles Chen <miles.chen@mediatek.com> wrote:
> >
> > On Wed, 2018-11-21 at 10:39 -0600, Rob Herring wrote:
> > > On Wed, Nov 21, 2018 at 2:11 AM <miles.chen@mediatek.com> wrote:
> > > >
> > > > From: Miles Chen <miles.chen@mediatek.com>
> > > >
> > > > When we use more than 32 entries in /resered-memory,
> > > > there will be an error message: "not enough space all defined regions.".
> > > > We can increase MAX_RESERVED_REGIONS to fix this.
> > > >
> > > > commit 22f8cc6e3373 ("drivers: of: increase MAX_RESERVED_REGIONS to 32")
> > > > increased MAX_RESERVED_REGIONS to 32 but I'm not sure if increasing
> > > > MAX_RESERVED_REGIONS to 64 is suitable for everyone.
> > > >
> > > > In this RFC patch, CONFIG_MAX_OF_RESERVED_REGIONS is added and used as
> > > > MAX_RESERVED_REGIONS. Add a sanity test to make sure that
> > > > MAX_RESERVED_REGIONS is less than INIT_MEMBLOCK_REGIONS.
> > > > Users can configure CONFIG_MAX_OF_RESERVED_REGIONS according to their
> > > > need.
> > >
> > > I don't want a kconfig option for this. I think it should be dynamic instead.
> > >
> > > The current flow is like this:
> > >
> > > for each reserved node:
> > >   - call memblock_reserve
> > >   - Add info to reserved_mem array
> > >
> > > I think we should change it to:
> > >
> > > for each reserved node:
> > >   - call memblock_reserve
> > >   - count number of nodes
> > >
> > > Alloc array using memblock_alloc
> > >
> > > for each reserved node:
> > >    - Add info to reserved_mem array
> > >
> >
> > thanks for your comment.
> >
> > I reviewed the flow and it might be easier to count the
> > nodes and setup array first:
> >
> > for each reserved node:
> >     - count number of nodes
> >
> > Alloc array using memblock_alloc
> >
> >
> > for each reserved node:
> >    - call memblock_reserve
> 
> The order here is wrong. It is important that you reserve the memory
> blocks before doing any allocations.

thanks for pointing that out. You are right. I'll follow your
suggestion.

> 
> >    - Add info to reserved_mem array
> >
> > What do you think?
> >
> > > Rob
> >
> >

Re: [RFC PATCH] of: make MAX_RESERVED_REGIONS configurable

[Miles Chen]

On Mon, 2018-11-26 at 09:33 +0800, Miles Chen wrote:
> On Sat, 2018-11-24 at 14:56 -0600, Rob Herring wrote:
> > On Wed, Nov 21, 2018 at 8:51 PM Miles Chen <miles.chen@mediatek.com> wrote:
> > >
> > > On Wed, 2018-11-21 at 10:39 -0600, Rob Herring wrote:
> > > > On Wed, Nov 21, 2018 at 2:11 AM <miles.chen@mediatek.com> wrote:
> > > > >
> > > > > From: Miles Chen <miles.chen@mediatek.com>
> > > > >
> > > > > When we use more than 32 entries in /resered-memory,
> > > > > there will be an error message: "not enough space all defined regions.".
> > > > > We can increase MAX_RESERVED_REGIONS to fix this.
> > > > >
> > > > > commit 22f8cc6e3373 ("drivers: of: increase MAX_RESERVED_REGIONS to 32")
> > > > > increased MAX_RESERVED_REGIONS to 32 but I'm not sure if increasing
> > > > > MAX_RESERVED_REGIONS to 64 is suitable for everyone.
> > > > >
> > > > > In this RFC patch, CONFIG_MAX_OF_RESERVED_REGIONS is added and used as
> > > > > MAX_RESERVED_REGIONS. Add a sanity test to make sure that
> > > > > MAX_RESERVED_REGIONS is less than INIT_MEMBLOCK_REGIONS.
> > > > > Users can configure CONFIG_MAX_OF_RESERVED_REGIONS according to their
> > > > > need.
> > > >
> > > > I don't want a kconfig option for this. I think it should be dynamic instead.
> > > >
> > > > The current flow is like this:
> > > >
> > > > for each reserved node:
> > > >   - call memblock_reserve
> > > >   - Add info to reserved_mem array
> > > >
> > > > I think we should change it to:
> > > >
> > > > for each reserved node:
> > > >   - call memblock_reserve
> > > >   - count number of nodes
> > > >
> > > > Alloc array using memblock_alloc
> > > >
> > > > for each reserved node:
> > > >    - Add info to reserved_mem array
> > > >
> > >
> > > thanks for your comment.
> > >
> > > I reviewed the flow and it might be easier to count the
> > > nodes and setup array first:
> > >
> > > for each reserved node:
> > >     - count number of nodes
> > >
> > > Alloc array using memblock_alloc
> > >
> > >
> > > for each reserved node:
> > >    - call memblock_reserve
> > 
> > The order here is wrong. It is important that you reserve the memory
> > blocks before doing any allocations.
> 
> thanks for pointing that out. You are right. I'll follow your
> suggestion.

I got a problem while implementing this change: the array allocation
is done successfully but the kernel does not create mappings for it yet,
so the array is not accessible. (kernel will create the mapping later in
paging_init())

For example: aarch64 setup_arch()
setup_arch()
{
    memblock_init(); /* early_init_fdt_scan_reserved_mem() is called */
    paging_init();
}

Could you give me some advice, please?

Miles
> 
> > 
> > >    - Add info to reserved_mem array
> > >
> > > What do you think?
> > >
> > > > Rob
> > >
> > >
> 
>