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

Suzuki K Poulose
2020-08-13T23:43:45Z

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

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