linux-kernel - Re: [PATCH v4] regulator: core: Resolve supply name earlier to prevent double-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: <932DEB17-70FB-4416-80B3-C48A7C31848F@kohlschutter.com>
Date:   Thu, 25 Aug 2022 17:18:20 +0200
From:   Christian Kohlschütter 
        <christian@...lschutter.com>
To:     Marek Szyprowski <m.szyprowski@...sung.com>,
        Mark Brown <broonie@...nel.org>
Cc:     Heiko Stübner <heiko@...ech.de>,
        Liam Girdwood <lgirdwood@...il.com>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Linux MMC List <linux-mmc@...r.kernel.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>,
        Markus Reichl <m.reichl@...etechno.de>,
        Robin Murphy <robin.murphy@....com>,
        Vincent Legoll <vincent.legoll@...il.com>, wens@...nel.org
Subject: Re: [PATCH v4] regulator: core: Resolve supply name earlier to
 prevent double-init

> On 25. Aug 2022, at 16:23, Marek Szyprowski <m.szyprowski@...sung.com> wrote:
> 
> Hi Mark,
> 
> On 25.08.2022 14:21, Mark Brown wrote:
>> On Thu, Aug 25, 2022 at 01:32:50PM +0200, Marek Szyprowski wrote:
>> 
>>> This patch landed recently in linux next as commit 8a866d527ac0
>>> ("regulator: core: Resolve supply name earlier to prevent double-init").
>>> Unfortunately it breaks booting of Samsung Exynos 5800 based Peach-Pi
>>> (arch/arm/boot/dts/exynos5800-peach-pi.dts) and Peach-Pit
>>> (arch/arm/boot/dts/exynos5420-peach-pit.dts) Chromebooks. The last
>>> message in the kernel log is a message about disabling 'vdd_1v2'
>>> regulator. This regulator is not used directly, however it is a supply
>>> for other critical regulators.
>> This suggests that supplies are ending up not getting bound.  Could you
>> perhaps add logging to check that we're attempting to resolve the supply
>> (in the
>> 
>> 
>> +       if ((rdev->supply_name && !rdev->supply) &&
>> +                       (rdev->constraints->always_on ||
>> +                        rdev->constraints->boot_on)) {
>> 
>> block)?
> 
> 
> I've spent a little time debugging this issue and here are my findings. 
> The problem is during the 'vdd_mif' regulator registration. It has one 
> supply called 'inb1' and provided by 'vdd_1v2' regulator. Both 'vdd_mif' 
> and 'vdd_1v2' regulators are provided by the same PMIC.
> 
> The problem is that 'inb1' supply is being routed to dummy regulator 
> after this change. The regulator_resolve_supply(), which is just after 
> the above mentioned check, returns 0 and bounds 'vdd_mif' supply to 
> dummy-regulator. This happens because regulator_dev_lookup() called in 
> regulator_resolve_supply() returns -19, what in turn lets the code to 
> use dummy-regulator. I didn't check why it doesn't return -EPROBE_DEFER 
> in that case yet.
> 
>> I'd also note that it's useful to paste the actual error
>> messages you're seeing rather than just a description of them.
> 
> There is really nothing more that I can paste here:
> 
> [   32.306264] systemd-logind[1375]: New seat seat0.
> [   32.331790] systemd-logind[1375]: Watching system buttons on 
> /dev/input/event1 (gpio-keys)
> [   32.550686] systemd-logind[1375]: Watching system buttons on 
> /dev/input/event0 (cros_ec)
> [   32.570493] systemd-logind[1375]: Failed to start user service, 
> ignoring: Unknown unit: user@...ervice
> [   32.750913] systemd-logind[1375]: New session c1 of user root.
> [   35.070357] vdd_1v2:
> 
> --- EOF ---
> 

I can reproduce these findings (also see the difference in "cat /sys/kernel/debug/regulator/regulator_summary")

The check "if (have_full_constraints())" in "regulator_resolve_supply" is called even though regulator_dev_lookup returned -ENODEV (-19).
Since my patch now calls "regulator_resolve_supply" twice, the first round should really treat ENODEV as "defer".

I propose adding a boolean defer argument to regulator_resolve_supply (with defer=true in the first, opportunistic run, and false in any other situation). I'll have a patch ready later tonight.

Thanks!
Christian