[PATCH 0/3] ARM: hisi: fix of_iomap errors

[PATCH 0/3] ARM: hisi: fix of_iomap errors

[Nicholas Mc Guire]

This patch set addresses two issues in arch/arm/mach-hisi/hotplug.c

1) The hisi hotplug functions were using a few unchecked 
   of_iomap() while at the same time the system relied on
   those mappings. Checks for !NULL were inserted.

2) Further some mandatory of_node_put() were missing and have
   been inserted.

[PATCH 1/3] ARM: hisi: fix error handling and missing of_node_put

[Nicholas Mc Guire]

of_iomap() can return NULL which seems critical here and thus should be
explicitly flagged so that the cause of system halting can be understood.
As of_find_compatible_node() is returning a device node with refcount
incremented it must be explicitly decremented here.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Fixes: commit 7fda91e73155 ("ARM: hisi: enable smp for HiP01")
---

Problem located by experimental coccinelle script

Patch was compile tested with: hisi_defconfig (implies CONFIG_SMP=y)

There are two change related checkpatch warnings about
"WARNING: Avoid crashing the kernel - try using WARN_ON"
but here the BUG_ON() seems adequate.

Patch is against 4.18-rc3 (localversion-next is next-20180712)

 arch/arm/mach-hisi/hotplug.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-hisi/hotplug.c b/arch/arm/mach-hisi/hotplug.c
index a129aae..3b0d1c6 100644
--- a/arch/arm/mach-hisi/hotplug.c
+++ b/arch/arm/mach-hisi/hotplug.c
@@ -219,10 +219,10 @@ void hip01_set_cpu(int cpu, bool enable)
 
 	if (!ctrl_base) {
 		np = of_find_compatible_node(NULL, NULL, "hisilicon,hip01-sysctrl");
-		if (np)
-			ctrl_base = of_iomap(np, 0);
-		else
-			BUG();
+		BUG_ON(!np);
+		ctrl_base = of_iomap(np, 0);
+		of_node_put(np);
+		BUG_ON(!ctrl_base);
 	}
 
 	if (enable) {
-- 
2.1.4

[PATCH 2/3] ARM: hisi: check of_iomap and fix missing of_node_put

[Nicholas Mc Guire]

of_find_compatible_node() returns a device node with refcount incremented
and thus needs an explicit of_node_put(). Further relying on an unchecked
of_iomap() which can return NULL is problematic here, after all ctrl_base
is critical enough for hix5hd2_set_cpu() to call BUG() if not available
so a check seems mandated here.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
0002 Fixes: commit 06cc5c1d4d73 ("ARM: hisi: enable hix5hd2 SoC")
---
Problem found by an experimental coccinelle script

Patch was compile tested with: hisi_defconfig (implies CONFIG_SMP=y)

Patch is against 4.18-rc3 (localversion-next is next-20180712)

 arch/arm/mach-hisi/hotplug.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-hisi/hotplug.c b/arch/arm/mach-hisi/hotplug.c
index 3b0d1c6..40857bf 100644
--- a/arch/arm/mach-hisi/hotplug.c
+++ b/arch/arm/mach-hisi/hotplug.c
@@ -173,11 +173,15 @@ static bool hix5hd2_hotplug_init(void)
 	struct device_node *np;
 
 	np = of_find_compatible_node(NULL, NULL, "hisilicon,cpuctrl");
-	if (np) {
-		ctrl_base = of_iomap(np, 0);
-		return true;
-	}
-	return false;
+	if (!np)
+		return false;
+
+	ctrl_base = of_iomap(np, 0);
+	of_node_put(np);
+	if (!ctrl_base)
+		return false;
+
+	return true;
 }
 
 void hix5hd2_set_cpu(int cpu, bool enable)
-- 
2.1.4

[PATCH 3/3] ARM: hisi: handle of_iomap and fix missing of_node_put

[Nicholas Mc Guire]

Relying on an unchecked of_iomap() which can return NULL is problematic
here, an explicit check seems mandatory. Also the call to
of_find_compatible_node() returns a device node with refcount incremented
therefor an explicit of_node_put() is needed here. 

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Fixes: commit 22bae4290457 ("ARM: hi3xxx: add hotplug support")
---

Problem found by an experimental coccinelle script

The way  id  is used is a bit redundant with the function return - not
really clear if this "double indication" is intentional or just happened ?
Also note that hi3xxx_hotplug_init probably should be bool as that is how
it is being used - the error return is not actually interpreted beyond
detection of failure in its only call site hi3xxx_set_cpu().

Patch was compile tested with: hisi_defconfig (implies CONFIG_SMP=y)

Patch is against 4.18-rc3 (localversion-next is next-20180712)

 arch/arm/mach-hisi/hotplug.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-hisi/hotplug.c b/arch/arm/mach-hisi/hotplug.c
index 40857bf..4036ffe 100644
--- a/arch/arm/mach-hisi/hotplug.c
+++ b/arch/arm/mach-hisi/hotplug.c
@@ -148,13 +148,20 @@ static int hi3xxx_hotplug_init(void)
 	struct device_node *node;
 
 	node = of_find_compatible_node(NULL, NULL, "hisilicon,sysctrl");
-	if (node) {
-		ctrl_base = of_iomap(node, 0);
-		id = HI3620_CTRL;
-		return 0;
+	if (!node) {
+		id = ERROR_CTRL;
+		return -ENOENT;
 	}
-	id = ERROR_CTRL;
-	return -ENOENT;
+
+	ctrl_base = of_iomap(node, 0);
+	of_node_put(node);
+	if (!ctrl_base) {
+		id = ERROR_CTRL;
+		return -ENOMEM;
+	}
+
+	id = HI3620_CTRL;
+	return 0;
 }
 
 void hi3xxx_set_cpu(int cpu, bool enable)
-- 
2.1.4

Re: [PATCH 0/3] ARM: hisi: fix of_iomap errors

[Wei Xu]

Hi Nicholas,

On 2018/7/12 10:28, Nicholas Mc Guire wrote:
> This patch set addresses two issues in arch/arm/mach-hisi/hotplug.c
> 
> 1) The hisi hotplug functions were using a few unchecked 
>    of_iomap() while at the same time the system relied on
>    those mappings. Checks for !NULL were inserted.
> 
> 2) Further some mandatory of_node_put() were missing and have
>    been inserted.
> 
> 
> .
> 

Thanks!
Applied all the 3 patches to the hisilicon SoC tree.

Best Regards,
Wei

[PATCH 0/4] ARM: imx legacy: cleanups

[Nicholas Mc Guire]

While preparing a proposed fix for a missing check on zmalloc a few
other checkpatch warnings poped up - this little set fixes those
warnings. There is one remaining checkpatch warning but that looks
like a false-positive to me:

ERROR: Use of __initconst requires a separate use of const
#171: FILE: arch/arm/mach-imx/mach-mx27ads.c:171:
+mx27ads_nand_board_info __initconst = {

The false positive seems due to the definition spanning two lines
anyway looks good to me

<snip>
static const struct mxc_nand_platform_data
mx27ads_nand_board_info __initconst = {
        .width = 1,
        .hw_ecc = 1,
};
<snip>

thx!
hofrat

[PATCH 0/3] ARM: hisi: fix of_iomap errors

[Nicholas Mc Guire]

This patch set addresses two issues in arch/arm/mach-hisi/hotplug.c

1) The hisi hotplug functions were using a few unchecked 
   of_iomap() while at the same time the system relied on
   those mappings. Checks for !NULL were inserted.

2) Further some mandatory of_node_put() were missing and have
   been inserted.