linux-kernel - Re: [PATCHv2] coresight: etm4x: Fix etm4_count race by moving cpuhp callbacks to init

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: <159592494608.1360974.13925720722764973592@swboyd.mtv.corp.google.com>
Date:   Tue, 28 Jul 2020 01:29:06 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Mathieu Poirier <mathieu.poirier@...aro.org>,
        Mike Leach <mike.leach@...aro.org>,
        Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>,
        Suzuki K Poulose <suzuki.poulose@....com>
Cc:     Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, coresight@...ts.linaro.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCHv2] coresight: etm4x: Fix etm4_count race by moving cpuhp callbacks to init

Quoting Sai Prakash Ranjan (2020-07-28 00:51:02)
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c
> index 6d7d2169bfb2..adb71987a1e3 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> @@ -48,8 +48,6 @@ module_param(pm_save_enable, int, 0444);
>  MODULE_PARM_DESC(pm_save_enable,
>         "Save/restore state on power down: 1 = never, 2 = self-hosted");
>  
> -/* The number of ETMv4 currently registered */
> -static int etm4_count;
>  static struct etmv4_drvdata *etmdrvdata[NR_CPUS];
>  static void etm4_set_default_config(struct etmv4_config *config);
>  static int etm4_set_event_filters(struct etmv4_drvdata *drvdata,
> @@ -1403,12 +1401,9 @@ static int etm4_pm_setup_cpuslocked(void)

Is this only called from __init now? If so please mark it as __init
then.

>  {
>         int ret;
>  
> -       if (etm4_count++)
> -               return 0;
> -
>         ret = cpu_pm_register_notifier(&etm4_cpu_pm_nb);
>         if (ret)
> -               goto reduce_count;
> +               return ret;
>  
>         ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING,
>                                                    "arm/coresight4:starting",
> @@ -1432,17 +1427,11 @@ static int etm4_pm_setup_cpuslocked(void)
>  
>  unregister_notifier:
>         cpu_pm_unregister_notifier(&etm4_cpu_pm_nb);
> -
> -reduce_count:
> -       --etm4_count;
>         return ret;
>  }
>  
>  static void etm4_pm_clear(void)

This is __init too?

>  {
> -       if (--etm4_count != 0)
> -               return;
> -
>         cpu_pm_unregister_notifier(&etm4_cpu_pm_nb);
>         cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
>         if (hp_online) {
> @@ -1598,4 +1576,29 @@ static struct amba_driver etm4x_driver = {
>         .probe          = etm4_probe,
>         .id_table       = etm4_ids,
>  };
> -builtin_amba_driver(etm4x_driver);
> +
> +static int __init etm4x_init(void)
> +{
> +       int ret;
> +
> +       cpus_read_lock();
> +       ret = etm4_pm_setup_cpuslocked();
> +       cpus_read_unlock();
> +
> +       /* etm4_pm_setup_cpuslocked() does its own cleanup - exit on error */
> +       if (ret)
> +               return ret;
> +
> +       ret = amba_driver_register(&etm4x_driver);
> +       if (ret) {
> +               pr_info("Error registering etm4x driver\n");

Use pr_err() please.

> +               goto err_init;
> +       }
> +
> +       return ret;
> +
> +err_init:

Why is this a goto?

> +       etm4_pm_clear();
> +       return ret;

Instead of just putting this in the if (ret) arm?

> +}
> +module_init(etm4x_init);

It was device_initcall before with builtin_amba_driver(), best to not
change that.