linux-kernel - Re: [PATCH 1/5] arm64: dts: Add basic DT to support Spreadtrum's SP9860G

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] [day] [month] [year] [list]

Message-ID: <20170220125644.GB9600@spreadtrum.com>
Date:   Mon, 20 Feb 2017 20:56:44 +0800
From:   Chunyan Zhang <chunyan.zhang@...eadtrum.com>
To:     Sudeep Holla <sudeep.holla@....com>
CC:     <devicetree@...r.kernel.org>, <orson.zhai@...eadtrum.com>,
        open list <linux-kernel@...r.kernel.org>,
        linux-arm <linux-arm-kernel@...ts.infradead.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH 1/5] arm64: dts: Add basic DT to support Spreadtrum's
 SP9860G

On 一,  2月 20, 2017 at 10:47:56上午 +0000, Sudeep Holla wrote:
> 
> 
> On 20/02/17 09:37, Chunyan Zhang wrote:
> > Hi Sudeep,
> > 
> > On 五,  2月 17, 2017 at 10:28:00上午 +0000, Sudeep Holla wrote:
> >>
> >>
> >> On 17/02/17 07:28, Chunyan Zhang wrote:
> >>> Hi Sudeep,
> >>>
> >>> On 二,  2月 14, 2017 at 04:44:53下午 +0000, Sudeep Holla wrote:
> >>>> On Tue, Feb 14, 2017 at 9:19 AM, Chunyan Zhang
> >>>> <chunyan.zhang@...eadtrum.com> wrote:
> >>
> >> [..]
> >>
> >>>>
> >>>>> +       idle-states{
> >>>>> +               entry-method = "arm,psci";
> >>>>> +
> >>>>> +               CORE_PD: core_pd {
> >>>>> +                       compatible = "arm,idle-state";
> >>>>> +                       entry-latency-us = <1000>;
> >>>>> +                       exit-latency-us = <700>;
> >>>>> +                       min-residency-us = <2500>;
> >>>>> +                       local-timer-stop;
> >>>>> +                       arm,psci-suspend-param = <0x00010002>;
> >>>>> +               };
> >>>>> +
> >>>>> +               CLUSTER_PD: cluster_pd {
> >>>>> +                       compatible = "arm,idle-state";
> >>>>> +                       entry-latency-us = <1000>;
> >>>>> +                       exit-latency-us = <1000>;
> >>>>> +                       min-residency-us = <3000>;
> >>>>> +                       local-timer-stop;
> >>>>> +                       arm,psci-suspend-param = <0x01010003>;
> >>>>> +               };
> >>>>> +
> >>>>> +               DEEP_SLEEP: deep_sleep {
> >>>>> +                       compatible = "arm,idle-state";
> >>>>> +                       wakeup-latency-us = <0xffffffff>;
> >>>>
> >>>> A value > 4294 seconds(i.e >1 hour) seems suspicious.
> >>>> Are you working around the firmware issue with high latency value so
> >>>> that it's never entered ? Why not remove advertising the state from DT.
> >>>>
> >>>
> >>> Haved checked with related colleagues, this node 'deep_sleep' was not for working
> >>> around any firmware issue, but was a trick utilization of idle subsystem, and that
> >>
> >> Really ? Any latency greater few milliseconds are sounds useless. I
> >> still don't understand what you mean by "trick utilization of idle
> >> subsystem".
> >>
> > 
> > Sorry for confused expression, I meant it was not a right way to utilize idle mechanism
> > and shouldn't be upstreamed.
> > 
> 
> No problem.
> 
> >>> was definitely not elegant, the author indeed intendly didn't want CPU entered this
> >>> state, I will remove this node therefore.
> >>
> >> It's quick and dirty "HACK* to retain and advertise the state but
> >> ensure it's never entered and obstruct the boot. It's not a trick to
> >> exploit any idle subsystem utilization.
> >>
> > 
> 
> > Right, actually deep_sleep was for 'suspend' (forces idleness upon 
> > the OS until a wake-up event resumes the OS from suspend), for 
> > example when users press power key on mobile phone to turn off the
> > screen. So the author implemented 'suspend' using cpu_psci_ops::cpu_suspend
> > I figure that this  way is not correct, I will remove this state from DT.
> 
> OK.
> 
> > I would appreciate any suggestion for how to implement this kind of
> > function properly.
> 
> 
> For the 'suspend' functionality you have described above, all you need
> is the firmware to implement PSCI SYSTEM_SUSPEND API in the firmware.
> The kernel psci driver detects the presence of the same and registers
> the suspend ops automatically. You need not add anything in the code or
> DT for the same.

Thank you Sudeep, I will have my colleague to study further according to
the direction you provided here.

Thanks for your comments,
Chunyan

> 
> -- 
> Regards,
> Sudeep