Messages in this thread Patch in this message | | | Date | Thu, 17 Feb 2022 18:11:39 +0000 | Subject | Re: [PATCH 1/2] drivers/perf: hisi: Add Support for CPA PMU | From | John Garry <> |
| |
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
| |