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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <CV7O0TYYEFA8.1Q42JITFSW77Q@otso>
Date:   Fri, 01 Sep 2023 16:54:09 +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 Wed Aug 30, 2023 at 4:57 PM CEST, Doug Anderson wrote:
> Hi,
>
> On Wed, Aug 30, 2023 at 7:43 AM Luca Weiss <luca.weiss@...rphone.com> wrote:
> >
> > On Wed Aug 30, 2023 at 4:30 PM CEST, Doug Anderson wrote:
> > > Hi,
> > >
> > > On Wed, Aug 30, 2023 at 2:58 AM Luca Weiss <luca.weiss@...rphone.com> wrote:
> > > >
> > > > On some platforms like sc7280 on non-ChromeOS devices the core clock
> > > > cannot be touched by Linux so we cannot provide it. Mark it as optional
> > > > as accessing qfprom works without it.
> > > >
> > > > Signed-off-by: Luca Weiss <luca.weiss@...rphone.com>
> > > > ---
> > > >  drivers/nvmem/qfprom.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > Are you actually testing burning fuses from the OS, or are you just
> > > using the nvmem in "read-only" mode? From comments in the bindings, if
> > > you're trying to burn the fuses then the clock is required. If things
> > > are in read-only mode then the clock isn't required.
> >
> > Hi Doug,
> >
> > I definitely don't plan on burning any fuses on this phone. Not even
> > sure that's allowed by the TZ / boot stack.
> >
> > >
> > > When I compare to the driver, it seems like the driver assumes that if
> > > more than one memory region is provided then you must be supporting
> > > burning fuses. The bindings agree that having 4 memory regions
> > > specified means that the nvmem supports burning and 1 memory region
> > > specified means read-only. The extra 3 memory regions in the nvmem are
> > > all about fuse burning, I believe.
> > >
> > > 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.

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?

Regards
Luca

>
> If somehow you do end up with something like your patch, though,
> you're still missing a bit. Specifically, you don't want to "enable
> writing" a few lines below if you didn't get the clock, right?
>
> -Doug