Re: [PATCH 1/2] drivers/perf: hisi: Add Support for CPA PMU

[John Garry <john.garry@huawei.com>]

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