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 2FCFAC0015E for ; Fri, 28 Jul 2023 15:21:07 +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: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=W8lbl+FYzk85VLY0tsSiVyIpHLlGADVsVf/oiUWMZ2o=; b=l/eyi4vM3BSA7S prd6vjzPi8MiHTClvB6W06ibCoUt3TcbvGmCL2o7UZBUY/OD9TE6knB4AAIqMwUMD0sRR5NvcqwFJ cMZm/AVucNk3ui/PMPqjaEG0m6pKAn7W74fPx4RPR5ZuqJtMFnSuzP8nRt0jqa4TRvMhACYXEQA51 HOcF1L8d5Hq+iTIRCMHgP+5WvSNRKqhw2gRvDg7RDFh4TDfQ7XfvB0NrdPEiDVIct/Mvzq/GiRbRL qz6TeyS+1MLD8XbLxLDuQ7krwb5olS1kRcJOJRnxsu8eBxa0rk5RiycZEBUFKmOcABv0WvhDpElKJ Ag8qvIBzax+udd0QskIg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qPPGm-003rlk-0Z; Fri, 28 Jul 2023 15:20:44 +0000 Received: from frasgout.his.huawei.com ([185.176.79.56]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qPPGi-003rk6-1h for linux-arm-kernel@lists.infradead.org; Fri, 28 Jul 2023 15:20:43 +0000 Received: from lhrpeml500005.china.huawei.com (unknown [172.18.147.206]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4RCB7c40Wcz6839B; Fri, 28 Jul 2023 23:17:00 +0800 (CST) Received: from localhost (10.202.227.76) by lhrpeml500005.china.huawei.com (7.191.163.240) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.27; Fri, 28 Jul 2023 16:20:31 +0100 Date: Fri, 28 Jul 2023 16:20:30 +0100 From: Jonathan Cameron To: Shuai Xue CC: Yicong Yang , , , , , , , , , , , , , Subject: Re: [PATCH v6 3/4] drivers/perf: add DesignWare PCIe PMU driver Message-ID: <20230728162030.00005e60@Huawei.com> In-Reply-To: <12958abe-4bdb-8532-bf67-8e772ed2a9dd@linux.alibaba.com> References: <20230606074938.97724-1-xueshuai@linux.alibaba.com> <20230606074938.97724-4-xueshuai@linux.alibaba.com> <31e2b012-3a29-d063-842d-e3f7736816e7@huawei.com> <20230727103929.00000544@Huawei.com> <12958abe-4bdb-8532-bf67-8e772ed2a9dd@linux.alibaba.com> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) MIME-Version: 1.0 X-Originating-IP: [10.202.227.76] X-ClientProxiedBy: lhrpeml100003.china.huawei.com (7.191.160.210) To lhrpeml500005.china.huawei.com (7.191.163.240) X-CFilter-Loop: Reflected X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230728_082040_848134_EF25513D X-CRM114-Status: GOOD ( 31.66 ) 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 ... > >>> + > >>> +static int dwc_pcie_pmu_online_cpu(unsigned int cpu, struct hlist_node *cpuhp_node) > >>> +{ > >>> + struct dwc_pcie_pmu *pcie_pmu; > >>> + struct pci_dev *pdev; > >>> + int node; > >>> + > >>> + pcie_pmu = hlist_entry_safe(cpuhp_node, struct dwc_pcie_pmu, cpuhp_node); > >>> + pdev = pcie_pmu->pdev; > >>> + node = dev_to_node(&pdev->dev); > >>> + > >>> + if (node != NUMA_NO_NODE && cpu_to_node(pcie_pmu->oncpu) != node && > > > > Perhaps worth a comment on when you might see node == NUMA_NO_NODE? > > Beyond NUMA being entirely disabled, I'd hope that never happens and for that you > > might be able to use a compile time check. > > > > I wonder if this can be simplified by a flag that says if we are already in the > > right node? Might be easier to follow than having similar dance in online and offline > > to figure that out. > > Ok, I will add a comment for NUMA_NO_NODE. If no numa support, I think > any CPU is fine to bind. Agreed. I would add a comment on that being the intent. > > pcie_pmu->on_cpu may be a good choise to be used as a flag, right? pcie_pmu->on_cpu > will be set as -1 when pcie_pmu is allocated and then check in > dwc_pcie_pmu_online_cpu() first. I think you still want to know whether it's in the right node - as maybe there are no local CPUs available at startup. > > Then, the code will be: > > static int dwc_pcie_pmu_online_cpu(unsigned int cpu, struct hlist_node *cpuhp_node) > { > struct dwc_pcie_pmu *pcie_pmu; > struct pci_dev *pdev; > int node; > > pcie_pmu = hlist_entry_safe(cpuhp_node, struct dwc_pcie_pmu, cpuhp_node); > /* If another CPU is already managing this PMU, simply return. */ > if (pcie_pmu->on_cpu != -1) > return 0; > > pdev = pcie_pmu->pdev; > node = dev_to_node(&pdev->dev); > > /* Select the first CPU if no numa support. */ > if (node == NUMA_NO_NODE) > pcie_pmu->on_cpu = cpu; > else if (cpu_to_node(pcie_pmu->on_cpu) != node && > cpu_to_node(cpu) == node) > dwc_pcie_pmu_migrate(pcie_pmu, cpu); > > return 0; > } > > > >>> +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) { > >>> + cpuhp_remove_multi_state(dwc_pcie_pmu_hp_state); > >>> + return ret; > >>> + } > >>> + > >>> + dwc_pcie_pmu_dev = platform_device_register_simple( > >>> + "dwc_pcie_pmu", PLATFORM_DEVID_NONE, NULL, 0); > >>> + if (IS_ERR(dwc_pcie_pmu_dev)) { > >>> + platform_driver_unregister(&dwc_pcie_pmu_driver); > >> > >> On failure we also need to remove cpuhp state as well. > > > > I'd suggest using gotos and a single error handling block. That > > makes it both harder to forget things like this and easier to > > compare that block with what happens in exit() - so slightly > > easier to review! > > Given that we have a appropriate way to tear down the PMUs via devm_add_action_or_reset(), > I am going to remove the redundant probe/remove framework via platform_driver_{un}register(). > for_each probe process in __dwc_pcie_pmu_probe() will be move into dwc_pcie_pmu_init(). > Is it a better way? I think I'd prefer to see a standard driver creation / probe flow even if you could in theory avoid it. Jonathan > > Thank you very much for your valuable comments. > > Best Regards, > Shuai > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel