Subject: Re: [PATCH v3 08/20] coresight: allow etm4x to be built as a module

From: [email-masked]
Date: Sat, 18 Jul 2020 11:25:42 +0800

Hi Mike,

Thanks for reporting this.  This is a good catch.

I was testing on db845c but I didn't encounter this issue with
multiple module load/unload test. I guess it's due to timing difference.
The issue is etm4_cpu_pm_notify() is removed from cpu pm notification
after last etm4 device is removed. There's racing condition when some
etm4 device say etm0 is removed but etm4_cpu_pm_notify() is still
called. I reproduced this issue by manually unbind etm4x device.

The fix is clean etmdrvdata[drvdata->cpu] in etm4_remove() so
etm4_cpu_pm_notify() is gracefully returned. I've verified this on
db845c with manually unbind etm4x device test. Do you mind test
on your db410 as well? I'll put it into v4.

@@ -1542,6 +1543,7 @@ static int __exit etm4_remove(struct amba_device 
*adev)

         etm_perf_symlink(drvdata->csdev, false);

+       etmdrvdata[drvdata->cpu] = NULL;
         if (--etm4_count == 0) {
                 etm4_cpu_pm_unregister();

On 2020-07-18 01:05, Mike Leach wrote:
> Hi Tingwei,
> 
> Couldn't see 00/20 cover note for this set, so reporting this here as
> it relates to etmv4 in part.
> 
> The following sequence:-
> (There is a load dependency here of course, if I try to load ETMv4
> without the coresight core, then I get a lot of missing symbol error
> messages - I assume this is expected behaviour?)

Coresight_etm4x module depends on coresight module, so it's expected
behavior.

> 
> root@linaro-developer:/home/linaro/cs-mods# insmod coresight.ko
> root@linaro-developer:/home/linaro/cs-mods# insmod coresight-etm4x.ko
> 
> correctly loads the coresight core then ETMv4 module.
> 
> [ 1208.214674] coresight etm0: CPU0: ETM v4.0 initialized
> [ 1208.215534] coresight etm1: CPU1: ETM v4.0 initialized
> [ 1208.221020] coresight etm2: CPU2: ETM v4.0 initialized
> [ 1208.224757] coresight etm3: CPU3: ETM v4.0 initialized
> 
> However, if I then unload the ETMv4 module:-
> 
> root@linaro-developer:/home/linaro/cs-mods# rmmod coresight-etm4x.ko
> 
> I get a crash:-
> 
> [ 1215.963689] ------------[ cut here ]------------
> [ 1215.963741] WARNING: CPU: 3 PID: 0 at
> drivers/hwtracing/coresight/coresight-etm4x-core.c:1364
> etm4_cpu_pm_notify+0xad8/0xb48 [coresight_etm4x]
> [ 1215.967373] Modules linked in: coresight_etm4x(-) coresight [last
> unloaded: coresight]
> [ 1215.979960] CPU: 3 PID: 0 Comm: swapper/3 Tainted: G        W
>   5.8.0-rc5cs-modscs-mods-00020-gc03fe910680d #282
> [ 1215.987856] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC
> (DT)
> [ 1215.998531] pstate: 80000085 (Nzcv daIf -PAN -UAO BTYPE=--)
> [ 1216.005396] pc : etm4_cpu_pm_notify+0xad8/0xb48 [coresight_etm4x]
> [ 1216.010687] lr : notifier_call_chain+0x5c/0xa0
> [ 1216.016926] sp : ffff000009fe7d50
> [ 1216.021265] x29: ffff000009fe7d50 x28: 0000000000000000
> [ 1216.024653] x27: 0000011b1d1464b5 x26: 0000000000000001
> [ 1216.030035] x25: ffff00003c79f888 x24: 0000000000000001
> [ 1216.035330] x23: 0000000000000000 x22: 0000000000000000
> [ 1216.040626] x21: 0000000000000000 x20: 0000000000000000
> [ 1216.045921] x19: ffff000038396480 x18: 0000000000000000
> [ 1216.051216] x17: 0000000000000000 x16: 0000000000000000
> [ 1216.056510] x15: 0000000000000000 x14: ffffffffffffffff
> [ 1216.061806] x13: ffffffffffffffff x12: 0000000000000000
> [ 1216.067101] x11: 0000000000000000 x10: 00000000000009b0
> [ 1216.072396] x9 : ffff000009fe7e70 x8 : ffff000009fd4010
> [ 1216.077690] x7 : 0000000000000000 x6 : 0000000023c2c562
> [ 1216.082986] x5 : 00ffffffffffffff x4 : 0000000000000003
> [ 1216.088281] x3 : ffff000038396880 x2 : ffff800008d8d280
> [ 1216.093576] x1 : ffff80002b1aa000 x0 : 000000005f454c42
> [ 1216.098873] Call trace:
> [ 1216.104178]  etm4_cpu_pm_notify+0xad8/0xb48 [coresight_etm4x]
> [ 1216.106343]  notifier_call_chain+0x5c/0xa0
> [ 1216.112242]  __atomic_notifier_call_chain+0x48/0x60
> [ 1216.116236]  cpu_pm_notify+0x44/0x70
> [ 1216.121008]  cpu_pm_enter+0x3c/0x80
> [ 1216.124829]  psci_enter_domain_idle_state+0x38/0xa8
> [ 1216.128040]  cpuidle_enter_state+0x88/0x478
> [ 1216.132900]  cpuidle_enter+0x44/0x58
> [ 1216.137069]  call_cpuidle+0x40/0x70
> [ 1216.140888]  do_idle+0x1e0/0x248
> [ 1216.144099]  cpu_startup_entry+0x28/0x98
> [ 1216.147574]  secondary_start_kernel+0x1a0/0x1f8
> [ 1216.151476] ---[ end trace 97bcd7b8bdd7f9a7 ]---
> 
> This happens each time, though the CPU number can change.
> Test platform is Dragonboard DB410, linux 5.8-rc5 + this patch set.
> 
> Regards
> 
> Mike
> 
> 
> On Fri, 17 Jul 2020 at 06:48, Tingwei Zhang <[email-masked]>
> wrote:
>> 
>> From: Kim Phillips <[email-masked]>
>> 
>> 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
>> 
>> Cc: Mathieu Poirier <[email-masked]>
>> Cc: Leo Yan <[email-masked]>
>> Cc: Alexander Shishkin <[email-masked]>
>> Cc: Randy Dunlap <[email-masked]>
>> Cc: Suzuki K Poulose <[email-masked]>
>> Cc: Greg Kroah-Hartman <[email-masked]>
>> Cc: Russell King <[email-masked]>
>> Signed-off-by: Kim Phillips <[email-masked]>
>> Signed-off-by: Tingwei Zhang <[email-masked]>
>> ---
>>  drivers/hwtracing/coresight/Kconfig           |  5 ++-
>>  drivers/hwtracing/coresight/Makefile          |  4 +--
>>  ...resight-etm4x.c => coresight-etm4x-core.c} | 31 
>> ++++++++++++++++++-
>>  3 files changed, 36 insertions(+), 4 deletions(-)
>>  rename drivers/hwtracing/coresight/{coresight-etm4x.c =>
> coresight-etm4x-core.c} (98%)
>> 
>> 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 98%
>> rename from drivers/hwtracing/coresight/coresight-etm4x.c
>> rename to drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index 747afc875f91..b5945f62794c 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -1536,6 +1536,26 @@ static struct amba_cs_uci_id uci_id_etm4[] = {
>>         }
>>  };
>> 
>> +static int __exit etm4_remove(struct amba_device *adev)
>> +{
>> +       struct etmv4_drvdata *drvdata = dev_get_drvdata(&adev->dev);
>> +
>> +       etm_perf_symlink(drvdata->csdev, false);
>> +
>> +       if (--etm4_count == 0) {
>> +               etm4_cpu_pm_unregister();
>> +
>> +
> cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
>> +               if (hp_online)
>> +                       cpuhp_remove_state_nocalls(hp_online);
>> +       }
>> +
>> +       coresight_unregister(drvdata->csdev);
>> +
>> +       return 0;
>> +}
>> +
>> +
>>  static const struct amba_id etm4_ids[] = {
>>         CS_AMBA_ID(0x000bb95d),                 /* Cortex-A53 */
>>         CS_AMBA_ID(0x000bb95e),                 /* Cortex-A57 */
>> @@ -1553,12 +1573,21 @@ static const struct amba_id etm4_ids[] = {
>>         {},
>>  };
>> 
>> +MODULE_DEVICE_TABLE(amba, etm4_ids);
>> +
>>  static struct amba_driver etm4x_driver = {
>>         .drv = {
>>                 .name   = "coresight-etm4x",
>> +               .owner  = THIS_MODULE,
>>                 .suppress_bind_attrs = true,
>>         },
>>         .probe          = etm4_probe,
>> +       .remove         = etm4_remove,
>>         .id_table       = etm4_ids,
>>  };
>> -builtin_amba_driver(etm4x_driver);
>> +module_amba_driver(etm4x_driver);
>> +
>> +MODULE_AUTHOR("Pratik Patel <[email-masked]>");
>> +MODULE_AUTHOR("Mathieu Poirier <[email-masked]>");
>> +MODULE_DESCRIPTION("Arm CoreSight Program Flow Trace v4 driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
>> a Linux Foundation Collaborative Project
>> 
>> 
>> _______________________________________________
>> linux-arm-kernel mailing list
>> [email-masked]
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
[email-masked]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel