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 029A132861A for ; Fri, 19 Dec 2025 12:17:21 +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=1766146645; cv=none; b=HBXSaNdQpmOK+bNwoYRuK+O/oCWC2BEWy9aZn7bN3krhTwIQmEmKzbX1m0XJQpgFVsAFalj3diz9KcaEckt50FX5nEx4MLhctm/Vbe3Sz+9E8d/InriwBDEKYlkIncjV8JGu7nsgmRcBdJZfzeEp5Zez1ep0J73tbTK4Mdnk8X4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1766146645; c=relaxed/simple; bh=aImfQP2ZogAmQHRXt+TTtEiTabUSem6a4gB49nLADtg=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=L3EHpmgIXZ6QWoRmiBeaBwWwzS53rZz8rOL9da09GNzyNaIjGPGuqB/orrZ7odD0r8hGfBcLf8e1+ZziK1jhbCAcndfQWCfmvcZu4th07Frp2fEAX3jUkaGREOcCYQylPcGnWvBJk1fK5scvMFiLT3C2wiIPi1cJzBqpegDmS4A= 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 7B8B3FEC; Fri, 19 Dec 2025 04:17:13 -0800 (PST) Received: from [10.1.196.46] (e134344.arm.com [10.1.196.46]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 484113F5CA; Fri, 19 Dec 2025 04:17:17 -0800 (PST) Message-ID: <7100e3a7-4f08-4156-bf0b-6598570abecd@arm.com> Date: Fri, 19 Dec 2025 12:17:15 +0000 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: [RFC PATCH 07/38] arm_mpam: resctrl: Add boilerplate cpuhp and domain allocation To: Jonathan Cameron , James Morse Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, D Scott Phillips OS , carl@os.amperecomputing.com, lcherian@marvell.com, bobo.shaobowang@huawei.com, tan.shaopeng@fujitsu.com, baolin.wang@linux.alibaba.com, Jamie Iles , Xin Hao , peternewman@google.com, dfustini@baylibre.com, amitsinght@marvell.com, David Hildenbrand , Dave Martin , Koba Ko , Shanker Donthineni , fenghuay@nvidia.com, baisheng.gao@unisoc.com, Gavin Shan , rohit.mathew@arm.com, reinette.chatre@intel.com, Punit Agrawal References: <20251205215901.17772-1-james.morse@arm.com> <20251205215901.17772-8-james.morse@arm.com> <20251218113014.00002691@huawei.com> From: Ben Horgan Content-Language: en-US In-Reply-To: <20251218113014.00002691@huawei.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi Jonathan, On 12/18/25 11:30, Jonathan Cameron wrote: > On Fri, 5 Dec 2025 21:58:30 +0000 > James Morse wrote: > >> resctrl has its own data structures to describe its resources. We >> can't use these directly as we play tricks with the 'MBA' resource, >> picking the MPAM controls or monitors that best apply. We may export >> the same component as both L3 and MBA. >> >> Add mpam_resctrl_exports[] as the array of class->resctrl mappings we >> are exporting, and add the cpuhp hooks that allocated and free the >> resctrl domain structures. >> >> While we're here, plumb in a few other obvious things. >> >> CONFIG_ARM_CPU_RESCTRL is used to allow this code to be built >> even though it can't yet be linked against resctrl. >> >> Signed-off-by: James Morse > Hi, > > A few code flow related comments. Fairly trivial stuff but I think > some parts of this can be made more readable / maintainable with > minor reorganization. > > Jonathan > > >> diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c >> index 2996ad93fc3e..efaf7633bc35 100644 >> --- a/drivers/resctrl/mpam_devices.c >> +++ b/drivers/resctrl/mpam_devices.c > ... > >> @@ -2516,6 +2522,12 @@ static void mpam_enable_once(void) >> mutex_unlock(&mpam_list_lock); >> cpus_read_unlock(); >> >> + if (!err) { >> + err = mpam_resctrl_setup(); >> + if (err) >> + pr_err("Failed to initialise resctrl: %d\n", err); >> + } >> + >> if (err) { >> mpam_disable_reason = "Failed to enable."; >> schedule_work(&mpam_broken_work); > > I'd be tempted to move this to an error handling block via a goto > making this bit > if (err) > goto err_disable_mpam; > > err = mpam_resctrl_setup(); > if (err) { > pr_err(); > goto err_dsiable_mpam; > } > > Up to you though. Personally I like all my good paths as straight line > code with the errors handled in if (err) as that consistency really helps > readability. > I'll leave this one as is. It looks like James tried hard to avoid a goto. Thanks, Ben