linux-kernel - Re: [PATCH 02/11] nvmem: qfprom: Mark core clk as optional

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: <CV9ZEMOTNO1D.378QGW3KMG4E8@otso>
Date:   Mon, 04 Sep 2023 10:14:42 +0200
From:   "Luca Weiss" <luca.weiss@...rphone.com>
To:     "Doug Anderson" <dianders@...omium.org>
Cc:     <cros-qcom-dts-watchers@...omium.org>,
        "Andy Gross" <agross@...nel.org>,
        "Bjorn Andersson" <andersson@...nel.org>,
        "Konrad Dybcio" <konrad.dybcio@...aro.org>,
        "Rob Herring" <robh+dt@...nel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>,
        "Conor Dooley" <conor+dt@...nel.org>,
        "Srinivas Kandagatla" <srinivas.kandagatla@...aro.org>,
        "Linus Walleij" <linus.walleij@...aro.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        "Viresh Kumar" <viresh.kumar@...aro.org>,
        <~postmarketos/upstreaming@...ts.sr.ht>,
        <phone-devel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-gpio@...r.kernel.org>, <linux-pm@...r.kernel.org>
Subject: Re: [PATCH 02/11] nvmem: qfprom: Mark core clk as optional

On Fri Sep 1, 2023 at 5:08 PM CEST, Doug Anderson wrote:
> Hi,
>
> On Fri, Sep 1, 2023 at 7:54 AM Luca Weiss <luca.weiss@...rphone.com> wrote:
> >
> > > > > So maybe the right fix here is to just change your dts to specify one
> > > > > memory region?
> > > >
> > > > I got feedback from Konrad that this here would be the preferred
> > > > approach compared to having a different dts for ChromeOS vs non-ChromeOS
> > > > devices. I don't feel strongly to either, for me it's also okay to
> > > > remove the extra memory regions and only have the main one used on
> > > > regular qcom devices.
> > > >
> > > > Let me know what you think.
> > >
> > > I don't hate the idea of leaving the extra memory regions in the dts.
> > > They do describe the hardware, after all, even if the main OS can't
> > > actually access those memory regions. ...though the same could also be
> > > said about the clock you've removed. Said another way: if you want to
> > > fully describe the hardware then the dts should have the extra memory
> > > regions and the clock. If you are OK w/ just describing the hardware
> > > in the way that the OS has access to then the dts should not have the
> > > extra memory regions and not have the clock. Does that sound right?
> >
> > Not sure which of those memory regions are actually accessible on this
> > board, but honestly I don't even want to try accessing it. Blowing fuses
> > is not my wish there ;)
> >
> > On downstream the node is just described like the following:
> >
> >         qfprom: qfprom@...000 {
> >                 compatible = "qcom,qfprom";
> >                 reg = <0x780000 0x7000>;
> >                 ...
> >         };
> >
> > So we have 0x780000 - 0x786fff here.
> >
> > In sc7280.dtsi we have the following:
> >
> >         qfprom: efuse@...000 {
> >                 compatible = "qcom,sc7280-qfprom", "qcom,qfprom";
> >                 reg = <0 0x00784000 0 0xa20>,
> >                           <0 0x00780000 0 0xa20>,
> >                           <0 0x00782000 0 0x120>,
> >                           <0 0x00786000 0 0x1fff>;
> >                 ...
> >         };
> >
> > So I guess this:
> > * 0x780000 - 0x780a1f
> > * 0x782000 - 0x78211f
> > * 0x784000 - 0x784a1f
> > * 0x786000 - 0x787ffe
> >
> > So at least the last memory region seems to be partially out of range
> > according to downstream.
>
> From the other discussion, it sounds as if you _can_ leave the clock
> in the device tree and then use "clk_get_optional" here. IMO then, the
> right answer is to use "clk_get_optional" but then also modify the
> check below so that instead of:
>
> /* Only enable writing if we have SoC data. */
> if (priv->soc_data)
>   econfig.reg_write = qfprom_reg_write;
>
> It is:
>
> /* Only enable writing if we have SoC data and a valid clock */
> if (priv->soc_data && priv->secclk)
>   econfig.reg_write = qfprom_reg_write;
>
>
> Does that work for you?

Thanks Doug, this feels like a good solution. I'll update v2 with that.

>
>
> > So after reading all of this I tried running this commmand on the phone
> > and the phone reboots into 900e mode.
> >
> >   $ cat /sys/devices/platform/soc@...84000.efuse/qfprom0/nvmem
> >
> > I guess normally this should work? So if I interpret this correctly, the
> > Linux driver thinks it can access more than it can/should. But also
> > should probably try this command on another chipset to see if it works
> > on any really?
>
> Presumably your firmware needs a different "sc7280_qfprom_keepout". If
> that's true then I guess you'll have to undergo negotiations with the
> DT bindings folks and the nvmem maintainer to figure out how to
> specify that your firmware protects different things than the ChromeOS
> firmware?

Right. But based on Konrad's reply here, I think I'll skip this for now
since generally qfprom seems to be fine with the current data, at least
for the purposes it's used for on my device.

Regards
Luca


>
>
> -Doug