Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752353AbbFDJx7 (ORCPT ); Thu, 4 Jun 2015 05:53:59 -0400 Received: from e23smtp01.au.ibm.com ([202.81.31.143]:36638 "EHLO e23smtp01.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750928AbbFDJxt (ORCPT ); Thu, 4 Jun 2015 05:53:49 -0400 Message-ID: <55701FF1.7050803@linux.vnet.ibm.com> Date: Thu, 04 Jun 2015 15:22:49 +0530 From: Madhavan Srinivasan User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: Daniel Axtens CC: linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, Stephane Eranian , Paul Mackerras , Sukadev Bhattiprolu , Anshuman Khandual Subject: Re: [PATCH v1 5/9]powerpc/powernv: nest pmu feature detection support References: <1433260778-26497-1-git-send-email-maddy@linux.vnet.ibm.com> <1433260778-26497-6-git-send-email-maddy@linux.vnet.ibm.com> <1433290876.438.46.camel@axtens.net> In-Reply-To: <1433290876.438.46.camel@axtens.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 15060409-1618-0000-0000-000002359186 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3624 Lines: 107 On Wednesday 03 June 2015 05:51 AM, Daniel Axtens wrote: > On Tue, 2015-06-02 at 21:29 +0530, Madhavan Srinivasan wrote: >> Patch adds a device tree function to detect the nest pmu >> support. Function will look for specific dt property "ibm,ima-chip" >> as a detection mechanism for the nest pmu. >> >> For Nest pmu, device tree will have two set of information. >> 1) Per-chip Homer address region for nest pmu counter collection area. >> 2) Supported Nest PMUs and events > What's HOMER? Nest PMUs are configured via PORE engine interface and PORE Engine collections the Nest counter value and updates in the main memory which is reserved for this use. >> >> +static int nest_ima_detect_parse(void) >> +{ >> + const __be32 *gcid; >> + const __be64 *chip_ima_reg; >> + const __be64 *chip_ima_size; >> + struct device_node *dev; >> + int rc = -EINVAL, idx; >> + >> + for_each_node_with_property(dev, "ibm,ima-chip") { >> + gcid = of_get_property(dev, "ibm,chip-id", NULL); >> + chip_ima_reg = of_get_property(dev, "reg", NULL); >> + chip_ima_size = of_get_property(dev, "size", NULL); >> + if ((!gcid) || (!chip_ima_reg) || (!chip_ima_size)) { >> + pr_err("%s: device %s missing property \n", >> + __func__, dev->full_name); > This is not a particularly informative error message. It'd be good if it > mentioned that it was for PMU. Sure will changes. >> + return rc >> + } >> + >> + idx = (uint32_t)be32_to_cpup(gcid); >> + p8_perchip_nest_info[idx].pbase = be64_to_cpup(chip_ima_reg); >> + p8_perchip_nest_info[idx].size = be64_to_cpup(chip_ima_size); >> + p8_perchip_nest_info[idx].vbase = (uint64_t) >> + phys_to_virt(p8_perchip_nest_info[idx].pbase); >> + >> + rc = 0; >> + } >> + >> + return rc; > I'm not sure your rc handling is correct. As I understand it: > - Start with rc = -EINVAL. > - If your first node is missing a property, return -EINVAL. > - Once your first node succeeds, set rc = 0 > - If any subsequent node is missing a property, return 0. > - Return 0 if any node is successfully processed, otherwise return > -EINVAL. Main loop is only for nodes with property "ibm,ima-chip". Not all the nodes will have this property. > If that's what you intended (especially with regards to returning 0 when > a subsequent node is missing a property), a comment explaining it would > be great. Yes. I will add comment explaining it. But i did add this in the commit message. > Also, why bail out if a property is missing on any node? Why not try all > of them and see if any succeed? Only the Nest Unit nodes in the device tree will have this property. Commit has the device tree hierarchy for the Nest instrumentation. So if we dont find this property then Nest instrumentation is not supported, hence bail out. > >> +} >> + >> static int __init nest_pmu_init(void) >> { >> int ret = 0; >> @@ -256,6 +287,12 @@ static int __init nest_pmu_init(void) >> >> cpumask_chip(); >> >> + /* >> + * Detect the Nest PMU feature >> + */ >> + if (nest_ima_detect_parse()) >> + return 0; >> + >> return 0; >> } > Zero is returned regardless of the output of nest_ima_detect_parse. Is > that intentional? If so, do you need the 'if'? No it should return "ret" which should be initialized to error value. WIll fix it >> device_initcall(nest_pmu_init); > Regards, > Daniel Axtens > Thanks for the review MAddy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/