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

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

[PATCH] ARM: add missing of_node_put()

[Nicholas Mc Guire]

 The call to of_find_compatible_node() returns a node pointer with refcount
incremented thus it must be explicitly decremented here after the last
usage. (see drivers/of/base.c:of_find_compatible_node())

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Problem located with an experimental coccinelle script

Patch was compile tested with: axm55xx_defconfig
(with some sparse warnings - not related to the proposed change though)

Patch is against 4.17.0 (localversion-next is next-20180614)

 arch/arm/mach-axxia/platsmp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-axxia/platsmp.c b/arch/arm/mach-axxia/platsmp.c
index 502e3df..c706a11 100644
--- a/arch/arm/mach-axxia/platsmp.c
+++ b/arch/arm/mach-axxia/platsmp.c
@@ -40,10 +40,11 @@ static int axxia_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	syscon_np = of_find_compatible_node(NULL, NULL, "lsi,axxia-syscon");
 	if (!syscon_np)
 		return -ENOENT;
 
 	syscon = of_iomap(syscon_np, 0);
+	of_node_put(syscon_np);
 	if (!syscon)
 		return -ENOMEM;
 
 	tmp = readl(syscon + SC_RST_CPU_HOLD);
 	writel(0xab, syscon + SC_CRIT_WRITE_KEY);
-- 
2.1.4

[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] ARM: imx: flag failure of of_iomap

[Nicholas Mc Guire]

 imx_set_aips is assuming that the address returned from of_iomap is
valid which it probably is in the normal case - as the call site
is void error propagation is not possible but never the less at least
a WARN_ON() seems warranted here.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Fixes: commit e57e4ab5fc2e ("ARM: i.MX: allow disabling supervisor protect via DT")
---

Problem found by experimental coccinelle script

Patch was compiletested with: multi_v7_defconfig

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

 arch/arm/mach-imx/cpu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-imx/cpu.c b/arch/arm/mach-imx/cpu.c
index 32969f3..c6b1bf9 100644
--- a/arch/arm/mach-imx/cpu.c
+++ b/arch/arm/mach-imx/cpu.c
@@ -68,6 +68,7 @@ void __init imx_aips_allow_unprivileged_access(
 
 	for_each_compatible_node(np, NULL, compat) {
 		aips_base_addr = of_iomap(np, 0);
+		WARN_ON(!aips_base_addr);
 		imx_set_aips(aips_base_addr);
 	}
 }
-- 
2.1.4

[PATCH 1/4 RFC] ARM: imx legacy: warn on failed allocation

[Nicholas Mc Guire]

Even in init the allocation can fail and thus should at least warn so
that the cause can be identified.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Problem located with an experimental coccinelle script

Note sure if there is a better solution as this is early in the boot
process so not that could be done anyway but fuss - anyway unchecked
allocation is not a good thing.

Patch was compile-tested with: imx_v4_v5_defconfig (implies
CONFIG_MACH_MX27ADS=y)
(with some unrelated sparse warnings about unimplemented syscalls
 and a few checkpatch warnings - covered in the other patches)

Patch is against 5.1-rc4 (localversion-next is 20190412)

 arch/arm/mach-imx/mach-mx27ads.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-imx/mach-mx27ads.c b/arch/arm/mach-imx/mach-mx27ads.c
index 6dd7f57..528846f 100644
--- a/arch/arm/mach-imx/mach-mx27ads.c
+++ b/arch/arm/mach-imx/mach-mx27ads.c
@@ -247,6 +247,7 @@ static void __init mx27ads_regulator_init(void)
 	struct gpio_chip *vchip;
 
 	vchip = kzalloc(sizeof(*vchip), GFP_KERNEL);
+	WARN_ON(!vchip);
 	vchip->owner		= THIS_MODULE;
 	vchip->label		= "LCD";
 	vchip->base		= MX27ADS_LCD_GPIO;
-- 
2.1.4

[PATCH] ARM: STi: at least warn if of_iomap fails

