From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 17B44CCF9E3 for ; Mon, 10 Nov 2025 16:44:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Subject:CC:To: From:Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=VGDWb/iFzZSZfcy7xrOlDJQ0Oquj7mM2lRCum+Lta6I=; b=K+YVFqCHSL33BCBIN1wCcSzpBI tM6msIpWjRJso2bpCprgtbUYXjz8dF7Hqd4vIgLI30b39pc9ypES2lxK1RTFPQ9Vsxk9hZ9YiG3DU RZWCAuxxJAUmdGX17vHGO4D1x825GDLhRo7vuJ2qSKCWC6ABpNPBSofwT5jUSB3J/3TurKfYjsvhH Cd/uSmsHGy/Qbk2zoR+voSP2DOwyLgsgVsflalXq6y4pM0NE6RWOhhZXVSbHYAynzu407SGgBypKs NMtuweT0U4a5DKD8o4ma4fxTMuuIZU11zC1JJqDN7IQo4DefTgNhtogRHyBEUFJ2YR4V1vD+me3SF URx9NH0Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1vIV0K-00000005qlX-3F4Q; Mon, 10 Nov 2025 16:44:32 +0000 Received: from frasgout.his.huawei.com ([185.176.79.56]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1vIV0F-00000005qkF-0TDp for linux-arm-kernel@lists.infradead.org; Mon, 10 Nov 2025 16:44:31 +0000 Received: from mail.maildlp.com (unknown [172.18.186.216]) by frasgout.his.huawei.com (SkyGuard) with ESMTPS id 4d4wTy6ZkgzJ46BR; Tue, 11 Nov 2025 00:43:50 +0800 (CST) Received: from dubpeml100005.china.huawei.com (unknown [7.214.146.113]) by mail.maildlp.com (Postfix) with ESMTPS id 6D33C140372; Tue, 11 Nov 2025 00:44:20 +0800 (CST) Received: from localhost (10.203.177.15) by dubpeml100005.china.huawei.com (7.214.146.113) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.36; Mon, 10 Nov 2025 16:44:18 +0000 Date: Mon, 10 Nov 2025 16:44:17 +0000 From: Jonathan Cameron To: Gavin Shan CC: Ben Horgan , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , "Shaopeng Tan" Subject: Re: [PATCH 10/33] arm_mpam: Add probe/remove for mpam msc driver and kbuild boiler plate Message-ID: <20251110164417.000042aa@huawei.com> In-Reply-To: <5b9136d6-b6c0-4f24-a8d2-05d7700140a8@redhat.com> References: <20251107123450.664001-1-ben.horgan@arm.com> <20251107123450.664001-11-ben.horgan@arm.com> <5b9136d6-b6c0-4f24-a8d2-05d7700140a8@redhat.com> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.42; x86_64-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.203.177.15] X-ClientProxiedBy: lhrpeml500011.china.huawei.com (7.191.174.215) To dubpeml100005.china.huawei.com (7.214.146.113) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20251110_084427_439151_AF2BB3CC X-CRM114-Status: GOOD ( 36.44 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Sat, 8 Nov 2025 19:28:43 +1000 Gavin Shan wrote: > Hi Ben, > > On 11/7/25 10:34 PM, Ben Horgan wrote: > > From: James Morse > > > > Probing MPAM is convoluted. MSCs that are integrated with a CPU may > > only be accessible from those CPUs, and they may not be online. > > Touching the hardware early is pointless as MPAM can't be used until > > the system-wide common values for num_partid and num_pmg have been > > discovered. > > > > I'm not sure if below commit log is more clearer as I'm not a English > native speaker: I'm normally very flexible on English usage as long as meaning is conveyed but I'm not keen on changing authors Engish in a fashion that doesn't necessarily improve it. So a few thoughts from me as a native speaker. Note don't mind changing text for clarity if the English usage is too obscure. > > MPAM probing is convoluted. MSCs that are integrated to a set of CPUs > may only be accessible from those CPUs, ... To me both are equally valid. Probing MPAM -> implies MPAM is a thing which is probed (MPAM as noun) MPAM probing -> implies that MPAM is a special form of probing (MPAM as kind of adjective giving the flavor of probing that is happening) So slight preference from me for current text. > > > Start with driver probe/remove and mapping the MSC. > > > > CC: Carl Worth > > Tested-by: Fenghua Yu > > Tested-by: Shaopeng Tan > > Tested-by: Peter Newman > > Signed-off-by: James Morse > > Signed-off-by: Ben Horgan > > diff --git a/drivers/resctrl/Kconfig b/drivers/resctrl/Kconfig > > new file mode 100644 > > index 000000000000..ef2f3adf64a9 > > --- /dev/null > > +++ b/drivers/resctrl/Kconfig > > @@ -0,0 +1,15 @@ > > +menuconfig ARM64_MPAM_DRIVER > > + bool "MPAM driver" > > + depends on ARM64 && ARM64_MPAM && EXPERT > > + help > > + Memory System Resource Partitioning and Monitoring (MPAM) driver for > > + System IP, e,g. caches and memory controllers. > > + > > +if ARM64_MPAM_DRIVER > > + > > +config ARM64_MPAM_DRIVER_DEBUG > > + bool "Enable debug messages from the MPAM driver" > > + help > > + Say yes here to enable debug messages from the MPAM driver. > > + > > +endif > > I am asking myself why "depends on ARM64_MPAM_DRIVER" can't be used here? :-) Fairly sure that came up in an earlier review. IIRC other stuff going to be added later in this if/endif block. > > > diff --git a/drivers/resctrl/Makefile b/drivers/resctrl/Makefile > > new file mode 100644 > > index 000000000000..898199dcf80d > > --- /dev/null > > +++ b/drivers/resctrl/Makefile > > @@ -0,0 +1,4 @@ > > +obj-$(CONFIG_ARM64_MPAM_DRIVER) += mpam.o > > +mpam-y += mpam_devices.o > > + > > +ccflags-$(CONFIG_ARM64_MPAM_DRIVER_DEBUG) += -DDEBUG > > diff --git a/drivers/resctrl/mpam_devices.c b/drivers/resctrl/mpam_devices.c > > new file mode 100644 > > index 000000000000..6c6be133d73a > > --- /dev/null > > +++ b/drivers/resctrl/mpam_devices.c > > @@ -0,0 +1,194 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +// Copyright (C) 2025 Arm Ltd. > > + > > +#define pr_fmt(fmt) "%s:%s: " fmt, KBUILD_MODNAME, __func__ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "mpam_internal.h" > > + > > +/* > > + * mpam_list_lock protects the SRCU lists when writing. Once the > > + * mpam_enabled key is enabled these lists are read-only, > > + * unless the error interrupt disables the driver. > > + */ > > s/when writing/for writing To me not a clear improvement. The when writing is a bit passive in that it is talking about something that 'is the case' rather than something 'we make' the case. > s/are read-only/become read-only Nope to this one. Become implies it happens at a later stage, are implies that it happens before mpam_enabled_key is enabled which is more correct (subtle though!) > > > +static DEFINE_MUTEX(mpam_list_lock); > > +static LIST_HEAD(mpam_all_msc); > > + > > +struct srcu_struct mpam_srcu; > > + > > +/* > > + * Number of MSCs that have been probed. Once all MSC have been probed MPAM > > + * can be enabled. > > + */ > > s/all MSC/all MSCs (?) yes. > > > +static atomic_t mpam_num_msc; > > + > > +/* > > + * An MSC can control traffic from a set of CPUs, but may only be accessible > > + * from a (hopefully wider) set of CPUs. The common reason for this is power > > + * management. If all the CPUs in a cluster are in PSCI:CPU_SUSPEND, the > > + * corresponding cache may also be powered off. By making accesses from > > + * one of those CPUs, we ensure this isn't the case. > > + */ > > s/An MSC/A MSC (?) No on this one. I would pronounce MSC and Em-ess-sea so rules are you therefore use An. This is a truely weird quirk of English! > s/from a/from the Another subtle case. "the" implies that we all know exactly which wider set of CPUs. "a" implies that there is at least one that can use. So in my mind, a is slightly more correct, but others may disagree. > s/isn't the case/is the case (?) I think original is correct. The thing that isn't the case is the ".. may also be powered off." and that's what we want to avoid. I'm not 100% sure on intention of that comment though - so good one to query! Thanks, Jonathan