linux-kernel - Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API

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]

Date:   Mon, 22 Mar 2021 08:58:35 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Sven Van Asbroeck <thesven73@...il.com>
Cc:     Clemens Gruber <clemens.gruber@...ruber.com>,
        linux-pwm@...r.kernel.org,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, Lee Jones <lee.jones@...aro.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>,
        David Jander <david@...tonic.nl>
Subject: Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API

On Thu, Dec 17, 2020 at 12:10:10PM -0500, Sven Van Asbroeck wrote:
> On Thu, Dec 17, 2020 at 11:48 AM Clemens Gruber
> <clemens.gruber@...ruber.com> wrote:
> >
> > I can initialize the values to 0 of course and check the file for other
> > places with missing initializations.
> >
> > Or would it be better to check the return codes of regmap_read/write in
> > such cases? I'm not sure.
> 
> I think that checking the regmap_read/write return values is overkill
> in this driver. These functions can't realistically fail, except if the i2c
> bus is bad, i.e. h/w failure or intermittency. And that's an externality
> which I believe we can ignore.

I think there are (rare) occasions where it's fine to not check for
errors, i.e. if you definitively know that calls can't fail. However,
given that this uses regmap and you don't really know what's backing
this, I think it's always better to err on the side of caution and
properly check the return values.

The fact that this can be externally caused is actually a reason why
we shouldn't be ignoring any errors. If there's a chip that's hogging
the I2C bus or if you've even just mistyped the I2C client's address
in DT, it's better if the PWM driver tells you with an error message
than if it is silently ignoring the errors and keeps you guessing at
why the PWM isn't behaving the way it should.

Granted, the error code isn't always going to pinpoint exactly what's
going wrong, but for serious errors often the I2C bus driver will let
you know with an extra error message. However, it's much easier to go
looking for that error message if the PWM driver lets you know that
something went wrong.

Please just add full checking of regmap operations.

Thierry

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