| 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
| ||
|
Message-ID: <28ac81fe-3c8c-0469-45c5-a2b4c6a730f7@huawei.com>
Date: Thu, 17 Feb 2022 18:11:39 +0000
From: John Garry <john.garry@...wei.com>
To: "liuqi (BA)" <liuqi115@...wei.com>,
"will@...nel.org" <will@...nel.org>,
"mark.rutland@....com" <mark.rutland@....com>
CC: "linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Linuxarm <linuxarm@...wei.com>,
Zhangshaokun <zhangshaokun@...ilicon.com>
Subject: Re: [PATCH 1/2] drivers/perf: hisi: Add Support for CPA PMU
On 16/02/2022 03:16, liuqi (BA) wrote:
>>>
>>> + name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
>>> "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
>>
>> why subtract 1? Looks dangerous if sccl_id == 0
> As CPA PMU is on SICL, and id of SICL is 1 smaller than id of adjacent
> SCCL. SCCL id in our Hip09 platform is set as 1, 3.... so, a reasonable
> sccl-id will never be 0.
I think that you need to view the HW IP independent of the SoC, and not
get into the practice of relying on these things. Anyway, doesn't the
sccl_id come from ACPI DSD (so we can set as we want)?
>
> Parameters sccl_id is read from ACPI, and is checked in
> hisi_pmu_cpu_is_associated_pmu(), so we could make sure that sccl_id
> here is reasonable and is not 0.
>
>
>>
>>> + cpa_pmu->index_id);
>>> +
>>> + cpa_pmu->pmu = (struct pmu) {
>>> + .name = name,
>>> + .module = THIS_MODULE,
>>> + .task_ctx_nr = perf_invalid_context,
>>> + .event_init = hisi_uncore_pmu_event_init,
>>> + .pmu_enable = hisi_uncore_pmu_enable,
>>> + .pmu_disable = hisi_uncore_pmu_disable,
>>> + .add = hisi_uncore_pmu_add,
>>> + .del = hisi_uncore_pmu_del,
>>> + .start = hisi_uncore_pmu_start,
>>> + .stop = hisi_uncore_pmu_stop,
>>> + .read = hisi_uncore_pmu_read,
>>> + .attr_groups = cpa_pmu->pmu_events.attr_groups,
>>> + .capabilities = PERF_PMU_CAP_NO_EXCLUDE,
>>> + };
>>> +
>>> + ret = perf_pmu_register(&cpa_pmu->pmu, name, -1);
>>> + if (ret) {
>>> + dev_err(cpa_pmu->dev, "CPA PMU register failed\n");
>>> + cpuhp_state_remove_instance_nocalls(
>>> + CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, &cpa_pmu->node);
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int hisi_cpa_pmu_remove(struct platform_device *pdev)
>>> +{
>>> + struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev);
>>> +
>>> + perf_pmu_unregister(&cpa_pmu->pmu);
>>> + cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>>> + &cpa_pmu->node);
>>> + return 0;
>>> +}
>>> +
>>> +static struct platform_driver hisi_cpa_pmu_driver = {
>>> + .driver = {
>>> + .name = "hisi_cpa_pmu",
>>> + .acpi_match_table = ACPI_PTR(hisi_cpa_pmu_acpi_match),
>>> + .suppress_bind_attrs = true,
>>> + },
>>> + .probe = hisi_cpa_pmu_probe,
>>> + .remove = hisi_cpa_pmu_remove,
>>> +};
>>> +
>>> +static int __init hisi_cpa_pmu_module_init(void)
>>> +{
>>> + int ret;
>>> +
>>> + ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>>> + "AP_PERF_ARM_HISI_CPA_ONLINE",
>>> + hisi_uncore_pmu_online_cpu,
>>> + hisi_uncore_pmu_offline_cpu);
>>> + if (ret) {
>>> + pr_err("CPA PMU: setup hotplug: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + ret = platform_driver_register(&hisi_cpa_pmu_driver);
>>> + if (ret)
>>> + cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE);
>>
>> I am not being totally serious, but this pattern of registering a CPU
>> hotplug handler and then a driver is so common that we could nearly
>> add a wrapper for it.
>>
>
> Hi John, do you mean somthing like this?
>
> in hisi_uncore_pmu.c:
>
> int hisi_uncore_pmu_module_init(enum cpuhp_state state, const char
> *name, struct platform_driver *drv)
> {
> int ret;
>
> ret = cpuhp_setup_state_multi(state, name, hisi_uncore_pmu_online_cpu,
> hisi_uncore_pmu_offline_cpu);
> if (ret) {
> pr_err("%s: setup hotplug: %d\n", drv->driver.name, ret);
> return ret;
> }
>
> ret = platform_driver_register(drv);
> if (ret)
> cpuhp_remove_multi_state(state);
>
> return ret;
> }
>
> in hisi_uncore_cpa_pmu.c:
>
> static int __init hisi_cpa_pmu_module_init(void)
> {
> int ret;
>
> ret = hisi_uncore_pmu_module_init(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
> "AP_PERF_ARM_HISI_CPA_ONLINE",
> &hisi_cpa_pmu_driver);
>
> return ret;
> }
> module_init(hisi_cpa_pmu_module_init);
Sure, but I'm talking about going further and having this as
drivers/perf or even platform_device.h helper:
diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
index 62299ab5a9be..22f635260a5f 100644
--- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
@@ -562,26 +562,11 @@ static struct platform_driver hisi_ddrc_pmu_driver = {
.remove = hisi_ddrc_pmu_remove,
};
-static int __init hisi_ddrc_pmu_module_init(void)
-{
- int ret;
-
- ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE,
- "AP_PERF_ARM_HISI_DDRC_ONLINE",
- hisi_uncore_pmu_online_cpu,
- hisi_uncore_pmu_offline_cpu);
- if (ret) {
- pr_err("DDRC PMU: setup hotplug, ret = %d\n", ret);
- return ret;
- }
-
- ret = platform_driver_register(&hisi_ddrc_pmu_driver);
- if (ret)
- cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE);
+module_platform_driver_cpu_hotplug(hisi_ddrc_pmu_driver,
+ AP_PERF_ARM_HISI_DDRC_ONLINE,
+ hisi_uncore_pmu_online_cpu,
+ hisi_uncore_pmu_online_cpu);
- return ret;
-}
-module_init(hisi_ddrc_pmu_module_init);
static void __exit hisi_ddrc_pmu_module_exit(void)
{
diff --git a/include/linux/platform_device.h
b/include/linux/platform_device.h
index 7c96f169d274..d0816dfc0637 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -261,6 +261,28 @@ static inline void platform_set_drvdata(struct
platform_device *pdev,
#define builtin_platform_driver(__platform_driver) \
builtin_driver(__platform_driver, platform_driver_register)
+#define module_platform_driver_cpu_hotplug(__platform_driver,
cpuhp_state, online, offline) \
+static int __init __module_platform_driver_cpu_hotplug##_init(void) \
+{ \
+ int ret; \
+ ret = cpuhp_setup_state_multi(CPUHP_##cpuhp_state, \
+ "##cpuhp_state", \
+ online, \
+ offline); \
+ if (ret) { \
+ pr_err("setup hotplug, ret = %d\n", ret); \
+ return ret; \
+ } \
+ \
+ ret = platform_driver_register(&__platform_driver); \
+ if (ret) \
+ cpuhp_remove_multi_state(CPUHP_##cpuhp_state); \
+ \
+ return ret; \
+} \
+module_init(__module_platform_driver_cpu_hotplug##_init);
+
+
/* module_platform_driver_probe() - Helper macro for drivers that don't do
* anything special in module init/exit. This eliminates a lot of
* boilerplate. Each module may only use this macro once, and
--
2.26.2
but only a half serious suggestion :)
Thanks,
John
Powered by blists - more mailing lists