[PATCH] of: of_reserved_mem: clean-up reserved memory with no-map

[PATCH] of: of_reserved_mem: clean-up reserved memory with no-map

[skseofh]

From: Daero Lee <daero_le.lee@samsung.com>

In early_init_dt_reserve_memory we only add memory w/o no-map flag to
memblock.reserved. But we need to add memory w/ no-map flag to
memblock.reserved, because NOMAP and memblock.reserved are semantically
different.

Signed-off-by: Daero Lee <daero_le.lee@samsung.com>
---
 drivers/of/of_reserved_mem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 8236ecae2953..1c916da8adaf 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -91,7 +91,8 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
 		    memblock_is_region_reserved(base, size))
 			return -EBUSY;
 
-		return memblock_mark_nomap(base, size);
+		if (memblock_mark_nomap(base, size))
+			return;
 	}
 	return memblock_reserve(base, size);
 }
-- 
2.25.1

Re: [PATCH] of: of_reserved_mem: clean-up reserved memory with no-map

[DaeRo Lee]

2024년 4월 28일 (일) 오후 9:55, <skseofh@gmail.com>님이 작성:
>
> From: Daero Lee <daero_le.lee@samsung.com>
>
> In early_init_dt_reserve_memory we only add memory w/o no-map flag to
> memblock.reserved. But we need to add memory w/ no-map flag to
> memblock.reserved, because NOMAP and memblock.reserved are semantically
> different.
>
> Signed-off-by: Daero Lee <daero_le.lee@samsung.com>
> ---
>  drivers/of/of_reserved_mem.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 8236ecae2953..1c916da8adaf 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -91,7 +91,8 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
>                     memblock_is_region_reserved(base, size))
>                         return -EBUSY;
>
> -               return memblock_mark_nomap(base, size);
> +               if (memblock_mark_nomap(base, size))
> +                       return;
Sorry. The return value is wrong.

Here is what I want to do:

--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -81,6 +81,7 @@ static void __init
fdt_reserved_mem_save_node(unsigned long node, const char *un
 static int __init early_init_dt_reserve_memory(phys_addr_t base,
                                               phys_addr_t size, bool nomap)
 {
+       int err = 0;
        if (nomap) {
                /*
                 * If the memory is already reserved (by another region), we
@@ -91,7 +92,10 @@ static int __init
early_init_dt_reserve_memory(phys_addr_t base,
                    memblock_is_region_reserved(base, size))
                        return -EBUSY;

-               return memblock_mark_nomap(base, size);
+
+               err = memblock_mark_nomap(base, size);
+               if (err)
+                       return err;
        }
        return memblock_reserve(base, size);
 }


Regards,
DaeRo Lee

Re: [PATCH] of: of_reserved_mem: clean-up reserved memory with no-map

[Mike Rapoport]

On Sun, Apr 28, 2024 at 10:10:17PM +0900, DaeRo Lee wrote:
> 2024년 4월 28일 (일) 오후 9:55, <skseofh@gmail.com>님이 작성:
> >
> > From: Daero Lee <daero_le.lee@samsung.com>
> >
> > In early_init_dt_reserve_memory we only add memory w/o no-map flag to
> > memblock.reserved. But we need to add memory w/ no-map flag to
> > memblock.reserved, because NOMAP and memblock.reserved are semantically
> > different.
> >
> > Signed-off-by: Daero Lee <daero_le.lee@samsung.com>
> > ---
> >  drivers/of/of_reserved_mem.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> > index 8236ecae2953..1c916da8adaf 100644
> > --- a/drivers/of/of_reserved_mem.c
> > +++ b/drivers/of/of_reserved_mem.c
> > @@ -91,7 +91,8 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
> >                     memblock_is_region_reserved(base, size))
> >                         return -EBUSY;
> >
> > -               return memblock_mark_nomap(base, size);
> > +               if (memblock_mark_nomap(base, size))
> > +                       return;
> Sorry. The return value is wrong.
> 
> Here is what I want to do:
> 
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -81,6 +81,7 @@ static void __init
> fdt_reserved_mem_save_node(unsigned long node, const char *un
>  static int __init early_init_dt_reserve_memory(phys_addr_t base,
>                                                phys_addr_t size, bool nomap)
>  {
> +       int err = 0;
>         if (nomap) {
>                 /*
>                  * If the memory is already reserved (by another region), we
> @@ -91,7 +92,10 @@ static int __init
> early_init_dt_reserve_memory(phys_addr_t base,
>                     memblock_is_region_reserved(base, size))
>                         return -EBUSY;
> 
> -               return memblock_mark_nomap(base, size);
> +
> +               err = memblock_mark_nomap(base, size);
> +               if (err)
> +                       return err;
>         }
>         return memblock_reserve(base, size);
>  }

Makes sense to me.

> Regards,
> DaeRo Lee

-- 
Sincerely yours,
Mike.

Re: [PATCH] of: of_reserved_mem: clean-up reserved memory with no-map

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.9-rc6 next-20240430]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/skseofh-gmail-com/of-of_reserved_mem-clean-up-reserved-memory-with-no-map/20240430-144643
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240428125505.434962-1-skseofh%40gmail.com
patch subject: [PATCH] of: of_reserved_mem: clean-up reserved memory with no-map
config: openrisc-allnoconfig (https://download.01.org/0day-ci/archive/20240501/202405010907.PHM9xSMi-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240501/202405010907.PHM9xSMi-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405010907.PHM9xSMi-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/of/of_reserved_mem.c: In function 'early_init_dt_reserve_memory':
>> drivers/of/of_reserved_mem.c:95:25: warning: 'return' with no value, in function returning non-void [-Wreturn-type]
      95 |                         return;
         |                         ^~~~~~
   drivers/of/of_reserved_mem.c:81:19: note: declared here
      81 | static int __init early_init_dt_reserve_memory(phys_addr_t base,
         |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/return +95 drivers/of/of_reserved_mem.c

    80	
    81	static int __init early_init_dt_reserve_memory(phys_addr_t base,
    82						       phys_addr_t size, bool nomap)
    83	{
    84		if (nomap) {
    85			/*
    86			 * If the memory is already reserved (by another region), we
    87			 * should not allow it to be marked nomap, but don't worry
    88			 * if the region isn't memory as it won't be mapped.
    89			 */
    90			if (memblock_overlaps_region(&memblock.memory, base, size) &&
    91			    memblock_is_region_reserved(base, size))
    92				return -EBUSY;
    93	
    94			if (memblock_mark_nomap(base, size))
  > 95				return;
    96		}
    97		return memblock_reserve(base, size);
    98	}
    99	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] of: of_reserved_mem: clean-up reserved memory with no-map

