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 BCB14CD3431 for ; Wed, 4 Sep 2024 12:26:51 +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:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=lcaK7bNGn1Qsn4k5j/QaQ5y+n+XkHQ+ZgSjYwoa+L38=; b=kh15I+aEjDpU9gD3ddABECsEK1 ZNzmlUEyorIYDU+huyKVGtV0QQ404b5WGiLRPbDKAirwikgHe6StoqMv1jMEvMA/I6VQWGmCqFRkr XzwDgnoCfZuuryBCjMitpY3AAVZGKgoF+s9qyHjdSdyxdQ1d0EasSXRoq3zto+PZnRuFJxFyG6Nez kMsoSmXJPz6sXHDhSaeM7lCkGUt09tXKfd/M6CPMickJaaDmG3nb4c6CKeTThcNycECm5WpfqSgF5 qbsAmVzy+gz2WqcZdvxo/1l0/jnDSRKgg+lbpau1IRy7k3gkdlbkl5dWqlxpMuuolSnH8Xcy6KDqd bvydTATg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1slp5t-00000004MHh-0M1M; Wed, 04 Sep 2024 12:26:41 +0000 Received: from dfw.source.kernel.org ([139.178.84.217]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1slp3Q-00000004Ly7-0ae2 for linux-arm-kernel@lists.infradead.org; Wed, 04 Sep 2024 12:25:44 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 0996E5C5421; Wed, 4 Sep 2024 12:24:04 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 22782C4CEC2; Wed, 4 Sep 2024 12:24:05 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1725452647; bh=j/87510LA4gtO73Emfr696qWrE/N497dC5nxyxs0adU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=BsK6zi6qoN5b4VLEVKr8CODHVpu+VCRsqnQcVrZ7ADRMPV2At4yz7FUKQIVBhCIgU dUojUxuigczjSgwSjPjHBLRi+iuriHoFk4qinqDXI3b6baB7LTBw3HJpSn3vwg+XzF Gpnhe6P065FrDC8zNupQJ+TLs5dxvK2Ev/Fa7nEp3zLNViPk9LrEx4kCIbwrCenHda VzM62ieSElgSkyOc2I/iM2VedonEVjpqs/2LnYJI0yTp1wZr4+m1iSV9Dh9OQM5kpb CbFz/6C0lTBIPfIidNGe2tK+Cg1tpkBwZA78Wss/PcMWwML+ND3gzBfKHkUoB/RoLR Rs25UtW669YuQ== Date: Wed, 4 Sep 2024 13:24:02 +0100 From: Will Deacon To: Robin Murphy Cc: mark.rutland@arm.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, jialong.yang@shingroup.cn Subject: Re: [PATCH v3 2/3] perf: Add driver for Arm NI-700 interconnect PMU Message-ID: <20240904122356.GE13550@willie-the-truck> References: <275e8ef450eeaf837468ce34e2c6930d59091fbc.1725037424.git.robin.murphy@arm.com> <20240902144714.GA11443@willie-the-truck> <0a41657e-a52c-43c9-9b73-89fd73a376c7@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0a41657e-a52c-43c9-9b73-89fd73a376c7@arm.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240904_052408_334667_5D9E3322 X-CRM114-Status: GOOD ( 23.96 ) 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 Mon, Sep 02, 2024 at 07:47:18PM +0100, Robin Murphy wrote: > On 02/09/2024 3:47 pm, Will Deacon wrote: > > > +static ssize_t arm_ni_format_show(struct device *dev, > > > + struct device_attribute *attr, char *buf) > > > +{ > > > + struct arm_ni_format_attr *fmt = container_of(attr, typeof(*fmt), attr); > > > + int lo = __ffs(fmt->field), hi = __fls(fmt->field); > > > + > > > + return sysfs_emit(buf, "config:%d-%d\n", lo, hi); > > > +} > > > > Nit: if you end up adding single-bit config fields in the future, this > > will quietly do the wrong thing. Maybe safe-guard the 'lo==hi' case (even > > if you just warn once and return without doing anything). > > The counter-argument is that I don't foresee having any reason to add > single-bit config fields here in future, nor indeed config1 or config2 > fields, so I intentionally pruned the would-be dead code while copy-pasting > this implementation from arm-cmn. Yes, if someone were to make an incomplete > change without paying attention or testing they could introduce a bug, but > when is that ever not true? I guess I'm just a little more wary when it comes to UAPI. Somebody starts relying on the broken message and then you're toast. It's also incredibly easy to avoid by construction and the dead code isn't hurting anybody. > > > + name = devm_kasprintf(ni->dev, GFP_KERNEL, "arm_ni_%d_cd_%d", ni->id, cd->id); > > > + if (!name) > > > + return -ENOMEM; > > > + > > > + err = cpuhp_state_add_instance(arm_ni_hp_state, &cd->cpuhp_node); > > > + if (err) > > > + return err; > > > > What happens if there's a CPU hotplug operation here? Can we end up calling > > perf_pmu_migrate_context() concurrently with perf_pmu_register()? > > Yes. Alternatively we could register the PMU before the hotplug handler, > then potentially miss a hotplug event and leave a user-visible PMU > associated with an invalid CPU. This is a known issue for all system PMU > drivers, and the conclusion 5 years ago was that it's impractical to close > this race from outside perf core itself[1][2]. Ok, I'm going to walk right into the trap you've set me... Why can't we prevent hotplug (e.g. with cpus_read_lock()) while we're setting this up? ... and climbing back out of that trap, is the conversation you had with Thomas written down anywhere? I don't want to block this patch, but if five years has passed with nobody looking at this then we probably need to address that at some point before adding more and more broken drivers. Will