linux-kernel - Re: [PATCH] Input: elants_i2c: Properly handle the reset GPIO when power is off

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]

Date:   Thu, 17 Nov 2022 22:13:00 -0800
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Benson Leung <bleung@...omium.org>,
        Johnny Chuang <johnny.chuang.emc@...il.com>,
        Scott Liu <scott.liu@....com.tw>, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Input: elants_i2c: Properly handle the reset GPIO when
 power is off

On Thu, Nov 17, 2022 at 03:48:17PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Thu, Nov 17, 2022 at 2:13 PM Dmitry Torokhov
> <dmitry.torokhov@...il.com> wrote:
> >
> > On Thu, Nov 17, 2022 at 12:38:23PM -0800, Douglas Anderson wrote:
> > > As can be seen in elants_i2c_power_off(), we want the reset GPIO
> > > asserted when power is off. The reset GPIO is active low so we need
> > > the reset line logic low when power is off to avoid leakage.
> > >
> > > We have a problem, though, at probe time. At probe time we haven't
> > > powered the regulators on yet but we have:
> > >   devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_LOW);
> > >
> > > While that _looks_ right, it turns out that it's not. The
> > > GPIOD_OUT_LOW doesn't mean to init the GPIO to low. It means init the
> > > GPIO to "not asserted". Since this is an active low GPIO that inits it
> > > to be high.
> > >
> > > Let's fix this to properly init the GPIO. Now after both probe and
> > > power off the state of the GPIO is consistent (it's "asserted" or
> > > level low).
> > >
> > > Once we fix this, we can see that at power on time we no longer to
> > > assert the reset GPIO as the first thing. The reset GPIO is _always_
> > > asserted before powering on. Let's fix powering on to account for
> > > this.
> >
> > I kind of like that elants_i2c_power_on() is self-contained and does the
> > full power sequence. Can we simply change devm_gpiod_get() to use
> > GPIOD_ASIS to avoid the momentary spike in reset line state (assuming
> > that the firmware initializes the reset line sanely because if it does
> > not we have much longer time where we are leaking into the controller)?
> 
> I'm not sure I see the benefit of elants_i2c_power_on() initting the
> reset GPIO. In general that function _has_ to make assumptions about
> the state of the world before it's called. Otherwise the function
> should start:
> 
> if (ts->did_I_inexplicably_turn_vcc33_on) {
>   regulator_disable(ts->vcc33);
>   ts->did_I_inexplicably_turn_vcc33_on = false;
> }
> 
> if (ts->did_I_inexplicably_turn_vccio_on) {
>   regulator_disable(ts->vccio);
>   ts->did_I_inexplicably_turn_vccio_on = false;
> }
> 
> Said another way: we already need to rely on the regulators being in a
> reasonable state when the function starts.  Why is that different from
> relying on the reset GPIO being in a reasonable state? The reset GPIO
> needs to be sequenced together with the regulators. It should always
> be "asserted" (driven low) when the regulators are off and only ever
> deasserted (driven high) when the regulators are on.
> 
> I'll also note that, as coded today (without my patch), the
> elants_i2c_power_on() is actively doing the _wrong_ thing in its error
> handling. Specifically if either of the regulators fail to turn on it
> will explicitly de-assert the reset again which, since it's active
> low, will set the GPIO to high and start leaking power / backdriving
> the touchscreen. We could remove this bit of error handling but then
> we're suddenly not undoing the things that the function did. ;-) It
> feels cleaner to me to just make it a requirement that the reset GPIO
> is always asserted (low) when the regulators are off.

OK, fair enough, applied.

Thanks.

-- 
Dmitry