From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752596AbbE0M0F (ORCPT ); Wed, 27 May 2015 08:26:05 -0400 Received: from mail-wi0-f174.google.com ([209.85.212.174]:37825 "EHLO mail-wi0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751367AbbE0M0B (ORCPT ); Wed, 27 May 2015 08:26:01 -0400 Message-ID: <5565B7D7.3000308@linaro.org> Date: Wed, 27 May 2015 14:25:59 +0200 From: Daniel Lezcano User-Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Preeti U Murthy , "Rafael J. Wysocki" , Linux PM list CC: Linux Kernel Mailing List , Peter Zijlstra , Len Brown Subject: Re: [PATCH] cpuidle: Do not use CPUIDLE_DRIVER_STATE_START in cpuidle.c References: <5494512.JcUbNX5Mxs@vostro.rjw.lan> <5565AB09.8090802@linux.vnet.ibm.com> In-Reply-To: <5565AB09.8090802@linux.vnet.ibm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/27/2015 01:31 PM, Preeti U Murthy wrote: > On 05/27/2015 07:06 AM, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki >> >> The CPUIDLE_DRIVER_STATE_START symbol is defined as 1 only if >> CONFIG_ARCH_HAS_CPU_RELAX is set, otherwise it is defined as 0. >> However, if CONFIG_ARCH_HAS_CPU_RELAX is set, the first (index 0) >> entry in the cpuidle driver's table of states is overwritten with >> the default "poll" entry by the core. The "state" defined by the >> "poll" entry doesn't provide ->enter_dead and ->enter_freeze >> callbacks and its exit_latency is 0. >> >> For this reason, it is not necessary to use CPUIDLE_DRIVER_STATE_START >> in cpuidle_play_dead() (->enter_dead is NULL, so the "poll state" >> will be skipped by the loop) and in find_deepest_state() (since >> exit_latency is 0, the "poll state" will become the default if the >> "s->exit_latency <= latency_req" check is replaced with >> "s->exit_latency < latency_req" which may only matter for drivers >> providing different states with the same exit_latency). >> >> Signed-off-by: Rafael J. Wysocki >> --- >> drivers/cpuidle/cpuidle.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) > > >> >> @@ -79,13 +79,13 @@ static int find_deepest_state(struct cpu >> bool freeze) >> { >> unsigned int latency_req = 0; >> - int i, ret = freeze ? -1 : CPUIDLE_DRIVER_STATE_START - 1; >> + int i, ret = -ENXIO; >> >> - for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { >> + for (i = 0; i < drv->state_count; i++) { >> struct cpuidle_state *s = &drv->states[i]; >> struct cpuidle_state_usage *su = &dev->states_usage[i]; >> >> - if (s->disabled || su->disable || s->exit_latency <= latency_req >> + if (s->disabled || su->disable || s->exit_latency < latency_req > > Prior to this patch, > > For drivers on which CPUIDLE_DRIVER_STATE_START takes a value 0 and > whose first idle state has an exit_latency of 0, find_deepest_state() > would return -1 if it failed to find a deeper idle state. > But as an effect of this patch, find_deepest_state() returns 0 in the > above circumstance. Except I am missing something, with an exit_latency = 0, the state will be never selected, because of the "s->exit_latency < latency_req" condition (strictly greater than). > My concern is if these drivers do not intend to enter a polling state > during suspend, this will cause an issue, won't it? This also gets me > wondering if polling state is an acceptable idle state during suspend, > given that the drivers with ARCH_HAS_CPU_RELAX permit entry into it > during suspend today. Definitively poll can cause thermal issues, especially when suspending. It is a dangerous state (let's imagine you close your laptop => suspend/poll and then put it in your bag for a travel). I don't think with the code above we can reach this situation but I agree this is something we have to take care carefully. Actually, I am in favour of removing poll at all from the cpuidle driver and poll only when a cpuidle state selection fails under certain condition. So I fully agree with your statement below. > I would expect the cpus to be in a hardware > defined idle state. -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog