[{"id":1731203,"web_url":"http://patchwork.ozlabs.org/comment/1731203/","msgid":"<3bd82a3f-ea68-3e6e-1a0f-19b7753e8621@linux.vnet.ibm.com>","date":"2017-07-27T18:25:23","subject":"Re: [PATCH 2/4] pseries/drc-info: Search DRC properties for CPU\n\tindexes","submitter":{"id":17146,"url":"http://patchwork.ozlabs.org/api/people/17146/","name":"Nathan Fontenot","email":"nfont@linux.vnet.ibm.com"},"content":"On 07/27/2017 11:10 AM, Michael Bringmann wrote:\n> \n> pseries/drc-info: Provide parallel routines to convert between\n> drc_index and CPU numbers at runtime, using the older device-tree\n> properties (\"ibm,drc-indexes\", \"ibm,drc-names\", \"ibm,drc-types\"\n> and \"ibm,drc-power-domains\"), or the new property \"ibm,drc-info\".\n> \n> Signed-off-by: Michael Bringmann \n> ---\n> arch/powerpc/include/asm/prom.h | 7 +\n> arch/powerpc/platforms/pseries/of_helpers.c | 79 ++++++++++++\n> arch/powerpc/platforms/pseries/pseries_energy.c | 150 +++++++++++++++++++----\n> 3 files changed, 210 insertions(+), 26 deletions(-)\n> ---\n> Changes in V2:\n> -- Minor changes to integrate to latest 4.13 code\n> \n> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h\n> index 4fb02cc..d469d7c 100644\n> --- a/arch/powerpc/include/asm/prom.h\n> +++ b/arch/powerpc/include/asm/prom.h\n> @@ -96,6 +96,13 @@ struct of_drconf_cell {\n> #define DRCONF_MEM_AI_INVALID\t0x00000040\n> #define DRCONF_MEM_RESERVED\t0x00000080\n> \n> +extern int of_one_drc_info(struct property **prop, void **curval,\n> +\t\t\tchar **dtype, char **dname,\n> +\t\t\tu32 *drc_index_start_p,\n> +\t\t\tu32 *num_sequential_elems_p,\n> +\t\t\tu32 *sequential_inc_p,\n> +\t\t\tu32 *last_drc_index_p);\n> +\n> /*\n> * There are two methods for telling firmware what our capabilities are.\n> * Newer machines have an \"ibm,client-architecture-support\" method on the\n> diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c\n> index 2798933..1d59939 100644\n> --- a/arch/powerpc/platforms/pseries/of_helpers.c\n> +++ b/arch/powerpc/platforms/pseries/of_helpers.c\n> @@ -36,3 +36,82 @@ struct device_node *pseries_of_derive_parent(const char *path)\n> \t\tkfree(parent_path);\n> \treturn parent ? parent : ERR_PTR(-EINVAL);\n> }\n> +\n> +\n> +/* Helper Routines to convert between drc_index to cpu numbers */\n> +\n> +int of_one_drc_info(struct property **prop, void **curval,\n> +\t\t\tchar **dtype, char **dname,\n> +\t\t\tu32 *drc_index_start_p,\n> +\t\t\tu32 *num_sequential_elems_p,\n> +\t\t\tu32 *sequential_inc_p,\n> +\t\t\tu32 *last_drc_index_p)\n> +{\n> +\tchar *drc_type, *drc_name_prefix;\n> +\tu32 drc_index_start, num_sequential_elems, dummy;\n> +\tu32 sequential_inc, last_drc_index;\n> +\tconst char *p;\n> +\tconst __be32 *p2;\n> +\n> +\tdrc_index_start = num_sequential_elems = 0;\n> +\tsequential_inc = last_drc_index = 0;\n> +\n> +\t/* Get drc-type:encode-string */\n> +\tp = drc_type = (*curval);\n> +\tp = of_prop_next_string(*prop, p);\n> +\tif (!p)\n> +\t\treturn -EINVAL;\n> +\n> +\t/* Get drc-name-prefix:encode-string */\n> +\tdrc_name_prefix = (char *)p;\n> +\tp = of_prop_next_string(*prop, p);\n> +\tif (!p)\n> +\t\treturn -EINVAL;\n> +\n> +\t/* Get drc-index-start:encode-int */\n> +\tp2 = (const __be32 *)p;\n> +\tp2 = of_prop_next_u32(*prop, p2, &drc_index_start);\n> +\tif (!p2)\n> +\t\treturn -EINVAL;\n> +\n> +\t/* Get/skip drc-name-suffix-start:encode-int */\n\nWhy are we skipping the drc name suffix start value? It seems this\nwould make the routine unusable for anyone wanting to get drc-name\nvalues.\n\n> +\tp2 = of_prop_next_u32(*prop, p2, &dummy);\n> +\tif (!p)\n\nshouldn't this be checking p2?\n\n> +\t\treturn -EINVAL;\n> +\n> +\t/* Get number-sequential-elements:encode-int */\n> +\tp2 = of_prop_next_u32(*prop, p2, &num_sequential_elems);\n> +\tif (!p2)\n> +\t\treturn -EINVAL;\n> +\n> +\t/* Get sequential-increment:encode-int */\n> +\tp2 = of_prop_next_u32(*prop, p2, &sequential_inc);\n> +\tif (!p2)\n> +\t\treturn -EINVAL;\n> +\n> +\t/* Get/skip drc-power-domain:encode-int */\n\nSame for power-domain, why skip it?\n\nI don't think any parts of the kernel are currently\nlooking at this piece of the DRC information, but if we are\ngoing to have a routine to return all the data for a drc-info\nblock shouldn't it return all of it?\n\nWould it be easier if the routine were to return a drc_info struct with\npointers to the string values and int values read into it?\n\n> +\tp2 = of_prop_next_u32(*prop, p2, &dummy);\n> +\tif (!p2)\n> +\t\treturn -EINVAL;\n> +\n> +\t/* Should now know end of current entry */\n> +\t(*curval) = (void *)p2;\n> +\tlast_drc_index = drc_index_start +\n> +\t\t\t((num_sequential_elems-1)*sequential_inc);\n> +\n> +\tif (dtype)\n> +\t\t*dtype = drc_type;\n> +\tif (dname)\n> +\t\t*dname = drc_name_prefix;\n> +\tif (drc_index_start_p)\n> +\t\t*drc_index_start_p = drc_index_start;\n> +\tif (num_sequential_elems_p)\n> +\t\t*num_sequential_elems_p = num_sequential_elems;\n> +\tif (sequential_inc_p)\n> +\t\t*sequential_inc_p = sequential_inc;\n> +\tif (last_drc_index_p)\n> +\t\t*last_drc_index_p = last_drc_index;\n> +\n> +\treturn 0;\n> +}\n> +EXPORT_SYMBOL(of_one_drc_info);\n> diff --git a/arch/powerpc/platforms/pseries/pseries_energy.c b/arch/powerpc/platforms/pseries/pseries_energy.c\n> index 164a13d..865c2af 100644\n> --- a/arch/powerpc/platforms/pseries/pseries_energy.c\n> +++ b/arch/powerpc/platforms/pseries/pseries_energy.c\n> @@ -22,6 +22,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> \n> #define MODULE_VERS \"1.0\"\n> @@ -38,7 +39,6 @@\n> static u32 cpu_to_drc_index(int cpu)\n> {\n> \tstruct device_node *dn = NULL;\n> -\tconst int *indexes;\n> \tint i;\n> \tint rc = 1;\n> \tu32 ret = 0;\n> @@ -46,18 +46,65 @@ static u32 cpu_to_drc_index(int cpu)\n> \tdn = of_find_node_by_path(\"/cpus\");\n> \tif (dn == NULL)\n> \t\tgoto err;\n> -\tindexes = of_get_property(dn, \"ibm,drc-indexes\", NULL);\n> -\tif (indexes == NULL)\n> -\t\tgoto err_of_node_put;\n> +\n> \t/* Convert logical cpu number to core number */\n> \ti = cpu_core_index_of_thread(cpu);\n> -\t/*\n> -\t * The first element indexes[0] is the number of drc_indexes\n> -\t * returned in the list. Hence i+1 will get the drc_index\n> -\t * corresponding to core number i.\n> -\t */\n> -\tWARN_ON(i > indexes[0]);\n> -\tret = indexes[i + 1];\n> +\n> +\tif (firmware_has_feature(FW_FEATURE_DRC_INFO)) {\n> +\t\tstruct property *info = NULL;\n> +\t\tint j, check_val = i;\n\nWhy use check_val here? it seems a bit confusing.\n\nI think perhaps renaming 'i' to thread_index and using that everywhere\nmay help the readability.\n\n> +\t\tu32 num_set_entries;\n> +\t\tu32 drc_index_start = 0;\n> +\t\tu32 last_drc_index = 0;\n> +\t\tu32 num_sequential_elems = 0;\n> +\t\tu32 sequential_inc = 0;\n> +\t\tchar *dtype;\n> +\t\tchar *dname;\n> +\t\tvoid *value;\n> +\n> +\t\tinfo = of_find_property(dn, \"ibm,drc-info\", NULL);\n> +\t\tif (info == NULL)\n> +\t\t\tgoto err_of_node_put;\n> +\n> +\t\tvalue = info->value;\n> +\t\tvalue = (void *)of_prop_next_u32(info, value, &num_set_entries);\n> +\t\tif (!value)\n> +\t\t\tgoto err_of_node_put;\n> +\n> +\t\tfor (j = 0; j < num_set_entries; j++) {\n> +\n> +\t\t\tof_one_drc_info(&info, &value, &dtype, &dname,\n> +\t\t\t\t\t&drc_index_start,\n> +\t\t\t\t\t&num_sequential_elems,\n> +\t\t\t\t\t&sequential_inc, &last_drc_index);\n> +\t\t\tif (strcmp(dtype, \"CPU\"))\n\nPerhaps use strncmp() here.\n\n> +\t\t\t\tgoto err;\n> +\n> +\t\t\tif (check_val < last_drc_index)\n\nSo, check_val here is just the thread_index for the cpu we are looking for.\nI think this value will likely always be less than last_drc_index. On systems\nI have always seen the drc-index values for CPUs start at 0x10000000 and\nincrement from there.\n\nIf there is ever more than one set of drc-info blocks in the drc-info\nproperty this check would not be valid.\n\n> +\t\t\t\tbreak;\n> +\n> +\t\t\tWARN_ON(((check_val-drc_index_start)%\n> +\t\t\t\t\tsequential_inc) != 0);\n> +\t\t}\n> +\t\tWARN_ON((num_sequential_elems == 0) || (sequential_inc == 0));\n> +\n> +\t\tret = last_drc_index + (check_val*sequential_inc);\n\nShouldn't this be drc_index_start?\n\n> +\t} else {\n> +\t\tconst __be32 *indexes;\n> +\n> +\t\tindexes = of_get_property(dn, \"ibm,drc-indexes\", NULL);\n> +\t\tif (indexes == NULL)\n> +\t\t\tgoto err_of_node_put;\n> +\n> +\t\t/*\n> +\t\t * The first element indexes[0] is the number of drc_indexes\n> +\t\t * returned in the list. Hence i+1 will get the drc_index\n> +\t\t * corresponding to core number i.\n> +\t\t */\n> +\t\tWARN_ON(i > indexes[0]);\n> +\t\tret = indexes[i + 1];\n> +\t}\n> +\n> \trc = 0;\n> \n> err_of_node_put:\n> @@ -72,34 +119,85 @@ static int drc_index_to_cpu(u32 drc_index)\n> {\n> \tstruct device_node *dn = NULL;\n> \tconst int *indexes;\n> -\tint i, cpu = 0;\n> +\tint thread = 0, cpu = 0;\n> \tint rc = 1;\n> \n> \tdn = of_find_node_by_path(\"/cpus\");\n> \tif (dn == NULL)\n> \t\tgoto err;\n> -\tindexes = of_get_property(dn, \"ibm,drc-indexes\", NULL);\n> -\tif (indexes == NULL)\n> -\t\tgoto err_of_node_put;\n> -\t/*\n> -\t * First element in the array is the number of drc_indexes\n> -\t * returned. Search through the list to find the matching\n> -\t * drc_index and get the core number\n> -\t */\n> -\tfor (i = 0; i < indexes[0]; i++) {\n> -\t\tif (indexes[i + 1] == drc_index)\n> +\n> +\tif (firmware_has_feature(FW_FEATURE_DRC_INFO)) {\n> +\t\tstruct property *info = NULL;\n> +\t\tint j;\n> +\t\tu32 num_set_entries;\n> +\t\tu32 drc_index_start = 0;\n> +\t\tu32 last_drc_index = 0;\n> +\t\tu32 num_sequential_elems = 0;\n> +\t\tu32 sequential_inc = 0;\n> +\t\tchar *dtype, *dname;\n> +\t\tvoid *value;\n> +\n> +\t\tinfo = of_find_property(dn, \"ibm,drc-info\", NULL);\n> +\t\tif (info == NULL)\n> +\t\t\tgoto err_of_node_put;\n> +\n> +\t\tvalue = info->value;\n> +\t\tvalue = (void *)of_prop_next_u32(info, value, &num_set_entries);\n> +\t\tif (!value)\n> +\t\t\tgoto err_of_node_put;\n> +\n> +\t\tfor (j = 0; j < num_set_entries; j++) {\n> +\n> +\t\t\tof_one_drc_info(&info, &value, &dtype, &dname,\n> +\t\t\t\t\t&drc_index_start,\n> +\t\t\t\t\t&num_sequential_elems,\n> +\t\t\t\t\t&sequential_inc, &last_drc_index);\n> +\t\t\tif (strcmp(dtype, \"CPU\"))\n> +\t\t\t\tgoto err;\n> +\n> +\t\t\tWARN_ON(drc_index < drc_index_start);\n> +\t\t\tWARN_ON(((drc_index-drc_index_start)%\n> +\t\t\t\t\tsequential_inc) != 0);\n> +\n> +\t\t\tif (drc_index > last_drc_index) {\n> +\t\t\t\tcpu += ((last_drc_index-drc_index_start)/\n> +\t\t\t\t\tsequential_inc);\n\nCould we just use num_sequential_elems here?\n\n\t\t\t\tcpu += num_sequential_elems;\n\n\n-Nathan\n\n> +\t\t\t\tcontinue;\n> +\t\t\t} else {\n> +\t\t\t\tcpu += ((drc_index-drc_index_start)/\n> +\t\t\t\t\tsequential_inc);\n> +\t\t\t}\n> +\n> +\t\t\tthread = cpu_first_thread_of_core(cpu);\n> +\t\t\trc = 0;\n> \t\t\tbreak;\n> +\t\t}\n> +\t} else {\n> +\t\tunsigned long int i;\n> +\n> +\t\tindexes = of_get_property(dn, \"ibm,drc-indexes\", NULL);\n> +\t\tif (indexes == NULL)\n> +\t\t\tgoto err_of_node_put;\n> +\t\t/*\n> +\t\t * First element in the array is the number of drc_indexes\n> +\t\t * returned. Search through the list to find the matching\n> +\t\t * drc_index and get the core number\n> +\t\t */\n> +\t\tfor (i = 0; i < indexes[0]; i++) {\n> +\t\t\tif (indexes[i + 1] == drc_index)\n> +\t\t\t\tbreak;\n> +\t\t}\n> +\t\t/* Convert core number to logical cpu number */\n> +\t\tthread = cpu_first_thread_of_core(i);\n> +\t\trc = 0;\n> \t}\n> -\t/* Convert core number to logical cpu number */\n> -\tcpu = cpu_first_thread_of_core(i);\n> -\trc = 0;\n> \n> err_of_node_put:\n> \tof_node_put(dn);\n> err:\n> \tif (rc)\n> \t\tprintk(KERN_WARNING \"drc_index_to_cpu(%d) failed\", drc_index);\n> -\treturn cpu;\n> +\treturn thread;\n> }\n> \n> /*\n>","headers":{"Return-Path":"","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xJL6k3LdQz9s75\n\tfor ;\n\tFri, 28 Jul 2017 04:27:10 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xJL6k2FJ0zDrPf\n\tfor ;\n\tFri, 28 Jul 2017 04:27:10 +1000 (AEST)","from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com\n\t[148.163.156.1])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xJL4s3C5NzDrLK\n\tfor ;\n\tFri, 28 Jul 2017 04:25:33 +1000 (AEST)","from pps.filterd (m0098399.ppops.net [127.0.0.1])\n\tby mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id\n\tv6RIOIuV091411\n\tfor ; Thu, 27 Jul 2017 14:25:28 -0400","from e38.co.us.ibm.com (e38.co.us.ibm.com [32.97.110.159])\n\tby mx0a-001b2d01.pphosted.com with ESMTP id 2byjkxquqq-1\n\t(version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT)\n\tfor ; Thu, 27 Jul 2017 14:25:28 -0400","from localhost\n\tby e38.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use\n\tOnly! Violators will be prosecuted\n\tfor from ; \n\tThu, 27 Jul 2017 12:25:27 -0600","from b03cxnp08028.gho.boulder.ibm.com (9.17.130.20)\n\tby e38.co.us.ibm.com (192.168.1.138) with IBM ESMTP SMTP Gateway:\n\tAuthorized Use Only! Violators will be prosecuted; \n\tThu, 27 Jul 2017 12:25:24 -0600","from b03ledav004.gho.boulder.ibm.com\n\t(b03ledav004.gho.boulder.ibm.com [9.17.130.235])\n\tby b03cxnp08028.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with\n\tESMTP id v6RIPOto32374842; Thu, 27 Jul 2017 11:25:24 -0700","from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1])\n\tby IMSVA (Postfix) with ESMTP id 0721F78038;\n\tThu, 27 Jul 2017 12:25:24 -0600 (MDT)","from [9.41.92.186] (unknown [9.41.92.186])\n\tby b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTP id A899E78037;\n\tThu, 27 Jul 2017 12:25:23 -0600 (MDT)"],"Subject":"Re: [PATCH 2/4] pseries/drc-info: Search DRC properties for CPU\n\tindexes","To":"Michael Bringmann , linuxppc-dev@lists.ozlabs.org","References":"<67551d00-5e79-8839-2040-81949f71ef3a@linux.vnet.ibm.com>","From":"Nathan Fontenot ","Date":"Thu, 27 Jul 2017 13:25:23 -0500","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<67551d00-5e79-8839-2040-81949f71ef3a@linux.vnet.ibm.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-TM-AS-GCONF":"00","x-cbid":"17072718-0028-0000-0000-00000819B88B","X-IBM-SpamModules-Scores":"","X-IBM-SpamModules-Versions":"BY=3.00007436; HX=3.00000241; KW=3.00000007;\n\tPH=3.00000004; SC=3.00000214; SDB=6.00893745; UDB=6.00446846;\n\tIPR=6.00673906; \n\tBA=6.00005495; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009;\n\tZB=6.00000000; \n\tZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00016411;\n\tXFM=3.00000015; UTC=2017-07-27 18:25:25","X-IBM-AV-DETECTION":"SAVI=unused REMOTE=unused XFE=unused","x-cbparentid":"17072718-0029-0000-0000-000036E051A8","Message-Id":"<3bd82a3f-ea68-3e6e-1a0f-19b7753e8621@linux.vnet.ibm.com>","X-Proofpoint-Virus-Version":"vendor=fsecure engine=2.50.10432:, ,\n\tdefinitions=2017-07-27_10:, , signatures=0","X-Proofpoint-Spam-Details":"rule=outbound_notspam policy=outbound score=0\n\tspamscore=0 suspectscore=0\n\tmalwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam\n\tadjust=0 reason=mlx scancount=1 engine=8.0.1-1706020000\n\tdefinitions=main-1707270287","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List\n\t","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n\t"}}]