[PATCH v4 00/20] coresight: allow to build coresight as modules

[PATCH v4 00/20] coresight: allow to build coresight as modules

[Tingwei Zhang]

Allow to build coresight as modules. This gives developers the feasibility to
test their code without reboot.

This series is based on below two series.

 - "coresight: allow to build components as modules"
   https://lkml.org/lkml/2018/6/5/989
 - "coresight: make drivers modular"
   https://lkml.org/lkml/2020/1/17/468

Change from v3:
Rebase to coresight-next (Mike and Mathieu)
Reorder try_get_module() (Suzuki)
Clean up etmdrvdata[] in device remote path (Mike)
Move cti_remove_conn_xrefs to cti_remove (Mike)

Change from v2:
Rebase to 5.8-rc5. Export coresight_add_sysfs_link and
coresight_remove_sysfs_link
Fix one cut and paste error on MODULE_DESCRIPTION of CTI

Change from v1:
Use try_module_get() to avoid module to be unloaded when device is used
in active trace session. (Mathieu P)

Change from above two series.
This series adds the support to dynamically remove module when the device in
that module is enabled and used by some trace path. It disables all trace
paths with that device and release the trace path.

Kim Phillips (7):
  coresight: use IS_ENABLED for CONFIGs that may be modules
  coresight: allow etm3x to be built as a module
  coresight: allow etm4x to be built as a module
  coresight: allow etb to be built as a module
  coresight: allow tpiu to be built as a module
  coresight: allow tmc to be built as a module
  coresight: allow funnel and replicator drivers to be built as modules

Mian Yousaf Kaukab (4):
  coresight: export global symbols
  coresight: remove multiple init calls from funnel driver
  coresight: remove multiple init calls from replicator driver
  coresight: tmc-etr: add function to register catu ops

Tingwei Zhang (9):
  coresight: cpu_debug: add module name in Kconfig
  coresight: cpu_debug: define MODULE_DEVICE_TABLE
  coresight: add coresight prefix to barrier_pkt
  coresight: add try_get_module() in coresight_grab_device()
  allow to build coresight-stm as a module, for ease of development.
  coresight: cti: add function to register cti associate ops
  coresight: allow cti to be built as a module
  coresight: allow catu drivers to be built as modules
  coresight: allow the coresight core driver to be built as a module

 drivers/hwtracing/coresight/Kconfig           |  54 ++++++++--
 drivers/hwtracing/coresight/Makefile          |  22 ++--
 drivers/hwtracing/coresight/coresight-catu.c  |  37 ++++++-
 drivers/hwtracing/coresight/coresight-catu.h  |   2 -
 .../{coresight.c => coresight-core.c}         | 102 ++++++++++++++----
 .../hwtracing/coresight/coresight-cpu-debug.c |   2 +
 .../{coresight-cti.c => coresight-cti-core.c} |  51 ++++++++-
 drivers/hwtracing/coresight/coresight-etb10.c |  22 +++-
 .../hwtracing/coresight/coresight-etm-perf.c  |   9 +-
 .../hwtracing/coresight/coresight-etm-perf.h  |   5 +-
 ...resight-etm3x.c => coresight-etm3x-core.c} |  27 ++++-
 ...resight-etm4x.c => coresight-etm4x-core.c} |  26 ++++-
 .../hwtracing/coresight/coresight-funnel.c    |  62 ++++++++++-
 .../hwtracing/coresight/coresight-platform.c  |   1 +
 drivers/hwtracing/coresight/coresight-priv.h  |  24 ++---
 .../coresight/coresight-replicator.c          |  63 ++++++++++-
 drivers/hwtracing/coresight/coresight-stm.c   |  20 +++-
 drivers/hwtracing/coresight/coresight-sysfs.c |   2 +
 .../{coresight-tmc.c => coresight-tmc-core.c} |  19 +++-
 .../hwtracing/coresight/coresight-tmc-etf.c   |   2 +-
 .../hwtracing/coresight/coresight-tmc-etr.c   |  21 +++-
 drivers/hwtracing/coresight/coresight-tmc.h   |   3 +
 drivers/hwtracing/coresight/coresight-tpiu.c  |  19 +++-
 include/linux/coresight.h                     |   2 +-
 24 files changed, 520 insertions(+), 77 deletions(-)
 rename drivers/hwtracing/coresight/{coresight.c => coresight-core.c} (95%)
 rename drivers/hwtracing/coresight/{coresight-cti.c => coresight-cti-core.c} (96%)
 rename drivers/hwtracing/coresight/{coresight-etm3x.c => coresight-etm3x-core.c} (97%)
 rename drivers/hwtracing/coresight/{coresight-etm4x.c => coresight-etm4x-core.c} (98%)
 rename drivers/hwtracing/coresight/{coresight-tmc.c => coresight-tmc-core.c} (96%)

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


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

[PATCH v4 01/20] coresight: cpu_debug: add module name in Kconfig

[Tingwei Zhang]

Provide name of cpu_debug module in Kconfig help section.

Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/Kconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 02dbb5ca3bcf..4663fd1bbffc 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -110,6 +110,9 @@ config CORESIGHT_CPU_DEBUG
 	  properly, please refer Documentation/trace/coresight/coresight-cpu-debug.rst
 	  for detailed description and the example for usage.
 
+	  To compile this driver as a module, choose M here: the
+	  module will be called coresight-cpu-debug.
+
 config CORESIGHT_CTI
 	bool "CoreSight Cross Trigger Interface (CTI) driver"
 	depends on ARM || ARM64
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 02/20] coresight: cpu_debug: define MODULE_DEVICE_TABLE

[Tingwei Zhang]

Define a MODULE_DEVICE_TABLE for cpu_debug so module can
be auto loaded on boot.

Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-cpu-debug.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-cpu-debug.c b/drivers/hwtracing/coresight/coresight-cpu-debug.c
index 96544b348c27..1d0880b3764a 100644
--- a/drivers/hwtracing/coresight/coresight-cpu-debug.c
+++ b/drivers/hwtracing/coresight/coresight-cpu-debug.c
@@ -665,6 +665,8 @@ static const struct amba_id debug_ids[] = {
 	{},
 };
 
+MODULE_DEVICE_TABLE(amba, debug_ids);
+
 static struct amba_driver debug_driver = {
 	.drv = {
 		.name   = "coresight-cpu-debug",
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 03/20] coresight: use IS_ENABLED for CONFIGs that may be modules

[Tingwei Zhang]

From: Kim Phillips <kim.phillips@arm.com>

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

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Kim Phillips <kim.phillips@arm.com>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-etm-perf.h | 2 +-
 drivers/hwtracing/coresight/coresight-priv.h     | 2 +-
 include/linux/coresight.h                        | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
index 015213abe00a..05f89723e282 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.h
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
@@ -57,7 +57,7 @@ struct etm_event_data {
 	struct list_head * __percpu *path;
 };
 
-#ifdef CONFIG_CORESIGHT
+#if IS_ENABLED(CONFIG_CORESIGHT)
 int etm_perf_symlink(struct coresight_device *csdev, bool link);
 int etm_perf_add_symlink_sink(struct coresight_device *csdev);
 void etm_perf_del_symlink_sink(struct coresight_device *csdev);
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index f2dc625ea585..d801a2755432 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -165,7 +165,7 @@ int coresight_make_links(struct coresight_device *orig,
 void coresight_remove_links(struct coresight_device *orig,
 			    struct coresight_connection *conn);
 
-#ifdef CONFIG_CORESIGHT_SOURCE_ETM3X
+#if IS_ENABLED(CONFIG_CORESIGHT_SOURCE_ETM3X)
 extern int etm_readl_cp14(u32 off, unsigned int *val);
 extern int etm_writel_cp14(u32 off, u32 val);
 #else
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 58fffdecdbfd..3bb738f9a326 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -324,7 +324,7 @@ struct coresight_ops {
 	const struct coresight_ops_ect *ect_ops;
 };
 
-#ifdef CONFIG_CORESIGHT
+#if IS_ENABLED(CONFIG_CORESIGHT)
 extern struct coresight_device *
 coresight_register(struct coresight_desc *desc);
 extern void coresight_unregister(struct coresight_device *csdev);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 04/20] coresight: add coresight prefix to barrier_pkt

[Tingwei Zhang]

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

Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 drivers/hwtracing/coresight/coresight-etb10.c   | 2 +-
 drivers/hwtracing/coresight/coresight-priv.h    | 8 ++++----
 drivers/hwtracing/coresight/coresight-tmc-etf.c | 2 +-
 drivers/hwtracing/coresight/coresight.c         | 3 ++-
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index 03e3f2590191..04ee9cda988d 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -525,7 +525,7 @@ static unsigned long etb_update_buffer(struct coresight_device *csdev,
 
 	cur = buf->cur;
 	offset = buf->offset;
-	barrier = barrier_pkt;
+	barrier = coresight_barrier_pkt;
 
 	for (i = 0; i < to_read; i += 4) {
 		buf_ptr = buf->data_pages[cur] + offset;
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index d801a2755432..dcb8aeb6af62 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -66,8 +66,8 @@ static DEVICE_ATTR_RO(name)
 #define coresight_simple_reg64(type, name, lo_off, hi_off)		\
 	__coresight_simple_func(type, NULL, name, lo_off, hi_off)
 
-extern const u32 barrier_pkt[4];
-#define CORESIGHT_BARRIER_PKT_SIZE (sizeof(barrier_pkt))
+extern const u32 coresight_barrier_pkt[4];
+#define CORESIGHT_BARRIER_PKT_SIZE (sizeof(coresight_barrier_pkt))
 
 enum etm_addr_type {
 	ETM_ADDR_TYPE_NONE,
@@ -104,10 +104,10 @@ struct cs_buffers {
 static inline void coresight_insert_barrier_packet(void *buf)
 {
 	if (buf)
-		memcpy(buf, barrier_pkt, CORESIGHT_BARRIER_PKT_SIZE);
+		memcpy(buf, coresight_barrier_pkt,
+				CORESIGHT_BARRIER_PKT_SIZE);
 }
 
-
 static inline void CS_LOCK(void __iomem *addr)
 {
 	do {
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 6375504ba8b0..44402d413ebb 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -519,7 +519,7 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev,
 
 	cur = buf->cur;
 	offset = buf->offset;
-	barrier = barrier_pkt;
+	barrier = coresight_barrier_pkt;
 
 	/* for every byte to read */
 	for (i = 0; i < to_read; i += 4) {
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index e9c90f2de34a..310b1b825dd1 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -53,7 +53,8 @@ static struct list_head *stm_path;
  * beginning of the data collected in a buffer.  That way the decoder knows that
  * it needs to look for another sync sequence.
  */
-const u32 barrier_pkt[4] = {0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fffffff};
+const u32 coresight_barrier_pkt[4] = {
+		0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fffffff};
 
 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


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

[PATCH v4 05/20] coresight: export global symbols

[Tingwei Zhang]

From: Mian Yousaf Kaukab <ykaukab@suse.de>

Export symbols used among coresight modules.

Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/coresight-etm-perf.c | 1 +
 drivers/hwtracing/coresight/coresight-sysfs.c    | 2 ++
 drivers/hwtracing/coresight/coresight-tmc-etr.c  | 6 ++++++
 drivers/hwtracing/coresight/coresight.c          | 8 ++++++++
 4 files changed, 17 insertions(+)

diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index 1a3169e69bb1..dcb0592418ae 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -517,6 +517,7 @@ int etm_perf_symlink(struct coresight_device *csdev, bool link)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(etm_perf_symlink);
 
 static ssize_t etm_perf_sink_name_show(struct device *dev,
 				       struct device_attribute *dattr,
diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c
index 82afeaf2ccc4..34d2a2d31d00 100644
--- a/drivers/hwtracing/coresight/coresight-sysfs.c
+++ b/drivers/hwtracing/coresight/coresight-sysfs.c
@@ -102,6 +102,7 @@ int coresight_add_sysfs_link(struct coresight_sysfs_link *info)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(coresight_add_sysfs_link);
 
 void coresight_remove_sysfs_link(struct coresight_sysfs_link *info)
 {
@@ -122,6 +123,7 @@ void coresight_remove_sysfs_link(struct coresight_sysfs_link *info)
 	info->orig->nr_links--;
 	info->target->nr_links--;
 }
+EXPORT_SYMBOL_GPL(coresight_remove_sysfs_link);
 
 /*
  * coresight_make_links: Make a link for a connection from a @orig
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index b29c2db94d96..ad991a37e2d2 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -255,6 +255,7 @@ void tmc_free_sg_table(struct tmc_sg_table *sg_table)
 	tmc_free_table_pages(sg_table);
 	tmc_free_data_pages(sg_table);
 }
+EXPORT_SYMBOL_GPL(tmc_free_sg_table);
 
 /*
  * Alloc pages for the table. Since this will be used by the device,
@@ -340,6 +341,7 @@ struct tmc_sg_table *tmc_alloc_sg_table(struct device *dev,
 
 	return sg_table;
 }
+EXPORT_SYMBOL_GPL(tmc_alloc_sg_table);
 
 /*
  * tmc_sg_table_sync_data_range: Sync the data buffer written
@@ -360,6 +362,7 @@ void tmc_sg_table_sync_data_range(struct tmc_sg_table *table,
 					PAGE_SIZE, DMA_FROM_DEVICE);
 	}
 }
+EXPORT_SYMBOL_GPL(tmc_sg_table_sync_data_range);
 
 /* tmc_sg_sync_table: Sync the page table */
 void tmc_sg_table_sync_table(struct tmc_sg_table *sg_table)
@@ -372,6 +375,7 @@ void tmc_sg_table_sync_table(struct tmc_sg_table *sg_table)
 		dma_sync_single_for_device(real_dev, table_pages->daddrs[i],
 					   PAGE_SIZE, DMA_TO_DEVICE);
 }
+EXPORT_SYMBOL_GPL(tmc_sg_table_sync_table);
 
 /*
  * tmc_sg_table_get_data: Get the buffer pointer for data @offset
@@ -401,6 +405,7 @@ ssize_t tmc_sg_table_get_data(struct tmc_sg_table *sg_table,
 		*bufpp = page_address(data_pages->pages[pg_idx]) + pg_offset;
 	return len;
 }
+EXPORT_SYMBOL_GPL(tmc_sg_table_get_data);
 
 #ifdef ETR_SG_DEBUG
 /* Map a dma address to virtual address */
@@ -766,6 +771,7 @@ tmc_etr_get_catu_device(struct tmc_drvdata *drvdata)
 
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(tmc_etr_get_catu_device);
 
 static inline int tmc_etr_enable_catu(struct tmc_drvdata *drvdata,
 				      struct etr_buf *etr_buf)
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 310b1b825dd1..b7151c5f81b1 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -55,6 +55,7 @@ static struct list_head *stm_path;
  */
 const u32 coresight_barrier_pkt[4] = {
 		0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fffffff};
+EXPORT_SYMBOL_GPL(coresight_barrier_pkt);
 
 static int coresight_id_match(struct device *dev, void *data)
 {
@@ -180,6 +181,7 @@ int coresight_claim_device_unlocked(void __iomem *base)
 	coresight_clear_claim_tags(base);
 	return -EBUSY;
 }
+EXPORT_SYMBOL_GPL(coresight_claim_device_unlocked);
 
 int coresight_claim_device(void __iomem *base)
 {
@@ -191,6 +193,7 @@ int coresight_claim_device(void __iomem *base)
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(coresight_claim_device);
 
 /*
  * coresight_disclaim_device_unlocked : Clear the claim tags for the device.
@@ -209,6 +212,7 @@ void coresight_disclaim_device_unlocked(void __iomem *base)
 		 */
 		WARN_ON_ONCE(1);
 }
+EXPORT_SYMBOL_GPL(coresight_disclaim_device_unlocked);
 
 void coresight_disclaim_device(void __iomem *base)
 {
@@ -216,6 +220,7 @@ void coresight_disclaim_device(void __iomem *base)
 	coresight_disclaim_device_unlocked(base);
 	CS_LOCK(base);
 }
+EXPORT_SYMBOL_GPL(coresight_disclaim_device);
 
 /* enable or disable an associated CTI device of the supplied CS device */
 static int
@@ -468,6 +473,7 @@ void coresight_disable_path(struct list_head *path)
 {
 	coresight_disable_path_from(path, NULL);
 }
+EXPORT_SYMBOL_GPL(coresight_disable_path);
 
 int coresight_enable_path(struct list_head *path, u32 mode, void *sink_data)
 {
@@ -1377,6 +1383,7 @@ int coresight_timeout(void __iomem *addr, u32 offset, int position, int value)
 
 	return -EAGAIN;
 }
+EXPORT_SYMBOL_GPL(coresight_timeout);
 
 struct bus_type coresight_bustype = {
 	.name	= "coresight",
@@ -1553,6 +1560,7 @@ bool coresight_loses_context_with_cpu(struct device *dev)
 	return fwnode_property_present(dev_fwnode(dev),
 				       "arm,coresight-loses-context-with-cpu");
 }
+EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
 
 /*
  * coresight_alloc_device_name - Get an index for a given device in the
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

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

Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index b7151c5f81b1..17bc76ea86ae 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -640,7 +640,7 @@ struct coresight_device *coresight_get_sink_by_id(u32 id)
  * 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;
 
@@ -648,10 +648,25 @@ static void coresight_grab_device(struct coresight_device *csdev)
 		struct coresight_device *child;
 
 		child  = csdev->pdata->conns[i].child_dev;
-		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
+		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
+			if (!try_module_get(child->dev.parent->driver->owner))
+				goto err;
 			pm_runtime_get_sync(child->dev.parent);
+		}
 	}
+	if (!try_module_get(csdev->dev.parent->driver->owner))
+		goto err;
 	pm_runtime_get_sync(csdev->dev.parent);
+	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)
+			pm_runtime_put(child->dev.parent);
+	}
+	return -ENODEV;
 }
 
 /*
@@ -663,12 +678,15 @@ static void coresight_drop_device(struct coresight_device *csdev)
 	int i;
 
 	pm_runtime_put(csdev->dev.parent);
+	module_put(csdev->dev.parent->driver->owner);
 	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)
+		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
 			pm_runtime_put(child->dev.parent);
+			module_put(child->dev.parent->driver->owner);
+		}
 	}
 }
 
@@ -721,7 +739,8 @@ static int _coresight_build_path(struct coresight_device *csdev,
 	if (!node)
 		return -ENOMEM;
 
-	coresight_grab_device(csdev);
+	if (coresight_grab_device(csdev))
+		return -ENODEV;
 	node->csdev = csdev;
 	list_add(&node->link, path);
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 07/20] allow to build coresight-stm as a module, for ease of development.

[Tingwei Zhang]

- Kconfig becomes a tristate, to allow =m
- add a stm_remove function, for module unload
- add a MODULE_DEVICE_TABLE for autoloading on boot

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Kim Phillips <kim.phillips@arm.com>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/Kconfig         |  5 ++++-
 drivers/hwtracing/coresight/coresight-stm.c | 20 +++++++++++++++++++-
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 4663fd1bbffc..6433f835fc97 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -86,7 +86,7 @@ config CORESIGHT_SOURCE_ETM4X
 	  data tracing may also be available.
 
 config CORESIGHT_STM
-	bool "CoreSight System Trace Macrocell driver"
+	tristate "CoreSight System Trace Macrocell driver"
 	depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
 	select CORESIGHT_LINKS_AND_SINKS
 	select STM
@@ -96,6 +96,9 @@ config CORESIGHT_STM
 	  logging useful software events or data coming from various entities
 	  in the system, possibly running different OSs
 
+	  To compile this driver as a module, choose M here: the
+	  module will be called coresight-stm.
+
 config CORESIGHT_CPU_DEBUG
 	tristate "CoreSight CPU Debug driver"
 	depends on ARM || ARM64
diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c
index 673d2f56ed1e..b74072e78436 100644
--- a/drivers/hwtracing/coresight/coresight-stm.c
+++ b/drivers/hwtracing/coresight/coresight-stm.c
@@ -948,6 +948,17 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id)
 	return ret;
 }
 
+static int __exit stm_remove(struct amba_device *adev)
+{
+	struct stm_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+	coresight_unregister(drvdata->csdev);
+
+	stm_unregister_device(&drvdata->stm);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM
 static int stm_runtime_suspend(struct device *dev)
 {
@@ -980,6 +991,8 @@ static const struct amba_id stm_ids[] = {
 	{ 0, 0},
 };
 
+MODULE_DEVICE_TABLE(amba, stm_ids);
+
 static struct amba_driver stm_driver = {
 	.drv = {
 		.name   = "coresight-stm",
@@ -988,7 +1001,12 @@ static struct amba_driver stm_driver = {
 		.suppress_bind_attrs = true,
 	},
 	.probe          = stm_probe,
+	.remove         = stm_remove,
 	.id_table	= stm_ids,
 };
 
-builtin_amba_driver(stm_driver);
+module_amba_driver(stm_driver);
+
+MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>");
+MODULE_DESCRIPTION("Arm CoreSight System Trace Macrocell driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 08/20] coresight: allow etm3x to be built as a module

[Tingwei Zhang]

From: Kim Phillips <kim.phillips@arm.com>

Allow to build coresight-etm3x as a module, for ease of development.

- Kconfig becomes a tristate, to allow =m
- append -core to source file name to allow module to
  be called coresight-etm3x by the Makefile
- add an etm_remove function, for module unload
- add a MODULE_DEVICE_TABLE for autoloading on boot

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Kim Phillips <kim.phillips@arm.com>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/Kconfig           |  5 +++-
 drivers/hwtracing/coresight/Makefile          |  3 ++-
 ...resight-etm3x.c => coresight-etm3x-core.c} | 27 ++++++++++++++++++-
 3 files changed, 32 insertions(+), 3 deletions(-)
 rename drivers/hwtracing/coresight/{coresight-etm3x.c => coresight-etm3x-core.c} (97%)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 6433f835fc97..8fd9fd139cf3 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -65,7 +65,7 @@ config CORESIGHT_SINK_ETBV10
 	  special enhancement or added features.
 
 config CORESIGHT_SOURCE_ETM3X
-	bool "CoreSight Embedded Trace Macrocell 3.x driver"
+	tristate "CoreSight Embedded Trace Macrocell 3.x driver"
 	depends on !ARM64
 	select CORESIGHT_LINKS_AND_SINKS
 	help
@@ -74,6 +74,9 @@ config CORESIGHT_SOURCE_ETM3X
 	  This is primarily useful for instruction level tracing.  Depending
 	  the ETM version data tracing may also be available.
 
+	  To compile this driver as a module, choose M here: the
+	  module will be called coresight-etm3x.
+
 config CORESIGHT_SOURCE_ETM4X
 	bool "CoreSight Embedded Trace Macrocell 4.x driver"
 	depends on ARM64
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 19497d1d92bf..d619cfd0abd8 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -11,7 +11,8 @@ obj-$(CONFIG_CORESIGHT_SINK_TPIU) += coresight-tpiu.o
 obj-$(CONFIG_CORESIGHT_SINK_ETBV10) += coresight-etb10.o
 obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
 					   coresight-replicator.o
-obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o coresight-etm-cp14.o \
+obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o
+coresight-etm3x-y := coresight-etm3x-core.o coresight-etm-cp14.o \
 					coresight-etm3x-sysfs.o
 obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
 					coresight-etm4x-sysfs.o
diff --git a/drivers/hwtracing/coresight/coresight-etm3x.c b/drivers/hwtracing/coresight/coresight-etm3x-core.c
similarity index 97%
rename from drivers/hwtracing/coresight/coresight-etm3x.c
rename to drivers/hwtracing/coresight/coresight-etm3x-core.c
index bf22dcfd3327..82b333c40006 100644
--- a/drivers/hwtracing/coresight/coresight-etm3x.c
+++ b/drivers/hwtracing/coresight/coresight-etm3x-core.c
@@ -895,6 +895,23 @@ static int etm_probe(struct amba_device *adev, const struct amba_id *id)
 	return ret;
 }
 
+static int __exit etm_remove(struct amba_device *adev)
+{
+	struct etm_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+	etm_perf_symlink(drvdata->csdev, false);
+
+	if (--etm_count == 0) {
+		cpuhp_remove_state_nocalls(CPUHP_AP_ARM_CORESIGHT_STARTING);
+		if (hp_online)
+			cpuhp_remove_state_nocalls(hp_online);
+	}
+
+	coresight_unregister(drvdata->csdev);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM
 static int etm_runtime_suspend(struct device *dev)
 {
@@ -937,6 +954,8 @@ static const struct amba_id etm_ids[] = {
 	{ 0, 0},
 };
 
+MODULE_DEVICE_TABLE(amba, etm_ids);
+
 static struct amba_driver etm_driver = {
 	.drv = {
 		.name	= "coresight-etm3x",
@@ -945,6 +964,12 @@ static struct amba_driver etm_driver = {
 		.suppress_bind_attrs = true,
 	},
 	.probe		= etm_probe,
+	.remove         = etm_remove,
 	.id_table	= etm_ids,
 };
-builtin_amba_driver(etm_driver);
+module_amba_driver(etm_driver);
+
+MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>");
+MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>");
+MODULE_DESCRIPTION("Arm CoreSight Program Flow Trace driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 09/20] coresight: allow etm4x to be built as a module

[Tingwei Zhang]

From: Kim Phillips <kim.phillips@arm.com>

Allow to build coresight-etm4x as a module, for ease of development.

- Kconfig becomes a tristate, to allow =m
- append -core to source file name to allow module to
  be called coresight-etm4x by the Makefile
- add an etm4_remove function, for module unload
- add a MODULE_DEVICE_TABLE for autoloading on boot

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Kim Phillips <kim.phillips@arm.com>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/Kconfig           |  5 +++-
 drivers/hwtracing/coresight/Makefile          |  4 +--
 ...resight-etm4x.c => coresight-etm4x-core.c} | 26 ++++++++++++++++++-
 3 files changed, 31 insertions(+), 4 deletions(-)
 rename drivers/hwtracing/coresight/{coresight-etm4x.c => coresight-etm4x-core.c} (98%)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 8fd9fd139cf3..d6e107bbd30b 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -78,7 +78,7 @@ config CORESIGHT_SOURCE_ETM3X
 	  module will be called coresight-etm3x.
 
 config CORESIGHT_SOURCE_ETM4X
-	bool "CoreSight Embedded Trace Macrocell 4.x driver"
+	tristate "CoreSight Embedded Trace Macrocell 4.x driver"
 	depends on ARM64
 	select CORESIGHT_LINKS_AND_SINKS
 	select PID_IN_CONTEXTIDR
@@ -88,6 +88,9 @@ config CORESIGHT_SOURCE_ETM4X
 	  for instruction level tracing. Depending on the implemented version
 	  data tracing may also be available.
 
+	  To compile this driver as a module, choose M here: the
+	  module will be called coresight-etm4x.
+
 config CORESIGHT_STM
 	tristate "CoreSight System Trace Macrocell driver"
 	depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index d619cfd0abd8..271dc255454f 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -14,8 +14,8 @@ obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-funnel.o \
 obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o
 coresight-etm3x-y := coresight-etm3x-core.o coresight-etm-cp14.o \
 					coresight-etm3x-sysfs.o
-obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o \
-					coresight-etm4x-sysfs.o
+obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o
+coresight-etm4x-y := coresight-etm4x-core.o coresight-etm4x-sysfs.o
 obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
 obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
 obj-$(CONFIG_CORESIGHT_CATU) += coresight-catu.o
diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
similarity index 98%
rename from drivers/hwtracing/coresight/coresight-etm4x.c
rename to drivers/hwtracing/coresight/coresight-etm4x-core.c
index 6d7d2169bfb2..8149853d5e9d 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -1573,6 +1573,21 @@ 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);
+
+	etmdrvdata[drvdata->cpu] = NULL;
+	etm4_pm_clear();
+
+	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 */
@@ -1590,12 +1605,21 @@ static const struct amba_id etm4_ids[] = {
 	{},
 };
 
+MODULE_DEVICE_TABLE(amba, etm4_ids);
+
 static struct amba_driver etm4x_driver = {
 	.drv = {
 		.name   = "coresight-etm4x",
+		.owner  = THIS_MODULE,
 		.suppress_bind_attrs = true,
 	},
 	.probe		= etm4_probe,
+	.remove         = etm4_remove,
 	.id_table	= etm4_ids,
 };
-builtin_amba_driver(etm4x_driver);
+module_amba_driver(etm4x_driver);
+
+MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>");
+MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>");
+MODULE_DESCRIPTION("Arm CoreSight Program Flow Trace v4 driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 10/20] coresight: allow etb to be built as a module

[Tingwei Zhang]

From: Kim Phillips <kim.phillips@arm.com>

Allow to build coresight-etb10 as a module, for ease of development.

- Kconfig becomes a tristate, to allow =m
- add an etb_remove function, for module unload
- add a MODULE_DEVICE_TABLE for autoloading on boot

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Kim Phillips <kim.phillips@arm.com>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/Kconfig           |  5 ++++-
 drivers/hwtracing/coresight/coresight-etb10.c | 20 ++++++++++++++++++-
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index d6e107bbd30b..996d84a1edb8 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -57,13 +57,16 @@ config CORESIGHT_SINK_TPIU
 	  the on-board coresight memory can handle.
 
 config CORESIGHT_SINK_ETBV10
-	bool "Coresight ETBv1.0 driver"
+	tristate "Coresight ETBv1.0 driver"
 	depends on CORESIGHT_LINKS_AND_SINKS
 	help
 	  This enables support for the Embedded Trace Buffer version 1.0 driver
 	  that complies with the generic implementation of the component without
 	  special enhancement or added features.
 
+	  To compile this driver as a module, choose M here: the
+	  module will be called coresight-etb10.
+
 config CORESIGHT_SOURCE_ETM3X
 	tristate "CoreSight Embedded Trace Macrocell 3.x driver"
 	depends on !ARM64
diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
index 04ee9cda988d..b40756497c9a 100644
--- a/drivers/hwtracing/coresight/coresight-etb10.c
+++ b/drivers/hwtracing/coresight/coresight-etb10.c
@@ -801,6 +801,16 @@ static int etb_probe(struct amba_device *adev, const struct amba_id *id)
 	return ret;
 }
 
+static int __exit etb_remove(struct amba_device *adev)
+{
+	struct etb_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+	misc_deregister(&drvdata->miscdev);
+	coresight_unregister(drvdata->csdev);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM
 static int etb_runtime_suspend(struct device *dev)
 {
@@ -835,6 +845,8 @@ static const struct amba_id etb_ids[] = {
 	{ 0, 0},
 };
 
+MODULE_DEVICE_TABLE(amba, etb_ids);
+
 static struct amba_driver etb_driver = {
 	.drv = {
 		.name	= "coresight-etb10",
@@ -844,6 +856,12 @@ static struct amba_driver etb_driver = {
 
 	},
 	.probe		= etb_probe,
+	.remove		= etb_remove,
 	.id_table	= etb_ids,
 };
-builtin_amba_driver(etb_driver);
+module_amba_driver(etb_driver);
+
+MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>");
+MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>");
+MODULE_DESCRIPTION("Arm CoreSight Embedded Trace Buffer driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 11/20] coresight: allow tpiu to be built as a module

[Tingwei Zhang]

From: Kim Phillips <kim.phillips@arm.com>

Allow to build coresight-tpiu as a module, for ease of development.

- Kconfig becomes a tristate, to allow =m
- add a tpiu_remove function, for module unload
- add a MODULE_DEVICE_TABLE for autoloading on boot

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Kim Phillips <kim.phillips@arm.com>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/Kconfig          |  5 ++++-
 drivers/hwtracing/coresight/coresight-tpiu.c | 19 ++++++++++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 996d84a1edb8..8fd9887fb03b 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -46,7 +46,7 @@ config CORESIGHT_CATU
 	   mode where the address is not translated.
 
 config CORESIGHT_SINK_TPIU
-	bool "Coresight generic TPIU driver"
+	tristate "Coresight generic TPIU driver"
 	depends on CORESIGHT_LINKS_AND_SINKS
 	help
 	  This enables support for the Trace Port Interface Unit driver,
@@ -56,6 +56,9 @@ config CORESIGHT_SINK_TPIU
 	  connected to an external host for use case capturing more traces than
 	  the on-board coresight memory can handle.
 
+	  To compile this driver as a module, choose M here: the
+	  module will be called coresight-tpiu.
+
 config CORESIGHT_SINK_ETBV10
 	tristate "Coresight ETBv1.0 driver"
 	depends on CORESIGHT_LINKS_AND_SINKS
diff --git a/drivers/hwtracing/coresight/coresight-tpiu.c b/drivers/hwtracing/coresight/coresight-tpiu.c
index f8583e4032a6..e4ddd9801535 100644
--- a/drivers/hwtracing/coresight/coresight-tpiu.c
+++ b/drivers/hwtracing/coresight/coresight-tpiu.c
@@ -173,6 +173,15 @@ static int tpiu_probe(struct amba_device *adev, const struct amba_id *id)
 	return PTR_ERR(drvdata->csdev);
 }
 
+static int __exit tpiu_remove(struct amba_device *adev)
+{
+	struct tpiu_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+	coresight_unregister(drvdata->csdev);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM
 static int tpiu_runtime_suspend(struct device *dev)
 {
@@ -216,6 +225,8 @@ static const struct amba_id tpiu_ids[] = {
 	{ 0, 0},
 };
 
+MODULE_DEVICE_TABLE(amba, tpiu_ids);
+
 static struct amba_driver tpiu_driver = {
 	.drv = {
 		.name	= "coresight-tpiu",
@@ -224,6 +235,12 @@ static struct amba_driver tpiu_driver = {
 		.suppress_bind_attrs = true,
 	},
 	.probe		= tpiu_probe,
+	.remove         = tpiu_remove,
 	.id_table	= tpiu_ids,
 };
-builtin_amba_driver(tpiu_driver);
+module_amba_driver(tpiu_driver);
+
+MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>");
+MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>");
+MODULE_DESCRIPTION("Arm CoreSight TPIU (Trace Port Interface Unit) driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 12/20] coresight: allow tmc to be built as a module

[Tingwei Zhang]

From: Kim Phillips <kim.phillips@arm.com>

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

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Kim Phillips <kim.phillips@arm.com>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/Kconfig           |  6 +++++-
 drivers/hwtracing/coresight/Makefile          |  6 +++---
 .../{coresight-tmc.c => coresight-tmc-core.c} | 19 ++++++++++++++++++-
 3 files changed, 26 insertions(+), 5 deletions(-)
 rename drivers/hwtracing/coresight/{coresight-tmc.c => coresight-tmc-core.c} (96%)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index 8fd9887fb03b..fc48ae086746 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -25,7 +25,8 @@ config CORESIGHT_LINKS_AND_SINKS
 	  entity at run time to form a complete trace path.
 
 config CORESIGHT_LINK_AND_SINK_TMC
-	bool "Coresight generic TMC driver"
+	tristate "Coresight generic TMC driver"
+
 	depends on CORESIGHT_LINKS_AND_SINKS
 	help
 	  This enables support for the Trace Memory Controller driver.
@@ -34,6 +35,9 @@ config CORESIGHT_LINK_AND_SINK_TMC
 	  complies with the generic implementation of the component without
 	  special enhancement or added features.
 
+	  To compile this driver as a module, choose M here: the
+	  module will be called coresight-tmc.
+
 config CORESIGHT_CATU
 	bool "Coresight Address Translation Unit (CATU) driver"
 	depends on CORESIGHT_LINK_AND_SINK_TMC
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 271dc255454f..f2a568b969c4 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -4,9 +4,9 @@
 #
 obj-$(CONFIG_CORESIGHT) += coresight.o coresight-etm-perf.o \
 			   coresight-platform.o coresight-sysfs.o
-obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o \
-					     coresight-tmc-etf.o \
-					     coresight-tmc-etr.o
+obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
+coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \
+		      coresight-tmc-etr.o
 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 \
diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
similarity index 96%
rename from drivers/hwtracing/coresight/coresight-tmc.c
rename to drivers/hwtracing/coresight/coresight-tmc-core.c
index 7040d583bed9..29d191ba7ccf 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
@@ -561,6 +561,16 @@ static void tmc_shutdown(struct amba_device *adev)
 	spin_unlock_irqrestore(&drvdata->spinlock, flags);
 }
 
+static int __exit tmc_remove(struct amba_device *adev)
+{
+	struct tmc_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+	misc_deregister(&drvdata->miscdev);
+	coresight_unregister(drvdata->csdev);
+
+	return 0;
+}
+
 static const struct amba_id tmc_ids[] = {
 	CS_AMBA_ID(0x000bb961),
 	/* Coresight SoC 600 TMC-ETR/ETS */
@@ -572,6 +582,8 @@ static const struct amba_id tmc_ids[] = {
 	{ 0, 0},
 };
 
+MODULE_DEVICE_TABLE(amba, tmc_ids);
+
 static struct amba_driver tmc_driver = {
 	.drv = {
 		.name   = "coresight-tmc",
@@ -580,6 +592,11 @@ static struct amba_driver tmc_driver = {
 	},
 	.probe		= tmc_probe,
 	.shutdown	= tmc_shutdown,
+	.remove		= tmc_remove,
 	.id_table	= tmc_ids,
 };
-builtin_amba_driver(tmc_driver);
+module_amba_driver(tmc_driver);
+
+MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>");
+MODULE_DESCRIPTION("Arm CoreSight Trace Memory Controller driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 13/20] coresight: remove multiple init calls from funnel driver

[Tingwei Zhang]

From: Mian Yousaf Kaukab <ykaukab@suse.de>

Dynamic-funnel uses module_amba_driver to register. Whereas
static-funnel uses builtin_platform_driver. Combine these init calls
into a single module_init/exit pair in preparation to make the driver
modular.

Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 .../hwtracing/coresight/coresight-funnel.c    | 30 +++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
index 900690a9f7f0..46b277ed8606 100644
--- a/drivers/hwtracing/coresight/coresight-funnel.c
+++ b/drivers/hwtracing/coresight/coresight-funnel.c
@@ -341,7 +341,6 @@ static struct platform_driver static_funnel_driver = {
 		.suppress_bind_attrs = true,
 	},
 };
-builtin_platform_driver(static_funnel_driver);
 
 static int dynamic_funnel_probe(struct amba_device *adev,
 				const struct amba_id *id)
@@ -372,4 +371,31 @@ static struct amba_driver dynamic_funnel_driver = {
 	.probe		= dynamic_funnel_probe,
 	.id_table	= dynamic_funnel_ids,
 };
-builtin_amba_driver(dynamic_funnel_driver);
+
+static int __init funnel_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&static_funnel_driver);
+	if (ret) {
+		pr_info("Error registering platform driver\n");
+		return ret;
+	}
+
+	ret = amba_driver_register(&dynamic_funnel_driver);
+	if (ret) {
+		pr_info("Error registering amba driver\n");
+		platform_driver_unregister(&static_funnel_driver);
+	}
+
+	return ret;
+}
+
+static void __exit funnel_exit(void)
+{
+	platform_driver_unregister(&static_funnel_driver);
+	amba_driver_unregister(&dynamic_funnel_driver);
+}
+
+module_init(funnel_init);
+module_exit(funnel_exit);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 14/20] coresight: remove multiple init calls from replicator driver

[Tingwei Zhang]

From: Mian Yousaf Kaukab <ykaukab@suse.de>

Dynamic-replicator uses module_amba_driver to register. Whereas
static-replicator uses builtin_platform_driver. Combine these init
calls into a single module_init/exit pair in preparation to make the
driver modular.

Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 .../coresight/coresight-replicator.c          | 30 +++++++++++++++++--
 1 file changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
index 78acf29c49ca..6b30bcee7458 100644
--- a/drivers/hwtracing/coresight/coresight-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-replicator.c
@@ -360,7 +360,6 @@ static struct platform_driver static_replicator_driver = {
 		.suppress_bind_attrs = true,
 	},
 };
-builtin_platform_driver(static_replicator_driver);
 
 static int dynamic_replicator_probe(struct amba_device *adev,
 				    const struct amba_id *id)
@@ -383,4 +382,31 @@ static struct amba_driver dynamic_replicator_driver = {
 	.probe		= dynamic_replicator_probe,
 	.id_table	= dynamic_replicator_ids,
 };
-builtin_amba_driver(dynamic_replicator_driver);
+
+static int __init replicator_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&static_replicator_driver);
+	if (ret) {
+		pr_info("Error registering platform driver\n");
+		return ret;
+	}
+
+	ret = amba_driver_register(&dynamic_replicator_driver);
+	if (ret) {
+		pr_info("Error registering amba driver\n");
+		platform_driver_unregister(&static_replicator_driver);
+	}
+
+	return ret;
+}
+
+static void __exit replicator_exit(void)
+{
+	platform_driver_unregister(&static_replicator_driver);
+	amba_driver_unregister(&dynamic_replicator_driver);
+}
+
+module_init(replicator_init);
+module_exit(replicator_exit);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 15/20] coresight: allow funnel and replicator drivers to be built as modules

[Tingwei Zhang]

From: Kim Phillips <kim.phillips@arm.com>

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

- Kconfig becomes a tristate, to allow =m
- add funnel_remove and replicator_remove functions,
  for module unload
- add a MODULE_DEVICE_TABLE for autoloading on boot

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Kim Phillips <kim.phillips@arm.com>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/Kconfig           |  5 ++-
 .../hwtracing/coresight/coresight-funnel.c    | 32 ++++++++++++++++++
 .../coresight/coresight-replicator.c          | 33 +++++++++++++++++++
 3 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index fc48ae086746..f31778dd0b5d 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -17,13 +17,16 @@ menuconfig CORESIGHT
 
 if CORESIGHT
 config CORESIGHT_LINKS_AND_SINKS
-	bool "CoreSight Link and Sink drivers"
+	tristate "CoreSight Link and Sink drivers"
 	help
 	  This enables support for CoreSight link and sink drivers that are
 	  responsible for transporting and collecting the trace data
 	  respectively.  Link and sinks are dynamically aggregated with a trace
 	  entity at run time to form a complete trace path.
 
+	  To compile these drivers as modules, choose M here: the
+	  modules will be called coresight-funnel and coresight-replicator.
+
 config CORESIGHT_LINK_AND_SINK_TMC
 	tristate "Coresight generic TMC driver"
 
diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
index 46b277ed8606..062694ef9879 100644
--- a/drivers/hwtracing/coresight/coresight-funnel.c
+++ b/drivers/hwtracing/coresight/coresight-funnel.c
@@ -274,6 +274,15 @@ static int funnel_probe(struct device *dev, struct resource *res)
 	return ret;
 }
 
+static int __exit funnel_remove(struct device *dev)
+{
+	struct funnel_drvdata *drvdata = dev_get_drvdata(dev);
+
+	coresight_unregister(drvdata->csdev);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM
 static int funnel_runtime_suspend(struct device *dev)
 {
@@ -319,20 +328,31 @@ static int static_funnel_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int __exit static_funnel_remove(struct platform_device *pdev)
+{
+	funnel_remove(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	return 0;
+}
+
 static const struct of_device_id static_funnel_match[] = {
 	{.compatible = "arm,coresight-static-funnel"},
 	{}
 };
 
+MODULE_DEVICE_TABLE(of, static_funnel_match);
+
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id static_funnel_ids[] = {
 	{"ARMHC9FE", 0},
 	{},
 };
+MODULE_DEVICE_TABLE(acpi, static_funnel_ids);
 #endif
 
 static struct platform_driver static_funnel_driver = {
 	.probe          = static_funnel_probe,
+	.probe          = static_funnel_remove,
 	.driver         = {
 		.name   = "coresight-static-funnel",
 		.of_match_table = static_funnel_match,
@@ -348,6 +368,11 @@ static int dynamic_funnel_probe(struct amba_device *adev,
 	return funnel_probe(&adev->dev, &adev->res);
 }
 
+static int __exit dynamic_funnel_remove(struct amba_device *adev)
+{
+	return funnel_remove(&adev->dev);
+}
+
 static const struct amba_id dynamic_funnel_ids[] = {
 	{
 		.id     = 0x000bb908,
@@ -361,6 +386,8 @@ static const struct amba_id dynamic_funnel_ids[] = {
 	{ 0, 0},
 };
 
+MODULE_DEVICE_TABLE(amba, dynamic_funnel_ids);
+
 static struct amba_driver dynamic_funnel_driver = {
 	.drv = {
 		.name	= "coresight-dynamic-funnel",
@@ -369,6 +396,7 @@ static struct amba_driver dynamic_funnel_driver = {
 		.suppress_bind_attrs = true,
 	},
 	.probe		= dynamic_funnel_probe,
+	.remove		= dynamic_funnel_remove,
 	.id_table	= dynamic_funnel_ids,
 };
 
@@ -399,3 +427,7 @@ static void __exit funnel_exit(void)
 
 module_init(funnel_init);
 module_exit(funnel_exit);
+
+MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>");
+MODULE_DESCRIPTION("Arm CoreSight Funnel Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
index 6b30bcee7458..05db434c79d0 100644
--- a/drivers/hwtracing/coresight/coresight-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-replicator.c
@@ -291,6 +291,14 @@ static int replicator_probe(struct device *dev, struct resource *res)
 	return ret;
 }
 
+static int __exit replicator_remove(struct device *dev)
+{
+	struct replicator_drvdata *drvdata = dev_get_drvdata(dev);
+
+	coresight_unregister(drvdata->csdev);
+	return 0;
+}
+
 static int static_replicator_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -310,6 +318,13 @@ static int static_replicator_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static int __exit static_replicator_remove(struct platform_device *pdev)
+{
+	replicator_remove(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	return 0;
+}
+
 #ifdef CONFIG_PM
 static int replicator_runtime_suspend(struct device *dev)
 {
@@ -342,18 +357,22 @@ static const struct of_device_id static_replicator_match[] = {
 	{.compatible = "arm,coresight-static-replicator"},
 	{}
 };
+MODULE_DEVICE_TABLE(of, static_replicator_match);
 
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id static_replicator_acpi_ids[] = {
 	{"ARMHC985", 0}, /* ARM CoreSight Static Replicator */
 	{}
 };
+MODULE_DEVICE_TABLE(acpi, static_replicator_acpi_ids);
 #endif
 
 static struct platform_driver static_replicator_driver = {
 	.probe          = static_replicator_probe,
+	.remove         = static_replicator_remove,
 	.driver         = {
 		.name   = "coresight-static-replicator",
+		.owner	= THIS_MODULE,
 		.of_match_table = of_match_ptr(static_replicator_match),
 		.acpi_match_table = ACPI_PTR(static_replicator_acpi_ids),
 		.pm	= &replicator_dev_pm_ops,
@@ -367,19 +386,28 @@ static int dynamic_replicator_probe(struct amba_device *adev,
 	return replicator_probe(&adev->dev, &adev->res);
 }
 
+static int __exit dynamic_replicator_remove(struct amba_device *adev)
+{
+	return replicator_remove(&adev->dev);
+}
+
 static const struct amba_id dynamic_replicator_ids[] = {
 	CS_AMBA_ID(0x000bb909),
 	CS_AMBA_ID(0x000bb9ec),		/* Coresight SoC-600 */
 	{},
 };
 
+MODULE_DEVICE_TABLE(amba, dynamic_replicator_ids);
+
 static struct amba_driver dynamic_replicator_driver = {
 	.drv = {
 		.name	= "coresight-dynamic-replicator",
 		.pm	= &replicator_dev_pm_ops,
+		.owner	= THIS_MODULE,
 		.suppress_bind_attrs = true,
 	},
 	.probe		= dynamic_replicator_probe,
+	.remove         = dynamic_replicator_remove,
 	.id_table	= dynamic_replicator_ids,
 };
 
@@ -410,3 +438,8 @@ static void __exit replicator_exit(void)
 
 module_init(replicator_init);
 module_exit(replicator_exit);
+
+MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora.org>");
+MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>");
+MODULE_DESCRIPTION("Arm CoreSight Replicator Driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 16/20] coresight: cti: add function to register cti associate ops

[Tingwei Zhang]

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

Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/coresight-cti.c  | 32 ++++++++++++++++++--
 drivers/hwtracing/coresight/coresight-priv.h | 14 ++++-----
 drivers/hwtracing/coresight/coresight.c      | 21 +++++++++++--
 3 files changed, 53 insertions(+), 14 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti.c
index 3ccc703dc940..1f470c47ba70 100644
--- a/drivers/hwtracing/coresight/coresight-cti.c
+++ b/drivers/hwtracing/coresight/coresight-cti.c
@@ -589,7 +589,6 @@ void cti_add_assoc_to_csdev(struct coresight_device *csdev)
 cti_add_done:
 	mutex_unlock(&ect_mutex);
 }
-EXPORT_SYMBOL_GPL(cti_add_assoc_to_csdev);
 
 /*
  * Removing the associated devices is easier.
@@ -616,7 +615,15 @@ void cti_remove_assoc_from_csdev(struct coresight_device *csdev)
 	}
 	mutex_unlock(&ect_mutex);
 }
-EXPORT_SYMBOL_GPL(cti_remove_assoc_from_csdev);
+
+/*
+ * Operations to add and remove associated CTI.
+ * Register to coresight core driver as call back function.
+ */
+static struct cti_assoc_op cti_assoc_ops = {
+	.add = cti_add_assoc_to_csdev,
+	.remove = cti_remove_assoc_from_csdev
+};
 
 /*
  * Update the cross references where the associated device was found
@@ -972,4 +979,23 @@ static struct amba_driver cti_driver = {
 	.probe		= cti_probe,
 	.id_table	= cti_ids,
 };
-builtin_amba_driver(cti_driver);
+
+static int __init cti_init(void)
+{
+	int ret;
+
+	ret = amba_driver_register(&cti_driver);
+	if (ret)
+		pr_info("Error registering cti driver\n");
+	coresight_set_cti_ops(&cti_assoc_ops);
+	return ret;
+}
+
+static void __exit cti_exit(void)
+{
+	coresight_remove_cti_ops();
+	amba_driver_unregister(&cti_driver);
+}
+
+module_init(cti_init);
+module_exit(cti_exit);
diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
index dcb8aeb6af62..6cde6cf42554 100644
--- a/drivers/hwtracing/coresight/coresight-priv.h
+++ b/drivers/hwtracing/coresight/coresight-priv.h
@@ -173,15 +173,13 @@ static inline int etm_readl_cp14(u32 off, unsigned int *val) { return 0; }
 static inline int etm_writel_cp14(u32 off, u32 val) { return 0; }
 #endif
 
-#ifdef CONFIG_CORESIGHT_CTI
-extern void cti_add_assoc_to_csdev(struct coresight_device *csdev);
-extern void cti_remove_assoc_from_csdev(struct coresight_device *csdev);
+struct cti_assoc_op {
+	void (*add)(struct coresight_device *csdev);
+	void (*remove)(struct coresight_device *csdev);
+};
 
-#else
-static inline void cti_add_assoc_to_csdev(struct coresight_device *csdev) {}
-static inline void
-cti_remove_assoc_from_csdev(struct coresight_device *csdev) {}
-#endif
+extern void coresight_set_cti_ops(const struct cti_assoc_op *cti_op);
+extern void coresight_remove_cti_ops(void);
 
 /*
  * Macros and inline functions to handle CoreSight UCI data and driver
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 17bc76ea86ae..5a5b81b5d0df 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -57,6 +57,20 @@ const u32 coresight_barrier_pkt[4] = {
 		0x7fffffff, 0x7fffffff, 0x7fffffff, 0x7fffffff};
 EXPORT_SYMBOL_GPL(coresight_barrier_pkt);
 
+static const struct cti_assoc_op *cti_assoc_ops;
+
+void coresight_set_cti_ops(const struct cti_assoc_op *cti_op)
+{
+	cti_assoc_ops = cti_op;
+}
+EXPORT_SYMBOL_GPL(coresight_set_cti_ops);
+
+void coresight_remove_cti_ops(void)
+{
+	cti_assoc_ops = NULL;
+}
+EXPORT_SYMBOL_GPL(coresight_remove_cti_ops);
+
 static int coresight_id_match(struct device *dev, void *data)
 {
 	int trace_id, i_trace_id;
@@ -1214,7 +1228,8 @@ static void coresight_device_release(struct device *dev)
 {
 	struct coresight_device *csdev = to_coresight_device(dev);
 
-	cti_remove_assoc_from_csdev(csdev);
+	if (cti_assoc_ops && cti_assoc_ops->remove)
+		cti_assoc_ops->remove(csdev);
 	fwnode_handle_put(csdev->dev.fwnode);
 	kfree(csdev->refcnt);
 	kfree(csdev);
@@ -1525,8 +1540,8 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 		ret = coresight_fixup_device_conns(csdev);
 	if (!ret)
 		ret = coresight_fixup_orphan_conns(csdev);
-	if (!ret)
-		cti_add_assoc_to_csdev(csdev);
+	if (!ret && cti_assoc_ops && cti_assoc_ops->add)
+		cti_assoc_ops->add(csdev);
 
 	mutex_unlock(&coresight_mutex);
 	if (ret) {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 17/20] coresight: allow cti to be built as a module

[Tingwei Zhang]

Allow to build coresight-cti as a module, for ease of development.

- Kconfig becomes a tristate, to allow =m
- append -core to source file name to allow module to
  be called coresight-cti by the Makefile
- add an cti_remove function, for module unload
- move cti_remove_conn_xrefs to cti_remove
- add a MODULE_DEVICE_TABLE for autoloading on boot

Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/Kconfig           |  5 ++++-
 drivers/hwtracing/coresight/Makefile          |  4 ++--
 .../{coresight-cti.c => coresight-cti-core.c} | 19 ++++++++++++++++++-
 .../hwtracing/coresight/coresight-platform.c  |  1 +
 drivers/hwtracing/coresight/coresight.c       |  1 +
 5 files changed, 26 insertions(+), 4 deletions(-)
 rename drivers/hwtracing/coresight/{coresight-cti.c => coresight-cti-core.c} (98%)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index f31778dd0b5d..b04aae2ceecc 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -136,7 +136,7 @@ config CORESIGHT_CPU_DEBUG
 	  module will be called coresight-cpu-debug.
 
 config CORESIGHT_CTI
-	bool "CoreSight Cross Trigger Interface (CTI) driver"
+	tristate "CoreSight Cross Trigger Interface (CTI) driver"
 	depends on ARM || ARM64
 	help
 	  This driver provides support for CoreSight CTI and CTM components.
@@ -147,6 +147,9 @@ config CORESIGHT_CTI
 	  halt compared to disabling sources and sinks normally in driver
 	  software.
 
+	  To compile this driver as a module, choose M here: the
+	  module will be called coresight-cti.
+
 config CORESIGHT_CTI_INTEGRATION_REGS
 	bool "Access CTI CoreSight Integration Registers"
 	depends on CORESIGHT_CTI
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index f2a568b969c4..0359d5a1588f 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -19,6 +19,6 @@ coresight-etm4x-y := coresight-etm4x-core.o coresight-etm4x-sysfs.o
 obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o
 obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o
 obj-$(CONFIG_CORESIGHT_CATU) += coresight-catu.o
-obj-$(CONFIG_CORESIGHT_CTI) += coresight-cti.o \
-				coresight-cti-platform.o \
+obj-$(CONFIG_CORESIGHT_CTI) += coresight-cti.o
+coresight-cti-y := coresight-cti-core.o	coresight-cti-platform.o \
 				coresight-cti-sysfs.o
diff --git a/drivers/hwtracing/coresight/coresight-cti.c b/drivers/hwtracing/coresight/coresight-cti-core.c
similarity index 98%
rename from drivers/hwtracing/coresight/coresight-cti.c
rename to drivers/hwtracing/coresight/coresight-cti-core.c
index 1f470c47ba70..612b0c067a3e 100644
--- a/drivers/hwtracing/coresight/coresight-cti.c
+++ b/drivers/hwtracing/coresight/coresight-cti-core.c
@@ -835,7 +835,6 @@ static void cti_device_release(struct device *dev)
 	struct cti_drvdata *ect_item, *ect_tmp;
 
 	mutex_lock(&ect_mutex);
-	cti_remove_conn_xrefs(drvdata);
 	cti_pm_release(drvdata);
 
 	/* remove from the list */
@@ -850,6 +849,18 @@ static void cti_device_release(struct device *dev)
 	if (drvdata->csdev_release)
 		drvdata->csdev_release(dev);
 }
+static int __exit cti_remove(struct amba_device *adev)
+{
+	struct cti_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+	mutex_lock(&ect_mutex);
+	cti_remove_conn_xrefs(drvdata);
+	mutex_unlock(&ect_mutex);
+
+	coresight_unregister(drvdata->csdev);
+
+	return 0;
+}
 
 static int cti_probe(struct amba_device *adev, const struct amba_id *id)
 {
@@ -969,6 +980,7 @@ static const struct amba_id cti_ids[] = {
 	CS_AMBA_UCI_ID(0x000bb9ed, uci_id_cti), /* Coresight CTI (SoC 600) */
 	{ 0, 0},
 };
+MODULE_DEVICE_TABLE(amba, cti_ids);
 
 static struct amba_driver cti_driver = {
 	.drv = {
@@ -977,6 +989,7 @@ static struct amba_driver cti_driver = {
 		.suppress_bind_attrs = true,
 	},
 	.probe		= cti_probe,
+	.remove		= cti_remove,
 	.id_table	= cti_ids,
 };
 
@@ -999,3 +1012,7 @@ static void __exit cti_exit(void)
 
 module_init(cti_init);
 module_exit(cti_exit);
+
+MODULE_AUTHOR("Mike Leach <mike.leach@linaro.org>");
+MODULE_DESCRIPTION("Arm CoreSight CTI Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
index bfd44231d7ad..635d55c1d580 100644
--- a/drivers/hwtracing/coresight/coresight-platform.c
+++ b/drivers/hwtracing/coresight/coresight-platform.c
@@ -75,6 +75,7 @@ coresight_find_csdev_by_fwnode(struct fwnode_handle *r_fwnode)
 	}
 	return csdev;
 }
+EXPORT_SYMBOL_GPL(coresight_find_csdev_by_fwnode);
 
 #ifdef CONFIG_OF
 static inline bool of_coresight_legacy_ep_is_input(struct device_node *ep)
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 5a5b81b5d0df..16329f16ac2b 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -273,6 +273,7 @@ void coresight_set_assoc_ectdev_mutex(struct coresight_device *csdev,
 	csdev->ect_dev = ect_csdev;
 	mutex_unlock(&coresight_mutex);
 }
+EXPORT_SYMBOL_GPL(coresight_set_assoc_ectdev_mutex);
 
 static int coresight_enable_sink(struct coresight_device *csdev,
 				 u32 mode, void *data)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 18/20] coresight: tmc-etr: add function to register catu ops

[Tingwei Zhang]

From: Mian Yousaf Kaukab <ykaukab@suse.de>

Make etr_catu_buf_ops static. Instead of directly accessing it in
etr_buf_ops[], add a function to let catu driver register the ops at
runtime. Break circular dependency between tmc-etr and catu drivers.

Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse.de>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/coresight-catu.c  | 22 +++++++++++++++++--
 drivers/hwtracing/coresight/coresight-catu.h  |  2 --
 .../hwtracing/coresight/coresight-tmc-etr.c   | 15 +++++++++++--
 drivers/hwtracing/coresight/coresight-tmc.h   |  3 +++
 4 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
index 1801804a7762..47696a7d24a7 100644
--- a/drivers/hwtracing/coresight/coresight-catu.c
+++ b/drivers/hwtracing/coresight/coresight-catu.c
@@ -358,7 +358,7 @@ static int catu_alloc_etr_buf(struct tmc_drvdata *tmc_drvdata,
 	return 0;
 }
 
-const struct etr_buf_operations etr_catu_buf_ops = {
+static const struct etr_buf_operations etr_catu_buf_ops = {
 	.alloc = catu_alloc_etr_buf,
 	.free = catu_free_etr_buf,
 	.sync = catu_sync_etr_buf,
@@ -582,4 +582,22 @@ static struct amba_driver catu_driver = {
 	.id_table			= catu_ids,
 };
 
-builtin_amba_driver(catu_driver);
+static int __init catu_init(void)
+{
+	int ret;
+
+	ret = amba_driver_register(&catu_driver);
+	if (ret)
+		pr_info("Error registering catu driver\n");
+	tmc_etr_set_catu_ops(&etr_catu_buf_ops);
+	return ret;
+}
+
+static void __exit catu_exit(void)
+{
+	tmc_etr_remove_catu_ops();
+	amba_driver_unregister(&catu_driver);
+}
+
+module_init(catu_init);
+module_exit(catu_exit);
diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h
index 80ceee3c739c..6160c2d75a56 100644
--- a/drivers/hwtracing/coresight/coresight-catu.h
+++ b/drivers/hwtracing/coresight/coresight-catu.h
@@ -108,6 +108,4 @@ static inline bool coresight_is_catu_device(struct coresight_device *csdev)
 	return true;
 }
 
-extern const struct etr_buf_operations etr_catu_buf_ops;
-
 #endif
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index ad991a37e2d2..714f9e867e5f 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -794,10 +794,21 @@ static inline void tmc_etr_disable_catu(struct tmc_drvdata *drvdata)
 static const struct etr_buf_operations *etr_buf_ops[] = {
 	[ETR_MODE_FLAT] = &etr_flat_buf_ops,
 	[ETR_MODE_ETR_SG] = &etr_sg_buf_ops,
-	[ETR_MODE_CATU] = IS_ENABLED(CONFIG_CORESIGHT_CATU)
-						? &etr_catu_buf_ops : NULL,
+	[ETR_MODE_CATU] = NULL,
 };
 
+void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu)
+{
+	etr_buf_ops[ETR_MODE_CATU] = catu;
+}
+EXPORT_SYMBOL_GPL(tmc_etr_set_catu_ops);
+
+void tmc_etr_remove_catu_ops(void)
+{
+	etr_buf_ops[ETR_MODE_CATU] = NULL;
+}
+EXPORT_SYMBOL_GPL(tmc_etr_remove_catu_ops);
+
 static inline int tmc_etr_mode_alloc_buf(int mode,
 					 struct tmc_drvdata *drvdata,
 					 struct etr_buf *etr_buf, int node,
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 6e8d2dc33d17..b91ec7dde7bc 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -326,4 +326,7 @@ tmc_sg_table_buf_size(struct tmc_sg_table *sg_table)
 
 struct coresight_device *tmc_etr_get_catu_device(struct tmc_drvdata *drvdata);
 
+void tmc_etr_set_catu_ops(const struct etr_buf_operations *catu);
+void tmc_etr_remove_catu_ops(void);
+
 #endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 19/20] coresight: allow catu drivers to be built as modules

[Tingwei Zhang]

Allow to build coresight-catu as modules, for ease of development.
- Kconfig becomes a tristate, to allow =m
- add catu_remove functions, for module unload
- add a MODULE_DEVICE_TABLE for autoloading on boot

Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/Kconfig          |  5 ++++-
 drivers/hwtracing/coresight/coresight-catu.c | 15 +++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index b04aae2ceecc..dfe407cde262 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -42,7 +42,7 @@ config CORESIGHT_LINK_AND_SINK_TMC
 	  module will be called coresight-tmc.
 
 config CORESIGHT_CATU
-	bool "Coresight Address Translation Unit (CATU) driver"
+	tristate "Coresight Address Translation Unit (CATU) driver"
 	depends on CORESIGHT_LINK_AND_SINK_TMC
 	help
 	   Enable support for the Coresight Address Translation Unit (CATU).
@@ -52,6 +52,9 @@ config CORESIGHT_CATU
 	   by looking up the provided table. CATU can also be used in pass-through
 	   mode where the address is not translated.
 
+	   To compile this driver as a module, choose M here: the
+	   module will be called coresight-catu.
+
 config CORESIGHT_SINK_TPIU
 	tristate "Coresight generic TPIU driver"
 	depends on CORESIGHT_LINKS_AND_SINKS
diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
index 47696a7d24a7..7428cbe67921 100644
--- a/drivers/hwtracing/coresight/coresight-catu.c
+++ b/drivers/hwtracing/coresight/coresight-catu.c
@@ -567,11 +567,21 @@ static int catu_probe(struct amba_device *adev, const struct amba_id *id)
 	return ret;
 }
 
+static int __exit catu_remove(struct amba_device *adev)
+{
+	struct catu_drvdata *drvdata = dev_get_drvdata(&adev->dev);
+
+	coresight_unregister(drvdata->csdev);
+	return 0;
+}
+
 static struct amba_id catu_ids[] = {
 	CS_AMBA_ID(0x000bb9ee),
 	{},
 };
 
+MODULE_DEVICE_TABLE(amba, catu_ids);
+
 static struct amba_driver catu_driver = {
 	.drv = {
 		.name			= "coresight-catu",
@@ -579,6 +589,7 @@ static struct amba_driver catu_driver = {
 		.suppress_bind_attrs	= true,
 	},
 	.probe				= catu_probe,
+	.remove				= catu_remove,
 	.id_table			= catu_ids,
 };
 
@@ -601,3 +612,7 @@ static void __exit catu_exit(void)
 
 module_init(catu_init);
 module_exit(catu_exit);
+
+MODULE_AUTHOR("Suzuki K Poulose <suzuki.poulose@arm.com>");
+MODULE_DESCRIPTION("Arm CoreSight Replicator Driver");
+MODULE_LICENSE("GPL v2");
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

[PATCH v4 20/20] coresight: allow the coresight core driver to be built as a module

[Tingwei Zhang]

Enhance coresight developer's efficiency to debug coresight drivers.
- Kconfig becomes a tristate, to allow =m
- append -core to source file name to allow module to
  be called coresight by the Makefile
- modules can have only one init/exit, so we add the etm_perf
  register/unregister function calls to the core init/exit
  functions.
- add a MODULE_DEVICE_TABLE for autoloading on boot

Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Leo Yan <leo.yan@linaro.org>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Randy Dunlap <rdunlap@infradead.org>
Cc: Suzuki K Poulose <Suzuki.Poulose@arm.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Russell King <linux@armlinux.org.uk>
Signed-off-by: Kim Phillips <kim.phillips@arm.com>
Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
---
 drivers/hwtracing/coresight/Kconfig           |  5 ++-
 drivers/hwtracing/coresight/Makefile          |  5 ++-
 .../{coresight.c => coresight-core.c}         | 42 ++++++++++++++-----
 .../hwtracing/coresight/coresight-etm-perf.c  |  8 +++-
 .../hwtracing/coresight/coresight-etm-perf.h  |  3 ++
 5 files changed, 48 insertions(+), 15 deletions(-)
 rename drivers/hwtracing/coresight/{coresight.c => coresight-core.c} (98%)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index dfe407cde262..c1198245461d 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -3,7 +3,7 @@
 # Coresight configuration
 #
 menuconfig CORESIGHT
-	bool "CoreSight Tracing Support"
+	tristate "CoreSight Tracing Support"
 	depends on ARM || ARM64
 	depends on OF || ACPI
 	select ARM_AMBA
@@ -15,6 +15,9 @@ menuconfig CORESIGHT
 	  specification and configure the right series of components when a
 	  trace source gets enabled.
 
+	  To compile this driver as a module, choose M here: the
+	  module will be called coresight.
+
 if CORESIGHT
 config CORESIGHT_LINKS_AND_SINKS
 	tristate "CoreSight Link and Sink drivers"
diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile
index 0359d5a1588f..1b35b55bd420 100644
--- a/drivers/hwtracing/coresight/Makefile
+++ b/drivers/hwtracing/coresight/Makefile
@@ -2,8 +2,9 @@
 #
 # Makefile for CoreSight drivers.
 #
-obj-$(CONFIG_CORESIGHT) += coresight.o coresight-etm-perf.o \
-			   coresight-platform.o coresight-sysfs.o
+obj-$(CONFIG_CORESIGHT) += coresight.o
+coresight-y := coresight-core.o  coresight-etm-perf.o coresight-platform.o \
+		coresight-sysfs.o
 obj-$(CONFIG_CORESIGHT_LINK_AND_SINK_TMC) += coresight-tmc.o
 coresight-tmc-y := coresight-tmc-core.o coresight-tmc-etf.o \
 		      coresight-tmc-etr.o
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight-core.c
similarity index 98%
rename from drivers/hwtracing/coresight/coresight.c
rename to drivers/hwtracing/coresight/coresight-core.c
index 16329f16ac2b..9d744b5e82d1 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1420,16 +1420,6 @@ int coresight_timeout(void __iomem *addr, u32 offset, int position, int value)
 }
 EXPORT_SYMBOL_GPL(coresight_timeout);
 
-struct bus_type coresight_bustype = {
-	.name	= "coresight",
-};
-
-static int __init coresight_init(void)
-{
-	return bus_register(&coresight_bustype);
-}
-postcore_initcall(coresight_init);
-
 /*
  * coresight_release_platform_data: Release references to the devices connected
  * to the output port of this device.
@@ -1636,3 +1626,35 @@ char *coresight_alloc_device_name(struct coresight_dev_list *dict,
 	return name;
 }
 EXPORT_SYMBOL_GPL(coresight_alloc_device_name);
+
+struct bus_type coresight_bustype = {
+	.name	= "coresight",
+};
+
+static int __init coresight_init(void)
+{
+	int ret;
+
+	ret = bus_register(&coresight_bustype);
+	if (ret)
+		return ret;
+
+	ret = etm_perf_init();
+	if (ret)
+		bus_unregister(&coresight_bustype);
+
+	return ret;
+}
+
+static void __exit coresight_exit(void)
+{
+	etm_perf_exit();
+	bus_unregister(&coresight_bustype);
+}
+
+module_init(coresight_init);
+module_exit(coresight_exit);
+
+MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro.org>");
+MODULE_DESCRIPTION("Arm CoreSight tracer driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
index dcb0592418ae..dc4bb0906492 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.c
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
@@ -591,7 +591,7 @@ void etm_perf_del_symlink_sink(struct coresight_device *csdev)
 	csdev->ea = NULL;
 }
 
-static int __init etm_perf_init(void)
+int __init etm_perf_init(void)
 {
 	int ret;
 
@@ -618,4 +618,8 @@ static int __init etm_perf_init(void)
 
 	return ret;
 }
-device_initcall(etm_perf_init);
+
+void __exit etm_perf_exit(void)
+{
+	perf_pmu_unregister(&etm_pmu);
+}
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.h b/drivers/hwtracing/coresight/coresight-etm-perf.h
index 05f89723e282..3e4f2ad5e193 100644
--- a/drivers/hwtracing/coresight/coresight-etm-perf.h
+++ b/drivers/hwtracing/coresight/coresight-etm-perf.h
@@ -82,4 +82,7 @@ static inline void *etm_perf_sink_config(struct perf_output_handle *handle)
 
 #endif /* CONFIG_CORESIGHT */
 
+int __init etm_perf_init(void);
+void __exit etm_perf_exit(void);
+
 #endif
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Mike Leach]

Hi Tingwei,

On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang <tingwei@codeaurora.org> wrote:
>
> When coresight device is in an active session, driver module of
> that device should not be removed. Use try_get_module() in
> coresight_grab_device() to prevent module to be unloaded.
>
> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> ---
>  drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index b7151c5f81b1..17bc76ea86ae 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -640,7 +640,7 @@ struct coresight_device *coresight_get_sink_by_id(u32 id)
>   * 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;
>
> @@ -648,10 +648,25 @@ static void coresight_grab_device(struct coresight_device *csdev)
>                 struct coresight_device *child;
>
>                 child  = csdev->pdata->conns[i].child_dev;
> -               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> +               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
> +                       if (!try_module_get(child->dev.parent->driver->owner))
> +                               goto err;
>                         pm_runtime_get_sync(child->dev.parent);
> +               }
>         }
> +       if (!try_module_get(csdev->dev.parent->driver->owner))
> +               goto err;
>         pm_runtime_get_sync(csdev->dev.parent);
> +       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)
> +                       pm_runtime_put(child->dev.parent);
> +       }
> +       return -ENODEV;
>  }
>
>  /*
> @@ -663,12 +678,15 @@ static void coresight_drop_device(struct coresight_device *csdev)
>         int i;
>
>         pm_runtime_put(csdev->dev.parent);
> +       module_put(csdev->dev.parent->driver->owner);
>         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)
> +               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
>                         pm_runtime_put(child->dev.parent);
> +                       module_put(child->dev.parent->driver->owner);
> +               }
>         }
>  }
>
> @@ -721,7 +739,8 @@ static int _coresight_build_path(struct coresight_device *csdev,
>         if (!node)
>                 return -ENOMEM;
>
> -       coresight_grab_device(csdev);
> +       if (coresight_grab_device(csdev))
> +               return -ENODEV;
>         node->csdev = csdev;
>         list_add(&node->link, path);
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

The CTI devices are associated with other coresight components using
csdev->ect_dev; These are not handled in the main linear trace path as
these have a star topology interlinking all components.
However, when a component has an associated ect_dev then is enabled at
the same time as the other component is enabled.

So a module_get/put will be needed for this association to prevent the
CTI being removed while a trace session is active.
I suggest in coresight.c : coresight_control_assoc_ectdev()

Regards

Mike


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

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

Re: [PATCH v4 00/20] coresight: allow to build coresight as modules

[Mike Leach]

Hi Tingwei,

Minor Nit: can we get some consistency with the patch naming.
coresight: <component>: <description>
e.g. coresight: cti: allow build as module
where specific components are used. This makes identifying patches for
specific components a lot easier.

Also it would be good to add my patch [1] to this series as requested
by Mathieu - it will ensure that the patch set works as a complete
set.

Thanks and Regards

Mike

[1] https://lists.linaro.org/pipermail/coresight/2020-July/004275.html

On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang <tingwei@codeaurora.org> wrote:
>
> Allow to build coresight as modules. This gives developers the feasibility to
> test their code without reboot.
>
> This series is based on below two series.
>
>  - "coresight: allow to build components as modules"
>    https://lkml.org/lkml/2018/6/5/989
>  - "coresight: make drivers modular"
>    https://lkml.org/lkml/2020/1/17/468
>
> Change from v3:
> Rebase to coresight-next (Mike and Mathieu)
> Reorder try_get_module() (Suzuki)
> Clean up etmdrvdata[] in device remote path (Mike)
> Move cti_remove_conn_xrefs to cti_remove (Mike)
>
> Change from v2:
> Rebase to 5.8-rc5. Export coresight_add_sysfs_link and
> coresight_remove_sysfs_link
> Fix one cut and paste error on MODULE_DESCRIPTION of CTI
>
> Change from v1:
> Use try_module_get() to avoid module to be unloaded when device is used
> in active trace session. (Mathieu P)
>
> Change from above two series.
> This series adds the support to dynamically remove module when the device in
> that module is enabled and used by some trace path. It disables all trace
> paths with that device and release the trace path.
>
> Kim Phillips (7):
>   coresight: use IS_ENABLED for CONFIGs that may be modules
>   coresight: allow etm3x to be built as a module
>   coresight: allow etm4x to be built as a module
>   coresight: allow etb to be built as a module
>   coresight: allow tpiu to be built as a module
>   coresight: allow tmc to be built as a module
>   coresight: allow funnel and replicator drivers to be built as modules
>
> Mian Yousaf Kaukab (4):
>   coresight: export global symbols
>   coresight: remove multiple init calls from funnel driver
>   coresight: remove multiple init calls from replicator driver
>   coresight: tmc-etr: add function to register catu ops
>
> Tingwei Zhang (9):
>   coresight: cpu_debug: add module name in Kconfig
>   coresight: cpu_debug: define MODULE_DEVICE_TABLE
>   coresight: add coresight prefix to barrier_pkt
>   coresight: add try_get_module() in coresight_grab_device()
>   allow to build coresight-stm as a module, for ease of development.
>   coresight: cti: add function to register cti associate ops
>   coresight: allow cti to be built as a module
>   coresight: allow catu drivers to be built as modules
>   coresight: allow the coresight core driver to be built as a module
>
>  drivers/hwtracing/coresight/Kconfig           |  54 ++++++++--
>  drivers/hwtracing/coresight/Makefile          |  22 ++--
>  drivers/hwtracing/coresight/coresight-catu.c  |  37 ++++++-
>  drivers/hwtracing/coresight/coresight-catu.h  |   2 -
>  .../{coresight.c => coresight-core.c}         | 102 ++++++++++++++----
>  .../hwtracing/coresight/coresight-cpu-debug.c |   2 +
>  .../{coresight-cti.c => coresight-cti-core.c} |  51 ++++++++-
>  drivers/hwtracing/coresight/coresight-etb10.c |  22 +++-
>  .../hwtracing/coresight/coresight-etm-perf.c  |   9 +-
>  .../hwtracing/coresight/coresight-etm-perf.h  |   5 +-
>  ...resight-etm3x.c => coresight-etm3x-core.c} |  27 ++++-
>  ...resight-etm4x.c => coresight-etm4x-core.c} |  26 ++++-
>  .../hwtracing/coresight/coresight-funnel.c    |  62 ++++++++++-
>  .../hwtracing/coresight/coresight-platform.c  |   1 +
>  drivers/hwtracing/coresight/coresight-priv.h  |  24 ++---
>  .../coresight/coresight-replicator.c          |  63 ++++++++++-
>  drivers/hwtracing/coresight/coresight-stm.c   |  20 +++-
>  drivers/hwtracing/coresight/coresight-sysfs.c |   2 +
>  .../{coresight-tmc.c => coresight-tmc-core.c} |  19 +++-
>  .../hwtracing/coresight/coresight-tmc-etf.c   |   2 +-
>  .../hwtracing/coresight/coresight-tmc-etr.c   |  21 +++-
>  drivers/hwtracing/coresight/coresight-tmc.h   |   3 +
>  drivers/hwtracing/coresight/coresight-tpiu.c  |  19 +++-
>  include/linux/coresight.h                     |   2 +-
>  24 files changed, 520 insertions(+), 77 deletions(-)
>  rename drivers/hwtracing/coresight/{coresight.c => coresight-core.c} (95%)
>  rename drivers/hwtracing/coresight/{coresight-cti.c => coresight-cti-core.c} (96%)
>  rename drivers/hwtracing/coresight/{coresight-etm3x.c => coresight-etm3x-core.c} (97%)
>  rename drivers/hwtracing/coresight/{coresight-etm4x.c => coresight-etm4x-core.c} (98%)
>  rename drivers/hwtracing/coresight/{coresight-tmc.c => coresight-tmc-core.c} (96%)
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
> _______________________________________________
> CoreSight mailing list
> CoreSight@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/coresight



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

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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Greg Kroah-Hartman]

On Thu, Jul 23, 2020 at 12:27:48PM +0800, Tingwei Zhang wrote:
> When coresight device is in an active session, driver module of
> that device should not be removed. Use try_get_module() in
> coresight_grab_device() to prevent module to be unloaded.

Are you sure this works?  Why is it needed at all?  Why not just tear
down the children properly when a module is removed so that you don't
need this at all?

> 
> Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> ---
>  drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index b7151c5f81b1..17bc76ea86ae 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -640,7 +640,7 @@ struct coresight_device *coresight_get_sink_by_id(u32 id)
>   * 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;
>  
> @@ -648,10 +648,25 @@ static void coresight_grab_device(struct coresight_device *csdev)
>  		struct coresight_device *child;
>  
>  		child  = csdev->pdata->conns[i].child_dev;
> -		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> +		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
> +			if (!try_module_get(child->dev.parent->driver->owner))

Why the child's parent?  Why not the child itself?


> +				goto err;

What about the error given to you here?  Why throw that away?

>  			pm_runtime_get_sync(child->dev.parent);
> +		}
>  	}
> +	if (!try_module_get(csdev->dev.parent->driver->owner))
> +		goto err;

You don't reduce the child's parent's driver owner module reference
here?

Ugh, that's a horid sentence :(

>  	pm_runtime_get_sync(csdev->dev.parent);
> +	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)
> +			pm_runtime_put(child->dev.parent);
> +	}
> +	return -ENODEV;
>  }
>  
>  /*
> @@ -663,12 +678,15 @@ static void coresight_drop_device(struct coresight_device *csdev)
>  	int i;
>  
>  	pm_runtime_put(csdev->dev.parent);
> +	module_put(csdev->dev.parent->driver->owner);
>  	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)
> +		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
>  			pm_runtime_put(child->dev.parent);
> +			module_put(child->dev.parent->driver->owner);
> +		}
>  	}
>  }
>  
> @@ -721,7 +739,8 @@ static int _coresight_build_path(struct coresight_device *csdev,
>  	if (!node)
>  		return -ENOMEM;
>  
> -	coresight_grab_device(csdev);
> +	if (coresight_grab_device(csdev))
> +		return -ENODEV;

Why not return the error given to you?


thanks,

greg k-h

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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Mathieu Poirier]

Hi Greg,

On Thu, Jul 23, 2020 at 01:07:07PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 23, 2020 at 12:27:48PM +0800, Tingwei Zhang wrote:
> > When coresight device is in an active session, driver module of
> > that device should not be removed. Use try_get_module() in
> > coresight_grab_device() to prevent module to be unloaded.
> 
> Are you sure this works?  Why is it needed at all?  Why not just tear
> down the children properly when a module is removed so that you don't
> need this at all?

Using the terms parent and child is somewhat ambiguous...  This is not a
parent-child relationship but simply an association between devices, something
like port 1 on device "parent" is connected to port 2 on device "child".  The
parent-child nomenclature was chosen to reflect that a device appears before
another in a coresight path.  Otherwise there is no other relation between
devices, hence the choice of using try_get_module()/put_module() to prevent
drivers from being taken away.  I'd be happy to proceed differently but haven't
found better options.

Going back to parent/child, we could have chosen left/right, up/down or A/B, all
of which are just as confusion. 

> 
> > 
> > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > ---
> >  drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
> >  1 file changed, 23 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> > index b7151c5f81b1..17bc76ea86ae 100644
> > --- a/drivers/hwtracing/coresight/coresight.c
> > +++ b/drivers/hwtracing/coresight/coresight.c
> > @@ -640,7 +640,7 @@ struct coresight_device *coresight_get_sink_by_id(u32 id)
> >   * 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;
> >  
> > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct coresight_device *csdev)
> >  		struct coresight_device *child;
> >  
> >  		child  = csdev->pdata->conns[i].child_dev;
> > -		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> > +		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
> > +			if (!try_module_get(child->dev.parent->driver->owner))
> 
> Why the child's parent?  Why not the child itself?

The device structure of each coresight_device is not associated with a driver.
It is there to take advantages of device goodies such as dev.type, dev.group,
dev.release and dev.bus.  Coresight IP blocks are discovered on the AMBA bus and as
such amba_device::dev::driver holds the driver itself.  In coresight_register()
the association coresigth::dev::parent = amba_device::dev is made.

> 
> 
> > +				goto err;
> 
> What about the error given to you here?  Why throw that away?
> 
> >  			pm_runtime_get_sync(child->dev.parent);
> > +		}
> >  	}
> > +	if (!try_module_get(csdev->dev.parent->driver->owner))
> > +		goto err;
> 
> You don't reduce the child's parent's driver owner module reference
> here?

Here @parent is referencing the current device.  Now that helper devices
connected to any of its outgoing ports have been enabled (and a reference count 
to the helper device driver incremented), a reference count to the current device
driver can also be incremented. 

I hope this clarify what is going on here.  It is a little unorthodox but so is
coresight.

Thanks,
Mathieu

> 
> Ugh, that's a horid sentence :(
> 
> >  	pm_runtime_get_sync(csdev->dev.parent);
> > +	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)
> > +			pm_runtime_put(child->dev.parent);
> > +	}
> > +	return -ENODEV;
> >  }
> >  
> >  /*
> > @@ -663,12 +678,15 @@ static void coresight_drop_device(struct coresight_device *csdev)
> >  	int i;
> >  
> >  	pm_runtime_put(csdev->dev.parent);
> > +	module_put(csdev->dev.parent->driver->owner);
> >  	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)
> > +		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
> >  			pm_runtime_put(child->dev.parent);
> > +			module_put(child->dev.parent->driver->owner);
> > +		}
> >  	}
> >  }
> >  
> > @@ -721,7 +739,8 @@ static int _coresight_build_path(struct coresight_device *csdev,
> >  	if (!node)
> >  		return -ENOMEM;
> >  
> > -	coresight_grab_device(csdev);
> > +	if (coresight_grab_device(csdev))
> > +		return -ENODEV;
> 
> Why not return the error given to you?
> 
> 
> thanks,
> 
> greg k-h

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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Greg Kroah-Hartman]

On Thu, Jul 23, 2020 at 12:04:47PM -0600, Mathieu Poirier wrote:
> Hi Greg,
> 
> On Thu, Jul 23, 2020 at 01:07:07PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jul 23, 2020 at 12:27:48PM +0800, Tingwei Zhang wrote:
> > > When coresight device is in an active session, driver module of
> > > that device should not be removed. Use try_get_module() in
> > > coresight_grab_device() to prevent module to be unloaded.
> > 
> > Are you sure this works?  Why is it needed at all?  Why not just tear
> > down the children properly when a module is removed so that you don't
> > need this at all?
> 
> Using the terms parent and child is somewhat ambiguous...  This is not a
> parent-child relationship but simply an association between devices, something
> like port 1 on device "parent" is connected to port 2 on device "child".  The
> parent-child nomenclature was chosen to reflect that a device appears before
> another in a coresight path.  Otherwise there is no other relation between
> devices, hence the choice of using try_get_module()/put_module() to prevent
> drivers from being taken away.  I'd be happy to proceed differently but haven't
> found better options.
> 
> Going back to parent/child, we could have chosen left/right, up/down or A/B, all
> of which are just as confusion. 

Ok, thanks.

But this causes confusion for everyone as seen below:

> > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > > ---
> > >  drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
> > >  1 file changed, 23 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> > > index b7151c5f81b1..17bc76ea86ae 100644
> > > --- a/drivers/hwtracing/coresight/coresight.c
> > > +++ b/drivers/hwtracing/coresight/coresight.c
> > > @@ -640,7 +640,7 @@ struct coresight_device *coresight_get_sink_by_id(u32 id)
> > >   * 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;
> > >  
> > > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct coresight_device *csdev)
> > >  		struct coresight_device *child;
> > >  
> > >  		child  = csdev->pdata->conns[i].child_dev;
> > > -		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> > > +		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
> > > +			if (!try_module_get(child->dev.parent->driver->owner))
> > 
> > Why the child's parent?  Why not the child itself?
> 
> The device structure of each coresight_device is not associated with a driver.
> It is there to take advantages of device goodies such as dev.type, dev.group,
> dev.release and dev.bus.  Coresight IP blocks are discovered on the AMBA bus and as
> such amba_device::dev::driver holds the driver itself.  In coresight_register()
> the association coresigth::dev::parent = amba_device::dev is made.
> 
> > 
> > 
> > > +				goto err;
> > 
> > What about the error given to you here?  Why throw that away?
> > 
> > >  			pm_runtime_get_sync(child->dev.parent);
> > > +		}
> > >  	}
> > > +	if (!try_module_get(csdev->dev.parent->driver->owner))
> > > +		goto err;
> > 
> > You don't reduce the child's parent's driver owner module reference
> > here?
> 
> Here @parent is referencing the current device.  Now that helper devices
> connected to any of its outgoing ports have been enabled (and a reference count 
> to the helper device driver incremented), a reference count to the current device
> driver can also be incremented. 

I mean the fact that your error handling does not seem to roll back the
module reference count you got up above in the other loop.

Or if it does, it's really really not obvious, and should at the very
least, be commented as to how it's all cleaning up properly.

thanks,

greg k-h

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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Mathieu Poirier]

On Thu, Jul 23, 2020 at 08:18:37PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 23, 2020 at 12:04:47PM -0600, Mathieu Poirier wrote:
> > Hi Greg,
> > 
> > On Thu, Jul 23, 2020 at 01:07:07PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jul 23, 2020 at 12:27:48PM +0800, Tingwei Zhang wrote:
> > > > When coresight device is in an active session, driver module of
> > > > that device should not be removed. Use try_get_module() in
> > > > coresight_grab_device() to prevent module to be unloaded.
> > > 
> > > Are you sure this works?  Why is it needed at all?  Why not just tear
> > > down the children properly when a module is removed so that you don't
> > > need this at all?
> > 
> > Using the terms parent and child is somewhat ambiguous...  This is not a
> > parent-child relationship but simply an association between devices, something
> > like port 1 on device "parent" is connected to port 2 on device "child".  The
> > parent-child nomenclature was chosen to reflect that a device appears before
> > another in a coresight path.  Otherwise there is no other relation between
> > devices, hence the choice of using try_get_module()/put_module() to prevent
> > drivers from being taken away.  I'd be happy to proceed differently but haven't
> > found better options.
> > 
> > Going back to parent/child, we could have chosen left/right, up/down or A/B, all
> > of which are just as confusion. 
> 
> Ok, thanks.
> 
> But this causes confusion for everyone as seen below:
> 
> > > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > > > ---
> > > >  drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
> > > >  1 file changed, 23 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> > > > index b7151c5f81b1..17bc76ea86ae 100644
> > > > --- a/drivers/hwtracing/coresight/coresight.c
> > > > +++ b/drivers/hwtracing/coresight/coresight.c
> > > > @@ -640,7 +640,7 @@ struct coresight_device *coresight_get_sink_by_id(u32 id)
> > > >   * 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;
> > > >  
> > > > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct coresight_device *csdev)
> > > >  		struct coresight_device *child;
> > > >  
> > > >  		child  = csdev->pdata->conns[i].child_dev;
> > > > -		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> > > > +		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
> > > > +			if (!try_module_get(child->dev.parent->driver->owner))
> > > 
> > > Why the child's parent?  Why not the child itself?
> > 
> > The device structure of each coresight_device is not associated with a driver.
> > It is there to take advantages of device goodies such as dev.type, dev.group,
> > dev.release and dev.bus.  Coresight IP blocks are discovered on the AMBA bus and as
> > such amba_device::dev::driver holds the driver itself.  In coresight_register()
> > the association coresigth::dev::parent = amba_device::dev is made.
> > 
> > > 
> > > 
> > > > +				goto err;
> > > 
> > > What about the error given to you here?  Why throw that away?
> > > 
> > > >  			pm_runtime_get_sync(child->dev.parent);
> > > > +		}
> > > >  	}
> > > > +	if (!try_module_get(csdev->dev.parent->driver->owner))
> > > > +		goto err;
> > > 
> > > You don't reduce the child's parent's driver owner module reference
> > > here?
> > 
> > Here @parent is referencing the current device.  Now that helper devices
> > connected to any of its outgoing ports have been enabled (and a reference count 
> > to the helper device driver incremented), a reference count to the current device
> > driver can also be incremented. 
> 
> I mean the fact that your error handling does not seem to roll back the
> module reference count you got up above in the other loop.

Ah! You were talking about the error condition, while I thought you were
referring to the normal execution path.  I am in agreement with all your comments on error
handling. 

> 
> Or if it does, it's really really not obvious, and should at the very
> least, be commented as to how it's all cleaning up properly.
> 
> thanks,
> 
> greg k-h

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

Re: [PATCH v4 00/20] coresight: allow to build coresight as modules

[Tingwei Zhang]

 Thu, Jul 23, 2020 at 07:03:10PM +0800, Mike Leach wrote:
> Hi Tingwei,
> 
> Minor Nit: can we get some consistency with the patch naming.
> coresight: <component>: <description>
> e.g. coresight: cti: allow build as module
> where specific components are used. This makes identifying patches for
> specific components a lot easier.
Thanks for the advice.  I'll rename them in next revision.
> 
> Also it would be good to add my patch [1] to this series as requested
> by Mathieu - it will ensure that the patch set works as a complete
> set.

My fault.  I thought he was talking about the change you shared which
depends on my change. I'll add your patch in next revision.

> 
> Thanks and Regards
> 
> Mike
> 
> [1] https://lists.linaro.org/pipermail/coresight/2020-July/004275.html
> 
> On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang <tingwei@codeaurora.org>
> wrote:
> >
> > Allow to build coresight as modules. This gives developers the
> feasibility to
> > test their code without reboot.
> >
> > This series is based on below two series.
> >
> >  - "coresight: allow to build components as modules"
> >    https://lkml.org/lkml/2018/6/5/989
> >  - "coresight: make drivers modular"
> >    https://lkml.org/lkml/2020/1/17/468
> >
> > Change from v3:
> > Rebase to coresight-next (Mike and Mathieu)
> > Reorder try_get_module() (Suzuki)
> > Clean up etmdrvdata[] in device remote path (Mike)
> > Move cti_remove_conn_xrefs to cti_remove (Mike)
> >
> > Change from v2:
> > Rebase to 5.8-rc5. Export coresight_add_sysfs_link and
> > coresight_remove_sysfs_link
> > Fix one cut and paste error on MODULE_DESCRIPTION of CTI
> >
> > Change from v1:
> > Use try_module_get() to avoid module to be unloaded when device is used
> > in active trace session. (Mathieu P)
> >
> > Change from above two series.
> > This series adds the support to dynamically remove module when the
> device in
> > that module is enabled and used by some trace path. It disables all
> trace
> > paths with that device and release the trace path.
> >
> > Kim Phillips (7):
> >   coresight: use IS_ENABLED for CONFIGs that may be modules
> >   coresight: allow etm3x to be built as a module
> >   coresight: allow etm4x to be built as a module
> >   coresight: allow etb to be built as a module
> >   coresight: allow tpiu to be built as a module
> >   coresight: allow tmc to be built as a module
> >   coresight: allow funnel and replicator drivers to be built as modules
> >
> > Mian Yousaf Kaukab (4):
> >   coresight: export global symbols
> >   coresight: remove multiple init calls from funnel driver
> >   coresight: remove multiple init calls from replicator driver
> >   coresight: tmc-etr: add function to register catu ops
> >
> > Tingwei Zhang (9):
> >   coresight: cpu_debug: add module name in Kconfig
> >   coresight: cpu_debug: define MODULE_DEVICE_TABLE
> >   coresight: add coresight prefix to barrier_pkt
> >   coresight: add try_get_module() in coresight_grab_device()
> >   allow to build coresight-stm as a module, for ease of development.
> >   coresight: cti: add function to register cti associate ops
> >   coresight: allow cti to be built as a module
> >   coresight: allow catu drivers to be built as modules
> >   coresight: allow the coresight core driver to be built as a module
> >
> >  drivers/hwtracing/coresight/Kconfig           |  54 ++++++++--
> >  drivers/hwtracing/coresight/Makefile          |  22 ++--
> >  drivers/hwtracing/coresight/coresight-catu.c  |  37 ++++++-
> >  drivers/hwtracing/coresight/coresight-catu.h  |   2 -
> >  .../{coresight.c => coresight-core.c}         | 102 ++++++++++++++----
> >  .../hwtracing/coresight/coresight-cpu-debug.c |   2 +
> >  .../{coresight-cti.c => coresight-cti-core.c} |  51 ++++++++-
> >  drivers/hwtracing/coresight/coresight-etb10.c |  22 +++-
> >  .../hwtracing/coresight/coresight-etm-perf.c  |   9 +-
> >  .../hwtracing/coresight/coresight-etm-perf.h  |   5 +-
> >  ...resight-etm3x.c => coresight-etm3x-core.c} |  27 ++++-
> >  ...resight-etm4x.c => coresight-etm4x-core.c} |  26 ++++-
> >  .../hwtracing/coresight/coresight-funnel.c    |  62 ++++++++++-
> >  .../hwtracing/coresight/coresight-platform.c  |   1 +
> >  drivers/hwtracing/coresight/coresight-priv.h  |  24 ++---
> >  .../coresight/coresight-replicator.c          |  63 ++++++++++-
> >  drivers/hwtracing/coresight/coresight-stm.c   |  20 +++-
> >  drivers/hwtracing/coresight/coresight-sysfs.c |   2 +
> >  .../{coresight-tmc.c => coresight-tmc-core.c} |  19 +++-
> >  .../hwtracing/coresight/coresight-tmc-etf.c   |   2 +-
> >  .../hwtracing/coresight/coresight-tmc-etr.c   |  21 +++-
> >  drivers/hwtracing/coresight/coresight-tmc.h   |   3 +
> >  drivers/hwtracing/coresight/coresight-tpiu.c  |  19 +++-
> >  include/linux/coresight.h                     |   2 +-
> >  24 files changed, 520 insertions(+), 77 deletions(-)
> >  rename drivers/hwtracing/coresight/{coresight.c => coresight-core.c}
> (95%)
> >  rename drivers/hwtracing/coresight/{coresight-cti.c =>
> coresight-cti-core.c} (96%)
> >  rename drivers/hwtracing/coresight/{coresight-etm3x.c =>
> coresight-etm3x-core.c} (97%)
> >  rename drivers/hwtracing/coresight/{coresight-etm4x.c =>
> coresight-etm4x-core.c} (98%)
> >  rename drivers/hwtracing/coresight/{coresight-tmc.c =>
> coresight-tmc-core.c} (96%)
> >
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
> > a Linux Foundation Collaborative Project
> >
> > _______________________________________________
> > CoreSight mailing list
> > CoreSight@lists.linaro.org
> > https://lists.linaro.org/mailman/listinfo/coresight
> 
> 
> 
> -- 
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Tingwei Zhang]

 Fri, Jul 24, 2020 at 03:15:40AM +0800, Mathieu Poirier wrote:
> On Thu, Jul 23, 2020 at 08:18:37PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jul 23, 2020 at 12:04:47PM -0600, Mathieu Poirier wrote:
> > > Hi Greg,
> > > 
> > > On Thu, Jul 23, 2020 at 01:07:07PM +0200, Greg Kroah-Hartman wrote:
> > > > On Thu, Jul 23, 2020 at 12:27:48PM +0800, Tingwei Zhang wrote:
> > > > > When coresight device is in an active session, driver module of
> > > > > that device should not be removed. Use try_get_module() in
> > > > > coresight_grab_device() to prevent module to be unloaded.
> > > > 
> > > > Are you sure this works?  Why is it needed at all?  Why not just
> tear
> > > > down the children properly when a module is removed so that you
> don't
> > > > need this at all?
> > > 
> > > Using the terms parent and child is somewhat ambiguous...  This is not
> a
> > > parent-child relationship but simply an association between devices,
> something
> > > like port 1 on device "parent" is connected to port 2 on device
> "child".  The
> > > parent-child nomenclature was chosen to reflect that a device appears
> before
> > > another in a coresight path.  Otherwise there is no other relation
> between
> > > devices, hence the choice of using try_get_module()/put_module() to
> prevent
> > > drivers from being taken away.  I'd be happy to proceed differently
> but haven't
> > > found better options.
> > > 
> > > Going back to parent/child, we could have chosen left/right, up/down
> or A/B, all
> > > of which are just as confusion. 
> > 
> > Ok, thanks.
> > 
> > But this causes confusion for everyone as seen below:
> > 
> > > > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > > > > ---
> > > > >  drivers/hwtracing/coresight/coresight.c | 27
> +++++++++++++++++++++----
> > > > >  1 file changed, 23 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/hwtracing/coresight/coresight.c
> b/drivers/hwtracing/coresight/coresight.c
> > > > > index b7151c5f81b1..17bc76ea86ae 100644
> > > > > --- a/drivers/hwtracing/coresight/coresight.c
> > > > > +++ b/drivers/hwtracing/coresight/coresight.c
> > > > > @@ -640,7 +640,7 @@ struct coresight_device
> *coresight_get_sink_by_id(u32 id)
> > > > >   * 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;
> > > > >  
> > > > > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct
> coresight_device *csdev)
> > > > >  		struct coresight_device *child;
> > > > >  
> > > > >  		child  = csdev->pdata->conns[i].child_dev;
> > > > > -		if (child && child->type ==
> CORESIGHT_DEV_TYPE_HELPER)
> > > > > +		if (child && child->type ==
> CORESIGHT_DEV_TYPE_HELPER) {
> > > > > +			if
> (!try_module_get(child->dev.parent->driver->owner))
> > > > 
> > > > Why the child's parent?  Why not the child itself?
> > > 
> > > The device structure of each coresight_device is not associated with a
> driver.
> > > It is there to take advantages of device goodies such as dev.type,
> dev.group,
> > > dev.release and dev.bus.  Coresight IP blocks are discovered on the
> AMBA bus and as
> > > such amba_device::dev::driver holds the driver itself.  In
> coresight_register()
> > > the association coresigth::dev::parent = amba_device::dev is made.
> > > 
> > > > 
> > > > 
> > > > > +				goto err;
> > > > 
> > > > What about the error given to you here?  Why throw that away?
> > > > 
> > > > >  			pm_runtime_get_sync(child->dev.parent);
> > > > > +		}
> > > > >  	}
> > > > > +	if (!try_module_get(csdev->dev.parent->driver->owner))
> > > > > +		goto err;
> > > > 
> > > > You don't reduce the child's parent's driver owner module reference
> > > > here?
> > > 
> > > Here @parent is referencing the current device.  Now that helper
> devices
> > > connected to any of its outgoing ports have been enabled (and a
> reference count 
> > > to the helper device driver incremented), a reference count to the
> current device
> > > driver can also be incremented. 
> > 
> > I mean the fact that your error handling does not seem to roll back the
> > module reference count you got up above in the other loop.
> 
> Ah! You were talking about the error condition, while I thought you were
> referring to the normal execution path.  I am in agreement with all your
> comments on error
> handling. 
>
Thanks to point this error out, Greg and Mathieu. I'll check and fix
them in next revision.
 
> > 
> > Or if it does, it's really really not obvious, and should at the very
> > least, be commented as to how it's all cleaning up properly.
> > 
> > thanks,
> > 
> > greg k-h
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Tingwei Zhang]

On Thu, Jul 23, 2020 at 07:07:07PM +0800, Greg Kroah-Hartman wrote:
> On Thu, Jul 23, 2020 at 12:27:48PM +0800, Tingwei Zhang wrote:
> > When coresight device is in an active session, driver module of
> > that device should not be removed. Use try_get_module() in
> > coresight_grab_device() to prevent module to be unloaded.
> 
> Are you sure this works?  Why is it needed at all?  Why not just tear
> down the children properly when a module is removed so that you don't
> need this at all?
> 
> > 
> > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > ---
> >  drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
> >  1 file changed, 23 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/hwtracing/coresight/coresight.c
> b/drivers/hwtracing/coresight/coresight.c
> > index b7151c5f81b1..17bc76ea86ae 100644
> > --- a/drivers/hwtracing/coresight/coresight.c
> > +++ b/drivers/hwtracing/coresight/coresight.c
> > @@ -640,7 +640,7 @@ struct coresight_device
> *coresight_get_sink_by_id(u32 id)
> >   * 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;
> >  
> > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct
> coresight_device *csdev)
> >  		struct coresight_device *child;
> >  
> >  		child  = csdev->pdata->conns[i].child_dev;
> > -		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> > +		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
> > +			if
> (!try_module_get(child->dev.parent->driver->owner))
> 
> Why the child's parent?  Why not the child itself?
> 
> 
> > +				goto err;
> 
> What about the error given to you here?  Why throw that away?
> 
try_module_get() returns bool which doesn't have error code.
> >  			pm_runtime_get_sync(child->dev.parent);
> > +		}
> >  	}
> > +	if (!try_module_get(csdev->dev.parent->driver->owner))
> > +		goto err;
> 
> You don't reduce the child's parent's driver owner module reference
> here?
> 

I'll fix in next revision.

> Ugh, that's a horid sentence :(
> 
> >  	pm_runtime_get_sync(csdev->dev.parent);
> > +	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)
> > +			pm_runtime_put(child->dev.parent);
> > +	}
> > +	return -ENODEV;
> >  }
> >  
> >  /*
> > @@ -663,12 +678,15 @@ static void coresight_drop_device(struct
> coresight_device *csdev)
> >  	int i;
> >  
> >  	pm_runtime_put(csdev->dev.parent);
> > +	module_put(csdev->dev.parent->driver->owner);
> >  	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)
> > +		if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
> >  			pm_runtime_put(child->dev.parent);
> > +			module_put(child->dev.parent->driver->owner);
> > +		}
> >  	}
> >  }
> >  
> > @@ -721,7 +739,8 @@ static int _coresight_build_path(struct
> coresight_device *csdev,
> >  	if (!node)
> >  		return -ENOMEM;
> >  
> > -	coresight_grab_device(csdev);
> > +	if (coresight_grab_device(csdev))
> > +		return -ENODEV;
> 
> Why not return the error given to you?

Thanks. I will fix it in next revision.

Thanks, Tingwei

> 
> 
> thanks,
> 
> greg k-h

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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Tingwei Zhang]

Hi Mike,

 Thu, Jul 23, 2020 at 06:55:47PM +0800, Mike Leach wrote:
> Hi Tingwei,
> 
> On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang <tingwei@codeaurora.org>
> wrote:
> >
> > When coresight device is in an active session, driver module of
> > that device should not be removed. Use try_get_module() in
> > coresight_grab_device() to prevent module to be unloaded.
> >
> > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > ---
> >  drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
> >  1 file changed, 23 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight.c
> b/drivers/hwtracing/coresight/coresight.c
> > index b7151c5f81b1..17bc76ea86ae 100644
> > --- a/drivers/hwtracing/coresight/coresight.c
> > +++ b/drivers/hwtracing/coresight/coresight.c
> > @@ -640,7 +640,7 @@ struct coresight_device
> *coresight_get_sink_by_id(u32 id)
> >   * 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;
> >
> > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct
> coresight_device *csdev)
> >                 struct coresight_device *child;
> >
> >                 child  = csdev->pdata->conns[i].child_dev;
> > -               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> > +               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
> > +                       if
> (!try_module_get(child->dev.parent->driver->owner))
> > +                               goto err;
> >                         pm_runtime_get_sync(child->dev.parent);
> > +               }
> >         }
> > +       if (!try_module_get(csdev->dev.parent->driver->owner))
> > +               goto err;
> >         pm_runtime_get_sync(csdev->dev.parent);
> > +       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)
> > +                       pm_runtime_put(child->dev.parent);
> > +       }
> > +       return -ENODEV;
> >  }
> >
> >  /*
> > @@ -663,12 +678,15 @@ static void coresight_drop_device(struct
> coresight_device *csdev)
> >         int i;
> >
> >         pm_runtime_put(csdev->dev.parent);
> > +       module_put(csdev->dev.parent->driver->owner);
> >         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)
> > +               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
> >                         pm_runtime_put(child->dev.parent);
> > +                       module_put(child->dev.parent->driver->owner);
> > +               }
> >         }
> >  }
> >
> > @@ -721,7 +739,8 @@ static int _coresight_build_path(struct
> coresight_device *csdev,
> >         if (!node)
> >                 return -ENOMEM;
> >
> > -       coresight_grab_device(csdev);
> > +       if (coresight_grab_device(csdev))
> > +               return -ENODEV;
> >         node->csdev = csdev;
> >         list_add(&node->link, path);
> >
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
> > a Linux Foundation Collaborative Project
> >
> 
> The CTI devices are associated with other coresight components using
> csdev->ect_dev; These are not handled in the main linear trace path as
> these have a star topology interlinking all components.
> However, when a component has an associated ect_dev then is enabled at
> the same time as the other component is enabled.
> 
> So a module_get/put will be needed for this association to prevent the
> CTI being removed while a trace session is active.
> I suggest in coresight.c : coresight_control_assoc_ectdev()
> 

In module unload process, devices of that module will be removed. In
this case, cti_remove() is called on all cti device when unloading
cti module. All cti connections is cleaned at that time.
csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev()
will return since since ect_csdev is NULL. 

I think we are safe without holding module reference since cti
driver has done a pretty good job on clean up.

Let me know if you still think we need hold the module reference.

Thanks, Tingwei
> Regards
> 
> Mike
> 
> 
> -- 
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Mike Leach]

Hi Tingwei,

On Sat, 25 Jul 2020 at 11:05, Tingwei Zhang <tingweiz@codeaurora.org> wrote:
>
> Hi Mike,
>
>  Thu, Jul 23, 2020 at 06:55:47PM +0800, Mike Leach wrote:
> > Hi Tingwei,
> >
> > On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang <tingwei@codeaurora.org>
> > wrote:
> > >
> > > When coresight device is in an active session, driver module of
> > > that device should not be removed. Use try_get_module() in
> > > coresight_grab_device() to prevent module to be unloaded.
> > >
> > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > > ---
> > >  drivers/hwtracing/coresight/coresight.c | 27 +++++++++++++++++++++----
> > >  1 file changed, 23 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/hwtracing/coresight/coresight.c
> > b/drivers/hwtracing/coresight/coresight.c
> > > index b7151c5f81b1..17bc76ea86ae 100644
> > > --- a/drivers/hwtracing/coresight/coresight.c
> > > +++ b/drivers/hwtracing/coresight/coresight.c
> > > @@ -640,7 +640,7 @@ struct coresight_device
> > *coresight_get_sink_by_id(u32 id)
> > >   * 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;
> > >
> > > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct
> > coresight_device *csdev)
> > >                 struct coresight_device *child;
> > >
> > >                 child  = csdev->pdata->conns[i].child_dev;
> > > -               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
> > > +               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
> > > +                       if
> > (!try_module_get(child->dev.parent->driver->owner))
> > > +                               goto err;
> > >                         pm_runtime_get_sync(child->dev.parent);
> > > +               }
> > >         }
> > > +       if (!try_module_get(csdev->dev.parent->driver->owner))
> > > +               goto err;
> > >         pm_runtime_get_sync(csdev->dev.parent);
> > > +       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)
> > > +                       pm_runtime_put(child->dev.parent);
> > > +       }
> > > +       return -ENODEV;
> > >  }
> > >
> > >  /*
> > > @@ -663,12 +678,15 @@ static void coresight_drop_device(struct
> > coresight_device *csdev)
> > >         int i;
> > >
> > >         pm_runtime_put(csdev->dev.parent);
> > > +       module_put(csdev->dev.parent->driver->owner);
> > >         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)
> > > +               if (child && child->type == CORESIGHT_DEV_TYPE_HELPER) {
> > >                         pm_runtime_put(child->dev.parent);
> > > +                       module_put(child->dev.parent->driver->owner);
> > > +               }
> > >         }
> > >  }
> > >
> > > @@ -721,7 +739,8 @@ static int _coresight_build_path(struct
> > coresight_device *csdev,
> > >         if (!node)
> > >                 return -ENOMEM;
> > >
> > > -       coresight_grab_device(csdev);
> > > +       if (coresight_grab_device(csdev))
> > > +               return -ENODEV;
> > >         node->csdev = csdev;
> > >         list_add(&node->link, path);
> > >
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > Forum,
> > > a Linux Foundation Collaborative Project
> > >
> >
> > The CTI devices are associated with other coresight components using
> > csdev->ect_dev; These are not handled in the main linear trace path as
> > these have a star topology interlinking all components.
> > However, when a component has an associated ect_dev then is enabled at
> > the same time as the other component is enabled.
> >
> > So a module_get/put will be needed for this association to prevent the
> > CTI being removed while a trace session is active.
> > I suggest in coresight.c : coresight_control_assoc_ectdev()
> >
>
> In module unload process, devices of that module will be removed. In
> this case, cti_remove() is called on all cti device when unloading
> cti module. All cti connections is cleaned at that time.
> csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev()
> will return since since ect_csdev is NULL.
>
> I think we are safe without holding module reference since cti
> driver has done a pretty good job on clean up.
>

The issue here is not about clean up. We need to keep all the
programmed coresight modules loaded and operational while a coresight
trace session is in progress. The CTI can be used to control the
generation of trace, trigger trace related events etc.
If the module is removed while a session is in progress then this
programming is lost and the trace data collected may not be what is
expected.

Regards

Mike

> Let me know if you still think we need hold the module reference.
>
> Thanks, Tingwei
> > Regards
> >
> > Mike
> >
> >
> > --
> > Mike Leach
> > Principal Engineer, ARM Ltd.
> > Manchester Design Centre. UK
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



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

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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Tingwei Zhang]

Hi Mike,
On Sat, Jul 25, 2020 at 09:51:31PM +0800, Mike Leach wrote:
> Hi Tingwei,
> 
> On Sat, 25 Jul 2020 at 11:05, Tingwei Zhang <tingweiz@codeaurora.org>
> wrote:
> >
> > Hi Mike,
> >
> >  Thu, Jul 23, 2020 at 06:55:47PM +0800, Mike Leach wrote:
> > > Hi Tingwei,
> > >
> > > On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang <tingwei@codeaurora.org>
> > > wrote:
> > > >
> > > > When coresight device is in an active session, driver module of
> > > > that device should not be removed. Use try_get_module() in
> > > > coresight_grab_device() to prevent module to be unloaded.
> > > >
> > > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > > > ---
> > > >  drivers/hwtracing/coresight/coresight.c | 27
> +++++++++++++++++++++----
> > > >  1 file changed, 23 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/hwtracing/coresight/coresight.c
> > > b/drivers/hwtracing/coresight/coresight.c
> > > > index b7151c5f81b1..17bc76ea86ae 100644
> > > > --- a/drivers/hwtracing/coresight/coresight.c
> > > > +++ b/drivers/hwtracing/coresight/coresight.c
> > > > @@ -640,7 +640,7 @@ struct coresight_device
> > > *coresight_get_sink_by_id(u32 id)
> > > >   * 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;
> > > >
> > > > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct
> > > coresight_device *csdev)
> > > >                 struct coresight_device *child;
> > > >
> > > >                 child  = csdev->pdata->conns[i].child_dev;
> > > > -               if (child && child->type ==
> CORESIGHT_DEV_TYPE_HELPER)
> > > > +               if (child && child->type ==
> CORESIGHT_DEV_TYPE_HELPER) {
> > > > +                       if
> > > (!try_module_get(child->dev.parent->driver->owner))
> > > > +                               goto err;
> > > >                         pm_runtime_get_sync(child->dev.parent);
> > > > +               }
> > > >         }
> > > > +       if (!try_module_get(csdev->dev.parent->driver->owner))
> > > > +               goto err;
> > > >         pm_runtime_get_sync(csdev->dev.parent);
> > > > +       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)
> > > > +                       pm_runtime_put(child->dev.parent);
> > > > +       }
> > > > +       return -ENODEV;
> > > >  }
> > > >
> > > >  /*
> > > > @@ -663,12 +678,15 @@ static void coresight_drop_device(struct
> > > coresight_device *csdev)
> > > >         int i;
> > > >
> > > >         pm_runtime_put(csdev->dev.parent);
> > > > +       module_put(csdev->dev.parent->driver->owner);
> > > >         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)
> > > > +               if (child && child->type ==
> CORESIGHT_DEV_TYPE_HELPER) {
> > > >                         pm_runtime_put(child->dev.parent);
> > > > +
> module_put(child->dev.parent->driver->owner);
> > > > +               }
> > > >         }
> > > >  }
> > > >
> > > > @@ -721,7 +739,8 @@ static int _coresight_build_path(struct
> > > coresight_device *csdev,
> > > >         if (!node)
> > > >                 return -ENOMEM;
> > > >
> > > > -       coresight_grab_device(csdev);
> > > > +       if (coresight_grab_device(csdev))
> > > > +               return -ENODEV;
> > > >         node->csdev = csdev;
> > > >         list_add(&node->link, path);
> > > >
> > > > --
> > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > > Forum,
> > > > a Linux Foundation Collaborative Project
> > > >
> > >
> > > The CTI devices are associated with other coresight components using
> > > csdev->ect_dev; These are not handled in the main linear trace path as
> > > these have a star topology interlinking all components.
> > > However, when a component has an associated ect_dev then is enabled at
> > > the same time as the other component is enabled.
> > >
> > > So a module_get/put will be needed for this association to prevent the
> > > CTI being removed while a trace session is active.
> > > I suggest in coresight.c : coresight_control_assoc_ectdev()
> > >
> >
> > In module unload process, devices of that module will be removed. In
> > this case, cti_remove() is called on all cti device when unloading
> > cti module. All cti connections is cleaned at that time.
> > csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev()
> > will return since since ect_csdev is NULL.
> >
> > I think we are safe without holding module reference since cti
> > driver has done a pretty good job on clean up.
> >
> 
> The issue here is not about clean up. We need to keep all the
> programmed coresight modules loaded and operational while a coresight
> trace session is in progress. The CTI can be used to control the
> generation of trace, trigger trace related events etc.
> If the module is removed while a session is in progress then this
> programming is lost and the trace data collected may not be what is
> expected.
>

Got your point now. In my opinion, CTI is kinda different to other
coresight components since it's optional.

For other components, they have to been there when path is built or
the enablement fails. Use can either get a successfully return which
means it's good and all devices/modules are there until path is
released, or a failed return which means trace session can't be
setup.

In CTI case, there's no garrantee that CTI related to the trace
session is there even the enablement is successful. For example, 
One cti is required for ETM tracing session. If cti module is
unloaded before enable that trace session. The enablement will
still be successfully done since there's no ect_dev connected
at all.

My point is holding cti module reference in enable path won't
fix all the problem. Altenatively, user should be aware that
unload cti module leads to all cti functioniol failure.
With current design, the behavior is consistant on cti. User
can unload cti module whenever he wants but it will break
the function.

Thanks, Tingwei
 
> Regards
> 
> Mike
> 
> > Let me know if you still think we need hold the module reference.
> >
> > Thanks, Tingwei
> > > Regards
> > >
> > > Mike
> > >
> > >
> > > --
> > > Mike Leach
> > > Principal Engineer, ARM Ltd.
> > > Manchester Design Centre. UK
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> 
> -- 
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Mike Leach]

Hi Tingwei,

On Sun, 26 Jul 2020 at 02:32, Tingwei Zhang <tingweiz@codeaurora.org> wrote:
>
> Hi Mike,
> On Sat, Jul 25, 2020 at 09:51:31PM +0800, Mike Leach wrote:
> > Hi Tingwei,
> >
> > On Sat, 25 Jul 2020 at 11:05, Tingwei Zhang <tingweiz@codeaurora.org>
> > wrote:
> > >
> > > Hi Mike,
> > >
> > >  Thu, Jul 23, 2020 at 06:55:47PM +0800, Mike Leach wrote:
> > > > Hi Tingwei,
> > > >
> > > > On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang <tingwei@codeaurora.org>
> > > > wrote:
> > > > >
> > > > > When coresight device is in an active session, driver module of
> > > > > that device should not be removed. Use try_get_module() in
> > > > > coresight_grab_device() to prevent module to be unloaded.
> > > > >
> > > > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > > > > ---
> > > > >  drivers/hwtracing/coresight/coresight.c | 27
> > +++++++++++++++++++++----
> > > > >  1 file changed, 23 insertions(+), 4 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hwtracing/coresight/coresight.c
> > > > b/drivers/hwtracing/coresight/coresight.c
> > > > > index b7151c5f81b1..17bc76ea86ae 100644
> > > > > --- a/drivers/hwtracing/coresight/coresight.c
> > > > > +++ b/drivers/hwtracing/coresight/coresight.c
> > > > > @@ -640,7 +640,7 @@ struct coresight_device
> > > > *coresight_get_sink_by_id(u32 id)
> > > > >   * 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;
> > > > >
> > > > > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct
> > > > coresight_device *csdev)
> > > > >                 struct coresight_device *child;
> > > > >
> > > > >                 child  = csdev->pdata->conns[i].child_dev;
> > > > > -               if (child && child->type ==
> > CORESIGHT_DEV_TYPE_HELPER)
> > > > > +               if (child && child->type ==
> > CORESIGHT_DEV_TYPE_HELPER) {
> > > > > +                       if
> > > > (!try_module_get(child->dev.parent->driver->owner))
> > > > > +                               goto err;
> > > > >                         pm_runtime_get_sync(child->dev.parent);
> > > > > +               }
> > > > >         }
> > > > > +       if (!try_module_get(csdev->dev.parent->driver->owner))
> > > > > +               goto err;
> > > > >         pm_runtime_get_sync(csdev->dev.parent);
> > > > > +       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)
> > > > > +                       pm_runtime_put(child->dev.parent);
> > > > > +       }
> > > > > +       return -ENODEV;
> > > > >  }
> > > > >
> > > > >  /*
> > > > > @@ -663,12 +678,15 @@ static void coresight_drop_device(struct
> > > > coresight_device *csdev)
> > > > >         int i;
> > > > >
> > > > >         pm_runtime_put(csdev->dev.parent);
> > > > > +       module_put(csdev->dev.parent->driver->owner);
> > > > >         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)
> > > > > +               if (child && child->type ==
> > CORESIGHT_DEV_TYPE_HELPER) {
> > > > >                         pm_runtime_put(child->dev.parent);
> > > > > +
> > module_put(child->dev.parent->driver->owner);
> > > > > +               }
> > > > >         }
> > > > >  }
> > > > >
> > > > > @@ -721,7 +739,8 @@ static int _coresight_build_path(struct
> > > > coresight_device *csdev,
> > > > >         if (!node)
> > > > >                 return -ENOMEM;
> > > > >
> > > > > -       coresight_grab_device(csdev);
> > > > > +       if (coresight_grab_device(csdev))
> > > > > +               return -ENODEV;
> > > > >         node->csdev = csdev;
> > > > >         list_add(&node->link, path);
> > > > >
> > > > > --
> > > > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > > > Forum,
> > > > > a Linux Foundation Collaborative Project
> > > > >
> > > >
> > > > The CTI devices are associated with other coresight components using
> > > > csdev->ect_dev; These are not handled in the main linear trace path as
> > > > these have a star topology interlinking all components.
> > > > However, when a component has an associated ect_dev then is enabled at
> > > > the same time as the other component is enabled.
> > > >
> > > > So a module_get/put will be needed for this association to prevent the
> > > > CTI being removed while a trace session is active.
> > > > I suggest in coresight.c : coresight_control_assoc_ectdev()
> > > >
> > >
> > > In module unload process, devices of that module will be removed. In
> > > this case, cti_remove() is called on all cti device when unloading
> > > cti module. All cti connections is cleaned at that time.
> > > csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev()
> > > will return since since ect_csdev is NULL.
> > >
> > > I think we are safe without holding module reference since cti
> > > driver has done a pretty good job on clean up.
> > >
> >
> > The issue here is not about clean up. We need to keep all the
> > programmed coresight modules loaded and operational while a coresight
> > trace session is in progress. The CTI can be used to control the
> > generation of trace, trigger trace related events etc.
> > If the module is removed while a session is in progress then this
> > programming is lost and the trace data collected may not be what is
> > expected.
> >
>
> Got your point now. In my opinion, CTI is kinda different to other
> coresight components since it's optional.
>
> For other components, they have to been there when path is built or
> the enablement fails. Use can either get a successfully return which
> means it's good and all devices/modules are there until path is
> released, or a failed return which means trace session can't be
> setup.
>

The module get/put do more than ensure that the system can be enabled.
They ensure that no components in the coresight path can be unloaded
until all paths in use are released. This ensures that clients do not
get the trace system pulled out from underneath - corrupting trace
data, and possibly affecting the client program.

> In CTI case, there's no garrantee that CTI related to the trace
> session is there even the enablement is successful.

And there is no guarantee that it is not programmed to perform a key
task in controlling the generation of trace.

The entire programmed coresight system must be protected when in use.
That includes CTI devices. Otherwise the integrity of the captured
trace could be compromised, and indeed there may be no trace captured
if this is relying on CTI triggers.

>For example,
> One cti is required for ETM tracing session. If cti module is
> unloaded before enable that trace session. The enablement will
> still be successfully done since there's no ect_dev connected
> at all.
>
> My point is holding cti module reference in enable path won't
> fix all the problem. Altenatively, user should be aware that
> unload cti module leads to all cti functioniol failure.
> With current design, the behavior is consistant on cti. User
> can unload cti module whenever he wants but it will break
> the function.

Precisely - and this is what should never be allowed - why can the CTI
be used to break the trace system but not other components?

Moving forwards we will see users who simply use pre-programmed
settings from client programs such as perf. It may be that some users
do not have the credentials to add and remove modules - this could be
an admin function where larger systems with multiple sessions are in
use.


Regards

Mike


>
> Thanks, Tingwei
>
> > Regards
> >
> > Mike
> >
> > > Let me know if you still think we need hold the module reference.
> > >
> > > Thanks, Tingwei
> > > > Regards
> > > >
> > > > Mike
> > > >
> > > >
> > > > --
> > > > Mike Leach
> > > > Principal Engineer, ARM Ltd.
> > > > Manchester Design Centre. UK
> > > >
> > > > _______________________________________________
> > > > linux-arm-kernel mailing list
> > > > linux-arm-kernel@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> >
> >
> > --
> > Mike Leach
> > Principal Engineer, ARM Ltd.
> > Manchester Design Centre. UK
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



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

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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Tingwei Zhang]

Hi Mike,

On Tue, Jul 28, 2020 at 01:04:21AM +0800, Mike Leach wrote:
> Hi Tingwei,
> 
> On Sun, 26 Jul 2020 at 02:32, Tingwei Zhang <tingweiz@codeaurora.org>
> wrote:
> >
> > Hi Mike,
> > On Sat, Jul 25, 2020 at 09:51:31PM +0800, Mike Leach wrote:
> > > Hi Tingwei,
> > >
> > > On Sat, 25 Jul 2020 at 11:05, Tingwei Zhang <tingweiz@codeaurora.org>
> > > wrote:
> > > >
> > > > Hi Mike,
> > > >
> > > >  Thu, Jul 23, 2020 at 06:55:47PM +0800, Mike Leach wrote:
> > > > > Hi Tingwei,
> > > > >
> > > > > On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang
> <tingwei@codeaurora.org>
> > > > > wrote:
> > > > > >
> > > > > > When coresight device is in an active session, driver module of
> > > > > > that device should not be removed. Use try_get_module() in
> > > > > > coresight_grab_device() to prevent module to be unloaded.
> > > > > >
> > > > > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > > > > > ---
> > > > > >  drivers/hwtracing/coresight/coresight.c | 27
> > > +++++++++++++++++++++----
> > > > > >  1 file changed, 23 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/hwtracing/coresight/coresight.c
> > > > > b/drivers/hwtracing/coresight/coresight.c
> > > > > > index b7151c5f81b1..17bc76ea86ae 100644
> > > > > > --- a/drivers/hwtracing/coresight/coresight.c
> > > > > > +++ b/drivers/hwtracing/coresight/coresight.c
> > > > > > @@ -640,7 +640,7 @@ struct coresight_device
> > > > > *coresight_get_sink_by_id(u32 id)
> > > > > >   * 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;
> > > > > >
> > > > > > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct
> > > > > coresight_device *csdev)
> > > > > >                 struct coresight_device *child;
> > > > > >
> > > > > >                 child  = csdev->pdata->conns[i].child_dev;
> > > > > > -               if (child && child->type ==
> > > CORESIGHT_DEV_TYPE_HELPER)
> > > > > > +               if (child && child->type ==
> > > CORESIGHT_DEV_TYPE_HELPER) {
> > > > > > +                       if
> > > > > (!try_module_get(child->dev.parent->driver->owner))
> > > > > > +                               goto err;
> > > > > >                         pm_runtime_get_sync(child->dev.parent);
> > > > > > +               }
> > > > > >         }
> > > > > > +       if (!try_module_get(csdev->dev.parent->driver->owner))
> > > > > > +               goto err;
> > > > > >         pm_runtime_get_sync(csdev->dev.parent);
> > > > > > +       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)
> > > > > > +                       pm_runtime_put(child->dev.parent);
> > > > > > +       }
> > > > > > +       return -ENODEV;
> > > > > >  }
> > > > > >
> > > > > >  /*
> > > > > > @@ -663,12 +678,15 @@ static void coresight_drop_device(struct
> > > > > coresight_device *csdev)
> > > > > >         int i;
> > > > > >
> > > > > >         pm_runtime_put(csdev->dev.parent);
> > > > > > +       module_put(csdev->dev.parent->driver->owner);
> > > > > >         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)
> > > > > > +               if (child && child->type ==
> > > CORESIGHT_DEV_TYPE_HELPER) {
> > > > > >                         pm_runtime_put(child->dev.parent);
> > > > > > +
> > > module_put(child->dev.parent->driver->owner);
> > > > > > +               }
> > > > > >         }
> > > > > >  }
> > > > > >
> > > > > > @@ -721,7 +739,8 @@ static int _coresight_build_path(struct
> > > > > coresight_device *csdev,
> > > > > >         if (!node)
> > > > > >                 return -ENOMEM;
> > > > > >
> > > > > > -       coresight_grab_device(csdev);
> > > > > > +       if (coresight_grab_device(csdev))
> > > > > > +               return -ENODEV;
> > > > > >         node->csdev = csdev;
> > > > > >         list_add(&node->link, path);
> > > > > >
> > > > > > --
> > > > > > The Qualcomm Innovation Center, Inc. is a member of the Code
> Aurora
> > > > > Forum,
> > > > > > a Linux Foundation Collaborative Project
> > > > > >
> > > > >
> > > > > The CTI devices are associated with other coresight components
> using
> > > > > csdev->ect_dev; These are not handled in the main linear trace
> path as
> > > > > these have a star topology interlinking all components.
> > > > > However, when a component has an associated ect_dev then is
> enabled at
> > > > > the same time as the other component is enabled.
> > > > >
> > > > > So a module_get/put will be needed for this association to prevent
> the
> > > > > CTI being removed while a trace session is active.
> > > > > I suggest in coresight.c : coresight_control_assoc_ectdev()
> > > > >
> > > >
> > > > In module unload process, devices of that module will be removed. In
> > > > this case, cti_remove() is called on all cti device when unloading
> > > > cti module. All cti connections is cleaned at that time.
> > > > csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev()
> > > > will return since since ect_csdev is NULL.
> > > >
> > > > I think we are safe without holding module reference since cti
> > > > driver has done a pretty good job on clean up.
> > > >
> > >
> > > The issue here is not about clean up. We need to keep all the
> > > programmed coresight modules loaded and operational while a coresight
> > > trace session is in progress. The CTI can be used to control the
> > > generation of trace, trigger trace related events etc.
> > > If the module is removed while a session is in progress then this
> > > programming is lost and the trace data collected may not be what is
> > > expected.
> > >
> >
> > Got your point now. In my opinion, CTI is kinda different to other
> > coresight components since it's optional.
> >
> > For other components, they have to been there when path is built or
> > the enablement fails. Use can either get a successfully return which
> > means it's good and all devices/modules are there until path is
> > released, or a failed return which means trace session can't be
> > setup.
> >
> 
> The module get/put do more than ensure that the system can be enabled.
> They ensure that no components in the coresight path can be unloaded
> until all paths in use are released. This ensures that clients do not
> get the trace system pulled out from underneath - corrupting trace
> data, and possibly affecting the client program.
>

I agree.
 
> > In CTI case, there's no garrantee that CTI related to the trace
> > session is there even the enablement is successful.
> 
> And there is no guarantee that it is not programmed to perform a key
> task in controlling the generation of trace.
> 
> The entire programmed coresight system must be protected when in use.
> That includes CTI devices. Otherwise the integrity of the captured
> trace could be compromised, and indeed there may be no trace captured
> if this is relying on CTI triggers.
> 

That's correct.  If CTI is not there, the trace may be wrong or no
trace as all.

> >For example,
> > One cti is required for ETM tracing session. If cti module is
> > unloaded before enable that trace session. The enablement will
> > still be successfully done since there's no ect_dev connected
> > at all.
> >
> > My point is holding cti module reference in enable path won't
> > fix all the problem. Altenatively, user should be aware that
> > unload cti module leads to all cti functioniol failure.
> > With current design, the behavior is consistant on cti. User
> > can unload cti module whenever he wants but it will break
> > the function.
> 
> Precisely - and this is what should never be allowed - why can the CTI
> be used to break the trace system but not other components?
> 

We can't prevent user to unload CTI module when there's no active
trace session. If we don't allow CTI to be not available, we need to
check whether CTI is there in build_path routine or coresight enable
routine. We need return failure if CTI is not there when user enables
the trace session. Currently, coresight framework check associate CTI
device, but if CTI deivce is not probed due to probe failure or
module unload, coresight framework assume there's no assocaite CTI
device and just continue the enablement.  It won't return failure.

If we have plan to change CTI check to be mandatory, make the coresight
enablement fail if CTI is not there like other trace components.  I'm
perfect fine to add module_get/module_put for CTI.

> Moving forwards we will see users who simply use pre-programmed
> settings from client programs such as perf. It may be that some users
> do not have the credentials to add and remove modules - this could be
> an admin function where larger systems with multiple sessions are in
> use.
> 
> 
> Regards
> 
> Mike
> 
> 
> >
> > Thanks, Tingwei
> >
> > > Regards
> > >
> > > Mike
> > >
> > > > Let me know if you still think we need hold the module reference.
> > > >
> > > > Thanks, Tingwei
> > > > > Regards
> > > > >
> > > > > Mike
> > > > >
> > > > >
> > > > > --
> > > > > Mike Leach
> > > > > Principal Engineer, ARM Ltd.
> > > > > Manchester Design Centre. UK
> > > > >
> > > > > _______________________________________________
> > > > > linux-arm-kernel mailing list
> > > > > linux-arm-kernel@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > >
> > >
> > >
> > > --
> > > Mike Leach
> > > Principal Engineer, ARM Ltd.
> > > Manchester Design Centre. UK
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> 
> -- 
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Mike Leach]

Hi Tingwei,

On Tue, 28 Jul 2020 at 02:07, Tingwei Zhang <tingweiz@codeaurora.org> wrote:
>
> Hi Mike,
>
> On Tue, Jul 28, 2020 at 01:04:21AM +0800, Mike Leach wrote:
> > Hi Tingwei,
> >
> > On Sun, 26 Jul 2020 at 02:32, Tingwei Zhang <tingweiz@codeaurora.org>
> > wrote:
> > >
> > > Hi Mike,
> > > On Sat, Jul 25, 2020 at 09:51:31PM +0800, Mike Leach wrote:
> > > > Hi Tingwei,
> > > >
> > > > On Sat, 25 Jul 2020 at 11:05, Tingwei Zhang <tingweiz@codeaurora.org>
> > > > wrote:
> > > > >
> > > > > Hi Mike,
> > > > >
> > > > >  Thu, Jul 23, 2020 at 06:55:47PM +0800, Mike Leach wrote:
> > > > > > Hi Tingwei,
> > > > > >
> > > > > > On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang
> > <tingwei@codeaurora.org>
> > > > > > wrote:
> > > > > > >
> > > > > > > When coresight device is in an active session, driver module of
> > > > > > > that device should not be removed. Use try_get_module() in
> > > > > > > coresight_grab_device() to prevent module to be unloaded.
> > > > > > >
> > > > > > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > > > > > > ---
> > > > > > >  drivers/hwtracing/coresight/coresight.c | 27
> > > > +++++++++++++++++++++----
> > > > > > >  1 file changed, 23 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/hwtracing/coresight/coresight.c
> > > > > > b/drivers/hwtracing/coresight/coresight.c
> > > > > > > index b7151c5f81b1..17bc76ea86ae 100644
> > > > > > > --- a/drivers/hwtracing/coresight/coresight.c
> > > > > > > +++ b/drivers/hwtracing/coresight/coresight.c
> > > > > > > @@ -640,7 +640,7 @@ struct coresight_device
> > > > > > *coresight_get_sink_by_id(u32 id)
> > > > > > >   * 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;
> > > > > > >
> > > > > > > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct
> > > > > > coresight_device *csdev)
> > > > > > >                 struct coresight_device *child;
> > > > > > >
> > > > > > >                 child  = csdev->pdata->conns[i].child_dev;
> > > > > > > -               if (child && child->type ==
> > > > CORESIGHT_DEV_TYPE_HELPER)
> > > > > > > +               if (child && child->type ==
> > > > CORESIGHT_DEV_TYPE_HELPER) {
> > > > > > > +                       if
> > > > > > (!try_module_get(child->dev.parent->driver->owner))
> > > > > > > +                               goto err;
> > > > > > >                         pm_runtime_get_sync(child->dev.parent);
> > > > > > > +               }
> > > > > > >         }
> > > > > > > +       if (!try_module_get(csdev->dev.parent->driver->owner))
> > > > > > > +               goto err;
> > > > > > >         pm_runtime_get_sync(csdev->dev.parent);
> > > > > > > +       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)
> > > > > > > +                       pm_runtime_put(child->dev.parent);
> > > > > > > +       }
> > > > > > > +       return -ENODEV;
> > > > > > >  }
> > > > > > >
> > > > > > >  /*
> > > > > > > @@ -663,12 +678,15 @@ static void coresight_drop_device(struct
> > > > > > coresight_device *csdev)
> > > > > > >         int i;
> > > > > > >
> > > > > > >         pm_runtime_put(csdev->dev.parent);
> > > > > > > +       module_put(csdev->dev.parent->driver->owner);
> > > > > > >         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)
> > > > > > > +               if (child && child->type ==
> > > > CORESIGHT_DEV_TYPE_HELPER) {
> > > > > > >                         pm_runtime_put(child->dev.parent);
> > > > > > > +
> > > > module_put(child->dev.parent->driver->owner);
> > > > > > > +               }
> > > > > > >         }
> > > > > > >  }
> > > > > > >
> > > > > > > @@ -721,7 +739,8 @@ static int _coresight_build_path(struct
> > > > > > coresight_device *csdev,
> > > > > > >         if (!node)
> > > > > > >                 return -ENOMEM;
> > > > > > >
> > > > > > > -       coresight_grab_device(csdev);
> > > > > > > +       if (coresight_grab_device(csdev))
> > > > > > > +               return -ENODEV;
> > > > > > >         node->csdev = csdev;
> > > > > > >         list_add(&node->link, path);
> > > > > > >
> > > > > > > --
> > > > > > > The Qualcomm Innovation Center, Inc. is a member of the Code
> > Aurora
> > > > > > Forum,
> > > > > > > a Linux Foundation Collaborative Project
> > > > > > >
> > > > > >
> > > > > > The CTI devices are associated with other coresight components
> > using
> > > > > > csdev->ect_dev; These are not handled in the main linear trace
> > path as
> > > > > > these have a star topology interlinking all components.
> > > > > > However, when a component has an associated ect_dev then is
> > enabled at
> > > > > > the same time as the other component is enabled.
> > > > > >
> > > > > > So a module_get/put will be needed for this association to prevent
> > the
> > > > > > CTI being removed while a trace session is active.
> > > > > > I suggest in coresight.c : coresight_control_assoc_ectdev()
> > > > > >
> > > > >
> > > > > In module unload process, devices of that module will be removed. In
> > > > > this case, cti_remove() is called on all cti device when unloading
> > > > > cti module. All cti connections is cleaned at that time.
> > > > > csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev()
> > > > > will return since since ect_csdev is NULL.
> > > > >
> > > > > I think we are safe without holding module reference since cti
> > > > > driver has done a pretty good job on clean up.
> > > > >
> > > >
> > > > The issue here is not about clean up. We need to keep all the
> > > > programmed coresight modules loaded and operational while a coresight
> > > > trace session is in progress. The CTI can be used to control the
> > > > generation of trace, trigger trace related events etc.
> > > > If the module is removed while a session is in progress then this
> > > > programming is lost and the trace data collected may not be what is
> > > > expected.
> > > >
> > >
> > > Got your point now. In my opinion, CTI is kinda different to other
> > > coresight components since it's optional.
> > >
> > > For other components, they have to been there when path is built or
> > > the enablement fails. Use can either get a successfully return which
> > > means it's good and all devices/modules are there until path is
> > > released, or a failed return which means trace session can't be
> > > setup.
> > >
> >
> > The module get/put do more than ensure that the system can be enabled.
> > They ensure that no components in the coresight path can be unloaded
> > until all paths in use are released. This ensures that clients do not
> > get the trace system pulled out from underneath - corrupting trace
> > data, and possibly affecting the client program.
> >
>
> I agree.
>
> > > In CTI case, there's no garrantee that CTI related to the trace
> > > session is there even the enablement is successful.
> >
> > And there is no guarantee that it is not programmed to perform a key
> > task in controlling the generation of trace.
> >
> > The entire programmed coresight system must be protected when in use.
> > That includes CTI devices. Otherwise the integrity of the captured
> > trace could be compromised, and indeed there may be no trace captured
> > if this is relying on CTI triggers.
> >
>
> That's correct.  If CTI is not there, the trace may be wrong or no
> trace as all.
>
> > >For example,
> > > One cti is required for ETM tracing session. If cti module is
> > > unloaded before enable that trace session. The enablement will
> > > still be successfully done since there's no ect_dev connected
> > > at all.
> > >
> > > My point is holding cti module reference in enable path won't
> > > fix all the problem. Altenatively, user should be aware that
> > > unload cti module leads to all cti functioniol failure.
> > > With current design, the behavior is consistant on cti. User
> > > can unload cti module whenever he wants but it will break
> > > the function.
> >
> > Precisely - and this is what should never be allowed - why can the CTI
> > be used to break the trace system but not other components?
> >
>

Sorry - I may not have been clear here - I do not want to make having
a CTI mandatory for enabling the trace session - this is an optional
component.

> We can't prevent user to unload CTI module when there's no active
> trace session.

Agreed - the same applies to all the other components.

> If we don't allow CTI to be not available, we need to
> check whether CTI is there in build_path routine or coresight enable
> routine. We need return failure if CTI is not there when user enables
> the trace session. Currently, coresight framework check associate CTI
> device, but if CTI deivce is not probed due to probe failure or
> module unload, coresight framework assume there's no assocaite CTI
> device and just continue the enablement.  It won't return failure.
>
> If we have plan to change CTI check to be mandatory, make the coresight
> enablement fail if CTI is not there like other trace components.  I'm
> perfect fine to add module_get/module_put for CTI.
>

If the CTI is present at the time the trace path is created, then it
must be enabled, and a failure to enable the CTI fails the entire path
enable - this is the operation that is implemented by
coresight_control_assoc_ectdev() at this time.
If a CTI is not present at the time the trace path is created, then as
you have said previously, the ect_dev pointer will be NULL and the
check and enable of CTI will be skipped by
coresight_control_assoc_ectdev() and have no effect on the trace path.

For the module version, if CTI is present and enabled then the module
must be locked so it cannot be removed while the trace is active -
this is the only change that is required. So adding get / put module
in coresight_control_assoc_ectdev() will lock the module in for the
duration that trace is active.

I've tested the code below to ensure that the cti module is correctly
locked while trace is active, but can be removed as required when
trace is inactive.

Regards

Mike

---------------------------------------------------------------------------------
static int
coresight_control_assoc_ectdev(struct coresight_device *csdev, bool enable)
{
    int ect_ret = 0;
    struct coresight_device *ect_csdev = csdev->ect_dev;
    struct module *mod;

    if (!ect_csdev)
        return 0;

    if ((!ect_ops(ect_csdev)->enable) || (!ect_ops(ect_csdev)->disable))
        return 0;

    mod = ect_csdev->dev.parent->driver->owner;

    if (enable) {
        if (try_module_get(mod)) {
            ect_ret = ect_ops(ect_csdev)->enable(ect_csdev);
            if (ect_ret)
                module_put(mod);
        } else
            ect_ret = -ENODEV;
    } else {
        module_put(mod);
        ect_ret = ect_ops(ect_csdev)->disable(ect_csdev);
    }

    /* output warning if ECT enable is preventing trace operation */
    if (ect_ret)
        dev_info(&csdev->dev, "Associated ECT device (%s) %s failed\n",
             dev_name(&ect_csdev->dev),
             enable ? "enable" : "disable");
    return ect_ret;
}
-----------------------------------------------------------------------------------------
Tests:-
-----------------------
root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-start.bash
Tracing via sysfs
enabling etf
wait before enable etm
enabling etm 0.
enabling etm 1.
enabling etm 2.
enabling etm 3.
waiting for trace
root@linaro-developer:/home/linaro/scripts# rmmod ../cs-mods/coresight-etm4x.ko
rmmod: ERROR: Module coresight_etm4x is in use
root@linaro-developer:/home/linaro/scripts# rmmod ../cs-mods/coresight-cti.ko
rmmod: ERROR: Module coresight_cti is in use
root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-end.bash
ending trace
disabling etm 0.
disabling etm 1.
disabling etm 2.
disabling etm 3.
disabling etf
16+0 records in
16+0 records out
8192 bytes (8.2 kB) copied, 0.000834116 s, 9.8 MB/s
root@linaro-developer:/home/linaro/scripts# rmmod ../cs-mods/coresight-cti.ko
root@linaro-developer:/home/linaro/scripts# rmmod ../cs-mods/coresight-etm4x.ko
root@linaro-developer:/home/linaro/scripts# insmod ../cs-mods/coresight-etm4x.ko
root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-start.bash
Tracing via sysfs
enabling etf
wait before enable etm
enabling etm 0.
enabling etm 1.
enabling etm 2.
enabling etm 3.
waiting for trace
root@linaro-developer:/home/linaro/scripts# rmmod ../cs-mods/coresight-etm4x.ko
rmmod: ERROR: Module coresight_etm4x is in use
root@linaro-developer:/home/linaro/scripts# rmmod ../cs-mods/coresight-cti.ko
rmmod: ERROR: Module coresight_cti is not currently loaded
root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-end.bash
ending trace
disabling etm 0.
disabling etm 1.
disabling etm 2.
disabling etm 3.
disabling etf
16+0 records in
16+0 records out
8192 bytes (8.2 kB) copied, 0.000819117 s, 10.0 MB/s

}



> > Moving forwards we will see users who simply use pre-programmed
> > settings from client programs such as perf. It may be that some users
> > do not have the credentials to add and remove modules - this could be
> > an admin function where larger systems with multiple sessions are in
> > use.
> >
> >
> > Regards
> >
> > Mike
> >
> >
> > >
> > > Thanks, Tingwei
> > >
> > > > Regards
> > > >
> > > > Mike
> > > >
> > > > > Let me know if you still think we need hold the module reference.
> > > > >
> > > > > Thanks, Tingwei
> > > > > > Regards
> > > > > >
> > > > > > Mike
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Mike Leach
> > > > > > Principal Engineer, ARM Ltd.
> > > > > > Manchester Design Centre. UK
> > > > > >
> > > > > > _______________________________________________
> > > > > > linux-arm-kernel mailing list
> > > > > > linux-arm-kernel@lists.infradead.org
> > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > >
> > > >
> > > >
> > > > --
> > > > Mike Leach
> > > > Principal Engineer, ARM Ltd.
> > > > Manchester Design Centre. UK
> > > >
> > > > _______________________________________________
> > > > linux-arm-kernel mailing list
> > > > linux-arm-kernel@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> >
> >
> > --
> > Mike Leach
> > Principal Engineer, ARM Ltd.
> > Manchester Design Centre. UK
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



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

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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Tingwei Zhang]

Hi Mike,
On Tue, Jul 28, 2020 at 06:08:37PM +0800, Mike Leach wrote:
> Hi Tingwei,
> 
> On Tue, 28 Jul 2020 at 02:07, Tingwei Zhang <tingweiz@codeaurora.org> wrote:
> >
> > Hi Mike,
> >
> > On Tue, Jul 28, 2020 at 01:04:21AM +0800, Mike Leach wrote:
> > > Hi Tingwei,
> > >
> > > On Sun, 26 Jul 2020 at 02:32, Tingwei Zhang <tingweiz@codeaurora.org>
> > > wrote:
> > > >
> > > > Hi Mike,
> > > > On Sat, Jul 25, 2020 at 09:51:31PM +0800, Mike Leach wrote:
> > > > > Hi Tingwei,
> > > > >
> > > > > On Sat, 25 Jul 2020 at 11:05, Tingwei Zhang 
> > > > > <tingweiz@codeaurora.org>
> > > > > wrote:
> > > > > >
> > > > > > Hi Mike,
> > > > > >
> > > > > >  Thu, Jul 23, 2020 at 06:55:47PM +0800, Mike Leach wrote:
> > > > > > > Hi Tingwei,
> > > > > > >
> > > > > > > On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang
> > > <tingwei@codeaurora.org>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > When coresight device is in an active session, driver module 
> > > > > > > > of
> > > > > > > > that device should not be removed. Use try_get_module() in
> > > > > > > > coresight_grab_device() to prevent module to be unloaded.
> > > > > > > >
> > > > > > > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > > > > > > > ---
> > > > > > > >  drivers/hwtracing/coresight/coresight.c | 27
> > > > > +++++++++++++++++++++----
> > > > > > > >  1 file changed, 23 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/hwtracing/coresight/coresight.c
> > > > > > > b/drivers/hwtracing/coresight/coresight.c
> > > > > > > > index b7151c5f81b1..17bc76ea86ae 100644
> > > > > > > > --- a/drivers/hwtracing/coresight/coresight.c
> > > > > > > > +++ b/drivers/hwtracing/coresight/coresight.c
> > > > > > > > @@ -640,7 +640,7 @@ struct coresight_device
> > > > > > > *coresight_get_sink_by_id(u32 id)
> > > > > > > >   * 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;
> > > > > > > >
> > > > > > > > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct
> > > > > > > coresight_device *csdev)
> > > > > > > >                 struct coresight_device *child;
> > > > > > > >
> > > > > > > >                 child  = csdev->pdata->conns[i].child_dev;
> > > > > > > > -               if (child && child->type ==
> > > > > CORESIGHT_DEV_TYPE_HELPER)
> > > > > > > > +               if (child && child->type ==
> > > > > CORESIGHT_DEV_TYPE_HELPER) {
> > > > > > > > +                       if
> > > > > > > (!try_module_get(child->dev.parent->driver->owner))
> > > > > > > > +                               goto err;
> > > > > > > > 
> > > > > > > > pm_runtime_get_sync(child->dev.parent);
> > > > > > > > +               }
> > > > > > > >         }
> > > > > > > > +       if (!try_module_get(csdev->dev.parent->driver->owner))
> > > > > > > > +               goto err;
> > > > > > > >         pm_runtime_get_sync(csdev->dev.parent);
> > > > > > > > +       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)
> > > > > > > > +                       pm_runtime_put(child->dev.parent);
> > > > > > > > +       }
> > > > > > > > +       return -ENODEV;
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  /*
> > > > > > > > @@ -663,12 +678,15 @@ static void coresight_drop_device(struct
> > > > > > > coresight_device *csdev)
> > > > > > > >         int i;
> > > > > > > >
> > > > > > > >         pm_runtime_put(csdev->dev.parent);
> > > > > > > > +       module_put(csdev->dev.parent->driver->owner);
> > > > > > > >         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)
> > > > > > > > +               if (child && child->type ==
> > > > > CORESIGHT_DEV_TYPE_HELPER) {
> > > > > > > >                         pm_runtime_put(child->dev.parent);
> > > > > > > > +
> > > > > module_put(child->dev.parent->driver->owner);
> > > > > > > > +               }
> > > > > > > >         }
> > > > > > > >  }
> > > > > > > >
> > > > > > > > @@ -721,7 +739,8 @@ static int _coresight_build_path(struct
> > > > > > > coresight_device *csdev,
> > > > > > > >         if (!node)
> > > > > > > >                 return -ENOMEM;
> > > > > > > >
> > > > > > > > -       coresight_grab_device(csdev);
> > > > > > > > +       if (coresight_grab_device(csdev))
> > > > > > > > +               return -ENODEV;
> > > > > > > >         node->csdev = csdev;
> > > > > > > >         list_add(&node->link, path);
> > > > > > > >
> > > > > > > > --
> > > > > > > > The Qualcomm Innovation Center, Inc. is a member of the Code
> > > Aurora
> > > > > > > Forum,
> > > > > > > > a Linux Foundation Collaborative Project
> > > > > > > >
> > > > > > >
> > > > > > > The CTI devices are associated with other coresight components
> > > using
> > > > > > > csdev->ect_dev; These are not handled in the main linear trace
> > > path as
> > > > > > > these have a star topology interlinking all components.
> > > > > > > However, when a component has an associated ect_dev then is
> > > enabled at
> > > > > > > the same time as the other component is enabled.
> > > > > > >
> > > > > > > So a module_get/put will be needed for this association to 
> > > > > > > prevent
> > > the
> > > > > > > CTI being removed while a trace session is active.
> > > > > > > I suggest in coresight.c : coresight_control_assoc_ectdev()
> > > > > > >
> > > > > >
> > > > > > In module unload process, devices of that module will be removed. 
> > > > > > In
> > > > > > this case, cti_remove() is called on all cti device when unloading
> > > > > > cti module. All cti connections is cleaned at that time.
> > > > > > csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev()
> > > > > > will return since since ect_csdev is NULL.
> > > > > >
> > > > > > I think we are safe without holding module reference since cti
> > > > > > driver has done a pretty good job on clean up.
> > > > > >
> > > > >
> > > > > The issue here is not about clean up. We need to keep all the
> > > > > programmed coresight modules loaded and operational while a 
> > > > > coresight
> > > > > trace session is in progress. The CTI can be used to control the
> > > > > generation of trace, trigger trace related events etc.
> > > > > If the module is removed while a session is in progress then this
> > > > > programming is lost and the trace data collected may not be what is
> > > > > expected.
> > > > >
> > > >
> > > > Got your point now. In my opinion, CTI is kinda different to other
> > > > coresight components since it's optional.
> > > >
> > > > For other components, they have to been there when path is built or
> > > > the enablement fails. Use can either get a successfully return which
> > > > means it's good and all devices/modules are there until path is
> > > > released, or a failed return which means trace session can't be
> > > > setup.
> > > >
> > >
> > > The module get/put do more than ensure that the system can be enabled.
> > > They ensure that no components in the coresight path can be unloaded
> > > until all paths in use are released. This ensures that clients do not
> > > get the trace system pulled out from underneath - corrupting trace
> > > data, and possibly affecting the client program.
> > >
> >
> > I agree.
> >
> > > > In CTI case, there's no garrantee that CTI related to the trace
> > > > session is there even the enablement is successful.
> > >
> > > And there is no guarantee that it is not programmed to perform a key
> > > task in controlling the generation of trace.
> > >
> > > The entire programmed coresight system must be protected when in use.
> > > That includes CTI devices. Otherwise the integrity of the captured
> > > trace could be compromised, and indeed there may be no trace captured
> > > if this is relying on CTI triggers.
> > >
> >
> > That's correct.  If CTI is not there, the trace may be wrong or no
> > trace as all.
> >
> > > >For example,
> > > > One cti is required for ETM tracing session. If cti module is
> > > > unloaded before enable that trace session. The enablement will
> > > > still be successfully done since there's no ect_dev connected
> > > > at all.
> > > >
> > > > My point is holding cti module reference in enable path won't
> > > > fix all the problem. Altenatively, user should be aware that
> > > > unload cti module leads to all cti functioniol failure.
> > > > With current design, the behavior is consistant on cti. User
> > > > can unload cti module whenever he wants but it will break
> > > > the function.
> > >
> > > Precisely - and this is what should never be allowed - why can the CTI
> > > be used to break the trace system but not other components?
> > >
> >
> 
> Sorry - I may not have been clear here - I do not want to make having
> a CTI mandatory for enabling the trace session - this is an optional
> component.
>
> > We can't prevent user to unload CTI module when there's no active
> > trace session.
> 
> Agreed - the same applies to all the other components.
> 
> > If we don't allow CTI to be not available, we need to
> > check whether CTI is there in build_path routine or coresight enable
> > routine. We need return failure if CTI is not there when user enables
> > the trace session. Currently, coresight framework check associate CTI
> > device, but if CTI deivce is not probed due to probe failure or
> > module unload, coresight framework assume there's no assocaite CTI
> > device and just continue the enablement.  It won't return failure.
> >
> > If we have plan to change CTI check to be mandatory, make the coresight
> > enablement fail if CTI is not there like other trace components.  I'm
> > perfect fine to add module_get/module_put for CTI.
> >
> 
> If the CTI is present at the time the trace path is created, then it
> must be enabled, and a failure to enable the CTI fails the entire path
> enable - this is the operation that is implemented by
> coresight_control_assoc_ectdev() at this time.
> If a CTI is not present at the time the trace path is created, then as
> you have said previously, the ect_dev pointer will be NULL and the
> check and enable of CTI will be skipped by
> coresight_control_assoc_ectdev() and have no effect on the trace path.
> 
> For the module version, if CTI is present and enabled then the module
> must be locked so it cannot be removed while the trace is active -
> this is the only change that is required. So adding get / put module
> in coresight_control_assoc_ectdev() will lock the module in for the
> duration that trace is active.
> 
> I've tested the code below to ensure that the cti module is correctly
> locked while trace is active, but can be removed as required when
> trace is inactive.
>

Thanks a lot for coming up with the patch and testing it, Mike. 
 
> Regards
> 
> Mike
> 
> ---------------------------------------------------------------------------------
> static int
> coresight_control_assoc_ectdev(struct coresight_device *csdev, bool enable)
> {
>     int ect_ret = 0;
>     struct coresight_device *ect_csdev = csdev->ect_dev;
>     struct module *mod;
> 
>     if (!ect_csdev)
>         return 0;
> 
>     if ((!ect_ops(ect_csdev)->enable) || (!ect_ops(ect_csdev)->disable))
>         return 0;
> 
>     mod = ect_csdev->dev.parent->driver->owner;
> 
>     if (enable) {
>         if (try_module_get(mod)) {
>             ect_ret = ect_ops(ect_csdev)->enable(ect_csdev);
>             if (ect_ret)
>                 module_put(mod);
>         } else
>             ect_ret = -ENODEV;
>     } else {
>         module_put(mod);
>         ect_ret = ect_ops(ect_csdev)->disable(ect_csdev);
>     }

If CTI module is removed before enablement of ETM and installed after ETM
enablement and before ETM disablement. We have a unbalanced module
reference number here. I made one test to prove this with above code.

We may need move module_put() after disable(). Return proper error from
disable() to indicate CTI was not enabled before so we can skip
module_put().

Tests:
db845c:/sys/bus/coresight/devices # echo 1 > tmc_etr0/enable_sink
db845c:/sys/bus/coresight/devices # echo 1 > etm1/enable_source
db845c:/sys/bus/coresight/devices # rmmod coresight_cti
rmmod: failed to unload coresight_cti: Try again
1|db845c:/sys/bus/coresight/devices # echo 0 > etm1/enable_source
db845c:/sys/bus/coresight/devices # rmmod coresight_cti
db845c:/sys/bus/coresight/devices # echo 1 > etm1/enable_source
db845c:/sys/bus/coresight/devices # insmod /data/modules/coresight-cti.ko
db845c:/sys/bus/coresight/devices # echo 1 > etm2/enable_source
db845c:/sys/bus/coresight/devices # echo 0 > etm1/enable_source
db845c:/sys/bus/coresight/devices # rmmod coresight_cti
db845c:/sys/bus/coresight/devices # 

Above module removal should fail since etm2 is still active now.

> 
>     /* output warning if ECT enable is preventing trace operation */
>     if (ect_ret)
>         dev_info(&csdev->dev, "Associated ECT device (%s) %s failed\n",
>              dev_name(&ect_csdev->dev),
>              enable ? "enable" : "disable");
>     return ect_ret;
> }
> -----------------------------------------------------------------------------------------
> Tests:-
> -----------------------
> root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-start.bash
> Tracing via sysfs
> enabling etf
> wait before enable etm
> enabling etm 0.
> enabling etm 1.
> enabling etm 2.
> enabling etm 3.
> waiting for trace
> root@linaro-developer:/home/linaro/scripts# rmmod 
> ../cs-mods/coresight-etm4x.ko
> rmmod: ERROR: Module coresight_etm4x is in use
> root@linaro-developer:/home/linaro/scripts# rmmod 
> ../cs-mods/coresight-cti.ko
> rmmod: ERROR: Module coresight_cti is in use
> root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-end.bash
> ending trace
> disabling etm 0.
> disabling etm 1.
> disabling etm 2.
> disabling etm 3.
> disabling etf
> 16+0 records in
> 16+0 records out
> 8192 bytes (8.2 kB) copied, 0.000834116 s, 9.8 MB/s
> root@linaro-developer:/home/linaro/scripts# rmmod 
> ../cs-mods/coresight-cti.ko
> root@linaro-developer:/home/linaro/scripts# rmmod 
> ../cs-mods/coresight-etm4x.ko
> root@linaro-developer:/home/linaro/scripts# insmod 
> ../cs-mods/coresight-etm4x.ko
> root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-start.bash
> Tracing via sysfs
> enabling etf
> wait before enable etm
> enabling etm 0.
> enabling etm 1.
> enabling etm 2.
> enabling etm 3.
> waiting for trace
> root@linaro-developer:/home/linaro/scripts# rmmod 
> ../cs-mods/coresight-etm4x.ko
> rmmod: ERROR: Module coresight_etm4x is in use
> root@linaro-developer:/home/linaro/scripts# rmmod 
> ../cs-mods/coresight-cti.ko
> rmmod: ERROR: Module coresight_cti is not currently loaded
> root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-end.bash
> ending trace
> disabling etm 0.
> disabling etm 1.
> disabling etm 2.
> disabling etm 3.
> disabling etf
> 16+0 records in
> 16+0 records out
> 8192 bytes (8.2 kB) copied, 0.000819117 s, 10.0 MB/s
> 
> }
> 
> 
> 
> > > Moving forwards we will see users who simply use pre-programmed
> > > settings from client programs such as perf. It may be that some users
> > > do not have the credentials to add and remove modules - this could be
> > > an admin function where larger systems with multiple sessions are in
> > > use.
> > >
> > >
> > > Regards
> > >
> > > Mike
> > >
> > >
> > > >
> > > > Thanks, Tingwei
> > > >
> > > > > Regards
> > > > >
> > > > > Mike
> > > > >
> > > > > > Let me know if you still think we need hold the module reference.
> > > > > >
> > > > > > Thanks, Tingwei
> > > > > > > Regards
> > > > > > >
> > > > > > > Mike
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Mike Leach
> > > > > > > Principal Engineer, ARM Ltd.
> > > > > > > Manchester Design Centre. UK
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > linux-arm-kernel mailing list
> > > > > > > linux-arm-kernel@lists.infradead.org
> > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Mike Leach
> > > > > Principal Engineer, ARM Ltd.
> > > > > Manchester Design Centre. UK
> > > > >
> > > > > _______________________________________________
> > > > > linux-arm-kernel mailing list
> > > > > linux-arm-kernel@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > >
> > >
> > >
> > > --
> > > Mike Leach
> > > Principal Engineer, ARM Ltd.
> > > Manchester Design Centre. UK
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> 
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK

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

Re: [PATCH v4 06/20] coresight: add try_get_module() in coresight_grab_device()

[Mike Leach]

Hi Tingwei,

On Tue, 28 Jul 2020 at 12:17, Tingwei Zhang <tingweiz@codeaurora.org> wrote:
>
> Hi Mike,
> On Tue, Jul 28, 2020 at 06:08:37PM +0800, Mike Leach wrote:
> > Hi Tingwei,
> >
> > On Tue, 28 Jul 2020 at 02:07, Tingwei Zhang <tingweiz@codeaurora.org> wrote:
> > >
> > > Hi Mike,
> > >
> > > On Tue, Jul 28, 2020 at 01:04:21AM +0800, Mike Leach wrote:
> > > > Hi Tingwei,
> > > >
> > > > On Sun, 26 Jul 2020 at 02:32, Tingwei Zhang <tingweiz@codeaurora.org>
> > > > wrote:
> > > > >
> > > > > Hi Mike,
> > > > > On Sat, Jul 25, 2020 at 09:51:31PM +0800, Mike Leach wrote:
> > > > > > Hi Tingwei,
> > > > > >
> > > > > > On Sat, 25 Jul 2020 at 11:05, Tingwei Zhang
> > > > > > <tingweiz@codeaurora.org>
> > > > > > wrote:
> > > > > > >
> > > > > > > Hi Mike,
> > > > > > >
> > > > > > >  Thu, Jul 23, 2020 at 06:55:47PM +0800, Mike Leach wrote:
> > > > > > > > Hi Tingwei,
> > > > > > > >
> > > > > > > > On Thu, 23 Jul 2020 at 05:28, Tingwei Zhang
> > > > <tingwei@codeaurora.org>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > When coresight device is in an active session, driver module
> > > > > > > > > of
> > > > > > > > > that device should not be removed. Use try_get_module() in
> > > > > > > > > coresight_grab_device() to prevent module to be unloaded.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Tingwei Zhang <tingwei@codeaurora.org>
> > > > > > > > > ---
> > > > > > > > >  drivers/hwtracing/coresight/coresight.c | 27
> > > > > > +++++++++++++++++++++----
> > > > > > > > >  1 file changed, 23 insertions(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/hwtracing/coresight/coresight.c
> > > > > > > > b/drivers/hwtracing/coresight/coresight.c
> > > > > > > > > index b7151c5f81b1..17bc76ea86ae 100644
> > > > > > > > > --- a/drivers/hwtracing/coresight/coresight.c
> > > > > > > > > +++ b/drivers/hwtracing/coresight/coresight.c
> > > > > > > > > @@ -640,7 +640,7 @@ struct coresight_device
> > > > > > > > *coresight_get_sink_by_id(u32 id)
> > > > > > > > >   * 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;
> > > > > > > > >
> > > > > > > > > @@ -648,10 +648,25 @@ static void coresight_grab_device(struct
> > > > > > > > coresight_device *csdev)
> > > > > > > > >                 struct coresight_device *child;
> > > > > > > > >
> > > > > > > > >                 child  = csdev->pdata->conns[i].child_dev;
> > > > > > > > > -               if (child && child->type ==
> > > > > > CORESIGHT_DEV_TYPE_HELPER)
> > > > > > > > > +               if (child && child->type ==
> > > > > > CORESIGHT_DEV_TYPE_HELPER) {
> > > > > > > > > +                       if
> > > > > > > > (!try_module_get(child->dev.parent->driver->owner))
> > > > > > > > > +                               goto err;
> > > > > > > > >
> > > > > > > > > pm_runtime_get_sync(child->dev.parent);
> > > > > > > > > +               }
> > > > > > > > >         }
> > > > > > > > > +       if (!try_module_get(csdev->dev.parent->driver->owner))
> > > > > > > > > +               goto err;
> > > > > > > > >         pm_runtime_get_sync(csdev->dev.parent);
> > > > > > > > > +       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)
> > > > > > > > > +                       pm_runtime_put(child->dev.parent);
> > > > > > > > > +       }
> > > > > > > > > +       return -ENODEV;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > >  /*
> > > > > > > > > @@ -663,12 +678,15 @@ static void coresight_drop_device(struct
> > > > > > > > coresight_device *csdev)
> > > > > > > > >         int i;
> > > > > > > > >
> > > > > > > > >         pm_runtime_put(csdev->dev.parent);
> > > > > > > > > +       module_put(csdev->dev.parent->driver->owner);
> > > > > > > > >         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)
> > > > > > > > > +               if (child && child->type ==
> > > > > > CORESIGHT_DEV_TYPE_HELPER) {
> > > > > > > > >                         pm_runtime_put(child->dev.parent);
> > > > > > > > > +
> > > > > > module_put(child->dev.parent->driver->owner);
> > > > > > > > > +               }
> > > > > > > > >         }
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > @@ -721,7 +739,8 @@ static int _coresight_build_path(struct
> > > > > > > > coresight_device *csdev,
> > > > > > > > >         if (!node)
> > > > > > > > >                 return -ENOMEM;
> > > > > > > > >
> > > > > > > > > -       coresight_grab_device(csdev);
> > > > > > > > > +       if (coresight_grab_device(csdev))
> > > > > > > > > +               return -ENODEV;
> > > > > > > > >         node->csdev = csdev;
> > > > > > > > >         list_add(&node->link, path);
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > The Qualcomm Innovation Center, Inc. is a member of the Code
> > > > Aurora
> > > > > > > > Forum,
> > > > > > > > > a Linux Foundation Collaborative Project
> > > > > > > > >
> > > > > > > >
> > > > > > > > The CTI devices are associated with other coresight components
> > > > using
> > > > > > > > csdev->ect_dev; These are not handled in the main linear trace
> > > > path as
> > > > > > > > these have a star topology interlinking all components.
> > > > > > > > However, when a component has an associated ect_dev then is
> > > > enabled at
> > > > > > > > the same time as the other component is enabled.
> > > > > > > >
> > > > > > > > So a module_get/put will be needed for this association to
> > > > > > > > prevent
> > > > the
> > > > > > > > CTI being removed while a trace session is active.
> > > > > > > > I suggest in coresight.c : coresight_control_assoc_ectdev()
> > > > > > > >
> > > > > > >
> > > > > > > In module unload process, devices of that module will be removed.
> > > > > > > In
> > > > > > > this case, cti_remove() is called on all cti device when unloading
> > > > > > > cti module. All cti connections is cleaned at that time.
> > > > > > > csdev->ect_dev is set to NULL. coresight_control_assoc_ectdev()
> > > > > > > will return since since ect_csdev is NULL.
> > > > > > >
> > > > > > > I think we are safe without holding module reference since cti
> > > > > > > driver has done a pretty good job on clean up.
> > > > > > >
> > > > > >
> > > > > > The issue here is not about clean up. We need to keep all the
> > > > > > programmed coresight modules loaded and operational while a
> > > > > > coresight
> > > > > > trace session is in progress. The CTI can be used to control the
> > > > > > generation of trace, trigger trace related events etc.
> > > > > > If the module is removed while a session is in progress then this
> > > > > > programming is lost and the trace data collected may not be what is
> > > > > > expected.
> > > > > >
> > > > >
> > > > > Got your point now. In my opinion, CTI is kinda different to other
> > > > > coresight components since it's optional.
> > > > >
> > > > > For other components, they have to been there when path is built or
> > > > > the enablement fails. Use can either get a successfully return which
> > > > > means it's good and all devices/modules are there until path is
> > > > > released, or a failed return which means trace session can't be
> > > > > setup.
> > > > >
> > > >
> > > > The module get/put do more than ensure that the system can be enabled.
> > > > They ensure that no components in the coresight path can be unloaded
> > > > until all paths in use are released. This ensures that clients do not
> > > > get the trace system pulled out from underneath - corrupting trace
> > > > data, and possibly affecting the client program.
> > > >
> > >
> > > I agree.
> > >
> > > > > In CTI case, there's no garrantee that CTI related to the trace
> > > > > session is there even the enablement is successful.
> > > >
> > > > And there is no guarantee that it is not programmed to perform a key
> > > > task in controlling the generation of trace.
> > > >
> > > > The entire programmed coresight system must be protected when in use.
> > > > That includes CTI devices. Otherwise the integrity of the captured
> > > > trace could be compromised, and indeed there may be no trace captured
> > > > if this is relying on CTI triggers.
> > > >
> > >
> > > That's correct.  If CTI is not there, the trace may be wrong or no
> > > trace as all.
> > >
> > > > >For example,
> > > > > One cti is required for ETM tracing session. If cti module is
> > > > > unloaded before enable that trace session. The enablement will
> > > > > still be successfully done since there's no ect_dev connected
> > > > > at all.
> > > > >
> > > > > My point is holding cti module reference in enable path won't
> > > > > fix all the problem. Altenatively, user should be aware that
> > > > > unload cti module leads to all cti functioniol failure.
> > > > > With current design, the behavior is consistant on cti. User
> > > > > can unload cti module whenever he wants but it will break
> > > > > the function.
> > > >
> > > > Precisely - and this is what should never be allowed - why can the CTI
> > > > be used to break the trace system but not other components?
> > > >
> > >
> >
> > Sorry - I may not have been clear here - I do not want to make having
> > a CTI mandatory for enabling the trace session - this is an optional
> > component.
> >
> > > We can't prevent user to unload CTI module when there's no active
> > > trace session.
> >
> > Agreed - the same applies to all the other components.
> >
> > > If we don't allow CTI to be not available, we need to
> > > check whether CTI is there in build_path routine or coresight enable
> > > routine. We need return failure if CTI is not there when user enables
> > > the trace session. Currently, coresight framework check associate CTI
> > > device, but if CTI deivce is not probed due to probe failure or
> > > module unload, coresight framework assume there's no assocaite CTI
> > > device and just continue the enablement.  It won't return failure.
> > >
> > > If we have plan to change CTI check to be mandatory, make the coresight
> > > enablement fail if CTI is not there like other trace components.  I'm
> > > perfect fine to add module_get/module_put for CTI.
> > >
> >
> > If the CTI is present at the time the trace path is created, then it
> > must be enabled, and a failure to enable the CTI fails the entire path
> > enable - this is the operation that is implemented by
> > coresight_control_assoc_ectdev() at this time.
> > If a CTI is not present at the time the trace path is created, then as
> > you have said previously, the ect_dev pointer will be NULL and the
> > check and enable of CTI will be skipped by
> > coresight_control_assoc_ectdev() and have no effect on the trace path.
> >
> > For the module version, if CTI is present and enabled then the module
> > must be locked so it cannot be removed while the trace is active -
> > this is the only change that is required. So adding get / put module
> > in coresight_control_assoc_ectdev() will lock the module in for the
> > duration that trace is active.
> >
> > I've tested the code below to ensure that the cti module is correctly
> > locked while trace is active, but can be removed as required when
> > trace is inactive.
> >
>
> Thanks a lot for coming up with the patch and testing it, Mike.
>
> > Regards
> >
> > Mike
> >
> > ---------------------------------------------------------------------------------
> > static int
> > coresight_control_assoc_ectdev(struct coresight_device *csdev, bool enable)
> > {
> >     int ect_ret = 0;
> >     struct coresight_device *ect_csdev = csdev->ect_dev;
> >     struct module *mod;
> >
> >     if (!ect_csdev)
> >         return 0;
> >
> >     if ((!ect_ops(ect_csdev)->enable) || (!ect_ops(ect_csdev)->disable))
> >         return 0;
> >
> >     mod = ect_csdev->dev.parent->driver->owner;
> >
> >     if (enable) {
> >         if (try_module_get(mod)) {
> >             ect_ret = ect_ops(ect_csdev)->enable(ect_csdev);
> >             if (ect_ret)
> >                 module_put(mod);
> >         } else
> >             ect_ret = -ENODEV;
> >     } else {
> >         module_put(mod);
> >         ect_ret = ect_ops(ect_csdev)->disable(ect_csdev);
> >     }
>
> If CTI module is removed before enablement of ETM and installed after ETM
> enablement and before ETM disablement. We have a unbalanced module
> reference number here. I made one test to prove this with above code.
>
> We may need move module_put() after disable(). Return proper error from
> disable() to indicate CTI was not enabled before so we can skip
> module_put().
>
> Tests:
> db845c:/sys/bus/coresight/devices # echo 1 > tmc_etr0/enable_sink
> db845c:/sys/bus/coresight/devices # echo 1 > etm1/enable_source
> db845c:/sys/bus/coresight/devices # rmmod coresight_cti
> rmmod: failed to unload coresight_cti: Try again
> 1|db845c:/sys/bus/coresight/devices # echo 0 > etm1/enable_source
> db845c:/sys/bus/coresight/devices # rmmod coresight_cti
> db845c:/sys/bus/coresight/devices # echo 1 > etm1/enable_source
> db845c:/sys/bus/coresight/devices # insmod /data/modules/coresight-cti.ko
> db845c:/sys/bus/coresight/devices # echo 1 > etm2/enable_source
> db845c:/sys/bus/coresight/devices # echo 0 > etm1/enable_source
> db845c:/sys/bus/coresight/devices # rmmod coresight_cti
> db845c:/sys/bus/coresight/devices #
>
> Above module removal should fail since etm2 is still active now.
>

Good point. Not sure this is sensible user operation, but it is
something that needs to be handled.
The issue goes beyond just the module count - the CTI driver has an
internal enable/disable refcount that is affected by this too.

I think the best way to handle this late CTI load is to flag which
Coresight devices have actually enabled the CTI, and only disable for
these.
So given a new flag in coresight_device, the following is possible:-

    if (enable) {
        if (try_module_get(mod)) {
            ect_ret = ect_ops(ect_csdev)->enable(ect_csdev);
            if (ect_ret)
                module_put(mod);
            else
                csdev->ect_enable = true;
        } else
            ect_ret = -ENODEV;
    } else {
        if (csdev->ect_enable) {
            module_put(mod);
            ect_ret = ect_ops(ect_csdev)->disable(ect_csdev);
            csdev->ect_enable = false;
        }
    }

During testing I have found that removing the etm4x module before the
cti module results in a crash. This is due to a bug in the cti driver
callback that removes sysfs links - and an issue in the coresight-core
where that callback is called.
I'll forward detail for all this tomorrow.

Regards

Mike





> >
> >     /* output warning if ECT enable is preventing trace operation */
> >     if (ect_ret)
> >         dev_info(&csdev->dev, "Associated ECT device (%s) %s failed\n",
> >              dev_name(&ect_csdev->dev),
> >              enable ? "enable" : "disable");
> >     return ect_ret;
> > }
> > -----------------------------------------------------------------------------------------
> > Tests:-
> > -----------------------
> > root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-start.bash
> > Tracing via sysfs
> > enabling etf
> > wait before enable etm
> > enabling etm 0.
> > enabling etm 1.
> > enabling etm 2.
> > enabling etm 3.
> > waiting for trace
> > root@linaro-developer:/home/linaro/scripts# rmmod
> > ../cs-mods/coresight-etm4x.ko
> > rmmod: ERROR: Module coresight_etm4x is in use
> > root@linaro-developer:/home/linaro/scripts# rmmod
> > ../cs-mods/coresight-cti.ko
> > rmmod: ERROR: Module coresight_cti is in use
> > root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-end.bash
> > ending trace
> > disabling etm 0.
> > disabling etm 1.
> > disabling etm 2.
> > disabling etm 3.
> > disabling etf
> > 16+0 records in
> > 16+0 records out
> > 8192 bytes (8.2 kB) copied, 0.000834116 s, 9.8 MB/s
> > root@linaro-developer:/home/linaro/scripts# rmmod
> > ../cs-mods/coresight-cti.ko
> > root@linaro-developer:/home/linaro/scripts# rmmod
> > ../cs-mods/coresight-etm4x.ko
> > root@linaro-developer:/home/linaro/scripts# insmod
> > ../cs-mods/coresight-etm4x.ko
> > root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-start.bash
> > Tracing via sysfs
> > enabling etf
> > wait before enable etm
> > enabling etm 0.
> > enabling etm 1.
> > enabling etm 2.
> > enabling etm 3.
> > waiting for trace
> > root@linaro-developer:/home/linaro/scripts# rmmod
> > ../cs-mods/coresight-etm4x.ko
> > rmmod: ERROR: Module coresight_etm4x is in use
> > root@linaro-developer:/home/linaro/scripts# rmmod
> > ../cs-mods/coresight-cti.ko
> > rmmod: ERROR: Module coresight_cti is not currently loaded
> > root@linaro-developer:/home/linaro/scripts# ./trace-sysfs-end.bash
> > ending trace
> > disabling etm 0.
> > disabling etm 1.
> > disabling etm 2.
> > disabling etm 3.
> > disabling etf
> > 16+0 records in
> > 16+0 records out
> > 8192 bytes (8.2 kB) copied, 0.000819117 s, 10.0 MB/s
> >
> > }
> >
> >
> >
> > > > Moving forwards we will see users who simply use pre-programmed
> > > > settings from client programs such as perf. It may be that some users
> > > > do not have the credentials to add and remove modules - this could be
> > > > an admin function where larger systems with multiple sessions are in
> > > > use.
> > > >
> > > >
> > > > Regards
> > > >
> > > > Mike
> > > >
> > > >
> > > > >
> > > > > Thanks, Tingwei
> > > > >
> > > > > > Regards
> > > > > >
> > > > > > Mike
> > > > > >
> > > > > > > Let me know if you still think we need hold the module reference.
> > > > > > >
> > > > > > > Thanks, Tingwei
> > > > > > > > Regards
> > > > > > > >
> > > > > > > > Mike
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > Mike Leach
> > > > > > > > Principal Engineer, ARM Ltd.
> > > > > > > > Manchester Design Centre. UK
> > > > > > > >
> > > > > > > > _______________________________________________
> > > > > > > > linux-arm-kernel mailing list
> > > > > > > > linux-arm-kernel@lists.infradead.org
> > > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Mike Leach
> > > > > > Principal Engineer, ARM Ltd.
> > > > > > Manchester Design Centre. UK
> > > > > >
> > > > > > _______________________________________________
> > > > > > linux-arm-kernel mailing list
> > > > > > linux-arm-kernel@lists.infradead.org
> > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > > >
> > > >
> > > >
> > > > --
> > > > Mike Leach
> > > > Principal Engineer, ARM Ltd.
> > > > Manchester Design Centre. UK
> > > >
> > > > _______________________________________________
> > > > linux-arm-kernel mailing list
> > > > linux-arm-kernel@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> >
> >
> > --
> > Mike Leach
> > Principal Engineer, ARM Ltd.
> > Manchester Design Centre. UK



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

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