linux-kernel - Re: [PATCH 2/2] arm64: dts: qcom: Fully describe fingerprint node on Herobrine

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]

Message-ID: <CAE-0n53UbZq4MqrzNLEpmJO4j1S+d-LNPqpSwu5VM0873NSyGA@mail.gmail.com>
Date:   Mon, 21 Mar 2022 19:53:45 +0100
From:   Stephen Boyd <swboyd@...omium.org>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        Alexandru M Stan <amstan@...omium.org>
Subject: Re: [PATCH 2/2] arm64: dts: qcom: Fully describe fingerprint node on Herobrine

Quoting Doug Anderson (2022-03-18 13:55:56)
> Hi,
>
> On Wed, Mar 16, 2022 at 6:06 PM Stephen Boyd <swboyd@...omium.org> wrote:
> >
> > Update the fingerprint node on Herobrine to match the fingerprint DT
> > binding. This will allow us to drive the reset and boot gpios from the
> > driver when it is re-attached after flashing. We'll also be able to boot
> > the fingerprint processor if the BIOS isn't doing it for us.
> >
> > Cc: Douglas Anderson <dianders@...omium.org>
> > Cc: Matthias Kaehlcke <mka@...omium.org>
> > Cc: Alexandru M Stan <amstan@...omium.org>
> > Signed-off-by: Stephen Boyd <swboyd@...omium.org>
> > ---
> >
> > Depends on https://lore.kernel.org/r/20220317005814.2496302-1-swboyd@chromium.org
> >
> >  arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > index 984a7faf0888..282dda78ba3f 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine.dtsi
> > @@ -396,13 +396,16 @@ ap_spi_fp: &spi9 {
> >         cs-gpios = <&tlmm 39 GPIO_ACTIVE_LOW>;
> >
> >         cros_ec_fp: ec@0 {
> > -               compatible = "google,cros-ec-spi";
> > +               compatible = "google,cros-ec-fp";
> >                 reg = <0>;
> >                 interrupt-parent = <&tlmm>;
> >                 interrupts = <61 IRQ_TYPE_LEVEL_LOW>;
> >                 pinctrl-names = "default";
> >                 pinctrl-0 = <&fp_to_ap_irq_l>, <&fp_rst_l>, <&fpmcu_boot0>;
> > +               boot0-gpios = <&tlmm 68 GPIO_ACTIVE_HIGH>;
> > +               reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
> >                 spi-max-frequency = <3000000>;
> > +               vdd-supply = <&pp3300_fp_mcu>;
>
> IMO we shouldn't specify vdd-supply here when it's a bogus regulator
> (doesn't actually control the relevant GPIO). Having device trees like
> this will make it hard to transition to the kernel controlling this
> GPIO in the future because the cros-ec-fp driver won't know whether
> it's controlling the GPIO or not. So my vote would be either:
>
> 1. Go whole hog and have the kernel in charge of the regulator,
> exposing regulator control to the userspace updater through some sort
> of GPIO
>
> - or -
>
> 2. Make the "vdd-supply" optional and don't specify it until we're
> ready to go whole hog.

It isn't an optional supply because the device always has this voltage
pin connected to power it. I think we decided in the driver side patches
that this isn't an issue because the driver will ignore the regulator
for now. Eventually it will take control and firmware flashing will be
done differently. If you don't agree please let me know.

>
> Also note: looking back at the note about the fingerprint regulator,
> there's something I wonder if we tried. Did we try to have the
> userspace "updater" try unbinding the fingerprint regulator so it
> could get control of the GPIO? Then the regulator could normally have
> control of it but if userspace wanted control it would unbind the
> regulator driver.
>

Nope we didn't try that. I find it pretty disappointing that userspace
needs to control the regulator at all though. We should work towards
moving the control into the kernel for all these gpios and regulators so
that userspace simply flashes the firmware in a platform agnostic way.