linux-kernel - Re: [PATCH] ARM: OMAP2+: Fix warnings with broken omap2_set_init

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: <EE749881-C3DB-4BBE-85FE-E5AF3D34884F@goldelico.com>
Date:   Tue, 3 Dec 2019 10:53:09 +0100
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Tony Lindgren <tony@...mide.com>
Cc:     Linux-OMAP <linux-omap@...r.kernel.org>,
        arm-soc <linux-arm-kernel@...ts.infradead.org>,
        Adam Ford <aford173@...il.com>,
        André Roth <neolynx@...il.com>,
        Nishanth Menon <nm@...com>, Tero Kristo <t-kristo@...com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Discussions about the Letux Kernel 
        <letux-kernel@...nphoenux.org>,
        Andreas Kemnade <andreas@...nade.info>
Subject: Re: [PATCH] ARM: OMAP2+: Fix warnings with broken omap2_set_init_voltage()

HiTony,

> Am 02.12.2019 um 22:39 schrieb Tony Lindgren <tony@...mide.com>:
> 
> Hi,
> 
> * H. Nikolaus Schaller <hns@...delico.com> [191202 21:10]:
>>> Am 25.09.2019 um 01:32 schrieb Tony Lindgren <tony@...mide.com>:
>>> Guys, please check and ack if we can really do this to get rid of some
>>> pointless dmesg -l3 errors without affecting the ongoing cpufreq and
>>> voltage work.
>> 
>> unfortunately we did not yet test in combination with the 1GHz OPP
>> patches for omap3630 (queued for v5.5-rc1) and it appears that this
>> patch breaks the 1GHz OPP.
>> 
>> The symptom is that it works fine on a dm3730 with 800MHz rating
>> but results in spurious kernel panics, stack corruption, virtual memory
>> failures, OneNAND DMA timeouts etc. on a dm3730 with 1GHz speed grade.
> 
> Hmm yeah OK, I was a bit worried about this breaking something.
> 
>> We can also re-add the "turbo-mode" tags in the omap36xx.dtsi (or
>> remove the 1GHz OPP) and can then boot with 800MHz max. clock. But
>> enabling boost (echo 1 >/sys/devices/system/cpu/cpufreq/boost) makes
>> the problem and its symptoms appear almost immediately.
>> 
>> After some scratching our heads we found that v5.3.7 is still good and
>> v5.3.8 is bad. A bisect of our tree (which includes the 1GHz OPP) did
>> point to this patch whichwas apparently already backported to v5.3.8 and
>> v5.4.
>> 
>> So I assume that the code removed here enabled or initialized something
>> we need for safe 1GHz transitions. Maybe the ABB-LDO. Or it looks up the
>> vdd regulator and initializes it earlier than without this code. Maybe
>> it also (pre-)initializes some clk which could now be left uninitialized
>> too long?
> 
> It was just doing voltdm_lookup() and clk_get_rate() and then failed
> dev_pm_opp_find_freq_ceil(), but I don't see what these might affect..
> 
>> Note that seeing the log message indicates that voltdm_scale() and
>> dev_pm_opp_get_voltage() are not called, but all functions before could
>> be with side-effects.
> 
> Yes that is strange. There's no clk_prepare() before we proceed to
> call clk_get_rate() either, not sure if that matter here though.
> 
>> v5.5-rc1 will likely fail right from the beginning (only on 1GHz rated
>> omap36xx) because it makes the combination of this patch and 1GHz OPP
>> public (linux-next should already fail but it appears that nobody has
>> tested).
> 
> OK

Well, it is not that urgent as I thought since I have not yet submitted
my patch to remove the turbo-mode tags for 1GHz OPP. Therefore even if this
code is deployed, no dm3730 will try to boot or run at 1GHz unless
manually enabled by echo 1 >/sys/devices/system/cpu/cpufreq/boost.

> 
>> Any ideas how to fix? Before I try to do a revert and then add goto exit;
>> after each function call and see which ones are essential for 1GHz.
> 
> If you have things reproducable, care to try to narrow the issue down
> a bit by trying see which parts of the old omap2_set_init_voltage()
> fix the issue?
> 
> The issue should be there somewhere in the few lines of code before
> dev_pm_opp_find_freq_ceil(), right?
> 
> It would be good to understand what's going on before reverting or
> fixing things condering that a revert would add back code that has
> it's own errors and fails to init :)

Indeed!

> 
> Another thing to check is if the dev instance is actually the right
> one we had in omap2_set_init_voltage() vs the dts dev instance as
> we use that with dev_pm_opp_find_freq_ceil().

As a first step I tried to comment out some steps but immediately
got failures.

What I then noticed is that there is only a message for

[    2.508392] omap2_set_init_voltage: unable to find boot up OPP for vdd_core
[    2.517639] omap2_set_init_voltage: unable to set vdd_core

There is none for vdd_mpu_iva. This OPP initialization is successful
and does call voltdm_scale() once.

So it appears as if omap3_init_voltages() is not a complete no-op.

IMHO the reason for the message is that u-boot defines a frequency
and voltage that can not be found in the OPP table at all.

Maybe a better solution to get rid of the message would be to modify 
dev_pm_opp_find_freq_ceil() to interpolate between OPPs?

Hm. After looking into the code I start to wonder why it fails at
all. _find_freq_ceil() should return the highest available frequency
above the one passed in and u-boot should not pass more than 800 MHz...

That is IMHO a good next step to go into details.

BR,
Nikolaus