linux-kernel - Re: [PATCH v1 07/12] scsi: ufs: add option to change default UFS power management level

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: <CAL_Jsq+mu0R58in7oOq-mho43ERqOQk=jiu5vCWXq5=4eJJSpQ@mail.gmail.com>
Date:   Mon, 19 Dec 2016 12:38:01 -0600
From:   Rob Herring <robh@...nel.org>
To:     Subhash Jadavani <subhashj@...eaurora.org>
Cc:     Vinayak Holikatti <vinholikatti@...il.com>,
        jejb@...ux.vnet.ibm.com,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        linux-scsi@...r.kernel.org, Mark Rutland <mark.rutland@....com>,
        Hannes Reinecke <hare@...e.com>,
        Yaniv Gardi <ygardi@...eaurora.org>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 07/12] scsi: ufs: add option to change default UFS
 power management level

On Tue, Dec 13, 2016 at 2:16 PM, Subhash Jadavani
<subhashj@...eaurora.org> wrote:
> On 2016-12-13 12:04, Rob Herring wrote:
>>
>> On Mon, Dec 12, 2016 at 04:54:20PM -0800, Subhash Jadavani wrote:
>>>
>>> UFS device and link can be put in multiple different low power modes
>>> hence
>>> UFS driver supports multiple different low power modes. By default UFS
>>> driver selects the default (optimal) low power mode (which gives moderate
>>> power savings and have relatively less enter and exit latencies) but
>>> we might have to tune this default power mode for different chipset
>>> platforms to meet the low power requirements/goals. Hence this patch
>>> adds option to change default UFS low power mode (level).
>>>
>>> Reviewed-by: Yaniv Gardi <ygardi@...eaurora.org>
>>> Signed-off-by: Subhash Jadavani <subhashj@...eaurora.org>
>>> ---
>>>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt      | 10 ++++++
>>>  drivers/scsi/ufs/ufshcd-pltfrm.c                   | 14 ++++++++
>>>  drivers/scsi/ufs/ufshcd.c                          | 39
>>> ++++++++++++++++++++++
>>>  drivers/scsi/ufs/ufshcd.h                          |  4 +--
>>>  4 files changed, 65 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>> index a99ed55..c3836c5 100644
>>> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>> @@ -41,6 +41,14 @@ Optional properties:
>>>  -lanes-per-direction   : number of lanes available per direction -
>>> either 1 or 2.
>>>                           Note that it is assume same number of lanes is
>>> used both
>>>                           directions at once. If not specified, default
>>> is 2 lanes per direction.
>>> +- rpm-level            : UFS Runtime power management level. Following
>>> PM levels are supported:
>>> +                         0 - Both UFS device and Link in active state
>>> (Highest power consumption)
>>> +                         1 - UFS device in active state but Link in
>>> Hibern8 state
>>> +                         2 - UFS device in Sleep state but Link in
>>> active state
>>> +                         3 - UFS device in Sleep state and Link in
>>> hibern8 state (default PM level)
>>> +                         4 - UFS device in Power-down state and Link in
>>> Hibern8 state
>>> +                         5 - UFS device in Power-down state and Link in
>>> OFF state (Lowest power consumption)
>>> +- spm-level            : UFS System power management level. Allowed PM
>>> levels are same as rpm-level.
>>
>>
>> This looks like you are putting policy for Linux into DT.
>>
>> What I would expect to see here is disabling of states that don't work
>> due to some h/w limitation. Otherwise, it is a user decision for what
>> modes to go into. Also, I think link and device states should be
>> separate.
>
>
> Yes, generally default level (3) is good enough (and recommended) for all
> platforms and most likely user is only expected to change this if they see
> issues (most H/W) on their platform or they want even more aggressive power
> state (level-4 or level-5) and ready to take the performance hit associated
> with resume latencies.

What latencies can be tolerated is going to depend on the application
and could vary while running, so putting in DT doesn't make sense. I
would break down settings like this:

broken h/w -> DT
user tuning/config -> sysfs
sensible defaults -> driver

> Also, I think it is better to keep Link and device states tied, one reason
> is that we can't keep device in sleep/active state when Link is in OFF
> state.

The driver can tie the states to together if needed. Just document
what's broken in DT and let the driver make decisions.

Rob