linux-kernel - Re: [PATCH] pwm: rockchip: Convert to use dev_err

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <27846879-6660-26b3-5839-997b92cda6d2@wanadoo.fr>
Date:   Wed, 17 Aug 2022 08:43:26 +0200
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     zhaoxiao <zhaoxiao@...ontech.com>, thierry.reding@...il.com,
        heiko@...ech.de
Cc:     u.kleine-koenig@...gutronix.de, linux-pwm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] pwm: rockchip: Convert to use dev_err_probe()

Le 17/08/2022 à 07:59, zhaoxiao a écrit :
> It's fine to call dev_err_probe() in ->probe() when error code is known.
> Convert the driver to use dev_err_probe().
> 
> Signed-off-by: zhaoxiao <zhaoxiao@...ontech.com>
> ---
>   drivers/pwm/pwm-rockchip.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index f3647b317152..bd7ab37aaf88 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -331,15 +331,12 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>   	if (IS_ERR(pc->pclk)) {
>   		ret = PTR_ERR(pc->pclk);
>   		if (ret != -EPROBE_DEFER)
> -			dev_err(&pdev->dev, "Can't get APB clk: %d\n", ret);
> -		return ret;
> +			return dev_err_probe(&pdev->dev, ret, "Can't get APB clk: %d\n");

Hi,

Previously, if (ret != -EPROBE_DEFER), we were returning. Now we just 
ignore it and continue.
If done on purpose, you should explain why in the commit log.

My guess is that the test should also be removed. dev_err_probe() 
handles that for us.

You could use PTR_ERR(pc->pclk) directly. There is no need to assign it 
to ret. This would simplify even further the code.

You removed the last 'ret' parameter, which is fine, but left the %d in 
the message.
Compiling trigger the issue.
Please, ALWAYS, at least compile test your changes, even when things 
look obvious.

Look at the code around line 316 to see how it is done.

>   	}
>   
>   	ret = clk_prepare_enable(pc->clk);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Can't prepare enable PWM clk: %d\n", ret);
> -		return ret;
> -	}
> +	if (ret)
> +		dev_err_probe(&pdev->dev, ret, "Can't prepare enable PWM clk: %d\n");

'return' before dev_err_probe() missing?

You removed the last 'ret' parameter, but left the %d in the message.
(same comment about compiling as above)

>   
>   	ret = clk_prepare_enable(pc->pclk);
>   	if (ret) {

Why just converting 2 dev_err() and leaving the other one in the probe 
untouched?

CJ