linux-kernel - Re: [PATCH 04/12] iio: adc: ad9467: fix reset gpio handling

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: <3925cb4b6453644c889675c20329b3477a06fcd5.camel@gmail.com>
Date:   Sat, 02 Dec 2023 09:36:47 +0100
From:   Nuno Sá <noname.nuno@...il.com>
To:     David Lechner <dlechner@...libre.com>
Cc:     nuno.sa@...log.com, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
        Olivier MOYSAN <olivier.moysan@...s.st.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>
Subject: Re: [PATCH 04/12] iio: adc: ad9467: fix reset gpio handling

On Fri, 2023-12-01 at 11:01 -0600, David Lechner wrote:
> On Fri, Dec 1, 2023 at 2:47 AM Nuno Sá <noname.nuno@...il.com> wrote:
> > 
> > On Thu, 2023-11-30 at 15:41 -0600, David Lechner wrote:
> > > On Tue, Nov 21, 2023 at 4:17 AM Nuno Sa via B4 Relay
> > > <devnull+nuno.sa.analog.com@...nel.org> wrote:
> > > > 
> > > > From: Nuno Sa <nuno.sa@...log.com>
> > > > 
> > > > The reset gpio was being requested with GPIOD_OUT_LOW which means, not
> > > > asserted. Then it was being asserted but never de-asserted which means
> > > > the devices was left in reset. Fix it by de-asserting the gpio.
> > > 
> > > It could be helpful to update the devicetree bindings to state the
> > > expected active-high or active-low setting for this gpio so it is
> > > clear which state means asserted.
> > > 
> > 
> > You could state that the chip is active low but I don't see that change that
> > important for now. Not sure if this is clear and maybe that's why your comment.
> > GPIOD_OUT_HIGH has nothing to do with active high or low. It just means, "get me
> > the
> > pin in the asserted state".
> > 
> 
> I would assume that this bug happened in the first place because
> someone forgot GPIOD_OUT_LOW in the devicetree when they were
> developing the driver. So this is why I suggested that updating the
> devicetree binding docs so that future users are less likely to make
> the same mistake. Currently, the bindings don't even have reset-gpios
> in the examples.

Hmm, I think you're missing the point... The bug has nothing to do with devicetree.
This is what was happening:

1) We were calling devm_gpiod_get_optional() with GPIOD_OUT_LOW. What this means is
that you get an output gpio deasserted. Hence the device is out of reset. And here is
the important part... what you have in dts does not matter. If you have active low,
it means the pin level will be 1. If you have high, the pin level is 0. And this is
all handled by gpiolib for you.

2) Then, we called gpiod_direction_output(..., 1), which means set the direction out
(which is actually not needed since it was already done when getting the pin) and
assert the pin. Hence, reset the device. And we were never de-asserting the pin so
the device would be left in reset.

- Nuno Sá