Subject: Re: [PATCH v8 10/24] coresight: etm4x: allow etm4x to be built as a module

From: Tingwei Zhang <[email-masked]>
Date: Fri, 14 Aug 2020 19:00:18 +0800

On Fri, Aug 14, 2020 at 06:42:01PM +0800, Suzuki K Poulose wrote:
> On Fri, Aug 14, 2020 at 05:52:24PM +0800, Tingwei Zhang wrote:
> > On Fri, Aug 14, 2020 at 07:46:12AM +0800, Suzuki K Poulose wrote:
> > > On 08/13/2020 08:39 PM, Mathieu Poirier wrote:
> > > >On Fri, Aug 07, 2020 at 07:11:39PM +0800, Tingwei Zhang wrote:
> > > >>From: Kim Phillips <[email-masked]>
> > > >>
> > > >>Allow to build coresight-etm4x as a module, for ease of development.
> > > >>
> > > >>- Kconfig becomes a tristate, to allow =m
> > > >>- append -core to source file name to allow module to
> > > >>   be called coresight-etm4x by the Makefile
> > > >>- add an etm4_remove function, for module unload
> > > >>- add a MODULE_DEVICE_TABLE for autoloading on boot
> > > >>- protect etmdrvdata[] with mutex lock from racing
> > > >>   between module unload and CPU hotplug
> > > >>
> > > >>Cc: Mathieu Poirier <[email-masked]>
> > > >>Cc: Leo Yan <[email-masked]>
> > > >>Cc: Alexander Shishkin <[email-masked]>
> > > >>Cc: Randy Dunlap <[email-masked]>
> > > >>Cc: Suzuki K Poulose <[email-masked]>
> > > >>Cc: Greg Kroah-Hartman <[email-masked]>
> > > >>Cc: Russell King <[email-masked]>
> > > >>Signed-off-by: Kim Phillips <[email-masked]>
> > > >>Signed-off-by: Tingwei Zhang <[email-masked]>
> > > >>Tested-by: Mike Leach <[email-masked]>
> > > >>---
> > > >>  drivers/hwtracing/coresight/Kconfig           |   5 +-
> > > >>  drivers/hwtracing/coresight/Makefile          |   4 +-
> > > >>  ...resight-etm4x.c => coresight-etm4x-core.c} | 118
> > > +++++++++++++-----
> > > >>  3 files changed, 92 insertions(+), 35 deletions(-)
> > > >>  rename drivers/hwtracing/coresight/{coresight-etm4x.c =>
> > > coresight-etm4x-core.c} (95%)
> > > >>
> > > >>diff --git a/drivers/hwtracing/coresight/Kconfig
> > > b/drivers/hwtracing/coresight/Kconfig
> > > >>index 8fd9fd139cf3..d6e107bbd30b 100644
> > > >>--- a/drivers/hwtracing/coresight/Kconfig
> > > >>+++ b/drivers/hwtracing/coresight/Kconfig
> > > >>@@ -78,7 +78,7 @@ config CORESIGHT_SOURCE_ETM3X
> > > >>  	  module will be called coresight-etm3x.
> > > >>  config CORESIGHT_SOURCE_ETM4X
> > > >>-	bool "CoreSight Embedded Trace Macrocell 4.x driver"
> > > >>+	tristate "CoreSight Embedded Trace Macrocell 4.x driver"
> > > >>  	depends on ARM64
> > > >>  	select CORESIGHT_LINKS_AND_SINKS
> > > >>  	select PID_IN_CONTEXTIDR
> > > >>@@ -88,6 +88,9 @@ config CORESIGHT_SOURCE_ETM4X
> > > >>  	  for instruction level tracing. Depending on the
> implemented
> > > version
> > > >>  	  data tracing may also be available.
> > > >>+	  To compile this driver as a module, choose M here: the
> > > >>+	  module will be called coresight-etm4x.
> > > >>+
> > > >>  config CORESIGHT_STM
> > > >>  	tristate "CoreSight System Trace Macrocell driver"
> > > >>  	depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T))
> || ARM64
> > > >>diff --git a/drivers/hwtracing/coresight/Makefile
> > > b/drivers/hwtracing/coresight/Makefile
> > > >>index d619cfd0abd8..271dc255454f 100644
> > > >>--- a/drivers/hwtracing/coresight/Makefile
> > > >>+++ b/drivers/hwtracing/coresight/Makefile
> > > >>@@ -14,8 +14,8 @@ obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) +=
> > > coresight-funnel.o \
> > > >>  obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o
> > > >>  coresight-etm3x-y := coresight-etm3x-core.o coresight-etm-cp14.o \
> > > >>  					coresight-etm3x-sysfs.o
> > > >>-obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
> > > >>-					coresight-etm4x-sysfs.o
> > > >>+obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o
> > > >>+coresight-etm4x-y := coresight-etm4x-core.o coresight-etm4x-sysfs.o
> > > >>  obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
> > > >>  obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
> > > >>  obj-$(CONFIG_CORESIGHT_CATU) += coresight-catu.o
> > > >>diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c
> > > b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > >>similarity index 95%
> > > >>rename from drivers/hwtracing/coresight/coresight-etm4x.c
> > > >>rename to drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > >>index fddfd93b9a7b..ae9847e194a3 100644
> > > >>--- a/drivers/hwtracing/coresight/coresight-etm4x.c
> > > >>+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > > >>@@ -48,6 +48,7 @@ module_param(pm_save_enable, int, 0444);
> > > >>  MODULE_PARM_DESC(pm_save_enable,
> > > >>  	"Save/restore state on power down: 1 = never, 2 =
> self-hosted");
> > > >>+static DEFINE_PER_CPU(struct mutex, etmdrvdata_lock);
> > > >>  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,
> > > >>@@ -1089,18 +1090,25 @@ void etm4_config_trace_mode(struct
> etmv4_config
> > > *config)
> > > >>  static int etm4_online_cpu(unsigned int cpu)
> > > >>  {
> > > >>-	if (!etmdrvdata[cpu])
> > > >>+	mutex_lock(&per_cpu(etmdrvdata_lock, cpu));
> > > >>+	if (!etmdrvdata[cpu]) {
> > > >>+		mutex_unlock(&per_cpu(etmdrvdata_lock, cpu));
> > > >>  		return 0;
> > > >>+	}
> > > >>  	if (etmdrvdata[cpu]->boot_enable &&
> > > !etmdrvdata[cpu]->sticky_enable)
> > > >>  		coresight_enable(etmdrvdata[cpu]->csdev);
> > > >>+	mutex_unlock(&per_cpu(etmdrvdata_lock, cpu));
> > > >>  	return 0;
> > > >>  }
> > > >>  static int etm4_starting_cpu(unsigned int cpu)
> > > >>  {
> > > >>-	if (!etmdrvdata[cpu])
> > > >>+	mutex_lock(&per_cpu(etmdrvdata_lock, cpu));
> > > >>+	if (!etmdrvdata[cpu]) {
> > > >>+		mutex_unlock(&per_cpu(etmdrvdata_lock, cpu));
> > > >>  		return 0;
> > > >>+	}
> > > >>  	spin_lock(&etmdrvdata[cpu]->spinlock);
> > > >>  	if (!etmdrvdata[cpu]->os_unlock)
> > > >>@@ -1109,18 +1117,23 @@ static int etm4_starting_cpu(unsigned int
> cpu)
> > > >>  	if (local_read(&etmdrvdata[cpu]->mode))
> > > >>  		etm4_enable_hw(etmdrvdata[cpu]);
> > > >>  	spin_unlock(&etmdrvdata[cpu]->spinlock);
> > > >>+	mutex_unlock(&per_cpu(etmdrvdata_lock, cpu));
> > > >
> > > >Ouch!
> > > >
> > > >A mutex wrapping a spinlock to protect the exact same drvdata
> structure.
> > > 
> > > Actually this mutex is for "protecting" etmdrvdata[cpu], not the
> > > drvdata struct as such. But as you said, it becomes like a double
> lock.
> > > 
> > > Having mutex is an overkill for sure and can't be used replace
> > > spin_lock, especially if needed from atomic context. Having looked
> > > at the code, we only ever access/modify the etmdrvdata[cpu] on a
> > > different CPU is when we probe and there are chances of races.
> > > One of such is described here
> > > 
> > >
> http://lists.infradead.org/pipermail/linux-arm-kernel/2020-July/589941.htm
> > > l
> > > 
> > > I will see if I can spin a couple of patches to address these.
> > > Once we make sure that the etmdrvdata[cpu] is only modified by "cpu"
> > > then we don't need this mutex and stick with what we have.
> > > 
> > > Suzuki
> > > 
> > 
> > With Suzuki's idea, I made some change to remove mutex lock and change
> > etmdrvdata[i] on CPU i. I didn't change the part in probe to assign
> > drvdata to etmdrvdata. That's after all drvdata is prepared and
> > coresight device is registered. Once hotplug/PM call back see the
> > value, it can use it directly. If we have racing in probe and these
> > call backs, the worse case is call backs finds out etmdrvdata is NULL
> > and return immediately. Does this make sense?
> > 
> > 
> > static void __exit clear_etmdrvdata(void *info)
> > {
> >         int cpu = *(int *)info;
> >         etmdrvdata[cpu] = NULL;
> > }
> > 
> > static int __exit etm4_remove(struct amba_device *adev)
> > {
> >         struct etmv4_drvdata *drvdata = dev_get_drvdata(&adev->dev);
> > 
> >         etm_perf_symlink(drvdata->csdev, false);
> > 
> >         /*
> >          * Taking hotplug lock here to avoid racing between etm4_remove
> and
> >          * CPU hotplug call backs.
> >          */
> >         cpus_read_lock();
> >         if (cpu_online(drvdata->cpu))
> >                 /*
> >                  * The readers for etmdrvdata[] are CPU hotplug call
> backs
> >                  * and PM notification call backs. Change etmdrvdata[i]
> on
> >                  * CPU i ensures these call backs has consistent view
> >                  * inside one call back function.
> >                  */
> >                 smp_call_function_single(drvdata->cpu, clear_etmdrvdata,
> &drvdata->cpu, 1);
> 
> You should check the error here to confirm if this was really complete.
> Otherwise,
> fallback to the manual clearing here.
> 
Yes. I don't need to check cpu_online since it's checked in
smp_call_function_single(). I can just check return value and
fallback to manual clearing.

> >         else
> >                 etmdrvdata[drvdata->cpu] = NULL;
> > 
> >         cpus_read_unlock();
> > 
> >         coresight_unregister(drvdata->csdev);
> > 
> >         return 0;
> > }
> 
> Yes, this is exactly what I was looking for. Additionally we should
> fix the races mentioned in the link above. I believe, we can solve
> that by the following :
>
I already did the same thing in this patch if you ignore the mutex
part. :)

Thanks,
Tingwei 
> ---8>---
> 
> coresight: etm4x: Delay advertising per-cpu drvdata
> 
> Delay advertising the per-cpu etmdrvdata until we have
> successfully initialised. This is to avoid races and
> unnecessary save/restore of the ETM state, before the
> device is properly setup.
> 
> Signed-off-by: Suzuki K Poulose <[email-masked]>
> ---
>  drivers/hwtracing/coresight/coresight-etm4x.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c
> b/drivers/hwtracing/coresight/coresight-etm4x.c
> index 6d7d2169bfb2..30f7ffa07eb6 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.c
> @@ -1499,8 +1499,6 @@ static int etm4_probe(struct amba_device *adev,
> const struct amba_id *id)
>  		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");
> @@ -1509,10 +1507,8 @@ static int etm4_probe(struct amba_device *adev,
> const struct amba_id *id)
>  	cpus_read_unlock();
>  
>  	/* etm4_pm_setup_cpuslocked() does its own cleanup - exit on error
> */
> -	if (ret) {
> -		etmdrvdata[drvdata->cpu] = NULL;
> +	if (ret)
>  		return ret;
> -	}
>  
>  	if (etm4_arch_supported(drvdata->arch) == false) {
>  		ret = -EINVAL;
> @@ -1547,6 +1543,8 @@ static int etm4_probe(struct amba_device *adev,
> const struct amba_id *id)
>  		goto err_arch_supported;
>  	}
>  
> +	/* Advertise this after we have successfully initialised */
> +	etmdrvdata[drvdata->cpu] = drvdata;
>  	pm_runtime_put(&adev->dev);
>  	dev_info(&drvdata->csdev->dev, "CPU%d: ETM v%d.%d initialized\n",
>  		 drvdata->cpu, drvdata->arch >> 4, drvdata->arch & 0xf);
> @@ -1559,7 +1557,6 @@ static int etm4_probe(struct amba_device *adev,
> const struct amba_id *id)
>  	return 0;
>  
>  err_arch_supported:
> -	etmdrvdata[drvdata->cpu] = NULL;
>  	etm4_pm_clear();
>  	return ret;
>  }
> -- 
> 2.24.1
> 

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel