Date:   Wed, 17 Nov 2021 23:41:08 +0100
From:   Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>
To:     Qi Liu <liuqi115@huawei.com>
Cc:     Will Deacon <will@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        PCI <linux-pci@vger.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linuxarm@huawei.com, zhangshaokun@hisilicon.com
Subject: Re: [PATCH v11 2/2] drivers/perf: hisi: Add driver for HiSilicon
 PCIe PMU
Message-ID: <CAC52Y8Zc5oRRBDiZq+zQNGw2CbURN2SRsfW9ek_gw96qDHB1zw@mail.gmail.com>
References: <20211029093632.4350-1-liuqi115@huawei.com>
 <20211029093632.4350-3-liuqi115@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20211029093632.4350-3-liuqi115@huawei.com>
Precedence: bulk

Hi Qi,

Thank you for working on this!  Looks really good!

Below a few tiny nitpicks that you are more than welcome to ignore, of
course, as these would have little weight on the final product, so to
speak.

> +struct hisi_pcie_pmu {
> +     struct perf_event *hw_events[HISI_PCIE_MAX_COUNTERS];
> +     struct hlist_node node;
> +     struct pci_dev *pdev;
> +     struct pmu pmu;
> +     void __iomem *base;
> +     int irq;
> +     u32 identifier;
> +     /* Minimum and maximum bdf of root ports monitored by PMU */
> +     u16 bdf_min;
> +     u16 bdf_max;
> +     int on_cpu;
> +};

Would the above "bdf" be the PCI addressing schema?  If so, then we could
capitalise the acronym to keep it consistent with how it's often referred
to in the PCI world.

[...]
> +static int __init hisi_pcie_module_init(void)
> +{
> +     int ret;
> +
> +     ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_PCIE_PMU_ONLINE,
> +                                   "AP_PERF_ARM_HISI_PCIE_PMU_ONLINE",
> +                                   hisi_pcie_pmu_online_cpu,
> +                                   hisi_pcie_pmu_offline_cpu);
> +     if (ret) {
> +             pr_err("Failed to setup PCIe PMU hotplug, ret = %d.\n", ret);
> +             return ret;
> +     }

The above error message could be made to be a little more aligned in terms
of format with the other messages, thus it would be as follows:

  pr_err("Failed to setup PCIe PMU hotplug: %d.\n", ret);

Interestingly, there would be then no need to add the final dot (period) at
the end here, and that would be true everywhere else.

Again, thank you so much for working on this, it's very much appreciated!

Acked-by: Krzysztof Wilczyński <kw@linux.com>

        Krzysztof