coresight: tmc: Add shutdown callback for TMC ETR/ETF

Add a flag to check whether TMC ETR/ETF is enabled or not. This is later used in shutdown callback to determine if we require to disable ETR/ETF.

Signed-off-by Sai Prakash Ranjan <[email protected]>

---
 drivers/hwtracing/coresight/coresight-tmc.c | 2 ++
 drivers/hwtracing/coresight/coresight-tmc.h | 2 ++
 2 files changed, 4 insertions(+)

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

index 39fba1d16e6e..5a271ebc4585 100644
@@ -62,11 +62,13 @@ void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
 
 void tmc_enable_hw(struct tmc_drvdata *drvdata)
 {
+       drvdata->enable = true;
        writel_relaxed(TMC_CTL_CAPT_EN, drvdata->base + TMC_CTL);
 }
 
 void tmc_disable_hw(struct tmc_drvdata *drvdata)
 {
+       drvdata->enable = false;
        writel_relaxed(0x0, drvdata->base + TMC_CTL);
 }
 

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

index 71de978575f3..d156860495c7 100644
@@ -184,6 +184,7 @@ struct etr_buf {
  * @idr_mutex: Access serialisation for idr.
  * @sysfs_buf: SYSFS buffer for ETR.
  * @perf_buf:  PERF buffer for ETR.
+ * @enable:    Indicates whether ETR/ETF is enabled.
  */
 struct tmc_drvdata {
        void __iomem            *base;

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

 };
 
 struct etr_buf_operations {
--

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Hi,

Quoted 42 lines · Sai Prakash Ranjan

Add a flag to check whether TMC ETR/ETF is enabled or not. This is later used in shutdown callback to determine if we require to disable ETR/ETF.

Signed-off-by Sai Prakash Ranjan <[email protected]>

---
 drivers/hwtracing/coresight/coresight-tmc.c | 2 ++
 drivers/hwtracing/coresight/coresight-tmc.h | 2 ++
 2 files changed, 4 insertions(+)

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

index 39fba1d16e6e..5a271ebc4585 100644
@@ -62,11 +62,13 @@ void tmc_flush_and_stop(struct tmc_drvdata 
 *drvdata)
 
 void tmc_enable_hw(struct tmc_drvdata *drvdata)
 {
+       drvdata->enable = true;
        writel_relaxed(TMC_CTL_CAPT_EN, drvdata->base + TMC_CTL);
 }
 
 void tmc_disable_hw(struct tmc_drvdata *drvdata)
 {
+       drvdata->enable = false;
        writel_relaxed(0x0, drvdata->base + TMC_CTL);
 }

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

index 71de978575f3..d156860495c7 100644
@@ -184,6 +184,7 @@ struct etr_buf {
  * @idr_mutex: Access serialisation for idr.
  * @sysfs_buf: SYSFS buffer for ETR.
  * @perf_buf:  PERF buffer for ETR.
+ * @enable:    Indicates whether ETR/ETF is enabled.
  */
 struct tmc_drvdata {
        void __iomem            *base;

drivers/hwtracing/coresight/coresight-tmc.h struct tmc_drvdata { · line 211

Quoted 5 lines

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

@@ -207,6 +208,7 @@ struct tmc_drvdata {
        struct mutex            idr_mutex;
        struct etr_buf          *sysfs_buf;
        struct etr_buf          *perf_buf;
+       bool                    enable;

Is this flag needed?
For TMC, drvdata->mode indicates if the device is in use.

Implement a shutdown callback to ensure ETR/ETF hardware is properly shutdown in reboot/shutdown path. This is required for ETR/ETF which has SMMU address translation enabled like on SC7180 SoC and few others. If the hardware is still accessing memory after SMMU translation is disabled as part of SMMU shutdown callback in system reboot or shutdown path, then IOVAs(I/O virtual address) which it was using will go on the bus as the physical addresses which might result in unknown crashes (NoC/interconnect errors). So we make sure from this shutdown callback that the ETR/ETF is shutdown before SMMU translation is disabled and device_link in SMMU driver will take care of ordering of shutdown callbacks such that SMMU shutdown callback is not called before any of its consumer shutdown callbacks.

Signed-off-by Sai Prakash Ranjan <[email protected]>

---
 .../hwtracing/coresight/coresight-tmc-etf.c   |  4 +--
 .../hwtracing/coresight/coresight-tmc-etr.c   |  2 +-
 drivers/hwtracing/coresight/coresight-tmc.c   | 29 +++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.h   |  3 ++
 4 files changed, 35 insertions(+), 3 deletions(-)

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

index 36cce2bfb744..cba3e7592820 100644
@@ -85,7 +85,7 @@ static void __tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
        CS_LOCK(drvdata->base);
 }
 
-static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
 {
        __tmc_etb_disable_hw(drvdata);
        coresight_disclaim_device(drvdata->base);

@@ -118,7 +118,7 @@ static int tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
        return 0;
 }
 
-static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
 {
        CS_UNLOCK(drvdata->base);
 

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

index 625882bc8b08..b29c2db94d96 100644
@@ -1110,7 +1110,7 @@ static void __tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 
 }
 
-static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 {
        __tmc_etr_disable_hw(drvdata);
        /* Disable CATU device if this ETR is connected to one */

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

index 5a271ebc4585..7e687a356fe0 100644

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

+               goto out;
+
+       /*
+        * We do not care about the active trace sessions here
+        * since the system is going down unlike remove callback,
+        * just make sure that the hardware is shutdown.
+        */
+       switch (drvdata->config_type) {
+       case TMC_CONFIG_TYPE_ETB:
+               tmc_etb_disable_hw(drvdata);
+               break;
+       case TMC_CONFIG_TYPE_ETF:
+               tmc_etf_disable_hw(drvdata);
+               break;
+       case TMC_CONFIG_TYPE_ETR:
+               tmc_etr_disable_hw(drvdata);
+       }
+
+out:
+       misc_deregister(&drvdata->miscdev);
+       coresight_unregister(drvdata->csdev);
+}
+
 static const struct amba_id tmc_ids[] = {
        CS_AMBA_ID(0x000bb961),
        /* Coresight SoC 600 TMC-ETR/ETS */

@@ -558,6 +586,7 @@ static struct amba_driver tmc_driver = {
                .suppress_bind_attrs = true,
        },
        .probe          = tmc_probe,
+       .shutdown       = tmc_shutdown,
        .id_table       = tmc_ids,
 };
 builtin_amba_driver(tmc_driver);

drivers/hwtracing/coresight/coresight-tmc.h +3 -1

index d156860495c7..f4f56c474e58 100644
@@ -262,6 +262,8 @@ u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata);
 /* ETB/ETF functions */
 int tmc_read_prepare_etb(struct tmc_drvdata *drvdata);
 int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata);
+void tmc_etb_disable_hw(struct tmc_drvdata *drvdata);
+void tmc_etf_disable_hw(struct tmc_drvdata *drvdata);
 extern const struct coresight_ops tmc_etb_cs_ops;
 extern const struct coresight_ops tmc_etf_cs_ops;
 

@@ -270,6 +272,7 @@ ssize_t tmc_etb_get_sysfs_trace(struct tmc_drvdata *drvdata,
 /* 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);
 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);
--

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

Hi,

Quoted 71 lines · Sai Prakash Ranjan

Implement a shutdown callback to ensure ETR/ETF hardware is properly shutdown in reboot/shutdown path. This is required for ETR/ETF which has SMMU address translation enabled like on SC7180 SoC and few others. If the hardware is still accessing memory after SMMU translation is disabled as part of SMMU shutdown callback in system reboot or shutdown path, then IOVAs(I/O virtual address) which it was using will go on the bus as the physical addresses which might result in unknown crashes (NoC/interconnect errors). So we make sure from this shutdown callback that the ETR/ETF is shutdown before SMMU translation is disabled and device_link in SMMU driver will take care of ordering of shutdown callbacks such that SMMU shutdown callback is not called before any of its consumer shutdown callbacks.

Signed-off-by Sai Prakash Ranjan <[email protected]>

---
 .../hwtracing/coresight/coresight-tmc-etf.c   |  4 +--
 .../hwtracing/coresight/coresight-tmc-etr.c   |  2 +-
 drivers/hwtracing/coresight/coresight-tmc.c   | 29 
+++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.h   |  3 ++
 4 files changed, 35 insertions(+), 3 deletions(-)

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

index 36cce2bfb744..cba3e7592820 100644
@@ -85,7 +85,7 @@ static void __tmc_etb_disable_hw(struct tmc_drvdata 
 *drvdata)
        CS_LOCK(drvdata->base);
 }
 
-static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
 {
        __tmc_etb_disable_hw(drvdata);

        coresight_disclaim_device(drvdata->base);

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

@@ -118,7 +118,7 @@ static int tmc_etf_enable_hw(struct tmc_drvdata 
 *drvdata)
        return 0;
 }
 
-static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
 {
        CS_UNLOCK(drvdata->base);

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

index 625882bc8b08..b29c2db94d96 100644
@@ -1110,7 +1110,7 @@ static void __tmc_etr_disable_hw(struct 
 tmc_drvdata *drvdata)
 
 }
 
-static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 {
        __tmc_etr_disable_hw(drvdata);

        /* Disable CATU device if this ETR is connected to one */

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

index 5a271ebc4585..7e687a356fe0 100644
@@ -540,6 +540,34 @@ static int tmc_probe(struct amba_device *adev, 
 const struct amba_id *id)
        return ret;
 }
 

drivers/hwtracing/coresight/coresight-tmc.c static int tmc_probe(struct amba_device *adev, const struct amba_id *id) · line 546

Quoted 4 lines

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

+static void tmc_shutdown(struct amba_device *adev)
+{
+       struct tmc_drvdata *drvdata = amba_get_drvdata(adev);
+

Take drvdata->spinlock here? The tmc_xxx_disable_hw functions are normally called with the spinlock claimed.

drivers/hwtracing/coresight/coresight-tmc.c static int tmc_probe(struct amba_device *adev, const struct amba_id *id) · line 547

Quoted 1 line

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

+       if (!drvdata->enable)

As per previous patch drvdata->mode can be used here.

Implement a shutdown callback to ensure ETR/ETF hardware is properly shutdown in reboot/shutdown path. This is required for ETR/ETF which has SMMU address translation enabled like on SC7180 SoC and few others. If the hardware is still accessing memory after SMMU translation is disabled as part of SMMU shutdown callback in system reboot or shutdown path, then IOVAs(I/O virtual address) which it was using will go on the bus as the physical addresses which might result in unknown crashes (NoC/interconnect errors). So we make sure from this shutdown callback that the ETR/ETF is shutdown before SMMU translation is disabled and device_link in SMMU driver will take care of ordering of shutdown callbacks such that SMMU shutdown callback is not called before any of its consumer shutdown callbacks.

Signed-off-by Sai Prakash Ranjan <[email protected]>

---
 .../hwtracing/coresight/coresight-tmc-etf.c   |  4 +--
 .../hwtracing/coresight/coresight-tmc-etr.c   |  2 +-
 drivers/hwtracing/coresight/coresight-tmc.c   | 29 +++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.h   |  3 ++
 4 files changed, 35 insertions(+), 3 deletions(-)

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

index 36cce2bfb744..cba3e7592820 100644
@@ -85,7 +85,7 @@ static void __tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
 	CS_LOCK(drvdata->base);
 }
 
-static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
 {
 	__tmc_etb_disable_hw(drvdata);
 	coresight_disclaim_device(drvdata->base);

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

index 625882bc8b08..b29c2db94d96 100644
@@ -1110,7 +1110,7 @@ static void __tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 
 }
 
-static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 {
 	__tmc_etr_disable_hw(drvdata);
 	/* Disable CATU device if this ETR is connected to one */

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

index 5a271ebc4585..7e687a356fe0 100644
@@ -540,6 +540,34 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
 	return ret;
 }
 
+static void tmc_shutdown(struct amba_device *adev)
+{
+	struct tmc_drvdata *drvdata = amba_get_drvdata(adev);
+
+	if (!drvdata->enable)
+		goto out;
+
+	/*
+	 * We do not care about the active trace sessions here
+	 * since the system is going down unlike remove callback,
+	 * just make sure that the hardware is shutdown.
+	 */
+	switch (drvdata->config_type) {
+	case TMC_CONFIG_TYPE_ETB:
+		tmc_etb_disable_hw(drvdata);
+		break;

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

+}
+
 static const struct amba_id tmc_ids[] = {
 	CS_AMBA_ID(0x000bb961),
 	/* Coresight SoC 600 TMC-ETR/ETS */

@@ -558,6 +586,7 @@ static struct amba_driver tmc_driver = {
 		.suppress_bind_attrs = true,
 	},
 	.probe		= tmc_probe,
+	.shutdown	= tmc_shutdown,
 	.id_table	= tmc_ids,
 };
 builtin_amba_driver(tmc_driver);

drivers/hwtracing/coresight/coresight-tmc.h +3 -0

index d156860495c7..f4f56c474e58 100644
@@ -262,6 +262,8 @@ u32 tmc_get_memwidth_mask(struct tmc_drvdata *drvdata);
 /* ETB/ETF functions */
 int tmc_read_prepare_etb(struct tmc_drvdata *drvdata);
 int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata);
+void tmc_etb_disable_hw(struct tmc_drvdata *drvdata);
+void tmc_etf_disable_hw(struct tmc_drvdata *drvdata);
 extern const struct coresight_ops tmc_etb_cs_ops;
 extern const struct coresight_ops tmc_etf_cs_ops;
 

@@ -270,6 +272,7 @@ ssize_t tmc_etb_get_sysfs_trace(struct tmc_drvdata *drvdata,
 /* 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);
 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);

Signature · 2 lines

QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Hi Sai,

On top of the comments already privided by Mike, I have the following:

Quoted 42 lines · Sai Prakash Ranjan

Implement a shutdown callback to ensure ETR/ETF hardware is properly shutdown in reboot/shutdown path. This is required for ETR/ETF which has SMMU address translation enabled like on SC7180 SoC and few others. If the hardware is still accessing memory after SMMU translation is disabled as part of SMMU shutdown callback in system reboot or shutdown path, then IOVAs(I/O virtual address) which it was using will go on the bus as the physical addresses which might result in unknown crashes (NoC/interconnect errors). So we make sure from this shutdown callback that the ETR/ETF is shutdown before SMMU translation is disabled and device_link in SMMU driver will take care of ordering of shutdown callbacks such that SMMU shutdown callback is not called before any of its consumer shutdown callbacks.

Signed-off-by Sai Prakash Ranjan <[email protected]>

---
 .../hwtracing/coresight/coresight-tmc-etf.c   |  4 +--
 .../hwtracing/coresight/coresight-tmc-etr.c   |  2 +-
 drivers/hwtracing/coresight/coresight-tmc.c   | 29 
+++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.h   |  3 ++
 4 files changed, 35 insertions(+), 3 deletions(-)

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

index 36cce2bfb744..cba3e7592820 100644
@@ -85,7 +85,7 @@ static void __tmc_etb_disable_hw(struct tmc_drvdata 
 *drvdata)
 	CS_LOCK(drvdata->base);
 }
 
-static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
 {
 	__tmc_etb_disable_hw(drvdata);

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

 	coresight_disclaim_device(drvdata->base);

@@ -118,7 +118,7 @@ static int tmc_etf_enable_hw(struct tmc_drvdata 
 *drvdata)
 	return 0;
 }
 

drivers/hwtracing/coresight/coresight-tmc-etf.c static int tmc_etf_enable_hw(struct tmc_drvdata *drvdata) · line 123

Quoted 5 lines

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

-static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
 {
 	CS_UNLOCK(drvdata->base);

Why do we care about ETB and ETF when they both use RAM internal to the device? Moreover, the system RAM they use is not dedicated and as such falls back to the kernel's memory pool.

Quoted 39 lines

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

index 625882bc8b08..b29c2db94d96 100644
@@ -1110,7 +1110,7 @@ static void __tmc_etr_disable_hw(struct 
 tmc_drvdata *drvdata)
 
 }
 
-static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 {
 	__tmc_etr_disable_hw(drvdata);

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

 	/* Disable CATU device if this ETR is connected to one */

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

index 5a271ebc4585..7e687a356fe0 100644
@@ -540,6 +540,34 @@ static int tmc_probe(struct amba_device *adev, 
 const struct amba_id *id)
 	return ret;
 }
 
+static void tmc_shutdown(struct amba_device *adev)
+{
+	struct tmc_drvdata *drvdata = amba_get_drvdata(adev);
+
+	if (!drvdata->enable)
+		goto out;
+
+	/*
+	 * We do not care about the active trace sessions here
+	 * since the system is going down unlike remove callback,
+	 * just make sure that the hardware is shutdown.
+	 */
+	switch (drvdata->config_type) {
+	case TMC_CONFIG_TYPE_ETB:
+		tmc_etb_disable_hw(drvdata);
+		break;

drivers/hwtracing/coresight/coresight-tmc.c static int tmc_probe(struct amba_device *adev, · line 569

Quoted 10 lines

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

+	case TMC_CONFIG_TYPE_ETF:
+		tmc_etf_disable_hw(drvdata);
+		break;
+	case TMC_CONFIG_TYPE_ETR:
+		tmc_etr_disable_hw(drvdata);
+	}
+
+out:
+	misc_deregister(&drvdata->miscdev);
+	coresight_unregister(drvdata->csdev);

If a session is active when tmc_shutdown() is called, unregistering the ETF/ETR will result in a kernel crash if the session is stopped before the kernel has had the opportunity to shutdown. It is the problem as trying to make coresight drivers modular.

For this to really work the ongoing session would need to be stopped. That would teardown the path and stop the sink.

That being said I'm sure that dependencies on an IOMMU isn't a problem confined to coresight. I am adding Robin Murphy, who added this commit [1], to the thread in the hope that he can provide guidance on the right way to do this.

Hi Mathieu,

Thanks for taking your time for review.

Quoted 48 lines · Mathieu Poirier

Hi Sai,

On top of the comments already privided by Mike, I have the following:

Quoted 43 lines · Sai Prakash Ranjan

Implement a shutdown callback to ensure ETR/ETF hardware is properly shutdown in reboot/shutdown path. This is required for ETR/ETF which has SMMU address translation enabled like on SC7180 SoC and few others. If the hardware is still accessing memory after SMMU translation is disabled as part of SMMU shutdown callback in system reboot or shutdown path, then IOVAs(I/O virtual address) which it was using will go on the bus as the physical addresses which might result in unknown crashes (NoC/interconnect errors). So we make sure from this shutdown callback that the ETR/ETF is shutdown before SMMU translation is disabled and device_link in SMMU driver will take care of ordering of shutdown callbacks such that SMMU shutdown callback is not called before any of its consumer shutdown callbacks.

Signed-off-by Sai Prakash Ranjan <[email protected]>

---
 .../hwtracing/coresight/coresight-tmc-etf.c   |  4 +--
 .../hwtracing/coresight/coresight-tmc-etr.c   |  2 +-
 drivers/hwtracing/coresight/coresight-tmc.c   | 29
+++++++++++++++++++
 drivers/hwtracing/coresight/coresight-tmc.h   |  3 ++
 4 files changed, 35 insertions(+), 3 deletions(-)

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

index 36cce2bfb744..cba3e7592820 100644
@@ -85,7 +85,7 @@ static void __tmc_etb_disable_hw(struct tmc_drvdata
 *drvdata)
     CS_LOCK(drvdata->base);
 }
 
-static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
 {
     __tmc_etb_disable_hw(drvdata);

     coresight_disclaim_device(drvdata->base);

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

@@ -118,7 +118,7 @@ static int tmc_etf_enable_hw(struct tmc_drvdata
 *drvdata)
     return 0;
 }
 

Quoted 12 lines

drivers/hwtracing/coresight/coresight-tmc-etf.c static int tmc_etf_enable_hw(struct tmc_drvdata *drvdata) · line 123

Quoted 5 lines

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

-static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
 {
     CS_UNLOCK(drvdata->base);

Why do we care about ETB and ETF when they both use RAM internal to the device?
Moreover, the system RAM they use is not dedicated and as such falls back to the
kernel's memory pool.

Actually we don't, I added the disable for ETF/ETB for completeness since we are
adding shutdown callback for TMC devices and not just ETR although this issue applies
only for ETR and it doesn't hurt to disable these devices in shutdown path.

Quoted 60 lines

Quoted 41 lines

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

index 625882bc8b08..b29c2db94d96 100644
@@ -1110,7 +1110,7 @@ static void __tmc_etr_disable_hw(struct
 tmc_drvdata *drvdata)
 
 }
 
-static void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
+void tmc_etr_disable_hw(struct tmc_drvdata *drvdata)
 {
     __tmc_etr_disable_hw(drvdata);

     /* Disable CATU device if this ETR is connected to one */

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

index 5a271ebc4585..7e687a356fe0 100644
@@ -540,6 +540,34 @@ static int tmc_probe(struct amba_device *adev,
 const struct amba_id *id)
     return ret;
 }
 
+static void tmc_shutdown(struct amba_device *adev)
+{
+    struct tmc_drvdata *drvdata = amba_get_drvdata(adev);
+
+    if (!drvdata->enable)
+            goto out;
+
+    /*
+     * We do not care about the active trace sessions here
+     * since the system is going down unlike remove callback,
+     * just make sure that the hardware is shutdown.
+     */
+    switch (drvdata->config_type) {
+    case TMC_CONFIG_TYPE_ETB:
+            tmc_etb_disable_hw(drvdata);
+            break;

drivers/hwtracing/coresight/coresight-tmc.c static int tmc_probe(struct amba_device *adev, · line 569

Quoted 10 lines

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

+    case TMC_CONFIG_TYPE_ETF:
+            tmc_etf_disable_hw(drvdata);
+            break;
+    case TMC_CONFIG_TYPE_ETR:
+            tmc_etr_disable_hw(drvdata);
+    }
+
+out:
+    misc_deregister(&drvdata->miscdev);
+    coresight_unregister(drvdata->csdev);

If a session is active when tmc_shutdown() is called, unregistering the ETF/ETR
will result in a kernel crash if the session is stopped before the kernel has
had the opportunity to shutdown. It is the problem as trying to make coresight
drivers modular.

Quoted 3 lines

For this to really work the ongoing session would need to be stopped. That
would teardown the path and stop the sink.

I have tested this with and without active trace sessions multiple times on 2 devices
and did not observe a single crash. The crash should be easily triggered as per
what you are saying if we have active sessions but I do not see any crash.

Quoted 7 lines

That being said I'm sure that dependencies on an IOMMU isn't a problem confined
to coresight. I am adding Robin Murphy, who added this commit [1], to the thread
in the hope that he can provide guidance on the right way to do this.

SMMU/IOMMU won't be able to do much here as it is the client's responsiblity to
properly shutdown and SMMU device link just makes sure that SMMU(supplier) shutdown is
called only after its consumers shutdown callbacks are called.

Thanks,
Sai

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Quoted 6 lines

SMMU/IOMMU won't be able to do much here as it is the client's responsiblity to
properly shutdown and SMMU device link just makes sure that SMMU(supplier) shutdown is
called only after its consumers shutdown callbacks are called.

I think this use case can be handled slightly differently than the general requirements for modular CoreSight drivers.

What is needed here is a way of stopping the underlying ETR hardware from issuing data to the SMMU, until the entire device has been shut down, in a way that does not remove the driver, breaking existing references and causing a system crash.

We could introduce a new mode to the ETR driver - e.g. CS_MODE_SHUTDOWN.

At the end of the block tmc_shutdown(struct amba_device *adev), set drvdata->mode to CS_MODE_SHUTDOWN & remove the coresight_unregister(). This new mode can be used to prevent the underlying hardware from being able to restart until the device is re-powered.

This mode can be detected in the code that enables / disables the ETR and handled appropriately (updates to tmc_enable_etr_sink and tmc_disable_etr_sink).
This mode will persist until the device is re-started - but because we are on the device shutdown path this is not an issue.

This should leave the CoreSight infrastructure stable until the drivers are shut down normally as part of the device power down process.

Hi Mike,

Thanks again for looking at this.

On 2020-06-03 03:42, Mike Leach wrote:
[...]

Quoted 29 lines

Quoted 6 lines

SMMU/IOMMU won't be able to do much here as it is the client's responsiblity to
properly shutdown and SMMU device link just makes sure that SMMU(supplier) shutdown is
called only after its consumers shutdown callbacks are called.

I think this use case can be handled slightly differently than the general requirements for modular CoreSight drivers.

What is needed here is a way of stopping the underlying ETR hardware from issuing data to the SMMU, until the entire device has been shut down, in a way that does not remove the driver, breaking existing references and causing a system crash.

We could introduce a new mode to the ETR driver - e.g. CS_MODE_SHUTDOWN.

At the end of the block tmc_shutdown(struct amba_device *adev), set drvdata->mode to CS_MODE_SHUTDOWN & remove the coresight_unregister(). This new mode can be used to prevent the underlying hardware from being able to restart until the device is re-powered.

This mode can be detected in the code that enables / disables the ETR and handled appropriately (updates to tmc_enable_etr_sink and tmc_disable_etr_sink).
This mode will persist until the device is re-started - but because we are on the device shutdown path this is not an issue.

Quoted 4 lines

This should leave the CoreSight infrastructure stable until the drivers are shut down normally as part of the device power down process.

Sounds good to me, but if the coresight_unregister() is the trouble point
causing these crashes, then can't we just remove that from tmc_shutdown()
callback? This would be like maintaining the same behaviour as now where on reboot/shutdown we basically don't do anything except for disabling ETR.

This way, we do not have to introduce any new mode as well. To be exact, in
tmc_shutdown() we just disable ETR and then return without unregistering which should not cause any issues since this is shutdown not the remove callback which is a requirement for making coresight modular like below:

static void tmc_shutdown(struct amba_device *adev)

  {
          unsigned long flags;
          struct tmc_drvdata *drvdata = amba_get_drvdata(adev);

          spin_lock_irqsave(&drvdata->spinlock, flags);

          if (drvdata->mode == CS_MODE_DISABLED)
                  goto out;

          if (drvdata->config_type == TMC_CONFIG_TYPE_ETR)
                  tmc_etr_disable_hw(drvdata);

          /*
           * We do not care about coresight unregister here unlike remove
           * callback which is required for making coresight modular

since
* the system is going down after this.
*/
out:

          spin_unlock_irqrestore(&drvdata->spinlock, flags);

} from disabling the ETR again - potentially freeing up memory twice.

Thanks,
Sai

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Hi,

Quoted 42 lines · Sai Prakash Ranjan

Hi Mike,

Thanks again for looking at this.

On 2020-06-03 03:42, Mike Leach wrote:
[...]

Quoted 29 lines

Quoted 6 lines

SMMU/IOMMU won't be able to do much here as it is the client's responsiblity to
properly shutdown and SMMU device link just makes sure that SMMU(supplier) shutdown is
called only after its consumers shutdown callbacks are called.

I think this use case can be handled slightly differently than the general requirements for modular CoreSight drivers.

What is needed here is a way of stopping the underlying ETR hardware from issuing data to the SMMU, until the entire device has been shut down, in a way that does not remove the driver, breaking existing references and causing a system crash.

We could introduce a new mode to the ETR driver - e.g. CS_MODE_SHUTDOWN.

At the end of the block tmc_shutdown(struct amba_device *adev), set drvdata->mode to CS_MODE_SHUTDOWN & remove the coresight_unregister(). This new mode can be used to prevent the underlying hardware from being able to restart until the device is re-powered.

This mode can be detected in the code that enables / disables the ETR and handled appropriately (updates to tmc_enable_etr_sink and tmc_disable_etr_sink).
This mode will persist until the device is re-started - but because we are on the device shutdown path this is not an issue.

Quoted 4 lines

This should leave the CoreSight infrastructure stable until the drivers are shut down normally as part of the device power down process.

Quoted 7 lines

Sounds good to me, but if the coresight_unregister() is the trouble point
causing these crashes, then can't we just remove that from tmc_shutdown()
callback? This would be like maintaining the same behaviour as now where on reboot/shutdown we basically don't do anything except for disabling ETR.

No - the new mode prevents race conditions where the thread shutting down the SMMU does the ETR shutdown, but then another thread happens to be trying to start trace and restarts the ETR.
It also prevents the condition Mathieu discussed where a thread might be attempting to shutdown trace - this could try to disable the hardware again re-releasing resources/ re-flushing and waiting for stop.

Hi Mike,

Quoted 55 lines · Mike Leach

Hi,

Quoted 42 lines · Sai Prakash Ranjan

Hi Mike,

Thanks again for looking at this.

On 2020-06-03 03:42, Mike Leach wrote:
[...]

Quoted 29 lines

Quoted 6 lines

SMMU/IOMMU won't be able to do much here as it is the client's responsiblity to
properly shutdown and SMMU device link just makes sure that SMMU(supplier) shutdown is
called only after its consumers shutdown callbacks are called.

I think this use case can be handled slightly differently than the general requirements for modular CoreSight drivers.

What is needed here is a way of stopping the underlying ETR hardware from issuing data to the SMMU, until the entire device has been shut down, in a way that does not remove the driver, breaking existing references and causing a system crash.

We could introduce a new mode to the ETR driver - e.g. CS_MODE_SHUTDOWN.

At the end of the block tmc_shutdown(struct amba_device *adev), set drvdata->mode to CS_MODE_SHUTDOWN & remove the coresight_unregister(). This new mode can be used to prevent the underlying hardware from being able to restart until the device is re-powered.

This mode can be detected in the code that enables / disables the ETR and handled appropriately (updates to tmc_enable_etr_sink and tmc_disable_etr_sink).
This mode will persist until the device is re-started - but because we are on the device shutdown path this is not an issue.

Quoted 4 lines

This should leave the CoreSight infrastructure stable until the drivers are shut down normally as part of the device power down process.

Quoted 8 lines

Sounds good to me, but if the coresight_unregister() is the trouble point
causing these crashes, then can't we just remove that from tmc_shutdown()
callback? This would be like maintaining the same behaviour as now where
on reboot/shutdown we basically don't do anything except for disabling ETR.

Quoted 8 lines

No - the new mode prevents race conditions where the thread shutting down the SMMU does the ETR shutdown, but then another thread happens to be trying to start trace and restarts the ETR.
It also prevents the condition Mathieu discussed where a thread might be attempting to shutdown trace - this could try to disable the hardware again re-releasing resources/ re-flushing and waiting for stop.

I do not think there will a race between SMMU shutdown and ETR shutdown. Driver core takes care of calling SMMU shutdown after its consumer shutdown callbacks via device link, otherwise there would already be bugs in all other client drivers.

Thanks,
Sai
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Hi Sai,

Quoted 68 lines · Sai Prakash Ranjan

Hi Mike,

Quoted 55 lines · Mike Leach

Hi,

Quoted 42 lines · Sai Prakash Ranjan

Hi Mike,

Thanks again for looking at this.

On 2020-06-03 03:42, Mike Leach wrote:
[...]

Quoted 29 lines

Quoted 6 lines

SMMU/IOMMU won't be able to do much here as it is the client's responsiblity to
properly shutdown and SMMU device link just makes sure that SMMU(supplier) shutdown is
called only after its consumers shutdown callbacks are called.

I think this use case can be handled slightly differently than the general requirements for modular CoreSight drivers.

What is needed here is a way of stopping the underlying ETR hardware from issuing data to the SMMU, until the entire device has been shut down, in a way that does not remove the driver, breaking existing references and causing a system crash.

We could introduce a new mode to the ETR driver - e.g. CS_MODE_SHUTDOWN.

At the end of the block tmc_shutdown(struct amba_device *adev), set drvdata->mode to CS_MODE_SHUTDOWN & remove the coresight_unregister(). This new mode can be used to prevent the underlying hardware from being able to restart until the device is re-powered.

This mode can be detected in the code that enables / disables the ETR and handled appropriately (updates to tmc_enable_etr_sink and tmc_disable_etr_sink).
This mode will persist until the device is re-started - but because we are on the device shutdown path this is not an issue.

Quoted 4 lines

This should leave the CoreSight infrastructure stable until the drivers are shut down normally as part of the device power down process.

Quoted 8 lines

Sounds good to me, but if the coresight_unregister() is the trouble point
causing these crashes, then can't we just remove that from tmc_shutdown()
callback? This would be like maintaining the same behaviour as now where
on reboot/shutdown we basically don't do anything except for disabling ETR.

Quoted 8 lines

No - the new mode prevents race conditions where the thread shutting down the SMMU does the ETR shutdown, but then another thread happens to be trying to start trace and restarts the ETR.
It also prevents the condition Mathieu discussed where a thread might be attempting to shutdown trace - this could try to disable the hardware again re-releasing resources/ re-flushing and waiting for stop.

Quoted 5 lines

I do not think there will a race between SMMU shutdown and ETR shutdown. Driver core takes care of calling SMMU shutdown after its consumer shutdown callbacks via device link, otherwise there would already be bugs in all other client drivers.

I am not saying there could be a race between tmc_shutdowm and Smmu_shutdown - there may be a case if the coresight_disable_path sequence is running and gets to the point of disabling the ETR after the SMMU callback has disabled it.

Quoted 78 lines · Mike Leach

Hi Sai,

Quoted 68 lines · Sai Prakash Ranjan

Hi Mike,

Quoted 55 lines · Mike Leach

Hi,

Quoted 42 lines · Sai Prakash Ranjan

Hi Mike,

Thanks again for looking at this.

On 2020-06-03 03:42, Mike Leach wrote:
[...]

Quoted 29 lines

Quoted 6 lines

SMMU/IOMMU won't be able to do much here as it is the client's responsiblity to
properly shutdown and SMMU device link just makes sure that SMMU(supplier) shutdown is
called only after its consumers shutdown callbacks are called.

I think this use case can be handled slightly differently than the general requirements for modular CoreSight drivers.

What is needed here is a way of stopping the underlying ETR hardware from issuing data to the SMMU, until the entire device has been shut down, in a way that does not remove the driver, breaking existing references and causing a system crash.

We could introduce a new mode to the ETR driver - e.g. CS_MODE_SHUTDOWN.

At the end of the block tmc_shutdown(struct amba_device *adev), set drvdata->mode to CS_MODE_SHUTDOWN & remove the coresight_unregister(). This new mode can be used to prevent the underlying hardware from being able to restart until the device is re-powered.

This mode can be detected in the code that enables / disables the ETR and handled appropriately (updates to tmc_enable_etr_sink and tmc_disable_etr_sink).
This mode will persist until the device is re-started - but because we are on the device shutdown path this is not an issue.

Quoted 4 lines

This should leave the CoreSight infrastructure stable until the drivers are shut down normally as part of the device power down process.

Quoted 8 lines

Sounds good to me, but if the coresight_unregister() is the trouble point
causing these crashes, then can't we just remove that from tmc_shutdown()
callback? This would be like maintaining the same behaviour as now where
on reboot/shutdown we basically don't do anything except for disabling ETR.

Quoted 8 lines

No - the new mode prevents race conditions where the thread shutting down the SMMU does the ETR shutdown, but then another thread happens to be trying to start trace and restarts the ETR.
It also prevents the condition Mathieu discussed where a thread might be attempting to shutdown trace - this could try to disable the hardware again re-releasing resources/ re-flushing and waiting for stop.

Quoted 5 lines

I do not think there will a race between SMMU shutdown and ETR shutdown. Driver core takes care of calling SMMU shutdown after its consumer shutdown callbacks via device link, otherwise there would already be bugs in all other client drivers.

Quoted 4 lines

I am not saying there could be a race between tmc_shutdowm and Smmu_shutdown - there may be a case if the coresight_disable_path sequence is running and gets to the point of disabling the ETR after the SMMU callback has disabled it.

I'm confused now - there is no "SMMU callback", we're talking about the system-wide cleanup from kernel_shutdown_prepare() or kernel_restart_prepare(). As far as I'm aware userspace should be long gone by that point, so although trace may have been left running, the chance of racing against other driver operations seems pretty unlikely.

Quoted 78 lines · Mike Leach

Hi Sai,

Quoted 68 lines · Sai Prakash Ranjan

Hi Mike,

Quoted 55 lines · Mike Leach

Hi,

Quoted 42 lines · Sai Prakash Ranjan

Hi Mike,

Thanks again for looking at this.

On 2020-06-03 03:42, Mike Leach wrote:
[...]

Quoted 29 lines

Quoted 6 lines

SMMU/IOMMU won't be able to do much here as it is the client's responsiblity to
properly shutdown and SMMU device link just makes sure that SMMU(supplier) shutdown is
called only after its consumers shutdown callbacks are called.

I think this use case can be handled slightly differently than the general requirements for modular CoreSight drivers.

What is needed here is a way of stopping the underlying ETR hardware from issuing data to the SMMU, until the entire device has been shut down, in a way that does not remove the driver, breaking existing references and causing a system crash.

We could introduce a new mode to the ETR driver - e.g. CS_MODE_SHUTDOWN.

At the end of the block tmc_shutdown(struct amba_device *adev), set drvdata->mode to CS_MODE_SHUTDOWN & remove the coresight_unregister(). This new mode can be used to prevent the underlying hardware from being able to restart until the device is re-powered.

This mode can be detected in the code that enables / disables the ETR and handled appropriately (updates to tmc_enable_etr_sink and tmc_disable_etr_sink).
This mode will persist until the device is re-started - but because we are on the device shutdown path this is not an issue.

Quoted 4 lines

This should leave the CoreSight infrastructure stable until the drivers are shut down normally as part of the device power down process.

Quoted 8 lines

Sounds good to me, but if the coresight_unregister() is the trouble point
causing these crashes, then can't we just remove that from tmc_shutdown()
callback? This would be like maintaining the same behaviour as now where
on reboot/shutdown we basically don't do anything except for disabling ETR.

Quoted 8 lines

No - the new mode prevents race conditions where the thread shutting down the SMMU does the ETR shutdown, but then another thread happens to be trying to start trace and restarts the ETR.
It also prevents the condition Mathieu discussed where a thread might be attempting to shutdown trace - this could try to disable the hardware again re-releasing resources/ re-flushing and waiting for stop.

Quoted 5 lines

I do not think there will a race between SMMU shutdown and ETR shutdown. Driver core takes care of calling SMMU shutdown after its consumer shutdown callbacks via device link, otherwise there would already be bugs in all other client drivers.

Quoted 4 lines

I am not saying there could be a race between tmc_shutdowm and Smmu_shutdown - there may be a case if the coresight_disable_path sequence is running and gets to the point of disabling the ETR after the SMMU callback has disabled it.

I'm confused now - there is no "SMMU callback", we're talking about the system-wide cleanup from kernel_shutdown_prepare() or kernel_restart_prepare(). As far as I'm aware userspace should be long gone by that point, so although trace may have been left running ||

chance of racing against other driver operations seems pretty unlikely.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

SMMU device link just makes sure that

Quoted 2 lines

Quoted 2 lines

SMMU(supplier) shutdown is
called only after its consumers shutdown callbacks are called.

Robin.

Hi,

Quoted 85 lines · Robin Murphy

Quoted 78 lines · Mike Leach

Hi Sai,

Quoted 68 lines · Sai Prakash Ranjan

Hi Mike,

Quoted 55 lines · Mike Leach

Hi,

Quoted 42 lines · Sai Prakash Ranjan

Hi Mike,

Thanks again for looking at this.

On 2020-06-03 03:42, Mike Leach wrote:
[...]

Quoted 29 lines

Quoted 6 lines

SMMU/IOMMU won't be able to do much here as it is the client's responsiblity to
properly shutdown and SMMU device link just makes sure that SMMU(supplier) shutdown is
called only after its consumers shutdown callbacks are called.

I think this use case can be handled slightly differently than the general requirements for modular CoreSight drivers.

What is needed here is a way of stopping the underlying ETR hardware from issuing data to the SMMU, until the entire device has been shut down, in a way that does not remove the driver, breaking existing references and causing a system crash.

We could introduce a new mode to the ETR driver - e.g. CS_MODE_SHUTDOWN.

At the end of the block tmc_shutdown(struct amba_device *adev), set drvdata->mode to CS_MODE_SHUTDOWN & remove the coresight_unregister(). This new mode can be used to prevent the underlying hardware from being able to restart until the device is re-powered.

This mode can be detected in the code that enables / disables the ETR and handled appropriately (updates to tmc_enable_etr_sink and tmc_disable_etr_sink).
This mode will persist until the device is re-started - but because we are on the device shutdown path this is not an issue.

Quoted 4 lines

This should leave the CoreSight infrastructure stable until the drivers are shut down normally as part of the device power down process.

Quoted 8 lines

Sounds good to me, but if the coresight_unregister() is the trouble point
causing these crashes, then can't we just remove that from tmc_shutdown()
callback? This would be like maintaining the same behaviour as now where
on reboot/shutdown we basically don't do anything except for disabling ETR.

Quoted 8 lines

No - the new mode prevents race conditions where the thread shutting down the SMMU does the ETR shutdown, but then another thread happens to be trying to start trace and restarts the ETR.
It also prevents the condition Mathieu discussed where a thread might be attempting to shutdown trace - this could try to disable the hardware again re-releasing resources/ re-flushing and waiting for stop.

Quoted 5 lines

I do not think there will a race between SMMU shutdown and ETR shutdown. Driver core takes care of calling SMMU shutdown after its consumer shutdown callbacks via device link, otherwise there would already be bugs in all other client drivers.

Quoted 4 lines

I am not saying there could be a race between tmc_shutdowm and Smmu_shutdown - there may be a case if the coresight_disable_path sequence is running and gets to the point of disabling the ETR after the SMMU callback has disabled it.

Quoted 4 lines

I'm confused now - there is no "SMMU callback", we're talking about the system-wide cleanup from kernel_shutdown_prepare() or kernel_restart_prepare(). As far as I'm aware userspace should be long gone by that point, so although trace may have been left running ||

           ((offset >= TRCCIDCVRn(0)) && (offset <= TRCVMIDCVRn(7)), 

the

Quoted 2 lines

chance of racing against other driver operations seems pretty unlikely.

Sorry - bad choice of terminology. I was referring to the SMMU ensuring that it had all its clients shut-down before if shut down. To quote Sai...

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

SMMU device link just makes sure that

Quoted 2 lines

Quoted 2 lines

SMMU(supplier) shutdown is
called only after its consumers shutdown callbacks are called.

I agree it is unlikely, but if removing the device from the CoreSight infrastructure via coresight_unregister() is a potential source of a crash, it would seem that there is a potential path where some CoreSight driver side work might be possible. therefore a mode to prevent this crash, and ensure that the device hardware remains off and not sending trace to SMMU until such time as shutdown / reboot restart occurs, seemed prudent.

Quoted 78 lines · Mike Leach

Hi Sai,

Quoted 68 lines · Sai Prakash Ranjan

Hi Mike,

Quoted 55 lines · Mike Leach

Hi,

Quoted 42 lines · Sai Prakash Ranjan

Hi Mike,

Thanks again for looking at this.

On 2020-06-03 03:42, Mike Leach wrote:
[...]

Quoted 29 lines

Quoted 6 lines

SMMU/IOMMU won't be able to do much here as it is the client's responsiblity to
properly shutdown and SMMU device link just makes sure that SMMU(supplier) shutdown is
called only after its consumers shutdown callbacks are called.

I think this use case can be handled slightly differently than the general requirements for modular CoreSight drivers.

What is needed here is a way of stopping the underlying ETR hardware from issuing data to the SMMU, until the entire device has been shut down, in a way that does not remove the driver, breaking existing references and causing a system crash.

We could introduce a new mode to the ETR driver - e.g. CS_MODE_SHUTDOWN.

At the end of the block tmc_shutdown(struct amba_device *adev), set drvdata->mode to CS_MODE_SHUTDOWN & remove the coresight_unregister(). This new mode can be used to prevent the underlying hardware from being able to restart until the device is re-powered.

This mode can be detected in the code that enables / disables the ETR and handled appropriately (updates to tmc_enable_etr_sink and tmc_disable_etr_sink).
This mode will persist until the device is re-started - but because we are on the device shutdown path this is not an issue.

Quoted 4 lines

This should leave the CoreSight infrastructure stable until the drivers are shut down normally as part of the device power down process.

Quoted 8 lines

Sounds good to me, but if the coresight_unregister() is the trouble point
causing these crashes, then can't we just remove that from tmc_shutdown()
callback? This would be like maintaining the same behaviour as now where
on reboot/shutdown we basically don't do anything except for disabling ETR.

Quoted 8 lines

No - the new mode prevents race conditions where the thread shutting down the SMMU does the ETR shutdown, but then another thread happens to be trying to start trace and restarts the ETR.
It also prevents the condition Mathieu discussed where a thread might be attempting to shutdown trace - this could try to disable the hardware again re-releasing resources/ re-flushing and waiting for stop.

Quoted 5 lines

I do not think there will a race between SMMU shutdown and ETR shutdown. Driver core takes care of calling SMMU shutdown after its consumer shutdown callbacks via device link, otherwise there would already be bugs in all other client drivers.

Quoted 4 lines

I am not saying there could be a race between tmc_shutdowm and Smmu_shutdown - there may be a case if the coresight_disable_path sequence is running and gets to the point of disabling the ETR after the SMMU callback has disabled it.

I'm confused now - there is no "SMMU callback", we're talking about the system-wide cleanup from kernel_shutdown_prepare() or kernel_restart_prepare(). As far as I'm aware userspace should be long gone by that point, so although trace may have been left running, the chance of racing against other driver operations seems pretty unlikely.

Robin.

Quoted 84 lines · Robin Murphy

Quoted 78 lines · Mike Leach

Hi Sai,

Quoted 68 lines · Sai Prakash Ranjan

Hi Mike,

Quoted 55 lines · Mike Leach

Hi,

Quoted 42 lines · Sai Prakash Ranjan

Hi Mike,

Thanks again for looking at this.

On 2020-06-03 03:42, Mike Leach wrote:
[...]

Quoted 29 lines

Quoted 6 lines

SMMU/IOMMU won't be able to do much here as it is the client's responsiblity to
properly shutdown and SMMU device link just makes sure that SMMU(supplier) shutdown is
called only after its consumers shutdown callbacks are called.

I think this use case can be handled slightly differently than the general requirements for modular CoreSight drivers.

What is needed here is a way of stopping the underlying ETR hardware from issuing data to the SMMU, until the entire device has been shut down, in a way that does not remove the driver, breaking existing references and causing a system crash.

We could introduce a new mode to the ETR driver - e.g. CS_MODE_SHUTDOWN.

At the end of the block tmc_shutdown(struct amba_device *adev), set drvdata->mode to CS_MODE_SHUTDOWN & remove the coresight_unregister(). This new mode can be used to prevent the underlying hardware from being able to restart until the device is re-powered.

This mode can be detected in the code that enables / disables the ETR and handled appropriately (updates to tmc_enable_etr_sink and tmc_disable_etr_sink).
This mode will persist until the device is re-started - but because we are on the device shutdown path this is not an issue.

Quoted 4 lines

This should leave the CoreSight infrastructure stable until the drivers are shut down normally as part of the device power down process.

Quoted 8 lines

Sounds good to me, but if the coresight_unregister() is the trouble point
causing these crashes, then can't we just remove that from tmc_shutdown()
callback? This would be like maintaining the same behaviour as now where
on reboot/shutdown we basically don't do anything except for disabling ETR.

Quoted 8 lines

No - the new mode prevents race conditions where the thread shutting down the SMMU does the ETR shutdown, but then another thread happens to be trying to start trace and restarts the ETR.
It also prevents the condition Mathieu discussed where a thread might be attempting to shutdown trace - this could try to disable the hardware again re-releasing resources/ re-flushing and waiting for stop.

Quoted 5 lines

I do not think there will a race between SMMU shutdown and ETR shutdown. Driver core takes care of calling SMMU shutdown after its consumer shutdown callbacks via device link, otherwise there would already be bugs in all other client drivers.

Quoted 4 lines

I am not saying there could be a race between tmc_shutdowm and Smmu_shutdown - there may be a case if the coresight_disable_path sequence is running and gets to the point of disabling the ETR after the SMMU callback has disabled it.

Quoted 5 lines

I'm confused now - there is no "SMMU callback", we're talking about the system-wide cleanup from kernel_shutdown_prepare() or kernel_restart_prepare(). As far as I'm aware userspace should be long gone by that point, so although trace may have been left running, the chance of racing against other driver operations seems pretty unlikely.

Robin has a point - user space is long gone at this time. As such the first question to ask is what kind of CS session was running at the time the system was shutting down. Was it a perf session of a sysfs session?

I'm guessing it was a sysfs session because user space has been blown away a while back and part of that process should have killed all perf sessions.

If I am correct then simply switching off the ETR HW in the shutdown() amba bus callback should be fine - otherwise Mike's approach is mandatory. There is also the exchange between Robin and Sai about removing the SMMU shutdown callback, but that thread is still incomplete.

Quoted 7 lines

Robin has a point - user space is long gone at this time. As such the first question to ask is what kind of CS session was running at the time the system was shutting down. Was it a perf session of a sysfs session?

I'm guessing it was a sysfs session because user space has been blown away a while back and part of that process should have killed all perf sessions.

I was enabling trace via sysfs.

Quoted 6 lines

If I am correct then simply switching off the ETR HW in the shutdown() amba bus callback should be fine - otherwise Mike's approach is mandatory. There is also the exchange between Robin and Sai about removing the SMMU shutdown callback, but that thread is still incomplete.

If Robin is hinting at removing SMMU shutdown callback, then I think adding all these shutdown callbacks to all clients of SMMU can be avoided. Git blaming the thing shows it was added to avoid some kexec memory corruption.

Hi Mathieu, Mike

Quoted 1 line · Sai Prakash Ranjan

[...]

Quoted 24 lines

Quoted 11 lines

Robin has a point - user space is long gone at this time. As such the first
question to ask is what kind of CS session was running at the time the system
was shutting down. Was it a perf session of a sysfs session?

I'm guessing it was a sysfs session because user space has been blown away a
while back and part of that process should have killed all perf sessions.

I was enabling trace via sysfs.

Quoted 9 lines

If I am correct then simply switching off the ETR HW in the shutdown() amba bus
callback should be fine - otherwise Mike's approach is mandatory. There is
also the exchange between Robin and Sai about removing the SMMU shutdown
callback, but that thread is still incomplete.

Quoted 6 lines

If Robin is hinting at removing SMMU shutdown callback, then I think adding
all these shutdown callbacks to all clients of SMMU can be avoided. Git blaming
the thing shows it was added to avoid some kexec memory corruption.

I think I misread the cryptic hint from Robin and it is not right to remove
SMMU shutdown callback. For more details on why that was a bad idea and would
break kexec, please refer to [1].

As for the coresight, can I disable the ETR only in the tmc shutdown callback
or are we still concerned about the userspace coming into picture?

[1] https://lore.kernel.org/patchwork/patch/1253131/

Thanks,
Sai

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

Quoted 45 lines · Sai Prakash Ranjan

Hi Mathieu, Mike

Quoted 1 line · Sai Prakash Ranjan

[...]

Quoted 24 lines

Quoted 11 lines

Robin has a point - user space is long gone at this time. As such the first
question to ask is what kind of CS session was running at the time the system
was shutting down. Was it a perf session of a sysfs session?

I'm guessing it was a sysfs session because user space has been blown away a
while back and part of that process should have killed all perf sessions.

I was enabling trace via sysfs.

Quoted 9 lines

If I am correct then simply switching off the ETR HW in the shutdown() amba bus
callback should be fine - otherwise Mike's approach is mandatory. There is
also the exchange between Robin and Sai about removing the SMMU shutdown
callback, but that thread is still incomplete.

Quoted 6 lines

If Robin is hinting at removing SMMU shutdown callback, then I think adding
all these shutdown callbacks to all clients of SMMU can be avoided. Git blaming
the thing shows it was added to avoid some kexec memory corruption.

I think I misread the cryptic hint from Robin and it is not right to remove
SMMU shutdown callback. For more details on why that was a bad idea and would
break kexec, please refer to [1].

Quoted 3 lines

As for the coresight, can I disable the ETR only in the tmc shutdown callback
or are we still concerned about the userspace coming into picture?

User space isn't a concern, especially after you've confirmed the problem occured during an ongoing sysfs session.

That being said I'm sure that dependencies on an IOMMU isn't a problem confined to coresight. I am adding Robin Murphy, who added this commit [1], to the thread in the hope that he can provide guidance on the right way to do this.

Quoted 3 lines · Mathieu Poirier

That being said I'm sure that dependencies on an IOMMU isn't a problem confined to coresight. I am adding Robin Murphy, who added this commit [1], to the thread in the hope that he can provide guidance on the right way to do this.

Right, it's not specific to CoreSight, and it's not even specific to IOMMUs really. In short, blame kexec ;)

The fundamental thing is that devices should stop any DMA activity at shutdown. For a normal poweroff you can typically get away without doing so, but over kexec, ongoing DMA traffic may corrupt memory in the new kernel (at worst, I think even DMA reads could potentially cause unexpected cache behaviour that might lead to mishaps, given the right combination of memory attributes).

IOMMUs merely help to make the situation more serious. For similar kexec reasons, they need to disable any existing translations at shutdown (imagine if the second kernel didn't have an IOMMU driver). And at that point, even the normal poweroff case becomes problematic, because any device DMA that hasn't been shut down beforehand is now not necessarily going benignly to memory as it would in the no-IOMMU case above, but potentially to random physical addresses, with all the hilarity ensuing that you would expect from that.

Hi Robin, Mathieu

Quoted 8 lines · Robin Murphy

Quoted 3 lines · Mathieu Poirier

That being said I'm sure that dependencies on an IOMMU isn't a problem confined to coresight. I am adding Robin Murphy, who added this commit [1], to the thread in the hope that he can provide guidance on the right way to do this.

Right, it's not specific to CoreSight, and it's not even specific to IOMMUs really. In short, blame kexec ;)

Yes it is not specific to coresight, we are targeting this for all consumers/clients of SMMU(atleast on SC7180 SoC). We have display throwing NoC/interconnect errors[1] during reboot after SMMU is disabled. This is also not specific to kexec either as you explained here [2] about a case with display which is exacly what is happening in our system [1].

[1] https://lore.kernel.org/lkml/[email protected]/

[2] https://lore.kernel.org/lkml/[email protected]/

Quoted 7 lines

The fundamental thing is that devices should stop any DMA activity at shutdown. For a normal poweroff you can typically get away without doing so, but over kexec, ongoing DMA traffic may corrupt memory in the new kernel (at worst, I think even DMA reads could potentially cause unexpected cache behaviour that might lead to mishaps, given the right combination of memory attributes).

Quoted 9 lines

IOMMUs merely help to make the situation more serious. For similar kexec reasons, they need to disable any existing translations at shutdown (imagine if the second kernel didn't have an IOMMU driver). And at that point, even the normal poweroff case becomes problematic, because any device DMA that hasn't been shut down beforehand is now not necessarily going benignly to memory as it would in the no-IOMMU case above, but potentially to random physical addresses, with all the hilarity ensuing that you would expect from that.

Thanks,
Sai

Quoted 12 lines · Sai Prakash Ranjan

Hi Robin, Mathieu

Quoted 8 lines · Robin Murphy

Quoted 3 lines · Mathieu Poirier

That being said I'm sure that dependencies on an IOMMU isn't a problem confined to coresight. I am adding Robin Murphy, who added this commit [1], to the thread in the hope that he can provide guidance on the right way to do this.

Right, it's not specific to CoreSight, and it's not even specific to IOMMUs really. In short, blame kexec ;)

Quoted 5 lines

Yes it is not specific to coresight, we are targeting this for all consumers/clients of SMMU(atleast on SC7180 SoC). We have display throwing NoC/interconnect errors[1] during reboot after SMMU is disabled. This is also not specific to kexec either as you explained here [2] about a case with display which is exacly what is happening in our system [1].

Sure, but those instances are begging the question of why the SMMU is disabled at reboot in the first place ;)

Hi Robin,

Quoted 19 lines · Robin Murphy

Quoted 17 lines · Sai Prakash Ranjan

Hi Robin, Mathieu

Quoted 8 lines · Robin Murphy

Quoted 3 lines · Mathieu Poirier

That being said I'm sure that dependencies on an IOMMU isn't a problem confined to coresight. I am adding Robin Murphy, who added this commit [1], to the thread in the hope that he can provide guidance on the right way to do this.

Right, it's not specific to CoreSight, and it's not even specific to IOMMUs really. In short, blame kexec ;)

Yes it is not specific to coresight, we are targeting this for all consumers/clients of SMMU(atleast on SC7180 SoC). We have display throwing NoC/interconnect errors[1] during reboot after SMMU is disabled. This is also not specific to kexec either as you explained here [2] about a case with display which is exacly what is happening in our system [1].

Quoted 3 lines

Sure, but those instances are begging the question of why the SMMU is disabled at reboot in the first place ;)

That is what happens in SMMU shutdown callback right? It is the reboot/shutdown flow.

   arm_smmu_device_shutdown()
    platform_drv_shutdown()
     device_shutdown()
      kernel_restart_prepare()
       kernel_restart()

Thanks,
Sai

Quoted 26 lines · Sai Prakash Ranjan

Hi Robin,

Quoted 22 lines · Robin Murphy

Quoted 12 lines · Sai Prakash Ranjan

Hi Robin, Mathieu

Quoted 8 lines · Robin Murphy

Quoted 3 lines · Mathieu Poirier

That being said I'm sure that dependencies on an IOMMU isn't a problem confined to coresight. I am adding Robin Murphy, who added this commit [1], to the thread in the hope that he can provide guidance on the right way to do this.

Right, it's not specific to CoreSight, and it's not even specific to IOMMUs really. In short, blame kexec ;)

Quoted 5 lines

Yes it is not specific to coresight, we are targeting this for all consumers/clients of SMMU(atleast on SC7180 SoC). We have display throwing NoC/interconnect errors[1] during reboot after SMMU is disabled. This is also not specific to kexec either as you explained here [2] about a case with display which is exacly what is happening in our system [1].

Sure, but those instances are begging the question of why the SMMU is disabled at reboot in the first place ;)

Quoted 1 line

That is what happens in SMMU shutdown callback right? It is the reboot/shutdown flow.

Yes, that's where it happens, but my point is *why* it happens at all.

hint: git log --grep=shutdown drivers/iommu/

If we could assume the system is always about to be powered off or reset, we wouldn't need to do anything to the SMMU either ;)