linux-kernel - Re: [PATCH v3 09/11] ARM: dts: imx6ul-kontron-n6x1x-s: Disable the snvs-poweroff driver

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: <7da30a17-d16c-3cde-12a8-430ff3eec692@kontron.de>
Date:   Mon, 4 Nov 2019 11:18:19 +0000
From:   Schrempf Frieder <frieder.schrempf@...tron.de>
To:     Shawn Guo <shawnguo@...nel.org>
CC:     Krzysztof Kozlowski <krzk@...nel.org>,
        Fabio Estevam <festevam@...il.com>,
        Mark Rutland <mark.rutland@....com>,
        NXP Linux Team <linux-imx@....com>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Rob Herring <robh+dt@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 09/11] ARM: dts: imx6ul-kontron-n6x1x-s: Disable the
 snvs-poweroff driver

On 04.11.19 09:06, Frieder Schrempf wrote:
> Hi Shawn,
> 
> On 04.11.19 08:43, Shawn Guo wrote:
>> On Thu, Oct 31, 2019 at 02:24:27PM +0000, Schrempf Frieder wrote:
>>> From: Frieder Schrempf <frieder.schrempf@...tron.de>
>>>
>>> The snvs-poweroff driver can power off the system by pulling the
>>> PMIC_ON_REQ signal low, to let the PMIC disable the power.
>>> The Kontron SoMs do not have this signal connected, so let's remove
>>> the node.
>>>
>>> This seems to fix a real issue when the signal is asserted at
>>> poweroff, but not actually causing the power to turn off. It was
>>> observed, that in this case the system would not shut down properly.
>>
>> I do not quite follow on this.  How does disabling snvs_poweroff fix the
>> issue?  The root cause of system not shut down properly seems to be that
>> PMIC doesn't shut down power.  This looks like a clean-up rather than
>> bug fix.
> 
> I don't know the exact reasons, but we had issues on these boards when 
> doing a "poweroff". The kernel would print something like the log below.
> Disabling the snvs-poweroff solved this.
> 
> But note that this has last been reproduced on v4.14. So I'm not sure if 
> this is still a problem with the current kernel.
> 
> #######
> reboot: Power down
> Unable to poweroff system
> 
> ====================================
> WARNING: halt/675 still has locks held!
> 4.14.104-exceet #1 Not tainted
> ------------------------------------
> 1 lock held by halt/675:
>   #0:  (reboot_mutex){+.+.}, at: [<c0145a98>] SyS_reboot+0x14c/0x1dc
> #######
> 
>>
>>>
>>> Signed-off-by: Frieder Schrempf <frieder.schrempf@...tron.de>
>>> Fixes: 1ea4b76cdfde ("ARM: dts: imx6ul-kontron-n6310: Add Kontron 
>>> i.MX6UL N6310 SoM and boards")
>>
>> If you think this is really a bug fix, it should be applied to the file
>> before renaming rather than the one after renaming.
> 
> I will try to reproduce the issue with the current kernel and depending 
> on the results either drop the Fixes tag or move the patch before the 
> renaming.

I just tried this with 5.4-rc5 and the issue persists. When 
snvs-poweroff is enabled without the hardware actually being able to 
power down via PMIC I get a locking WARNING from the kernel. Probably 
because the system is still running, when it's already supposed to be 
shut down by the PMIC.

So this fixes a real problem and therefore I will keep the Fixes tag and 
move the patch to the proper place in this series.

> 
> Thanks for reviewing,
> Frieder
> 
>>
>> Shawn
>>
>>> ---
>>>   arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi | 4 ----
>>>   1 file changed, 4 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi 
>>> b/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
>>> index e18a8bd239be..4682a79f5b23 100644
>>> --- a/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
>>> +++ b/arch/arm/boot/dts/imx6ul-kontron-n6x1x-s.dtsi
>>> @@ -158,10 +158,6 @@
>>>       status = "okay";
>>>   };
>>> -&snvs_poweroff {
>>> -    status = "okay";
>>> -};
>>> -
>>>   &uart1 {
>>>       pinctrl-names = "default";
>>>       pinctrl-0 = <&pinctrl_uart1>;
>>> -- 
>>> 2.17.1