From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 3x3sjn0J36zDr5d for ; Fri, 7 Jul 2017 21:25:48 +1000 (AEST) Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v67BOgkv024017 for ; Fri, 7 Jul 2017 07:25:46 -0400 Received: from e32.co.us.ibm.com (e32.co.us.ibm.com [32.97.110.150]) by mx0a-001b2d01.pphosted.com with ESMTP id 2bj9cy00w3-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 07 Jul 2017 07:25:46 -0400 Received: from localhost by e32.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 7 Jul 2017 05:25:45 -0600 Date: Fri, 7 Jul 2017 16:55:39 +0530 From: Gautham R Shenoy To: Nicholas Piggin Cc: "Gautham R. Shenoy" , Michael Ellerman , Michael Neuling , Vaidyanathan Srinivasan , Shilpasri G Bhat , "Rafael J. Wysocki" , Akshay Adiga , linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org Subject: Re: [PATCH 1/5] powernv:idle: Move device-tree parsing to one place. Reply-To: ego@linux.vnet.ibm.com References: <1499272696-28751-1-git-send-email-ego@linux.vnet.ibm.com> <1499272696-28751-2-git-send-email-ego@linux.vnet.ibm.com> <20170707005340.003c530b@roar.ozlabs.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <20170707005340.003c530b@roar.ozlabs.ibm.com> Message-Id: <20170707112539.GA8913@in.ibm.com> List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Hello Nicholas, On Fri, Jul 07, 2017 at 12:53:40AM +1000, Nicholas Piggin wrote: > On Wed, 5 Jul 2017 22:08:12 +0530 > "Gautham R. Shenoy" wrote: > > > From: "Gautham R. Shenoy" > > > > The details of the platform idle state are exposed by the firmware to > > the kernel via device tree. > > > > In the current code, we parse the device tree twice : > > > > 1) During the boot up in arch/powerpc/platforms/powernv/idle.c Here, > > the device tree is parsed to obtain the details of the > > supported_cpuidle_states which is used to determine the default idle > > state (which would be used when cpuidle is absent) and the deepest > > idle state (which would be used for cpu-hotplug). > > > > 2) During the powernv cpuidle driver initializion > > (drivers/cpuidle/cpuidle-powernv.c). Here we parse the device tree to > > populate the cpuidle driver's states. > > > > This patch moves all the device tree parsing to the platform idle > > code. It defines data-structures for recording the details of the > > parsed idle states. Any other kernel subsystem that is interested in > > the idle states (eg: cpuidle-powernv driver) can just use the > > in-kernel data structure instead of parsing the device tree all over > > again. > > > > Further, this helps to check the validity of states in one place and > > in case of invalid states (eg : stop states whose psscr values are > > errorenous) flag them as invalid, so that the other subsystems can be > > prevented from using those. > > > > Signed-off-by: Gautham R. Shenoy > > Hi, > > I think the overall direction is good. A few small things. Thanks for reviewing the patches. > > > > + > > +#define PNV_IDLE_NAME_LEN 16 > > +struct pnv_idle_state { > > + char name[PNV_IDLE_NAME_LEN]; > > + u32 flags; > > + u32 latency_ns; > > + u32 residency_ns; > > + u64 ctrl_reg_val; /* The ctrl_reg on POWER8 would be pmicr. */ > > + u64 ctrl_reg_mask; /* On POWER9 it is psscr */ > > + bool valid; > > +}; > > Do we use PMICR anywhere in the idle code? What about allowing for some > machine-specific fields? PMICR is not used anywhere so far. I will change to to psscr_val and psscr_mask for now. If there is a use for pmicr n the future, we can change this to the union struct as you suggest. > > union { > struct { /* p9 */ > u64 psscr_val; > u64 psscr_mask; > }; > struct { /* p8 */ > u64 pmicr...; > > > > diff --git a/arch/powerpc/platforms/powernv/idle.c b/arch/powerpc/platforms/powernv/idle.c > > index 2abee07..b747bb5 100644 > > --- a/arch/powerpc/platforms/powernv/idle.c > > +++ b/arch/powerpc/platforms/powernv/idle.c > > @@ -58,6 +58,17 @@ > > static u64 pnv_deepest_stop_psscr_mask; > > static bool deepest_stop_found; > > > > +/* > > + * Data structure that stores details of > > + * all the platform idle states. > > + */ > > +struct pnv_idle_states pnv_idle; > > + > > +struct pnv_idle_states *get_pnv_idle_states(void) > > +{ > > + return &pnv_idle; > > +} > > I wouldn't have the wrapper function... but it's your code so it's > up to you. One thing though is that this function you have called get_ > just to return the pointer, but it does not take a reference or > allocate memory or initialize the structure. Other functions with the > same prefix do such things. Can we make something more consistent? I agree with the wrapper function. But then the alternative was to declare this variable as an extern so that cpuidle can access it. Is that preferable ? > > ... > > > +/** > > + * get_idle_prop_u32_array: Returns an array of u32 elements > > + * parsed from the device tree corresponding > > + * to the property provided in variable propname. > > + * > > + * @np: Pointer to device tree node "/ibm,opal/power-mgt" > > + * @nr_states: Expected number of elements. > > + * @propname : Name of the property whose values is an array of > > + * u32 elements > > + * > > + * Returns a pointer to a u32 array of size nr_states on success. > > + * Returns NULL on failure. > > + */ > > +static inline u32 *get_idle_prop_u32_array(struct device_node *np, > > + int nr_states, > > + const char *propname) > > +{ > > + u32 *ret_array; > > + int rc, count; > > + > > + count = of_property_count_u32_elems(np, propname); > > + rc = validate_dt_prop_sizes("ibm,cpu-idle-state-flags", nr_states, > > + propname, count); > > + if (rc) > > + return NULL; > > + > > + ret_array = kcalloc(nr_states, sizeof(*ret_array), GFP_KERNEL); > > + if (!ret_array) > > + return NULL; > > So I would say for this, how about moving the allocations into the caller? > You're still doing most of the error handling freeing there, so I would > say it's more balanced if you do that. Sure, that makes sense. I will move the allocation to the main function and remove the "inline" associated with these helpers. > > Also, perhaps consider dropping most of the inline keywords. Unless it's > performance critical or does some significant optimisation due to constant > parameters I would say avoid the keyword as a rule. > > [snip] > > There's a lot of code movement, I haven't reviewed it all carefully, but > it looks good in general. I'll apply the patches and check the result > in the next few days when I get a bit of time. If it helps, I will post the subsequent version breaking this patch into smaller ones. > > Thanks, > Nick > -- Thanks and Regards gautham.