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

From: Kim Phillips kim.phillips@arm.com

Checking for ifdef CONFIG_x fails if CONFIG_x=m. Use IS_ENABLED that is true for both built-ins and modules, instead. Required when building coresight components as modules.

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 Reviewed-by: Suzuki K Poulose suzuki.poulose@arm.com

---

drivers/hwtracing/coresight/coresight-etm-perf.h | 2 +- drivers/hwtracing/coresight/coresight-priv.h | 2 +- include/linux/coresight.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)

For patches 02 and 03:

Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h index 015213abe00a..05f89723e282 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.h +++ b/drivers/hwtracing/coresight/coresight-etm-perf.h @@ -57,7 +57,7 @@ struct etm_event_data { struct list_head * __percpu *path; }; -#ifdef CONFIG_CORESIGHT +#if IS_ENABLED(CONFIG_CORESIGHT) int etm_perf_symlink(struct coresight_device *csdev, bool link); int etm_perf_add_symlink_sink(struct coresight_device *csdev); void etm_perf_del_symlink_sink(struct coresight_device *csdev); diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index f2dc625ea585..d801a2755432 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -165,7 +165,7 @@ int coresight_make_links(struct coresight_device *orig, void coresight_remove_links(struct coresight_device *orig, struct coresight_connection *conn); -#ifdef CONFIG_CORESIGHT_SOURCE_ETM3X +#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM3X) extern int etm_readl_cp14(u32 off, unsigned int *val); extern int etm_writel_cp14(u32 off, u32 val); #else diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 58fffdecdbfd..3bb738f9a326 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -324,7 +324,7 @@ struct coresight_ops { const struct coresight_ops_ect *ect_ops; }; -#ifdef CONFIG_CORESIGHT +#if IS_ENABLED(CONFIG_CORESIGHT) extern struct coresight_device * coresight_register(struct coresight_desc *desc); extern void coresight_unregister(struct coresight_device *csdev); -- 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:33PM +0800, Tingwei Zhang wrote:

Add coresight prefix to make it specific. It will be a export symbol.

Signed-off-by: Mian Yousaf Kaukab ykaukab@suse.de Signed-off-by: Tingwei Zhang tingwei@codeaurora.org Reviewed-by: Suzuki K Poulose suzuki.poulose@arm.com

---

