linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] of: make MAX_RESERVED_REGIONS configurable
@ 2018-11-21  8:11 miles.chen
  2018-11-21 16:39 ` Rob Herring
  0 siblings, 1 reply; 6+ messages in thread
From: miles.chen @ 2018-11-21  8:11 UTC (permalink / raw)
  To: Rob Herring, Frank Rowand
  Cc: devicetree, linux-kernel, linux-arm-kernel, linux-mediatek,
	wsd_upstream, 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] of: make MAX_RESERVED_REGIONS configurable
  2018-11-21  8:11 [RFC PATCH] of: make MAX_RESERVED_REGIONS configurable miles.chen
@ 2018-11-21 16:39 ` Rob Herring
  2018-11-22  2:51   ` Miles Chen
  0 siblings, 1 reply; 6+ messages in thread
From: Rob Herring @ 2018-11-21 16:39 UTC (permalink / raw)
  To: Miles Chen
  Cc: Frank Rowand, devicetree, linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-mediatek, wsd_upstream

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] of: make MAX_RESERVED_REGIONS configurable
  2018-11-21 16:39 ` Rob Herring
@ 2018-11-22  2:51   ` Miles Chen
  2018-11-24 20:56     ` Rob Herring
  0 siblings, 1 reply; 6+ messages in thread
From: Miles Chen @ 2018-11-22  2:51 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, devicetree, linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-mediatek, wsd_upstream

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] of: make MAX_RESERVED_REGIONS configurable
  2018-11-22  2:51   ` Miles Chen
@ 2018-11-24 20:56     ` Rob Herring
  2018-11-26  1:33       ` Miles Chen
  0 siblings, 1 reply; 6+ messages in thread
From: Rob Herring @ 2018-11-24 20:56 UTC (permalink / raw)
  To: Miles Chen
  Cc: Frank Rowand, devicetree, linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-mediatek, wsd_upstream

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
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] of: make MAX_RESERVED_REGIONS configurable
  2018-11-24 20:56     ` Rob Herring
@ 2018-11-26  1:33       ` Miles Chen
  2018-11-28  1:56         ` Miles Chen
  0 siblings, 1 reply; 6+ messages in thread
From: Miles Chen @ 2018-11-26  1:33 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, devicetree, linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-mediatek, wsd_upstream

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
> >
> >



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] of: make MAX_RESERVED_REGIONS configurable
  2018-11-26  1:33       ` Miles Chen
@ 2018-11-28  1:56         ` Miles Chen
  0 siblings, 0 replies; 6+ messages in thread
From: Miles Chen @ 2018-11-28  1:56 UTC (permalink / raw)
  To: Rob Herring
  Cc: Frank Rowand, devicetree, linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	linux-mediatek, wsd_upstream

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
> > >
> > >
> 
> 



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-11-28  1:56 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-21  8:11 [RFC PATCH] of: make MAX_RESERVED_REGIONS configurable miles.chen
2018-11-21 16:39 ` Rob Herring
2018-11-22  2:51   ` Miles Chen
2018-11-24 20:56     ` Rob Herring
2018-11-26  1:33       ` Miles Chen
2018-11-28  1:56         ` Miles Chen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).