coresight_panic_cb is called with interrupts disabled during panics.
However, bus_for_each_dev calls bus_to_subsys which takes
bus_kset->list_lock without disabling IRQs. This may cause a deadlock.

Instead of adding a panic API for coresight devices, just have coresight
drivers that need a panic callback register directly with the panic
notifier.

Fixes: 46006ceb5d02 ("coresight: core: Add provision for panic callbacks")
Fixes: 6dbcbcfc4496 ("coresight: tmc: Enable panic sync handling")
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

Changes in v3:
- Rewrite patch to remove the panic sync API entirely

Changes in v2:
- Add a comment describing csdev_lock/list
- Consolidate list removal in coresight_device_release

 drivers/hwtracing/coresight/coresight-core.c  | 42 -------------------
 .../hwtracing/coresight/coresight-tmc-core.c  | 20 ++++++++-
 .../hwtracing/coresight/coresight-tmc-etf.c   | 12 +++---
 .../hwtracing/coresight/coresight-tmc-etr.c   | 12 +++---
 drivers/hwtracing/coresight/coresight-tmc.h   | 10 ++++-
 include/linux/coresight.h                     | 11 +----
 6 files changed, 38 insertions(+), 69 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index fa758cc21827..297af270bf3d 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -19,7 +19,6 @@
 #include <linux/property.h>
 #include <linux/delay.h>
 #include <linux/pm_runtime.h>
-#include <linux/panic_notifier.h>
 
 #include "coresight-etm-perf.h"
 #include "coresight-priv.h"
@@ -1563,36 +1562,6 @@ const struct bus_type coresight_bustype = {
 	.name	= "coresight",
 };
 
-static int coresight_panic_sync(struct device *dev, void *data)
-{
-	int mode;
-	struct coresight_device *csdev;
-
-	/* Run through panic sync handlers for all enabled devices */
-	csdev = container_of(dev, struct coresight_device, dev);
-	mode = coresight_get_mode(csdev);
-
-	if ((mode == CS_MODE_SYSFS) || (mode == CS_MODE_PERF)) {
-		if (panic_ops(csdev))
-			panic_ops(csdev)->sync(csdev);
-	}
-
-	return 0;
-}
-
-static int coresight_panic_cb(struct notifier_block *self,
-			       unsigned long v, void *p)
-{
-	bus_for_each_dev(&coresight_bustype, NULL, NULL,
-				 coresight_panic_sync);
-
-	return 0;
-}
-
-static struct notifier_block coresight_notifier = {
-	.notifier_call = coresight_panic_cb,
-};
-
 static int __init coresight_init(void)
 {
 	int ret;
@@ -1605,20 +1574,11 @@ static int __init coresight_init(void)
 	if (ret)
 		goto exit_bus_unregister;
 
-	/* Register function to be called for panic */
-	ret = atomic_notifier_chain_register(&panic_notifier_list,
-					     &coresight_notifier);
-	if (ret)
-		goto exit_perf;
-
 	/* initialise the coresight syscfg API */
 	ret = cscfg_init();
 	if (!ret)
 		return 0;
 
-	atomic_notifier_chain_unregister(&panic_notifier_list,
-					     &coresight_notifier);
-exit_perf:
 	etm_perf_exit();
 exit_bus_unregister:
 	bus_unregister(&coresight_bustype);
@@ -1628,8 +1588,6 @@ static int __init coresight_init(void)
 static void __exit coresight_exit(void)
 {
 	cscfg_exit();
-	atomic_notifier_chain_unregister(&panic_notifier_list,
-					     &coresight_notifier);
 	etm_perf_exit();
 	bus_unregister(&coresight_bustype);
 }
diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
index 88afb16bb6be..012f76dbf7d3 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-core.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
@@ -16,6 +16,7 @@
 #include <linux/fs.h>
 #include <linux/miscdevice.h>
 #include <linux/mutex.h>
+#include <linux/panic_notifier.h>
 #include <linux/property.h>
 #include <linux/uaccess.h>
 #include <linux/slab.h>
@@ -834,6 +835,7 @@ static int __tmc_probe(struct device *dev, struct resource *res)
 		desc.type = CORESIGHT_DEV_TYPE_SINK;
 		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM;
 		desc.ops = &tmc_etr_cs_ops;
+		drvdata->panic_notifier.notifier_call = tmc_panic_sync_etr;
 		ret = tmc_etr_setup_caps(dev, devid, &desc.access);
 		if (ret)
 			goto out;
@@ -847,6 +849,7 @@ static int __tmc_probe(struct device *dev, struct resource *res)
 		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
 		desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO;
 		desc.ops = &tmc_etf_cs_ops;
+		drvdata->panic_notifier.notifier_call = tmc_panic_sync_etf;
 		dev_list = &etf_devs;
 		break;
 	default:
@@ -869,11 +872,18 @@ static int __tmc_probe(struct device *dev, struct resource *res)
 	dev->platform_data = pdata;
 	desc.pdata = pdata;
 
+	if (drvdata->panic_notifier.notifier_call) {
+		ret = atomic_notifier_chain_register(&panic_notifier_list,
+						     &drvdata->panic_notifier);
+		if (ret)
+			goto out;
+	}
+
 	coresight_clear_self_claim_tag(&desc.access);
 	drvdata->csdev = coresight_register(&desc);
 	if (IS_ERR(drvdata->csdev)) {
 		ret = PTR_ERR(drvdata->csdev);
-		goto out;
+		goto err;
 	}
 
 	drvdata->miscdev.name = desc.name;
@@ -882,7 +892,10 @@ static int __tmc_probe(struct device *dev, struct resource *res)
 	ret = misc_register(&drvdata->miscdev);
 	if (ret) {
 		coresight_unregister(drvdata->csdev);
-		goto out;
+err:
+		if (drvdata->panic_notifier.notifier_call)
+			atomic_notifier_chain_unregister(&panic_notifier_list,
+							 &drvdata->panic_notifier);
 	}
 
 out:
@@ -944,6 +957,9 @@ static void __tmc_remove(struct device *dev)
 	if (drvdata->crashdev.fops)
 		misc_deregister(&drvdata->crashdev);
 	coresight_unregister(drvdata->csdev);
+	if (drvdata->panic_notifier.notifier_call)
+		atomic_notifier_chain_unregister(&panic_notifier_list,
+						 &drvdata->panic_notifier);
 }
 
 static void tmc_remove(struct amba_device *adev)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 0f45ab5e5249..9c8dfd5436ab 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -601,11 +601,14 @@ static unsigned long tmc_update_etf_buffer(struct coresight_device *csdev,
 	return to_read;
 }
 
-static int tmc_panic_sync_etf(struct coresight_device *csdev)
+int tmc_panic_sync_etf(struct notifier_block *nb, unsigned long action,
+		       void *data)
 {
 	u32 val;
 	struct tmc_crash_metadata *mdata;
-	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+	struct tmc_drvdata *drvdata = container_of(nb, struct tmc_drvdata,
+						   panic_notifier);
+	struct coresight_device *csdev = drvdata->csdev;
 
 	mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
 
@@ -689,10 +692,6 @@ static const struct coresight_ops_link tmc_etf_link_ops = {
 	.disable	= tmc_disable_etf_link,
 };
 
-static const struct coresight_ops_panic tmc_etf_sync_ops = {
-	.sync		= tmc_panic_sync_etf,
-};
-
 const struct coresight_ops tmc_etb_cs_ops = {
 	.sink_ops	= &tmc_etf_sink_ops,
 };
@@ -700,7 +699,6 @@ const struct coresight_ops tmc_etb_cs_ops = {
 const struct coresight_ops tmc_etf_cs_ops = {
 	.sink_ops	= &tmc_etf_sink_ops,
 	.link_ops	= &tmc_etf_link_ops,
-	.panic_ops	= &tmc_etf_sync_ops,
 };
 
 int tmc_read_prepare_etb(struct tmc_drvdata *drvdata)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index b07fcdb3fe1a..91061462cd92 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1824,11 +1824,14 @@ static int tmc_disable_etr_sink(struct coresight_device *csdev)
 	return 0;
 }
 
-static int tmc_panic_sync_etr(struct coresight_device *csdev)
+int tmc_panic_sync_etr(struct notifier_block *nb, unsigned long action,
+		       void *data)
 {
 	u32 val;
 	struct tmc_crash_metadata *mdata;
-	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+	struct tmc_drvdata *drvdata = container_of(nb, struct tmc_drvdata,
+						   panic_notifier);
+	struct coresight_device *csdev = drvdata->csdev;
 
 	mdata = (struct tmc_crash_metadata *)drvdata->crash_mdata.vaddr;
 
@@ -1900,13 +1903,8 @@ static const struct coresight_ops_sink tmc_etr_sink_ops = {
 	.free_buffer	= tmc_free_etr_buffer,
 };
 
-static const struct coresight_ops_panic tmc_etr_sync_ops = {
-	.sync		= tmc_panic_sync_etr,
-};
-
 const struct coresight_ops tmc_etr_cs_ops = {
 	.sink_ops	= &tmc_etr_sink_ops,
-	.panic_ops	= &tmc_etr_sync_ops,
 };
 
 int tmc_read_prepare_etr(struct tmc_drvdata *drvdata)
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 6541a27a018e..d8e6bf5526c6 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -242,6 +242,7 @@ struct tmc_resrv_buf {
  *		(after crash) by default.
  * @crash_mdata: Reserved memory for storing tmc crash metadata.
  *		 Used by ETR/ETF.
+ * @panic_notifier: Notifier block used to clean up during a panic
  */
 struct tmc_drvdata {
 	struct clk		*pclk;
@@ -271,6 +272,7 @@ struct tmc_drvdata {
 	struct etr_buf		*perf_buf;
 	struct tmc_resrv_buf	resrv_buf;
 	struct tmc_resrv_buf	crash_mdata;
+	struct notifier_block	panic_notifier;
 };
 
 struct etr_buf_operations {
@@ -322,18 +324,24 @@ void tmc_disable_hw(struct tmc_drvdata *drvdata);
 u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata);
 int tmc_read_prepare_crashdata(struct tmc_drvdata *drvdata);
 
+struct notifier_block;
+
 /* ETB/ETF functions */
 int tmc_read_prepare_etb(struct tmc_drvdata *drvdata);
 int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata);
+int tmc_panic_sync_etf(struct notifier_block *nb, unsigned long action,
+		       void *data);
 extern const struct coresight_ops tmc_etb_cs_ops;
 extern const struct coresight_ops tmc_etf_cs_ops;
-
 ssize_t tmc_etb_get_sysfs_trace(struct tmc_drvdata *drvdata,
 				loff_t pos, size_t len, char **bufpp);
+
 /* ETR functions */
 int tmc_read_prepare_etr(struct tmc_drvdata *drvdata);
 int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata);
 void tmc_etr_disable_hw(struct tmc_drvdata *drvdata);
+int tmc_panic_sync_etr(struct notifier_block *nb, unsigned long action,
+		       void *data);
 extern const struct coresight_ops tmc_etr_cs_ops;
 ssize_t tmc_etr_get_sysfs_trace(struct tmc_drvdata *drvdata,
 				loff_t pos, size_t len, char **bufpp);
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index 4ac65c68bbf4..64e12ffd507b 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -302,6 +302,7 @@ struct coresight_device {
 	/* system configuration and feature lists */
 	struct list_head feature_csdev_list;
 	struct list_head config_csdev_list;
+	struct list_head csdev_list;
 	raw_spinlock_t cscfg_csdev_lock;
 	void *active_cscfg_ctxt;
 };
@@ -427,15 +428,6 @@ struct coresight_ops_helper {
 };
 
 
-/**
- * struct coresight_ops_panic - Generic device ops for panic handing
- *
- * @sync	: Sync the device register state/trace data
- */
-struct coresight_ops_panic {
-	int (*sync)(struct coresight_device *csdev);
-};
-
 struct coresight_ops {
 	int (*trace_id)(struct coresight_device *csdev, enum cs_mode mode,
 			struct coresight_device *sink);
@@ -443,7 +435,6 @@ struct coresight_ops {
 	const struct coresight_ops_link *link_ops;
 	const struct coresight_ops_source *source_ops;
 	const struct coresight_ops_helper *helper_ops;
-	const struct coresight_ops_panic *panic_ops;
 };
 
 static inline u32 csdev_access_relaxed_read32(struct csdev_access *csa,
-- 
2.35.1.1320.gc452695387.dirty

On Fri, Sep 12, 2025 at 11:13:14AM -0400, Sean Anderson wrote:
> coresight_panic_cb is called with interrupts disabled during panics.
> However, bus_for_each_dev calls bus_to_subsys which takes
> bus_kset->list_lock without disabling IRQs. This may cause a deadlock.

I would rephrase it to make it clearer for anyone reading it later:

  coresight_panic_cb() is called during panics, which can preempt a flow
  that triggers exceptions (such as data or instruction aborts).
  However, bus_for_each_dev() calls bus_to_subsys(), which takes
  'bus_kset->list_lock'. If a panic occurs after the lock has been
  acquired, it will cause a deadlock.

> Instead of adding a panic API for coresight devices, just have coresight
> drivers that need a panic callback register directly with the panic
> notifier.
> 
> Fixes: 46006ceb5d02 ("coresight: core: Add provision for panic callbacks")
> Fixes: 6dbcbcfc4496 ("coresight: tmc: Enable panic sync handling")

Let us fix the bug caused by the commit 46006ceb5d02. OTOH, I think we
should not touch change introduced in the commit 6dbcbcfc4496.

Please see details below.

> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
> Changes in v3:
> - Rewrite patch to remove the panic sync API entirely
> 
> Changes in v2:
> - Add a comment describing csdev_lock/list
> - Consolidate list removal in coresight_device_release
> 
>  drivers/hwtracing/coresight/coresight-core.c  | 42 -------------------
>  .../hwtracing/coresight/coresight-tmc-core.c  | 20 ++++++++-
>  .../hwtracing/coresight/coresight-tmc-etf.c   | 12 +++---
>  .../hwtracing/coresight/coresight-tmc-etr.c   | 12 +++---
>  drivers/hwtracing/coresight/coresight-tmc.h   | 10 ++++-
>  include/linux/coresight.h                     | 11 +----
>  6 files changed, 38 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index fa758cc21827..297af270bf3d 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -19,7 +19,6 @@
>  #include <linux/property.h>
>  #include <linux/delay.h>
>  #include <linux/pm_runtime.h>
> -#include <linux/panic_notifier.h>
>  
>  #include "coresight-etm-perf.h"
>  #include "coresight-priv.h"
> @@ -1563,36 +1562,6 @@ const struct bus_type coresight_bustype = {
>  	.name	= "coresight",
>  };
>  
> -static int coresight_panic_sync(struct device *dev, void *data)
> -{
> -	int mode;
> -	struct coresight_device *csdev;
> -
> -	/* Run through panic sync handlers for all enabled devices */
> -	csdev = container_of(dev, struct coresight_device, dev);
> -	mode = coresight_get_mode(csdev);
> -
> -	if ((mode == CS_MODE_SYSFS) || (mode == CS_MODE_PERF)) {
> -		if (panic_ops(csdev))
> -			panic_ops(csdev)->sync(csdev);
> -	}
> -
> -	return 0;
> -}
> -
> -static int coresight_panic_cb(struct notifier_block *self,
> -			       unsigned long v, void *p)
> -{
> -	bus_for_each_dev(&coresight_bustype, NULL, NULL,
> -				 coresight_panic_sync);
> -
> -	return 0;
> -}
> -
> -static struct notifier_block coresight_notifier = {
> -	.notifier_call = coresight_panic_cb,
> -};
> -
>  static int __init coresight_init(void)
>  {
>  	int ret;
> @@ -1605,20 +1574,11 @@ static int __init coresight_init(void)
>  	if (ret)
>  		goto exit_bus_unregister;
>  
> -	/* Register function to be called for panic */
> -	ret = atomic_notifier_chain_register(&panic_notifier_list,
> -					     &coresight_notifier);
> -	if (ret)
> -		goto exit_perf;
> -
>  	/* initialise the coresight syscfg API */
>  	ret = cscfg_init();
>  	if (!ret)
>  		return 0;
>  
> -	atomic_notifier_chain_unregister(&panic_notifier_list,
> -					     &coresight_notifier);
> -exit_perf:
>  	etm_perf_exit();
>  exit_bus_unregister:
>  	bus_unregister(&coresight_bustype);
> @@ -1628,8 +1588,6 @@ static int __init coresight_init(void)
>  static void __exit coresight_exit(void)
>  {
>  	cscfg_exit();
> -	atomic_notifier_chain_unregister(&panic_notifier_list,
> -					     &coresight_notifier);
>  	etm_perf_exit();
>  	bus_unregister(&coresight_bustype);
>  }

Removing panic notifier in coresight-core.c looks good to me.

> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> index 88afb16bb6be..012f76dbf7d3 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> @@ -16,6 +16,7 @@
>  #include <linux/fs.h>
>  #include <linux/miscdevice.h>
>  #include <linux/mutex.h>
> +#include <linux/panic_notifier.h>
>  #include <linux/property.h>
>  #include <linux/uaccess.h>
>  #include <linux/slab.h>
> @@ -834,6 +835,7 @@ static int __tmc_probe(struct device *dev, struct resource *res)
>  		desc.type = CORESIGHT_DEV_TYPE_SINK;
>  		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM;
>  		desc.ops = &tmc_etr_cs_ops;
> +		drvdata->panic_notifier.notifier_call = tmc_panic_sync_etr;
>  		ret = tmc_etr_setup_caps(dev, devid, &desc.access);
>  		if (ret)
>  			goto out;
> @@ -847,6 +849,7 @@ static int __tmc_probe(struct device *dev, struct resource *res)
>  		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
>  		desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO;
>  		desc.ops = &tmc_etf_cs_ops;
> +		drvdata->panic_notifier.notifier_call = tmc_panic_sync_etf;
>  		dev_list = &etf_devs;
>  		break;
>  	default:
> @@ -869,11 +872,18 @@ static int __tmc_probe(struct device *dev, struct resource *res)
>  	dev->platform_data = pdata;
>  	desc.pdata = pdata;
>  
> +	if (drvdata->panic_notifier.notifier_call) {
> +		ret = atomic_notifier_chain_register(&panic_notifier_list,
> +						     &drvdata->panic_notifier);
> +		if (ret)
> +			goto out;
> +	}

When I review this patch, I recognize we can consolidate panic notifier
in coresight-tmc-core.c, so we don't need to distribute the changes
into ETF and ETR drivers (sorry if I misled you in my previous reply).

How about we change like below? We still can keep the panic_ops
callback, and then register a common callback tmc_panic_cb(), which
still checks device state and invoke device's sync callback.

diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
index bd783c9dcb56..f7c175b3ae4c 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-core.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
@@ -21,6 +21,7 @@
 #include <linux/slab.h>
 #include <linux/dma-mapping.h>
 #include <linux/spinlock.h>
+#include <linux/panic_notifier.h>
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
@@ -769,6 +770,22 @@ static void register_crash_dev_interface(struct tmc_drvdata *drvdata,
 			"Valid crash tracedata found\n");
 }
 
+static int tmc_panic_cb(struct notifier_block *nb, unsigned long v, void *p)
+{
+	struct tmc_drvdata *drvdata = container_of(nb, struct tmc_drvdata,
+						   panic_notifier);
+	struct coresight_device *csdev = drvdata->csdev;
+	int mode = coresight_get_mode(csdev);
+
+	if (mode == CS_MODE_DISABLED)
+		return 0;
+
+	if (panic_ops(csdev))
+		panic_ops(csdev)->sync(csdev);
+
+	return 0;
+}
+
 static int __tmc_probe(struct device *dev, struct resource *res)
 {
 	int ret = 0;
@@ -886,6 +903,12 @@ static int __tmc_probe(struct device *dev, struct resource *res)
 		goto out;
 	}
 
+        if (panic_ops(drvdata->csdev)) {
+		drvdata->panic_notifier.notifier_call = tmc_panic_cb;
+		atomic_notifier_chain_register(&panic_notifier_list,
+					       &drvdata->panic_notifier);
+	}
+
 out:
 	if (is_tmc_crashdata_valid(drvdata) &&
 	    !tmc_prepare_crashdata(drvdata))
@@ -930,6 +953,10 @@ static void __tmc_remove(struct device *dev)
 {
 	struct tmc_drvdata *drvdata = dev_get_drvdata(dev);
 
+	if (panic_ops(drvdata->csdev))
+		atomic_notifier_chain_unregister(&panic_notifier_list,
+						 &drvdata->panic_notifier);
+
 	/*
 	 * Since misc_open() holds a refcount on the f_ops, which is
 	 * etb fops in this case, device is there until last file
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index cbb4ba439158..873c5427673c 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -243,6 +243,7 @@ struct tmc_resrv_buf {
  *		(after crash) by default.
  * @crash_mdata: Reserved memory for storing tmc crash metadata.
  *		 Used by ETR/ETF.
+ * @panic_notifier: Notifier used to clean up during a panic
  */
 struct tmc_drvdata {
 	struct clk		*atclk;
@@ -273,6 +274,7 @@ struct tmc_drvdata {
 	struct etr_buf		*perf_buf;
 	struct tmc_resrv_buf	resrv_buf;
 	struct tmc_resrv_buf	crash_mdata;
+	struct notifier_block	panic_notifier;
 };
 
 struct etr_buf_operations {

On 9/15/25 05:58, Leo Yan wrote:
> On Fri, Sep 12, 2025 at 11:13:14AM -0400, Sean Anderson wrote:
>> coresight_panic_cb is called with interrupts disabled during panics.
>> However, bus_for_each_dev calls bus_to_subsys which takes
>> bus_kset->list_lock without disabling IRQs. This may cause a deadlock.
> 
> I would rephrase it to make it clearer for anyone reading it later:
> 
>   coresight_panic_cb() is called during panics, which can preempt a flow
>   that triggers exceptions (such as data or instruction aborts).

I don't see what exceptions have to do with it. You can also panic
during a regular interrupt.

>   However, bus_for_each_dev() calls bus_to_subsys(), which takes
>   'bus_kset->list_lock'. If a panic occurs after the lock has been
>   acquired, it will cause a deadlock.
> 
>> Instead of adding a panic API for coresight devices, just have coresight
>> drivers that need a panic callback register directly with the panic
>> notifier.
>> 
>> Fixes: 46006ceb5d02 ("coresight: core: Add provision for panic callbacks")
>> Fixes: 6dbcbcfc4496 ("coresight: tmc: Enable panic sync handling")
> 
> Let us fix the bug caused by the commit 46006ceb5d02. OTOH, I think we
> should not touch change introduced in the commit 6dbcbcfc4496.

They are the same change. The API is introduced in one and the only users
are added in the other.

> Please see details below.
> 
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>> 
>> Changes in v3:
>> - Rewrite patch to remove the panic sync API entirely
>> 
>> Changes in v2:
>> - Add a comment describing csdev_lock/list
>> - Consolidate list removal in coresight_device_release
>> 
>>  drivers/hwtracing/coresight/coresight-core.c  | 42 -------------------
>>  .../hwtracing/coresight/coresight-tmc-core.c  | 20 ++++++++-
>>  .../hwtracing/coresight/coresight-tmc-etf.c   | 12 +++---
>>  .../hwtracing/coresight/coresight-tmc-etr.c   | 12 +++---
>>  drivers/hwtracing/coresight/coresight-tmc.h   | 10 ++++-
>>  include/linux/coresight.h                     | 11 +----
>>  6 files changed, 38 insertions(+), 69 deletions(-)
>> 
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
>> index fa758cc21827..297af270bf3d 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -19,7 +19,6 @@
>>  #include <linux/property.h>
>>  #include <linux/delay.h>
>>  #include <linux/pm_runtime.h>
>> -#include <linux/panic_notifier.h>
>>  
>>  #include "coresight-etm-perf.h"
>>  #include "coresight-priv.h"
>> @@ -1563,36 +1562,6 @@ const struct bus_type coresight_bustype = {
>>  	.name	= "coresight",
>>  };
>>  
>> -static int coresight_panic_sync(struct device *dev, void *data)
>> -{
>> -	int mode;
>> -	struct coresight_device *csdev;
>> -
>> -	/* Run through panic sync handlers for all enabled devices */
>> -	csdev = container_of(dev, struct coresight_device, dev);
>> -	mode = coresight_get_mode(csdev);
>> -
>> -	if ((mode == CS_MODE_SYSFS) || (mode == CS_MODE_PERF)) {
>> -		if (panic_ops(csdev))
>> -			panic_ops(csdev)->sync(csdev);
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>> -static int coresight_panic_cb(struct notifier_block *self,
>> -			       unsigned long v, void *p)
>> -{
>> -	bus_for_each_dev(&coresight_bustype, NULL, NULL,
>> -				 coresight_panic_sync);
>> -
>> -	return 0;
>> -}
>> -
>> -static struct notifier_block coresight_notifier = {
>> -	.notifier_call = coresight_panic_cb,
>> -};
>> -
>>  static int __init coresight_init(void)
>>  {
>>  	int ret;
>> @@ -1605,20 +1574,11 @@ static int __init coresight_init(void)
>>  	if (ret)
>>  		goto exit_bus_unregister;
>>  
>> -	/* Register function to be called for panic */
>> -	ret = atomic_notifier_chain_register(&panic_notifier_list,
>> -					     &coresight_notifier);
>> -	if (ret)
>> -		goto exit_perf;
>> -
>>  	/* initialise the coresight syscfg API */
>>  	ret = cscfg_init();
>>  	if (!ret)
>>  		return 0;
>>  
>> -	atomic_notifier_chain_unregister(&panic_notifier_list,
>> -					     &coresight_notifier);
>> -exit_perf:
>>  	etm_perf_exit();
>>  exit_bus_unregister:
>>  	bus_unregister(&coresight_bustype);
>> @@ -1628,8 +1588,6 @@ static int __init coresight_init(void)
>>  static void __exit coresight_exit(void)
>>  {
>>  	cscfg_exit();
>> -	atomic_notifier_chain_unregister(&panic_notifier_list,
>> -					     &coresight_notifier);
>>  	etm_perf_exit();
>>  	bus_unregister(&coresight_bustype);
>>  }
> 
> Removing panic notifier in coresight-core.c looks good to me.
> 
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
>> index 88afb16bb6be..012f76dbf7d3 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
>> @@ -16,6 +16,7 @@
>>  #include <linux/fs.h>
>>  #include <linux/miscdevice.h>
>>  #include <linux/mutex.h>
>> +#include <linux/panic_notifier.h>
>>  #include <linux/property.h>
>>  #include <linux/uaccess.h>
>>  #include <linux/slab.h>
>> @@ -834,6 +835,7 @@ static int __tmc_probe(struct device *dev, struct resource *res)
>>  		desc.type = CORESIGHT_DEV_TYPE_SINK;
>>  		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM;
>>  		desc.ops = &tmc_etr_cs_ops;
>> +		drvdata->panic_notifier.notifier_call = tmc_panic_sync_etr;
>>  		ret = tmc_etr_setup_caps(dev, devid, &desc.access);
>>  		if (ret)
>>  			goto out;
>> @@ -847,6 +849,7 @@ static int __tmc_probe(struct device *dev, struct resource *res)
>>  		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
>>  		desc.subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO;
>>  		desc.ops = &tmc_etf_cs_ops;
>> +		drvdata->panic_notifier.notifier_call = tmc_panic_sync_etf;
>>  		dev_list = &etf_devs;
>>  		break;
>>  	default:
>> @@ -869,11 +872,18 @@ static int __tmc_probe(struct device *dev, struct resource *res)
>>  	dev->platform_data = pdata;
>>  	desc.pdata = pdata;
>>  
>> +	if (drvdata->panic_notifier.notifier_call) {
>> +		ret = atomic_notifier_chain_register(&panic_notifier_list,
>> +						     &drvdata->panic_notifier);
>> +		if (ret)
>> +			goto out;
>> +	}
> 
> When I review this patch, I recognize we can consolidate panic notifier
> in coresight-tmc-core.c, so we don't need to distribute the changes
> into ETF and ETR drivers (sorry if I misled you in my previous reply).

And this kind of thing is why I went with the straightforward fix
initially. I do not want to bikeshed the extent that this gets removed.
IMO the whole "panic ops" stuff should be done directly with the panic
notifier, hence this patch. If you do not agree with that, then ack v2
and send a follow up of your own to fix it how you see fit.

--Sean

> How about we change like below? We still can keep the panic_ops
> callback, and then register a common callback tmc_panic_cb(), which
> still checks device state and invoke device's sync callback.
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> index bd783c9dcb56..f7c175b3ae4c 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> @@ -21,6 +21,7 @@
>  #include <linux/slab.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/spinlock.h>
> +#include <linux/panic_notifier.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> @@ -769,6 +770,22 @@ static void register_crash_dev_interface(struct tmc_drvdata *drvdata,
>  			"Valid crash tracedata found\n");
>  }
>  
> +static int tmc_panic_cb(struct notifier_block *nb, unsigned long v, void *p)
> +{
> +	struct tmc_drvdata *drvdata = container_of(nb, struct tmc_drvdata,
> +						   panic_notifier);
> +	struct coresight_device *csdev = drvdata->csdev;
> +	int mode = coresight_get_mode(csdev);
> +
> +	if (mode == CS_MODE_DISABLED)
> +		return 0;
> +
> +	if (panic_ops(csdev))
> +		panic_ops(csdev)->sync(csdev);
> +
> +	return 0;
> +}
> +
>  static int __tmc_probe(struct device *dev, struct resource *res)
>  {
>  	int ret = 0;
> @@ -886,6 +903,12 @@ static int __tmc_probe(struct device *dev, struct resource *res)
>  		goto out;
>  	}
>  
> +        if (panic_ops(drvdata->csdev)) {
> +		drvdata->panic_notifier.notifier_call = tmc_panic_cb;
> +		atomic_notifier_chain_register(&panic_notifier_list,
> +					       &drvdata->panic_notifier);
> +	}
> +
>  out:
>  	if (is_tmc_crashdata_valid(drvdata) &&
>  	    !tmc_prepare_crashdata(drvdata))
> @@ -930,6 +953,10 @@ static void __tmc_remove(struct device *dev)
>  {
>  	struct tmc_drvdata *drvdata = dev_get_drvdata(dev);
>  
> +	if (panic_ops(drvdata->csdev))
> +		atomic_notifier_chain_unregister(&panic_notifier_list,
> +						 &drvdata->panic_notifier);
> +
>  	/*
>  	 * Since misc_open() holds a refcount on the f_ops, which is
>  	 * etb fops in this case, device is there until last file
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index cbb4ba439158..873c5427673c 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -243,6 +243,7 @@ struct tmc_resrv_buf {
>   *		(after crash) by default.
>   * @crash_mdata: Reserved memory for storing tmc crash metadata.
>   *		 Used by ETR/ETF.
> + * @panic_notifier: Notifier used to clean up during a panic
>   */
>  struct tmc_drvdata {
>  	struct clk		*atclk;
> @@ -273,6 +274,7 @@ struct tmc_drvdata {
>  	struct etr_buf		*perf_buf;
>  	struct tmc_resrv_buf	resrv_buf;
>  	struct tmc_resrv_buf	crash_mdata;
> +	struct notifier_block	panic_notifier;
>  };
>  
>  struct etr_buf_operations {

On Mon, Sep 15, 2025 at 10:31:24AM -0400, Sean Anderson wrote:
> On 9/15/25 05:58, Leo Yan wrote:
> > On Fri, Sep 12, 2025 at 11:13:14AM -0400, Sean Anderson wrote:
> >> coresight_panic_cb is called with interrupts disabled during panics.
> >> However, bus_for_each_dev calls bus_to_subsys which takes
> >> bus_kset->list_lock without disabling IRQs. This may cause a deadlock.
> > 
> > I would rephrase it to make it clearer for anyone reading it later:
> > 
> >   coresight_panic_cb() is called during panics, which can preempt a flow
> >   that triggers exceptions (such as data or instruction aborts).
> 
> I don't see what exceptions have to do with it. You can also panic
> during a regular interrupt.

The commit mentioned "without disabling IRQs" gives the impression that
the deadlock is caused by IRQ-unsafe locking, which might mislead into
thinking why the issue cannot be fixed with IRQ-safe locking.

Regardless of whether IRQs are disabled, and regardless of the context
(interrupt, bottom-half, or normal thread), the conditions for the
deadlock are only about:

  (a) The bus lock has been acquired;
  (b) A panic is triggered to try to acquire the same lock.

[...]

> > When I review this patch, I recognize we can consolidate panic notifier
> > in coresight-tmc-core.c, so we don't need to distribute the changes
> > into ETF and ETR drivers (sorry if I misled you in my previous reply).
> 
> And this kind of thing is why I went with the straightforward fix
> initially. I do not want to bikeshed the extent that this gets removed.
> IMO the whole "panic ops" stuff should be done directly with the panic
> notifier, hence this patch. If you do not agree with that, then ack v2
> and send a follow up of your own to fix it how you see fit.

I would fix it in one go.

I agree with you that "the whole panic ops stuff should be done directly
with the panic". The only difference between us is that I would keep the
`panic_ops` callback. To me, this encapsulates panic callbacks into
different modules, to make the code more general.

Could you check if the drafted patch below looks good to you? If so, I
will send out a formal patch.

---8<---

From ea78dd22cbdd97f709c5991d5bd3be97be6e137e Mon Sep 17 00:00:00 2001
From: Sean Anderson <sean.anderson@linux.dev>
Date: Tue, 16 Sep 2025 16:03:58 +0100
Subject: [PATCH] coresight: Fix possible deadlock in coresight_panic_cb()

coresight_panic_cb() is called during a panic. It invokes
bus_for_each_dev(), which then calls bus_to_subsys() and takes the
'bus_kset->list_lock'. If a panic occurs after the lock has been
acquired, it can lead to a deadlock.

Instead of using a common panic notifier to iterate the bus, this commit
directly registers the TMC device's panic notifier. This avoids bus
iteration and effectively eliminates the race condition that could cause
the deadlock.

Fixes: 46006ceb5d02 ("coresight: core: Add provision for panic callbacks")
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
 drivers/hwtracing/coresight/coresight-core.c  | 42 -------------------
 .../hwtracing/coresight/coresight-tmc-core.c  | 26 ++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.h   |  2 +
 3 files changed, 28 insertions(+), 42 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 3267192f0c1c..cb0cc8d77056 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -21,7 +21,6 @@
 #include <linux/property.h>
 #include <linux/delay.h>
 #include <linux/pm_runtime.h>
-#include <linux/panic_notifier.h>
 
 #include "coresight-etm-perf.h"
 #include "coresight-priv.h"
@@ -1566,36 +1565,6 @@ const struct bus_type coresight_bustype = {
 	.name	= "coresight",
 };
 
-static int coresight_panic_sync(struct device *dev, void *data)
-{
-	int mode;
-	struct coresight_device *csdev;
-
-	/* Run through panic sync handlers for all enabled devices */
-	csdev = container_of(dev, struct coresight_device, dev);
-	mode = coresight_get_mode(csdev);
-
-	if ((mode == CS_MODE_SYSFS) || (mode == CS_MODE_PERF)) {
-		if (panic_ops(csdev))
-			panic_ops(csdev)->sync(csdev);
-	}
-
-	return 0;
-}
-
-static int coresight_panic_cb(struct notifier_block *self,
-			       unsigned long v, void *p)
-{
-	bus_for_each_dev(&coresight_bustype, NULL, NULL,
-				 coresight_panic_sync);
-
-	return 0;
-}
-
-static struct notifier_block coresight_notifier = {
-	.notifier_call = coresight_panic_cb,
-};
-
 static int __init coresight_init(void)
 {
 	int ret;
@@ -1608,20 +1577,11 @@ static int __init coresight_init(void)
 	if (ret)
 		goto exit_bus_unregister;
 
-	/* Register function to be called for panic */
-	ret = atomic_notifier_chain_register(&panic_notifier_list,
-					     &coresight_notifier);
-	if (ret)
-		goto exit_perf;
-
 	/* initialise the coresight syscfg API */
 	ret = cscfg_init();
 	if (!ret)
 		return 0;
 
-	atomic_notifier_chain_unregister(&panic_notifier_list,
-					     &coresight_notifier);
-exit_perf:
 	etm_perf_exit();
 exit_bus_unregister:
 	bus_unregister(&coresight_bustype);
@@ -1631,8 +1591,6 @@ static int __init coresight_init(void)
 static void __exit coresight_exit(void)
 {
 	cscfg_exit();
-	atomic_notifier_chain_unregister(&panic_notifier_list,
-					     &coresight_notifier);
 	etm_perf_exit();
 	bus_unregister(&coresight_bustype);
 }
diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
index 36599c431be6..108ed9daf56d 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-core.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
@@ -21,6 +21,7 @@
 #include <linux/slab.h>
 #include <linux/dma-mapping.h>
 #include <linux/spinlock.h>
+#include <linux/panic_notifier.h>
 #include <linux/pm_runtime.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
@@ -769,6 +770,21 @@ static void register_crash_dev_interface(struct tmc_drvdata *drvdata,
 			"Valid crash tracedata found\n");
 }
 
+static int tmc_panic_cb(struct notifier_block *nb, unsigned long v, void *p)
+{
+	struct tmc_drvdata *drvdata = container_of(nb, struct tmc_drvdata,
+						   panic_notifier);
+	struct coresight_device *csdev = drvdata->csdev;
+
+	if (coresight_get_mode(csdev) == CS_MODE_DISABLED)
+		return 0;
+
+	if (panic_ops(csdev))
+		panic_ops(csdev)->sync(csdev);
+
+	return 0;
+}
+
 static int __tmc_probe(struct device *dev, struct resource *res)
 {
 	int ret = 0;
@@ -885,6 +901,12 @@ static int __tmc_probe(struct device *dev, struct resource *res)
 		goto out;
 	}
 
+	if (panic_ops(drvdata->csdev)) {
+		drvdata->panic_notifier.notifier_call = tmc_panic_cb;
+		atomic_notifier_chain_register(&panic_notifier_list,
+					       &drvdata->panic_notifier);
+	}
+
 out:
 	if (is_tmc_crashdata_valid(drvdata) &&
 	    !tmc_prepare_crashdata(drvdata))
@@ -929,6 +951,10 @@ static void __tmc_remove(struct device *dev)
 {
 	struct tmc_drvdata *drvdata = dev_get_drvdata(dev);
 
+	if (panic_ops(drvdata->csdev))
+		atomic_notifier_chain_unregister(&panic_notifier_list,
+						 &drvdata->panic_notifier);
+
 	/*
 	 * Since misc_open() holds a refcount on the f_ops, which is
 	 * etb fops in this case, device is there until last file
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index cbb4ba439158..873c5427673c 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -243,6 +243,7 @@ struct tmc_resrv_buf {
  *		(after crash) by default.
  * @crash_mdata: Reserved memory for storing tmc crash metadata.
  *		 Used by ETR/ETF.
+ * @panic_notifier: Notifier used to clean up during a panic
  */
 struct tmc_drvdata {
 	struct clk		*atclk;
@@ -273,6 +274,7 @@ struct tmc_drvdata {
 	struct etr_buf		*perf_buf;
 	struct tmc_resrv_buf	resrv_buf;
 	struct tmc_resrv_buf	crash_mdata;
+	struct notifier_block	panic_notifier;
 };
 
 struct etr_buf_operations {
-- 
2.34.1

On 9/16/25 12:00, Leo Yan wrote:
> On Mon, Sep 15, 2025 at 10:31:24AM -0400, Sean Anderson wrote:
>> On 9/15/25 05:58, Leo Yan wrote:
>> > On Fri, Sep 12, 2025 at 11:13:14AM -0400, Sean Anderson wrote:
>> >> coresight_panic_cb is called with interrupts disabled during panics.
>> >> However, bus_for_each_dev calls bus_to_subsys which takes
>> >> bus_kset->list_lock without disabling IRQs. This may cause a deadlock.
>> > 
>> > I would rephrase it to make it clearer for anyone reading it later:
>> > 
>> >   coresight_panic_cb() is called during panics, which can preempt a flow
>> >   that triggers exceptions (such as data or instruction aborts).
>> 
>> I don't see what exceptions have to do with it. You can also panic
>> during a regular interrupt.
> 
> The commit mentioned "without disabling IRQs" gives the impression that
> the deadlock is caused by IRQ-unsafe locking, which might mislead into
> thinking why the issue cannot be fixed with IRQ-safe locking.
> 
> Regardless of whether IRQs are disabled, and regardless of the context
> (interrupt, bottom-half, or normal thread), the conditions for the
> deadlock are only about:
> 
>   (a) The bus lock has been acquired;
>   (b) A panic is triggered to try to acquire the same lock.
> 
> [...]
> 
>> > When I review this patch, I recognize we can consolidate panic notifier
>> > in coresight-tmc-core.c, so we don't need to distribute the changes
>> > into ETF and ETR drivers (sorry if I misled you in my previous reply).
>> 
>> And this kind of thing is why I went with the straightforward fix
>> initially. I do not want to bikeshed the extent that this gets removed.
>> IMO the whole "panic ops" stuff should be done directly with the panic
>> notifier, hence this patch. If you do not agree with that, then ack v2
>> and send a follow up of your own to fix it how you see fit.
> 
> I would fix it in one go.
> 
> I agree with you that "the whole panic ops stuff should be done directly
> with the panic". The only difference between us is that I would keep the
> `panic_ops` callback. To me, this encapsulates panic callbacks into
> different modules, to make the code more general.
> 
> Could you check if the drafted patch below looks good to you? If so, I

As stated above I disagree with a half-hearted removal. If you want to do that,
then I will resend v2 done with an rcu list and you can make your own follow-up.

--Sean

On Tue, Sep 16, 2025 at 12:14:40PM -0400, Sean Anderson wrote:

[...]

> > Could you check if the drafted patch below looks good to you? If so, I
> 
> As stated above I disagree with a half-hearted removal. If you want to do that,
> then I will resend v2 done with an rcu list and you can make your own follow-up.

It is fine to disagree, but please don't resend v2 :)

We have plan to refactor locking in CoreSight driver, I will try my
best to avoid adding new lock unless with a strong reason.

Thanks,
Leo

On 9/16/25 12:48, Leo Yan wrote:
> On Tue, Sep 16, 2025 at 12:14:40PM -0400, Sean Anderson wrote:
> 
> [...]
> 
>> > Could you check if the drafted patch below looks good to you? If so, I
>> 
>> As stated above I disagree with a half-hearted removal. If you want to do that,
>> then I will resend v2 done with an rcu list and you can make your own follow-up.
> 
> It is fine to disagree, but please don't resend v2 :)
> 
> We have plan to refactor locking in CoreSight driver, I will try my
> best to avoid adding new lock unless with a strong reason.

As said above it will be done with an rcu list, so no new lock.

Or I can do this patch but stick the notifier block in csdev as suggested by Suzuki.

--Sean

On Tue, Sep 16, 2025 at 12:51:11PM -0400, Sean Anderson wrote:
> On 9/16/25 12:48, Leo Yan wrote:
> > On Tue, Sep 16, 2025 at 12:14:40PM -0400, Sean Anderson wrote:
> > 
> > [...]
> > 
> >> > Could you check if the drafted patch below looks good to you? If so, I
> >> 
> >> As stated above I disagree with a half-hearted removal. If you want to do that,
> >> then I will resend v2 done with an rcu list and you can make your own follow-up.
> > 
> > It is fine to disagree, but please don't resend v2 :)
> > 
> > We have plan to refactor locking in CoreSight driver, I will try my
> > best to avoid adding new lock unless with a strong reason.
> 
> As said above it will be done with an rcu list, so no new lock.
> 
> Or I can do this patch but stick the notifier block in csdev as suggested by Suzuki.

I am fine for adding the notifier block in csdev.

Suzuki, could you confirm if this is the right way to move forward?

Thanks,
Leo

On 16/09/2025 18:17, Leo Yan wrote:
> On Tue, Sep 16, 2025 at 12:51:11PM -0400, Sean Anderson wrote:
>> On 9/16/25 12:48, Leo Yan wrote:
>>> On Tue, Sep 16, 2025 at 12:14:40PM -0400, Sean Anderson wrote:
>>>
>>> [...]
>>>
>>>>> Could you check if the drafted patch below looks good to you? If so, I
>>>>
>>>> As stated above I disagree with a half-hearted removal. If you want to do that,
>>>> then I will resend v2 done with an rcu list and you can make your own follow-up.
>>>
>>> It is fine to disagree, but please don't resend v2 :)
>>>
>>> We have plan to refactor locking in CoreSight driver, I will try my
>>> best to avoid adding new lock unless with a strong reason.
>>
>> As said above it will be done with an rcu list, so no new lock.
>>
>> Or I can do this patch but stick the notifier block in csdev as suggested by Suzuki.
> 
> I am fine for adding the notifier block in csdev.
> 
> Suzuki, could you confirm if this is the right way to move forward?

Yes, if we are planning to keep a csdev panic_ops, why not stick in the
notifier there, rather than splitting it between core code and drivers.

Suzuki




> 
> Thanks,
> Leo

On 16/09/2025 17:00, Leo Yan wrote:
> On Mon, Sep 15, 2025 at 10:31:24AM -0400, Sean Anderson wrote:
>> On 9/15/25 05:58, Leo Yan wrote:
>>> On Fri, Sep 12, 2025 at 11:13:14AM -0400, Sean Anderson wrote:
>>>> coresight_panic_cb is called with interrupts disabled during panics.
>>>> However, bus_for_each_dev calls bus_to_subsys which takes
>>>> bus_kset->list_lock without disabling IRQs. This may cause a deadlock.
>>>
>>> I would rephrase it to make it clearer for anyone reading it later:
>>>
>>>    coresight_panic_cb() is called during panics, which can preempt a flow
>>>    that triggers exceptions (such as data or instruction aborts).
>>
>> I don't see what exceptions have to do with it. You can also panic
>> during a regular interrupt.
> 
> The commit mentioned "without disabling IRQs" gives the impression that
> the deadlock is caused by IRQ-unsafe locking, which might mislead into
> thinking why the issue cannot be fixed with IRQ-safe locking.
> 
> Regardless of whether IRQs are disabled, and regardless of the context
> (interrupt, bottom-half, or normal thread), the conditions for the
> deadlock are only about:
> 
>    (a) The bus lock has been acquired;
>    (b) A panic is triggered to try to acquire the same lock.
> 
> [...]
> 
>>> When I review this patch, I recognize we can consolidate panic notifier
>>> in coresight-tmc-core.c, so we don't need to distribute the changes
>>> into ETF and ETR drivers (sorry if I misled you in my previous reply).
>>
>> And this kind of thing is why I went with the straightforward fix
>> initially. I do not want to bikeshed the extent that this gets removed.
>> IMO the whole "panic ops" stuff should be done directly with the panic
>> notifier, hence this patch. If you do not agree with that, then ack v2
>> and send a follow up of your own to fix it how you see fit.
> 
> I would fix it in one go.
> 
> I agree with you that "the whole panic ops stuff should be done directly
> with the panic". The only difference between us is that I would keep the
> `panic_ops` callback. To me, this encapsulates panic callbacks into
> different modules, to make the code more general.
> 
> Could you check if the drafted patch below looks good to you? If so, I
> will send out a formal patch.
> 
> ---8<---
> 
>  From ea78dd22cbdd97f709c5991d5bd3be97be6e137e Mon Sep 17 00:00:00 2001
> From: Sean Anderson <sean.anderson@linux.dev>
> Date: Tue, 16 Sep 2025 16:03:58 +0100
> Subject: [PATCH] coresight: Fix possible deadlock in coresight_panic_cb()
> 
> coresight_panic_cb() is called during a panic. It invokes
> bus_for_each_dev(), which then calls bus_to_subsys() and takes the
> 'bus_kset->list_lock'. If a panic occurs after the lock has been
> acquired, it can lead to a deadlock.
> 
> Instead of using a common panic notifier to iterate the bus, this commit
> directly registers the TMC device's panic notifier. This avoids bus
> iteration and effectively eliminates the race condition that could cause
> the deadlock.

Well, if you are going that far, why not register the notifier from
coresight-core ? We could always check the panic_ops->sync, and
then register/unregister the callback. Add the notifer to the csdev.
Otherwise, there is no point in keeping the panic_ops in
coresight_device, it is just for the TMC device.

Suzuki



> 
> Fixes: 46006ceb5d02 ("coresight: core: Add provision for panic callbacks")
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
>   drivers/hwtracing/coresight/coresight-core.c  | 42 -------------------
>   .../hwtracing/coresight/coresight-tmc-core.c  | 26 ++++++++++++
>   drivers/hwtracing/coresight/coresight-tmc.h   |  2 +
>   3 files changed, 28 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 3267192f0c1c..cb0cc8d77056 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -21,7 +21,6 @@
>   #include <linux/property.h>
>   #include <linux/delay.h>
>   #include <linux/pm_runtime.h>
> -#include <linux/panic_notifier.h>
>   
>   #include "coresight-etm-perf.h"
>   #include "coresight-priv.h"
> @@ -1566,36 +1565,6 @@ const struct bus_type coresight_bustype = {
>   	.name	= "coresight",
>   };
>   
> -static int coresight_panic_sync(struct device *dev, void *data)
> -{
> -	int mode;
> -	struct coresight_device *csdev;
> -
> -	/* Run through panic sync handlers for all enabled devices */
> -	csdev = container_of(dev, struct coresight_device, dev);
> -	mode = coresight_get_mode(csdev);
> -
> -	if ((mode == CS_MODE_SYSFS) || (mode == CS_MODE_PERF)) {
> -		if (panic_ops(csdev))
> -			panic_ops(csdev)->sync(csdev);
> -	}
> -
> -	return 0;
> -}
> -
> -static int coresight_panic_cb(struct notifier_block *self,
> -			       unsigned long v, void *p)
> -{
> -	bus_for_each_dev(&coresight_bustype, NULL, NULL,
> -				 coresight_panic_sync);
> -
> -	return 0;
> -}
> -
> -static struct notifier_block coresight_notifier = {
> -	.notifier_call = coresight_panic_cb,
> -};
> -
>   static int __init coresight_init(void)
>   {
>   	int ret;
> @@ -1608,20 +1577,11 @@ static int __init coresight_init(void)
>   	if (ret)
>   		goto exit_bus_unregister;
>   
> -	/* Register function to be called for panic */
> -	ret = atomic_notifier_chain_register(&panic_notifier_list,
> -					     &coresight_notifier);
> -	if (ret)
> -		goto exit_perf;
> -
>   	/* initialise the coresight syscfg API */
>   	ret = cscfg_init();
>   	if (!ret)
>   		return 0;
>   
> -	atomic_notifier_chain_unregister(&panic_notifier_list,
> -					     &coresight_notifier);
> -exit_perf:
>   	etm_perf_exit();
>   exit_bus_unregister:
>   	bus_unregister(&coresight_bustype);
> @@ -1631,8 +1591,6 @@ static int __init coresight_init(void)
>   static void __exit coresight_exit(void)
>   {
>   	cscfg_exit();
> -	atomic_notifier_chain_unregister(&panic_notifier_list,
> -					     &coresight_notifier);
>   	etm_perf_exit();
>   	bus_unregister(&coresight_bustype);
>   }
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> index 36599c431be6..108ed9daf56d 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> @@ -21,6 +21,7 @@
>   #include <linux/slab.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/spinlock.h>
> +#include <linux/panic_notifier.h>
>   #include <linux/pm_runtime.h>
>   #include <linux/of.h>
>   #include <linux/of_address.h>
> @@ -769,6 +770,21 @@ static void register_crash_dev_interface(struct tmc_drvdata *drvdata,
>   			"Valid crash tracedata found\n");
>   }
>   
> +static int tmc_panic_cb(struct notifier_block *nb, unsigned long v, void *p)
> +{
> +	struct tmc_drvdata *drvdata = container_of(nb, struct tmc_drvdata,
> +						   panic_notifier);
> +	struct coresight_device *csdev = drvdata->csdev;
> +
> +	if (coresight_get_mode(csdev) == CS_MODE_DISABLED)
> +		return 0;
> +
> +	if (panic_ops(csdev))
> +		panic_ops(csdev)->sync(csdev);
> +
> +	return 0;
> +}
> +
>   static int __tmc_probe(struct device *dev, struct resource *res)
>   {
>   	int ret = 0;
> @@ -885,6 +901,12 @@ static int __tmc_probe(struct device *dev, struct resource *res)
>   		goto out;
>   	}
>   
> +	if (panic_ops(drvdata->csdev)) {
> +		drvdata->panic_notifier.notifier_call = tmc_panic_cb;
> +		atomic_notifier_chain_register(&panic_notifier_list,
> +					       &drvdata->panic_notifier);
> +	}
> +
>   out:
>   	if (is_tmc_crashdata_valid(drvdata) &&
>   	    !tmc_prepare_crashdata(drvdata))
> @@ -929,6 +951,10 @@ static void __tmc_remove(struct device *dev)
>   {
>   	struct tmc_drvdata *drvdata = dev_get_drvdata(dev);
>   
> +	if (panic_ops(drvdata->csdev))
> +		atomic_notifier_chain_unregister(&panic_notifier_list,
> +						 &drvdata->panic_notifier);
> +
>   	/*
>   	 * Since misc_open() holds a refcount on the f_ops, which is
>   	 * etb fops in this case, device is there until last file
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index cbb4ba439158..873c5427673c 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -243,6 +243,7 @@ struct tmc_resrv_buf {
>    *		(after crash) by default.
>    * @crash_mdata: Reserved memory for storing tmc crash metadata.
>    *		 Used by ETR/ETF.
> + * @panic_notifier: Notifier used to clean up during a panic
>    */
>   struct tmc_drvdata {
>   	struct clk		*atclk;
> @@ -273,6 +274,7 @@ struct tmc_drvdata {
>   	struct etr_buf		*perf_buf;
>   	struct tmc_resrv_buf	resrv_buf;
>   	struct tmc_resrv_buf	crash_mdata;
> +	struct notifier_block	panic_notifier;
>   };
>   
>   struct etr_buf_operations {

On Tue, Sep 16, 2025 at 05:09:01PM +0100, Suzuki Kuruppassery Poulose wrote:

[...]

> >  From ea78dd22cbdd97f709c5991d5bd3be97be6e137e Mon Sep 17 00:00:00 2001
> > From: Sean Anderson <sean.anderson@linux.dev>
> > Date: Tue, 16 Sep 2025 16:03:58 +0100
> > Subject: [PATCH] coresight: Fix possible deadlock in coresight_panic_cb()
> > 
> > coresight_panic_cb() is called during a panic. It invokes
> > bus_for_each_dev(), which then calls bus_to_subsys() and takes the
> > 'bus_kset->list_lock'. If a panic occurs after the lock has been
> > acquired, it can lead to a deadlock.
> > 
> > Instead of using a common panic notifier to iterate the bus, this commit
> > directly registers the TMC device's panic notifier. This avoids bus
> > iteration and effectively eliminates the race condition that could cause
> > the deadlock.
> 
> Well, if you are going that far, why not register the notifier from
> coresight-core ?

I have thought this but gave up.

When register a panic's notifier, it does not provide an argument for
passing a private data. So the code below uses container_of() to convert
notifier block pointer to the TMC driver data, as a result, the code is
specific to TMC driver.

I searched the kernel but did not find a case for passing private data
for panic notifier (note, this should be only an issue for panic
notifier but not for common notifiers).

> We could always check the panic_ops->sync, and
> then register/unregister the callback. Add the notifer to the csdev.
> Otherwise, there is no point in keeping the panic_ops in
> coresight_device, it is just for the TMC device.

ETF and ETR devices both support panic, this is why I think keeping
`panic_ops` callbacks can benefit code consolidation in
coresight-tmc-core.c (you could see the difference between my proposed
change and Sean's v3 patch).

But it is fine for me to drop `panic_ops` in coresight_device. At
least, this is much better than adding lock and list in patch v2.

Thanks,
Leo

> > Fixes: 46006ceb5d02 ("coresight: core: Add provision for panic callbacks")
> > Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> > Signed-off-by: Leo Yan <leo.yan@arm.com>
> > ---
> >   drivers/hwtracing/coresight/coresight-core.c  | 42 -------------------
> >   .../hwtracing/coresight/coresight-tmc-core.c  | 26 ++++++++++++
> >   drivers/hwtracing/coresight/coresight-tmc.h   |  2 +
> >   3 files changed, 28 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> > index 3267192f0c1c..cb0cc8d77056 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -21,7 +21,6 @@
> >   #include <linux/property.h>
> >   #include <linux/delay.h>
> >   #include <linux/pm_runtime.h>
> > -#include <linux/panic_notifier.h>
> >   #include "coresight-etm-perf.h"
> >   #include "coresight-priv.h"
> > @@ -1566,36 +1565,6 @@ const struct bus_type coresight_bustype = {
> >   	.name	= "coresight",
> >   };
> > -static int coresight_panic_sync(struct device *dev, void *data)
> > -{
> > -	int mode;
> > -	struct coresight_device *csdev;
> > -
> > -	/* Run through panic sync handlers for all enabled devices */
> > -	csdev = container_of(dev, struct coresight_device, dev);
> > -	mode = coresight_get_mode(csdev);
> > -
> > -	if ((mode == CS_MODE_SYSFS) || (mode == CS_MODE_PERF)) {
> > -		if (panic_ops(csdev))
> > -			panic_ops(csdev)->sync(csdev);
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> > -static int coresight_panic_cb(struct notifier_block *self,
> > -			       unsigned long v, void *p)
> > -{
> > -	bus_for_each_dev(&coresight_bustype, NULL, NULL,
> > -				 coresight_panic_sync);
> > -
> > -	return 0;
> > -}
> > -
> > -static struct notifier_block coresight_notifier = {
> > -	.notifier_call = coresight_panic_cb,
> > -};
> > -
> >   static int __init coresight_init(void)
> >   {
> >   	int ret;
> > @@ -1608,20 +1577,11 @@ static int __init coresight_init(void)
> >   	if (ret)
> >   		goto exit_bus_unregister;
> > -	/* Register function to be called for panic */
> > -	ret = atomic_notifier_chain_register(&panic_notifier_list,
> > -					     &coresight_notifier);
> > -	if (ret)
> > -		goto exit_perf;
> > -
> >   	/* initialise the coresight syscfg API */
> >   	ret = cscfg_init();
> >   	if (!ret)
> >   		return 0;
> > -	atomic_notifier_chain_unregister(&panic_notifier_list,
> > -					     &coresight_notifier);
> > -exit_perf:
> >   	etm_perf_exit();
> >   exit_bus_unregister:
> >   	bus_unregister(&coresight_bustype);
> > @@ -1631,8 +1591,6 @@ static int __init coresight_init(void)
> >   static void __exit coresight_exit(void)
> >   {
> >   	cscfg_exit();
> > -	atomic_notifier_chain_unregister(&panic_notifier_list,
> > -					     &coresight_notifier);
> >   	etm_perf_exit();
> >   	bus_unregister(&coresight_bustype);
> >   }
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> > index 36599c431be6..108ed9daf56d 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> > @@ -21,6 +21,7 @@
> >   #include <linux/slab.h>
> >   #include <linux/dma-mapping.h>
> >   #include <linux/spinlock.h>
> > +#include <linux/panic_notifier.h>
> >   #include <linux/pm_runtime.h>
> >   #include <linux/of.h>
> >   #include <linux/of_address.h>
> > @@ -769,6 +770,21 @@ static void register_crash_dev_interface(struct tmc_drvdata *drvdata,
> >   			"Valid crash tracedata found\n");
> >   }
> > +static int tmc_panic_cb(struct notifier_block *nb, unsigned long v, void *p)
> > +{
> > +	struct tmc_drvdata *drvdata = container_of(nb, struct tmc_drvdata,
> > +						   panic_notifier);
> > +	struct coresight_device *csdev = drvdata->csdev;
> > +
> > +	if (coresight_get_mode(csdev) == CS_MODE_DISABLED)
> > +		return 0;
> > +
> > +	if (panic_ops(csdev))
> > +		panic_ops(csdev)->sync(csdev);
> > +
> > +	return 0;
> > +}
> > +
> >   static int __tmc_probe(struct device *dev, struct resource *res)
> >   {
> >   	int ret = 0;
> > @@ -885,6 +901,12 @@ static int __tmc_probe(struct device *dev, struct resource *res)
> >   		goto out;
> >   	}
> > +	if (panic_ops(drvdata->csdev)) {
> > +		drvdata->panic_notifier.notifier_call = tmc_panic_cb;
> > +		atomic_notifier_chain_register(&panic_notifier_list,
> > +					       &drvdata->panic_notifier);
> > +	}
> > +
> >   out:
> >   	if (is_tmc_crashdata_valid(drvdata) &&
> >   	    !tmc_prepare_crashdata(drvdata))
> > @@ -929,6 +951,10 @@ static void __tmc_remove(struct device *dev)
> >   {
> >   	struct tmc_drvdata *drvdata = dev_get_drvdata(dev);
> > +	if (panic_ops(drvdata->csdev))
> > +		atomic_notifier_chain_unregister(&panic_notifier_list,
> > +						 &drvdata->panic_notifier);
> > +
> >   	/*
> >   	 * Since misc_open() holds a refcount on the f_ops, which is
> >   	 * etb fops in this case, device is there until last file
> > diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> > index cbb4ba439158..873c5427673c 100644
> > --- a/drivers/hwtracing/coresight/coresight-tmc.h
> > +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> > @@ -243,6 +243,7 @@ struct tmc_resrv_buf {
> >    *		(after crash) by default.
> >    * @crash_mdata: Reserved memory for storing tmc crash metadata.
> >    *		 Used by ETR/ETF.
> > + * @panic_notifier: Notifier used to clean up during a panic
> >    */
> >   struct tmc_drvdata {
> >   	struct clk		*atclk;
> > @@ -273,6 +274,7 @@ struct tmc_drvdata {
> >   	struct etr_buf		*perf_buf;
> >   	struct tmc_resrv_buf	resrv_buf;
> >   	struct tmc_resrv_buf	crash_mdata;
> > +	struct notifier_block	panic_notifier;
> >   };
> >   struct etr_buf_operations {
>

On 16/09/2025 17:38, Leo Yan wrote:
> On Tue, Sep 16, 2025 at 05:09:01PM +0100, Suzuki Kuruppassery Poulose wrote:
> 
> [...]
> 
>>>   From ea78dd22cbdd97f709c5991d5bd3be97be6e137e Mon Sep 17 00:00:00 2001
>>> From: Sean Anderson <sean.anderson@linux.dev>
>>> Date: Tue, 16 Sep 2025 16:03:58 +0100
>>> Subject: [PATCH] coresight: Fix possible deadlock in coresight_panic_cb()
>>>
>>> coresight_panic_cb() is called during a panic. It invokes
>>> bus_for_each_dev(), which then calls bus_to_subsys() and takes the
>>> 'bus_kset->list_lock'. If a panic occurs after the lock has been
>>> acquired, it can lead to a deadlock.
>>>
>>> Instead of using a common panic notifier to iterate the bus, this commit
>>> directly registers the TMC device's panic notifier. This avoids bus
>>> iteration and effectively eliminates the race condition that could cause
>>> the deadlock.
>>
>> Well, if you are going that far, why not register the notifier from
>> coresight-core ?
> 
> I have thought this but gave up.
> 
> When register a panic's notifier, it does not provide an argument for
> passing a private data. So the code below uses container_of() to convert
> notifier block pointer to the TMC driver data, as a result, the code is
> specific to TMC driver.

notifier_block in csdev ?

On Tue, Sep 16, 2025 at 05:42:07PM +0100, Suzuki Kuruppassery Poulose wrote:

[...]

> > > Well, if you are going that far, why not register the notifier from
> > > coresight-core ?
> > 
> > I have thought this but gave up.
> > 
> > When register a panic's notifier, it does not provide an argument for
> > passing a private data. So the code below uses container_of() to convert
> > notifier block pointer to the TMC driver data, as a result, the code is
> > specific to TMC driver.
> 
> notifier_block in csdev ?

csdev is a common structure, some devices may never use the notifier.

This is the reason I thought it is fine to put notifier into TMC's
driver data, as this can meet current requirement perfectly.

Thanks,
Leo