linux-kernel - Re: [PATCH v3] i2c: imx-lpi2c: add error message when i2c peripheral clk doesn't work

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230725064455.i5oq5y2jvjatxesc@pengutronix.de>
Date:   Tue, 25 Jul 2023 08:44:55 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     carlos.song@....com
Cc:     festevam@...il.com, andi.shyti@...nel.org, aisheng.dong@....com,
        shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de,
        haibo.chen@....com, linux-kernel@...r.kernel.org,
        xiaoning.wang@....com, linux-i2c@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-imx@....com
Subject: Re: [PATCH v3] i2c: imx-lpi2c: add error message when i2c peripheral
 clk doesn't work

On Tue, Jul 25, 2023 at 10:11:08AM +0800, carlos.song@....com wrote:
> From: Gao Pan <pandy.gao@....com>
> 
> Output error log when i2c peripheral clk rate is 0, then
> directly return -EINVAL.
> 
> Fixes: a55fa9d0e42e ("i2c: imx-lpi2c: add low power i2c bus driver")
> Signed-off-by: Gao Pan <pandy.gao@....com>
> Signed-off-by: Carlos Song <carlos.song@....com>
> ---
> Changes for V2:
> - adjust the Subject from "debug message" to "error message"
> ---
>  drivers/i2c/busses/i2c-imx-lpi2c.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index e93ff3b5373c..12b4f2a89343 100644

I wonder which tree you based this on. The object e93ff3b5373c is
neither in v6.5-rc1 nor next nor in Wolfram's tree. (Probably not
critical for this patch. But if you base your patch on a public tree,
you're making things easier for the build bots. Related to that I
recommend git format-patch --base=...)

> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -214,6 +214,11 @@ static int lpi2c_imx_config(struct lpi2c_imx_struct *lpi2c_imx)
>  	lpi2c_imx_set_mode(lpi2c_imx);
>  
>  	clk_rate = clk_get_rate(lpi2c_imx->clks[0].clk);
> +	if (!clk_rate) {
> +		dev_err(&lpi2c_imx->adapter.dev, "clk_per rate is 0\n");
> +		return -EINVAL;
> +	}

I'm not sure this is a good idea. In my book error messages in functions
that might be called often (and triggered by user space) do more harm
than good as they flood the kernel log buffer and so might make it hard
to find the original cause of the problem. I'd say returning an error
code should be enough here.

Having said that I wonder if you're fixing a real issue? (i.e. did it
happen to you that clk_rate was 0? Did this result in a div-by-zero? Or
a hardware hang? Or a too fast (or too slow) bus speed?) Or is this just
addressing a warning by some static checker?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)