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 <jiash...@iscas.ac.cn>
> ---
> 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);
