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

[tingwei]

Sorry for the noise.  Please ignore previous comment.
The change is in old patch set of my series.
This change is good to go.
On 2020-08-07 11:52, Tingwei Zhang wrote:
> On Wed, Jul 29, 2020 at 01:13:10PM +0800, Sai Prakash Ranjan wrote:
> > etm4_count keeps track of number of ETMv4 registered and on some 
> > systems,
> > a race is observed on etm4_count variable which can lead to multiple 
> > calls
> > to cpuhp_setup_state_nocalls_cpuslocked(). This function internally 
> > calls
> > cpuhp_store_callbacks() which prevents multiple registrations of 
> > callbacks
> > for a given state and due to this race, it returns -EBUSY leading to 
> > ETM
> > probe failures like below.
> >
> >  coresight-etm4x: probe of 7040000.etm failed with error -16
> >
> > This race can easily be triggered with async probe by setting probe 
> > type
> > as PROBE_PREFER_ASYNCHRONOUS and with ETM power management property
> > "arm,coresight-loses-context-with-cpu".
> >
> > Prevent this race by moving cpuhp callbacks to etm driver init since 
> > the
> > cpuhp callbacks doesn't have to depend on the etm4_count and can be 
> > once
> > setup during driver init. Similarly we move cpu_pm notifier 
> > registration
> > to driver init and completely remove etm4_count usage. Also now we can
> > use non cpuslocked version of cpuhp callbacks with this movement.
> >
> > Fixes: 9b6a3f3633a5 ("coresight: etmv4: Fix CPU power management setup 
> > in
> > probe() function")
> > Fixes: 58eb457be028 ("hwtracing/coresight-etm4x: Convert to hotplug 
> > state
> > machine")
> > Suggested-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@xxxxxxxxxxxxxx>
> > ---
> >
> > Changes in v3:
> >  * Minor cleanups from v2 and change to device_initcall (Stephen Boyd)
> >  * Move to non cpuslocked cpuhp callbacks and rename to etm_pm_setup() 
> > (Mike
> > Leach)
> >
> > Changes in v2:
> >  * Rearrange cpuhp callbacks and move them to driver init (Suzuki K 
> > Poulose)
> >
> > ---
> >  drivers/hwtracing/coresight/coresight-etm4x.c | 65 
> > +++++++++----------
> >  1 file changed, 31 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c
> > b/drivers/hwtracing/coresight/coresight-etm4x.c
> > index 6d7d2169bfb2..fddfd93b9a7b 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,
> > @@ -1398,28 +1396,25 @@ static struct notifier_block etm4_cpu_pm_nb = 
> > {
> >  	.notifier_call = etm4_cpu_pm_notify,
> >  };
> >
> > -/* Setup PM. Called with cpus locked. Deals with error conditions and
> > counts */
> > -static int etm4_pm_setup_cpuslocked(void)
> > +/* Setup PM. Deals with error conditions and counts */
> > +static int __init etm4_pm_setup(void)
> >  {
> >  	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",
> > -						   etm4_starting_cpu, etm4_dying_cpu);
> > +	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING,
> > +					"arm/coresight4:starting",
> > +					etm4_starting_cpu, etm4_dying_cpu);
> >
> >  	if (ret)
> >  		goto unregister_notifier;
> >
> > -	ret = cpuhp_setup_state_nocalls_cpuslocked(CPUHP_AP_ONLINE_DYN,
> > -						   "arm/coresight4:online",
> > -						   etm4_online_cpu, NULL);
> > +	ret = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> > +					"arm/coresight4:online",
> > +					etm4_online_cpu, NULL);
> >
> >  	/* HP dyn state ID returned in ret on success */
> >  	if (ret > 0) {
> > @@ -1428,21 +1423,15 @@ static int etm4_pm_setup_cpuslocked(void)
> >  	}
> >
> >  	/* failed dyn state - remove others */
> > -	cpuhp_remove_state_nocalls_cpuslocked(CPUHP_AP_ARM_CORESIGHT_STARTING);
> > +	cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
> >
> >  unregister_notifier:
> >  	cpu_pm_unregister_notifier(&etm4_cpu_pm_nb);
> > -
> > -reduce_count:
> > -	--etm4_count;
> >  	return ret;
> >  }
> >
> > -static void etm4_pm_clear(void)
> > +static void __init etm4_pm_clear(void)
> >  {
> > -	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) {
> > @@ -1498,22 +1487,12 @@ static int etm4_probe(struct amba_device 
> > *adev,
> > const struct amba_id *id)
> >  	if (!desc.name)
> >  		return -ENOMEM;
> >
> > -	cpus_read_lock();
> >  	etmdrvdata[drvdata->cpu] = drvdata;
> >
> >  	if (smp_call_function_single(drvdata->cpu,
> >  				etm4_init_arch_data,  drvdata, 1))
> >  		dev_err(dev, "ETM arch init failed\n");
> >
> > -	ret = etm4_pm_setup_cpuslocked();
> > -	cpus_read_unlock();
> > -
> > -	/* etm4_pm_setup_cpuslocked() does its own cleanup - exit on error 
> > */
> > -	if (ret) {
> > -		etmdrvdata[drvdata->cpu] = NULL;
> > -		return ret;
> > -	}
> > -
> >  	if (etm4_arch_supported(drvdata->arch) == false) {
> >  		ret = -EINVAL;
> >  		goto err_arch_supported;
> > @@ -1560,7 +1539,6 @@ static int etm4_probe(struct amba_device *adev, 
> > const
> > struct amba_id *id)
> >
> >  err_arch_supported:
> >  	etmdrvdata[drvdata->cpu] = NULL;
> > -	etm4_pm_clear();
> >  	return ret;
> >  }
> >
> > @@ -1598,4 +1576,23 @@ 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;
> > +
> > +	ret = etm4_pm_setup();
>
> etm4_pm_setup() is called in etm4x_init(), but etm4_pm_clear() is 
> called
> in etm4_remove(). If one ETM4x device is removed, etm4_remove() would 
> be
> called and we could still have other ETM4x device alive.
>
> Should etm4_pm_clear() be called in etm4x_exit()? It should only be 
> called
> when module exits.
>
> Thanks,
> Tingwei
>
> > +
> > +	/* etm4_pm_setup() does its own cleanup - exit on error */
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = amba_driver_register(&etm4x_driver);
> > +	if (ret) {
> > +		pr_err("Error registering etm4x driver\n");
> > +		etm4_pm_clear();
> > +	}
> > +
> > +	return ret;
> > +}
> > +device_initcall(etm4x_init);
> > --
> > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
> > member
> > of Code Aurora Forum, hosted by The Linux Foundation
> >
> > _______________________________________________
> > CoreSight mailing list
> > CoreSight@xxxxxxxxxxxxxxxx
> > https://lists.linaro.org/mailman/listinfo/coresight
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel