linux-kernel - Re: [PATCH v11 2/2] drivers/perf: hisi: Add driver for HiSilicon PCIe PMU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAC52Y8Zc5oRRBDiZq+zQNGw2CbURN2SRsfW9ek_gw96qDHB1zw@mail.gmail.com>
Date:   Wed, 17 Nov 2021 23:41:08 +0100
From:   Krzysztof Wilczyński <kw@...ux.com>
To:     Qi Liu <liuqi115@...wei.com>
Cc:     Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        PCI <linux-pci@...r.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linuxarm@...wei.com, zhangshaokun@...ilicon.com
Subject: Re: [PATCH v11 2/2] drivers/perf: hisi: Add driver for HiSilicon
 PCIe PMU

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@...ux.com>

        Krzysztof