linux-kernel - Re: [PATCH v2 3/3] i2c: rk3x: Add check for clk

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: <fj2vssllh7zgl76a4jfakrcngcflwvqxzwwa2dhpkauli2u7wb@pz23fznmy25e>
Date: Thu, 7 Nov 2024 00:01:12 +0100
From: Andi Shyti <andi.shyti@...nel.org>
To: Doug Anderson <dianders@...omium.org>
Cc: Jiasheng Jiang <jiashengjiangcool@...il.com>, 
	rmk@...-67.arm.linux.org.uk, max.schwarz@...ine.de, david.wu@...k-chips.com, heiko@...ech.de, 
	vz@...ia.com, wsa@...nel.org, manabian@...il.com, 
	linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org, linux-i2c@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] i2c: rk3x: Add check for clk_enable()

Hi Doug,

On Tue, Nov 05, 2024 at 01:29:40PM -0800, Doug Anderson wrote:
> On Tue, Nov 5, 2024 at 8:18 AM Jiasheng Jiang
> > Add check for the return value of clk_enable() in order to catch the
> > potential exception. Moreover, convert the return type of
> > rk3x_i2c_adapt_div() into int and add the check.
> >
> > Signed-off-by: Jiasheng Jiang <jiashengjiangcool@...il.com>
> > ---
> > Changelog:
> >
> > v1 -> v2:
> >
> > 1. Remove the Fixes tag.
> > 2. Use dev_err_probe to simplify error handling.
> > ---
> >  drivers/i2c/busses/i2c-rk3x.c | 51 +++++++++++++++++++++++++----------
> >  1 file changed, 37 insertions(+), 14 deletions(-)
> 
> Wow, this is a whole lot of code to add to check for an error that
> can't really happen as far as I'm aware. Turning on a clock is just
> some MMIO writes and can't fail, right? Is this really worth it? Maybe
> just wrap clk_enable() and spam an error to the logs if it fails? If
> we ever see that error we can figure out what's going on and if
> there's a sensible reason it could fail we could add the more complex
> code.

We've had this discussion several times. I'm of the school that
if a function can return an error, that error should be checked.
It's not spam, we do it everywhere.

On the other hand, there is another school, bigger than mine,
that doesn't want such a check.

I decided not to bother. If someone adds the check, I'm going to
accept it. If someone doesn't add the check, I won't bother
asking for it.

Said that, MMIO reads and writes can fail: in other drivers I'm
seeing bogus reads due to some firmware failures during device
reset.

I agree with you with the rest of the comments and thanks for
checking this out.

Andi