linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] of: base: remove unnecessary for loop
       [not found] <CGME20210701140328epcms1p85149318b6c18fa18b3c7c8e966c14db0@epcms1p8>
@ 2021-07-01 14:03 ` 권오훈
  2021-07-14 22:30   ` Rob Herring
  2021-07-01 14:11 ` [PATCH] of: of_reserved_mem: match memblock_free with memblock_reserve 권오훈
  1 sibling, 1 reply; 4+ messages in thread
From: 권오훈 @ 2021-07-01 14:03 UTC (permalink / raw)
  To: robh+dt, frowand.list
  Cc: lee.jones, 권오훈,
	ohkwon1043, devicetree, linux-kernel

In __of_get_next_child function, loop iteration for getting next node is
unnecessary.

for loop is already checking if next is NULL or not, and
of_node_get(next) always returns next itself.

Therefore checking return value in the if clause always evaluates to
true, and thus it always breaks out from for loop in the first iteration.

Remove the unnecessary for loop for readability.

I tested the code as below, and it showed that BUG was never called.

-       for (; next; next = next->sibling)
+       for (; next; next = next->sibling) {
                if (of_node_get(next))
                        break;
+               BUG();
+       }

Signed-off-by: Ohhoon Kwon <ohoono.kwon@samsung.com>
---
 drivers/of/base.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 48e941f99558..ca60988ef428 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -708,9 +708,7 @@ static struct device_node *__of_get_next_child(const struct device_node *node,
 		return NULL;
 
 	next = prev ? prev->sibling : node->child;
-	for (; next; next = next->sibling)
-		if (of_node_get(next))
-			break;
+	of_node_get(next);
 	of_node_put(prev);
 	return next;
 }
-- 
2.17.1

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

* [PATCH] of: of_reserved_mem: match memblock_free with memblock_reserve
       [not found] <CGME20210701140328epcms1p85149318b6c18fa18b3c7c8e966c14db0@epcms1p8>
  2021-07-01 14:03 ` [PATCH] of: base: remove unnecessary for loop 권오훈
@ 2021-07-01 14:11 ` 권오훈
  1 sibling, 0 replies; 4+ messages in thread
From: 권오훈 @ 2021-07-01 14:11 UTC (permalink / raw)
  To: robh+dt, frowand.list
  Cc: lee.jones, 권오훈,
	ohkwon1043, devicetree, linux-kernel

When __reserved_mem_init_node called from fdt_init_reserved_mem fails,
we try to undo __reserved_mem_alloc_size to prevent memory leak.
'commit d0b8ed47e83a ("of: reserved_mem: fix reserve memory leak")'

Meanwhile, __reserved_mem_alloc_size calls
early_init_dt_alloc_reserved_memory_arch to allocate memory,
which calls
1) memblock_remove when rmem is declared nomap,
2) memblock_reserve, otherwise.

static int __init early_init_dt_alloc_reserved_memory_arch(
...
	if (nomap)
		return memblock_remove(base, size);

	return memblock_reserve(base, size);
}

Therefore the proper undo-logic should be as follows:
1) memblock_add when rmem is declared nomap,
2) memblock_free, otherwise.

Match the undo functions for readability.

Signed-off-by: Ohhoon Kwon <ohoono.kwon@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 15e2417974d6..2279e1b55d1d 100644
--- a/drivers/of/of_reserved_mem.c
+++ b/drivers/of/of_reserved_mem.c
@@ -273,9 +273,10 @@ void __init fdt_init_reserved_mem(void)
 			if (err != 0 && err != -ENOENT) {
 				pr_info("node %s compatible matching fail\n",
 					rmem->name);
-				memblock_free(rmem->base, rmem->size);
 				if (nomap)
 					memblock_add(rmem->base, rmem->size);
+				else
+					memblock_free(rmem->base, rmem->size);
 			}
 		}
 	}
-- 
2.17.1

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

* Re: [PATCH] of: base: remove unnecessary for loop
  2021-07-01 14:03 ` [PATCH] of: base: remove unnecessary for loop 권오훈
