* [PATCH] coresight: tmc-etr: Don't enable ETR when it's not ready
@ 2023-01-09 23:43 Yabin Cui
2023-01-10 9:30 ` James Clark
0 siblings, 1 reply; 18+ messages in thread
From: Yabin Cui @ 2023-01-09 23:43 UTC (permalink / raw)
To: Mathieu Poirier, Suzuki K Poulose, Mike Leach, Leo Yan
Cc: coresight, linux-arm-kernel, linux-kernel, Yabin Cui
Otherwise, it may cause error in AXI bus and result in a kernel panic.
Signed-off-by: Yabin Cui <yabinc@google.com>
---
.../hwtracing/coresight/coresight-tmc-core.c | 4 +++-
.../hwtracing/coresight/coresight-tmc-etr.c | 18 +++++++++++++++---
drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
3 files changed, 19 insertions(+), 5 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-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
index 867ad8bb9b0c..2da99dd41ed6 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, "not ready ETR isn't enabled\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.0.314.g84b9a713c41-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] coresight: tmc-etr: Don't enable ETR when it's not ready
2023-01-09 23:43 [PATCH] coresight: tmc-etr: Don't enable ETR when it's not ready Yabin Cui
@ 2023-01-10 9:30 ` James Clark
2023-01-10 17:48 ` Mike Leach
0 siblings, 1 reply; 18+ messages in thread
From: James Clark @ 2023-01-10 9:30 UTC (permalink / raw)
To: Yabin Cui
Cc: coresight, linux-arm-kernel, linux-kernel, Mathieu Poirier,
Suzuki K Poulose, Mike Leach, Leo Yan
On 09/01/2023 23:43, Yabin Cui wrote:
> Otherwise, it may cause error in AXI bus and result in a kernel panic.
Hi Yabin,
Thanks for the fix. Do you have a reproducer for this or some more info?
For example is it a regression or has it always been there? And on which
platform.
Thanks
James
>
> Signed-off-by: Yabin Cui <yabinc@google.com>
> ---
> .../hwtracing/coresight/coresight-tmc-core.c | 4 +++-
> .../hwtracing/coresight/coresight-tmc-etr.c | 18 +++++++++++++++---
> drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
> 3 files changed, 19 insertions(+), 5 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-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 867ad8bb9b0c..2da99dd41ed6 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, "not ready ETR isn't enabled\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);
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] coresight: tmc-etr: Don't enable ETR when it's not ready
2023-01-10 9:30 ` James Clark
@ 2023-01-10 17:48 ` Mike Leach
2023-01-10 18:04 ` Suzuki K Poulose
0 siblings, 1 reply; 18+ messages in thread
From: Mike Leach @ 2023-01-10 17:48 UTC (permalink / raw)
To: James Clark
Cc: Yabin Cui, coresight, linux-arm-kernel, linux-kernel,
Mathieu Poirier, Suzuki K Poulose, Leo Yan
Hi,
On Tue, 10 Jan 2023 at 09:30, James Clark <james.clark@arm.com> wrote:
>
>
>
> On 09/01/2023 23:43, Yabin Cui wrote:
> > Otherwise, it may cause error in AXI bus and result in a kernel panic.
>
> Hi Yabin,
>
> Thanks for the fix. Do you have a reproducer for this or some more info?
> For example is it a regression or has it always been there? And on which
> platform.
>
> Thanks
> James
>
> >
> > Signed-off-by: Yabin Cui <yabinc@google.com>
> > ---
> > .../hwtracing/coresight/coresight-tmc-core.c | 4 +++-
> > .../hwtracing/coresight/coresight-tmc-etr.c | 18 +++++++++++++++---
> > drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
> > 3 files changed, 19 insertions(+), 5 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-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > index 867ad8bb9b0c..2da99dd41ed6 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, "not ready ETR isn't enabled\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);
There is no point in waiting for a timeout, and then carrying on even
when it is exceeded. As such this patch seems reasonable.
We should also apply the same principle to the ETF and ETB devices
which use the same tmc_wait_for_tmcready() function.
However - the concern is that this appears to be happening on starting
the ETR - there should be no outstanding AXI operations that cause the
system to not be ready - as we will either be using this the first
time after reset, or we should have successfully stopped and flushed
the ETR from the previous operation. This warrants further
investigation - should we be extending the timeout - which is already
at a rather generous 100uS, and do we also need to check the MemErr
bit in the status register?
As James says, we need details of when and how the problem occurs -
as far as I know it has not been seen on any systems we currently use
(though could have been missed given the current code)
Regards
Mike
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] coresight: tmc-etr: Don't enable ETR when it's not ready
2023-01-10 17:48 ` Mike Leach
@ 2023-01-10 18:04 ` Suzuki K Poulose
2023-01-10 21:06 ` Yabin Cui
0 siblings, 1 reply; 18+ messages in thread
From: Suzuki K Poulose @ 2023-01-10 18:04 UTC (permalink / raw)
To: Mike Leach, James Clark
Cc: Yabin Cui, coresight, linux-arm-kernel, linux-kernel,
Mathieu Poirier, Leo Yan
On 10/01/2023 17:48, Mike Leach wrote:
> Hi,
>
> On Tue, 10 Jan 2023 at 09:30, James Clark <james.clark@arm.com> wrote:
>>
>>
>>
>> On 09/01/2023 23:43, Yabin Cui wrote:
>>> Otherwise, it may cause error in AXI bus and result in a kernel panic.
>>
>> Hi Yabin,
>>
>> Thanks for the fix. Do you have a reproducer for this or some more info?
>> For example is it a regression or has it always been there? And on which
>> platform.
>>
>> Thanks
>> James
>>
>>>
>>> Signed-off-by: Yabin Cui <yabinc@google.com>
>>> ---
>>> .../hwtracing/coresight/coresight-tmc-core.c | 4 +++-
>>> .../hwtracing/coresight/coresight-tmc-etr.c | 18 +++++++++++++++---
>>> drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
>>> 3 files changed, 19 insertions(+), 5 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-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>> index 867ad8bb9b0c..2da99dd41ed6 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, "not ready ETR isn't enabled\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);
>
> There is no point in waiting for a timeout, and then carrying on even
> when it is exceeded. As such this patch seems reasonable.
> We should also apply the same principle to the ETF and ETB devices
> which use the same tmc_wait_for_tmcready() function.
+1
I am fine with pushing this change, as it is doing the right thing.
>
> However - the concern is that this appears to be happening on starting
> the ETR - there should be no outstanding AXI operations that cause the
> system to not be ready - as we will either be using this the first
> time after reset, or we should have successfully stopped and flushed
> the ETR from the previous operation. This warrants further
> investigation - should we be extending the timeout - which is already
> at a rather generous 100uS, and do we also need to check the MemErr
> bit in the status register?
It would be good to dump the value of TMC_STATUS to see what is going
on.
>
> As James says, we need details of when and how the problem occurs -
> as far as I know it has not been seen on any systems we currently use
> (though could have been missed given the current code)
+1
Kind regards
Suzuki
>
> Regards
>
> Mike
>
>
> --
> Mike Leach
> Principal Engineer, ARM Ltd.
> Manchester Design Centre. UK
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] coresight: tmc-etr: Don't enable ETR when it's not ready
2023-01-10 18:04 ` Suzuki K Poulose
@ 2023-01-10 21:06 ` Yabin Cui
2023-01-24 20:09 ` Yabin Cui
0 siblings, 1 reply; 18+ messages in thread
From: Yabin Cui @ 2023-01-10 21:06 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: Mike Leach, James Clark, coresight, linux-arm-kernel,
linux-kernel, Mathieu Poirier, Leo Yan
> Do you have a reproducer for this or some more info?
> For example is it a regression or has it always been there? And on which
> platform.
It happens on Pixel 6 and 7. We collect ETM data periodically from some
internal dogfood devices. The problem has happened several times on
dogfood devices. But I am still trying to reproduce it locally.
We use the scatter-gather mode of ETR, and allocate a 4M buffer. In userspace,
we use simpleperf in Android to collect system wide ETM data. What is special
is, simpleperf disables and reenables perf events every 100ms to flush ETM
data to perf aux buffer.
Pixel 6 and 7 have hardware monitoring AXI traffic. The hardware finds ETR is
trying to read from or write to a low invalid address (like 0x2E0000). The
problem always happens right after the "tmc_etr: timeout while waiting for TMC
to be Ready" message. And in almost all cases, I can find a "timeout while
waiting for completion of Manual Flush" message from the previous session.
One log history is below:
[11484.610008][ C0] coresight tmc_etr0: timeout while waiting for
completion of Manual Flush
[11484.610177][ C0] coresight tmc_etr0: timeout while waiting for
TMC to be Ready
[11484.615367][ C0] coresight tmc_etr0: timeout while waiting for
completion of Manual Flush
[11484.615534][ C0] coresight tmc_etr0: timeout while waiting for
TMC to be Ready
[12089.486044][ C0] coresight tmc_etr0: timeout while waiting for
TMC to be Ready
AXI error report reading from invalid address
Another log history is below:
[76709.382650][ C5] coresight tmc_etf1: timeout while waiting for
TMC to be Ready
[76709.382852][ C7] coresight tmc_etr0: timeout while waiting for
completion of Manual Flush
[76709.382995][ C7] coresight tmc_etr0: timeout while waiting for
TMC to be Ready
[76709.384510][ C7] coresight tmc_etr0: timeout while waiting for
completion of Manual Flush
[76709.384649][ C7] coresight tmc_etr0: timeout while waiting for
TMC to be Ready
[76709.384838][ C0] coresight tmc_etr0: timeout while waiting for
TMC to be Ready
AIX error report writing to invalid address
It seems if the previous manual flush doesn't finish gracefully, ETR may not be
ready for the next enable (even after 10min as in the first log). And if we
continue to enable ETR, an invalid AXI IO may happen.
Thanks,
Yabin
On Tue, Jan 10, 2023 at 10:04 AM Suzuki K Poulose
<suzuki.poulose@arm.com> wrote:
>
> On 10/01/2023 17:48, Mike Leach wrote:
> > Hi,
> >
> > On Tue, 10 Jan 2023 at 09:30, James Clark <james.clark@arm.com> wrote:
> >>
> >>
> >>
> >> On 09/01/2023 23:43, Yabin Cui wrote:
> >>> Otherwise, it may cause error in AXI bus and result in a kernel panic.
> >>
> >> Hi Yabin,
> >>
> >> Thanks for the fix. Do you have a reproducer for this or some more info?
> >> For example is it a regression or has it always been there? And on which
> >> platform.
> >>
> >> Thanks
> >> James
> >>
> >>>
> >>> Signed-off-by: Yabin Cui <yabinc@google.com>
> >>> ---
> >>> .../hwtracing/coresight/coresight-tmc-core.c | 4 +++-
> >>> .../hwtracing/coresight/coresight-tmc-etr.c | 18 +++++++++++++++---
> >>> drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
> >>> 3 files changed, 19 insertions(+), 5 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-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> >>> index 867ad8bb9b0c..2da99dd41ed6 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, "not ready ETR isn't enabled\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);
> >
> > There is no point in waiting for a timeout, and then carrying on even
> > when it is exceeded. As such this patch seems reasonable.
> > We should also apply the same principle to the ETF and ETB devices
> > which use the same tmc_wait_for_tmcready() function.
>
> +1
>
> I am fine with pushing this change, as it is doing the right thing.
>
> >
> > However - the concern is that this appears to be happening on starting
> > the ETR - there should be no outstanding AXI operations that cause the
> > system to not be ready - as we will either be using this the first
> > time after reset, or we should have successfully stopped and flushed
> > the ETR from the previous operation. This warrants further
> > investigation - should we be extending the timeout - which is already
> > at a rather generous 100uS, and do we also need to check the MemErr
> > bit in the status register?
>
> It would be good to dump the value of TMC_STATUS to see what is going
> on.
>
> >
> > As James says, we need details of when and how the problem occurs -
> > as far as I know it has not been seen on any systems we currently use
> > (though could have been missed given the current code)
>
> +1
>
> Kind regards
> Suzuki
>
>
> >
> > Regards
> >
> > Mike
> >
> >
> > --
> > Mike Leach
> > Principal Engineer, ARM Ltd.
> > Manchester Design Centre. UK
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] coresight: tmc-etr: Don't enable ETR when it's not ready
2023-01-10 21:06 ` Yabin Cui
@ 2023-01-24 20:09 ` Yabin Cui
2023-01-25 10:21 ` James Clark
2023-01-25 10:27 ` Suzuki K Poulose
0 siblings, 2 replies; 18+ messages in thread
From: Yabin Cui @ 2023-01-24 20:09 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: Mike Leach, James Clark, coresight, linux-arm-kernel,
linux-kernel, Mathieu Poirier, Leo Yan
Ping for review. And I still can't reproduce it, even if I reduced the
timeout to 2us and tried different workloads. Any suggestions for how
to reproduce it?
Thanks,
Yabin
On Tue, Jan 10, 2023 at 1:06 PM Yabin Cui <yabinc@google.com> wrote:
>
> > Do you have a reproducer for this or some more info?
> > For example is it a regression or has it always been there? And on which
> > platform.
>
> It happens on Pixel 6 and 7. We collect ETM data periodically from some
> internal dogfood devices. The problem has happened several times on
> dogfood devices. But I am still trying to reproduce it locally.
>
> We use the scatter-gather mode of ETR, and allocate a 4M buffer. In userspace,
> we use simpleperf in Android to collect system wide ETM data. What is special
> is, simpleperf disables and reenables perf events every 100ms to flush ETM
> data to perf aux buffer.
>
> Pixel 6 and 7 have hardware monitoring AXI traffic. The hardware finds ETR is
> trying to read from or write to a low invalid address (like 0x2E0000). The
> problem always happens right after the "tmc_etr: timeout while waiting for TMC
> to be Ready" message. And in almost all cases, I can find a "timeout while
> waiting for completion of Manual Flush" message from the previous session.
>
> One log history is below:
> [11484.610008][ C0] coresight tmc_etr0: timeout while waiting for
> completion of Manual Flush
> [11484.610177][ C0] coresight tmc_etr0: timeout while waiting for
> TMC to be Ready
> [11484.615367][ C0] coresight tmc_etr0: timeout while waiting for
> completion of Manual Flush
> [11484.615534][ C0] coresight tmc_etr0: timeout while waiting for
> TMC to be Ready
> [12089.486044][ C0] coresight tmc_etr0: timeout while waiting for
> TMC to be Ready
> AXI error report reading from invalid address
>
> Another log history is below:
> [76709.382650][ C5] coresight tmc_etf1: timeout while waiting for
> TMC to be Ready
> [76709.382852][ C7] coresight tmc_etr0: timeout while waiting for
> completion of Manual Flush
> [76709.382995][ C7] coresight tmc_etr0: timeout while waiting for
> TMC to be Ready
> [76709.384510][ C7] coresight tmc_etr0: timeout while waiting for
> completion of Manual Flush
> [76709.384649][ C7] coresight tmc_etr0: timeout while waiting for
> TMC to be Ready
> [76709.384838][ C0] coresight tmc_etr0: timeout while waiting for
> TMC to be Ready
> AIX error report writing to invalid address
>
> It seems if the previous manual flush doesn't finish gracefully, ETR may not be
> ready for the next enable (even after 10min as in the first log). And if we
> continue to enable ETR, an invalid AXI IO may happen.
>
> Thanks,
> Yabin
>
> On Tue, Jan 10, 2023 at 10:04 AM Suzuki K Poulose
> <suzuki.poulose@arm.com> wrote:
> >
> > On 10/01/2023 17:48, Mike Leach wrote:
> > > Hi,
> > >
> > > On Tue, 10 Jan 2023 at 09:30, James Clark <james.clark@arm.com> wrote:
> > >>
> > >>
> > >>
> > >> On 09/01/2023 23:43, Yabin Cui wrote:
> > >>> Otherwise, it may cause error in AXI bus and result in a kernel panic.
> > >>
> > >> Hi Yabin,
> > >>
> > >> Thanks for the fix. Do you have a reproducer for this or some more info?
> > >> For example is it a regression or has it always been there? And on which
> > >> platform.
> > >>
> > >> Thanks
> > >> James
> > >>
> > >>>
> > >>> Signed-off-by: Yabin Cui <yabinc@google.com>
> > >>> ---
> > >>> .../hwtracing/coresight/coresight-tmc-core.c | 4 +++-
> > >>> .../hwtracing/coresight/coresight-tmc-etr.c | 18 +++++++++++++++---
> > >>> drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
> > >>> 3 files changed, 19 insertions(+), 5 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-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> > >>> index 867ad8bb9b0c..2da99dd41ed6 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, "not ready ETR isn't enabled\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);
> > >
> > > There is no point in waiting for a timeout, and then carrying on even
> > > when it is exceeded. As such this patch seems reasonable.
> > > We should also apply the same principle to the ETF and ETB devices
> > > which use the same tmc_wait_for_tmcready() function.
> >
> > +1
> >
> > I am fine with pushing this change, as it is doing the right thing.
> >
> > >
> > > However - the concern is that this appears to be happening on starting
> > > the ETR - there should be no outstanding AXI operations that cause the
> > > system to not be ready - as we will either be using this the first
> > > time after reset, or we should have successfully stopped and flushed
> > > the ETR from the previous operation. This warrants further
> > > investigation - should we be extending the timeout - which is already
> > > at a rather generous 100uS, and do we also need to check the MemErr
> > > bit in the status register?
> >
> > It would be good to dump the value of TMC_STATUS to see what is going
> > on.
> >
> > >
> > > As James says, we need details of when and how the problem occurs -
> > > as far as I know it has not been seen on any systems we currently use
> > > (though could have been missed given the current code)
> >
> > +1
> >
> > Kind regards
> > Suzuki
> >
> >
> > >
> > > Regards
> > >
> > > Mike
> > >
> > >
> > > --
> > > Mike Leach
> > > Principal Engineer, ARM Ltd.
> > > Manchester Design Centre. UK
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] coresight: tmc-etr: Don't enable ETR when it's not ready
2023-01-24 20:09 ` Yabin Cui
@ 2023-01-25 10:21 ` James Clark
2023-01-25 10:27 ` Suzuki K Poulose
1 sibling, 0 replies; 18+ messages in thread
From: James Clark @ 2023-01-25 10:21 UTC (permalink / raw)
To: Yabin Cui, Suzuki K Poulose
Cc: Mike Leach, coresight, linux-arm-kernel, linux-kernel,
Mathieu Poirier, Leo Yan
On 24/01/2023 20:09, Yabin Cui wrote:
> Ping for review. And I still can't reproduce it, even if I reduced the
> timeout to 2us and tried different workloads. Any suggestions for how
> to reproduce it?
>
Hi Yabin,
I just found this in my kernel log files from a few days ago.
Jan 19 15:03:35 n1-sdp kernel: [865337.521413] coresight tmc_etr0:
timeout while waiting for completion of Manual Flush
Jan 19 15:03:35 n1-sdp kernel: [865337.529351] coresight tmc_etr0:
timeout while waiting for TMC to be Ready
This is on N1SDP, and I've only been running "normal" perf commands like:
perf record -e cs_etm//k -- taskset -c 2 sleep 1
perf record -e cs_etm// -- stress -c 2 -t 1
I also haven't been able to reproduce it since
James
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] coresight: tmc-etr: Don't enable ETR when it's not ready
2023-01-24 20:09 ` Yabin Cui
2023-01-25 10:21 ` James Clark
@ 2023-01-25 10:27 ` Suzuki K Poulose
2023-01-27 23:10 ` [PATCH v2] coresight: tmc: Don't enable TMC " Yabin Cui
1 sibling, 1 reply; 18+ messages in thread
From: Suzuki K Poulose @ 2023-01-25 10:27 UTC (permalink / raw)
To: Yabin Cui
Cc: Mike Leach, James Clark, coresight, linux-arm-kernel,
linux-kernel, Mathieu Poirier, Leo Yan
Folks,
On 24/01/2023 20:09, Yabin Cui wrote:
> Ping for review. And I still can't reproduce it, even if I reduced the
> timeout to 2us and tried different workloads. Any suggestions for how
> to reproduce it?
>
I think we should go ahead and fix this in the driver to handle flaky
hardware cases. But I would like this to be addressed for all the TMC
types, not just ETRs, as Mike pointed out.
Thanks
Suzuki
> Thanks,
> Yabin
>
> On Tue, Jan 10, 2023 at 1:06 PM Yabin Cui <yabinc@google.com> wrote:
>>
>>> Do you have a reproducer for this or some more info?
>>> For example is it a regression or has it always been there? And on which
>>> platform.
>>
>> It happens on Pixel 6 and 7. We collect ETM data periodically from some
>> internal dogfood devices. The problem has happened several times on
>> dogfood devices. But I am still trying to reproduce it locally.
>>
>> We use the scatter-gather mode of ETR, and allocate a 4M buffer. In userspace,
>> we use simpleperf in Android to collect system wide ETM data. What is special
>> is, simpleperf disables and reenables perf events every 100ms to flush ETM
>> data to perf aux buffer.
>>
>> Pixel 6 and 7 have hardware monitoring AXI traffic. The hardware finds ETR is
>> trying to read from or write to a low invalid address (like 0x2E0000). The
>> problem always happens right after the "tmc_etr: timeout while waiting for TMC
>> to be Ready" message. And in almost all cases, I can find a "timeout while
>> waiting for completion of Manual Flush" message from the previous session.
>>
>> One log history is below:
>> [11484.610008][ C0] coresight tmc_etr0: timeout while waiting for
>> completion of Manual Flush
>> [11484.610177][ C0] coresight tmc_etr0: timeout while waiting for
>> TMC to be Ready
>> [11484.615367][ C0] coresight tmc_etr0: timeout while waiting for
>> completion of Manual Flush
>> [11484.615534][ C0] coresight tmc_etr0: timeout while waiting for
>> TMC to be Ready
>> [12089.486044][ C0] coresight tmc_etr0: timeout while waiting for
>> TMC to be Ready
>> AXI error report reading from invalid address
>>
>> Another log history is below:
>> [76709.382650][ C5] coresight tmc_etf1: timeout while waiting for
>> TMC to be Ready
>> [76709.382852][ C7] coresight tmc_etr0: timeout while waiting for
>> completion of Manual Flush
>> [76709.382995][ C7] coresight tmc_etr0: timeout while waiting for
>> TMC to be Ready
>> [76709.384510][ C7] coresight tmc_etr0: timeout while waiting for
>> completion of Manual Flush
>> [76709.384649][ C7] coresight tmc_etr0: timeout while waiting for
>> TMC to be Ready
>> [76709.384838][ C0] coresight tmc_etr0: timeout while waiting for
>> TMC to be Ready
>> AIX error report writing to invalid address
>>
>> It seems if the previous manual flush doesn't finish gracefully, ETR may not be
>> ready for the next enable (even after 10min as in the first log). And if we
>> continue to enable ETR, an invalid AXI IO may happen.
>>
>> Thanks,
>> Yabin
>>
>> On Tue, Jan 10, 2023 at 10:04 AM Suzuki K Poulose
>> <suzuki.poulose@arm.com> wrote:
>>>
>>> On 10/01/2023 17:48, Mike Leach wrote:
>>>> Hi,
>>>>
>>>> On Tue, 10 Jan 2023 at 09:30, James Clark <james.clark@arm.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 09/01/2023 23:43, Yabin Cui wrote:
>>>>>> Otherwise, it may cause error in AXI bus and result in a kernel panic.
>>>>>
>>>>> Hi Yabin,
>>>>>
>>>>> Thanks for the fix. Do you have a reproducer for this or some more info?
>>>>> For example is it a regression or has it always been there? And on which
>>>>> platform.
>>>>>
>>>>> Thanks
>>>>> James
>>>>>
>>>>>>
>>>>>> Signed-off-by: Yabin Cui <yabinc@google.com>
>>>>>> ---
>>>>>> .../hwtracing/coresight/coresight-tmc-core.c | 4 +++-
>>>>>> .../hwtracing/coresight/coresight-tmc-etr.c | 18 +++++++++++++++---
>>>>>> drivers/hwtracing/coresight/coresight-tmc.h | 2 +-
>>>>>> 3 files changed, 19 insertions(+), 5 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-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>>>>>> index 867ad8bb9b0c..2da99dd41ed6 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, "not ready ETR isn't enabled\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);
>>>>
>>>> There is no point in waiting for a timeout, and then carrying on even
>>>> when it is exceeded. As such this patch seems reasonable.
>>>> We should also apply the same principle to the ETF and ETB devices
>>>> which use the same tmc_wait_for_tmcready() function.
>>>
>>> +1
>>>
>>> I am fine with pushing this change, as it is doing the right thing.
>>>
>>>>
>>>> However - the concern is that this appears to be happening on starting
>>>> the ETR - there should be no outstanding AXI operations that cause the
>>>> system to not be ready - as we will either be using this the first
>>>> time after reset, or we should have successfully stopped and flushed
>>>> the ETR from the previous operation. This warrants further
>>>> investigation - should we be extending the timeout - which is already
>>>> at a rather generous 100uS, and do we also need to check the MemErr
>>>> bit in the status register?
>>>
>>> It would be good to dump the value of TMC_STATUS to see what is going
>>> on.
>>>
>>>>
>>>> As James says, we need details of when and how the problem occurs -
>>>> as far as I know it has not been seen on any systems we currently use
>>>> (though could have been missed given the current code)
>>>
>>> +1
>>>
>>> Kind regards
>>> Suzuki
>>>
>>>
>>>>
>>>> Regards
>>>>
>>>> Mike
>>>>
>>>>
>>>> --
>>>> Mike Leach
>>>> Principal Engineer, ARM Ltd.
>>>> Manchester Design Centre. UK
>>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2] coresight: tmc: Don't enable TMC when it's not ready.
2023-01-25 10:27 ` Suzuki K Poulose
@ 2023-01-27 23:10 ` Yabin Cui
2023-02-01 20:43 ` Mike Leach
0 siblings, 1 reply; 18+ messages in thread
From: Yabin Cui @ 2023-01-27 23:10 UTC (permalink / raw)
To: Mathieu Poirier, Suzuki K Poulose, Mike Leach, Leo Yan, James Clark
Cc: coresight, linux-arm-kernel, linux-kernel, Yabin Cui
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-goog
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2] coresight: tmc: Don't enable TMC when it's not ready.
2023-01-27 23:10 ` [PATCH v2] coresight: tmc: Don't enable TMC " Yabin Cui
@ 2023-02-01 20:43 ` Mike Leach
2023-02-02 11:24 ` Suzuki K Poulose
0 siblings, 1 reply; 18+ messages in thread
From: Mike Leach @ 2023-02-01 20:43 UTC (permalink / raw)
To: Yabin Cui
Cc: Mathieu Poirier, Suzuki K Poulose, Leo Yan, James Clark,
coresight, linux-arm-kernel, linux-kernel
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().
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] coresight: tmc: Don't enable TMC when it's not ready.
2023-02-01 20:43 ` Mike Leach
@ 2023-02-02 11:24 ` Suzuki K Poulose
2023-02-02 20:54 ` Yabin Cui
0 siblings, 1 reply; 18+ messages in thread
From: Suzuki K Poulose @ 2023-02-02 11:24 UTC (permalink / raw)
To: Mike Leach, Yabin Cui
Cc: Mathieu Poirier, Leo Yan, James Clark, coresight,
linux-arm-kernel, linux-kernel
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
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] coresight: tmc: Don't enable TMC when it's not ready.
2023-02-02 11:24 ` Suzuki K Poulose
@ 2023-02-02 20:54 ` Yabin Cui
2023-02-02 21:46 ` [PATCH v3] " Yabin Cui
0 siblings, 1 reply; 18+ messages in thread
From: Yabin Cui @ 2023-02-02 20:54 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: Mike Leach, Mathieu Poirier, Leo Yan, James Clark, coresight,
linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3] coresight: tmc: Don't enable TMC when it's not ready.
2023-02-02 20:54 ` Yabin Cui
@ 2023-02-02 21:46 ` Yabin Cui
2023-02-09 1:08 ` Yabin Cui
0 siblings, 1 reply; 18+ messages in thread
From: Yabin Cui @ 2023-02-02 21:46 UTC (permalink / raw)
To: Mathieu Poirier, Suzuki K Poulose, Mike Leach, Leo Yan, James Clark
Cc: coresight, linux-arm-kernel, linux-kernel, Yabin Cui
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3] coresight: tmc: Don't enable TMC when it's not ready.
2023-02-02 21:46 ` [PATCH v3] " Yabin Cui
@ 2023-02-09 1:08 ` Yabin Cui
2023-02-09 10:31 ` Suzuki K Poulose
0 siblings, 1 reply; 18+ messages in thread
From: Yabin Cui @ 2023-02-09 1:08 UTC (permalink / raw)
To: Mathieu Poirier, Suzuki K Poulose, Mike Leach, Leo Yan, James Clark
Cc: coresight, linux-arm-kernel, linux-kernel
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
>
> .../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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3] coresight: tmc: Don't enable TMC when it's not ready.
2023-02-09 1:08 ` Yabin Cui
@ 2023-02-09 10:31 ` Suzuki K Poulose
2023-02-10 23:43 ` [PATCH] coresight: tmc-etr: Handle enable failure in tmc_read_unprepare_etr Yabin Cui
0 siblings, 1 reply; 18+ messages in thread
From: Suzuki K Poulose @ 2023-02-09 10:31 UTC (permalink / raw)
To: Yabin Cui, Mathieu Poirier, Mike Leach, Leo Yan, James Clark
Cc: coresight, linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] coresight: tmc-etr: Handle enable failure in tmc_read_unprepare_etr
2023-02-09 10:31 ` Suzuki K Poulose
@ 2023-02-10 23:43 ` Yabin Cui
2023-02-21 18:38 ` Yabin Cui
0 siblings, 1 reply; 18+ messages in thread
From: Yabin Cui @ 2023-02-10 23:43 UTC (permalink / raw)
To: Mathieu Poirier, Suzuki K Poulose, Mike Leach, Leo Yan, James Clark
Cc: coresight, linux-arm-kernel, linux-kernel, Yabin Cui
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] coresight: tmc-etr: Handle enable failure in tmc_read_unprepare_etr
2023-02-10 23:43 ` [PATCH] coresight: tmc-etr: Handle enable failure in tmc_read_unprepare_etr Yabin Cui
@ 2023-02-21 18:38 ` Yabin Cui
2023-02-22 9:33 ` Suzuki K Poulose
0 siblings, 1 reply; 18+ messages in thread
From: Yabin Cui @ 2023-02-21 18:38 UTC (permalink / raw)
To: Mathieu Poirier, Suzuki K Poulose, Mike Leach, Leo Yan, James Clark
Cc: coresight, linux-arm-kernel, linux-kernel
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>
> ---
> 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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] coresight: tmc-etr: Handle enable failure in tmc_read_unprepare_etr
2023-02-21 18:38 ` Yabin Cui
@ 2023-02-22 9:33 ` Suzuki K Poulose
0 siblings, 0 replies; 18+ messages in thread
From: Suzuki K Poulose @ 2023-02-22 9:33 UTC (permalink / raw)
To: Yabin Cui, Mathieu Poirier, Mike Leach, Leo Yan, James Clark
Cc: coresight, linux-arm-kernel, linux-kernel
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
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-02-22 9:37 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-09 23:43 [PATCH] coresight: tmc-etr: Don't enable ETR when it's not ready Yabin Cui
2023-01-10 9:30 ` James Clark
2023-01-10 17:48 ` Mike Leach
2023-01-10 18:04 ` Suzuki K Poulose
2023-01-10 21:06 ` Yabin Cui
2023-01-24 20:09 ` Yabin Cui
2023-01-25 10:21 ` James Clark
2023-01-25 10:27 ` Suzuki K Poulose
2023-01-27 23:10 ` [PATCH v2] coresight: tmc: Don't enable TMC " Yabin Cui
2023-02-01 20:43 ` Mike Leach
2023-02-02 11:24 ` Suzuki K Poulose
2023-02-02 20:54 ` Yabin Cui
2023-02-02 21:46 ` [PATCH v3] " Yabin Cui
2023-02-09 1:08 ` Yabin Cui
2023-02-09 10:31 ` Suzuki K Poulose
2023-02-10 23:43 ` [PATCH] coresight: tmc-etr: Handle enable failure in tmc_read_unprepare_etr Yabin Cui
2023-02-21 18:38 ` Yabin Cui
2023-02-22 9:33 ` Suzuki K Poulose
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).