Re: [PATCH] soc: fsl: qe: Add check for ioremap

2022-10-09 Thread Christophe Leroy 


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

Re: [PATCH] soc: fsl: qe: Add check for ioremap

2022-10-08 Thread kernel test robot 
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 

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; \
   ^~
   :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; \
   ^~
   :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; \
   ^~
   :31:1:

[PATCH] soc: fsl: qe: Add check for ioremap

2022-10-07 Thread Jiasheng Jiang 
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 
---
 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