@ 2021-07-14 22:30   ` Rob Herring
  0 siblings, 0 replies; 4+ messages in thread
From: Rob Herring @ 2021-07-14 22:30 UTC (permalink / raw)
  To: 권오훈
  Cc: ohkwon1043, lee.jones, robh+dt, linux-kernel, devicetree, frowand.list

On Thu, 01 Jul 2021 23:03:28 +0900, 권오훈 wrote:
> In __of_get_next_child function, loop iteration for getting next node is
> unnecessary.
> 
> for loop is already checking if next is NULL or not, and
> of_node_get(next) always returns next itself.
> 
> Therefore checking return value in the if clause always evaluates to
> true, and thus it always breaks out from for loop in the first iteration.
> 
> Remove the unnecessary for loop for readability.
> 
> I tested the code as below, and it showed that BUG was never called.
> 
> -       for (; next; next = next->sibling)
> +       for (; next; next = next->sibling) {
>                 if (of_node_get(next))
>                         break;
> +               BUG();
> +       }
> 
> Signed-off-by: Ohhoon Kwon <ohoono.kwon@samsung.com>
> ---
>  drivers/of/base.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 

Applied, thanks!

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

* Re: [PATCH] of: of_reserved_mem: match memblock_free with memblock_reserve
       [not found] ` <20210701141049epcms1p774955cc32210584be5aca8f1b3126e9c@epcms1p7>
@ 2021-07-01 14:51   ` Rob Herring
  0 siblings, 0 replies; 4+ messages in thread
From: Rob Herring @ 2021-07-01 14:51 UTC (permalink / raw)
  To: ohoono.kwon; +Cc: frowand.list, lee.jones, ohkwon1043, devicetree, linux-kernel

On Thu, Jul 1, 2021 at 8:10 AM 권오훈 <ohoono.kwon@samsung.com> wrote:
>
> When __reserved_mem_init_node called from fdt_init_reserved_mem fails,
>
> we try to undo __reserved_mem_alloc_size to prevent memory leak.
>
> 'commit d0b8ed47e83a ("of: reserved_mem: fix reserve memory leak")'

Your patch is corrupted and not plain text.

In any case, I believe this issue has already been fixed. Check the
latest kernel tree(s).

>
> Meanwhile, __reserved_mem_alloc_size calls
>
> early_init_dt_alloc_reserved_memory_arch to allocate memory,
>
> which calls
>
> 1) memblock_remove when rmem is declared nomap,
>
> 2) memblock_reserve, otherwise.
>
>
>
> static int __init early_init_dt_alloc_reserved_memory_arch(
>
> ...
>
>         if (nomap)
>
>                 return memblock_remove(base, size);
>
>
>
>         return memblock_reserve(base, size);
>
> }
>
>
>
> Therefore the proper undo-logic should be as follows:
>
> 1) memblock_add when rmem is declared nomap,
>
> 2) memblock_free, otherwise.
>
>
>
> Match the undo functions for readability.
>
>
>
> Signed-off-by: Ohhoon Kwon <ohoono.kwon@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 15e2417974d6..2279e1b55d1d 100644
>
> --- a/drivers/of/of_reserved_mem.c
>
> +++ b/drivers/of/of_reserved_mem.c
>
> @@ -273,9 +273,10 @@ void __init fdt_init_reserved_mem(void)
>
>                          if (err != 0 && err != -ENOENT) {
>
>                                  pr_info("node %s compatible matching fail\n",
>
>                                          rmem->name);
>
> -                                memblock_free(rmem->base, rmem->size);
>
>                                  if (nomap)
>
>                                          memblock_add(rmem->base, rmem->size);
>
> +                                else
>
> +                                        memblock_free(rmem->base, rmem->size);
>
>                          }
>
>                  }
>
>          }
>
> --
>
> 2.17.1
>
>
>
>
>
>

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

end of thread, other threads:[~2021-07-14 22:31 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20210701140328epcms1p85149318b6c18fa18b3c7c8e966c14db0@epcms1p8>
2021-07-01 14:03 ` [PATCH] of: base: remove unnecessary for loop 권오훈
2021-07-14 22:30   ` Rob Herring
2021-07-01 14:11 ` [PATCH] of: of_reserved_mem: match memblock_free with memblock_reserve 권오훈
     [not found] <CGME20210701140328epcms1p85149318b6c18fa18b3c7c8e966c14db0@epcms1p7>
     [not found] ` <20210701141049epcms1p774955cc32210584be5aca8f1b3126e9c@epcms1p7>
2021-07-01 14:51   ` Rob Herring

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