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 6135DCCFA13 for ; Mon, 10 Nov 2025 16:59:05 +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=TQ/AXBT36UtOkGGAyshCxjIIYPi3WYjvbfR9uQWaKlU=; b=N17kEY20KtMqWPKL1K9RDAmq0n jHYphGZMKev+TrpotjXDNSpC/b8nH8ulGiNp4C/Uv+D+NU9zs82oiJnwp/qRmsaCnU1hyf+PPUquo 3cLLlvZ/EqcMK6t7yfld3SqaopDF9q5Yf6bQbiZYmKxW9DFXItxhmlQ9HVpvBEwMgjZ0/LT7K7WnW WLJyygpibNvwrM2jGQs0NNnP7tz5przhKFQZZY2GC+oMCR/M9HWy4osRy184yxEIwR77hEa6pgfEJ ueNsSZsMbQeDGXr9pF1N7Vdaxi2xcLtKfZTb+xHiSokmiQ61/lLOerL+OXP6GT+NJWR6Tvu+CeYmk jiNDIzBQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1vIVEJ-00000005rtn-0cfd; Mon, 10 Nov 2025 16:58:59 +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 1vIVEE-00000005rtE-0IRI for linux-arm-kernel@lists.infradead.org; Mon, 10 Nov 2025 16:58:56 +0000 Received: from mail.maildlp.com (unknown [172.18.186.31]) by frasgout.his.huawei.com (SkyGuard) with ESMTPS id 4d4wpr66H6zHnGfQ; Tue, 11 Nov 2025 00:58:28 +0800 (CST) Received: from dubpeml100005.china.huawei.com (unknown [7.214.146.113]) by mail.maildlp.com (Postfix) with ESMTPS id 512161402F5; Tue, 11 Nov 2025 00:58:44 +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:58:42 +0000 Date: Mon, 10 Nov 2025 16:58:41 +0000 From: Jonathan Cameron To: Ben Horgan CC: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Shaopeng Tan Subject: Re: [PATCH 10/33] arm_mpam: Add probe/remove for mpam msc driver and kbuild boiler plate Message-ID: <20251110165841.00005a74@huawei.com> In-Reply-To: <20251107123450.664001-11-ben.horgan@arm.com> References: <20251107123450.664001-1-ben.horgan@arm.com> <20251107123450.664001-11-ben.horgan@arm.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: lhrpeml500012.china.huawei.com (7.191.174.4) 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_085854_403877_31489E03 X-CRM114-Status: GOOD ( 27.95 ) 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 Fri, 7 Nov 2025 12:34:27 +0000 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. > > 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 Hi Ben, A few minor things from a fresh read. Nothing to prevent a tag though. Reviewed-by: Jonathan Cameron > 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 > + > +static void mpam_msc_drv_remove(struct platform_device *pdev) > +{ > + struct mpam_msc *msc = platform_get_drvdata(pdev); > + > + if (!msc) > + return; Agree with Gavin on this. If there is a reason this might be NULL then a comment would avoid the question being raised again. If not drop the check. > + > + mutex_lock(&mpam_list_lock); > + mpam_msc_destroy(msc); > + mutex_unlock(&mpam_list_lock); > + > + synchronize_srcu(&mpam_srcu); Trivial but perhaps a comment on why. I assume this is because the devm_ cleanup isn't safe until after an RCU grace period? > +} > + > +static struct mpam_msc *do_mpam_msc_drv_probe(struct platform_device *pdev) > +{ > + int err; > + u32 tmp; > + struct mpam_msc *msc; > + struct resource *msc_res; > + struct device *dev = &pdev->dev; > + > + lockdep_assert_held(&mpam_list_lock); > + > + msc = devm_kzalloc(&pdev->dev, sizeof(*msc), GFP_KERNEL); > + if (!msc) > + return ERR_PTR(-ENOMEM); > + > + err = devm_mutex_init(dev, &msc->probe_lock); > + if (err) > + return ERR_PTR(err); Trivial but I'd add a blank line here. > + err = devm_mutex_init(dev, &msc->part_sel_lock); > + if (err) > + return ERR_PTR(err); Trivial but I'd add a blank line here. > + msc->id = pdev->id; > + msc->pdev = pdev; > + INIT_LIST_HEAD_RCU(&msc->all_msc_list); > + INIT_LIST_HEAD_RCU(&msc->ris); > + > + err = update_msc_accessibility(msc); > + if (err) > + return ERR_PTR(err); > + if (cpumask_empty(&msc->accessibility)) { > + dev_err_once(dev, "MSC is not accessible from any CPU!"); > + return ERR_PTR(-EINVAL); > + } > + > + if (device_property_read_u32(&pdev->dev, "pcc-channel", &tmp)) > + msc->iface = MPAM_IFACE_MMIO; > + else > + msc->iface = MPAM_IFACE_PCC; > + > + if (msc->iface == MPAM_IFACE_MMIO) { > + void __iomem *io; > + > + io = devm_platform_get_and_ioremap_resource(pdev, 0, > + &msc_res); > + if (IS_ERR(io)) { > + dev_err_once(dev, "Failed to map MSC base address\n"); > + return ERR_CAST(io); > + } > + msc->mapped_hwpage_sz = msc_res->end - msc_res->start; > + msc->mapped_hwpage = io; > + } else { > + return ERR_PTR(-ENOENT); > + } > + > + list_add_rcu(&msc->all_msc_list, &mpam_all_msc); > + platform_set_drvdata(pdev, msc); > + > + return msc; > +} > + > +static int mpam_msc_drv_probe(struct platform_device *pdev) > +{ > + int err; > + struct mpam_msc *msc = NULL; > + void *plat_data = pdev->dev.platform_data; > + > + mutex_lock(&mpam_list_lock); > + msc = do_mpam_msc_drv_probe(pdev); > + mutex_unlock(&mpam_list_lock); > + if (!IS_ERR(msc)) { > + /* Create RIS entries described by firmware */ > + err = acpi_mpam_parse_resources(msc, plat_data); > + if (err) > + mpam_msc_drv_remove(pdev); > + } else { > + err = PTR_ERR(msc); > + } Seems convoluted. Not obvious to me why you can't do early exits on err and having simpler flow. Maybe something more messy happens in patches after this series to justify the complex approach. if (IS_ERR(msc)) return PTR_ERR(msc); /* Create RIS entries described by firmware */ err = acpi_mpam_parse_resources(msc, plat_data); if (err) { mpam_msc_drv_remove(pdev); return err; } if (atomic_add_return(1, &mpam_num_msc) == fw_num_msc) pr_info("Discovered all MSC\n"); return 0; > + > + if (!err && atomic_add_return(1, &mpam_num_msc) == fw_num_msc) > + pr_info("Discovered all MSC\n"); > + > + return err; > +} > + > +static struct platform_driver mpam_msc_driver = { > + .driver = { > + .name = "mpam_msc", > + }, > + .probe = mpam_msc_drv_probe, > + .remove = mpam_msc_drv_remove, > +}; > + > +static int __init mpam_msc_driver_init(void) > +{ > + if (!system_supports_mpam()) > + return -EOPNOTSUPP; > + > + init_srcu_struct(&mpam_srcu); > + > + fw_num_msc = acpi_mpam_count_msc(); > + Trivial but I'd drop this blank line to keep the call closely associated with the error check. > + if (fw_num_msc <= 0) { > + pr_err("No MSC devices found in firmware\n"); > + return -EINVAL; > + } > + > + return platform_driver_register(&mpam_msc_driver); > +} > +subsys_initcall(mpam_msc_driver_init);