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

From: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
To: Stephen Boyd <swboyd@chromium.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>,
	Mike Leach <mike.leach@linaro.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	linux-arm-msm@vger.kernel.org, coresight@lists.linaro.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCHv2] coresight: etm4x: Fix etm4_count race by moving cpuhp callbacks to init
Date: Tue, 28 Jul 2020 14:17:14 +0530	[thread overview]
Message-ID: <e3bb9a0bec27d769b0ff6284e6cd8ef3@codeaurora.org> (raw)
In-Reply-To: <159592494608.1360974.13925720722764973592@swboyd.mtv.corp.google.com>

On 2020-07-28 13:59, Stephen Boyd wrote:
> 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.
> 

Yes, will change it.

>>  {
>>         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?
> 

Will change.

>>  {
>> -       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.
> 

Yes indeed, will change.

>> +               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?
> 

Will change.

>> +}
>> +module_init(etm4x_init);
> 
> It was device_initcall before with builtin_amba_driver(), best to not
> change that.

Sure.

I will wait to see if there are any more comments on this patch and then 
post a v3.
Thanks for the review Stephen.

Thanks,
Sai

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member
of Code Aurora Forum, hosted by The Linux Foundation