From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 432622E11B3 for ; Thu, 10 Jul 2025 11:15:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752146115; cv=none; b=r8w35RhI31ACxQ1AUJ/IVye3SW244+O82a1NngMmYWTG8Zx7BHOCEwVQv51FNyBoDj6RgcRX0J/+4wODAHA87yxPA+npoWQavRPcJir2MzTueEdRPTLLTwA++Ybc4qp6rXcFkYPKfZg+iC1TNUkGKWsJ48HJMzhCNTmYnGt992M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752146115; c=relaxed/simple; bh=/KNvUT70/P8YpgaATRZxKoGeGpydTeBdGvjDmlHFXSo=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=mhQ4IV3xu2oo9EyfJtKxXEgiCrWsEKKvg5Ng3d7x40kVI1GQJ0ZzfW0ao2M4nWwlGFgD1Lb3AbnoH2Rl6jPlYxLh5l+7bCNFrTL0dZnuZWXWjynXBX7zpMMV4eViC6G+kVERRa2AknfC7bhuiwUSgWGO7Kq7S81lNvD6QFTxwPk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 1AFFF1EA6; Thu, 10 Jul 2025 04:15:01 -0700 (PDT) Received: from [10.1.197.43] (eglon.cambridge.arm.com [10.1.197.43]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id ABD8D3F66E; Thu, 10 Jul 2025 04:15:09 -0700 (PDT) Message-ID: <89c6f2a2-e084-4899-a6d6-819917eb6324@arm.com> Date: Thu, 10 Jul 2025 12:15:08 +0100 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/3] cacheinfo: Set cache 'id' based on DT data To: Jonathan Cameron , Ben Horgan Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Greg Kroah-Hartman , "Rafael J . Wysocki" , sudeep.holla@arm.com, Rob Herring , Catalin Marinas , WillDeaconwill@kernel.org References: <20250704173826.13025-1-james.morse@arm.com> <20250704173826.13025-2-james.morse@arm.com> <9495df36-053e-49a3-8046-1e6aed63b4af@arm.com> <20250707133207.00001b88@huawei.com> Content-Language: en-GB From: James Morse In-Reply-To: <20250707133207.00001b88@huawei.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi Ben, Jonathan, On 07/07/2025 13:32, Jonathan Cameron wrote: > On Mon, 7 Jul 2025 11:27:06 +0100 > Ben Horgan wrote: >> On 7/4/25 18:38, James Morse wrote: >>> From: Rob Herring >>> Use the minimum CPU h/w id of the CPUs associated with the cache for the >>> cache 'id'. This will provide a stable id value for a given system. As >>> we need to check all possible CPUs, we can't use the shared_cpu_map >>> which is just online CPUs. As there's not a cache to CPUs mapping in DT, >>> we have to walk all CPU nodes and then walk cache levels. >>> >>> The cache_id exposed to user-space has historically been 32 bits, and >>> is too late to change. This value is parsed into a u32 by user-space >>> libraries such as libvirt: >>> https://github.com/libvirt/libvirt/blob/master/src/util/virresctrl.c#L1588 >>> >>> Give up on assigning cache-id's if a CPU h/w id greater than 32 bits >>> is found. >>> diff --git a/drivers/base/cacheinfo.c b/drivers/base/cacheinfo.c >>> index cf0d455209d7..df593da0d5f7 100644 >>> --- a/drivers/base/cacheinfo.c >>> +++ b/drivers/base/cacheinfo.c >>> @@ -183,6 +184,42 @@ static bool cache_node_is_unified(struct cacheinfo *this_leaf, >>> return of_property_read_bool(np, "cache-unified"); >>> } >>> >>> +static bool match_cache_node(struct device_node *cpu, >>> + const struct device_node *cache_node) >>> +{ >>> + for (struct device_node *cache __free(device_node) = of_find_next_cache_node(cpu); >> Looks like the creation of this helper function has upset the >> device_node reference counting. This first __free(device_node) will only >> cause of_node_put() to be called in the case of the early return from >> the loop. You've dropped the second __free(device_node) which accounts >> for 'cache' changing on each iteration. Heh, I just took this hunk verbatim. Fixing this up with the __free() magic is tricky as the existing patterns all drop the reference to cpu, which we don't want to do here. I think at this point the __free() magic is just making this harder to understand. How about the old fashioned way: | static bool match_cache_node(struct device_node *cpu, | const struct device_node *cache_node) | { | struct device_node *prev, *cache = of_find_next_cache_node(cpu); | | while (cache) { | if (cache == cache_node) { | of_node_put(cache); | return true; | } | | prev = cache; | cache = of_find_next_cache_node(cache); | of_node_put(prev); | } | | return false; | } > Good catch - this behaves differently from many of the of_get_next* type > helpers in that it doesn't drop the reference to the previous iteration > within the call. > > Maybe it should? > > I checked a few of the call sites and some would be simplified if it did > others would need some more complex restructuring but might benefit as > well. If it did, we'd end up dropping the reference to cpu on the way in, which of_get_next_cpu_node() in for_each_of_cpu_node() was expecting to do. Thanks, James