| Message ID | 20200807111153.7784-11-tingwei@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | coresight: allow to build coresight as modules | expand |
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. Because etmdrvdata_lock[] is a per cpu variable I suggest to get rid of drvdata::spinlock altogether. To do so: 1) In etm4_enable_sysfs(), get rid of the spinlock/unlock(). 2) In etm4_enable_hw_smp_call(), grab the mutex based on the value of drvdata->cpu and set drvdata->stick_enable based on the return value of etm4_enable_hw() while holding the mutex. 3) In etm4_disable_sysfs(), just change spinlock/unlock to mutex operations. 4) Get rid of spinlock operations in etm4_starting/dying_cpu(). As Suzuki pointed out the same kind of gymnastic needs to be done for ETMv3. > return 0; > } > > static int etm4_dying_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 (local_read(&etmdrvdata[cpu]->mode)) > etm4_disable_hw(etmdrvdata[cpu]); > spin_unlock(&etmdrvdata[cpu]->spinlock); > + mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); > return 0; > } > > @@ -1360,24 +1373,30 @@ static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd, > { > struct etmv4_drvdata *drvdata; > unsigned int cpu = smp_processor_id(); > + int ret = NOTIFY_OK; > > + mutex_lock(&per_cpu(etmdrvdata_lock, cpu)); > if (!etmdrvdata[cpu]) > - return NOTIFY_OK; > + goto out; > > drvdata = etmdrvdata[cpu]; > > if (!drvdata->save_state) > - return NOTIFY_OK; > + goto out; > > - if (WARN_ON_ONCE(drvdata->cpu != cpu)) > - return NOTIFY_BAD; > + if (WARN_ON_ONCE(drvdata->cpu != cpu)) { > + ret = NOTIFY_BAD; > + goto out; > + } > > switch (cmd) { > case CPU_PM_ENTER: > /* save the state if self-hosted coresight is in use */ > if (local_read(&drvdata->mode)) > - if (etm4_cpu_save(drvdata)) > - return NOTIFY_BAD; > + if (etm4_cpu_save(drvdata)) { > + ret = NOTIFY_BAD; > + goto out; > + } > break; > case CPU_PM_EXIT: > /* fallthrough */ > @@ -1386,10 +1405,12 @@ static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd, > etm4_cpu_restore(drvdata); > break; > default: > - return NOTIFY_DONE; > + goto out; > } > > - return NOTIFY_OK; > +out: > + mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); > + return ret; > } > > static struct notifier_block etm4_cpu_pm_nb = { > @@ -1430,7 +1451,7 @@ static int __init etm4_pm_setup(void) > return ret; > } > > -static void __init etm4_pm_clear(void) > +static void etm4_pm_clear(void) > { > cpu_pm_unregister_notifier(&etm4_cpu_pm_nb); > cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING); > @@ -1487,25 +1508,20 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > if (!desc.name) > return -ENOMEM; > > - 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"); > > - if (etm4_arch_supported(drvdata->arch) == false) { > - ret = -EINVAL; > - goto err_arch_supported; > - } > + if (etm4_arch_supported(drvdata->arch) == false) > + return -EINVAL; > > etm4_init_trace_id(drvdata); > etm4_set_default(&drvdata->config); > > pdata = coresight_get_platform_data(dev); > - if (IS_ERR(pdata)) { > - ret = PTR_ERR(pdata); > - goto err_arch_supported; > - } > + if (IS_ERR(pdata)) > + return PTR_ERR(pdata); > + > adev->dev.platform_data = pdata; > > desc.type = CORESIGHT_DEV_TYPE_SOURCE; > @@ -1515,17 +1531,19 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > desc.dev = dev; > desc.groups = coresight_etmv4_groups; > drvdata->csdev = coresight_register(&desc); > - if (IS_ERR(drvdata->csdev)) { > - ret = PTR_ERR(drvdata->csdev); > - goto err_arch_supported; > - } > + if (IS_ERR(drvdata->csdev)) > + return PTR_ERR(drvdata->csdev); > > ret = etm_perf_symlink(drvdata->csdev, true); > if (ret) { > coresight_unregister(drvdata->csdev); > - goto err_arch_supported; > + return ret; > } > > + mutex_lock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > + etmdrvdata[drvdata->cpu] = drvdata; > + mutex_unlock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > + > 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); > @@ -1536,10 +1554,6 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > } > > return 0; > - > -err_arch_supported: > - etmdrvdata[drvdata->cpu] = NULL; > - return ret; > } > > static struct amba_cs_uci_id uci_id_etm4[] = { > @@ -1551,6 +1565,22 @@ 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); > + > + mutex_lock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > + etmdrvdata[drvdata->cpu] = NULL; > + mutex_unlock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > + > + 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 */ > @@ -1568,18 +1598,26 @@ 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, > }; > > static int __init etm4x_init(void) > { > int ret; > + int cpu; > + > + for_each_possible_cpu(cpu) > + mutex_init(&per_cpu(etmdrvdata_lock, cpu)); > > ret = etm4_pm_setup(); > > @@ -1595,4 +1633,20 @@ static int __init etm4x_init(void) > > return ret; > } > -device_initcall(etm4x_init); > + > +static void __exit etm4x_exit(void) > +{ > + int cpu; > + > + amba_driver_unregister(&etm4x_driver); > + etm4_pm_clear(); > + for_each_possible_cpu(cpu) > + mutex_destroy(&per_cpu(etmdrvdata_lock, cpu)); > +} > +module_init(etm4x_init); > +module_exit(etm4x_exit); > + > +MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>"); > +MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>"); > +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 >
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)); > return 0; > } > > static int etm4_dying_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 (local_read(&etmdrvdata[cpu]->mode)) > etm4_disable_hw(etmdrvdata[cpu]); > spin_unlock(&etmdrvdata[cpu]->spinlock); > + mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); > return 0; > } > > @@ -1360,24 +1373,30 @@ static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd, > { > struct etmv4_drvdata *drvdata; > unsigned int cpu = smp_processor_id(); > + int ret = NOTIFY_OK; > > + mutex_lock(&per_cpu(etmdrvdata_lock, cpu)); > if (!etmdrvdata[cpu]) > - return NOTIFY_OK; > + goto out; > > drvdata = etmdrvdata[cpu]; > > if (!drvdata->save_state) > - return NOTIFY_OK; > + goto out; > > - if (WARN_ON_ONCE(drvdata->cpu != cpu)) > - return NOTIFY_BAD; > + if (WARN_ON_ONCE(drvdata->cpu != cpu)) { > + ret = NOTIFY_BAD; > + goto out; > + } > > switch (cmd) { > case CPU_PM_ENTER: > /* save the state if self-hosted coresight is in use */ > if (local_read(&drvdata->mode)) > - if (etm4_cpu_save(drvdata)) > - return NOTIFY_BAD; > + if (etm4_cpu_save(drvdata)) { > + ret = NOTIFY_BAD; > + goto out; > + } > break; > case CPU_PM_EXIT: > /* fallthrough */ > @@ -1386,10 +1405,12 @@ static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd, > etm4_cpu_restore(drvdata); > break; > default: > - return NOTIFY_DONE; > + goto out; > } > > - return NOTIFY_OK; > +out: > + mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); > + return ret; > } > > static struct notifier_block etm4_cpu_pm_nb = { > @@ -1430,7 +1451,7 @@ static int __init etm4_pm_setup(void) > return ret; > } > > -static void __init etm4_pm_clear(void) > +static void etm4_pm_clear(void) > { > cpu_pm_unregister_notifier(&etm4_cpu_pm_nb); > cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING); > @@ -1487,25 +1508,20 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > if (!desc.name) > return -ENOMEM; > > - 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"); > > - if (etm4_arch_supported(drvdata->arch) == false) { > - ret = -EINVAL; > - goto err_arch_supported; > - } > + if (etm4_arch_supported(drvdata->arch) == false) > + return -EINVAL; > > etm4_init_trace_id(drvdata); > etm4_set_default(&drvdata->config); > > pdata = coresight_get_platform_data(dev); > - if (IS_ERR(pdata)) { > - ret = PTR_ERR(pdata); > - goto err_arch_supported; > - } > + if (IS_ERR(pdata)) > + return PTR_ERR(pdata); > + > adev->dev.platform_data = pdata; > > desc.type = CORESIGHT_DEV_TYPE_SOURCE; > @@ -1515,17 +1531,19 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > desc.dev = dev; > desc.groups = coresight_etmv4_groups; > drvdata->csdev = coresight_register(&desc); > - if (IS_ERR(drvdata->csdev)) { > - ret = PTR_ERR(drvdata->csdev); > - goto err_arch_supported; > - } > + if (IS_ERR(drvdata->csdev)) > + return PTR_ERR(drvdata->csdev); > > ret = etm_perf_symlink(drvdata->csdev, true); > if (ret) { > coresight_unregister(drvdata->csdev); > - goto err_arch_supported; > + return ret; > } > > + mutex_lock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > + etmdrvdata[drvdata->cpu] = drvdata; > + mutex_unlock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > + > 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); > @@ -1536,10 +1554,6 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > } > > return 0; > - > -err_arch_supported: > - etmdrvdata[drvdata->cpu] = NULL; > - return ret; > } > > static struct amba_cs_uci_id uci_id_etm4[] = { > @@ -1551,6 +1565,22 @@ 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); > + > + mutex_lock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > + etmdrvdata[drvdata->cpu] = NULL; > + mutex_unlock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > + > + 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 */ > @@ -1568,18 +1598,26 @@ 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, > }; > > static int __init etm4x_init(void) > { > int ret; > + int cpu; > + > + for_each_possible_cpu(cpu) > + mutex_init(&per_cpu(etmdrvdata_lock, cpu)); > > ret = etm4_pm_setup(); > > @@ -1595,4 +1633,20 @@ static int __init etm4x_init(void) > > return ret; > } > -device_initcall(etm4x_init); > + > +static void __exit etm4x_exit(void) > +{ > + int cpu; > + > + amba_driver_unregister(&etm4x_driver); > + etm4_pm_clear(); > + for_each_possible_cpu(cpu) > + mutex_destroy(&per_cpu(etmdrvdata_lock, cpu)); > +} > +module_init(etm4x_init); > +module_exit(etm4x_exit); > + > +MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>"); > +MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>"); > +MODULE_DESCRIPTION("Arm CoreSight Program Flow Trace v4 driver"); ... And this should be "Arm CoreSight Embedded Trace Macrocell 4.x driver". > +MODULE_LICENSE("GPL v2"); > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
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)); > return 0; > } > > static int etm4_dying_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 (local_read(&etmdrvdata[cpu]->mode)) > etm4_disable_hw(etmdrvdata[cpu]); > spin_unlock(&etmdrvdata[cpu]->spinlock); > + mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); > return 0; > } > > @@ -1360,24 +1373,30 @@ static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd, > { > struct etmv4_drvdata *drvdata; > unsigned int cpu = smp_processor_id(); > + int ret = NOTIFY_OK; > > + mutex_lock(&per_cpu(etmdrvdata_lock, cpu)); > if (!etmdrvdata[cpu]) > - return NOTIFY_OK; > + goto out; > > drvdata = etmdrvdata[cpu]; > > if (!drvdata->save_state) > - return NOTIFY_OK; > + goto out; > > - if (WARN_ON_ONCE(drvdata->cpu != cpu)) > - return NOTIFY_BAD; > + if (WARN_ON_ONCE(drvdata->cpu != cpu)) { > + ret = NOTIFY_BAD; > + goto out; > + } > > switch (cmd) { > case CPU_PM_ENTER: > /* save the state if self-hosted coresight is in use */ > if (local_read(&drvdata->mode)) > - if (etm4_cpu_save(drvdata)) > - return NOTIFY_BAD; > + if (etm4_cpu_save(drvdata)) { > + ret = NOTIFY_BAD; > + goto out; > + } > break; > case CPU_PM_EXIT: > /* fallthrough */ > @@ -1386,10 +1405,12 @@ static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd, > etm4_cpu_restore(drvdata); > break; > default: > - return NOTIFY_DONE; > + goto out; > } > > - return NOTIFY_OK; > +out: > + mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); > + return ret; > } > > static struct notifier_block etm4_cpu_pm_nb = { > @@ -1430,7 +1451,7 @@ static int __init etm4_pm_setup(void) > return ret; > } > > -static void __init etm4_pm_clear(void) > +static void etm4_pm_clear(void) > { > cpu_pm_unregister_notifier(&etm4_cpu_pm_nb); > cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING); > @@ -1487,25 +1508,20 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > if (!desc.name) > return -ENOMEM; > > - 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"); > > - if (etm4_arch_supported(drvdata->arch) == false) { > - ret = -EINVAL; > - goto err_arch_supported; > - } > + if (etm4_arch_supported(drvdata->arch) == false) > + return -EINVAL; > > etm4_init_trace_id(drvdata); > etm4_set_default(&drvdata->config); > > pdata = coresight_get_platform_data(dev); > - if (IS_ERR(pdata)) { > - ret = PTR_ERR(pdata); > - goto err_arch_supported; > - } > + if (IS_ERR(pdata)) > + return PTR_ERR(pdata); > + > adev->dev.platform_data = pdata; > > desc.type = CORESIGHT_DEV_TYPE_SOURCE; > @@ -1515,17 +1531,19 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > desc.dev = dev; > desc.groups = coresight_etmv4_groups; > drvdata->csdev = coresight_register(&desc); > - if (IS_ERR(drvdata->csdev)) { > - ret = PTR_ERR(drvdata->csdev); > - goto err_arch_supported; > - } > + if (IS_ERR(drvdata->csdev)) > + return PTR_ERR(drvdata->csdev); > > ret = etm_perf_symlink(drvdata->csdev, true); > if (ret) { > coresight_unregister(drvdata->csdev); > - goto err_arch_supported; > + return ret; > } > > + mutex_lock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > + etmdrvdata[drvdata->cpu] = drvdata; > + mutex_unlock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > + > 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); > @@ -1536,10 +1554,6 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) > } > > return 0; > - > -err_arch_supported: > - etmdrvdata[drvdata->cpu] = NULL; > - return ret; > } > > static struct amba_cs_uci_id uci_id_etm4[] = { > @@ -1551,6 +1565,22 @@ 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); > + > + mutex_lock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > + etmdrvdata[drvdata->cpu] = NULL; > + mutex_unlock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > + > + coresight_unregister(drvdata->csdev); > + > + return 0; > +} > + > + Extra line > static const struct amba_id etm4_ids[] = { > CS_AMBA_ID(0x000bb95d), /* Cortex-A53 */ > CS_AMBA_ID(0x000bb95e), /* Cortex-A57 */ > @@ -1568,18 +1598,26 @@ 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, > }; > > static int __init etm4x_init(void) > { > int ret; > + int cpu; > + > + for_each_possible_cpu(cpu) > + mutex_init(&per_cpu(etmdrvdata_lock, cpu)); > > ret = etm4_pm_setup(); > > @@ -1595,4 +1633,20 @@ static int __init etm4x_init(void) > > return ret; > } > -device_initcall(etm4x_init); > + > +static void __exit etm4x_exit(void) > +{ > + int cpu; > + > + amba_driver_unregister(&etm4x_driver); > + etm4_pm_clear(); > + for_each_possible_cpu(cpu) > + mutex_destroy(&per_cpu(etmdrvdata_lock, cpu)); > +} > +module_init(etm4x_init); > +module_exit(etm4x_exit); > + > +MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>"); > +MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>"); > +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 >
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
On Fri, Aug 14, 2020 at 03:45:06AM +0800, 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)); > > return 0; > > } > > > > static int etm4_dying_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 (local_read(&etmdrvdata[cpu]->mode)) > > etm4_disable_hw(etmdrvdata[cpu]); > > spin_unlock(&etmdrvdata[cpu]->spinlock); > > + mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); > > return 0; > > } > > > > @@ -1360,24 +1373,30 @@ static int etm4_cpu_pm_notify(struct > notifier_block *nb, unsigned long cmd, > > { > > struct etmv4_drvdata *drvdata; > > unsigned int cpu = smp_processor_id(); > > + int ret = NOTIFY_OK; > > > > + mutex_lock(&per_cpu(etmdrvdata_lock, cpu)); > > if (!etmdrvdata[cpu]) > > - return NOTIFY_OK; > > + goto out; > > > > drvdata = etmdrvdata[cpu]; > > > > if (!drvdata->save_state) > > - return NOTIFY_OK; > > + goto out; > > > > - if (WARN_ON_ONCE(drvdata->cpu != cpu)) > > - return NOTIFY_BAD; > > + if (WARN_ON_ONCE(drvdata->cpu != cpu)) { > > + ret = NOTIFY_BAD; > > + goto out; > > + } > > > > switch (cmd) { > > case CPU_PM_ENTER: > > /* save the state if self-hosted coresight is in use */ > > if (local_read(&drvdata->mode)) > > - if (etm4_cpu_save(drvdata)) > > - return NOTIFY_BAD; > > + if (etm4_cpu_save(drvdata)) { > > + ret = NOTIFY_BAD; > > + goto out; > > + } > > break; > > case CPU_PM_EXIT: > > /* fallthrough */ > > @@ -1386,10 +1405,12 @@ static int etm4_cpu_pm_notify(struct > notifier_block *nb, unsigned long cmd, > > etm4_cpu_restore(drvdata); > > break; > > default: > > - return NOTIFY_DONE; > > + goto out; > > } > > > > - return NOTIFY_OK; > > +out: > > + mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); > > + return ret; > > } > > > > static struct notifier_block etm4_cpu_pm_nb = { > > @@ -1430,7 +1451,7 @@ static int __init etm4_pm_setup(void) > > return ret; > > } > > > > -static void __init etm4_pm_clear(void) > > +static void etm4_pm_clear(void) > > { > > cpu_pm_unregister_notifier(&etm4_cpu_pm_nb); > > cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING); > > @@ -1487,25 +1508,20 @@ static int etm4_probe(struct amba_device *adev, > const struct amba_id *id) > > if (!desc.name) > > return -ENOMEM; > > > > - 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"); > > > > - if (etm4_arch_supported(drvdata->arch) == false) { > > - ret = -EINVAL; > > - goto err_arch_supported; > > - } > > + if (etm4_arch_supported(drvdata->arch) == false) > > + return -EINVAL; > > > > etm4_init_trace_id(drvdata); > > etm4_set_default(&drvdata->config); > > > > pdata = coresight_get_platform_data(dev); > > - if (IS_ERR(pdata)) { > > - ret = PTR_ERR(pdata); > > - goto err_arch_supported; > > - } > > + if (IS_ERR(pdata)) > > + return PTR_ERR(pdata); > > + > > adev->dev.platform_data = pdata; > > > > desc.type = CORESIGHT_DEV_TYPE_SOURCE; > > @@ -1515,17 +1531,19 @@ static int etm4_probe(struct amba_device *adev, > const struct amba_id *id) > > desc.dev = dev; > > desc.groups = coresight_etmv4_groups; > > drvdata->csdev = coresight_register(&desc); > > - if (IS_ERR(drvdata->csdev)) { > > - ret = PTR_ERR(drvdata->csdev); > > - goto err_arch_supported; > > - } > > + if (IS_ERR(drvdata->csdev)) > > + return PTR_ERR(drvdata->csdev); > > > > ret = etm_perf_symlink(drvdata->csdev, true); > > if (ret) { > > coresight_unregister(drvdata->csdev); > > - goto err_arch_supported; > > + return ret; > > } > > > > + mutex_lock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > > + etmdrvdata[drvdata->cpu] = drvdata; > > + mutex_unlock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > > + > > 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); > > @@ -1536,10 +1554,6 @@ static int etm4_probe(struct amba_device *adev, > const struct amba_id *id) > > } > > > > return 0; > > - > > -err_arch_supported: > > - etmdrvdata[drvdata->cpu] = NULL; > > - return ret; > > } > > > > static struct amba_cs_uci_id uci_id_etm4[] = { > > @@ -1551,6 +1565,22 @@ 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); > > + > > + mutex_lock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > > + etmdrvdata[drvdata->cpu] = NULL; > > + mutex_unlock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > > + > > + coresight_unregister(drvdata->csdev); > > + > > + return 0; > > +} > > + > > + > > Extra line > Will fix in next revision. > > static const struct amba_id etm4_ids[] = { > > CS_AMBA_ID(0x000bb95d), /* Cortex-A53 */ > > CS_AMBA_ID(0x000bb95e), /* Cortex-A57 */ > > @@ -1568,18 +1598,26 @@ 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, > > }; > > > > static int __init etm4x_init(void) > > { > > int ret; > > + int cpu; > > + > > + for_each_possible_cpu(cpu) > > + mutex_init(&per_cpu(etmdrvdata_lock, cpu)); > > > > ret = etm4_pm_setup(); > > > > @@ -1595,4 +1633,20 @@ static int __init etm4x_init(void) > > > > return ret; > > } > > -device_initcall(etm4x_init); > > + > > +static void __exit etm4x_exit(void) > > +{ > > + int cpu; > > + > > + amba_driver_unregister(&etm4x_driver); > > + etm4_pm_clear(); > > + for_each_possible_cpu(cpu) > > + mutex_destroy(&per_cpu(etmdrvdata_lock, cpu)); > > +} > > +module_init(etm4x_init); > > +module_exit(etm4x_exit); > > + > > +MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>"); > > +MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>"); > > +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 > >
On Fri, Aug 14, 2020 at 03:40:57AM +0800, 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)); > > return 0; > > } > > > > static int etm4_dying_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 (local_read(&etmdrvdata[cpu]->mode)) > > etm4_disable_hw(etmdrvdata[cpu]); > > spin_unlock(&etmdrvdata[cpu]->spinlock); > > + mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); > > return 0; > > } > > > > @@ -1360,24 +1373,30 @@ static int etm4_cpu_pm_notify(struct > notifier_block *nb, unsigned long cmd, > > { > > struct etmv4_drvdata *drvdata; > > unsigned int cpu = smp_processor_id(); > > + int ret = NOTIFY_OK; > > > > + mutex_lock(&per_cpu(etmdrvdata_lock, cpu)); > > if (!etmdrvdata[cpu]) > > - return NOTIFY_OK; > > + goto out; > > > > drvdata = etmdrvdata[cpu]; > > > > if (!drvdata->save_state) > > - return NOTIFY_OK; > > + goto out; > > > > - if (WARN_ON_ONCE(drvdata->cpu != cpu)) > > - return NOTIFY_BAD; > > + if (WARN_ON_ONCE(drvdata->cpu != cpu)) { > > + ret = NOTIFY_BAD; > > + goto out; > > + } > > > > switch (cmd) { > > case CPU_PM_ENTER: > > /* save the state if self-hosted coresight is in use */ > > if (local_read(&drvdata->mode)) > > - if (etm4_cpu_save(drvdata)) > > - return NOTIFY_BAD; > > + if (etm4_cpu_save(drvdata)) { > > + ret = NOTIFY_BAD; > > + goto out; > > + } > > break; > > case CPU_PM_EXIT: > > /* fallthrough */ > > @@ -1386,10 +1405,12 @@ static int etm4_cpu_pm_notify(struct > notifier_block *nb, unsigned long cmd, > > etm4_cpu_restore(drvdata); > > break; > > default: > > - return NOTIFY_DONE; > > + goto out; > > } > > > > - return NOTIFY_OK; > > +out: > > + mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); > > + return ret; > > } > > > > static struct notifier_block etm4_cpu_pm_nb = { > > @@ -1430,7 +1451,7 @@ static int __init etm4_pm_setup(void) > > return ret; > > } > > > > -static void __init etm4_pm_clear(void) > > +static void etm4_pm_clear(void) > > { > > cpu_pm_unregister_notifier(&etm4_cpu_pm_nb); > > cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING); > > @@ -1487,25 +1508,20 @@ static int etm4_probe(struct amba_device *adev, > const struct amba_id *id) > > if (!desc.name) > > return -ENOMEM; > > > > - 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"); > > > > - if (etm4_arch_supported(drvdata->arch) == false) { > > - ret = -EINVAL; > > - goto err_arch_supported; > > - } > > + if (etm4_arch_supported(drvdata->arch) == false) > > + return -EINVAL; > > > > etm4_init_trace_id(drvdata); > > etm4_set_default(&drvdata->config); > > > > pdata = coresight_get_platform_data(dev); > > - if (IS_ERR(pdata)) { > > - ret = PTR_ERR(pdata); > > - goto err_arch_supported; > > - } > > + if (IS_ERR(pdata)) > > + return PTR_ERR(pdata); > > + > > adev->dev.platform_data = pdata; > > > > desc.type = CORESIGHT_DEV_TYPE_SOURCE; > > @@ -1515,17 +1531,19 @@ static int etm4_probe(struct amba_device *adev, > const struct amba_id *id) > > desc.dev = dev; > > desc.groups = coresight_etmv4_groups; > > drvdata->csdev = coresight_register(&desc); > > - if (IS_ERR(drvdata->csdev)) { > > - ret = PTR_ERR(drvdata->csdev); > > - goto err_arch_supported; > > - } > > + if (IS_ERR(drvdata->csdev)) > > + return PTR_ERR(drvdata->csdev); > > > > ret = etm_perf_symlink(drvdata->csdev, true); > > if (ret) { > > coresight_unregister(drvdata->csdev); > > - goto err_arch_supported; > > + return ret; > > } > > > > + mutex_lock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > > + etmdrvdata[drvdata->cpu] = drvdata; > > + mutex_unlock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > > + > > 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); > > @@ -1536,10 +1554,6 @@ static int etm4_probe(struct amba_device *adev, > const struct amba_id *id) > > } > > > > return 0; > > - > > -err_arch_supported: > > - etmdrvdata[drvdata->cpu] = NULL; > > - return ret; > > } > > > > static struct amba_cs_uci_id uci_id_etm4[] = { > > @@ -1551,6 +1565,22 @@ 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); > > + > > + mutex_lock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > > + etmdrvdata[drvdata->cpu] = NULL; > > + mutex_unlock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); > > + > > + 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 */ > > @@ -1568,18 +1598,26 @@ 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, > > }; > > > > static int __init etm4x_init(void) > > { > > int ret; > > + int cpu; > > + > > + for_each_possible_cpu(cpu) > > + mutex_init(&per_cpu(etmdrvdata_lock, cpu)); > > > > ret = etm4_pm_setup(); > > > > @@ -1595,4 +1633,20 @@ static int __init etm4x_init(void) > > > > return ret; > > } > > -device_initcall(etm4x_init); > > + > > +static void __exit etm4x_exit(void) > > +{ > > + int cpu; > > + > > + amba_driver_unregister(&etm4x_driver); > > + etm4_pm_clear(); > > + for_each_possible_cpu(cpu) > > + mutex_destroy(&per_cpu(etmdrvdata_lock, cpu)); > > +} > > +module_init(etm4x_init); > > +module_exit(etm4x_exit); > > + > > +MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>"); > > +MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>"); > > +MODULE_DESCRIPTION("Arm CoreSight Program Flow Trace v4 driver"); > > ... And this should be "Arm CoreSight Embedded Trace Macrocell 4.x > driver". > This will be fixed in v9. > > +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 > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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); else etmdrvdata[drvdata->cpu] = NULL; cpus_read_unlock(); coresight_unregister(drvdata->csdev); return 0; } Thanks, Tingwei > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
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. > 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 : ---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; }
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 >
On 08/14/2020 12:00 PM, Tingwei Zhang wrote: > 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> >>>>>> @@ -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. :) Oh, yes. I see that now :-) Cheers Suzuki
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)); return 0; } static int etm4_dying_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 (local_read(&etmdrvdata[cpu]->mode)) etm4_disable_hw(etmdrvdata[cpu]); spin_unlock(&etmdrvdata[cpu]->spinlock); + mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); return 0; } @@ -1360,24 +1373,30 @@ static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd, { struct etmv4_drvdata *drvdata; unsigned int cpu = smp_processor_id(); + int ret = NOTIFY_OK; + mutex_lock(&per_cpu(etmdrvdata_lock, cpu)); if (!etmdrvdata[cpu]) - return NOTIFY_OK; + goto out; drvdata = etmdrvdata[cpu]; if (!drvdata->save_state) - return NOTIFY_OK; + goto out; - if (WARN_ON_ONCE(drvdata->cpu != cpu)) - return NOTIFY_BAD; + if (WARN_ON_ONCE(drvdata->cpu != cpu)) { + ret = NOTIFY_BAD; + goto out; + } switch (cmd) { case CPU_PM_ENTER: /* save the state if self-hosted coresight is in use */ if (local_read(&drvdata->mode)) - if (etm4_cpu_save(drvdata)) - return NOTIFY_BAD; + if (etm4_cpu_save(drvdata)) { + ret = NOTIFY_BAD; + goto out; + } break; case CPU_PM_EXIT: /* fallthrough */ @@ -1386,10 +1405,12 @@ static int etm4_cpu_pm_notify(struct notifier_block *nb, unsigned long cmd, etm4_cpu_restore(drvdata); break; default: - return NOTIFY_DONE; + goto out; } - return NOTIFY_OK; +out: + mutex_unlock(&per_cpu(etmdrvdata_lock, cpu)); + return ret; } static struct notifier_block etm4_cpu_pm_nb = { @@ -1430,7 +1451,7 @@ static int __init etm4_pm_setup(void) return ret; } -static void __init etm4_pm_clear(void) +static void etm4_pm_clear(void) { cpu_pm_unregister_notifier(&etm4_cpu_pm_nb); cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING); @@ -1487,25 +1508,20 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) if (!desc.name) return -ENOMEM; - 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"); - if (etm4_arch_supported(drvdata->arch) == false) { - ret = -EINVAL; - goto err_arch_supported; - } + if (etm4_arch_supported(drvdata->arch) == false) + return -EINVAL; etm4_init_trace_id(drvdata); etm4_set_default(&drvdata->config); pdata = coresight_get_platform_data(dev); - if (IS_ERR(pdata)) { - ret = PTR_ERR(pdata); - goto err_arch_supported; - } + if (IS_ERR(pdata)) + return PTR_ERR(pdata); + adev->dev.platform_data = pdata; desc.type = CORESIGHT_DEV_TYPE_SOURCE; @@ -1515,17 +1531,19 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) desc.dev = dev; desc.groups = coresight_etmv4_groups; drvdata->csdev = coresight_register(&desc); - if (IS_ERR(drvdata->csdev)) { - ret = PTR_ERR(drvdata->csdev); - goto err_arch_supported; - } + if (IS_ERR(drvdata->csdev)) + return PTR_ERR(drvdata->csdev); ret = etm_perf_symlink(drvdata->csdev, true); if (ret) { coresight_unregister(drvdata->csdev); - goto err_arch_supported; + return ret; } + mutex_lock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); + etmdrvdata[drvdata->cpu] = drvdata; + mutex_unlock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); + 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); @@ -1536,10 +1554,6 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) } return 0; - -err_arch_supported: - etmdrvdata[drvdata->cpu] = NULL; - return ret; } static struct amba_cs_uci_id uci_id_etm4[] = { @@ -1551,6 +1565,22 @@ 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); + + mutex_lock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); + etmdrvdata[drvdata->cpu] = NULL; + mutex_unlock(&per_cpu(etmdrvdata_lock, drvdata->cpu)); + + 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 */ @@ -1568,18 +1598,26 @@ 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, }; static int __init etm4x_init(void) { int ret; + int cpu; + + for_each_possible_cpu(cpu) + mutex_init(&per_cpu(etmdrvdata_lock, cpu)); ret = etm4_pm_setup(); @@ -1595,4 +1633,20 @@ static int __init etm4x_init(void) return ret; } -device_initcall(etm4x_init); + +static void __exit etm4x_exit(void) +{ + int cpu; + + amba_driver_unregister(&etm4x_driver); + etm4_pm_clear(); + for_each_possible_cpu(cpu) + mutex_destroy(&per_cpu(etmdrvdata_lock, cpu)); +} +module_init(etm4x_init); +module_exit(etm4x_exit); + +MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>"); +MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>"); +MODULE_DESCRIPTION("Arm CoreSight Program Flow Trace v4 driver"); +MODULE_LICENSE("GPL v2");