[PATCH v8 10/24] coresight: etm4x: allow etm4x to be built as a module
Suzuki K Poulose
Suzuki.Poulose at arm.com
Fri Aug 14 06:42:01 EDT 2020
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 at 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 at linaro.org>
> > >>Cc: Leo Yan <leo.yan at linaro.org>
> > >>Cc: Alexander Shishkin <alexander.shishkin at linux.intel.com>
> > >>Cc: Randy Dunlap <rdunlap at infradead.org>
> > >>Cc: Suzuki K Poulose <Suzuki.Poulose at arm.com>
> > >>Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> > >>Cc: Russell King <linux at armlinux.org.uk>
> > >>Signed-off-by: Kim Phillips <kim.phillips at arm.com>
> > >>Signed-off-by: Tingwei Zhang <tingwei at codeaurora.org>
> > >>Tested-by: Mike Leach <mike.leach at 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 at 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
More information about the linux-arm-kernel
mailing list