linux-kernel - Re: [PATCH 1/2] dt-bindings: input: goodix: Add powered-in-suspend property

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:   Tue, 25 Apr 2023 16:34:16 +0800
From:   Fei Shao <fshao@...omium.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Jeff LaBundy <jeff@...undy.com>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        linux-mediatek <linux-mediatek@...ts.infradead.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        devicetree@...r.kernel.org, linux-input@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] dt-bindings: input: goodix: Add powered-in-suspend property

On Tue, Apr 25, 2023 at 2:15 AM Doug Anderson <dianders@...omium.org> wrote:
>
> Hi,
>
> On Sun, Apr 23, 2023 at 8:31 PM Jeff LaBundy <jeff@...undy.com> wrote:
> >
> > Back-powering can come in two forms:
> >
> > 1. The one you've described, which is by far the most common. As you mention,
> > it is not the case here since the issue happens only when we drive the GPIO
> > low and not high.
> >
> > 2. Through a forbidden power supply sequence, an input pin of an IC with
> > multiple power supplies becomes a weak power supply itself. Grounding the
> > GPIO then sinks current into the SoC.
> >
> > This problem smelled like (2), especially since asserting the GPIO or powering
> > down the supply alleviates the symptom. Most Goodix controllers I've worked
> > with have two or more supplies, and the datasheet requires them to be enabled
> > or disabled in a specific order.
> >
> > Based on Fei's feedback, however, this IC does not seem to be one of those
> > since there is reportedly only a single rail. I guess either vdd or vddio is
> > tied to a dummy regulator? If so, then perhaps we can scratch this theory.
>
> There is one rail provided from the main board, but there can be two
> voltage levels involved depending on stuffings on the touchscreen
> itself. I talked a bit about this in commit 1d18c1f3b7d9
> ("dt-bindings: HID: i2c-hid: goodix: Add mainboard-vddio-supply").
>
>
> > Fair enough, I was simply sketpical that this was the _right_ workaround.
> > Were this an issue of only supply A left on yet the IC datasheet requires
> > supply B to remain on if supply A is on, I would rather see this solved by
> > updating a regulator dts node, trusted FW sequence, etc.
>
> Right. That type of stuff was looked at in detail by two separate
> teams, so I don't think this is the issue.
>
>
> > Thanks for your due diligence; if this really is a silicon issue, then
> > the driver should indeed accommodate it.
> >
> > That being said, the name 'powered-in-suspend' seems a bit conflated. We
> > should not be duplicating regulator policy in this driver; the existing
> > naming also may cause confusion for other HW configurations that do leave
> > vdd and vddio on during suspend, but don't have this problem.
> >
> > Here, we actually mean to control the behavior of the reset GPIO through
> > suspend and therefore something like 'goodix,no-reset-during-suspend' is
> > closer to what I understand us to intend to do. I will add more details
> > in patch [2/2].
>
> I'd be fine with that name. ...ah, and adding the "goodix," prefix is
> a good call.
>
> -Doug

That sounds good to me, too. I'll update the name in the next patch.

Regards,
Fei