drivers/hwtracing/coresight/coresight-etb10.c | 2 +- drivers/hwtracing/coresight/coresight-priv.h | 8 ++++---- drivers/hwtracing/coresight/coresight-tmc-etf.c | 2 +- drivers/hwtracing/coresight/coresight.c | 3 ++- 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c index 03e3f2590191..04ee9cda988d 100644 --- a/drivers/hwtracing/coresight/coresight-etb10.c +++ b/drivers/hwtracing/coresight/coresight-etb10.c @@ -525,7 +525,7 @@ static unsigned long etb_update_buffer(struct coresight_device *csdev, cur = buf->cur; offset = buf->offset;

• barrier = barrier_pkt;

• barrier = coresight_barrier_pkt;

for (i = 0; i < to_read; i += 4) { buf_ptr = buf->data_pages[cur] + offset; diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index d801a2755432..dcb8aeb6af62 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -66,8 +66,8 @@ static DEVICE_ATTR_RO(name) #define coresight_simple_reg64(type, name, lo_off, hi_off) \ __coresight_simple_func(type, NULL, name, lo_off, hi_off) -extern const u32 barrier_pkt[4]; -#define CORESIGHT_BARRIER_PKT_SIZE (sizeof(barrier_pkt)) +extern const u32 coresight_barrier_pkt[4]; +#define CORESIGHT_BARRIER_PKT_SIZE (sizeof(coresight_barrier_pkt)) enum etm_addr_type { ETM_ADDR_TYPE_NONE, @@ -104,10 +104,10 @@ struct cs_buffers { static inline void coresight_insert_barrier_packet(void *buf) { if (buf)

• memcpy(buf, barrier_pkt, CORESIGHT_BARRIER_PKT_SIZE);
  

• memcpy(buf, coresight_barrier_pkt,
  

• 		CORESIGHT_BARRIER_PKT_SIZE);
  

Indentation problem.

}

static inline void CS_LOCK(void __iomem *addr) { do { diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index 6375504ba8b0..44402d413ebb 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -519,7 +519,7 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev, cur = buf->cur; offset = buf->offset;

• barrier = barrier_pkt;

• barrier = coresight_barrier_pkt;

/* for every byte to read */ for (i = 0; i < to_read; i += 4) { diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index e9c90f2de34a..310b1b825dd1 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -53,7 +53,8 @@ static struct list_head *stm_path;

• beginning of the data collected in a buffer. That way the decoder knows that
• it needs to look for another sync sequence.

*/ -const u32 barrier_pkt[4] = {0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fffffff}; +const u32 coresight_barrier_pkt[4] = {

• 0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fffffff};
  

As of kernel 5.8 lines up to 100 characters are allowed. No need to break this line.

With that:

Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org

static int coresight_id_match(struct device *dev, void *data) { -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

On Thu, Aug 13, 2020 at 03:36:12AM +0800, Mathieu Poirier wrote:

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

...

When coresight device is in an active session, driver module of that device should not be removed. Use try_get_module() in coresight_grab_device() to prevent module to be unloaded. Use get_device()/put_device() to protect device data in the middle of active session.

Signed-off-by: Tingwei Zhang tingwei@codeaurora.org Tested-by: Mike Leach mike.leach@linaro.org Suggested-by: Suzuki K Poulose suzuki.poulose@arm.com

---

drivers/hwtracing/coresight/coresight.c | 61 ++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 7 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight.c

b/drivers/hwtracing/coresight/coresight.c

...

index b7151c5f81b1..0b0e31577b9b 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -634,13 +634,45 @@ struct coresight_device

*coresight_get_sink_by_id(u32 id)

...

return dev ? to_coresight_device(dev) : NULL; } +/**

• • coresight_get_ref- Helper function to increase reference count to

module

...

• • and device.
• • Return true in successful case and power up the device.
• • Return false when failed to get reference of module.
• */

+static inline bool coresight_get_ref(struct coresight_device *csdev) +{

• struct device *dev = csdev->dev.parent;
• /* Make sure the driver can't be removed */
• if (!try_module_get(dev->driver->owner))

• return false;
  

• /* Make sure the device can't go away */
• get_device(dev);
• pm_runtime_get_sync(dev);
• return true;

+}

+/**

• • coresight_put_ref- Helper function to decrease reference count to

module

...

• • and device. Power off the device.
• */

+static inline void coresight_put_ref(struct coresight_device *csdev) +{

• struct device *dev = csdev->dev.parent;
• pm_runtime_put(dev);
• put_device(dev);
• module_put(dev->driver->owner);

+}

/*

• coresight_grab_device - Power up this device and any of the helper
• devices connected to it for trace operation. Since the helper

devices

...

• don't appear on the trace path, they should be handled along with

the

...

• the master device.

*/ -static void coresight_grab_device(struct coresight_device *csdev) +static int coresight_grab_device(struct coresight_device *csdev) { int i; @@ -649,9 +681,20 @@ static void coresight_grab_device(struct

coresight_device *csdev)

...

child = csdev->pdata->conns[i].child_dev; if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)

• 	pm_runtime_get_sync(child->dev.parent);
  

• 	if (!coresight_get_ref(child))
  

• 		goto err;
  

• }
• if (coresight_get_ref(csdev))

• return 0;
  

+err:

• for (i--; i >= 0; i--) {

• struct coresight_device *child;
  

• child  = csdev->pdata->conns[i].child_dev;
  

• if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
  

• 	coresight_put_ref(child);
  

  }

• pm_runtime_get_sync(csdev->dev.parent);

• return -ENODEV;

} /* @@ -662,13 +705,13 @@ static void coresight_drop_device(struct

coresight_device *csdev)

...

{ int i;

• pm_runtime_put(csdev->dev.parent);

• coresight_put_ref(csdev); for (i = 0; i < csdev->pdata->nr_outport; i++) { struct coresight_device *child;

child = csdev->pdata->conns[i].child_dev; if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)

• 	pm_runtime_put(child->dev.parent);
  

• 	coresight_put_ref(child);
  

  }

} @@ -687,7 +730,7 @@ static int _coresight_build_path(struct

coresight_device *csdev,

...

		 struct coresight_device *sink,
		 struct list_head *path)

{

• int i;

• int i, ret; bool found = false; struct coresight_node *node;

@@ -721,7 +764,11 @@ static int _coresight_build_path(struct

coresight_device *csdev,

...

if (!node) return -ENOMEM;

• coresight_grab_device(csdev);

• ret = coresight_grab_device(csdev);

I suggest trying to grab the device before allocating memory for the node. If grabing the device fails memory doesn't have to be allocated and released needlessly.

Agree. I'll fix in next revision.

Thanks, Tingwei

I will likely come back to this patch later after reviewing the rest the this set.

Thanks, Mathieu

...

• if (ret) {

• kfree(node);
  

• return ret;
  

• } node->csdev = csdev; list_add(&node->link, path);

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

...

a Linux Foundation Collaborative Project

On Thu, Aug 13, 2020 at 04:47:41AM +0800, Mathieu Poirier wrote:

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

...

From: Kim Phillips kim.phillips@arm.com

Allow to build coresight-etm3x 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-etm3x by the Makefile
• add an etm_remove function, for module unload
• add a MODULE_DEVICE_TABLE for autoloading on boot

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 Reviewed-by: Mike Leach mike.leach@linaro.org

---

drivers/hwtracing/coresight/Kconfig | 5 +++- drivers/hwtracing/coresight/Makefile | 3 ++- ...resight-etm3x.c => coresight-etm3x-core.c} | 27 ++++++++++++++++++- 3 files changed, 32 insertions(+), 3 deletions(-) rename drivers/hwtracing/coresight/{coresight-etm3x.c =>

coresight-etm3x-core.c} (97%)

...

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

b/drivers/hwtracing/coresight/Kconfig

...

index 6433f835fc97..8fd9fd139cf3 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -65,7 +65,7 @@ config CORESIGHT_SINK_ETBV10 special enhancement or added features. config CORESIGHT_SOURCE_ETM3X

• bool "CoreSight Embedded Trace Macrocell 3.x driver"

• tristate "CoreSight Embedded Trace Macrocell 3.x driver" depends on !ARM64 select CORESIGHT_LINKS_AND_SINKS help

@@ -74,6 +74,9 @@ config CORESIGHT_SOURCE_ETM3X This is primarily useful for instruction level tracing.

Depending

...

 the ETM version data tracing may also be available.

•  To compile this driver as a module, choose M here: the
  

•  module will be called coresight-etm3x.
  

config CORESIGHT_SOURCE_ETM4X bool "CoreSight Embedded Trace Macrocell 4.x driver" depends on ARM64 diff --git a/drivers/hwtracing/coresight/Makefile

b/drivers/hwtracing/coresight/Makefile

...

index 19497d1d92bf..d619cfd0abd8 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -11,7 +11,8 @@ obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \ coresight-replicator.o -obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o

coresight-etm-cp14.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 diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c

b/drivers/hwtracing/coresight/coresight-etm3x-core.c

...

similarity index 97% rename from drivers/hwtracing/coresight/coresight-etm3x.c rename to drivers/hwtracing/coresight/coresight-etm3x-core.c index bf22dcfd3327..82b333c40006 100644 --- a/drivers/hwtracing/coresight/coresight-etm3x.c +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c @@ -895,6 +895,23 @@ static int etm_probe(struct amba_device *adev,

const struct amba_id *id)

...

return ret; } +static int __exit etm_remove(struct amba_device *adev) +{

Not sure adding __exit does anything special here since all the code is removed by the kernel anyway. The same comment goes for the other drivers in this set.

We still need to remove any reference in other part of Kernel to etm device. For example, we remove ETM module but keep coresight module. ETM-perf and coresight-core still could access ETM device structure if etm_perf_symlink() and coresigh_unregister() are not called in etm_remove. We need clean any reference to ETM device structure up.

Thanks, Tingwei

...

• struct etm_drvdata *drvdata = dev_get_drvdata(&adev->dev);
• etm_perf_symlink(drvdata->csdev, false);
• if (--etm_count == 0) {

cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);

...

• if (hp_online)
  

• 	cpuhp_remove_state_nocalls(hp_online);
  

• }
• coresight_unregister(drvdata->csdev);
• return 0;

+}

#ifdef CONFIG_PM static int etm_runtime_suspend(struct device *dev) { @@ -937,6 +954,8 @@ static const struct amba_id etm_ids[] = { { 0, 0}, }; +MODULE_DEVICE_TABLE(amba, etm_ids);

static struct amba_driver etm_driver = { .drv = { .name = "coresight-etm3x", @@ -945,6 +964,12 @@ static struct amba_driver etm_driver = { .suppress_bind_attrs = true, }, .probe = etm_probe,

• .remove = etm_remove, .id_table = etm_ids,

}; -builtin_amba_driver(etm_driver); +module_amba_driver(etm_driver);

+MODULE_AUTHOR("Pratik Patel pratikp@codeaurora.org"); +MODULE_AUTHOR("Mathieu Poirier mathieu.poirier@linaro.org"); +MODULE_DESCRIPTION("Arm CoreSight Program Flow Trace driver");

That should be "Arm CoreSight Embedded Trace Macrocell 3.x driver"

With the above:

Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org

...

+MODULE_LICENSE("GPL v2");

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

...

a Linux Foundation Collaborative Project

On Thu, Aug 13, 2020 at 05:47:22PM +0800, Suzuki K Poulose wrote:

On 08/07/2020 12:11 PM, Tingwei Zhang wrote:

...

From: Kim Phillips kim.phillips@arm.com

Allow to build coresight-etm3x 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-etm3x by the Makefile
• add an etm_remove function, for module unload
• add a MODULE_DEVICE_TABLE for autoloading on boot

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 Reviewed-by: Mike Leach mike.leach@linaro.org

---

drivers/hwtracing/coresight/Kconfig | 5 +++- drivers/hwtracing/coresight/Makefile | 3 ++- ...resight-etm3x.c => coresight-etm3x-core.c} | 27 ++++++++++++++++++- 3 files changed, 32 insertions(+), 3 deletions(-) rename drivers/hwtracing/coresight/{coresight-etm3x.c => coresight-etm3x-core.c} (97%)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig index 6433f835fc97..8fd9fd139cf3 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -65,7 +65,7 @@ config CORESIGHT_SINK_ETBV10 special enhancement or added features.

config CORESIGHT_SOURCE_ETM3X

• bool "CoreSight Embedded Trace Macrocell 3.x driver"

• tristate "CoreSight Embedded Trace Macrocell 3.x driver" depends on !ARM64 select CORESIGHT_LINKS_AND_SINKS help

@@ -74,6 +74,9 @@ config CORESIGHT_SOURCE_ETM3X This is primarily useful for instruction level tracing. Depending the ETM version data tracing may also be available.

•  To compile this driver as a module, choose M here: the
  

•  module will be called coresight-etm3x.
  

config CORESIGHT_SOURCE_ETM4X bool "CoreSight Embedded Trace Macrocell 4.x driver" depends on ARM64 diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index 19497d1d92bf..d619cfd0abd8 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -11,7 +11,8 @@ obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \ coresight-replicator.o -obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o coresight-etm-cp14.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 diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c similarity index 97% rename from drivers/hwtracing/coresight/coresight-etm3x.c rename to drivers/hwtracing/coresight/coresight-etm3x-core.c index bf22dcfd3327..82b333c40006 100644 --- a/drivers/hwtracing/coresight/coresight-etm3x.c +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c @@ -895,6 +895,23 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id) return ret; }

+static int __exit etm_remove(struct amba_device *adev) +{

• struct etm_drvdata *drvdata = dev_get_drvdata(&adev->dev);
• etm_perf_symlink(drvdata->csdev, false);
• if (--etm_count == 0) {

• cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
  

• if (hp_online)
  

• 	cpuhp_remove_state_nocalls(hp_online);
  

• }

As mentioned in the previous version, this needs fixing similar to the etm4x to avoid races with etm_count. I understand that it is an exisiting problem. But this set exposes it more due to etm_remove() calls and is much more probable to hit it.

OK. I'll fix this. Can you help to review ETM4 fix? If that's fine, I'll make similar fix here.

Unfortunately, I don't have any device with ETM3. Can anyone help to verify the function on ETM3? I'll really appreciate that.

Thanks, Tingwei

Suzuki

On Thu, Aug 13, 2020 at 11:00:04AM -0600, Mathieu Poirier wrote:

On Thu, 13 Aug 2020 at 04:57, Tingwei Zhang tingweiz@codeaurora.org wrote:

...

On Thu, Aug 13, 2020 at 05:47:22PM +0800, Suzuki K Poulose wrote:

...

On 08/07/2020 12:11 PM, Tingwei Zhang wrote:

...

From: Kim Phillips kim.phillips@arm.com

Allow to build coresight-etm3x 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-etm3x by the Makefile
• add an etm_remove function, for module unload
• add a MODULE_DEVICE_TABLE for autoloading on boot

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 Reviewed-by: Mike Leach mike.leach@linaro.org

---

drivers/hwtracing/coresight/Kconfig | 5 +++- drivers/hwtracing/coresight/Makefile | 3 ++- ...resight-etm3x.c => coresight-etm3x-core.c} | 27 ++++++++++++++++++- 3 files changed, 32 insertions(+), 3 deletions(-) rename drivers/hwtracing/coresight/{coresight-etm3x.c => coresight-etm3x-core.c} (97%)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig index 6433f835fc97..8fd9fd139cf3 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -65,7 +65,7 @@ config CORESIGHT_SINK_ETBV10 special enhancement or added features.

config CORESIGHT_SOURCE_ETM3X

• bool "CoreSight Embedded Trace Macrocell 3.x driver"

• tristate "CoreSight Embedded Trace Macrocell 3.x driver" depends on !ARM64 select CORESIGHT_LINKS_AND_SINKS help

@@ -74,6 +74,9 @@ config CORESIGHT_SOURCE_ETM3X This is primarily useful for instruction level tracing. Depending the ETM version data tracing may also be available.

•  To compile this driver as a module, choose M here: the
  

•  module will be called coresight-etm3x.
  

config CORESIGHT_SOURCE_ETM4X bool "CoreSight Embedded Trace Macrocell 4.x driver" depends on ARM64 diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index 19497d1d92bf..d619cfd0abd8 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -11,7 +11,8 @@ obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \ coresight-replicator.o -obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o coresight-etm-cp14.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 diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c similarity index 97% rename from drivers/hwtracing/coresight/coresight-etm3x.c rename to drivers/hwtracing/coresight/coresight-etm3x-core.c index bf22dcfd3327..82b333c40006 100644 --- a/drivers/hwtracing/coresight/coresight-etm3x.c +++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c @@ -895,6 +895,23 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id) return ret; }

+static int __exit etm_remove(struct amba_device *adev) +{

• struct etm_drvdata *drvdata = dev_get_drvdata(&adev->dev);
• etm_perf_symlink(drvdata->csdev, false);
• if (--etm_count == 0) {

•        cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
  

•        if (hp_online)
  

•                cpuhp_remove_state_nocalls(hp_online);
  

• }

As mentioned in the previous version, this needs fixing similar to the etm4x to avoid races with etm_count. I understand that it is an exisiting problem. But this set exposes it more due to etm_remove() calls and is much more probable to hit it.

OK. I'll fix this. Can you help to review ETM4 fix? If that's fine, I'll make similar fix here.

Unfortunately, I don't have any device with ETM3. Can anyone help to verify the function on ETM3? I'll really appreciate that.

I'll take care of ETMv3. Please wait before sending a new revision since I am still reviewing the current set. I will let you know when I am done.

Thanks a lot, Mathieu. I'll wait for your comments.

...

Thanks, Tingwei

...

Suzuki

---

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

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 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 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 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 Fri, Aug 14, 2020 at 04:35:11AM +0800, Mathieu Poirier wrote:

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

...

From: Kim Phillips kim.phillips@arm.com

Allow to build coresight-replicator as modules, for ease of development.

• Kconfig becomes a tristate, to allow =m
• combine static and dynamic replicator init into single module_init/exit call
• add replicator_remove functions, for module unload
• add a MODULE_DEVICE_TABLE for autoloading on boot

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 Co-developed-by: Mian Yousaf Kaukab ykaukab@suse.de Signed-off-by: Mian Yousaf Kaukab ykaukab@suse.de 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 +- .../coresight/coresight-replicator.c | 63 ++++++++++++++++++- 2 files changed, 65 insertions(+), 3 deletions(-)

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

b/drivers/hwtracing/coresight/Kconfig

...

index fc48ae086746..f31778dd0b5d 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -17,13 +17,16 @@ menuconfig CORESIGHT if CORESIGHT config CORESIGHT_LINKS_AND_SINKS

• bool "CoreSight Link and Sink drivers"

• tristate "CoreSight Link and Sink drivers" help This enables support for CoreSight link and sink drivers that

are

...

 responsible for transporting and collecting the trace data
 respectively.  Link and sinks are dynamically aggregated with a

trace

...

 entity at run time to form a complete trace path.

With the passage of time and the refinement of the supported components, the CONFIG_ and description are no longer valid. I'll leave it up to you to decide if you want to fix it as part of this set, the choice is entirely up to you.

I'll leave that to another patch and focus this patch set to make driver as module.

...

•  To compile these drivers as modules, choose M here: the
  

•  modules will be called coresight-funnel and
  

coresight-replicator.

...

config CORESIGHT_LINK_AND_SINK_TMC tristate "Coresight generic TMC driver" diff --git a/drivers/hwtracing/coresight/coresight-replicator.c

b/drivers/hwtracing/coresight/coresight-replicator.c

...

index 78acf29c49ca..05db434c79d0 100644 --- a/drivers/hwtracing/coresight/coresight-replicator.c +++ b/drivers/hwtracing/coresight/coresight-replicator.c @@ -291,6 +291,14 @@ static int replicator_probe(struct device *dev,

struct resource *res)

...

return ret; } +static int __exit replicator_remove(struct device *dev) +{

• struct replicator_drvdata *drvdata = dev_get_drvdata(dev);
• coresight_unregister(drvdata->csdev);
• return 0;

+}

static int static_replicator_probe(struct platform_device *pdev) { int ret; @@ -310,6 +318,13 @@ static int static_replicator_probe(struct

platform_device *pdev)

...

return ret; } +static int __exit static_replicator_remove(struct platform_device

*pdev)

...

+{

• replicator_remove(&pdev->dev);
• pm_runtime_disable(&pdev->dev);
• return 0;

+}

#ifdef CONFIG_PM static int replicator_runtime_suspend(struct device *dev) { @@ -342,25 +357,28 @@ static const struct of_device_id

static_replicator_match[] = {

...

{.compatible = "arm,coresight-static-replicator"}, {} }; +MODULE_DEVICE_TABLE(of, static_replicator_match);

Pick a way to do this, a newline or not, but stick with it for _all_ the drivers in this set.

I will add a new line and check all the coresight drivers.

...

#ifdef CONFIG_ACPI static const struct acpi_device_id static_replicator_acpi_ids[] = { {"ARMHC985", 0}, /* ARM CoreSight Static Replicator */ {} }; +MODULE_DEVICE_TABLE(acpi, static_replicator_acpi_ids);

Same as above

I will add a new line.

...

#endif static struct platform_driver static_replicator_driver = { .probe = static_replicator_probe,

• .remove = static_replicator_remove, .driver = { .name = "coresight-static-replicator",

• .owner	= THIS_MODULE,
  

  .of_match_table = of_match_ptr(static_replicator_match), .acpi_match_table = ACPI_PTR(static_replicator_acpi_ids), .pm = &replicator_dev_pm_ops, .suppress_bind_attrs = true, },

}; -builtin_platform_driver(static_replicator_driver); static int dynamic_replicator_probe(struct amba_device *adev, const struct amba_id *id) @@ -368,19 +386,60 @@ static int dynamic_replicator_probe(struct

amba_device *adev,

...

return replicator_probe(&adev->dev, &adev->res); } +static int __exit dynamic_replicator_remove(struct amba_device *adev) +{

• return replicator_remove(&adev->dev);

+}

static const struct amba_id dynamic_replicator_ids[] = { CS_AMBA_ID(0x000bb909), CS_AMBA_ID(0x000bb9ec), /* Coresight SoC-600 */ {}, }; +MODULE_DEVICE_TABLE(amba, dynamic_replicator_ids);

static struct amba_driver dynamic_replicator_driver = { .drv = { .name = "coresight-dynamic-replicator", .pm = &replicator_dev_pm_ops,

• .owner	= THIS_MODULE,
  

  .suppress_bind_attrs = true, }, .probe = dynamic_replicator_probe,
• .remove = dynamic_replicator_remove, .id_table = dynamic_replicator_ids,

}; -builtin_amba_driver(dynamic_replicator_driver);

+static int __init replicator_init(void) +{

• int ret;
• ret = platform_driver_register(&static_replicator_driver);
• if (ret) {

• pr_info("Error registering platform driver\n");
  

• return ret;
  

• }
• ret = amba_driver_register(&dynamic_replicator_driver);
• if (ret) {

• pr_info("Error registering amba driver\n");
  

• platform_driver_unregister(&static_replicator_driver);
  

• }
• return ret;

+}

+static void __exit replicator_exit(void) +{

• platform_driver_unregister(&static_replicator_driver);
• amba_driver_unregister(&dynamic_replicator_driver);

+}

Same here, choose to add a newline or not but stay consistent for all the drives.

Sure.

...

+module_init(replicator_init); +module_exit(replicator_exit);

+MODULE_AUTHOR("Pratik Patel pratikp@codeaurora.org");

Pratik should also be mentioned in the funnel driver.

I'll add Pratik in v9.

...

+MODULE_AUTHOR("Mathieu Poirier mathieu.poirier@linaro.org"); +MODULE_DESCRIPTION("Arm CoreSight Replicator 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:45PM +0800, Tingwei Zhang wrote:

Add static cti_assoc_ops to coresight core driver. Let cti driver register the add_assoc and remove_assoc call back. Avoid coresight core driver to depend on cti driver.

Signed-off-by: Tingwei Zhang tingwei@codeaurora.org Tested-by: Mike Leach mike.leach@linaro.org

---

drivers/hwtracing/coresight/coresight-cti.c | 32 ++++++++++++++++++-- drivers/hwtracing/coresight/coresight-priv.h | 14 ++++----- drivers/hwtracing/coresight/coresight.c | 21 +++++++++++-- 3 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c index 3ccc703dc940..1f470c47ba70 100644 --- a/drivers/hwtracing/coresight/coresight-cti.c +++ b/drivers/hwtracing/coresight/coresight-cti.c @@ -589,7 +589,6 @@ void cti_add_assoc_to_csdev(struct coresight_device *csdev) cti_add_done: mutex_unlock(&ect_mutex); } -EXPORT_SYMBOL_GPL(cti_add_assoc_to_csdev); /*

• Removing the associated devices is easier.

@@ -616,7 +615,15 @@ void cti_remove_assoc_from_csdev(struct coresight_device *csdev) } mutex_unlock(&ect_mutex); } -EXPORT_SYMBOL_GPL(cti_remove_assoc_from_csdev);

Mike and Tingwei,

It seems to me that functions cti_add_assoc_to_csdev() and cti_remote_assoc_from_csdev() don't have to be exported. They could be called in cti_probe() and cti_device_release() with the ect_mutex held, the same way cti_update_conn_xrefs() and cti_remove_conn_xrefs() are.

If my assessment is correct this patch would be simpler.

Thanks, Mathieu

+/*

• • Operations to add and remove associated CTI.
• • Register to coresight core driver as call back function.
• */

+static struct cti_assoc_op cti_assoc_ops = {

• .add = cti_add_assoc_to_csdev,
• .remove = cti_remove_assoc_from_csdev

+}; /*

• Update the cross references where the associated device was found

@@ -972,4 +979,23 @@ static struct amba_driver cti_driver = { .probe = cti_probe, .id_table = cti_ids, }; -builtin_amba_driver(cti_driver);

+static int __init cti_init(void) +{

• int ret;
• ret = amba_driver_register(&cti_driver);
• if (ret)

• pr_info("Error registering cti driver\n");
  

• coresight_set_cti_ops(&cti_assoc_ops);
• return ret;

+}

+static void __exit cti_exit(void) +{

• coresight_remove_cti_ops();
• amba_driver_unregister(&cti_driver);

+}

+module_init(cti_init); +module_exit(cti_exit); diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index dcb8aeb6af62..6cde6cf42554 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -173,15 +173,13 @@ static inline int etm_readl_cp14(u32 off, unsigned int *val) { return 0; } static inline int etm_writel_cp14(u32 off, u32 val) { return 0; } #endif -#ifdef CONFIG_CORESIGHT_CTI -extern void cti_add_assoc_to_csdev(struct coresight_device *csdev); -extern void cti_remove_assoc_from_csdev(struct coresight_device *csdev); +struct cti_assoc_op {

• void (*add)(struct coresight_device *csdev);
• void (*remove)(struct coresight_device *csdev);

+}; -#else -static inline void cti_add_assoc_to_csdev(struct coresight_device *csdev) {} -static inline void -cti_remove_assoc_from_csdev(struct coresight_device *csdev) {} -#endif +extern void coresight_set_cti_ops(const struct cti_assoc_op *cti_op); +extern void coresight_remove_cti_ops(void); /*

• Macros and inline functions to handle CoreSight UCI data and driver

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 0b0e31577b9b..31b9ec8d3b9c 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -57,6 +57,20 @@ const u32 coresight_barrier_pkt[4] = { 0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fffffff}; EXPORT_SYMBOL_GPL(coresight_barrier_pkt); +static const struct cti_assoc_op *cti_assoc_ops;

+void coresight_set_cti_ops(const struct cti_assoc_op *cti_op) +{

• cti_assoc_ops = cti_op;

+} +EXPORT_SYMBOL_GPL(coresight_set_cti_ops);

+void coresight_remove_cti_ops(void) +{

• cti_assoc_ops = NULL;

+} +EXPORT_SYMBOL_GPL(coresight_remove_cti_ops);

static int coresight_id_match(struct device *dev, void *data) { int trace_id, i_trace_id; @@ -1242,7 +1256,8 @@ static void coresight_device_release(struct device *dev) { struct coresight_device *csdev = to_coresight_device(dev);

• cti_remove_assoc_from_csdev(csdev);

• if (cti_assoc_ops && cti_assoc_ops->remove)

• cti_assoc_ops->remove(csdev);
  

  fwnode_handle_put(csdev->dev.fwnode); kfree(csdev->refcnt); kfree(csdev);

@@ -1553,8 +1568,8 @@ struct coresight_device *coresight_register(struct coresight_desc *desc) ret = coresight_fixup_device_conns(csdev); if (!ret) ret = coresight_fixup_orphan_conns(csdev);

• if (!ret)

• cti_add_assoc_to_csdev(csdev);
  

• if (!ret && cti_assoc_ops && cti_assoc_ops->add)

• cti_assoc_ops->add(csdev);
  

mutex_unlock(&coresight_mutex); if (ret) { -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Good day,

On Mon, Aug 17, 2020 at 07:49:16PM +0100, Mike Leach wrote:

Hi Mathieu,

On Mon, 17 Aug 2020 at 17:02, Mathieu Poirier mathieu.poirier@linaro.org wrote:

...

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

...

Add static cti_assoc_ops to coresight core driver. Let cti driver register the add_assoc and remove_assoc call back. Avoid coresight core driver to depend on cti driver.

Signed-off-by: Tingwei Zhang tingwei@codeaurora.org Tested-by: Mike Leach mike.leach@linaro.org

---

drivers/hwtracing/coresight/coresight-cti.c | 32 ++++++++++++++++++-- drivers/hwtracing/coresight/coresight-priv.h | 14 ++++----- drivers/hwtracing/coresight/coresight.c | 21 +++++++++++-- 3 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c index 3ccc703dc940..1f470c47ba70 100644 --- a/drivers/hwtracing/coresight/coresight-cti.c +++ b/drivers/hwtracing/coresight/coresight-cti.c @@ -589,7 +589,6 @@ void cti_add_assoc_to_csdev(struct coresight_device *csdev) cti_add_done: mutex_unlock(&ect_mutex); } -EXPORT_SYMBOL_GPL(cti_add_assoc_to_csdev);

/*

• Removing the associated devices is easier.

@@ -616,7 +615,15 @@ void cti_remove_assoc_from_csdev(struct coresight_device *csdev) } mutex_unlock(&ect_mutex); } -EXPORT_SYMBOL_GPL(cti_remove_assoc_from_csdev);

Mike and Tingwei,

It seems to me that functions cti_add_assoc_to_csdev() and cti_remote_assoc_from_csdev() don't have to be exported. They could be called in cti_probe() and cti_device_release() with the ect_mutex held, the same way cti_update_conn_xrefs() and cti_remove_conn_xrefs() are.

The different functions handle two different initialisation & two different teardown ordering scenarios a) csdev - e.g. ETM is discovered & initialised before the associated CTI. Therefore the CTI has to establish interconnections on creation - calls cti_update_conn_xrefs() . b) CTI is discovered before csdev - therefore the csdev calls cti_add_assoc_to_csdev(). c) csdev is torn down before CTI - therefore csdev calls cti_remove_assoc_from_csdev() d) CTI is torn down before csdev - therefore calls cti_remove_conn_xrefs()

We can't call cti_add_assoc_to_csdev() in cti probe as the csdev does not exist yet. Bear in mind that one CTI can actually be associated with multiple different csdev devices - especially in the none CPU bound infrastructure such as funnels and sinks.

Yes, I see your point now. The problem needs to be viewed from other devices' (than CTI) perspective. And that part is done in coresight_register/unregister().

Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org

This has always been the case since the first CTI set - but is even more important now the modules can be removed / added as required.

Regards

Mike

...

If my assessment is correct this patch would be simpler.

Thanks, Mathieu

...

+/*

• • Operations to add and remove associated CTI.
• • Register to coresight core driver as call back function.
• */

+static struct cti_assoc_op cti_assoc_ops = {

• .add = cti_add_assoc_to_csdev,
  

• .remove = cti_remove_assoc_from_csdev
  

+};

/*

• Update the cross references where the associated device was found

@@ -972,4 +979,23 @@ static struct amba_driver cti_driver = { .probe = cti_probe, .id_table = cti_ids, }; -builtin_amba_driver(cti_driver);

+static int __init cti_init(void) +{

• int ret;
  

• ret = amba_driver_register(&cti_driver);
  

• if (ret)
  

•         pr_info("Error registering cti driver\n");
  

• coresight_set_cti_ops(&cti_assoc_ops);
  

• return ret;
  

+}

+static void __exit cti_exit(void) +{

• coresight_remove_cti_ops();
  

• amba_driver_unregister(&cti_driver);
  

+}

+module_init(cti_init); +module_exit(cti_exit); diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h index dcb8aeb6af62..6cde6cf42554 100644 --- a/drivers/hwtracing/coresight/coresight-priv.h +++ b/drivers/hwtracing/coresight/coresight-priv.h @@ -173,15 +173,13 @@ static inline int etm_readl_cp14(u32 off, unsigned int *val) { return 0; } static inline int etm_writel_cp14(u32 off, u32 val) { return 0; } #endif

-#ifdef CONFIG_CORESIGHT_CTI -extern void cti_add_assoc_to_csdev(struct coresight_device *csdev); -extern void cti_remove_assoc_from_csdev(struct coresight_device *csdev); +struct cti_assoc_op {

• void (*add)(struct coresight_device *csdev);
  

• void (*remove)(struct coresight_device *csdev);
  

+};

-#else -static inline void cti_add_assoc_to_csdev(struct coresight_device *csdev) {} -static inline void -cti_remove_assoc_from_csdev(struct coresight_device *csdev) {} -#endif +extern void coresight_set_cti_ops(const struct cti_assoc_op *cti_op); +extern void coresight_remove_cti_ops(void);

/*

• Macros and inline functions to handle CoreSight UCI data and driver

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 0b0e31577b9b..31b9ec8d3b9c 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -57,6 +57,20 @@ const u32 coresight_barrier_pkt[4] = { 0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fffffff}; EXPORT_SYMBOL_GPL(coresight_barrier_pkt);

+static const struct cti_assoc_op *cti_assoc_ops;

+void coresight_set_cti_ops(const struct cti_assoc_op *cti_op) +{

• cti_assoc_ops = cti_op;
  

+} +EXPORT_SYMBOL_GPL(coresight_set_cti_ops);

+void coresight_remove_cti_ops(void) +{

• cti_assoc_ops = NULL;
  

+} +EXPORT_SYMBOL_GPL(coresight_remove_cti_ops);

static int coresight_id_match(struct device *dev, void *data) { int trace_id, i_trace_id; @@ -1242,7 +1256,8 @@ static void coresight_device_release(struct device *dev) { struct coresight_device *csdev = to_coresight_device(dev);

• cti_remove_assoc_from_csdev(csdev);
  

• if (cti_assoc_ops && cti_assoc_ops->remove)
  

•         cti_assoc_ops->remove(csdev);
  fwnode_handle_put(csdev->dev.fwnode);
  kfree(csdev->refcnt);
  kfree(csdev);
  

@@ -1553,8 +1568,8 @@ struct coresight_device *coresight_register(struct coresight_desc *desc) ret = coresight_fixup_device_conns(csdev); if (!ret) ret = coresight_fixup_orphan_conns(csdev);

• if (!ret)
  

•         cti_add_assoc_to_csdev(csdev);
  

• if (!ret && cti_assoc_ops && cti_assoc_ops->add)
  

•         cti_assoc_ops->add(csdev);
  
  mutex_unlock(&coresight_mutex);
  if (ret) {
  

-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

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

From: Mike Leach mike.leach@linaro.org

CTI code to remove sysfs link to other devices on shutdown, incorrectly tries to remove a single ended link when these are all double ended. This implementation leaves elements in the link info structure undefined which results in a crash in recent tests for driver module unload.

This patch corrects the link removal code.

Fixes: 73274abb6557 (coresight: cti: Add in sysfs links to other coresight devices)

This should be:

Fixes: 73274abb6557 ("coresight: cti: Add in sysfs links to other coresight devices")

Reported-by: Tingwei Zhang tingwei@codeaurora.org Signed-off-by: Mike Leach mike.leach@linaro.org

---

drivers/hwtracing/coresight/coresight-cti.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c index 1f470c47ba70..1c34db7d1b1b 100644 --- a/drivers/hwtracing/coresight/coresight-cti.c +++ b/drivers/hwtracing/coresight/coresight-cti.c @@ -511,12 +511,15 @@ static bool cti_add_sysfs_link(struct cti_drvdata *drvdata, return !link_err; } -static void cti_remove_sysfs_link(struct cti_trig_con *tc) +static void cti_remove_sysfs_link(struct cti_drvdata *drvdata,

• 		  struct cti_trig_con *tc)
  

{ struct coresight_sysfs_link link_info;

• link_info.orig = drvdata->csdev; link_info.orig_name = tc->con_dev_name; link_info.target = tc->con_dev;
• link_info.target_name = dev_name(&drvdata->csdev->dev); coresight_remove_sysfs_link(&link_info);

} @@ -606,7 +609,7 @@ void cti_remove_assoc_from_csdev(struct coresight_device *csdev) ctidev = &ctidrv->ctidev; list_for_each_entry(tc, &ctidev->trig_cons, node) { if (tc->con_dev == csdev->ect_dev) {

• 		cti_remove_sysfs_link(tc);
  

• 		cti_remove_sysfs_link(ctidrv, tc);
  	tc->con_dev = NULL;
  	break;
  }
  

@@ -658,7 +661,7 @@ static void cti_remove_conn_xrefs(struct cti_drvdata *drvdata) if (tc->con_dev) { coresight_set_assoc_ectdev_mutex(tc->con_dev, NULL);

• 	cti_remove_sysfs_link(tc);
  

• 	cti_remove_sysfs_link(drvdata, tc);
  

With the above:

Reviewed-by: Mathieu Poirier mathieu.poirier@linaro.org

	tc->con_dev = NULL;
}

}

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Hi Mathieu,

On Mon, 17 Aug 2020 at 17:38, Mathieu Poirier mathieu.poirier@linaro.org wrote:

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

...

If associated ect device is not enabled at first place, disable routine should not be called. Add ect_enabled flag to check whether ect device is enabled. Fix the issue in below case. Ect device is not available when associated coresight device enabled and the association is established after coresight device is enabled.

Signed-off-by: Mike Leach mike.leach@linaro.org Signed-off-by: Tingwei Zhang tingwei@codeaurora.org

---

drivers/hwtracing/coresight/coresight.c | 11 ++++++++--- include/linux/coresight.h | 1 + 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index d066411aa794..27ad8317e3cf 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -245,13 +245,18 @@ coresight_control_assoc_ectdev(struct coresight_device *csdev, bool enable)

  if (!ect_csdev)
          return 0;

• if ((!ect_ops(ect_csdev)->enable) || (!ect_ops(ect_csdev)->disable))
  

•         return 0;
  
  if (enable) {
  

•         if (ect_ops(ect_csdev)->enable)
  

•                 ect_ret = ect_ops(ect_csdev)->enable(ect_csdev);
  

•         ect_ret = ect_ops(ect_csdev)->enable(ect_csdev);
  

•         if (!ect_ret)
  

•                 csdev->ect_enabled = true;
  } else {
  

•         if (ect_ops(ect_csdev)->disable)
  

•         if (csdev->ect_enabled) {
                  ect_ret = ect_ops(ect_csdev)->disable(ect_csdev);
  

•                 csdev->ect_enabled = false;
  

•         }
  }
  
  /* output warning if ECT enable is preventing trace operation */
  

diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 3bb738f9a326..7d3c87e5b97c 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -208,6 +208,7 @@ struct coresight_device { /* sysfs links between components */ int nr_links; bool has_conns_grp;

• bool ect_enabled; /* true only if associated ect device is enabled */
  

We have cti_config::enable_req_count and cti_config::hw_enabled, both used in cti_enable_hw() and cti_disable_hw(). I would have thought they'd be sufficient to address the counting problems. If they are not I would much rather see a solution confined to the cti driver than in the core itself.

This is related to the fact that under sysfs it is possible under sysfs to enable an etm e.g. etm1 without the cti module present, then insert the CTI module, then enable another ETM e.g etm2. This is an issue that is caused by the possibility of module load and unload, and though inadvisable from a system usage point of view - Tingwei correctly points out that it could happen.

At the point that the first ETM is enabled, the associated ect pointer would be NULL, and thus no attempt to enable the ect/CTI is made. The CTI module on load will set the ect pointers on all registered csdev devices, including ones that are already enabled - etm1. So when we come to disable etm1, it will try to disable a CTI that it did not enable - a fact that cannot be counted in the CTI driver as it was not there when the etm was enabled. So we have a flag in csdev to record if this csdev did in fact enable the associated device, so it is clear to disable it on shutdown.

Regards

Mike

Thanks, Mathieu

...

};

/*

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

On Wed, Aug 19, 2020 at 01:39:09AM +0800, Mathieu Poirier wrote:

On Mon, Aug 17, 2020 at 08:04:06PM +0100, Mike Leach wrote:

...

Hi Mathieu,

On Mon, 17 Aug 2020 at 17:38, Mathieu Poirier mathieu.poirier@linaro.org wrote:

...

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

...

If associated ect device is not enabled at first place, disable routine should not be called. Add ect_enabled flag to check whether ect device is enabled. Fix the issue in below case. Ect device is not available when associated coresight device enabled and the association is established after coresight device is enabled.

Signed-off-by: Mike Leach mike.leach@linaro.org Signed-off-by: Tingwei Zhang tingwei@codeaurora.org

---

drivers/hwtracing/coresight/coresight.c | 11 ++++++++--- include/linux/coresight.h | 1 + 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight.c

b/drivers/hwtracing/coresight/coresight.c

...

...

...

index d066411aa794..27ad8317e3cf 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -245,13 +245,18 @@ coresight_control_assoc_ectdev(struct

coresight_device *csdev, bool enable)

...

...

...

  if (!ect_csdev)
          return 0;

• if ((!ect_ops(ect_csdev)->enable) ||
  

(!ect_ops(ect_csdev)->disable))

...

...

...

•         return 0;
  
  if (enable) {
  

•         if (ect_ops(ect_csdev)->enable)
  

•                 ect_ret =
  

ect_ops(ect_csdev)->enable(ect_csdev);

...

...

...

•         ect_ret = ect_ops(ect_csdev)->enable(ect_csdev);
  

•         if (!ect_ret)
  

•                 csdev->ect_enabled = true;
  } else {
  

•         if (ect_ops(ect_csdev)->disable)
  

•         if (csdev->ect_enabled) {
                  ect_ret =
  

ect_ops(ect_csdev)->disable(ect_csdev);

...

...

...

•                 csdev->ect_enabled = false;
  

•         }
  }
  
  /* output warning if ECT enable is preventing trace operation
  

*/

...

...

...

diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 3bb738f9a326..7d3c87e5b97c 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -208,6 +208,7 @@ struct coresight_device { /* sysfs links between components */ int nr_links; bool has_conns_grp;

• bool ect_enabled; /* true only if associated ect device is
  

enabled */

...

...

We have cti_config::enable_req_count and cti_config::hw_enabled, both

used in

...

...

cti_enable_hw() and cti_disable_hw(). I would have thought they'd be

sufficient

...

...

to address the counting problems. If they are not I would much rather

see a

...

...

solution confined to the cti driver than in the core itself.

This is related to the fact that under sysfs it is possible under sysfs to enable an etm e.g. etm1 without the cti module present, then insert the CTI module, then enable another ETM e.g etm2. This is an issue that is caused by the possibility of module load and unload, and though inadvisable from a system usage point of view - Tingwei correctly points out that it could happen.

At the point that the first ETM is enabled, the associated ect pointer would be NULL, and thus no attempt to enable the ect/CTI is made. The CTI module on load will set the ect pointers on all registered csdev devices, including ones that are already enabled - etm1. So when we come to disable etm1, it will try to disable a CTI that it did not enable - a fact that cannot be counted in the CTI driver as it was not there when the etm was enabled. So we have a flag in csdev to record if this csdev did in fact enable the associated device, so it is clear to disable it on shutdown.

In the above example the csdev structures associated with ETM1 and ETM2 will have different csdev->ect_dev pointers, one for each CTI it is assocated with. And I would think the reference count for CTI1 would not have been incremented since it was never enabled.

What am I missing?

The issue here is we could have two or more coresght devices associated to single CTI device, especially for sys cti case. For example, we have coresight devcie A and B. They are assocaited to CTI device C. CTI module is not loaded at first. Device A is enabled. CTI C is not enabled. Then CTI module is loaded. Device B is enabled. CTI C is enabled with enable_req_count = 1. Device A is disabled. It decrease enable_req_count to 0, so CTI C is disabled. It's kinda weird to me that CTI C is enabled with device B and disabled by device A.

The point is we need some flag in coresight device structure to save the status of associated CTI enablement when CTI module is not loaded yet.

Thanks, Tingwei

...

Regards

Mike

...

Thanks, Mathieu

...

};

/*

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

...

...

...

a Linux Foundation Collaborative Project

-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK

---

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

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

Allow to build coresight-cti 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-cti by the Makefile
• add an cti_remove function, for module unload
• move cti_remove_conn_xrefs to cti_remove
• add a MODULE_DEVICE_TABLE for autoloading on boot

Signed-off-by: Tingwei Zhang tingwei@codeaurora.org Tested-by: Mike Leach mike.leach@linaro.org

Mike,

Please add a "Reviewed-by" tag once you have reviewed this patch.

---

drivers/hwtracing/coresight/Kconfig | 5 ++++- drivers/hwtracing/coresight/Makefile | 4 ++-- .../{coresight-cti.c => coresight-cti-core.c} | 19 ++++++++++++++++++- .../hwtracing/coresight/coresight-platform.c | 1 + drivers/hwtracing/coresight/coresight.c | 1 + 5 files changed, 26 insertions(+), 4 deletions(-) rename drivers/hwtracing/coresight/{coresight-cti.c => coresight-cti-core.c} (98%)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig index f31778dd0b5d..b04aae2ceecc 100644 --- a/drivers/hwtracing/coresight/Kconfig +++ b/drivers/hwtracing/coresight/Kconfig @@ -136,7 +136,7 @@ config CORESIGHT_CPU_DEBUG module will be called coresight-cpu-debug. config CORESIGHT_CTI

• bool "CoreSight Cross Trigger Interface (CTI) driver"

• tristate "CoreSight Cross Trigger Interface (CTI) driver" depends on ARM || ARM64 help This driver provides support for CoreSight CTI and CTM components.

@@ -147,6 +147,9 @@ config CORESIGHT_CTI halt compared to disabling sources and sinks normally in driver software.

•  To compile this driver as a module, choose M here: the
  

•  module will be called coresight-cti.
  

config CORESIGHT_CTI_INTEGRATION_REGS bool "Access CTI CoreSight Integration Registers" depends on CORESIGHT_CTI diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index f2a568b969c4..0359d5a1588f 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -19,6 +19,6 @@ 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 -obj-$(CONFIG_CORESIGHT_CTI) += coresight-cti.o \

• 		coresight-cti-platform.o \
  

+obj-$(CONFIG_CORESIGHT_CTI) += coresight-cti.o +coresight-cti-y := coresight-cti-core.o coresight-cti-platform.o \ coresight-cti-sysfs.o

Please align "coresight-cti-sysfs.o" with "coresight-cti-core.o". The same should be done for the etm3x driver.

diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti-core.c similarity index 98% rename from drivers/hwtracing/coresight/coresight-cti.c rename to drivers/hwtracing/coresight/coresight-cti-core.c index 6bb06c5715db..ca31da6cf1c0 100644 --- a/drivers/hwtracing/coresight/coresight-cti.c +++ b/drivers/hwtracing/coresight/coresight-cti-core.c @@ -838,7 +838,6 @@ static void cti_device_release(struct device *dev) struct cti_drvdata *ect_item, *ect_tmp; mutex_lock(&ect_mutex);

• cti_remove_conn_xrefs(drvdata); cti_pm_release(drvdata);

/* remove from the list */ @@ -853,6 +852,18 @@ static void cti_device_release(struct device *dev) if (drvdata->csdev_release) drvdata->csdev_release(dev); } +static int __exit cti_remove(struct amba_device *adev) +{

• struct cti_drvdata *drvdata = dev_get_drvdata(&adev->dev);
• mutex_lock(&ect_mutex);
• cti_remove_conn_xrefs(drvdata);
• mutex_unlock(&ect_mutex);

The changelog point out this change but now _why_ it is done - please revise.

• coresight_unregister(drvdata->csdev);
• return 0;

+} static int cti_probe(struct amba_device *adev, const struct amba_id *id) { @@ -972,6 +983,7 @@ static const struct amba_id cti_ids[] = { CS_AMBA_UCI_ID(0x000bb9ed, uci_id_cti), /* Coresight CTI (SoC 600) */ { 0, 0}, }; +MODULE_DEVICE_TABLE(amba, cti_ids); static struct amba_driver cti_driver = { .drv = { @@ -980,6 +992,7 @@ static struct amba_driver cti_driver = { .suppress_bind_attrs = true, }, .probe = cti_probe,

• .remove = cti_remove, .id_table = cti_ids,

}; @@ -1002,3 +1015,7 @@ static void __exit cti_exit(void) module_init(cti_init); module_exit(cti_exit);

+MODULE_AUTHOR("Mike Leach mike.leach@linaro.org"); +MODULE_DESCRIPTION("Arm CoreSight CTI Driver"); +MODULE_LICENSE("GPL v2"); diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index bfd44231d7ad..635d55c1d580 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -75,6 +75,7 @@ coresight_find_csdev_by_fwnode(struct fwnode_handle *r_fwnode) } return csdev; } +EXPORT_SYMBOL_GPL(coresight_find_csdev_by_fwnode); #ifdef CONFIG_OF static inline bool of_coresight_legacy_ep_is_input(struct device_node *ep) diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c index 8dcd73ea33b4..f9566d667261 100644 --- a/drivers/hwtracing/coresight/coresight.c +++ b/drivers/hwtracing/coresight/coresight.c @@ -289,6 +289,7 @@ void coresight_set_assoc_ectdev_mutex(struct coresight_device *csdev, csdev->ect_dev = ect_csdev; mutex_unlock(&coresight_mutex); } +EXPORT_SYMBOL_GPL(coresight_set_assoc_ectdev_mutex); static int coresight_enable_sink(struct coresight_device *csdev, u32 mode, void *data) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project