As ioremap can return NULL pointer, it should
be better to check the return value return error
if fails.
Moreover, the return value of qe_reset should be
checked by cascade.
Fixes: 68f047e3d62e ("fsl/qe: add rx_sync and tx_sync for TDM mode")
Signed-off-by: Jiasheng Jiang <[email protected]>
---
drivers/soc/fsl/qe/qe.c | 23 ++++++++++++++++++-----
include/soc/fsl/qe/qe.h | 4 ++--
2 files changed, 20 insertions(+), 7 deletions(-)
diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
index b3c226eb5292..88e335e8eef7 100644
--- a/drivers/soc/fsl/qe/qe.c
+++ b/drivers/soc/fsl/qe/qe.c
@@ -83,10 +83,13 @@ static phys_addr_t get_qe_base(void)
return qebase;
}
-void qe_reset(void)
+int qe_reset(void)
{
- if (qe_immr == NULL)
+ if (qe_immr == NULL) {
qe_immr = ioremap(get_qe_base(), QE_IMMAP_SIZE);
+ if (qe_immr == NULL)
+ return -ENOMEM;
+ }
qe_snums_init();
@@ -98,6 +101,8 @@ void qe_reset(void)
if (qe_sdma_init())
panic("sdma init failed!");
+
+ return 0;
}
int qe_issue_cmd(u32 cmd, u32 device, u8 mcn_protocol, u32 cmd_input)
@@ -640,11 +645,14 @@ EXPORT_SYMBOL(qe_get_num_of_snums);
static int __init qe_init(void)
{
struct device_node *np;
+ int ret;
np = of_find_compatible_node(NULL, NULL, "fsl,qe");
if (!np)
return -ENODEV;
- qe_reset();
+ ret = qe_reset();
+ if (ret)
+ return ret;
of_node_put(np);
return 0;
}
@@ -653,8 +661,13 @@ subsys_initcall(qe_init);
#if defined(CONFIG_SUSPEND) && defined(CONFIG_PPC_85xx)
static int qe_resume(struct platform_device *ofdev)
{
- if (!qe_alive_during_sleep())
- qe_reset();
+ int ret;
+
+ if (!qe_alive_during_sleep()) {
+ ret = qe_reset();
+ if (ret)
+ return ret;
+ }
return 0;
}
diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
index b02e9fe69146..71129b8a5807 100644
--- a/include/soc/fsl/qe/qe.h
+++ b/include/soc/fsl/qe/qe.h
@@ -84,9 +84,9 @@ extern spinlock_t cmxgcr_lock;
/* Export QE common operations */
#ifdef CONFIG_QUICC_ENGINE
-extern void qe_reset(void);
+extern int qe_reset(void);
#else
-static inline void qe_reset(void) {}
+static inline int qe_reset(void) {}
#endif
int cpm_muram_init(void);
--
2.25.1
Hi Jiasheng,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on soc/for-next]
[also build test ERROR on linus/master v6.0 next-20221007]
[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/Jiasheng-Jiang/soc-fsl-qe-Add-check-for-ioremap/20221008-104034
base: https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
config: powerpc-mpc866_ads_defconfig
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 791a7ae1ba3efd6bca96338e10ffde557ba83920)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc cross compiling tool for clang build
# apt-get install binutils-powerpc-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/cd2197ddf995fc2d10fbe8639c1ddc70e6551aeb
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Jiasheng-Jiang/soc-fsl-qe-Add-check-for-ioremap/20221008-104034
git checkout cd2197ddf995fc2d10fbe8639c1ddc70e6551aeb
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/net/ethernet/freescale/fs_enet/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
In file included from drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c:26:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:27:1: note: expanded from here
__do_insw
^
arch/powerpc/include/asm/io.h:578:56: note: expanded from macro '__do_insw'
#define __do_insw(p, b, n) readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c:26:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:29:1: note: expanded from here
__do_insl
^
arch/powerpc/include/asm/io.h:579:56: note: expanded from macro '__do_insl'
#define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c:26:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:31:1: note: expanded from here
__do_outsb
^
arch/powerpc/include/asm/io.h:580:58: note: expanded from macro '__do_outsb'
#define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c:26:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:33:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:581:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c:26:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:35:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:582:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c:49:
In file included from drivers/net/ethernet/freescale/fs_enet/fs_enet.h:16:
In file included from arch/powerpc/include/asm/cpm1.h:24:
In file included from arch/powerpc/include/asm/cpm.h:1:
In file included from include/soc/fsl/cpm.h:9:
>> include/soc/fsl/qe/qe.h:89:35: error: non-void function does not return a value [-Werror,-Wreturn-type]
static inline int qe_reset(void) {}
^
drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c:321:33: warning: variable 'fpi' set but not used [-Wunused-but-set-variable]
const struct fs_platform_info *fpi;
^
7 warnings and 1 error generated.
--
In file included from drivers/net/ethernet/freescale/fs_enet/mac-scc.c:22:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:45:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insw, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:27:1: note: expanded from here
__do_insw
^
arch/powerpc/include/asm/io.h:578:56: note: expanded from macro '__do_insw'
#define __do_insw(p, b, n) readsw((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/net/ethernet/freescale/fs_enet/mac-scc.c:22:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:47:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(insl, (unsigned long p, void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:29:1: note: expanded from here
__do_insl
^
arch/powerpc/include/asm/io.h:579:56: note: expanded from macro '__do_insl'
#define __do_insl(p, b, n) readsl((PCI_IO_ADDR)_IO_BASE+(p), (b), (n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/net/ethernet/freescale/fs_enet/mac-scc.c:22:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:49:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsb, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:31:1: note: expanded from here
__do_outsb
^
arch/powerpc/include/asm/io.h:580:58: note: expanded from macro '__do_outsb'
#define __do_outsb(p, b, n) writesb((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/net/ethernet/freescale/fs_enet/mac-scc.c:22:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:51:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsw, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:33:1: note: expanded from here
__do_outsw
^
arch/powerpc/include/asm/io.h:581:58: note: expanded from macro '__do_outsw'
#define __do_outsw(p, b, n) writesw((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/net/ethernet/freescale/fs_enet/mac-scc.c:22:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from arch/powerpc/include/asm/hardirq.h:6:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/powerpc/include/asm/io.h:640:
arch/powerpc/include/asm/io-defs.h:53:1: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
DEF_PCI_AC_NORET(outsl, (unsigned long p, const void *b, unsigned long c),
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/powerpc/include/asm/io.h:637:3: note: expanded from macro 'DEF_PCI_AC_NORET'
__do_##name al; \
^~~~~~~~~~~~~~
<scratch space>:35:1: note: expanded from here
__do_outsl
^
arch/powerpc/include/asm/io.h:582:58: note: expanded from macro '__do_outsl'
#define __do_outsl(p, b, n) writesl((PCI_IO_ADDR)_IO_BASE+(p),(b),(n))
~~~~~~~~~~~~~~~~~~~~~^
In file included from drivers/net/ethernet/freescale/fs_enet/mac-scc.c:40:
In file included from drivers/net/ethernet/freescale/fs_enet/fs_enet.h:16:
In file included from arch/powerpc/include/asm/cpm1.h:24:
In file included from arch/powerpc/include/asm/cpm.h:1:
In file included from include/soc/fsl/cpm.h:9:
>> include/soc/fsl/qe/qe.h:89:35: error: non-void function does not return a value [-Werror,-Wreturn-type]
static inline int qe_reset(void) {}
^
6 warnings and 1 error generated.
vim +89 include/soc/fsl/qe/qe.h
84
85 /* Export QE common operations */
86 #ifdef CONFIG_QUICC_ENGINE
87 extern int qe_reset(void);
88 #else
> 89 static inline int qe_reset(void) {}
90 #endif
91
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Le 08/10/2022 à 04:38, Jiasheng Jiang a écrit :
> As ioremap can return NULL pointer, it should
> be better to check the return value return error
> if fails.
> Moreover, the return value of qe_reset should be
> checked by cascade.
>
> Fixes: 68f047e3d62e ("fsl/qe: add rx_sync and tx_sync for TDM mode")
> Signed-off-by: Jiasheng Jiang <[email protected]>
> ---
> drivers/soc/fsl/qe/qe.c | 23 ++++++++++++++++++-----
> include/soc/fsl/qe/qe.h | 4 ++--
> 2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
> index b3c226eb5292..88e335e8eef7 100644
> --- a/drivers/soc/fsl/qe/qe.c
> +++ b/drivers/soc/fsl/qe/qe.c
> @@ -83,10 +83,13 @@ static phys_addr_t get_qe_base(void)
> return qebase;
> }
>
> -void qe_reset(void)
> +int qe_reset(void)
> {
> - if (qe_immr == NULL)
> + if (qe_immr == NULL) {
> qe_immr = ioremap(get_qe_base(), QE_IMMAP_SIZE);
> + if (qe_immr == NULL)
> + return -ENOMEM;
> + }
I'd prefer with the following form:
if (qe_immr == NULL)
qe_immr = ioremap(get_qe_base(), QE_IMMAP_SIZE);
if (qe_immr == NULL)
return -ENOMEM;
>
> qe_snums_init();
>
> @@ -98,6 +101,8 @@ void qe_reset(void)
>
> if (qe_sdma_init())
> panic("sdma init failed!");
> +
> + return 0;
> }
>
> int qe_issue_cmd(u32 cmd, u32 device, u8 mcn_protocol, u32 cmd_input)
> @@ -640,11 +645,14 @@ EXPORT_SYMBOL(qe_get_num_of_snums);
> static int __init qe_init(void)
> {
> struct device_node *np;
> + int ret;
>
> np = of_find_compatible_node(NULL, NULL, "fsl,qe");
> if (!np)
> return -ENODEV;
> - qe_reset();
> + ret = qe_reset();
> + if (ret)
> + return ret;
I think that if you return before the of_node_put(), you leak the
reference taken by of_find_compatible_node()
qe_reset() doesn't use np, so you can likely perform the of_node_put()
before calling qe_reset(), then you can simply do
return qe_reset();
> of_node_put(np);
> return 0;
> }
> @@ -653,8 +661,13 @@ subsys_initcall(qe_init);
> #if defined(CONFIG_SUSPEND) && defined(CONFIG_PPC_85xx)
> static int qe_resume(struct platform_device *ofdev)
> {
> - if (!qe_alive_during_sleep())
> - qe_reset();
> + int ret;
> +
> + if (!qe_alive_during_sleep()) {
> + ret = qe_reset();
> + if (ret)
> + return ret;
> + }
More complicated that needed.
You could simply do:
if (!qe_alive_during_sleep())
return qe_reset();
> return 0;
> }
>
> diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
> index b02e9fe69146..71129b8a5807 100644
> --- a/include/soc/fsl/qe/qe.h
> +++ b/include/soc/fsl/qe/qe.h
> @@ -84,9 +84,9 @@ extern spinlock_t cmxgcr_lock;
>
> /* Export QE common operations */
> #ifdef CONFIG_QUICC_ENGINE
> -extern void qe_reset(void);
> +extern int qe_reset(void);
Please taken the opportunity to remove the pointless 'extern' keyword
when changing such a prototype.
> #else
> -static inline void qe_reset(void) {}
> +static inline int qe_reset(void) {}
If qe_reset() is not void anymore, it must return something.
> #endif
>
> int cpm_muram_init(void);