[kernel test robot]

Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.9-rc6 next-20240430]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/skseofh-gmail-com/of-of_reserved_mem-clean-up-reserved-memory-with-no-map/20240430-144643
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240428125505.434962-1-skseofh%40gmail.com
patch subject: [PATCH] of: of_reserved_mem: clean-up reserved memory with no-map
config: arm-defconfig (https://download.01.org/0day-ci/archive/20240501/202405011208.qsZQwChO-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240501/202405011208.qsZQwChO-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405011208.qsZQwChO-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/of/of_reserved_mem.c:95:4: warning: non-void function 'early_init_dt_reserve_memory' should return a value [-Wreturn-type]
                           return;
                           ^
   1 warning generated.


vim +/early_init_dt_reserve_memory +95 drivers/of/of_reserved_mem.c

    80	
    81	static int __init early_init_dt_reserve_memory(phys_addr_t base,
    82						       phys_addr_t size, bool nomap)
    83	{
    84		if (nomap) {
    85			/*
    86			 * If the memory is already reserved (by another region), we
    87			 * should not allow it to be marked nomap, but don't worry
    88			 * if the region isn't memory as it won't be mapped.
    89			 */
    90			if (memblock_overlaps_region(&memblock.memory, base, size) &&
    91			    memblock_is_region_reserved(base, size))
    92				return -EBUSY;
    93	
    94			if (memblock_mark_nomap(base, size))
  > 95				return;
    96		}
    97		return memblock_reserve(base, size);
    98	}
    99	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2] of: of_reserved_mem: clean-up reserved memory with no-map

[skseofh]

From: Daero Lee <daero_le.lee@samsung.com>

In early_init_dt_reserve_memory we only add memory w/o no-map flag to
memblock.reserved. But we need to add memory w/ no-map flag to
memblock.reserved, because NOMAP and memblock.reserved are semantically
different.

Signed-off-by: Daero Lee <daero_le.lee@samsung.com>
---
 drivers/of/of_reserved_mem.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
