Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755368AbcKBImd (ORCPT ); Wed, 2 Nov 2016 04:42:33 -0400 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:33072 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754360AbcKBIma (ORCPT ); Wed, 2 Nov 2016 04:42:30 -0400 Date: Wed, 2 Nov 2016 14:12:21 +0530 From: Gautham R Shenoy To: "Oliver O'Halloran" Cc: "Gautham R. Shenoy" , Michael Ellerman , Benjamin Herrenschmidt , Paul Mackerras , "Rafael J. Wysocki" , Daniel Lezcano , Michael Neuling , Vaidyanathan Srinivasan , "Shreyas B. Prabhu" , Shilpasri G Bhat , Stewart Smith , Balbir Singh , skiboot list , linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org Subject: Re: [PATCH v2 2/3] cpuidle:powernv: Add helper function to populate powernv idle states. Reply-To: ego@linux.vnet.ibm.com References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16110208-0004-0000-0000-000010BF2022 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006018; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000189; SDB=6.00775729; UDB=6.00373019; IPR=6.00552823; BA=6.00004850; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00013184; XFM=3.00000011; UTC=2016-11-02 08:42:27 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16110208-0005-0000-0000-00007A3A3F12 Message-Id: <20161102084221.GC17909@in.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-11-02_02:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000 definitions=main-1611020161 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9497 Lines: 212 Hi Oliver, Thanks for reviewing the patch! On Tue, Nov 01, 2016 at 07:32:58PM +1100, Oliver O'Halloran wrote: > On Thu, Oct 27, 2016 at 7:35 PM, Gautham R. Shenoy > wrote: > > From: "Gautham R. Shenoy" > > > > In the current code for powernv_add_idle_states, there is a lot of code > > duplication while initializing an idle state in powernv_states table. > > > > Add an inline helper function to populate the powernv_states[] table for > > a given idle state. Invoke this for populating the "Nap", "Fastsleep" > > and the stop states in powernv_add_idle_states. > > > > Signed-off-by: Gautham R. Shenoy > > --- > > drivers/cpuidle/cpuidle-powernv.c | 82 +++++++++++++++++++++++---------------- > > include/linux/cpuidle.h | 1 + > > 2 files changed, 49 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c > > index 7fe442c..11b22b9 100644 > > --- a/drivers/cpuidle/cpuidle-powernv.c > > +++ b/drivers/cpuidle/cpuidle-powernv.c > > @@ -167,6 +167,28 @@ static int powernv_cpuidle_driver_init(void) > > return 0; > > } > > > > +static inline void add_powernv_state(int index, const char *name, > > + unsigned int flags, > > + int (*idle_fn)(struct cpuidle_device *, > > + struct cpuidle_driver *, > > + int), > > + unsigned int target_residency, > > + unsigned int exit_latency, > > + u64 psscr_val) > > +{ > > + strncpy(powernv_states[index].name, name, CPUIDLE_NAME_LEN); > > + strncpy(powernv_states[index].desc, name, CPUIDLE_NAME_LEN); > > If the supplied name is equal to CPUIDLE_NAME_LEN then strncpy() won't > terminate the string. The least annoying fix is to memset() the whole > structure to zero and set n to CPUIDLE_NAME_LEN - 1. I will change this to use strlcpy as Paul suggested in the reply. > > > + powernv_states[index].flags = flags; > > + powernv_states[index].target_residency = target_residency; > > + powernv_states[index].exit_latency = exit_latency; > > + powernv_states[index].enter = idle_fn; > > + > > + if (idle_fn != stop_loop) > > + return; > > This can probably be deleted. The nap/sleep loops don't look at the > psscr setting and you've passed in a dummy value of zero anyway. The subsequent patch adds the missing bits to the psscr_val if we are running on the older firmware. But in any case, as you rightly pointed out we don't use psscr_table in nap/sleep loops. So this can go. > > > + > > + stop_psscr_table[index] = psscr_val; > > +} > > + > > static int powernv_add_idle_states(void) > > { > > struct device_node *power_mgt; > > @@ -236,6 +258,7 @@ static int powernv_add_idle_states(void) > > "ibm,cpu-idle-state-residency-ns", residency_ns, dt_idle_states); > > > > for (i = 0; i < dt_idle_states; i++) { > > + unsigned int exit_latency, target_residency; > > /* > > * If an idle state has exit latency beyond > > * POWERNV_THRESHOLD_LATENCY_NS then don't use it > > @@ -244,27 +267,30 @@ static int powernv_add_idle_states(void) > > if (latency_ns[i] > POWERNV_THRESHOLD_LATENCY_NS) > > continue; > > > > + exit_latency = ((unsigned int)latency_ns[i]) / 1000; > > + target_residency = (!rc) ? ((unsigned int)residency_ns[i]) : 0; > > + /* > > + * Firmware passes residency values in ns. > > + * cpuidle expects it in us. > > + */ > > + target_residency /= 1000; > > + > > /* > > * Cpuidle accepts exit_latency and target_residency in us. > > This comment says the same thing as the one above. Will clean this up to reflect the state of the code. > > > * Use default target_residency values if f/w does not expose it. > > */ > > if (flags[i] & OPAL_PM_NAP_ENABLED) { > > + target_residency = 100; > > Hmm, the above comment says that we should only use the default value > for target_residency if firmware hasn't provided a value. Is there a > reason we always use the hard coded value? Ah, good catch! I should be setting target_residency to the default value only if it is not set by the firmware. Will fix this. > > > /* Add NAP state */ > > - strcpy(powernv_states[nr_idle_states].name, "Nap"); > > - strcpy(powernv_states[nr_idle_states].desc, "Nap"); > > - powernv_states[nr_idle_states].flags = 0; > > - powernv_states[nr_idle_states].target_residency = 100; > > - powernv_states[nr_idle_states].enter = nap_loop; > > + add_powernv_state(nr_idle_states, "Nap", > > + CPUIDLE_FLAG_NONE, nap_loop, > > + target_residency, exit_latency, 0); > > } else if ((flags[i] & OPAL_PM_STOP_INST_FAST) && > > !(flags[i] & OPAL_PM_TIMEBASE_STOP)) { > > - strncpy(powernv_states[nr_idle_states].name, > > - names[i], CPUIDLE_NAME_LEN); > > - strncpy(powernv_states[nr_idle_states].desc, > > - names[i], CPUIDLE_NAME_LEN); > > - powernv_states[nr_idle_states].flags = 0; > > - > > - powernv_states[nr_idle_states].enter = stop_loop; > > - stop_psscr_table[nr_idle_states] = psscr_val[i]; > > + add_powernv_state(nr_idle_states, names[i], > > + CPUIDLE_FLAG_NONE, stop_loop, > > + target_residency, exit_latency, > > + psscr_val[i]); > > } > > > > /* > > @@ -274,32 +300,20 @@ static int powernv_add_idle_states(void) > > #ifdef CONFIG_TICK_ONESHOT > > if (flags[i] & OPAL_PM_SLEEP_ENABLED || > > flags[i] & OPAL_PM_SLEEP_ENABLED_ER1) { > > + target_residency = 300000; > > Same comment as above. > > > /* Add FASTSLEEP state */ > > - strcpy(powernv_states[nr_idle_states].name, "FastSleep"); > > - strcpy(powernv_states[nr_idle_states].desc, "FastSleep"); > > - powernv_states[nr_idle_states].flags = CPUIDLE_FLAG_TIMER_STOP; > > - powernv_states[nr_idle_states].target_residency = 300000; > > - powernv_states[nr_idle_states].enter = fastsleep_loop; > > + add_powernv_state(nr_idle_states, "FastSleep", > > + CPUIDLE_FLAG_TIMER_STOP, > > + fastsleep_loop, > > + target_residency, exit_latency, 0); > > } else if ((flags[i] & OPAL_PM_STOP_INST_DEEP) && > > (flags[i] & OPAL_PM_TIMEBASE_STOP)) { > > - strncpy(powernv_states[nr_idle_states].name, > > - names[i], CPUIDLE_NAME_LEN); > > - strncpy(powernv_states[nr_idle_states].desc, > > - names[i], CPUIDLE_NAME_LEN); > > - > > - powernv_states[nr_idle_states].flags = CPUIDLE_FLAG_TIMER_STOP; > > - powernv_states[nr_idle_states].enter = stop_loop; > > - stop_psscr_table[nr_idle_states] = psscr_val[i]; > > + add_powernv_state(nr_idle_states, names[i], > > + CPUIDLE_FLAG_TIMER_STOP, stop_loop, > > + target_residency, exit_latency, > > + psscr_val[i]); > > } > > #endif > > - powernv_states[nr_idle_states].exit_latency = > > - ((unsigned int)latency_ns[i]) / 1000; > > - > > - if (!rc) { > > - powernv_states[nr_idle_states].target_residency = > > - ((unsigned int)residency_ns[i]) / 1000; > > - } > > - > > nr_idle_states++; > > } > > out: > > diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h > > index bb31373..c4e10f8 100644 > > --- a/include/linux/cpuidle.h > > +++ b/include/linux/cpuidle.h > > @@ -62,6 +62,7 @@ struct cpuidle_state { > > }; > > > > /* Idle State Flags */ > > +#define CPUIDLE_FLAG_NONE (0x00) > > #define CPUIDLE_FLAG_COUPLED (0x02) /* state applies to multiple cpus */ > > #define CPUIDLE_FLAG_TIMER_STOP (0x04) /* timer is stopped on this state */ > > > > -- > > 1.9.4 > > > > Looks good otherwise. > > Reviewed-by: Oliver O'Halloran Thanks again! > -- Regards gautham.