[Nicholas Mc Guire]

The call to of_iomap() is unchecked but scu_enable(), which the returned
address is passed on to, assumes a valid mapping. If the mapping is
invalid this could probably lead to undefined system state so at least
a warning should be issued.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Fixes: commit 65ebcc115889 ("ARM: sti: Add STiH415 SOC support")
---
Problem was found by an experimental coccinelle script

Patch was compile tested with: multi_v7_defconfig (implies
CONFIG_ARCH_STI=y, CONFIG_SMP=y)

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

 arch/arm/mach-sti/platsmp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-sti/platsmp.c b/arch/arm/mach-sti/platsmp.c
index 231f19e..89ae76f 100644
--- a/arch/arm/mach-sti/platsmp.c
+++ b/arch/arm/mach-sti/platsmp.c
@@ -109,6 +109,7 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus)
 
 	if (np) {
 		scu_base = of_iomap(np, 0);
+		WARN_ON(!scu_base);
 		scu_enable(scu_base);
 		of_node_put(np);
 	}
-- 
2.1.4

[PATCH V2] ARM: STi: warn if scu mapping fails

[Nicholas Mc Guire]

 If the device node is not found or the of_iomap() failed, calling
scu_enable would not be safe and could lead to an undefined system
state. So warn in both failure paths before returning.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Link: http://lkml.org/lkml/2018/7/16/219
Fixes: commit 65ebcc115889 ("ARM: sti: Add STiH415 SOC support")
---

V2: replacement of WARN_ON() by a meaningful pr_err as suggested by
    Patrice CHOTARD <patrice.chotard@st.com>.

Problem was found by an experimental coccinelle script

Patch was compile tested with: multi_v7_defconfig (implies
CONFIG_ARCH_STI=y, CONFIG_SMP=y)
(with one sparse warning unrelated to the proposed change)

Patch is against 4.18-rc4 (localversion-next is next-20180716)

 arch/arm/mach-sti/platsmp.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-sti/platsmp.c b/arch/arm/mach-sti/platsmp.c
index 231f19e..530e9a5 100644
--- a/arch/arm/mach-sti/platsmp.c
+++ b/arch/arm/mach-sti/platsmp.c
@@ -107,12 +107,22 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus)
 
 	np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
 
-	if (np) {
-		scu_base = of_iomap(np, 0);
-		scu_enable(scu_base);
-		of_node_put(np);
+	if (!np) {
+		pr_err("SCU not found at %s:%s():%d\n",
+		       __FILE__, __func__, __LINE__);
+		return;
 	}
 
+	scu_base = of_iomap(np, 0);
+	of_node_put(np);
+	if (!scu_base) {
+		pr_err("SCU remap failed at %s:%s():%d\n",
+		       __FILE__, __func__, __LINE__);
+		return;
+	}
+
+	scu_enable(scu_base);
+
 	if (max_cpus <= 1)
 		return;
 
-- 
2.1.4

[PATCH] ARM: u300: add missing check for kmalloc

[Nicholas Mc Guire]

 kmalloc return for bigrxbuf_virtual was not being checked - in case
of failure set status, cleanup bigtxbuf_virtual and baile out.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Fixes: c7c8c78fdf6e ("ARM: 5667/3: U300 SSP/SPI board setup and test")
---

Issue found with experimental coccinelle script

Not sure about the checkpatch message: 
CHECK: Comparison to NULL could be written "!bigrxbuf_virtual"
#32: FILE: arch/arm/mach-u300/dummyspichip.c:67:
+       if (bigrxbuf_virtual == NULL) {
As the current check for bigtxbuf_virtual uses == NULL that 
was retained for consistency here.

Patch was compile tested with: u300_defconfig (implies MACH_U300_SPIDUMMY=y)

Patch is against 4.18 (localversion-next is next-20180824)

 arch/arm/mach-u300/dummyspichip.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/mach-u300/dummyspichip.c b/arch/arm/mach-u300/dummyspichip.c
index 68fe986..ff293ee 100644
--- a/arch/arm/mach-u300/dummyspichip.c
+++ b/arch/arm/mach-u300/dummyspichip.c
@@ -62,7 +62,13 @@ static ssize_t dummy_looptest(struct device *dev,
 		status = -ENOMEM;
 		goto out;
 	}
+
 	bigrxbuf_virtual = kmalloc(DMA_TEST_SIZE, GFP_KERNEL);
+	if (bigrxbuf_virtual == NULL) {
+		kfree(bigtxbuf_virtual);
+		status = -ENOMEM;
+		goto out;
+	}
 
 	/* Fill TXBUF with some happy pattern */
 	memset(bigtxbuf_virtual, 0xAA, DMA_TEST_SIZE);
-- 
2.1.4

Re: [PATCH] ARM: u300: add missing check for kmalloc

[Linus Walleij]

Hi Nicholas,

thanks for your patch!

On Sat, Apr 13, 2019 at 9:20 AM Nicholas Mc Guire <hofrat@osadl.org> wrote:

>  kmalloc return for bigrxbuf_virtual was not being checked - in case
> of failure set status, cleanup bigtxbuf_virtual and baile out.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: c7c8c78fdf6e ("ARM: 5667/3: U300 SSP/SPI board setup and test")

This code is deleted from the kernel and the code had
zero users before that, so no need to fix it! Sorry for your
trouble!

Yours,
Linus Walleij

[PATCH] ARM: ux500: add missing of_node_put()

[Nicholas Mc Guire]

 of_find_compatible_node() returns a pointer with refcount incremented
so both in the error path as well as after usage in soc_info_populate()
respectively actually b8500_read_soc_id() an explicit of_node_put is
needed to release backupram.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Fixes: commit 18a992787896 ("ARM: ux500: move soc_id driver to drivers/soc")
---

Problem located with experimental cocinelle script

get_maintainer.pl only returns linux-kernel@vger.kernel.org for
this file ? Is MAINTAINERS entry missing ?

Not really sure about the referenced fixes commit 18a992787896
("ARM: ux500: move soc_id driver to drivers/soc") the commit log notes
only that the driver is being moved and not expected to change (v4.8)
but looking at the previous version in v4.7 it does seem that while
moving the driver there was also a relevant change to the driver code
including the switch to using of_find_compatible_node().

Patch was compiletested with: u8500_defconfig (implies
ONFIG_UX500_SOC_ID=y)

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

 drivers/soc/ux500/ux500-soc-id.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/ux500/ux500-soc-id.c b/drivers/soc/ux500/ux500-soc-id.c
index 6c1be74..e22597d 100644
--- a/drivers/soc/ux500/ux500-soc-id.c
+++ b/drivers/soc/ux500/ux500-soc-id.c
@@ -203,10 +203,13 @@ static int __init ux500_soc_device_init(void)
 	ux500_setup_id();
 
 	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
-	if (!soc_dev_attr)
+	if (!soc_dev_attr) {
+		of_node_put(backupram);
 		return -ENOMEM;
+	}
 
 	soc_info_populate(soc_dev_attr, backupram);
+	of_node_put(backupram);
 
 	soc_dev = soc_device_register(soc_dev_attr);
 	if (IS_ERR(soc_dev)) {
-- 
2.1.4

Re: [PATCH] ARM: ux500: add missing of_node_put()

[Ulf Hansson]

On Sat, 13 Apr 2019 at 09:20, Nicholas Mc Guire <hofrat@osadl.org> wrote:
>
>  of_find_compatible_node() returns a pointer with refcount incremented
> so both in the error path as well as after usage in soc_info_populate()
> respectively actually b8500_read_soc_id() an explicit of_node_put is
> needed to release backupram.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: commit 18a992787896 ("ARM: ux500: move soc_id driver to drivers/soc")

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

> ---
>
> Problem located with experimental cocinelle script
>
> get_maintainer.pl only returns linux-kernel@vger.kernel.org for
> this file ? Is MAINTAINERS entry missing ?

drivers/soc/ux500 should be added to the ARM/NOMADIK/U300/Ux500
ARCHITECTURES section, which is maintained by Linus Walleij.

If you send a patch, I am sure Linus will ack it.

>
> Not really sure about the referenced fixes commit 18a992787896
> ("ARM: ux500: move soc_id driver to drivers/soc") the commit log notes
> only that the driver is being moved and not expected to change (v4.8)
> but looking at the previous version in v4.7 it does seem that while
> moving the driver there was also a relevant change to the driver code
> including the switch to using of_find_compatible_node().
>
> Patch was compiletested with: u8500_defconfig (implies
> ONFIG_UX500_SOC_ID=y)
>
> Patch is against 4.18-rc3 (localversion-next is next-20180706)
>
>  drivers/soc/ux500/ux500-soc-id.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/ux500/ux500-soc-id.c b/drivers/soc/ux500/ux500-soc-id.c
> index 6c1be74..e22597d 100644
> --- a/drivers/soc/ux500/ux500-soc-id.c
> +++ b/drivers/soc/ux500/ux500-soc-id.c
> @@ -203,10 +203,13 @@ static int __init ux500_soc_device_init(void)
>         ux500_setup_id();
>
>         soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> -       if (!soc_dev_attr)
> +       if (!soc_dev_attr) {
> +               of_node_put(backupram);
>                 return -ENOMEM;
> +       }
>
>         soc_info_populate(soc_dev_attr, backupram);
> +       of_node_put(backupram);
>
>         soc_dev = soc_device_register(soc_dev_attr);
>         if (IS_ERR(soc_dev)) {
> --
> 2.1.4
>

Re: [PATCH] ARM: ux500: add missing of_node_put()

[Linus Walleij]

On Sat, Apr 13, 2019 at 9:20 AM Nicholas Mc Guire <hofrat@osadl.org> wrote:

>  of_find_compatible_node() returns a pointer with refcount incremented
> so both in the error path as well as after usage in soc_info_populate()
> respectively actually b8500_read_soc_id() an explicit of_node_put is
> needed to release backupram.
>
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: commit 18a992787896 ("ARM: ux500: move soc_id driver to drivers/soc")

Patch applied to my Ux500 tree with Ulf's ACK.

Yours,
Linus Walleij

[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

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

[Markus Elfring]

> +++ b/arch/arm/mach-hisi/hotplug.c
> @@ -173,11 +173,15 @@  static bool hix5hd2_hotplug_init(void)
…
> +	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)

Would the use of a ternary operator be more succinct here?

+	return ctrl_base ? true : false;

Regards,
Markus

[PATCH 2/4] ARM: imx legacy: declare unsigned int rather than unsigned

[Nicholas Mc Guire]

provide the proper type for unsigned int.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Problem reported by checkpatch

Patch was compile-tested with: imx_v4_v5_defconfig (implies
CONFIG_MACH_MX27ADS=y)

Patch is against 5.1-rc4 (localversion-next is 20190412)

 arch/arm/mach-imx/mach-mx27ads.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/mach-mx27ads.c b/arch/arm/mach-imx/mach-mx27ads.c
index 528846f..49ac898 100644
--- a/arch/arm/mach-imx/mach-mx27ads.c
+++ b/arch/arm/mach-imx/mach-mx27ads.c
@@ -202,7 +202,7 @@ static const struct imxi2c_platform_data mx27ads_i2c1_data __initconst = {
 static struct i2c_board_info mx27ads_i2c_devices[] = {
 };
 
-static void vgpio_set(struct gpio_chip *chip, unsigned offset, int value)
+static void vgpio_set(struct gpio_chip *chip, unsigned int offset, int value)
 {
 	if (value)
 		imx_writew(PBC_BCTRL1_LCDON, PBC_BCTRL1_SET_REG);
@@ -210,7 +210,7 @@ static void vgpio_set(struct gpio_chip *chip, unsigned offset, int value)
 		imx_writew(PBC_BCTRL1_LCDON, PBC_BCTRL1_CLEAR_REG);
 }
 
-static int vgpio_dir_out(struct gpio_chip *chip, unsigned offset, int value)
+static int vgpio_dir_out(struct gpio_chip *chip, unsigned int offset, int value)
 {
 	return 0;
 }
-- 
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

[PATCH 3/4 RFC] ARM: imx legacy: wrap complex macro in ()

[Nicholas Mc Guire]

Checkpatch suggests to place a parenthesis around this complex macro.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---

Problem reported by checkpatch

I'm actually not sure this really is improving readability but by
default checkpatch gets it right so...

Patch was compile-tested with: imx_v4_v5_defconfig (implies
CONFIG_MACH_MX27ADS=y)

Patch is against 5.1-rc4 (localversion-next is 20190412)

 arch/arm/mach-imx/mach-mx27ads.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-imx/mach-mx27ads.c b/arch/arm/mach-imx/mach-mx27ads.c
index 49ac898..c83fdd3 100644
--- a/arch/arm/mach-imx/mach-mx27ads.c
+++ b/arch/arm/mach-imx/mach-mx27ads.c
@@ -42,8 +42,8 @@
  * Base address of PBC controller, CS4
  */
 #define PBC_BASE_ADDRESS        0xf4300000
-#define PBC_REG_ADDR(offset)    (void __force __iomem *) \
-		(PBC_BASE_ADDRESS + (offset))
+#define PBC_REG_ADDR(offset)    ((void __force __iomem *) \
+		(PBC_BASE_ADDRESS + (offset)))
 
 /* When the PBC address connection is fixed in h/w, defined as 1 */
 #define PBC_ADDR_SH             0
-- 
2.1.4

[PATCH 4/4] ARM: imx legacy: add an SPDX license identifier

[Nicholas Mc Guire]

The header clearly identifies this code as GPL V2 or later - so pop
in the SPDX license identifier.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
---
 arch/arm/mach-imx/mach-mx27ads.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-imx/mach-mx27ads.c b/arch/arm/mach-imx/mach-mx27ads.c
index c83fdd3..3f68972 100644
--- a/arch/arm/mach-imx/mach-mx27ads.c
+++ b/arch/arm/mach-imx/mach-mx27ads.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0+
 /*
  *  Copyright (C) 2000 Deep Blue Solutions Ltd
  *  Copyright (C) 2002 Shane Nay (shane@minirl.com)
-- 
2.1.4

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

[Joe Perches]

On Sat, 2019-04-13 at 09:14 +0200, Nicholas Mc Guire wrote:
> 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,
> };

Yeah, it's a false positive.

You could use

static const __initconst etc...

if you really wanted to avoid this but feel free
to ignore checkpatch anyway when it's dumb.

[PATCH] ARM: STi: at least warn if of_iomap fails

[Nicholas Mc Guire]

The call to of_iomap() is unchecked but scu_enable(), which the returned
address is passed on to, assumes a valid mapping. If the mapping is
invalid this could probably lead to undefined system state so at least
a warning should be issued.

Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
Fixes: commit 65ebcc115889 ("ARM: sti: Add STiH415 SOC support")
---
Problem was found by an experimental coccinelle script

Patch was compile tested with: multi_v7_defconfig (implies
CONFIG_ARCH_STI=y, CONFIG_SMP=y)

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

 arch/arm/mach-sti/platsmp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-sti/platsmp.c b/arch/arm/mach-sti/platsmp.c
index 231f19e..89ae76f 100644
--- a/arch/arm/mach-sti/platsmp.c
+++ b/arch/arm/mach-sti/platsmp.c
@@ -109,6 +109,7 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus)
 
 	if (np) {
 		scu_base = of_iomap(np, 0);
+		WARN_ON(!scu_base);
 		scu_enable(scu_base);
 		of_node_put(np);
 	}
-- 
2.1.4

Re: [PATCH] ARM: STi: at least warn if of_iomap fails

[Patrice CHOTARD]

Hi Nicholas

On 07/12/2018 05:48 PM, Nicholas Mc Guire wrote:
> The call to of_iomap() is unchecked but scu_enable(), which the returned
> address is passed on to, assumes a valid mapping. If the mapping is
> invalid this could probably lead to undefined system state so at least
> a warning should be issued.
> 
> Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> Fixes: commit 65ebcc115889 ("ARM: sti: Add STiH415 SOC support")
> ---
> Problem was found by an experimental coccinelle script
> 
> Patch was compile tested with: multi_v7_defconfig (implies
> CONFIG_ARCH_STI=y, CONFIG_SMP=y)
> 
> Patch is against 4.18-rc3 (localversion-next is next-20180712)
> 
>   arch/arm/mach-sti/platsmp.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/mach-sti/platsmp.c b/arch/arm/mach-sti/platsmp.c
> index 231f19e..89ae76f 100644
> --- a/arch/arm/mach-sti/platsmp.c
> +++ b/arch/arm/mach-sti/platsmp.c
> @@ -109,6 +109,7 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus)
>   
>   	if (np) {
>   		scu_base = of_iomap(np, 0);
> +		WARN_ON(!scu_base);
>   		scu_enable(scu_base);
>   		of_node_put(np);
>   	}
> 

I should prefer to exit with an error to avoid unpredictable behavior,
something like this is better :

scu_base = of_iomap(np, 0);
of_node_put(np);
if (!scu_base) {
	pr_err("No SCU remap\n");
	return;
}

scu_enable(scu_base);


Thanks

Re: [PATCH] ARM: STi: at least warn if of_iomap fails

[Nicholas Mc Guire]

On Mon, Jul 16, 2018 at 07:55:08AM +0000, Patrice CHOTARD wrote:
> Hi Nicholas
> 
> On 07/12/2018 05:48 PM, Nicholas Mc Guire wrote:
> > The call to of_iomap() is unchecked but scu_enable(), which the returned
> > address is passed on to, assumes a valid mapping. If the mapping is
> > invalid this could probably lead to undefined system state so at least
> > a warning should be issued.
> > 
> > Signed-off-by: Nicholas Mc Guire <hofrat@osadl.org>
> > Fixes: commit 65ebcc115889 ("ARM: sti: Add STiH415 SOC support")
> > ---
> > Problem was found by an experimental coccinelle script
> > 
> > Patch was compile tested with: multi_v7_defconfig (implies
> > CONFIG_ARCH_STI=y, CONFIG_SMP=y)
> > 
> > Patch is against 4.18-rc3 (localversion-next is next-20180712)
> > 
> >   arch/arm/mach-sti/platsmp.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/mach-sti/platsmp.c b/arch/arm/mach-sti/platsmp.c
> > index 231f19e..89ae76f 100644
> > --- a/arch/arm/mach-sti/platsmp.c
> > +++ b/arch/arm/mach-sti/platsmp.c
> > @@ -109,6 +109,7 @@ static void __init sti_smp_prepare_cpus(unsigned int max_cpus)
> >   
> >   	if (np) {
> >   		scu_base = of_iomap(np, 0);
> > +		WARN_ON(!scu_base);
> >   		scu_enable(scu_base);
> >   		of_node_put(np);
> >   	}
> > 
> 
> I should prefer to exit with an error to avoid unpredictable behavior,
> something like this is better :
> 
> scu_base = of_iomap(np, 0);
> of_node_put(np);
> if (!scu_base) {
> 	pr_err("No SCU remap\n");
> 	return;
> }
> 
> scu_enable(scu_base);
>
even though it would be possible to locate that then from that message
would it not be prefereable to provide the infos upfront like:

	pr_err("SCU remap failed at %s:%s():%d\n",
               __FILE__, __func__, __LINE__);

if that makes sense I´ll resend - or is that considered too verbouse ?

thx!
hofrat