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 20E3CC433F5 for ; Tue, 18 Jan 2022 17:16:54 +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:MIME-Version:Date:Message-ID:From: References:CC:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=P/SSSNrmHBsII/Ai7fReYwrV8HSCqLqEl8t84JhDpNc=; b=iKv1IWI3FHDR4946/EvDn0rxB6 LD72O7A0tSD1t9gjYRaOxFd4nyj3ve7GqSUOewZwQIRq01+KEfxHTdm9Tk0U/oHVMLgGPp1iXEdnq YDKO7IGH1KD/4xxhQESTKTc02X7N5dc7XwmnYpumzzXfVOAZfDRDgn3a2sYsd4cxQEXRw1eUogEZY aLykGmqI7+6uusJ5TE3dU6fIZvjqkyQ6a3QMohdkVBwews38oPAYc5AZDEIgRiHpkG37Z6oPtakHQ DL4vh3IaH9xj070SC+rqVUrdKYHT8eyCujVftN1gyJqu6WXW1hN8yerLkQ+O3MRusm1ApAj7WMOO3 pLqIgZ3Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1n9s4Q-002PPc-81; Tue, 18 Jan 2022 17:14:58 +0000 Received: from frasgout.his.huawei.com ([185.176.79.56]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1n9s4K-002POT-JW for linux-arm-kernel@lists.infradead.org; Tue, 18 Jan 2022 17:14:55 +0000 Received: from fraeml712-chm.china.huawei.com (unknown [172.18.147.207]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4JdZzT4ntdz67lnC; Wed, 19 Jan 2022 01:10:45 +0800 (CST) Received: from lhreml724-chm.china.huawei.com (10.201.108.75) by fraeml712-chm.china.huawei.com (10.206.15.61) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.21; Tue, 18 Jan 2022 18:14:40 +0100 Received: from [10.47.94.101] (10.47.94.101) by lhreml724-chm.china.huawei.com (10.201.108.75) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.21; Tue, 18 Jan 2022 17:14:39 +0000 Subject: Re: [RESEND PATCH 2/2] drivers/perf: hisi: add driver for HNS3 PMU To: Guangbin Huang , , CC: , , , , , , , Linuxarm References: <20220117015222.9617-1-huangguangbin2@huawei.com> <20220117015222.9617-3-huangguangbin2@huawei.com> From: John Garry Message-ID: <271e4b5c-7dc4-058f-9299-d940a0fb79e0@huawei.com> Date: Tue, 18 Jan 2022 17:14:07 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.12.1 MIME-Version: 1.0 In-Reply-To: <20220117015222.9617-3-huangguangbin2@huawei.com> Content-Language: en-US X-Originating-IP: [10.47.94.101] X-ClientProxiedBy: lhreml736-chm.china.huawei.com (10.201.108.87) To lhreml724-chm.china.huawei.com (10.201.108.75) X-CFilter-Loop: Reflected X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220118_091453_022675_D48B86D3 X-CRM114-Status: GOOD ( 41.59 ) 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 17/01/2022 01:52, Guangbin Huang wrote: +linuxarm > HNS3 PMU End Point device supports to collect performance statistics > of bandwidth, latency, packet rate, interrupt rate in HiSilicon SoC > NIC. > > NIC of each IO DIE has one PMU device for it. Driver registers each > PMU device to perf, and exports information of supported events, > filter mode of each event, identifier and so on via sysfs. > > Each PMU device has its own control, counter and interrupt registers, > and supports up to 8 events by hardware. > > Filter options contains: > event - select event > subevent - select subevent > port - select physical port of nic > tc - select tc(must be used with port) > func - select PF/VF > queue - select queue of PF/VF(must be used with func) > intr - select interrupt number(must be used with func) > global - select all functions of IO DIE > > Signed-off-by: Guangbin Huang > --- > MAINTAINERS | 6 + > drivers/perf/hisilicon/Kconfig | 9 + > drivers/perf/hisilicon/Makefile | 1 + > drivers/perf/hisilicon/hns3_pmu.c | 1631 +++++++++++++++++++++++++++++ > include/linux/cpuhotplug.h | 1 + > 5 files changed, 1648 insertions(+) > create mode 100644 drivers/perf/hisilicon/hns3_pmu.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 90a7d7b21982..e8e258bb8f8d 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -8614,6 +8614,12 @@ F: Documentation/admin-guide/perf/hisi-pcie-pmu.rst > F: Documentation/admin-guide/perf/hisi-pmu.rst > F: drivers/perf/hisilicon > > +HISILICON HNS3 PMU DRIVER > +M: Guangbin Huang > +S: Supported > +F: Documentation/admin-guide/perf/hns3-pmu.rst > +F: drivers/perf/hisilicon/hns3_pmu.c > + > HISILICON QM AND ZIP Controller DRIVER > M: Zhou Wang > L: linux-crypto@vger.kernel.org > diff --git a/drivers/perf/hisilicon/Kconfig b/drivers/perf/hisilicon/Kconfig > index 5546218b5598..8a9661c97ec8 100644 > --- a/drivers/perf/hisilicon/Kconfig > +++ b/drivers/perf/hisilicon/Kconfig > @@ -14,3 +14,12 @@ config HISI_PCIE_PMU > RCiEP devices. > Adds the PCIe PMU into perf events system for monitoring latency, > bandwidth etc. > + > +config HNS3_PMU > + tristate "HNS3 PERF PMU" > + depends on ARM64 && PCI Any reason not to add COMPILE_TEST, like: depends on ARM64 || COMPILE_TEST depends on PCI > + help > + Provide support for HNS3 performance monitoring unit (PMU) IEP > + devices. > + Adds the HNS3 PMU into perf events system for monitoring latency, > + bandwidth etc. > diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile > index 506ed39e3266..13297ec2798f 100644 > --- a/drivers/perf/hisilicon/Makefile > +++ b/drivers/perf/hisilicon/Makefile > @@ -4,3 +4,4 @@ obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o \ > hisi_uncore_pa_pmu.o > > obj-$(CONFIG_HISI_PCIE_PMU) += hisi_pcie_pmu.o > +obj-$(CONFIG_HNS3_PMU) += hns3_pmu.o > diff --git a/drivers/perf/hisilicon/hns3_pmu.c b/drivers/perf/hisilicon/hns3_pmu.c > new file mode 100644 > index 000000000000..075b7eefe8d6 > --- /dev/null > +++ b/drivers/perf/hisilicon/hns3_pmu.c > @@ -0,0 +1,1631 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * This driver adds support for HNS3 PMU iEP device. > Related perf events are > + * bandwidth, latency, packet rate, interrupt rate etc. I don't think that this is relevant here > + * > + * Copyright (C) 2021 HiSilicon Limited > + */ 2022, I suppose now... > +}; > + > +#define HNS3_PMU_SET_HW_FILTER(_hwc, _mode) \ > + ((_hwc)->addr_filters = (void *)&hns3_pmu_hw_filter_modes[(_mode)]) > + > +static ssize_t identifier_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct hns3_pmu *hns3_pmu; > + > + hns3_pmu = to_hns3_pmu(dev_get_drvdata(dev)); Can the declaration and assignment be on the same line? > + > + return sysfs_emit(buf, "0x%x\n", hns3_pmu->identifier); > +} > +static DEVICE_ATTR_RO(identifier); > + > +static ssize_t cpumask_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct hns3_pmu *hns3_pmu = to_hns3_pmu(dev_get_drvdata(dev)); > + > + return sysfs_emit(buf, "%d\n", hns3_pmu->on_cpu); > +} > +static DEVICE_ATTR_RO(cpumask); > + > +static ssize_t bdf_min_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct hns3_pmu *hns3_pmu = to_hns3_pmu(dev_get_drvdata(dev)); > + > + return sysfs_emit(buf, "0x%4x\n", hns3_pmu->bdf_min); > +} > +static DEVICE_ATTR_RO(bdf_min); > + > +static ssize_t bdf_max_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct hns3_pmu *hns3_pmu = to_hns3_pmu(dev_get_drvdata(dev)); > + > + return sysfs_emit(buf, "0x%4x\n", hns3_pmu->bdf_max); are these same files as in HISI PCI PMU driver? > +} > +static DEVICE_ATTR_RO(bdf_max); > + > +static struct attribute *hns3_pmu_events_attr[] = { > + /* bandwidth events */ > + > +static struct attribute_group hns3_pmu_events_group = { > + .name = "events", > + .attrs = hns3_pmu_events_attr, > +}; > + > +static struct attribute_group hns3_pmu_filter_mode_group = { > + .name = "filtermode", > +}; > + > +} > + > +static bool hns3_pmu_valid_bdf(struct hns3_pmu *hns3_pmu, u16 bdf) > +{ > + struct pci_dev *pdev; > + > + if (bdf < hns3_pmu->bdf_min || bdf > hns3_pmu->bdf_max) { > + pci_err(hns3_pmu->pdev, "Invalid EP device: %#x!\n", bdf); > + return false; > + } > + > + pdev = pci_get_domain_bus_and_slot(pci_domain_nr(hns3_pmu->pdev->bus), > + PCI_BUS_NUM(bdf), > + GET_PCI_DEVFN(bdf)); > + if (!pdev) { > + pci_err(hns3_pmu->pdev, "Nonexistent EP device: %#x!\n", bdf); > + return false; > + } > + > + pci_dev_put(pdev); > + return true; > +} if any of this code is same as hisi pci pmu driver then please put in a common lib > + > +static void hns3_pmu_set_qid_para(struct hns3_pmu *hns3_pmu, u32 idx, u16 bdf, > + u16 queue) > +{ > + u32 val; > + > + val = GET_PCI_DEVFN(bdf); > + val |= (u32)queue << HNS3_PMU_QID_PARA_QUEUE_S; > + hns3_pmu_writel(hns3_pmu, HNS3_PMU_REG_EVENT_QID_PARA, idx, val); > +} > + > +static bool hns3_pmu_qid_req_start(struct hns3_pmu *hns3_pmu, u32 idx) > +{ > + bool queue_id_valid = false; > + u32 reg_qid_ctrl, val; > + int err; > + > + /* enable queue id request */ > + hns3_pmu_writel(hns3_pmu, HNS3_PMU_REG_EVENT_QID_CTRL, idx, > + HNS3_PMU_QID_CTRL_REQ_ENABLE); > + ... > +} > + > +static bool hns3_pmu_validate_event_group(struct perf_event *event) > +{ > + struct perf_event *leader = event->group_leader; > + struct perf_event *sibling; > + int counters = 1; > + > + if (!is_software_event(leader)) { > + /* > + * We must NOT create groups containing mixed PMUs, although > + * software events are acceptable > + */ > + if (leader->pmu != event->pmu) > + return false; > + > + /* Increase counter for the leader */ > + if (leader != event) > + counters++; > + } > + > + for_each_sibling_event(sibling, event->group_leader) { > + if (is_software_event(sibling)) > + continue; > + > + if (sibling->pmu != event->pmu) > + return false; > + > + /* Increase counter for each sibling */ > + counters++; > + } > + > + return counters <= HNS3_PMU_MAX_COUNTERS; > +} > + This is exact same effectively as hisi_validate_event_group(), apart from: return counters <= HNS3_PMU_MAX_COUNTERS; vs return counters <= hisi_pmu->num_counters; Can we factor all this out? Less code to review makes reviewing easier and quicker and less daunting ... > +} > + > +/* > + * The performance calculation formula of all types of event is > + * performance = data / data_ext. > + * The arguments data and data_ext have different meanings for different types > + * of events as follow: > + * event_type data data_ext > + * bandwidth byte number cycles number > + * packet rate packet number cycles number > + * interrupt rate interrupt number cycles number > + * latency cycles number packet number > + */ > +static u64 hns3_pmu_get_performance(u8 event_type, u32 hw_clk_freq, u64 data, > + u64 data_ext) > +{ > + u64 result; > + > + if (!hw_clk_freq || !data || !data_ext) > + return 0; > + > + switch (event_type) { > + case HNS3_PMU_EVENT_TYPE_BANDWIDTH: > + /* process data to set unit of bandwidth as "KBits/s" */ > + result = data / hns3_pmu_tick_to_ms(hw_clk_freq, data_ext); > + result = BYTES_TO_BITS(result); > + break; > + > + case HNS3_PMU_EVENT_TYPE_PACKET_RATE: > + case HNS3_PMU_EVENT_TYPE_INTR_RATE: > + /* process data to set unit as "pps" */ > + result = data * 1000 / > + hns3_pmu_tick_to_ms(hw_clk_freq, data_ext); > + break; > + > + case HNS3_PMU_EVENT_TYPE_LATENCY: > + /* process data to set unit of latency as "ns" */ > + result = data / data_ext; > + result = result * 1000000000 / hw_clk_freq; > + break; > + > + default: > + result = data / data_ext; > + break; > + } > + > + return result; > +} > + > +static int hns3_pmu_event_init(struct perf_event *event) > +{ > + struct hns3_pmu *hns3_pmu = to_hns3_pmu(event->pmu); > + struct hw_perf_event *hwc = &event->hw; > + int idx; > + int ret; > + > + if (event->attr.type != event->pmu->type) > + return -ENOENT; > + > + /* Sampling is not supported */ > + if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK) > + return -EOPNOTSUPP; > + > + event->cpu = hns3_pmu->on_cpu; > + > + idx = hns3_pmu_get_event_idx(hns3_pmu); > + if (idx < 0) { > + pci_err(hns3_pmu->pdev, "Up to %u events are supported!\n", > + HNS3_PMU_MAX_COUNTERS); > + return -EBUSY; > + } > + > + hwc->idx = idx; > + > + ret = hns3_pmu_select_filter_mode(event, hns3_pmu); > + if (ret) { > + pci_err(hns3_pmu->pdev, "Invalid filter, ret = %d.\n", ret); > + return ret; > + } > + > + if (!hns3_pmu_validate_event_group(event)) { > + pci_err(hns3_pmu->pdev, "Invalid event group.\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static void hns3_pmu_read(struct perf_event *event) > +{ > + struct hns3_pmu *hns3_pmu = to_hns3_pmu(event->pmu); > + struct hw_perf_event *hwc = &event->hw; > + struct hw_perf_event_extra *hwc_ext; > + u64 new_cnt_ext, prev_cnt_ext; > + u64 new_cnt, prev_cnt, delta; > + > + hwc_ext = &hwc->extra_reg; > + do { > + prev_cnt = local64_read(&hwc->prev_count); > + prev_cnt_ext = hwc_ext->config; > + hns3_pmu_read_counter(event, &new_cnt, &new_cnt_ext); > + } while (local64_cmpxchg(&hwc->prev_count, prev_cnt, new_cnt) != > + prev_cnt); > + > + hwc_ext->config = new_cnt_ext; > + > + delta = hns3_pmu_get_performance(hns3_get_event(event), > + hns3_pmu->hw_clk_freq, > + new_cnt - prev_cnt, > + new_cnt_ext - prev_cnt_ext); This looks very much like what was originally had in the hisi PCI PMU driver: https://lore.kernel.org/linux-arm-kernel/20210616134257.GA22905@willie-the-truck/ https://lore.kernel.org/linux-arm-kernel/20210617175704.GF24813@willie-the-truck/ Please consult with liuqi If it is, I figure that we're going to need to expose the 2x values and do the calculation in userspace. > + > + local64_add(delta, &event->count); > +} > + > +static void hns3_pmu_start(struct perf_event *event, int flags) > +{ > + struct hns3_pmu *hns3_pmu = to_hns3_pmu(event->pmu); > + struct hw_perf_event *hwc = &event->hw; > + > + if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED))) > + return; > + > + WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE)); ... > + > + val = readl(hns3_pmu->base + HNS3_PMU_REG_DEVICE_ID); > + device_id = val & 0xffff; > + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hns3_pmu_%u", device_id); would adding sccl to the name help it make it more clear that this is per sccl, like hns3_pmu_sccl%u? > + if (!name) { > + pci_err(pdev, "failed to allocate name for HNS3 PMU.\n"); I don't think that you need this -ENOMEM messages > + ret = -ENOMEM; > + goto err_alloc_dev_name; > + } > + > + hns3_pmu->pdev = pdev; > + hns3_pmu->on_cpu = HNS3_PMU_INVALID_CPU_ID; can you possibly just initially target the probe CPU? > + hns3_pmu->identifier = readl(hns3_pmu->base + HNS3_PMU_REG_VERSION); > + hns3_pmu->pmu = (struct pmu) { > + .name = name, > + .module = THIS_MODULE, > + .event_init = hns3_pmu_event_init, > + .pmu_enable = hns3_pmu_enable, > + .pmu_disable = hns3_pmu_disable, > + .add = hns3_pmu_add, > + .del = hns3_pmu_del, > + .start = hns3_pmu_start, > + .stop = hns3_pmu_stop, > + .read = hns3_pmu_read, > + .task_ctx_nr = perf_invalid_context, > + .attr_groups = hns3_pmu_attr_groups, > + .capabilities = PERF_PMU_CAP_NO_EXCLUDE, > + }; > + > + return 0; > + > +err_alloc_dev_name: > + iounmap(hns3_pmu->base); > +err_ioremap_bar: > + pci_release_regions(pdev); > + > + return ret; > +} > + > +static irqreturn_t hns3_pmu_irq(int irq, void *data) > +{ > + struct hns3_pmu *hns3_pmu = data; > + u32 intr_status, idx; > + > + for (idx = 0; idx < HNS3_PMU_MAX_COUNTERS; idx++) { > + intr_status = hns3_pmu_readl(hns3_pmu, > + HNS3_PMU_REG_EVENT_INTR_STATUS, > + idx); > + > + /* > + * As each counter will restart from 0 when it is overflowed, > + * extra processing is no need, just clear interrupt status. > + */ > + if (intr_status) > + hns3_pmu_clear_intr_status(hns3_pmu, idx); > + } > + > + return IRQ_HANDLED; > +} > + > +static int hns3_pmu_online_cpu(unsigned int cpu, struct hlist_node *node) > +{ > + struct hns3_pmu *hns3_pmu; > + > + hns3_pmu = hlist_entry_safe(node, struct hns3_pmu, node); > + if (!hns3_pmu) > + return -ENODEV; > + > + if (hns3_pmu->on_cpu == HNS3_PMU_INVALID_CPU_ID) { > + hns3_pmu->on_cpu = cpu; > + irq_set_affinity_hint(hns3_pmu->irq, cpumask_of(cpu)); > + } > + > + return 0; > +} > + > +static int hns3_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node) > +{ > + struct hns3_pmu *hns3_pmu; > + unsigned int target; > + > + hns3_pmu = hlist_entry_safe(node, struct hns3_pmu, node); > + if (!hns3_pmu) > + return -ENODEV; > + > + /* Nothing to do if this CPU doesn't own the PMU */ > + if (hns3_pmu->on_cpu != cpu) > + return 0; > + > + /* Choose a new CPU from all online cpus */ > + target = cpumask_any_but(cpu_online_mask, cpu); > + if (target >= nr_cpu_ids) > + return 0; > + > + perf_pmu_migrate_context(&hns3_pmu->pmu, cpu, target); > + hns3_pmu->on_cpu = target; > + irq_set_affinity_hint(hns3_pmu->irq, cpumask_of(target)); > + > + return 0; > +} > + > +static int hns3_pmu_irq_register(struct pci_dev *pdev, > + struct hns3_pmu *hns3_pmu) > +{ > + int irq, ret; > + > + ret = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI); I don't think that a pcim exists here, so can use devm_add_action() to free them [maybe the name is wrong I mentioned] > + if (ret < 0) { > + pci_err(pdev, "failed to enable MSI vectors, ret = %d.\n", ret); > + return ret; > + } > + > + irq = pci_irq_vector(pdev, 0); > + ret = request_irq(irq, hns3_pmu_irq, 0, hns3_pmu->pmu.name, hns3_pmu); devm_request_irq() > + if (ret) { > + pci_err(pdev, "failed to register irq, ret = %d.\n", ret); > + pci_free_irq_vectors(pdev); > + return ret; > + } > + > + hns3_pmu->irq = irq; > + > + return 0; > +} > + > +static void hns3_pmu_irq_unregister(struct pci_dev *pdev, > + struct hns3_pmu *hns3_pmu) > +{ > + free_irq(hns3_pmu->irq, hns3_pmu); > + pci_free_irq_vectors(pdev); > +} > + > +static int hns3_pmu_init(struct pci_dev *pdev, struct hns3_pmu *hns3_pmu) > +{ > + int ret; > + > + ret = hns3_pmu_alloc_pmu(pdev, hns3_pmu); > + if (ret) > + return ret; > + > + ret = hns3_pmu_irq_register(pdev, hns3_pmu); > + if (ret) > + goto err_irq_register; > + > + ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HNS3_PMU_ONLINE, > + &hns3_pmu->node); > + if (ret) { > + pci_err(pdev, "failed to register hotplug, ret = %d.\n", ret); > + goto err_register_hotplug; > + } > + > + ret = perf_pmu_register(&hns3_pmu->pmu, hns3_pmu->pmu.name, -1); > + if (ret) { > + pci_err(pdev, "failed to register HNS3 PMU, ret = %d.\n", ret); do you need to mention "HNS3 PMU", since pdev is passed > + goto err_register_pmu; > + } > + > + return ret; > + > +err_register_pmu: > + cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HNS3_PMU_ONLINE, > + &hns3_pmu->node); > + irq_set_affinity_hint(hns3_pmu->irq, NULL); > + > +err_register_hotplug: > + hns3_pmu_irq_unregister(pdev, hns3_pmu); > + > +err_irq_register: > + iounmap(hns3_pmu->base); > + > + return ret; > +} > + > +static void hns3_pmu_uninit(struct pci_dev *pdev) > +{ > + struct hns3_pmu *hns3_pmu = (struct hns3_pmu *)pci_get_drvdata(pdev); no need to cast a void ptr > + > + perf_pmu_unregister(&hns3_pmu->pmu); > + cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HNS3_PMU_ONLINE, > + &hns3_pmu->node); > + irq_set_affinity_hint(hns3_pmu->irq, NULL); > + hns3_pmu_irq_unregister(pdev, hns3_pmu); > + iounmap(hns3_pmu->base); > +} > + > +static int hns3_pmu_init_dev(struct pci_dev *pdev) > +{ > + int ret; > + > + ret = pci_enable_device(pdev); isn't there pci-managed versions of this so that you don't need worry about undo'ing this manually? > + if (ret) { > + pci_err(pdev, "failed to enable pci device, ret = %d.\n", ret); > + return ret; > + } > + > + ret = pci_request_mem_regions(pdev, "hns3_pmu"); > + if (ret < 0) { > + pci_err(pdev, "failed to request pci mem regions, ret = %d.\n", > + ret); > + pci_disable_device(pdev); > + return ret; > + } > + > + pci_set_master(pdev); > + > + return 0; > +} > + > +static void hns3_pmu_uninit_dev(struct pci_dev *pdev) > +{ > + pci_clear_master(pdev); > + pci_release_mem_regions(pdev); > + pci_disable_device(pdev); > +} > + _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel