lkml.org 
[lkml]   [2022]   [Feb]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] drivers/perf: hisi: Add Support for CPA PMU
From
Date

HiJohn,
On 2022/2/18 2:11, John Garry wrote:
> 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)?
>

Yes, sccl_id in driver is read from ACPI DSD, and is checked in
hisi_pmu_cpu_is_associated_pmu().
>>
>> 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);
> +
> +

got it, I think we could make another patch to do these cleanup.

Thanks,
Qi
>  /* 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

\
 
 \ /
  Last update: 2022-02-18 09:09    [W:0.053 / U:1.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site