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

Tingwei Zhang
2020-08-14T11:02:24Z

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