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

[Suzuki K Poulose]

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