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 35390C6FA82 for ; Thu, 22 Sep 2022 17:33:35 +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-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version: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:References: List-Owner; bh=tTWHEYFSQ6p1kMig8DSu21buV4xB33lm4N1wglzKw5U=; b=eQemgLoKv1O1q3 tJnajxfbLagkGKlTRdLOef+i/SKfUbQkNdSYUpcG6SYWMj394Q5ydoV0u0EEH3Se5oWTkvkBR4ZNW CyKSoBVj9Dqb9iT0+E4LEJT8f9sBDnDHC/EqGulb+++MKNoYv44nm3Q1cGF6DWulKQHr6UMg+euKD 4TkuhsOv1xDzecCbxSZXFYxE/FNtxYgH/7iRYZDzwT5zTHvZJ9C1sZGQsUWgC9ButpslpqPBEwm8x Z6S6wJDfCc/9Q64ex3ROQMcML8kW7Q0AIZcqYwpHd/Z9VXNmrJI19x52cLCWcWzNS8UknW1Q+hxK3 4HjPwyOd0GrkHOmCjrGQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1obQ3l-00H5Hy-4O; Thu, 22 Sep 2022 17:32:25 +0000 Received: from dfw.source.kernel.org ([2604:1380:4641:c500::1]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1obQ3h-00H5Gp-Nz for linux-arm-kernel@lists.infradead.org; Thu, 22 Sep 2022 17:32:23 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id EBA52636C9; Thu, 22 Sep 2022 17:32:19 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 16F94C433D6; Thu, 22 Sep 2022 17:32:18 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1663867939; bh=1Skf5wiBol+qGBVz6JasNK7icy4lAhhPx0SLST8QPpI=; h=Date:From:To:Cc:Subject:In-Reply-To:From; b=eGu3JUSEuVFz1QcOmxcMhM48f8mxiQn4zee7aDnTEQEOrAHrHePtkHA8I188kLAbA RdyLpQvP0kNPcjlIMkk6Wch055YUPjOFDBDw9aWqt5StVGoYtVIaIFcNCSFlzcdKqA yWDKr/YbD3M0730Aqw8bMg1S1cXhPUArWtdBpBfZ8swnyL96llHB6DGhsPnaAkggrZ dzHhmHU3xGotL1faI8WgyPcplaR9WzAEA00vl2HLhZHbUOb3boKv0prUaxvFICJMxk Beovo7zhCbp1hhBGwE+6Lna+dUtoJ6wMwdH2wayQtL0lmbnxPOy/VzhwrkkX3quFLC vrD1Gb0fhi9aw== Date: Thu, 22 Sep 2022 12:32:17 -0500 From: Bjorn Helgaas To: Jonathan Cameron Cc: Shuai Xue , will@kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, rdunlap@infradead.org, robin.murphy@arm.com, mark.rutland@arm.com, baolin.wang@linux.alibaba.com, zhuo.song@linux.alibaba.com, linux-pci@vger.kernel.org Subject: Re: [PATCH v1 2/3] drivers/perf: add DesignWare PCIe PMU driver Message-ID: <20220922173217.GA1316280@bhelgaas> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220922165820.000017b6@huawei.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220922_103221_906450_510594BC X-CRM114-Status: GOOD ( 33.89 ) 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-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Sep 22, 2022 at 04:58:20PM +0100, Jonathan Cameron wrote: > On Sat, 17 Sep 2022 20:10:35 +0800 > Shuai Xue wrote: > > > This commit adds the PCIe Performance Monitoring Unit (PMU) driver support > > for T-Head Yitian SoC chip. Yitian is based on the Synopsys PCI Express > > Core controller IP which provides statistics feature. The PMU is not a PCIe > > Root Complex integrated End Point(RCiEP) device but only register counters > > provided by each PCIe Root Port. > > > > To facilitate collection of statistics the controller provides the > > following two features for each Root Port: > > > > - Time Based Analysis (RX/TX data throughput and time spent in each > > low-power LTSSM state) > > - Event counters (Error and Non-Error for lanes) > > > > Note, only one counter for each type. > > > > This driver add PMU devices for each PCIe Root Port. And the PMU device is > > named based the BDF of Root Port. For example, > > > > 10:00.0 PCI bridge: Device 1ded:8000 (rev 01) > > > > the PMU device name for this Root Port is pcie_bdf_100000. > > > > Example usage of counting PCIe RX TLP data payload (Units of 16 bytes):: > > > > $# perf stat -a -e pcie_bdf_200/Rx_PCIe_TLP_Data_Payload/ > > > > average RX bandwidth can be calculated like this: > > > > PCIe TX Bandwidth = PCIE_TX_DATA * 16B / Measure_Time_Window > > > > Signed-off-by: Shuai Xue > > +CC linux-pci list and Bjorn. Thanks, this is definitely of interest to linux-pci. > Question in here which I've been meaning to address for other reasons > around how to register 'extra features' on pci ports. > > This particular PMU is in config space in a Vendor Specific Extended > Capability. > > I've focused on that aspect for this review rather than the perf parts. > We'll need to figure that story out first as doing this from a bus walk > makes triggered of a platform driver is not the way I'd expect to see > this work. > > +static int dwc_pcie_pmu_discover(struct dwc_pcie_pmu_priv *priv) > > +{ > > + int val, where, index = 0; > > + struct pci_dev *pdev = NULL; > > + struct dwc_pcie_info_table *pcie_info; > > + > > + priv->pcie_table = > > + devm_kcalloc(priv->dev, RP_NUM_MAX, sizeof(*pcie_info), GFP_KERNEL); > > + if (!priv->pcie_table) > > + return -EINVAL; > > + > > + pcie_info = priv->pcie_table; > > + while ((pdev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, pdev)) != NULL && > > + index < RP_NUM_MAX) { > > This having a driver than then walks the pci topology to find root ports and add > extra stuff to them is not a clean solution. > > The probing should be driven from the existing PCI driver topology. > There are a bunch of new features we need to add to ports in the near future > anyway - this would just be another one. > Same problem exists for CXL CPMU perf devices - so far we only support those > on end points, partly because we need a clean way to probe them on pci ports. > > Whatever we come up with there will apply here as well. I agree, I don't like to see more uses of pci_get_device() because it doesn't fit the driver model at all. For one thing, it really screws up the hotplug model because this doesn't account for hot-added devices and there's no clear cleanup path for removal. Hotplug is likely not an issue in this particular case, but it gets copied to places where it is an issue. Maybe we need some kind of PCI core interface whereby drivers can register their interest in VSEC and/or DVSEC capabilities. Bjorn _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel