coresight: allow to build coresight as modules

Provide name of cpu_debug module in Kconfig help section.

Signed-off-by Tingwei Zhang <[email protected]>

Tested-by Mike Leach <[email protected]>

Reviewed-by Suzuki K Poulose <[email protected]>

---
 drivers/hwtracing/coresight/Kconfig | 3 +++
 1 file changed, 3 insertions(+)

drivers/hwtracing/coresight/Kconfig +0 -0

index 02dbb5ca3bcf..4663fd1bbffc 100644

drivers/hwtracing/coresight/Kconfig +0 -0

 config CORESIGHT_CTI
 	bool "CoreSight Cross Trigger Interface (CTI) driver"
 	depends on ARM || ARM64

Signature · 2 lines

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

From: Kim Phillips <[email protected]>

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 <[email protected]>

Cc Leo Yan <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Kim Phillips <[email protected]>

Signed-off-by Tingwei Zhang <[email protected]>

Reviewed-by Suzuki K Poulose <[email protected]>

---
 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(-)

drivers/hwtracing/coresight/coresight-etm-perf.h +1 -1

index 015213abe00a..05f89723e282 100644
@@ -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);

drivers/hwtracing/coresight/coresight-priv.h +1 -1

index f2dc625ea585..d801a2755432 100644
@@ -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

include/linux/coresight.h +1 -1

index 58fffdecdbfd..3bb738f9a326 100644
@@ -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);

Signature · 2 lines

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

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

Signed-off-by Mian Yousaf Kaukab <[email protected]>

Signed-off-by Tingwei Zhang <[email protected]>

Reviewed-by Suzuki K Poulose <[email protected]>

---
 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(-)

drivers/hwtracing/coresight/coresight-etb10.c +1 -1

index 03e3f2590191..04ee9cda988d 100644
@@ -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;

drivers/hwtracing/coresight/coresight-priv.h +2 -2

index d801a2755432..dcb8aeb6af62 100644
@@ -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,

drivers/hwtracing/coresight/coresight-priv.h +0 -1

 }
 
-
 static inline void CS_LOCK(void __iomem *addr)
 {
 	do {

drivers/hwtracing/coresight/coresight-tmc-etf.c +1 -1

index 6375504ba8b0..44402d413ebb 100644
@@ -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) {

drivers/hwtracing/coresight/coresight.c +0 -0

index e9c90f2de34a..310b1b825dd1 100644

drivers/hwtracing/coresight/coresight.c +0 -1

 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

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 <[email protected]>

Tested-by Mike Leach <[email protected]>

Suggested-by Suzuki K Poulose <[email protected]>

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

drivers/hwtracing/coresight/coresight.c +49 -6

index b7151c5f81b1..0b0e31577b9b 100644
@@ -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;
 

drivers/hwtracing/coresight/coresight.c +4 -0

+	if (ret) {
+		kfree(node);
+		return ret;
+	}
 	node->csdev = csdev;
 	list_add(&node->link, path);
 

Signature · 2 lines

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

Quoted 6 lines · Tingwei Zhang

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.

Review

Quoted 8 lines

Signed-off-by Tingwei Zhang <[email protected]>

Tested-by Mike Leach <[email protected]>

Suggested-by Suzuki K Poulose <[email protected]>

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

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/coresight.c

drivers/hwtracing/coresight/coresight.c struct coresight_device

Quoted 4 lines

index b7151c5f81b1..0b0e31577b9b 100644

drivers/hwtracing/coresight/coresight.c +0 -0

@@ -634,13 +634,45 @@ struct coresight_device

*coresight_get_sink_by_id(u32 id)

drivers/hwtracing/coresight/coresight.c struct coresight_device *coresight_get_sink_by_id(u32 id)

Quoted 5 lines

drivers/hwtracing/coresight/coresight.c +2 -0

 	return dev ? to_coresight_device(dev) : NULL;
 }
 
+/**
+ * coresight_get_ref- Helper function to increase reference count to

module

Quoted 9 lines

drivers/hwtracing/coresight/coresight.c +9 -0

+ * 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 */

drivers/hwtracing/coresight/coresight.c struct coresight_device *coresight_get_sink_by_id(u32 id)

Quoted 10 lines

drivers/hwtracing/coresight/coresight.c +10 -0

+	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

Quoted 4 lines

drivers/hwtracing/coresight/coresight.c +4 -0

+ * and device. Power off the device.
+ */
+static inline void coresight_put_ref(struct coresight_device *csdev)
+{

drivers/hwtracing/coresight/coresight.c struct coresight_device *coresight_get_sink_by_id(u32 id)

Quoted 10 lines

drivers/hwtracing/coresight/coresight.c +7 -0

+	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

drivers/hwtracing/coresight/coresight.c struct coresight_device · line 672

Quoted 1 line

drivers/hwtracing/coresight/coresight.c +0 -0

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

the

drivers/hwtracing/coresight/coresight.c static void coresight_grab_device(struct

Quoted 8 lines

drivers/hwtracing/coresight/coresight.c +1 -1

  * 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)

drivers/hwtracing/coresight/coresight.c static void coresight_drop_device(struct

Quoted 23 lines

drivers/hwtracing/coresight/coresight.c +13 -2

 		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)

drivers/hwtracing/coresight/coresight.c static int _coresight_build_path(struct

Quoted 16 lines

drivers/hwtracing/coresight/coresight.c +2 -2

 {
 	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,

drivers/hwtracing/coresight/coresight.c static int _coresight_build_path(struct

Quoted 9 lines

drivers/hwtracing/coresight/coresight.c +1 -1

 				 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,

Quoted 3 lines

drivers/hwtracing/coresight/coresight.c +0 -0

 	if (!node)
 		return -ENOMEM;
 

drivers/hwtracing/coresight/coresight.c static int _coresight_build_path(struct coresight_device *csdev, · line 767

Quoted 2 lines

drivers/hwtracing/coresight/coresight.c +1 -1

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

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

Thanks,
Mathieu

Quoted 7 lines

drivers/hwtracing/coresight/coresight.c +4 -0

+	if (ret) {
+		kfree(node);
+		return ret;
+	}
 	node->csdev = csdev;
 	list_add(&node->link, path);
 

Quoted 2 lines

Signature · 1 line

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

Forum,

Quoted 2 lines

a Linux Foundation Collaborative Project

When coresight_build_path() fails on all the cpus, etm_setup_aux calls etm_free_aux() to free allocated event_data. WARN_ON(cpumask_empty(mask) will be triggered since cpu mask is empty. Check event_data->snk_config is not NULL first to avoid this warning.

Fixes f5200aa9831f38 ("coresight: perf: Refactor function free_event_data()")

Signed-off-by Tingwei Zhang <[email protected]>

Reviewed-by Mike Leach <[email protected]>

Reviewed-by Suzuki K Poulose <[email protected]>

---
 drivers/hwtracing/coresight/coresight-etm-perf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/hwtracing/coresight/coresight-etm-perf.c +2 -2

index dcb0592418ae..3728c44e5763 100644
@@ -126,10 +126,10 @@ static void free_sink_buffer(struct etm_event_data *event_data)
 	cpumask_t *mask = &event_data->mask;
 	struct coresight_device *sink;
 
-	if (WARN_ON(cpumask_empty(mask)))
+	if (!event_data->snk_config)
 		return;
 
-	if (!event_data->snk_config)
+	if (WARN_ON(cpumask_empty(mask)))
 		return;
 
 	cpu = cpumask_first(mask);

Signature · 2 lines

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

From: Kim Phillips <[email protected]>

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 <[email protected]>

Cc Leo Yan <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Kim Phillips <[email protected]>

Signed-off-by Tingwei Zhang <[email protected]>

Reviewed-by Mike Leach <[email protected]>

---
 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%)

drivers/hwtracing/coresight/Kconfig +4 -1

index 6433f835fc97..8fd9fd139cf3 100644
@@ -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

drivers/hwtracing/coresight/Makefile +2 -1

index 19497d1d92bf..d619cfd0abd8 100644
@@ -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

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +0 -0

similarity index 97%
rename from drivers/hwtracing/coresight/coresight-etm3x.c
rename to drivers/hwtracing/coresight/coresight-etm3x-core.c
index bf22dcfd3327..82b333c40006 100644

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +17 -0

+	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,
 	},

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +1 -0

+MODULE_LICENSE("GPL v2");

Signature · 2 lines

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

Quoted 20 lines · Tingwei Zhang

From: Kim Phillips <[email protected]>

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 <[email protected]>

Cc Leo Yan <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Kim Phillips <[email protected]>

Signed-off-by Tingwei Zhang <[email protected]>

Reviewed-by Mike Leach <[email protected]>

Quoted 6 lines

---
 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%)

Review

Quoted 2 lines

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/Kconfig

Quoted 13 lines

index 6433f835fc97..8fd9fd139cf3 100644

drivers/hwtracing/coresight/Kconfig +1 -1

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

drivers/hwtracing/coresight/Kconfig config CORESIGHT_SOURCE_ETM3X · line 74

Quoted 1 line

drivers/hwtracing/coresight/Kconfig +0 -0

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

Depending

Review

Quoted 9 lines

drivers/hwtracing/coresight/Kconfig +3 -0

 	  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

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/Makefile

drivers/hwtracing/coresight/Makefile obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o · line 14

Quoted 8 lines

index 19497d1d92bf..d619cfd0abd8 100644

drivers/hwtracing/coresight/Makefile +0 -1

@@ -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 \

Review

Quoted 6 lines

drivers/hwtracing/coresight/Makefile +2 -0

+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

(quoted diff) +0 -0

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

drivers/hwtracing/coresight/coresight-etm3x-core.c static int etm_probe(struct amba_device *adev,

Quoted 7 lines

similarity index 97%
rename from drivers/hwtracing/coresight/coresight-etm3x.c rename to drivers/hwtracing/coresight/coresight-etm3x-core.c index bf22dcfd3327..82b333c40006 100644

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +0 -0

@@ -895,6 +895,23 @@ static int etm_probe(struct amba_device *adev,

const struct amba_id *id)

Quoted 3 lines

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +0 -0

 	return ret;
 }
 

drivers/hwtracing/coresight/coresight-etm3x-core.c static int etm_probe(struct amba_device *adev, const struct amba_id *id) · line 899

Quoted 2 lines

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +2 -0

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

drivers/hwtracing/coresight/coresight-etm3x-core.c static int etm_probe(struct amba_device *adev, const struct amba_id *id) · line 909

Quoted 6 lines

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +6 -0

+	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);

Quoted 24 lines

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +11 -0

+		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,
 	},

drivers/hwtracing/coresight/coresight-etm3x-core.c static struct amba_driver etm_driver = { · line 974

Quoted 10 lines

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +6 -1

 	.probe		= etm_probe,
+	.remove         = etm_remove,
 	.id_table	= etm_ids,
 };
-builtin_amba_driver(etm_driver);
+module_amba_driver(etm_driver);
+
+MODULE_AUTHOR("Pratik Patel <[email protected]>");
+MODULE_AUTHOR("Mathieu Poirier <[email protected]>");
+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 <[email protected]>

Quoted 3 lines

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +1 -0

+MODULE_LICENSE("GPL v2");

Signature · 1 line

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

Forum,

Quoted 2 lines

a Linux Foundation Collaborative Project

From: Kim Phillips <[email protected]>

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 <[email protected]>

Cc Leo Yan <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Kim Phillips <[email protected]>

Signed-off-by Tingwei Zhang <[email protected]>

Reviewed-by Mike Leach <[email protected]>

---
  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%)

drivers/hwtracing/coresight/Kconfig +4 -1

index 6433f835fc97..8fd9fd139cf3 100644
@@ -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

drivers/hwtracing/coresight/Makefile +2 -1

index 19497d1d92bf..d619cfd0abd8 100644
@@ -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

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +2 -0

similarity index 97%
rename from drivers/hwtracing/coresight/coresight-etm3x.c
rename to drivers/hwtracing/coresight/coresight-etm3x-core.c
index bf22dcfd3327..82b333c40006 100644
@@ -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)
+{

Quoted 90 lines · Tingwei Zhang

From: Kim Phillips <[email protected]>

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 <[email protected]>

Cc Leo Yan <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Kim Phillips <[email protected]>

Signed-off-by Tingwei Zhang <[email protected]>

Reviewed-by Mike Leach <[email protected]>

---
 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%)

drivers/hwtracing/coresight/Kconfig +4 -1

index 6433f835fc97..8fd9fd139cf3 100644
@@ -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

drivers/hwtracing/coresight/Makefile +2 -1

index 19497d1d92bf..d619cfd0abd8 100644
@@ -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 \

drivers/hwtracing/coresight/Makefile +0 -0

 					coresight-etm4x-sysfs.o

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +9 -0

similarity index 97%
rename from drivers/hwtracing/coresight/coresight-etm3x.c
rename to drivers/hwtracing/coresight/coresight-etm3x-core.c
index bf22dcfd3327..82b333c40006 100644
@@ -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)

drivers/hwtracing/coresight/coresight-etm3x-core.c static int etm_probe(struct amba_device *adev, const struct amba_id *id) · line 909

Quoted 3 lines

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +3 -0

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

Suzuki

Quoted 91 lines · Suzuki K Poulose

Quoted 90 lines · Tingwei Zhang

From: Kim Phillips <[email protected]>

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 <[email protected]>

Cc Leo Yan <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Kim Phillips <[email protected]>

Signed-off-by Tingwei Zhang <[email protected]>

Reviewed-by Mike Leach <[email protected]>

---
 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%)

drivers/hwtracing/coresight/Kconfig +4 -1

index 6433f835fc97..8fd9fd139cf3 100644
@@ -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

drivers/hwtracing/coresight/Makefile +2 -1

index 19497d1d92bf..d619cfd0abd8 100644
@@ -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

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +9 -0

similarity index 97%
rename from drivers/hwtracing/coresight/coresight-etm3x.c
rename to drivers/hwtracing/coresight/coresight-etm3x-core.c
index bf22dcfd3327..82b333c40006 100644
@@ -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)

Quoted 9 lines

drivers/hwtracing/coresight/coresight-etm3x-core.c static int etm_probe(struct amba_device *adev, const struct amba_id *id) · line 909

Quoted 3 lines

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +3 -0

+                    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

Quoted 2 lines

Suzuki

Quoted 105 lines · Tingwei Zhang

Quoted 91 lines · Suzuki K Poulose

Quoted 90 lines · Tingwei Zhang

From: Kim Phillips <[email protected]>

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 <[email protected]>

Cc Leo Yan <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Kim Phillips <[email protected]>

Signed-off-by Tingwei Zhang <[email protected]>

Reviewed-by Mike Leach <[email protected]>

---
 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%)

drivers/hwtracing/coresight/Kconfig +4 -1

index 6433f835fc97..8fd9fd139cf3 100644
@@ -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

drivers/hwtracing/coresight/Makefile +2 -1

index 19497d1d92bf..d619cfd0abd8 100644
@@ -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

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +9 -0

similarity index 97%
rename from drivers/hwtracing/coresight/coresight-etm3x.c
rename to drivers/hwtracing/coresight/coresight-etm3x-core.c
index bf22dcfd3327..82b333c40006 100644
@@ -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)

Quoted 9 lines

drivers/hwtracing/coresight/coresight-etm3x-core.c static int etm_probe(struct amba_device *adev, const struct amba_id *id) · line 909

Quoted 3 lines

drivers/hwtracing/coresight/coresight-etm3x-core.c renamed from drivers/hwtracing/coresight/coresight-etm3x.c +3 -0

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

Quoted 2 lines

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.

Quoted 5 lines

Thanks,
Tingwei

Quoted 2 lines

Suzuki

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

From: Kim Phillips <[email protected]>

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 <[email protected]>

Cc Leo Yan <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Kim Phillips <[email protected]>

Signed-off-by Tingwei Zhang <[email protected]>

Tested-by Mike Leach <[email protected]>

---
 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%)

drivers/hwtracing/coresight/Kconfig +4 -1

index 8fd9fd139cf3..d6e107bbd30b 100644
@@ -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

drivers/hwtracing/coresight/Makefile +2 -2

index d619cfd0abd8..271dc255454f 100644
@@ -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

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +10 -2

similarity index 95%
rename from drivers/hwtracing/coresight/coresight-etm4x.c
rename to drivers/hwtracing/coresight/coresight-etm4x-core.c
index fddfd93b9a7b..ae9847e194a3 100644
@@ -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)

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +75 -30

 	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 <[email protected]>");
+MODULE_AUTHOR("Mathieu Poirier <[email protected]>");
+MODULE_DESCRIPTION("Arm CoreSight Program Flow Trace v4 driver");
+MODULE_LICENSE("GPL v2");

Signature · 2 lines

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

Quoted 111 lines · Tingwei Zhang

From: Kim Phillips <[email protected]>

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 <[email protected]>

Cc Leo Yan <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Kim Phillips <[email protected]>

Signed-off-by Tingwei Zhang <[email protected]>

Tested-by Mike Leach <[email protected]>

---
  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%)

drivers/hwtracing/coresight/Kconfig +4 -1

index 8fd9fd139cf3..d6e107bbd30b 100644
@@ -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

drivers/hwtracing/coresight/Makefile +2 -2

index d619cfd0abd8..271dc255454f 100644
@@ -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

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +10 -2

similarity index 95%
rename from drivers/hwtracing/coresight/coresight-etm4x.c
rename to drivers/hwtracing/coresight/coresight-etm4x-core.c
index fddfd93b9a7b..ae9847e194a3 100644
@@ -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)

drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_starting_cpu(unsigned int cpu) · line 1120

Quoted 4 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +1 -0

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

Quoted 13 lines · Mathieu Poirier

Quoted 12 lines · Tingwei Zhang

From: Kim Phillips <[email protected]>

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

Quoted 14 lines

Quoted 10 lines

Cc Mathieu Poirier <[email protected]>

Cc Leo Yan <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Kim Phillips <[email protected]>

Signed-off-by Tingwei Zhang <[email protected]>

Tested-by Mike Leach <[email protected]>

Quoted 4 lines

---
 drivers/hwtracing/coresight/Kconfig           |   5 +-
 drivers/hwtracing/coresight/Makefile          |   4 +-
 ...resight-etm4x.c => coresight-etm4x-core.c} | 118

+++++++++++++-----

Quoted 2 lines

Quoted 2 lines

 3 files changed, 92 insertions(+), 35 deletions(-)
 rename drivers/hwtracing/coresight/{coresight-etm4x.c =>

coresight-etm4x-core.c} (95%)

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/Kconfig

Quoted 5 lines

Quoted 5 lines

index 8fd9fd139cf3..d6e107bbd30b 100644

drivers/hwtracing/coresight/Kconfig +0 -0

@@ -78,7 +78,7 @@ config CORESIGHT_SOURCE_ETM3X
 	  module will be called coresight-etm3x.

Quoted 8 lines

drivers/hwtracing/coresight/Kconfig config CORESIGHT_SOURCE_ETM4X · line 88

Quoted 8 lines

drivers/hwtracing/coresight/Kconfig +1 -1

 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

Quoted 8 lines

Review

Quoted 8 lines

drivers/hwtracing/coresight/Kconfig +3 -0

 	  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

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/Makefile

Quoted 4 lines

drivers/hwtracing/coresight/Makefile obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) +=

Quoted 4 lines

index d619cfd0abd8..271dc255454f 100644

drivers/hwtracing/coresight/Makefile +0 -0

@@ -14,8 +14,8 @@ obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) +=

coresight-funnel.o \

Quoted 11 lines

Review

Quoted 11 lines

drivers/hwtracing/coresight/Makefile +2 -2

 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

(quoted diff) +0 -0

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

Quoted 6 lines

Quoted 6 lines

similarity index 95%
rename from drivers/hwtracing/coresight/coresight-etm4x.c rename to drivers/hwtracing/coresight/coresight-etm4x-core.c index fddfd93b9a7b..ae9847e194a3 100644

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +0 -0

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c void etm4_config_trace_mode(struct etmv4_config

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +1 -0

@@ -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)

Quoted 9 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c void etm4_config_trace_mode(struct etmv4_config *config)

Quoted 9 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +4 -1

 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)

Quoted 22 lines

Quoted 15 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +5 -1

 		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)

drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_starting_cpu(unsigned int cpu) · line 1120

Quoted 4 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +1 -0

@@ -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!

Quoted 1 line

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

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

Quoted 133 lines · Suzuki K Poulose

Quoted 13 lines · Mathieu Poirier

Quoted 12 lines · Tingwei Zhang

From: Kim Phillips <[email protected]>

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

Quoted 14 lines

Quoted 10 lines

Cc Mathieu Poirier <[email protected]>

Cc Leo Yan <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Kim Phillips <[email protected]>

Signed-off-by Tingwei Zhang <[email protected]>

Tested-by Mike Leach <[email protected]>

Quoted 4 lines

---
 drivers/hwtracing/coresight/Kconfig           |   5 +-
 drivers/hwtracing/coresight/Makefile          |   4 +-
 ...resight-etm4x.c => coresight-etm4x-core.c} | 118

+++++++++++++-----

Quoted 2 lines

Quoted 2 lines

 3 files changed, 92 insertions(+), 35 deletions(-)
 rename drivers/hwtracing/coresight/{coresight-etm4x.c =>

coresight-etm4x-core.c} (95%)

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/Kconfig

Quoted 5 lines

Quoted 5 lines

index 8fd9fd139cf3..d6e107bbd30b 100644

drivers/hwtracing/coresight/Kconfig +0 -0

@@ -78,7 +78,7 @@ config CORESIGHT_SOURCE_ETM3X
 	  module will be called coresight-etm3x.

Quoted 8 lines

drivers/hwtracing/coresight/Kconfig config CORESIGHT_SOURCE_ETM4X · line 88

Quoted 8 lines

drivers/hwtracing/coresight/Kconfig +1 -1

 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

Quoted 8 lines

Review

Quoted 8 lines

drivers/hwtracing/coresight/Kconfig +3 -0

 	  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

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/Makefile

Quoted 4 lines

drivers/hwtracing/coresight/Makefile obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) +=

Quoted 4 lines

index d619cfd0abd8..271dc255454f 100644

drivers/hwtracing/coresight/Makefile +0 -0

@@ -14,8 +14,8 @@ obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) +=

coresight-funnel.o \

Quoted 11 lines

Review

Quoted 11 lines

drivers/hwtracing/coresight/Makefile +2 -2

 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

(quoted diff) +0 -0

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

Quoted 6 lines

Quoted 6 lines

similarity index 95%
rename from drivers/hwtracing/coresight/coresight-etm4x.c rename to drivers/hwtracing/coresight/coresight-etm4x-core.c index fddfd93b9a7b..ae9847e194a3 100644

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +0 -0

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c void etm4_config_trace_mode(struct etmv4_config

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +1 -0

@@ -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)

Quoted 9 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c void etm4_config_trace_mode(struct etmv4_config · line 1097

Quoted 9 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +4 -1

 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)

Quoted 22 lines

Quoted 15 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +5 -1

 		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)

drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_starting_cpu(unsigned int cpu) · line 1120

Quoted 4 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +1 -0

@@ -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!

Quoted 1 line

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

Quoted 9 lines

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;
}

Quoted 36 lines · Tingwei Zhang

Quoted 35 lines · Suzuki K Poulose

Quoted 13 lines · Mathieu Poirier

Quoted 12 lines · Tingwei Zhang

From: Kim Phillips <[email protected]>

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

Quoted 14 lines

Quoted 10 lines

Cc Mathieu Poirier <[email protected]>

Cc Leo Yan <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Kim Phillips <[email protected]>

Signed-off-by Tingwei Zhang <[email protected]>

Tested-by Mike Leach <[email protected]>

Quoted 4 lines

---
 drivers/hwtracing/coresight/Kconfig           |   5 +-
 drivers/hwtracing/coresight/Makefile          |   4 +-
 ...resight-etm4x.c => coresight-etm4x-core.c} | 118

+++++++++++++-----

Quoted 2 lines

Quoted 2 lines

 3 files changed, 92 insertions(+), 35 deletions(-)
 rename drivers/hwtracing/coresight/{coresight-etm4x.c =>

coresight-etm4x-core.c} (95%)

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/Kconfig

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

index 8fd9fd139cf3..d6e107bbd30b 100644

drivers/hwtracing/coresight/Kconfig +0 -0

@@ -78,7 +78,7 @@ config CORESIGHT_SOURCE_ETM3X
 	  module will be called coresight-etm3x.

Quoted 8 lines

Quoted 8 lines

drivers/hwtracing/coresight/Kconfig config CORESIGHT_SOURCE_ETM4X · line 88

Quoted 8 lines

drivers/hwtracing/coresight/Kconfig +1 -1

 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

Quoted 8 lines

Quoted 8 lines

version

Quoted 7 lines

drivers/hwtracing/coresight/Kconfig config CORESIGHT_SOURCE_ETM4X · line 95

Quoted 7 lines

drivers/hwtracing/coresight/Kconfig +3 -0

 	  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

Quoted 19 lines

Quoted 19 lines

Quoted 1 line

Review

Quoted 1 line

(quoted diff) +0 -0

b/drivers/hwtracing/coresight/Makefile

Quoted 4 lines

drivers/hwtracing/coresight/Makefile obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) +=

Quoted 4 lines

index d619cfd0abd8..271dc255454f 100644

drivers/hwtracing/coresight/Makefile +0 -0

@@ -14,8 +14,8 @@ obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) +=

coresight-funnel.o \

Quoted 11 lines

Review

Quoted 11 lines

drivers/hwtracing/coresight/Makefile +2 -2

 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

(quoted diff) +0 -0

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

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c module_param(pm_save_enable, int, 0444); · line 49

Quoted 9 lines

similarity index 95%
rename from drivers/hwtracing/coresight/coresight-etm4x.c rename to drivers/hwtracing/coresight/coresight-etm4x-core.c index fddfd93b9a7b..ae9847e194a3 100644

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +0 -0

@@ -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");

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c void etm4_config_trace_mode(struct

Quoted 5 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +1 -0

+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

Quoted 11 lines

Quoted 11 lines

*config)

Quoted 9 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c void etm4_config_trace_mode(struct · line 1097

Quoted 9 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +4 -1

 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)

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +1 -1

 		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])

Quoted 8 lines

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_starting_cpu(unsigned int

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +4 -0

+	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)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c static int etm4_starting_cpu(unsigned int · line 1120

Quoted 4 lines

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +1 -0

 	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.

Quoted 3 lines

Quoted 3 lines

Actually this mutex is for "protecting" etmdrvdata[cpu], not the drvdata struct as such. But as you said, it becomes like a double

lock.

Quoted 8 lines

Quoted 8 lines

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

Quoted 24 lines

Quoted 8 lines

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;
}

Quoted 8 lines

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

Quoted 6 lines

         * CPU hotplug call backs.
         */
        cpus_read_lock();
        if (cpu_online(drvdata->cpu))
                /*
                 * The readers for etmdrvdata[] are CPU hotplug call

backs

Quoted 1 line

* and PM notification call backs. Change etmdrvdata[i]

on

Quoted 4 lines

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

Quoted 5 lines

        else
                etmdrvdata[drvdata->cpu] = NULL;

        cpus_read_unlock();

Quoted 4 lines

        coresight_unregister(drvdata->csdev);

        return 0;
}

Yes, this is exactly what I was looking for. Additionally we should fix the races mentioned in the link above. I believe, we can solve that by the following :

---8>---

coresight: etm4x: Delay advertising per-cpu drvdata

Delay advertising the per-cpu etmdrvdata until we have successfully initialised. This is to avoid races and unnecessary save/restore of the ETM state, before the device is properly setup.

Signed-off-by Suzuki K Poulose <[email protected]>

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

drivers/hwtracing/coresight/coresight-etm4x.c +0 -2

index 6d7d2169bfb2..30f7ffa07eb6 100644
@@ -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))

drivers/hwtracing/coresight/coresight-etm4x.c +1 -3

 		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;
-	}
 

drivers/hwtracing/coresight/coresight-etm4x.c +2 -0

 	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",

drivers/hwtracing/coresight/coresight-etm4x.c +0 -1

 		 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;

}

Signature · 1 line

2.24.1

Quoted 77 lines · Suzuki K Poulose

Quoted 7 lines · Tingwei Zhang

Quoted 6 lines · Suzuki K Poulose

Quoted 5 lines · Mathieu Poirier

Review static int etm4_starting_cpu(unsigned int

Quoted 4 lines · Tingwei Zhang

From: Kim Phillips <[email protected]>

(quoted diff) +0 -0

@@ -1109,18 +1117,23 @@ static int etm4_starting_cpu(unsigned int

cpu)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Review static int etm4_starting_cpu(unsigned int · line 1120

Quoted 4 lines

(quoted diff) +1 -0

  	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.

Quoted 3 lines

Quoted 3 lines

Actually this mutex is for "protecting" etmdrvdata[cpu], not the drvdata struct as such. But as you said, it becomes like a double

lock.

Quoted 8 lines

Quoted 8 lines

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

Quoted 24 lines

Quoted 8 lines

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;
}

Quoted 8 lines

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

Quoted 6 lines

          * CPU hotplug call backs.
          */
         cpus_read_lock();
         if (cpu_online(drvdata->cpu))
                 /*
                  * The readers for etmdrvdata[] are CPU hotplug call

backs

Quoted 1 line

* and PM notification call backs. Change etmdrvdata[i]

on

Quoted 4 lines

* CPU i ensures these call backs has consistent view
* inside one call back function.
*/
smp_call_function_single(drvdata->cpu, clear_etmdrvdata,

Quoted 6 lines

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

Quoted 10 lines

Quoted 9 lines

         else
                 etmdrvdata[drvdata->cpu] = NULL;

         cpus_read_unlock();

         coresight_unregister(drvdata->csdev);

         return 0;
}

Quoted 4 lines

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

From: Kim Phillips <[email protected]>

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 <[email protected]>

Cc Leo Yan <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Randy Dunlap <[email protected]>

Cc Suzuki K Poulose <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc Russell King <[email protected]>

Signed-off-by Kim Phillips <[email protected]>

Signed-off-by Tingwei Zhang <[email protected]>

Tested-by Mike Leach <[email protected]>

---
 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%)

drivers/hwtracing/coresight/Kconfig +4 -1

index 8fd9fd139cf3..d6e107bbd30b 100644
@@ -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

drivers/hwtracing/coresight/Makefile +2 -2

index d619cfd0abd8..271dc255454f 100644
@@ -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

drivers/hwtracing/coresight/coresight-etm4x-core.c renamed from drivers/hwtracing/coresight/coresight-etm4x.c +75 -32

similarity index 95%
rename from drivers/hwtracing/coresight/coresight-etm4x.c
rename to drivers/hwtracing/coresight/coresight-etm4x-core.c
index fddfd93b9a7b..ae9847e194a3 100644
@@ -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);