.../hwtracing/coresight/coresight-tmc-core.c | 4 +- .../hwtracing/coresight/coresight-tmc-etf.c | 43 +++++++++++++++---- .../hwtracing/coresight/coresight-tmc-etr.c | 18 ++++++-- drivers/hwtracing/coresight/coresight-tmc.h | 2 +- 4 files changed, 53 insertions(+), 14 deletions(-)
If TMC ETR is enabled without being ready, in later use we may
see AXI bus errors caused by accessing invalid addresses.
Signed-off-by: Yabin Cui <yabinc@google.com>
---
V1 -> V2: Make change to all TMCs instead of just ETR
.../hwtracing/coresight/coresight-tmc-core.c | 4 +-
.../hwtracing/coresight/coresight-tmc-etf.c | 43 +++++++++++++++----
.../hwtracing/coresight/coresight-tmc-etr.c | 18 ++++++--
drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
4 files changed, 53 insertions(+), 14 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
index 07abf28ad725..c106d142e632 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-core.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
@@ -31,7 +31,7 @@ DEFINE_CORESIGHT_DEVLIST(etb_devs, "tmc_etb");
DEFINE_CORESIGHT_DEVLIST(etf_devs, "tmc_etf");
DEFINE_CORESIGHT_DEVLIST(etr_devs, "tmc_etr");
-void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
+int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
{
struct coresight_device *csdev = drvdata->csdev;
struct csdev_access *csa = &csdev->access;
@@ -40,7 +40,9 @@ void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
if (coresight_timeout(csa, TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
dev_err(&csdev->dev,
"timeout while waiting for TMC to be Ready\n");
+ return -EBUSY;
}
+ return 0;
}
void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 4c4cbd1f7258..2840227e9135 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -16,12 +16,19 @@
static int tmc_set_etf_buffer(struct coresight_device *csdev,
struct perf_output_handle *handle);
-static void __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
+static int __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
{
+ int rc = 0;
+
CS_UNLOCK(drvdata->base);
/* Wait for TMCSReady bit to be set */
- tmc_wait_for_tmcready(drvdata);
+ rc = tmc_wait_for_tmcready(drvdata);
+ if (rc) {
+ dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
+ CS_LOCK(drvdata->base);
+ return rc;
+ }
writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI |
@@ -33,6 +40,7 @@ static void __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
tmc_enable_hw(drvdata);
CS_LOCK(drvdata->base);
+ return rc;
}
static int tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
@@ -42,8 +50,10 @@ static int tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
if (rc)
return rc;
- __tmc_etb_enable_hw(drvdata);
- return 0;
+ rc = __tmc_etb_enable_hw(drvdata);
+ if (rc)
+ coresight_disclaim_device(drvdata->csdev);
+ return rc;
}
static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata)
@@ -91,12 +101,19 @@ static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
coresight_disclaim_device(drvdata->csdev);
}
-static void __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
+static int __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
{
+ int rc = 0;
+
CS_UNLOCK(drvdata->base);
/* Wait for TMCSReady bit to be set */
- tmc_wait_for_tmcready(drvdata);
+ rc = tmc_wait_for_tmcready(drvdata);
+ if (rc) {
+ dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
+ CS_LOCK(drvdata->base);
+ return rc;
+ }
writel_relaxed(TMC_MODE_HARDWARE_FIFO, drvdata->base + TMC_MODE);
writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI,
@@ -105,6 +122,7 @@ static void __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
tmc_enable_hw(drvdata);
CS_LOCK(drvdata->base);
+ return rc;
}
static int tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
@@ -114,8 +132,10 @@ static int tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
if (rc)
return rc;
- __tmc_etf_enable_hw(drvdata);
- return 0;
+ rc = __tmc_etf_enable_hw(drvdata);
+ if (rc)
+ coresight_disclaim_device(drvdata->csdev);
+ return rc;
}
static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
@@ -639,6 +659,7 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
char *buf = NULL;
enum tmc_mode mode;
unsigned long flags;
+ int rc = 0;
/* config types are set a boot time and never change */
if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETB &&
@@ -664,7 +685,11 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
* can't be NULL.
*/
memset(drvdata->buf, 0, drvdata->size);
- __tmc_etb_enable_hw(drvdata);
+ rc = __tmc_etb_enable_hw(drvdata);
+ if (rc) {
+ spin_unlock_irqrestore(&drvdata->spinlock, flags);
+ return rc;
+ }
} else {
/*
* The ETB/ETF is not tracing and the buffer was just read.
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 867ad8bb9b0c..0811cb44588b 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -983,15 +983,21 @@ static void tmc_sync_etr_buf(struct tmc_drvdata *drvdata)
etr_buf->ops->sync(etr_buf, rrp, rwp);
}
-static void __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
+static int __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
{
u32 axictl, sts;
struct etr_buf *etr_buf = drvdata->etr_buf;
+ int rc = 0;
CS_UNLOCK(drvdata->base);
/* Wait for TMCSReady bit to be set */
- tmc_wait_for_tmcready(drvdata);
+ rc = tmc_wait_for_tmcready(drvdata);
+ if (rc) {
+ dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
+ CS_LOCK(drvdata->base);
+ return rc;
+ }
writel_relaxed(etr_buf->size / 4, drvdata->base + TMC_RSZ);
writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
@@ -1032,6 +1038,7 @@ static void __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
tmc_enable_hw(drvdata);
CS_LOCK(drvdata->base);
+ return rc;
}
static int tmc_etr_enable_hw(struct tmc_drvdata *drvdata,
@@ -1060,7 +1067,12 @@ static int tmc_etr_enable_hw(struct tmc_drvdata *drvdata,
rc = coresight_claim_device(drvdata->csdev);
if (!rc) {
drvdata->etr_buf = etr_buf;
- __tmc_etr_enable_hw(drvdata);
+ rc = __tmc_etr_enable_hw(drvdata);
+ if (rc) {
+ drvdata->etr_buf = NULL;
+ coresight_disclaim_device(drvdata->csdev);
+ tmc_etr_disable_catu(drvdata);
+ }
}
return rc;
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 66959557cf39..01c0382a29c0 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -255,7 +255,7 @@ struct tmc_sg_table {
};
/* Generic functions */
-void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
+int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
void tmc_flush_and_stop(struct tmc_drvdata *drvdata);
void tmc_enable_hw(struct tmc_drvdata *drvdata);
void tmc_disable_hw(struct tmc_drvdata *drvdata);
--
2.39.1.456.gfc5497dd1b-googHi,
On Fri, 27 Jan 2023 at 23:10, Yabin Cui <yabinc@google.com> wrote:
>
> If TMC ETR is enabled without being ready, in later use we may
> see AXI bus errors caused by accessing invalid addresses.
>
> Signed-off-by: Yabin Cui <yabinc@google.com>
> ---
> V1 -> V2: Make change to all TMCs instead of just ETR
>
> .../hwtracing/coresight/coresight-tmc-core.c | 4 +-
> .../hwtracing/coresight/coresight-tmc-etf.c | 43 +++++++++++++++----
> .../hwtracing/coresight/coresight-tmc-etr.c | 18 ++++++--
> drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
> 4 files changed, 53 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> index 07abf28ad725..c106d142e632 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> @@ -31,7 +31,7 @@ DEFINE_CORESIGHT_DEVLIST(etb_devs, "tmc_etb");
> DEFINE_CORESIGHT_DEVLIST(etf_devs, "tmc_etf");
> DEFINE_CORESIGHT_DEVLIST(etr_devs, "tmc_etr");
>
> -void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
> +int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
> {
> struct coresight_device *csdev = drvdata->csdev;
> struct csdev_access *csa = &csdev->access;
> @@ -40,7 +40,9 @@ void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
> if (coresight_timeout(csa, TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
> dev_err(&csdev->dev,
> "timeout while waiting for TMC to be Ready\n");
> + return -EBUSY;
> }
> + return 0;
> }
>
> void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index 4c4cbd1f7258..2840227e9135 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -16,12 +16,19 @@
> static int tmc_set_etf_buffer(struct coresight_device *csdev,
> struct perf_output_handle *handle);
>
> -static void __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
> +static int __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
> {
> + int rc = 0;
> +
> CS_UNLOCK(drvdata->base);
>
> /* Wait for TMCSReady bit to be set */
> - tmc_wait_for_tmcready(drvdata);
> + rc = tmc_wait_for_tmcready(drvdata);
> + if (rc) {
> + dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
> + CS_LOCK(drvdata->base);
> + return rc;
> + }
>
> writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
> writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI |
> @@ -33,6 +40,7 @@ static void __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
> tmc_enable_hw(drvdata);
>
> CS_LOCK(drvdata->base);
> + return rc;
> }
>
> static int tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
> @@ -42,8 +50,10 @@ static int tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
> if (rc)
> return rc;
>
> - __tmc_etb_enable_hw(drvdata);
> - return 0;
> + rc = __tmc_etb_enable_hw(drvdata);
> + if (rc)
> + coresight_disclaim_device(drvdata->csdev);
> + return rc;
> }
>
> static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata)
> @@ -91,12 +101,19 @@ static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
> coresight_disclaim_device(drvdata->csdev);
> }
>
> -static void __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
> +static int __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
> {
> + int rc = 0;
> +
> CS_UNLOCK(drvdata->base);
>
> /* Wait for TMCSReady bit to be set */
> - tmc_wait_for_tmcready(drvdata);
> + rc = tmc_wait_for_tmcready(drvdata);
> + if (rc) {
> + dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
> + CS_LOCK(drvdata->base);
> + return rc;
> + }
>
> writel_relaxed(TMC_MODE_HARDWARE_FIFO, drvdata->base + TMC_MODE);
> writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI,
> @@ -105,6 +122,7 @@ static void __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
> tmc_enable_hw(drvdata);
>
> CS_LOCK(drvdata->base);
> + return rc;
> }
>
> static int tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
> @@ -114,8 +132,10 @@ static int tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
> if (rc)
> return rc;
>
> - __tmc_etf_enable_hw(drvdata);
> - return 0;
> + rc = __tmc_etf_enable_hw(drvdata);
> + if (rc)
> + coresight_disclaim_device(drvdata->csdev);
> + return rc;
> }
>
> static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
> @@ -639,6 +659,7 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
> char *buf = NULL;
> enum tmc_mode mode;
> unsigned long flags;
> + int rc = 0;
>
> /* config types are set a boot time and never change */
> if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETB &&
> @@ -664,7 +685,11 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
> * can't be NULL.
> */
> memset(drvdata->buf, 0, drvdata->size);
> - __tmc_etb_enable_hw(drvdata);
> + rc = __tmc_etb_enable_hw(drvdata);
> + if (rc) {
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> + return rc;
> + }
There is a similar unprepare function in ETR - this should have similar updates.
> } else {
> /*
> * The ETB/ETF is not tracing and the buffer was just read.
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 867ad8bb9b0c..0811cb44588b 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -983,15 +983,21 @@ static void tmc_sync_etr_buf(struct tmc_drvdata *drvdata)
> etr_buf->ops->sync(etr_buf, rrp, rwp);
> }
>
> -static void __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
> +static int __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
> {
> u32 axictl, sts;
> struct etr_buf *etr_buf = drvdata->etr_buf;
> + int rc = 0;
>
> CS_UNLOCK(drvdata->base);
>
> /* Wait for TMCSReady bit to be set */
> - tmc_wait_for_tmcready(drvdata);
> + rc = tmc_wait_for_tmcready(drvdata);
> + if (rc) {
> + dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
> + CS_LOCK(drvdata->base);
> + return rc;
> + }
>
> writel_relaxed(etr_buf->size / 4, drvdata->base + TMC_RSZ);
> writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
> @@ -1032,6 +1038,7 @@ static void __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
> tmc_enable_hw(drvdata);
>
> CS_LOCK(drvdata->base);
> + return rc;
> }
>
> static int tmc_etr_enable_hw(struct tmc_drvdata *drvdata,
> @@ -1060,7 +1067,12 @@ static int tmc_etr_enable_hw(struct tmc_drvdata *drvdata,
> rc = coresight_claim_device(drvdata->csdev);
> if (!rc) {
> drvdata->etr_buf = etr_buf;
> - __tmc_etr_enable_hw(drvdata);
> + rc = __tmc_etr_enable_hw(drvdata);
> + if (rc) {
> + drvdata->etr_buf = NULL;
> + coresight_disclaim_device(drvdata->csdev);
> + tmc_etr_disable_catu(drvdata);
> + }
> }
>
> return rc;
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index 66959557cf39..01c0382a29c0 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -255,7 +255,7 @@ struct tmc_sg_table {
> };
>
> /* Generic functions */
> -void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
> +int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
> void tmc_flush_and_stop(struct tmc_drvdata *drvdata);
> void tmc_enable_hw(struct tmc_drvdata *drvdata);
> void tmc_disable_hw(struct tmc_drvdata *drvdata);
> --
> 2.39.1.456.gfc5497dd1b-goog
>
The tmc_flush_and_stop() function also calls tmc_wait_for_tmcready().
The etb/etf_prepare_read operations disable the etb /etf before a
read commences. For consistency if this fails due to TMC not being
ready do we also need to fail the read prepare operations?
Regards
Mike
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
Hi Mike,
Thanks for the comments, please note that I have queued this patch for next.
On 01/02/2023 20:43, Mike Leach wrote:
> Hi,
>
> On Fri, 27 Jan 2023 at 23:10, Yabin Cui <yabinc@google.com> wrote:
>>
>> If TMC ETR is enabled without being ready, in later use we may
>> see AXI bus errors caused by accessing invalid addresses.
>>
>> Signed-off-by: Yabin Cui <yabinc@google.com>
>> ---
>> V1 -> V2: Make change to all TMCs instead of just ETR
>>
>> .../hwtracing/coresight/coresight-tmc-core.c | 4 +-
>> .../hwtracing/coresight/coresight-tmc-etf.c | 43 +++++++++++++++----
>> .../hwtracing/coresight/coresight-tmc-etr.c | 18 ++++++--
>> drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
>> 4 files changed, 53 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
>> index 07abf28ad725..c106d142e632 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
>> @@ -31,7 +31,7 @@ DEFINE_CORESIGHT_DEVLIST(etb_devs, "tmc_etb");
>> DEFINE_CORESIGHT_DEVLIST(etf_devs, "tmc_etf");
>> DEFINE_CORESIGHT_DEVLIST(etr_devs, "tmc_etr");
>>
>> -void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
>> +int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
>> {
>> struct coresight_device *csdev = drvdata->csdev;
>> struct csdev_access *csa = &csdev->access;
>> @@ -40,7 +40,9 @@ void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
>> if (coresight_timeout(csa, TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
>> dev_err(&csdev->dev,
>> "timeout while waiting for TMC to be Ready\n");
>> + return -EBUSY;
>> }
>> + return 0;
>> }
>>
>> void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> index 4c4cbd1f7258..2840227e9135 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
>> @@ -16,12 +16,19 @@
>> static int tmc_set_etf_buffer(struct coresight_device *csdev,
>> struct perf_output_handle *handle);
>>
>> -static void __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
>> +static int __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
>> {
>> + int rc = 0;
>> +
>> CS_UNLOCK(drvdata->base);
>>
>> /* Wait for TMCSReady bit to be set */
>> - tmc_wait_for_tmcready(drvdata);
>> + rc = tmc_wait_for_tmcready(drvdata);
>> + if (rc) {
>> + dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
>> + CS_LOCK(drvdata->base);
>> + return rc;
>> + }
>>
>> writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
>> writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI |
>> @@ -33,6 +40,7 @@ static void __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
>> tmc_enable_hw(drvdata);
>>
>> CS_LOCK(drvdata->base);
>> + return rc;
>> }
>>
>> static int tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
>> @@ -42,8 +50,10 @@ static int tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
>> if (rc)
>> return rc;
>>
>> - __tmc_etb_enable_hw(drvdata);
>> - return 0;
>> + rc = __tmc_etb_enable_hw(drvdata);
>> + if (rc)
>> + coresight_disclaim_device(drvdata->csdev);
>> + return rc;
>> }
>>
>> static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata)
>> @@ -91,12 +101,19 @@ static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
>> coresight_disclaim_device(drvdata->csdev);
>> }
>>
>> -static void __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
>> +static int __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
>> {
>> + int rc = 0;
>> +
>> CS_UNLOCK(drvdata->base);
>>
>> /* Wait for TMCSReady bit to be set */
>> - tmc_wait_for_tmcready(drvdata);
>> + rc = tmc_wait_for_tmcready(drvdata);
>> + if (rc) {
>> + dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
>> + CS_LOCK(drvdata->base);
>> + return rc;
>> + }
>>
>> writel_relaxed(TMC_MODE_HARDWARE_FIFO, drvdata->base + TMC_MODE);
>> writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI,
>> @@ -105,6 +122,7 @@ static void __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
>> tmc_enable_hw(drvdata);
>>
>> CS_LOCK(drvdata->base);
>> + return rc;
>> }
>>
>> static int tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
>> @@ -114,8 +132,10 @@ static int tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
>> if (rc)
>> return rc;
>>
>> - __tmc_etf_enable_hw(drvdata);
>> - return 0;
>> + rc = __tmc_etf_enable_hw(drvdata);
>> + if (rc)
>> + coresight_disclaim_device(drvdata->csdev);
>> + return rc;
>> }
>>
>> static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
>> @@ -639,6 +659,7 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
>> char *buf = NULL;
>> enum tmc_mode mode;
>> unsigned long flags;
>> + int rc = 0;
>>
>> /* config types are set a boot time and never change */
>> if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETB &&
>> @@ -664,7 +685,11 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
>> * can't be NULL.
>> */
>> memset(drvdata->buf, 0, drvdata->size);
>> - __tmc_etb_enable_hw(drvdata);
>> + rc = __tmc_etb_enable_hw(drvdata);
>> + if (rc) {
>> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> + return rc;
>> + }
>
> There is a similar unprepare function in ETR - this should have similar updates.
>
>
>> } else {
>> /*
>> * The ETB/ETF is not tracing and the buffer was just read.
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> index 867ad8bb9b0c..0811cb44588b 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> @@ -983,15 +983,21 @@ static void tmc_sync_etr_buf(struct tmc_drvdata *drvdata)
>> etr_buf->ops->sync(etr_buf, rrp, rwp);
>> }
>>
>> -static void __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
>> +static int __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
>> {
>> u32 axictl, sts;
>> struct etr_buf *etr_buf = drvdata->etr_buf;
>> + int rc = 0;
>>
>> CS_UNLOCK(drvdata->base);
>>
>> /* Wait for TMCSReady bit to be set */
>> - tmc_wait_for_tmcready(drvdata);
>> + rc = tmc_wait_for_tmcready(drvdata);
>> + if (rc) {
>> + dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
>> + CS_LOCK(drvdata->base);
>> + return rc;
>> + }
>>
>> writel_relaxed(etr_buf->size / 4, drvdata->base + TMC_RSZ);
>> writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
>> @@ -1032,6 +1038,7 @@ static void __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
>> tmc_enable_hw(drvdata);
>>
>> CS_LOCK(drvdata->base);
>> + return rc;
>> }
>>
>> static int tmc_etr_enable_hw(struct tmc_drvdata *drvdata,
>> @@ -1060,7 +1067,12 @@ static int tmc_etr_enable_hw(struct tmc_drvdata *drvdata,
>> rc = coresight_claim_device(drvdata->csdev);
>> if (!rc) {
>> drvdata->etr_buf = etr_buf;
>> - __tmc_etr_enable_hw(drvdata);
>> + rc = __tmc_etr_enable_hw(drvdata);
>> + if (rc) {
>> + drvdata->etr_buf = NULL;
>> + coresight_disclaim_device(drvdata->csdev);
>> + tmc_etr_disable_catu(drvdata);
>> + }
>> }
>>
>> return rc;
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
>> index 66959557cf39..01c0382a29c0 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc.h
>> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
>> @@ -255,7 +255,7 @@ struct tmc_sg_table {
>> };
>>
>> /* Generic functions */
>> -void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
>> +int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
>> void tmc_flush_and_stop(struct tmc_drvdata *drvdata);
>> void tmc_enable_hw(struct tmc_drvdata *drvdata);
>> void tmc_disable_hw(struct tmc_drvdata *drvdata);
>> --
>> 2.39.1.456.gfc5497dd1b-goog
>>
>
> The tmc_flush_and_stop() function also calls tmc_wait_for_tmcready().
I think this already an error message there, when it fails to complete
the flush.I thought of adding a WARN_ON, but thought it is not worth much.
>
> The etb/etf_prepare_read operations disable the etb /etf before a
> read commences. For consistency if this fails due to TMC not being
> ready do we also need to fail the read prepare operations?
I think that should be OK, as we can read what the TMC has already
flushed/written to the memory. A following session may not be able
to use the ETR, which is fine.
Cheers
Suzuki
>
> Regards
>
> Mike
>
>
Hi, > > There is a similar unprepare function in ETR - this should have similar updates. Thanks for reminding me! I will update the patch to fix it. > > The tmc_flush_and_stop() function also calls tmc_wait_for_tmcready(). > > I think this already an error message there, when it fails to complete > the flush.I thought of adding a WARN_ON, but thought it is not worth much. I can see error messages when tmc flush timeouts or tmcready timeouts. I don't know how the callers (read_buffer and disable) can handle this error. I think the perf event call chain doesn't accept failures when disabling coresight devices. So I feel reporting the error is the best effort we can do. Thanks, Yabin
If TMC ETR is enabled without being ready, in later use we may
see AXI bus errors caused by accessing invalid addresses.
Signed-off-by: Yabin Cui <yabinc@google.com>
---
V1 -> V2: Make change to all TMCs instead of just ETR
V2 -> V3: Handle etr enable failure in tmc_read_unprepare_etr
.../hwtracing/coresight/coresight-tmc-core.c | 4 +-
.../hwtracing/coresight/coresight-tmc-etf.c | 43 +++++++++++++++----
.../hwtracing/coresight/coresight-tmc-etr.c | 25 +++++++++--
drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
4 files changed, 59 insertions(+), 15 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
index 07abf28ad725..c106d142e632 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-core.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
@@ -31,7 +31,7 @@ DEFINE_CORESIGHT_DEVLIST(etb_devs, "tmc_etb");
DEFINE_CORESIGHT_DEVLIST(etf_devs, "tmc_etf");
DEFINE_CORESIGHT_DEVLIST(etr_devs, "tmc_etr");
-void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
+int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
{
struct coresight_device *csdev = drvdata->csdev;
struct csdev_access *csa = &csdev->access;
@@ -40,7 +40,9 @@ void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
if (coresight_timeout(csa, TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
dev_err(&csdev->dev,
"timeout while waiting for TMC to be Ready\n");
+ return -EBUSY;
}
+ return 0;
}
void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 4c4cbd1f7258..2840227e9135 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -16,12 +16,19 @@
static int tmc_set_etf_buffer(struct coresight_device *csdev,
struct perf_output_handle *handle);
-static void __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
+static int __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
{
+ int rc = 0;
+
CS_UNLOCK(drvdata->base);
/* Wait for TMCSReady bit to be set */
- tmc_wait_for_tmcready(drvdata);
+ rc = tmc_wait_for_tmcready(drvdata);
+ if (rc) {
+ dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
+ CS_LOCK(drvdata->base);
+ return rc;
+ }
writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI |
@@ -33,6 +40,7 @@ static void __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
tmc_enable_hw(drvdata);
CS_LOCK(drvdata->base);
+ return rc;
}
static int tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
@@ -42,8 +50,10 @@ static int tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
if (rc)
return rc;
- __tmc_etb_enable_hw(drvdata);
- return 0;
+ rc = __tmc_etb_enable_hw(drvdata);
+ if (rc)
+ coresight_disclaim_device(drvdata->csdev);
+ return rc;
}
static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata)
@@ -91,12 +101,19 @@ static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
coresight_disclaim_device(drvdata->csdev);
}
-static void __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
+static int __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
{
+ int rc = 0;
+
CS_UNLOCK(drvdata->base);
/* Wait for TMCSReady bit to be set */
- tmc_wait_for_tmcready(drvdata);
+ rc = tmc_wait_for_tmcready(drvdata);
+ if (rc) {
+ dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
+ CS_LOCK(drvdata->base);
+ return rc;
+ }
writel_relaxed(TMC_MODE_HARDWARE_FIFO, drvdata->base + TMC_MODE);
writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI,
@@ -105,6 +122,7 @@ static void __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
tmc_enable_hw(drvdata);
CS_LOCK(drvdata->base);
+ return rc;
}
static int tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
@@ -114,8 +132,10 @@ static int tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
if (rc)
return rc;
- __tmc_etf_enable_hw(drvdata);
- return 0;
+ rc = __tmc_etf_enable_hw(drvdata);
+ if (rc)
+ coresight_disclaim_device(drvdata->csdev);
+ return rc;
}
static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
@@ -639,6 +659,7 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
char *buf = NULL;
enum tmc_mode mode;
unsigned long flags;
+ int rc = 0;
/* config types are set a boot time and never change */
if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETB &&
@@ -664,7 +685,11 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
* can't be NULL.
*/
memset(drvdata->buf, 0, drvdata->size);
- __tmc_etb_enable_hw(drvdata);
+ rc = __tmc_etb_enable_hw(drvdata);
+ if (rc) {
+ spin_unlock_irqrestore(&drvdata->spinlock, flags);
+ return rc;
+ }
} else {
/*
* The ETB/ETF is not tracing and the buffer was just read.
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 867ad8bb9b0c..4952425dd6e4 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -983,15 +983,21 @@ static void tmc_sync_etr_buf(struct tmc_drvdata *drvdata)
etr_buf->ops->sync(etr_buf, rrp, rwp);
}
-static void __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
+static int __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
{
u32 axictl, sts;
struct etr_buf *etr_buf = drvdata->etr_buf;
+ int rc = 0;
CS_UNLOCK(drvdata->base);
/* Wait for TMCSReady bit to be set */
- tmc_wait_for_tmcready(drvdata);
+ rc = tmc_wait_for_tmcready(drvdata);
+ if (rc) {
+ dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
+ CS_LOCK(drvdata->base);
+ return rc;
+ }
writel_relaxed(etr_buf->size / 4, drvdata->base + TMC_RSZ);
writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
@@ -1032,6 +1038,7 @@ static void __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
tmc_enable_hw(drvdata);
CS_LOCK(drvdata->base);
+ return rc;
}
static int tmc_etr_enable_hw(struct tmc_drvdata *drvdata,
@@ -1060,7 +1067,12 @@ static int tmc_etr_enable_hw(struct tmc_drvdata *drvdata,
rc = coresight_claim_device(drvdata->csdev);
if (!rc) {
drvdata->etr_buf = etr_buf;
- __tmc_etr_enable_hw(drvdata);
+ rc = __tmc_etr_enable_hw(drvdata);
+ if (rc) {
+ drvdata->etr_buf = NULL;
+ coresight_disclaim_device(drvdata->csdev);
+ tmc_etr_disable_catu(drvdata);
+ }
}
return rc;
@@ -1750,6 +1762,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
{
unsigned long flags;
struct etr_buf *sysfs_buf = NULL;
+ int rc = 0;
/* config types are set a boot time and never change */
if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETR))
@@ -1764,7 +1777,11 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
* buffer. Since the tracer is still enabled drvdata::buf can't
* be NULL.
*/
- __tmc_etr_enable_hw(drvdata);
+ rc = __tmc_etr_enable_hw(drvdata);
+ if (rc) {
+ spin_unlock_irqrestore(&drvdata->spinlock, flags);
+ return rc;
+ }
} else {
/*
* The ETR is not tracing and the buffer was just read.
diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
index 66959557cf39..01c0382a29c0 100644
--- a/drivers/hwtracing/coresight/coresight-tmc.h
+++ b/drivers/hwtracing/coresight/coresight-tmc.h
@@ -255,7 +255,7 @@ struct tmc_sg_table {
};
/* Generic functions */
-void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
+int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
void tmc_flush_and_stop(struct tmc_drvdata *drvdata);
void tmc_enable_hw(struct tmc_drvdata *drvdata);
void tmc_disable_hw(struct tmc_drvdata *drvdata);
--
2.39.1.519.gcb327c4b5f-googFriendly ping?
On Thu, Feb 2, 2023 at 1:46 PM Yabin Cui <yabinc@google.com> wrote:
>
> If TMC ETR is enabled without being ready, in later use we may
> see AXI bus errors caused by accessing invalid addresses.
>
> Signed-off-by: Yabin Cui <yabinc@google.com>
> ---
> V1 -> V2: Make change to all TMCs instead of just ETR
> V2 -> V3: Handle etr enable failure in tmc_read_unprepare_etr
>
> .../hwtracing/coresight/coresight-tmc-core.c | 4 +-
> .../hwtracing/coresight/coresight-tmc-etf.c | 43 +++++++++++++++----
> .../hwtracing/coresight/coresight-tmc-etr.c | 25 +++++++++--
> drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
> 4 files changed, 59 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> index 07abf28ad725..c106d142e632 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> @@ -31,7 +31,7 @@ DEFINE_CORESIGHT_DEVLIST(etb_devs, "tmc_etb");
> DEFINE_CORESIGHT_DEVLIST(etf_devs, "tmc_etf");
> DEFINE_CORESIGHT_DEVLIST(etr_devs, "tmc_etr");
>
> -void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
> +int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
> {
> struct coresight_device *csdev = drvdata->csdev;
> struct csdev_access *csa = &csdev->access;
> @@ -40,7 +40,9 @@ void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata)
> if (coresight_timeout(csa, TMC_STS, TMC_STS_TMCREADY_BIT, 1)) {
> dev_err(&csdev->dev,
> "timeout while waiting for TMC to be Ready\n");
> + return -EBUSY;
> }
> + return 0;
> }
>
> void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index 4c4cbd1f7258..2840227e9135 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -16,12 +16,19 @@
> static int tmc_set_etf_buffer(struct coresight_device *csdev,
> struct perf_output_handle *handle);
>
> -static void __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
> +static int __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
> {
> + int rc = 0;
> +
> CS_UNLOCK(drvdata->base);
>
> /* Wait for TMCSReady bit to be set */
> - tmc_wait_for_tmcready(drvdata);
> + rc = tmc_wait_for_tmcready(drvdata);
> + if (rc) {
> + dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
> + CS_LOCK(drvdata->base);
> + return rc;
> + }
>
> writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
> writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI |
> @@ -33,6 +40,7 @@ static void __tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
> tmc_enable_hw(drvdata);
>
> CS_LOCK(drvdata->base);
> + return rc;
> }
>
> static int tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
> @@ -42,8 +50,10 @@ static int tmc_etb_enable_hw(struct tmc_drvdata *drvdata)
> if (rc)
> return rc;
>
> - __tmc_etb_enable_hw(drvdata);
> - return 0;
> + rc = __tmc_etb_enable_hw(drvdata);
> + if (rc)
> + coresight_disclaim_device(drvdata->csdev);
> + return rc;
> }
>
> static void tmc_etb_dump_hw(struct tmc_drvdata *drvdata)
> @@ -91,12 +101,19 @@ static void tmc_etb_disable_hw(struct tmc_drvdata *drvdata)
> coresight_disclaim_device(drvdata->csdev);
> }
>
> -static void __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
> +static int __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
> {
> + int rc = 0;
> +
> CS_UNLOCK(drvdata->base);
>
> /* Wait for TMCSReady bit to be set */
> - tmc_wait_for_tmcready(drvdata);
> + rc = tmc_wait_for_tmcready(drvdata);
> + if (rc) {
> + dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
> + CS_LOCK(drvdata->base);
> + return rc;
> + }
>
> writel_relaxed(TMC_MODE_HARDWARE_FIFO, drvdata->base + TMC_MODE);
> writel_relaxed(TMC_FFCR_EN_FMT | TMC_FFCR_EN_TI,
> @@ -105,6 +122,7 @@ static void __tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
> tmc_enable_hw(drvdata);
>
> CS_LOCK(drvdata->base);
> + return rc;
> }
>
> static int tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
> @@ -114,8 +132,10 @@ static int tmc_etf_enable_hw(struct tmc_drvdata *drvdata)
> if (rc)
> return rc;
>
> - __tmc_etf_enable_hw(drvdata);
> - return 0;
> + rc = __tmc_etf_enable_hw(drvdata);
> + if (rc)
> + coresight_disclaim_device(drvdata->csdev);
> + return rc;
> }
>
> static void tmc_etf_disable_hw(struct tmc_drvdata *drvdata)
> @@ -639,6 +659,7 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
> char *buf = NULL;
> enum tmc_mode mode;
> unsigned long flags;
> + int rc = 0;
>
> /* config types are set a boot time and never change */
> if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETB &&
> @@ -664,7 +685,11 @@ int tmc_read_unprepare_etb(struct tmc_drvdata *drvdata)
> * can't be NULL.
> */
> memset(drvdata->buf, 0, drvdata->size);
> - __tmc_etb_enable_hw(drvdata);
> + rc = __tmc_etb_enable_hw(drvdata);
> + if (rc) {
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> + return rc;
> + }
> } else {
> /*
> * The ETB/ETF is not tracing and the buffer was just read.
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 867ad8bb9b0c..4952425dd6e4 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -983,15 +983,21 @@ static void tmc_sync_etr_buf(struct tmc_drvdata *drvdata)
> etr_buf->ops->sync(etr_buf, rrp, rwp);
> }
>
> -static void __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
> +static int __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
> {
> u32 axictl, sts;
> struct etr_buf *etr_buf = drvdata->etr_buf;
> + int rc = 0;
>
> CS_UNLOCK(drvdata->base);
>
> /* Wait for TMCSReady bit to be set */
> - tmc_wait_for_tmcready(drvdata);
> + rc = tmc_wait_for_tmcready(drvdata);
> + if (rc) {
> + dev_err(&drvdata->csdev->dev, "fails to enable not ready TMC\n");
> + CS_LOCK(drvdata->base);
> + return rc;
> + }
>
> writel_relaxed(etr_buf->size / 4, drvdata->base + TMC_RSZ);
> writel_relaxed(TMC_MODE_CIRCULAR_BUFFER, drvdata->base + TMC_MODE);
> @@ -1032,6 +1038,7 @@ static void __tmc_etr_enable_hw(struct tmc_drvdata *drvdata)
> tmc_enable_hw(drvdata);
>
> CS_LOCK(drvdata->base);
> + return rc;
> }
>
> static int tmc_etr_enable_hw(struct tmc_drvdata *drvdata,
> @@ -1060,7 +1067,12 @@ static int tmc_etr_enable_hw(struct tmc_drvdata *drvdata,
> rc = coresight_claim_device(drvdata->csdev);
> if (!rc) {
> drvdata->etr_buf = etr_buf;
> - __tmc_etr_enable_hw(drvdata);
> + rc = __tmc_etr_enable_hw(drvdata);
> + if (rc) {
> + drvdata->etr_buf = NULL;
> + coresight_disclaim_device(drvdata->csdev);
> + tmc_etr_disable_catu(drvdata);
> + }
> }
>
> return rc;
> @@ -1750,6 +1762,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
> {
> unsigned long flags;
> struct etr_buf *sysfs_buf = NULL;
> + int rc = 0;
>
> /* config types are set a boot time and never change */
> if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETR))
> @@ -1764,7 +1777,11 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
> * buffer. Since the tracer is still enabled drvdata::buf can't
> * be NULL.
> */
> - __tmc_etr_enable_hw(drvdata);
> + rc = __tmc_etr_enable_hw(drvdata);
> + if (rc) {
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> + return rc;
> + }
> } else {
> /*
> * The ETR is not tracing and the buffer was just read.
> diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h
> index 66959557cf39..01c0382a29c0 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc.h
> +++ b/drivers/hwtracing/coresight/coresight-tmc.h
> @@ -255,7 +255,7 @@ struct tmc_sg_table {
> };
>
> /* Generic functions */
> -void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
> +int tmc_wait_for_tmcready(struct tmc_drvdata *drvdata);
> void tmc_flush_and_stop(struct tmc_drvdata *drvdata);
> void tmc_enable_hw(struct tmc_drvdata *drvdata);
> void tmc_disable_hw(struct tmc_drvdata *drvdata);
> --
> 2.39.1.519.gcb327c4b5f-goog
>
On 09/02/2023 01:08, Yabin Cui wrote: > Friendly ping? > > On Thu, Feb 2, 2023 at 1:46 PM Yabin Cui <yabinc@google.com> wrote: >> >> If TMC ETR is enabled without being ready, in later use we may >> see AXI bus errors caused by accessing invalid addresses. >> >> Signed-off-by: Yabin Cui <yabinc@google.com> >> --- >> V1 -> V2: Make change to all TMCs instead of just ETR >> V2 -> V3: Handle etr enable failure in tmc_read_unprepare_etr As I mentioned, v2 was queued. Please could you update your changes on top of the coresight next branch and resend the patch ? Suzuki
It's similar to what we did in tmc_read_unprepare_etb.
Signed-off-by: Yabin Cui <yabinc@google.com>
---
drivers/hwtracing/coresight/coresight-tmc-etr.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 918d461fcf4a..b04f12079efd 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
@@ -1763,6 +1763,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
{
unsigned long flags;
struct etr_buf *sysfs_buf = NULL;
+ int rc = 0;
/* config types are set a boot time and never change */
if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETR))
@@ -1777,7 +1778,11 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
* buffer. Since the tracer is still enabled drvdata::buf can't
* be NULL.
*/
- __tmc_etr_enable_hw(drvdata);
+ rc = __tmc_etr_enable_hw(drvdata);
+ if (rc) {
+ spin_unlock_irqrestore(&drvdata->spinlock, flags);
+ return rc;
+ }
} else {
/*
* The ETR is not tracing and the buffer was just read.
--
2.39.1.581.gbfd45094c4-googPing for review?
On Fri, Feb 10, 2023 at 11:43 PM Yabin Cui <yabinc@google.com> wrote:
>
> It's similar to what we did in tmc_read_unprepare_etb.
>
> Signed-off-by: Yabin Cui <yabinc@google.com>
> ---
> drivers/hwtracing/coresight/coresight-tmc-etr.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 918d461fcf4a..b04f12079efd 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1763,6 +1763,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
> {
> unsigned long flags;
> struct etr_buf *sysfs_buf = NULL;
> + int rc = 0;
>
> /* config types are set a boot time and never change */
> if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETR))
> @@ -1777,7 +1778,11 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
> * buffer. Since the tracer is still enabled drvdata::buf can't
> * be NULL.
> */
> - __tmc_etr_enable_hw(drvdata);
> + rc = __tmc_etr_enable_hw(drvdata);
> + if (rc) {
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> + return rc;
> + }
> } else {
> /*
> * The ETR is not tracing and the buffer was just read.
> --
> 2.39.1.581.gbfd45094c4-goog
>
On 21/02/2023 18:38, Yabin Cui wrote:
> Ping for review?
>
> On Fri, Feb 10, 2023 at 11:43 PM Yabin Cui <yabinc@google.com> wrote:
>>
>> It's similar to what we did in tmc_read_unprepare_etb.
>>
>> Signed-off-by: Yabin Cui <yabinc@google.com>
>> ---
Thanks Yabin for the patch, will queue this at rc1
Suzuki
>> drivers/hwtracing/coresight/coresight-tmc-etr.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> index 918d461fcf4a..b04f12079efd 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> @@ -1763,6 +1763,7 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
>> {
>> unsigned long flags;
>> struct etr_buf *sysfs_buf = NULL;
>> + int rc = 0;
>>
>> /* config types are set a boot time and never change */
>> if (WARN_ON_ONCE(drvdata->config_type != TMC_CONFIG_TYPE_ETR))
>> @@ -1777,7 +1778,11 @@ int tmc_read_unprepare_etr(struct tmc_drvdata *drvdata)
>> * buffer. Since the tracer is still enabled drvdata::buf can't
>> * be NULL.
>> */
>> - __tmc_etr_enable_hw(drvdata);
>> + rc = __tmc_etr_enable_hw(drvdata);
>> + if (rc) {
>> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> + return rc;
>> + }
>> } else {
>> /*
>> * The ETR is not tracing and the buffer was just read.
>> --
>> 2.39.1.581.gbfd45094c4-goog
>>
© 2016 - 2025 Red Hat, Inc.