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

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:

quoted

On Fri, Aug 14, 2020 at 07:46:12AM +0800, Suzuki K Poulose wrote:

quoted

On 08/13/2020 08:39 PM, Mathieu Poirier wrote:

quoted

On Fri, Aug 07, 2020 at 07:11:39PM +0800, Tingwei Zhang wrote:

quoted

From: Kim Phillips <redacted>

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 <redacted>
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 <redacted>
Signed-off-by: Tingwei Zhang <redacted>
Tested-by: Mike Leach <redacted>
---
 drivers/hwtracing/coresight/Kconfig           |   5 +-
 drivers/hwtracing/coresight/Makefile          |   4 +-
 ...resight-etm4x.c => coresight-etm4x-core.c} | 118

+++++++++++++-----

quoted

quoted

 3 files changed, 92 insertions(+), 35 deletions(-)
 rename drivers/hwtracing/coresight/{coresight-etm4x.c =>

coresight-etm4x-core.c} (95%)

quoted

quoted

diff --git a/drivers/hwtracing/coresight/Kconfig

b/drivers/hwtracing/coresight/Kconfig

quoted

quoted

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

quoted

quoted

version

quoted

quoted

 	  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

quoted

quoted

quoted

quoted

diff --git a/drivers/hwtracing/coresight/Makefile

b/drivers/hwtracing/coresight/Makefile

quoted

quoted

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 \

quoted

quoted

 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

quoted

quoted

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");

quoted

quoted

quoted

quoted

+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

quoted

quoted

*config)

quoted

quoted

 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)

quoted

quoted

 		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)

quoted

quoted

quoted

quoted

 	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.

quoted

quoted

Actually this mutex is for "protecting" etmdrvdata[cpu], not the
drvdata struct as such. But as you said, it becomes like a double

lock.

quoted

quoted

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

quoted

quoted

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

quoted

         * CPU hotplug call backs.
         */
        cpus_read_lock();
        if (cpu_online(drvdata->cpu))
                /*
                 * The readers for etmdrvdata[] are CPU hotplug call

backs

quoted

                 * and PM notification call backs. Change etmdrvdata[i]

on

quoted

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

quoted

        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 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel