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 7F14BC25B46 for ; Mon, 23 Oct 2023 18:52:09 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To:Subject: MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=S0IlQgEKnur6fgB5tUCLuE6ofHS9UTMiTnUOFyu3bNc=; b=Rz/PO+39MbWNdF i7fribpaoJmpEE0AfNZYGEBGCCT0Okyezm2gP4U51ICn3eZXVIVFzKME69cnlU5NJEJodQ0bRWMHv MuIG0RKJEjMktU1Npe3Xv0owURZJ37XD0w6ijnrkXETM5W+ZaantYVNnlTTTppsszslQ6NRLZTjor VEsAbmStozToLtaevhqK7V0t93xdGpFTPVMMPD+cFLKBh2knKg3TPyanRIf9cyHGBjSq2fvwBULdM mOazRJHltsO6MfegZ28L51VTKBUAvC7xLr6eLUGMX7nNaegst0n5zTz7LlpqV8nQ1hANKejwEhBFO NUOVtMWNaAmqvAAPWuSA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qv01h-0087HD-0p; Mon, 23 Oct 2023 18:51:45 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qv01d-0087Fy-1K for linux-arm-kernel@lists.infradead.org; Mon, 23 Oct 2023 18:51:43 +0000 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 5F7D22F4; Mon, 23 Oct 2023 11:52:14 -0700 (PDT) Received: from [10.57.5.125] (unknown [10.57.5.125]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 1B5393F762; Mon, 23 Oct 2023 11:51:30 -0700 (PDT) Message-ID: Date: Mon, 23 Oct 2023 19:51:30 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v9 3/4] drivers/perf: add DesignWare PCIe PMU driver Content-Language: en-GB To: Will Deacon , Shuai Xue Cc: chengyou@linux.alibaba.com, kaishen@linux.alibaba.com, helgaas@kernel.org, yangyicong@huawei.com, Jonathan.Cameron@huawei.com, baolin.wang@linux.alibaba.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org, rdunlap@infradead.org, mark.rutland@arm.com, zhuo.song@linux.alibaba.com, renyu.zj@linux.alibaba.com References: <20231020134230.53342-1-xueshuai@linux.alibaba.com> <20231020134230.53342-4-xueshuai@linux.alibaba.com> <20231023123202.GA3515@willie-the-truck> From: Robin Murphy In-Reply-To: <20231023123202.GA3515@willie-the-truck> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231023_115141_550673_5A32E8DC X-CRM114-Status: GOOD ( 27.34 ) 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: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 2023-10-23 13:32, Will Deacon wrote: [...] >> + >> + /* >> + * The Group#1 event measures the amount of data processed in 16-byte >> + * units. Simplify the end-user interface by multiplying the counter >> + * at the point of read. >> + */ >> + if (event_id >= 0x20 && event_id <= 0x23) >> + return (((u64)hi << 32) | lo) << 4; >> + else >> + return (((u64)hi << 32) | lo); > > nit, but I think it would be clearer to do: > > ret = ((u64)hi << 32) | lo; > > /* ... */ > if (event_id >= 0x20 && event_id <= 0x23) > ret <<= 4; Nit: "ret *= 16;" since the comment says it's multiplying a value, not moving a bitfield. The compiler already knows the most efficient way to implement constant multiplication. > > return ret; > [...] >> +static int __init dwc_pcie_pmu_init(void) >> +{ >> + int ret; >> + >> + ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, >> + "perf/dwc_pcie_pmu:online", >> + dwc_pcie_pmu_online_cpu, >> + dwc_pcie_pmu_offline_cpu); >> + if (ret < 0) >> + return ret; >> + >> + dwc_pcie_pmu_hp_state = ret; >> + >> + ret = platform_driver_register(&dwc_pcie_pmu_driver); >> + if (ret) >> + goto platform_driver_register_err; >> + >> + dwc_pcie_pmu_dev = platform_device_register_simple( >> + "dwc_pcie_pmu", PLATFORM_DEVID_NONE, NULL, 0); >> + if (IS_ERR(dwc_pcie_pmu_dev)) { >> + ret = PTR_ERR(dwc_pcie_pmu_dev); >> + goto platform_device_register_error; >> + } > > I'm a bit confused as to why you're having to create a platform device > for a PCI device -- is this because the main designware driver has already > bound to it? A comment here explaining why you need to do this would be > very helpful. In particular, is there any dependency on another driver > to make sure that e.g. config space accesses work properly? If so, we > probably need to enforce module load ordering or something like that. AFAICS the platform device/driver serve no purpose other than being a hilariously roundabout way to run the for_each_pci_dev() loop in dwc_pcie_pmu_probe() upon module init, and to save explicitly freeing the PMU name/data. Furthermore the devres action for dwc_pcie_pmu_remove_cpuhp_instance() is apparently going for even more style points at module exit by not even relying on the corresponding .remove callback of the tenuous platform driver to undo what its .probe did, but (ab)using the device's devres list to avoid having to keep track of an explicit list of PMU instances at all. Frankly I think it would be a lot more straightforward to just maintain that explicit list of PMU instances, do the PMU creation directly in dwc_pcie_pmu_init(), then unregister and free them in dwc_pcie_pmu_exit(). Not every driver has to contain a literal struct device_driver. It also smells a bit odd that it handles PCI hot-remove but not hot-add - if the underlying device really is hotpluggable, wouldn't we also want to handle new ones turning up after module load? Conversely if it isn't, why pretend to handle it being removed? Even if it's not to do with physical hotplug of the PMU but with the user unloading the PCI controller driver itself (since there's no module/driver-level dependency enforced) and thus tearing down the whole PCI bus, then the same point still applies - if that *can* happen, then what if the user then re-loads it again, or indeed if this module loads first to begin with; wouldn't we want to be able to (re-)discover the PMUs rather than leave the whole PMU driver degraded to a useless state? Thanks, Robin. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel