From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965150AbdJRFjr (ORCPT ); Wed, 18 Oct 2017 01:39:47 -0400 Received: from mail-co1nam03on0052.outbound.protection.outlook.com ([104.47.40.52]:51959 "EHLO NAM03-CO1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753143AbdJRFjn (ORCPT ); Wed, 18 Oct 2017 01:39:43 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Tomasz.Nowicki@cavium.com; Subject: Re: [PATCH v3 1/7] ACPI/PPTT: Add Processor Properties Topology Table parsing To: Jeremy Linton , linux-acpi@vger.kernel.org Cc: mark.rutland@arm.com, Jonathan.Zhang@cavium.com, Jayachandran.Nair@cavium.com, lorenzo.pieralisi@arm.com, catalin.marinas@arm.com, gregkh@linuxfoundation.org, jhugo@codeaurora.org, rjw@rjwysocki.net, linux-pm@vger.kernel.org, will.deacon@arm.com, linux-kernel@vger.kernel.org, ahs3@redhat.com, viresh.kumar@linaro.org, hanjun.guo@linaro.org, sudeep.holla@arm.com, austinwc@codeaurora.org, wangxiongfeng2@huawei.com, linux-arm-kernel@lists.infradead.org References: <20171012194856.13844-1-jeremy.linton@arm.com> <20171012194856.13844-2-jeremy.linton@arm.com> <38c63cfd-4c13-9461-0d4a-74b0943be0fd@semihalf.com> <2ce92b90-fc2c-3fff-a3e3-b9517057ab2c@arm.com> From: Tomasz Nowicki Message-ID: <4ee4af4d-d987-33f9-4d55-67bfede3ebfa@caviumnetworks.com> Date: Wed, 18 Oct 2017 07:39:25 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <2ce92b90-fc2c-3fff-a3e3-b9517057ab2c@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 8bit X-Originating-IP: [31.172.191.173] X-ClientProxiedBy: VI1PR08CA0128.eurprd08.prod.outlook.com (2603:10a6:800:d4::30) To SN4PR0701MB3663.namprd07.prod.outlook.com (2603:10b6:803:4d::17) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 65df1986-b7c2-44c9-0c38-08d515eaa00e X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(2017030254152)(2017052603199)(201703131423075)(201703031133081)(201702281549075);SRVR:SN4PR0701MB3663; X-Microsoft-Exchange-Diagnostics: 1;SN4PR0701MB3663;3:QnxQmj+ykJhMmGdp38J82lElDDadMsbJ0Y8yC5q9iqeMshdP6LQ402B8hdfVMBJBL6E1LD5Fr1g227d+nuN9yzmXtusz26A3Oea/KJFFf9is+TuVPZIjqTfwbK/thjjgs0RnW2JAhGGSXNLPwoZdrxUguas39oDNd6AgXlRu3GRKLbcoVA4gUAk7N9sxekVUoWFbOz0Rm9WUyU32HPU58yVd1xU/HGNVfcq5rV1SvFK7frmsZIXXZ5hrA4kDEQtx;25:+AmHZrCT6uB+VyTipwpzFz2bK0gjH2Muv2GTPeKBVcTSmhmV/YWfkBzkWq5OIlB7DUr3xoQOZhpGOMn241+UYYHqtb4qPAvsb0lUySGkslb+OkaEw0emrRNGmGduBVVRWqq+6sYqMws4YAuXIvoyhtJTCSrV4cafO1CeeraYzZe7B0RCXpRzJUjaYVVQthpjUQD3HEXZ30WoXKswxD0H7MWwfBEJrpqa7fzxYb9JV2390Uo2ie00GpcePTg2rZQ2kgdWRGb9UNIxfejRUFGS4mR1tFSmV/9mI4LcfX5ub4uVMQtANtf3D8FSex8UUbwvywTMehWdKb+X/ERbtCVTxw==;31:0ebLTd1C6UIXMuLTcKt1ULhSQy+7FPd16bjVtzt1M0FiZPOmm6DU9sS4rDI2YRY32IPMQNXQ3cIvjwEhIphCOF1vxiu2zhZcVZKuLxRctifFxTgQtlazrAtHPmGLQNONy/ve+YYOVrh/5u9zpqvYsMsHLSnUz7dWZkMd0R8JE3kypUgGlV3THro+9Rozyz5DsJzHIqrQ+FqfwRxnx8dJS7u37kzq4b61pSdMsgvu8Es= X-MS-TrafficTypeDiagnostic: SN4PR0701MB3663: X-Microsoft-Exchange-Diagnostics: 1;SN4PR0701MB3663;20:YzxUb817Dra7+g4N2fcdqksZnxVHwBu78y7a0iwQAC+WcD74jUDZYzUp2gGgLToeVHXLrgLMCiZiGZrgx/krP1KXScvxcHYBHjc2D7Un7h/hfRUOtS0t655Loomf+tnG8M/Vu6CerZ69lSjC3xL75lOPU5YT8dGqDy8RkBnrbr3L06k4uQdydHzNTwtMu+7YIcEtrBtzXRdibVKeRIrUYlFcK9oSMzUdkyu7Tz/F29GcG75hM57Bh4RjQqnZeyHpIBlqYDomkTPWhHd2izY/5Pb+GfnHIzqV3xBst0UHlIwYiJAUvt2XeTGYBqRiluDMHLa5hBHTpZ5mv2Nt3X61aCHUghl+nkGSrqn5e396Tso72gBM5ZLcvSLtgPmVIp5aNG5we5rSRf8sAF3lNONpVgJr+6oazQDFP31yWlplnOM6poaMU6NWLk/CYSoPBRbbh2fB3kgPCN5oRzkjqIyrqoKF4yy3OwfUysMiT+m8XVB1qnIxMYffqrBCh+3pUmNo7kUnj3UXEdKZyUS/WqLq0C5ETPgGlqo4tleMCaOaM6493PqkHT1Sack9je12bBUjj8rbB+FIhKPuPGsl5qm+c3RYuNJa8GPIDCmC2hBQYAc=;4:fcyGMxsUFb2UBfrK0kmiYnyqsUWWnjA08LEav1Zx36h7mslue5bxoMROwX9x0mP3RLH4SoD8phE0E93+0xE9zYPqT4xcrL6YIT/jKafmFw4o0j0Q1gr8ofU550rMzy2E7ltNuHTTIAOj/p5mEnMgOFjceAnD1RvUYzxXkgOf6/bWkDLRkUuc6sXgJevQuXFoRMH9Iw1WtNqw4it6vBfVocRD06rFUhth9482jEbTCYIK7ApQ9BoMzUNUoSJI7Qy0NLApwM/CHep1tZNpxBa+SZLyWDJ/oGEOXhCf2XneQsk= X-Exchange-Antispam-Report-Test: UriScan:(180628864354917); X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(8121501046)(5005006)(100000703101)(100105400095)(10201501046)(3002001)(93006095)(6041248)(20161123560025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123562025)(20161123564025)(20161123558100)(20161123555025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095);SRVR:SN4PR0701MB3663;BCL:0;PCL:0;RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);SRVR:SN4PR0701MB3663; X-Forefront-PRVS: 0464DBBBC4 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(6009001)(6049001)(346002)(376002)(189002)(377454003)(24454002)(199003)(68736007)(16526018)(36756003)(2950100002)(106356001)(66066001)(42882006)(189998001)(31686004)(5660300001)(64126003)(93886005)(83506001)(58126008)(16576012)(65956001)(7416002)(50466002)(316002)(47776003)(7736002)(101416001)(25786009)(72206003)(76176999)(97736004)(478600001)(3846002)(54356999)(31696002)(23676002)(305945005)(33646002)(8936002)(229853002)(4326008)(81166006)(81156014)(8676002)(53546010)(6486002)(53936002)(77096006)(2870700001)(6116002)(105586002)(2906002)(6246003)(6666003)(50986999)(2004002);DIR:OUT;SFP:1101;SCL:1;SRVR:SN4PR0701MB3663;H:[10.0.0.85];FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtTTjRQUjA3MDFNQjM2NjM7MjM6dit1cUVwMEpCNzhmZ2VhbXFWZEFZZzZU?= =?utf-8?B?V05odWlra3VHcmZtVkh2WURVRkZWOThTOUkyUnZUN0pNQmVuRW40ank5Vm1l?= =?utf-8?B?S1o3SXlsZmNMS3d3MEJUU0NCTElZWFdEMk5CYjRFYzFVWHozZ0d5ZEkxZXFR?= =?utf-8?B?azN1OWFmWnBpcmoyL2pzNzNBRS9Oc2xaZ3llQkF2U3RFQVFOSFVsSU1HdGNh?= =?utf-8?B?aEdhVFUvUGxkMVNGL0Y1NWEzaUJ5WEZkZW8vWTlvTXdES1oyMTluZEFHVldp?= =?utf-8?B?dlFyZS94TXVDZVM3NlpSSGdaRG9xRVNXK0srNUFiN2JpOTF0dTBEUUdTNHAr?= =?utf-8?B?SVdRVk9YMWZQZlh3VDdSNzZ5UlAzQjg4dXo4VU4yNllaUFVva1RHYVZPOFZH?= =?utf-8?B?c1hyS3RadGdzV2Q4eUd0aFkyekpyWVFsTllSNjJIcWErY29wOUlCYTN2V0Vj?= =?utf-8?B?T2w1Z1RBc2tpbzZpeTFTelhkVTlQaU13QmdKa3lveStyNWhCWWs3eFpBdDFH?= =?utf-8?B?ZTlRMXQ5UnlUZFM0NzZDTzFCYkZoaFMrWFc1YXl6ajR0cVNaN3pmTkdrQ0JF?= =?utf-8?B?VFRzVW5wZnpac3B3UU9ZazV1OW5sd0dIbk5VNThqRGVzKzBrUlNheUhzWE15?= =?utf-8?B?ay9wZDFWT21mT2puODkzSXdDNG10bTRkVm5HV3ZVUHdVUC9YYi8vVkErUm1q?= =?utf-8?B?UUMvaDlXdUxHalhFWkp0UkRNUzBvcUFuTnVnNkRWanJtMVhOaGEwajVhM01Q?= =?utf-8?B?a0ZWZUVhaEhiRk5zYXFOYm5KMWdqZmdKUVlITy94eFRXalFSdHJidWtmZDgz?= =?utf-8?B?TC85WEZTUHRuUVVWRXcwUDFwZ0hFb0Z0V2N0cnVxOVFJUjRXd3ZNTjUvUWFO?= =?utf-8?B?cVozT0F1RXQyOVdyNlpWS1ZLR0hzdzNrWkFpbkFzU2hhMmk4RGNOemxEZUI3?= =?utf-8?B?N2p6V3BYaGdUbnJMeU9odVhFTG9YY1E4V3FLYldNRlQvWS8wcTUzZW9lSk81?= =?utf-8?B?U1h0MUZuOWlSOHhBczlVN0FSRmRLQnpRaWl0VzNKT1lLdTYreWFuSS9BUWw0?= =?utf-8?B?SnBXVzBJRENLUmpwZmszbmllWDdnbDY3Yk90Tmp4R2MyazBQelFtMlVKUWFU?= =?utf-8?B?YmtpbXRKVzhPSGZsMUZ6WVV5azc5emhNeklONzZlNmRRWVRURS9ReFZndTNx?= =?utf-8?B?UzBpdUYzeWUzR1VwNUVDYXFJR290RVlwRFphanNudXJqcFR0cVlrT2xPbVRZ?= =?utf-8?B?WEU2b3VEU1ovWTl4aVN1T1BCUEVXZGg3aUN1Ym9tc3pUclZ3aUFZVEN4Nkdz?= =?utf-8?B?RG01b3JGZnlXeElIZXpHMWxuL3pJMzV4aEhZcnY0WjVHeEpjQ01CQ2JyNXJ1?= =?utf-8?B?dlczVzNKY3JjZUJVNmNrYzVVeGxtZDhTSFdSUHB4N1FHZmxPZVFoWHhjdjR0?= =?utf-8?B?NStWamZWcFZhelZTcG9NTS92ZE4wRmdzOGZXZ0dLUzdVQzByK1Y5YkZrdmI1?= =?utf-8?B?ejdDcDZKZFI3bXJiSFk3a3cyZk8xeENZS2FXeFJ6RCtUNWdNWlFqeWJCTlRF?= =?utf-8?B?bnphNlBpR3BaRjlyUy9PeHBRTGNsOWJXUVlaQmtSc090L0JpeGdmOXFFTmk2?= =?utf-8?B?OFNQUE1ZaFV3SktnL1pYUVZmTVA2TVpOV3pxUFRyNXpvTFdlZ0hGUkEzRTFV?= =?utf-8?B?dktPWVVaUmFaV2JIQmw2ZlNBam9JRG95d3FWVytvcUxvWWVDTnlNR1YrRStW?= =?utf-8?B?L25rbmtZNGI0eTdsa1d6U0xTNGRoVU04NEFrbFRVc01PS0RkdEN6dWpqSE9F?= =?utf-8?B?MGVCUXZ1SkRHL3BQUHUyY3NVbVZaL29PNHNOM1M2V283T0xhQT09?= X-Microsoft-Exchange-Diagnostics: 1;SN4PR0701MB3663;6:0X7qK+XOAl/dTW9JUfoCFqh8RyalhCGF2LYXCh44BZFwysEstAThZlx2DqGrCj5N08EPt/opcSttR50HpJdjr8GfFMjHtB6nEn3ExCCSlRTvOsU44gaKNGP79jZBx+ZxeFFKmSdgfeCgFYHIE9mWku05aFPx5P6wXezeqECAi3XbPHT0r0aYkEQE2qzUtT5pBMmOVf+9B4MYh1BQ4qNp/Aw/pX6rGpENP61ot+FUajKzK2RsCdYf2/tsk4QjecnMYfevxb5RBy1HM9LGJbtPRTV8jP7fZk7g5KHFoRk7zdfOD/crmf7AAjIzJbhzW6Q5YEjPPXWhiQ7fP5P+Wud7GA==;5:QEL+6O6d9lTfwXAgDaNxmhLlj1uVov+lDHytavywKuSySKgQ0nULGDhr75QWyITmQHj4PRbBLxKDD3cd6am2WelIDGl1VsQKC0BOrmy4QxAp//Tuk98CzC1LDAtxgUg8wB6XMwBGQMzcwWDIiHRXrQ==;24:0GvZvBD6/eyNfJ/OT3qjTVEK8PDFCJNteJXIayGJiTpY4h/T/Wb8PzVvZIob6wyeavC/2hFRBzHPd82njeFI5ia9nrnsp4B5xs+aHOsm6Ko=;7:nDyIG8AddBC4sBDbfZZMPK17XOQiGbS10udqQ/XysWHGNCpJ66ki1mKUmaSzLl3GLDsh4jaT5g4EbL1hpm2gxcslur6vbGnphEDzwfGEQpDXUGIYwAtJeGh1Y3UR9FrgZ6hx5r4neNw0Vikd8LTdQoHzW9adCpLA3dioe/gflnVfhkhMOxv8DUec+3XdKJCYWa/46DCG0NaT7icskbMzSf5u6IEP8AkrIBF7XRRUft0= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: caviumnetworks.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Oct 2017 05:39:35.9598 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 711e4ccf-2e9b-4bcf-a551-4094005b6194 X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN4PR0701MB3663 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 17.10.2017 17:22, Jeremy Linton wrote: > Hi, > > On 10/17/2017 08:25 AM, Tomasz Nowicki wrote: >> Hi Jeremy, >> >> I did second round of review and have some more comments, please see >> below: >> >> On 12.10.2017 21:48, Jeremy Linton wrote: >>> ACPI 6.2 adds a new table, which describes how processing units >>> are related to each other in tree like fashion. Caches are >>> also sprinkled throughout the tree and describe the properties >>> of the caches in relation to other caches and processing units. >>> >>> Add the code to parse the cache hierarchy and report the total >>> number of levels of cache for a given core using >>> acpi_find_last_cache_level() as well as fill out the individual >>> cores cache information with cache_setup_acpi() once the >>> cpu_cacheinfo structure has been populated by the arch specific >>> code. >>> >>> Further, report peers in the topology using setup_acpi_cpu_topology() >>> to report a unique ID for each processing unit at a given level >>> in the tree. These unique id's can then be used to match related >>> processing units which exist as threads, COD (clusters >>> on die), within a given package, etc. >>> >>> Signed-off-by: Jeremy Linton >>> --- >>>   drivers/acpi/pptt.c | 485 >>> ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>   1 file changed, 485 insertions(+) >>>   create mode 100644 drivers/acpi/pptt.c >>> >>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c >>> new file mode 100644 >>> index 000000000000..c86715fed4a7 >>> --- /dev/null >>> +++ b/drivers/acpi/pptt.c >>> @@ -0,1 +1,485 @@ >>> +/* >>> + * Copyright (C) 2017, ARM >>> + * >>> + * This program is free software; you can redistribute it and/or >>> modify it >>> + * under the terms and conditions of the GNU General Public License, >>> + * version 2, as published by the Free Software Foundation. >>> + * >>> + * This program is distributed in the hope it will be useful, but >>> WITHOUT >>> + * ANY WARRANTY; without even the implied warranty of >>> MERCHANTABILITY or >>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public >>> License for >>> + * more details. >>> + * >>> + * This file implements parsing of Processor Properties Topology >>> Table (PPTT) >>> + * which is optionally used to describe the processor and cache >>> topology. >>> + * Due to the relative pointers used throughout the table, this doesn't >>> + * leverage the existing subtable parsing in the kernel. >>> + */ >>> +#define pr_fmt(fmt) "ACPI PPTT: " fmt >>> + >>> +#include >>> +#include >>> +#include >>> + >>> +/* >>> + * Given the PPTT table, find and verify that the subtable entry >>> + * is located within the table >>> + */ >>> +static struct acpi_subtable_header *fetch_pptt_subtable( >>> +    struct acpi_table_header *table_hdr, u32 pptt_ref) >>> +{ >>> +    struct acpi_subtable_header *entry; >>> + >>> +    /* there isn't a subtable at reference 0 */ >>> +    if (!pptt_ref) >>> +        return NULL; >>> + >>> +    if (pptt_ref + sizeof(struct acpi_subtable_header) > >>> table_hdr->length) >>> +        return NULL; >>> + >>> +    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + >>> pptt_ref); >>> + >>> +    if (pptt_ref + entry->length > table_hdr->length) >>> +        return NULL; >>> + >>> +    return entry; >>> +} >>> + >>> +static struct acpi_pptt_processor *fetch_pptt_node( >>> +    struct acpi_table_header *table_hdr, u32 pptt_ref) >>> +{ >>> +    return (struct acpi_pptt_processor >>> *)fetch_pptt_subtable(table_hdr, pptt_ref); >>> +} >>> + >>> +static struct acpi_pptt_cache *fetch_pptt_cache( >>> +    struct acpi_table_header *table_hdr, u32 pptt_ref) >>> +{ >>> +    return (struct acpi_pptt_cache *)fetch_pptt_subtable(table_hdr, >>> pptt_ref); >>> +} >>> + >>> +static struct acpi_subtable_header *acpi_get_pptt_resource( >>> +    struct acpi_table_header *table_hdr, >>> +    struct acpi_pptt_processor *node, int resource) >>> +{ >>> +    u32 ref; >>> + >>> +    if (resource >= node->number_of_priv_resources) >>> +        return NULL; >>> + >>> +    ref = *(u32 *)((u8 *)node + sizeof(struct acpi_pptt_processor) + >>> +              sizeof(u32) * resource); >>> + >>> +    return fetch_pptt_subtable(table_hdr, ref); >>> +} >>> + >>> +/* >>> + * given a pptt resource, verify that it is a cache node, then walk >>> + * down each level of caches, counting how many levels are found >>> + * as well as checking the cache type (icache, dcache, unified). If a >>> + * level & type match, then we set found, and continue the search. >>> + * Once the entire cache branch has been walked return its max >>> + * depth. >>> + */ >>> +static int acpi_pptt_walk_cache(struct acpi_table_header *table_hdr, >>> +                int local_level, >>> +                struct acpi_subtable_header *res, >>> +                struct acpi_pptt_cache **found, >>> +                int level, int type) >>> +{ >>> +    struct acpi_pptt_cache *cache; >>> + >>> +    if (res->type != ACPI_PPTT_TYPE_CACHE) >>> +        return 0; >>> + >>> +    cache = (struct acpi_pptt_cache *) res; >>> +    while (cache) { >>> +        local_level++; >>> + >>> +        if ((local_level == level) && >>> +            (cache->flags & ACPI_PPTT_CACHE_TYPE_VALID) && >>> +            ((cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) == >>> type)) { >> >> Attributes have to be shifted: >> >> (cache->attributes & ACPI_PPTT_MASK_CACHE_TYPE) >> 2 > > Hmmm, I'm not sure that is true, the top level function in this routine > convert the "linux" constant to the ACPI version of that constant. In > that case the "type" field is pre-shifted, so that it matches the result > of just anding against the field... That is unless I messed something > up, which I don't see at the moment (and the code of course has been > tested with PPTT's from multiple people at this point). For ThunderX2 I got lots of errors in dmesg: Found duplicate cache level/type unable to determine uniqueness So I fixed "type" macros definitions (without shifting) and shift it here which fixes the issue. As you said, it can be pre-shifted as well. > > >> >>> +            if (*found != NULL) >>> +                pr_err("Found duplicate cache level/type unable to >>> determine uniqueness\n"); >>> + >>> +            pr_debug("Found cache @ level %d\n", level); >>> +            *found = cache; >>> +            /* >>> +             * continue looking at this node's resource list >>> +             * to verify that we don't find a duplicate >>> +             * cache node. >>> +             */ >>> +        } >>> +        cache = fetch_pptt_cache(table_hdr, >>> cache->next_level_of_cache); >>> +    } >>> +    return local_level; >>> +} >>> + >>> +/* >>> + * Given a CPU node look for cache levels that exist at this level, >>> and then >>> + * for each cache node, count how many levels exist below (logically >>> above) it. >>> + * If a level and type are specified, and we find that level/type, >>> abort >>> + * processing and return the acpi_pptt_cache structure. >>> + */ >>> +static struct acpi_pptt_cache *acpi_find_cache_level( >>> +    struct acpi_table_header *table_hdr, >>> +    struct acpi_pptt_processor *cpu_node, >>> +    int *starting_level, int level, int type) >>> +{ >>> +    struct acpi_subtable_header *res; >>> +    int number_of_levels = *starting_level; >>> +    int resource = 0; >>> +    struct acpi_pptt_cache *ret = NULL; >>> +    int local_level; >>> + >>> +    /* walk down from the processor node */ >>> +    while ((res = acpi_get_pptt_resource(table_hdr, cpu_node, >>> resource))) { >>> +        resource++; >>> + >>> +        local_level = acpi_pptt_walk_cache(table_hdr, *starting_level, >>> +                           res, &ret, level, type); >>> +        /* >>> +         * we are looking for the max depth. Since its potentially >>> +         * possible for a given node to have resources with differing >>> +         * depths verify that the depth we have found is the largest. >>> +         */ >>> +        if (number_of_levels < local_level) >>> +            number_of_levels = local_level; >>> +    } >>> +    if (number_of_levels > *starting_level) >>> +        *starting_level = number_of_levels; >>> + >>> +    return ret; >>> +} >>> + >>> +/* >>> + * given a processor node containing a processing unit, walk into it >>> and count >>> + * how many levels exist solely for it, and then walk up each level >>> until we hit >>> + * the root node (ignore the package level because it may be >>> possible to have >>> + * caches that exist across packages). Count the number of cache >>> levels that >>> + * exist at each level on the way up. >>> + */ >>> +static int acpi_process_node(struct acpi_table_header *table_hdr, >>> +                 struct acpi_pptt_processor *cpu_node) >>> +{ >>> +    int total_levels = 0; >>> + >>> +    do { >>> +        acpi_find_cache_level(table_hdr, cpu_node, &total_levels, 0, >>> 0); >>> +        cpu_node = fetch_pptt_node(table_hdr, cpu_node->parent); >>> +    } while (cpu_node); >>> + >>> +    return total_levels; >>> +} >>> + >>> +/* determine if the given node is a leaf node */ >>> +static int acpi_pptt_leaf_node(struct acpi_table_header *table_hdr, >>> +                   struct acpi_pptt_processor *node) >>> +{ >>> +    struct acpi_subtable_header *entry; >>> +    unsigned long table_end; >>> +    u32 node_entry; >>> +    struct acpi_pptt_processor *cpu_node; >>> + >>> +    table_end = (unsigned long)table_hdr + table_hdr->length; >>> +    node_entry = (u32)((u8 *)node - (u8 *)table_hdr); >>> +    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + >>> +                        sizeof(struct acpi_table_pptt)); >>> + >>> +    while (((unsigned long)entry) + sizeof(struct >>> acpi_subtable_header) < table_end) { >>> +        cpu_node = (struct acpi_pptt_processor *)entry; >>> +        if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) && >>> +            (cpu_node->parent == node_entry)) >>> +            return 0; >>> +        entry = (struct acpi_subtable_header *)((u8 *)entry + >>> entry->length); >>> +    } >>> +    return 1; >>> +} >>> + >>> +/* >>> + * Find the subtable entry describing the provided processor >>> + */ >>> +static struct acpi_pptt_processor *acpi_find_processor_node( >>> +    struct acpi_table_header *table_hdr, >>> +    u32 acpi_cpu_id) >>> +{ >>> +    struct acpi_subtable_header *entry; >>> +    unsigned long table_end; >>> +    struct acpi_pptt_processor *cpu_node; >>> + >>> +    table_end = (unsigned long)table_hdr + table_hdr->length; >>> +    entry = (struct acpi_subtable_header *)((u8 *)table_hdr + >>> +                        sizeof(struct acpi_table_pptt)); >>> + >>> +    /* find the processor structure associated with this cpuid */ >>> +    while (((unsigned long)entry) + sizeof(struct >>> acpi_subtable_header) < table_end) { >>> +        cpu_node = (struct acpi_pptt_processor *)entry; >>> + >>> +        if ((entry->type == ACPI_PPTT_TYPE_PROCESSOR) && >>> +            acpi_pptt_leaf_node(table_hdr, cpu_node)) { >>> +            pr_debug("checking phy_cpu_id %d against acpi id %d\n", >>> +                 acpi_cpu_id, cpu_node->acpi_processor_id); >>> +            if (acpi_cpu_id == cpu_node->acpi_processor_id) { >>> +                /* found the correct entry */ >>> +                pr_debug("match found!\n"); >>> +                return (struct acpi_pptt_processor *)entry; >>> +            } >>> +        } >>> + >>> +        if (entry->length == 0) { >>> +            pr_err("Invalid zero length subtable\n"); >>> +            break; >>> +        } >>> +        entry = (struct acpi_subtable_header *) >>> +            ((u8 *)entry + entry->length); >>> +    } >>> + >>> +    return NULL; >>> +} >>> + >>> +/* >>> + * Given a acpi_pptt_processor node, walk up until we identify the >>> + * package that the node is associated with or we run out of levels >>> + * to request. >>> + */ >>> +static struct acpi_pptt_processor *acpi_find_processor_package_id( >>> +    struct acpi_table_header *table_hdr, >>> +    struct acpi_pptt_processor *cpu, >>> +    int level) >>> +{ >>> +    struct acpi_pptt_processor *prev_node; >>> + >>> +    while (cpu && level && !(cpu->flags & >>> ACPI_PPTT_PHYSICAL_PACKAGE)) { >>> +        pr_debug("level %d\n", level); >>> +        prev_node = fetch_pptt_node(table_hdr, cpu->parent); >>> +        if (prev_node == NULL) >>> +            break; >>> +        cpu = prev_node; >>> +        level--; >>> +    } >>> +    return cpu; >>> +} >>> + >>> +static int acpi_parse_pptt(struct acpi_table_header *table_hdr, u32 >>> acpi_cpu_id) >> >> The function name can be more descriptive. How about: >> >> acpi_count_cache_level() ? > > The naming has drifted a bit, so yes, that routine is only used by the > portion which is determining the number of cache levels for a given PE. > > >> >>> +{ >>> +    int number_of_levels = 0; >>> +    struct acpi_pptt_processor *cpu; >>> + >>> +    cpu = acpi_find_processor_node(table_hdr, acpi_cpu_id); >>> +    if (cpu) >>> +        number_of_levels = acpi_process_node(table_hdr, cpu); >>> + >>> +    return number_of_levels; >>> +} >> >> It is hard to follow what acpi_find_cache_level() and >> acpi_pptt_walk_cache() really do. It is because they are trying to do >> too many things at the same time. IMO, splitting >> acpi_find_cache_level() logic to: >> 1. counting the cache levels (max depth) >> 2. finding the specific cache node >> makes sense. > > I disagree, that routine is shared by the two code paths because its > functionality is 99% duplicated between the two. The difference being > whether it terminates the search at a given level, or continues > searching until it runs out of nodes. The latter case is simply a > degenerate version of the first. Mostly it is about trade-off between code simplicity and redundancy, I personally prefer the former. It is not the critical issue though. > > >> >> Also, seems like we can merge acpi_parse_pptt() & acpi_process_node(). > > That is true, but I fail to see how any of this is actually fixes > anything. There are a million ways to do this, including as pointed out > by building another data-structure to simplify the parsing what is a > table that is less than ideal for runtime parsing (starting with the > direction of the relative pointers, and ending with having to "infer" > information that isn't directly flagged). I actually built a couple > other versions of this, including a nice cute version which is about 1/8 > this size of this and really easy to understand but of course is > recursive... I believe this will improve code readability. Obviously, you can disagree with my suggestions. Thanks, Tomasz