Re: [PATCH] coresight: etmv4: Fix CPU power management setup in probe() function.

Hi Mike,

On Tue, Jun 23, 2020 at 09:46:06PM +0100, Mike Leach wrote:

The current probe() function calls a pair of cpuhp_xxx API functions to setup CPU hotplug handling. The hotplug lock is held for the duration of the two calls and other CPU related code using cpus_read_lock() / cpus_read_unlock() calls.

The problem is that on error states, goto: statements bypass the cpus_read_unlock() call. This code has increased in complexity as the driver has developed.

This patch introduces a pair of helper functions etm4_pm_setup() and etm4_pm_clear() which correct the issues above and group the PM code a little better.

The two functions etm4_cpu_pm_register() and etm4_cpu_pm_unregister() are dropped as these call cpu_pm_register_notifier() / ..unregister_notifier() dependent on CONFIG_CPU_PM - but this define is used to nop these functions out in the pm headers - so the wrapper functions are superfluous.

Fixes: f188b5e76aae ("coresight: etm4x: Save/restore state across CPU low power states") Fixes: e9f5d63f84fe ("hwtracing/coresight-etm4x: Use cpuhp_setup_state_nocalls_cpuslocked()") Fixes: 58eb457be028 ("etm4x: Convert to hotplug state machine")

Signed-off-by: Mike Leach mike.leach@linaro.org

drivers/hwtracing/coresight/coresight-etm4x.c | 79 ++++++++++++------- 1 file changed, 51 insertions(+), 28 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index 919b76fa49c4..4dd59de440de 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -1407,18 +1407,56 @@ static struct notifier_block etm4_cpu_pm_nb = { .notifier_call = etm4_cpu_pm_notify, }; -static int etm4_cpu_pm_register(void) +/* Setup PM. Called with cpus locked. Deals with error conditions and counts */ +static int etm4_pm_setup_cpuslocked(void) {

• if (IS_ENABLED(CONFIG_CPU_PM))

• return cpu_pm_register_notifier(&etm4_cpu_pm_nb);
  

• int ret;

• return 0;

• if (etm4_count++)

• return 0;
  

• ret = cpu_pm_register_notifier(&etm4_cpu_pm_nb);
• if (ret)

• goto reduce_count;
  

• ret = cpuhp_setup_state_nocalls_cpuslocked(

• CPUHP_AP_ARM_CORESIGHT_STARTING, "arm/coresight4:starting",
  

• etm4_starting_cpu, etm4_dying_cpu);
  

The 80 character limit per line has been relaxed to 100 in the 5.8 cycle. As such you can arrange CPUHP_AP_ARM_CORESIGHT_STARTING the same way you did below with CPUHP_AP_ONLINE_DYN without getting yelled at by checkpatch.

• if (ret)

• goto unregister_notifier;
  

• ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,

• 				   "arm/coresight4:online",
  

• 				   etm4_online_cpu, NULL);
  

• /* HP dyn state ID returned in ret on success */
• if (ret > 0) {

• hp_online = ret;
  

• return 0;
  

• }
• /* failed dyn state - remove others */
• cpuhp_remove_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING);

+unregister_notifier:

• cpu_pm_unregister_notifier(&etm4_cpu_pm_nb);

+reduce_count:

• --etm4_count;
• return ret;

} -static void etm4_cpu_pm_unregister(void) +static void etm4_pm_clear(void) {

• if (IS_ENABLED(CONFIG_CPU_PM))

• if (--etm4_count == 0) {

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

• 	cpuhp_remove_state_nocalls(hp_online);
  

• 	hp_online = 0;
  

• }
  

• }

} static int etm4_probe(struct amba_device *adev, const struct amba_id *id) @@ -1475,24 +1513,15 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) etm4_init_arch_data, drvdata, 1)) dev_err(dev, "ETM arch init failed\n");

• if (!etm4_count++) {

• cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING,
  

• 				     "arm/coresight4:starting",
  

• 				     etm4_starting_cpu, etm4_dying_cpu);
  

• ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
  

• 					   "arm/coresight4:online",
  

• 					   etm4_online_cpu, NULL);
  

• if (ret < 0)
  

• 	goto err_arch_supported;
  

• hp_online = ret;
  

• ret = etm4_pm_setup_cpuslocked();
• cpus_read_unlock();

• ret = etm4_cpu_pm_register();
  

• if (ret)
  

• 	goto err_arch_supported;
  

• /* etm4_pm_setup does its own cleanup - just exit on error here */

s/etm4_pm_setup/etm4_pm_setup_cpuslocked()

Otherwise this is a good cleanup.

Thanks, Mathieu

• if (ret) {

• etmdrvdata[drvdata->cpu] = NULL;
  

• return ret;
  

  }

• cpus_read_unlock();
• if (etm4_arch_supported(drvdata->arch) == false) { ret = -EINVAL; goto err_arch_supported;

@@ -1544,13 +1573,7 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) err_arch_supported: etmdrvdata[drvdata->cpu] = NULL;

• if (--etm4_count == 0) {

• etm4_cpu_pm_unregister();
  

• cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
  

• if (hp_online)
  

• 	cpuhp_remove_state_nocalls(hp_online);
  

• }

• etm4_pm_clear(); return ret;

} -- 2.17.1