index 8236ecae2953..d00a17a9cebc 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -81,6 +81,7 @@ static void __init fdt_reserved_mem_save_node(unsigned long node, const char *un
 static int __init early_init_dt_reserve_memory(phys_addr_t base,
 					       phys_addr_t size, bool nomap)
 {
+	int err = 0;
 	if (nomap) {
 		/*
 		 * If the memory is already reserved (by another region), we
@@ -91,7 +92,10 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
 		    memblock_is_region_reserved(base, size))
 			return -EBUSY;
 
-		return memblock_mark_nomap(base, size);
+
+		err = memblock_mark_nomap(base, size);
+		if (err)
+			return err;
 	}
 	return memblock_reserve(base, size);
 }
-- 
2.25.1

Re: [PATCH v2] of: of_reserved_mem: clean-up reserved memory with no-map

[Rob Herring]

On Wed, May 01, 2024 at 10:23:59PM +0900, skseofh@gmail.com wrote:
> From: Daero Lee <daero_le.lee@samsung.com>
> 
> In early_init_dt_reserve_memory we only add memory w/o no-map flag to
> memblock.reserved. But we need to add memory w/ no-map flag to
> memblock.reserved, because NOMAP and memblock.reserved are semantically
> different.
> 
> Signed-off-by: Daero Lee <daero_le.lee@samsung.com>
> ---
>  drivers/of/of_reserved_mem.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> index 8236ecae2953..d00a17a9cebc 100644
> --- a/drivers/of/of_reserved_mem.c
> +++ b/drivers/of/of_reserved_mem.c
> @@ -81,6 +81,7 @@ static void __init fdt_reserved_mem_save_node(unsigned long node, const char *un
>  static int __init early_init_dt_reserve_memory(phys_addr_t base,
>  					       phys_addr_t size, bool nomap)
>  {
> +	int err = 0;
>  	if (nomap) {
>  		/*
>  		 * If the memory is already reserved (by another region), we
> @@ -91,7 +92,10 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
>  		    memblock_is_region_reserved(base, size))
>  			return -EBUSY;
>  
> -		return memblock_mark_nomap(base, size);
> +
> +		err = memblock_mark_nomap(base, size);

The last time this was touched, it was to make the handling aligned with 
EFI memory map handling. Is that still going to be the case with this 
change? Or does EFI memory map handling have the same issue?

> +		if (err)
> +			return err;
>  	}
>  	return memblock_reserve(base, size);
>  }
> -- 
> 2.25.1
> 

Re: [PATCH v2] of: of_reserved_mem: clean-up reserved memory with no-map

[DaeRo Lee]

2024년 5월 22일 (수) 오후 11:31, Rob Herring <robh@kernel.org>님이 작성:
>
> On Wed, May 01, 2024 at 10:23:59PM +0900, skseofh@gmail.com wrote:
> > From: Daero Lee <daero_le.lee@samsung.com>
> >
> > In early_init_dt_reserve_memory we only add memory w/o no-map flag to
> > memblock.reserved. But we need to add memory w/ no-map flag to
> > memblock.reserved, because NOMAP and memblock.reserved are semantically
> > different.
> >
> > Signed-off-by: Daero Lee <daero_le.lee@samsung.com>
> > ---
> >  drivers/of/of_reserved_mem.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> > index 8236ecae2953..d00a17a9cebc 100644
> > --- a/drivers/of/of_reserved_mem.c
> > +++ b/drivers/of/of_reserved_mem.c
> > @@ -81,6 +81,7 @@ static void __init fdt_reserved_mem_save_node(unsigned long node, const char *un
> >  static int __init early_init_dt_reserve_memory(phys_addr_t base,
> >                                              phys_addr_t size, bool nomap)
> >  {
> > +     int err = 0;
> >       if (nomap) {
> >               /*
> >                * If the memory is already reserved (by another region), we
> > @@ -91,7 +92,10 @@ static int __init early_init_dt_reserve_memory(phys_addr_t base,
> >                   memblock_is_region_reserved(base, size))
> >                       return -EBUSY;
> >
> > -             return memblock_mark_nomap(base, size);
> > +
> > +             err = memblock_mark_nomap(base, size);
>
> The last time this was touched, it was to make the handling aligned with
> EFI memory map handling. Is that still going to be the case with this
> change? Or does EFI memory map handling have the same issue?
Can I get more information about EFI memory map handling that you're saying?

1) Are you talking about uefi_mem in the reserved-memory node like below?
 ex) arm64/boot/dts/qcom/qcs404.dtsi
                uefi_mem: memory@9f800000 {
                        reg = <0 0x9f800000 0 0x800000>;
                        no-map;
                };

2) Or, about handling EFI memory map function efi_init() -> reserve_regions()?

>
> > +             if (err)
> > +                     return err;
> >       }
> >       return memblock_reserve(base, size);
> >  }
> > --
> > 2.25.1
> >