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

[Suzuki K Poulose <Suzuki.Poulose@arm.com>]

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 <kim.phillips@arm.com>
> > >>
> > >>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 <mathieu.poirier@linaro.org>
> > >>Cc: Leo Yan <leo.yan@linaro.org>
> > >>Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> > >>Cc: Randy Dunlap <rdunlap@infradead.org>
> > >>Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com>
> > >>Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > >>Cc: Russell King <linux@armlinux.org.uk>
> > >>Signed-off-by: Kim Phillips <kim.phillips@arm.com>
> > >>Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > >>Tested-by: Mike Leach <mike.leach@linaro.org>
> > >>---
> > >>  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.

>         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 :

---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 <suzuki.poulose@arm.com>
---
 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
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel