When tracing etm data of multiple threads on multiple cpus through perf
interface, some link devices are shared between paths of different cpus.
It creates race conditions when different cpus wants to enable/disable
the same link device at the same time.
Example 1:
Two cpus want to enable different ports of a coresight funnel, thus
calling the funnel enable operation at the same time. But the funnel
enable operation isn't reentrantable.
Example 2:
For an enabled coresight dynamic replicator with refcnt=1, one cpu wants
to disable it, while another cpu wants to enable it. Ideally we still have
an enabled replicator with refcnt=1 at the end. But in reality the result
is uncertain.
Since coresight devices claim themselves when enabled for self-hosted
usage, the race conditions above usually make the link devices not usable
after many cycles.
To fix the race conditions, this patch uses spinlocks to serialize
enabling/disabling link devices.
Fixes: a06ae8609b3d ("coresight: add CoreSight core layer framework")
Signed-off-by: Yabin Cui <[email protected]>
---
v3 -> v4: moved lock from coresight_enable/disable_link() to
enable/disable functions of each link device.
I also removed lock protection of csdev->enable in v3. Because that
needs to move csdev->enable inside the enable/disable functions of
each link device. That's much effort with almost no benefits.
csdev->enable seems only used for source devices in sysfs interface.
---
.../hwtracing/coresight/coresight-funnel.c | 29 ++++++++----
.../coresight/coresight-replicator.c | 31 +++++++++----
.../hwtracing/coresight/coresight-tmc-etf.c | 39 ++++++++--------
drivers/hwtracing/coresight/coresight.c | 45 ++++++-------------
4 files changed, 77 insertions(+), 67 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
index 05f7896c3a01..8326d03a0d03 100644
--- a/drivers/hwtracing/coresight/coresight-funnel.c
+++ b/drivers/hwtracing/coresight/coresight-funnel.c
@@ -44,6 +44,7 @@ struct funnel_drvdata {
struct clk *atclk;
struct coresight_device *csdev;
unsigned long priority;
+ spinlock_t spinlock;
};
static int dynamic_funnel_enable_hw(struct funnel_drvdata *drvdata, int port)
@@ -76,12 +77,20 @@ static int funnel_enable(struct coresight_device *csdev, int inport,
{
int rc = 0;
struct funnel_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+ unsigned long flags;
- if (drvdata->base)
- rc = dynamic_funnel_enable_hw(drvdata, inport);
+ spin_lock_irqsave(&drvdata->spinlock, flags);
+ if (atomic_inc_return(&csdev->refcnt[inport]) == 1) {
+ if (drvdata->base)
+ rc = dynamic_funnel_enable_hw(drvdata, inport);
- if (!rc)
- dev_dbg(&csdev->dev, "FUNNEL inport %d enabled\n", inport);
+ if (rc)
+ atomic_dec(&csdev->refcnt[inport]);
+ else
+ dev_dbg(&csdev->dev, "FUNNEL inport %d enabled\n",
+ inport);
+ }
+ spin_unlock_irqrestore(&drvdata->spinlock, flags);
return rc;
}
@@ -107,11 +116,15 @@ static void funnel_disable(struct coresight_device *csdev, int inport,
int outport)
{
struct funnel_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+ unsigned long flags;
- if (drvdata->base)
- dynamic_funnel_disable_hw(drvdata, inport);
-
- dev_dbg(&csdev->dev, "FUNNEL inport %d disabled\n", inport);
+ spin_lock_irqsave(&drvdata->spinlock, flags);
+ if (atomic_dec_return(&csdev->refcnt[inport]) == 0) {
+ if (drvdata->base)
+ dynamic_funnel_disable_hw(drvdata, inport);
+ dev_dbg(&csdev->dev, "FUNNEL inport %d disabled\n", inport);
+ }
+ spin_unlock_irqrestore(&drvdata->spinlock, flags);
}
static const struct coresight_ops_link funnel_link_ops = {
diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
index b29ba640eb25..427d8b8d0917 100644
--- a/drivers/hwtracing/coresight/coresight-replicator.c
+++ b/drivers/hwtracing/coresight/coresight-replicator.c
@@ -36,6 +36,7 @@ struct replicator_drvdata {
void __iomem *base;
struct clk *atclk;
struct coresight_device *csdev;
+ spinlock_t spinlock;
};
static void dynamic_replicator_reset(struct replicator_drvdata *drvdata)
@@ -97,11 +98,20 @@ static int replicator_enable(struct coresight_device *csdev, int inport,
{
int rc = 0;
struct replicator_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
-
- if (drvdata->base)
- rc = dynamic_replicator_enable(drvdata, inport, outport);
- if (!rc)
- dev_dbg(&csdev->dev, "REPLICATOR enabled\n");
+ unsigned long flags;
+
+ spin_lock_irqsave(&drvdata->spinlock, flags);
+ if (atomic_inc_return(&csdev->refcnt[outport]) == 1) {
+ if (drvdata->base)
+ rc = dynamic_replicator_enable(drvdata, inport,
+ outport);
+
+ if (rc)
+ atomic_dec(&csdev->refcnt[outport]);
+ else
+ dev_dbg(&csdev->dev, "REPLICATOR enabled\n");
+ }
+ spin_unlock_irqrestore(&drvdata->spinlock, flags);
return rc;
}
@@ -137,10 +147,15 @@ static void replicator_disable(struct coresight_device *csdev, int inport,
int outport)
{
struct replicator_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+ unsigned long flags;
- if (drvdata->base)
- dynamic_replicator_disable(drvdata, inport, outport);
- dev_dbg(&csdev->dev, "REPLICATOR disabled\n");
+ spin_lock_irqsave(&drvdata->spinlock, flags);
+ if (atomic_dec_return(&csdev->refcnt[outport]) == 0) {
+ if (drvdata->base)
+ dynamic_replicator_disable(drvdata, inport, outport);
+ dev_dbg(&csdev->dev, "REPLICATOR disabled\n");
+ }
+ spin_unlock_irqrestore(&drvdata->spinlock, flags);
}
static const struct coresight_ops_link replicator_link_ops = {
diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
index 807416b75ecc..cb4a38541bf8 100644
--- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
+++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
@@ -334,23 +334,25 @@ static int tmc_disable_etf_sink(struct coresight_device *csdev)
static int tmc_enable_etf_link(struct coresight_device *csdev,
int inport, int outport)
{
- int ret;
+ int ret = 0;
unsigned long flags;
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
spin_lock_irqsave(&drvdata->spinlock, flags);
- if (drvdata->reading) {
- spin_unlock_irqrestore(&drvdata->spinlock, flags);
- return -EBUSY;
+ if (atomic_inc_return(&csdev->refcnt[0]) == 1) {
+ if (drvdata->reading)
+ ret = -EBUSY;
+ else
+ ret = tmc_etf_enable_hw(drvdata);
+
+ if (ret) {
+ atomic_dec(&csdev->refcnt[0]);
+ } else {
+ drvdata->mode = CS_MODE_SYSFS;
+ dev_dbg(&csdev->dev, "TMC-ETF enabled\n");
+ }
}
-
- ret = tmc_etf_enable_hw(drvdata);
- if (!ret)
- drvdata->mode = CS_MODE_SYSFS;
spin_unlock_irqrestore(&drvdata->spinlock, flags);
-
- if (!ret)
- dev_dbg(&csdev->dev, "TMC-ETF enabled\n");
return ret;
}
@@ -361,16 +363,13 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
spin_lock_irqsave(&drvdata->spinlock, flags);
- if (drvdata->reading) {
- spin_unlock_irqrestore(&drvdata->spinlock, flags);
- return;
- }
-
- tmc_etf_disable_hw(drvdata);
- drvdata->mode = CS_MODE_DISABLED;
+ if (atomic_dec_return(&csdev->refcnt[0]) == 0)
+ if (!drvdata->reading) {
+ tmc_etf_disable_hw(drvdata);
+ drvdata->mode = CS_MODE_DISABLED;
+ dev_dbg(&csdev->dev, "TMC-ETF disabled\n");
+ }
spin_unlock_irqrestore(&drvdata->spinlock, flags);
-
- dev_dbg(&csdev->dev, "TMC-ETF disabled\n");
}
static void *tmc_alloc_etf_buffer(struct coresight_device *csdev,
diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 6453c67a4d01..0bbce0d29158 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -253,9 +253,9 @@ static int coresight_enable_link(struct coresight_device *csdev,
struct coresight_device *parent,
struct coresight_device *child)
{
- int ret;
+ int ret = 0;
int link_subtype;
- int refport, inport, outport;
+ int inport, outport;
if (!parent || !child)
return -EINVAL;
@@ -264,29 +264,17 @@ static int coresight_enable_link(struct coresight_device *csdev,
outport = coresight_find_link_outport(csdev, child);
link_subtype = csdev->subtype.link_subtype;
- if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG)
- refport = inport;
- else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT)
- refport = outport;
- else
- refport = 0;
-
- if (refport < 0)
- return refport;
+ if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG && inport < 0)
+ return inport;
+ if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT && outport < 0)
+ return outport;
- if (atomic_inc_return(&csdev->refcnt[refport]) == 1) {
- if (link_ops(csdev)->enable) {
- ret = link_ops(csdev)->enable(csdev, inport, outport);
- if (ret) {
- atomic_dec(&csdev->refcnt[refport]);
- return ret;
- }
- }
- }
-
- csdev->enable = true;
+ if (link_ops(csdev)->enable)
+ ret = link_ops(csdev)->enable(csdev, inport, outport);
+ if (!ret)
+ csdev->enable = true;
- return 0;
+ return ret;
}
static void coresight_disable_link(struct coresight_device *csdev,
@@ -295,7 +283,7 @@ static void coresight_disable_link(struct coresight_device *csdev,
{
int i, nr_conns;
int link_subtype;
- int refport, inport, outport;
+ int inport, outport;
if (!parent || !child)
return;
@@ -305,20 +293,15 @@ static void coresight_disable_link(struct coresight_device *csdev,
link_subtype = csdev->subtype.link_subtype;
if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG) {
- refport = inport;
nr_conns = csdev->pdata->nr_inport;
} else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT) {
- refport = outport;
nr_conns = csdev->pdata->nr_outport;
} else {
- refport = 0;
nr_conns = 1;
}
- if (atomic_dec_return(&csdev->refcnt[refport]) == 0) {
- if (link_ops(csdev)->disable)
- link_ops(csdev)->disable(csdev, inport, outport);
- }
+ if (link_ops(csdev)->disable)
+ link_ops(csdev)->disable(csdev, inport, outport);
for (i = 0; i < nr_conns; i++)
if (atomic_read(&csdev->refcnt[i]) != 0)
--
2.23.0.866.gb869b98d4c-goog
Good morning Yabin,
On Fri, Oct 18, 2019 at 11:14:03AM -0700, Yabin Cui wrote:
> When tracing etm data of multiple threads on multiple cpus through perf
> interface, some link devices are shared between paths of different cpus.
> It creates race conditions when different cpus wants to enable/disable
> the same link device at the same time.
>
> Example 1:
> Two cpus want to enable different ports of a coresight funnel, thus
> calling the funnel enable operation at the same time. But the funnel
> enable operation isn't reentrantable.
>
> Example 2:
> For an enabled coresight dynamic replicator with refcnt=1, one cpu wants
> to disable it, while another cpu wants to enable it. Ideally we still have
> an enabled replicator with refcnt=1 at the end. But in reality the result
> is uncertain.
>
> Since coresight devices claim themselves when enabled for self-hosted
> usage, the race conditions above usually make the link devices not usable
> after many cycles.
>
> To fix the race conditions, this patch uses spinlocks to serialize
> enabling/disabling link devices.
>
> Fixes: a06ae8609b3d ("coresight: add CoreSight core layer framework")
> Signed-off-by: Yabin Cui <[email protected]>
> ---
>
> v3 -> v4: moved lock from coresight_enable/disable_link() to
> enable/disable functions of each link device.
>
> I also removed lock protection of csdev->enable in v3. Because that
> needs to move csdev->enable inside the enable/disable functions of
> each link device. That's much effort with almost no benefits.
> csdev->enable seems only used for source devices in sysfs interface.
>
> ---
> .../hwtracing/coresight/coresight-funnel.c | 29 ++++++++----
> .../coresight/coresight-replicator.c | 31 +++++++++----
> .../hwtracing/coresight/coresight-tmc-etf.c | 39 ++++++++--------
> drivers/hwtracing/coresight/coresight.c | 45 ++++++-------------
> 4 files changed, 77 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
> index 05f7896c3a01..8326d03a0d03 100644
> --- a/drivers/hwtracing/coresight/coresight-funnel.c
> +++ b/drivers/hwtracing/coresight/coresight-funnel.c
> @@ -44,6 +44,7 @@ struct funnel_drvdata {
> struct clk *atclk;
> struct coresight_device *csdev;
> unsigned long priority;
> + spinlock_t spinlock;
Please document the new entry as it is for other fields of the structure.
> };
>
> static int dynamic_funnel_enable_hw(struct funnel_drvdata *drvdata, int port)
> @@ -76,12 +77,20 @@ static int funnel_enable(struct coresight_device *csdev, int inport,
> {
> int rc = 0;
> struct funnel_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> + unsigned long flags;
>
> - if (drvdata->base)
> - rc = dynamic_funnel_enable_hw(drvdata, inport);
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_inc_return(&csdev->refcnt[inport]) == 1) {
> + if (drvdata->base)
> + rc = dynamic_funnel_enable_hw(drvdata, inport);
>
> - if (!rc)
> - dev_dbg(&csdev->dev, "FUNNEL inport %d enabled\n", inport);
> + if (rc)
> + atomic_dec(&csdev->refcnt[inport]);
> + else
> + dev_dbg(&csdev->dev, "FUNNEL inport %d enabled\n",
> + inport);
Calling dev_dbg() within an atomic context is guaranteed to make the lockdep
mechanic very angry. Move it outside of the spinlock scope and control the
output of the message with a variable.
The same comment applies for the following hunks.
Thanks,
Mathieu
> + }
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> return rc;
> }
>
> @@ -107,11 +116,15 @@ static void funnel_disable(struct coresight_device *csdev, int inport,
> int outport)
> {
> struct funnel_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> + unsigned long flags;
>
> - if (drvdata->base)
> - dynamic_funnel_disable_hw(drvdata, inport);
> -
> - dev_dbg(&csdev->dev, "FUNNEL inport %d disabled\n", inport);
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_dec_return(&csdev->refcnt[inport]) == 0) {
> + if (drvdata->base)
> + dynamic_funnel_disable_hw(drvdata, inport);
> + dev_dbg(&csdev->dev, "FUNNEL inport %d disabled\n", inport);
> + }
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> }
>
> static const struct coresight_ops_link funnel_link_ops = {
> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
> index b29ba640eb25..427d8b8d0917 100644
> --- a/drivers/hwtracing/coresight/coresight-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> @@ -36,6 +36,7 @@ struct replicator_drvdata {
> void __iomem *base;
> struct clk *atclk;
> struct coresight_device *csdev;
> + spinlock_t spinlock;
> };
>
> static void dynamic_replicator_reset(struct replicator_drvdata *drvdata)
> @@ -97,11 +98,20 @@ static int replicator_enable(struct coresight_device *csdev, int inport,
> {
> int rc = 0;
> struct replicator_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> -
> - if (drvdata->base)
> - rc = dynamic_replicator_enable(drvdata, inport, outport);
> - if (!rc)
> - dev_dbg(&csdev->dev, "REPLICATOR enabled\n");
> + unsigned long flags;
> +
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_inc_return(&csdev->refcnt[outport]) == 1) {
> + if (drvdata->base)
> + rc = dynamic_replicator_enable(drvdata, inport,
> + outport);
> +
> + if (rc)
> + atomic_dec(&csdev->refcnt[outport]);
> + else
> + dev_dbg(&csdev->dev, "REPLICATOR enabled\n");
> + }
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> return rc;
> }
>
> @@ -137,10 +147,15 @@ static void replicator_disable(struct coresight_device *csdev, int inport,
> int outport)
> {
> struct replicator_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> + unsigned long flags;
>
> - if (drvdata->base)
> - dynamic_replicator_disable(drvdata, inport, outport);
> - dev_dbg(&csdev->dev, "REPLICATOR disabled\n");
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_dec_return(&csdev->refcnt[outport]) == 0) {
> + if (drvdata->base)
> + dynamic_replicator_disable(drvdata, inport, outport);
> + dev_dbg(&csdev->dev, "REPLICATOR disabled\n");
> + }
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> }
>
> static const struct coresight_ops_link replicator_link_ops = {
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index 807416b75ecc..cb4a38541bf8 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -334,23 +334,25 @@ static int tmc_disable_etf_sink(struct coresight_device *csdev)
> static int tmc_enable_etf_link(struct coresight_device *csdev,
> int inport, int outport)
> {
> - int ret;
> + int ret = 0;
> unsigned long flags;
> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> spin_lock_irqsave(&drvdata->spinlock, flags);
> - if (drvdata->reading) {
> - spin_unlock_irqrestore(&drvdata->spinlock, flags);
> - return -EBUSY;
> + if (atomic_inc_return(&csdev->refcnt[0]) == 1) {
> + if (drvdata->reading)
> + ret = -EBUSY;
> + else
> + ret = tmc_etf_enable_hw(drvdata);
> +
> + if (ret) {
> + atomic_dec(&csdev->refcnt[0]);
> + } else {
> + drvdata->mode = CS_MODE_SYSFS;
> + dev_dbg(&csdev->dev, "TMC-ETF enabled\n");
> + }
> }
> -
> - ret = tmc_etf_enable_hw(drvdata);
> - if (!ret)
> - drvdata->mode = CS_MODE_SYSFS;
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
> -
> - if (!ret)
> - dev_dbg(&csdev->dev, "TMC-ETF enabled\n");
> return ret;
> }
>
> @@ -361,16 +363,13 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> spin_lock_irqsave(&drvdata->spinlock, flags);
> - if (drvdata->reading) {
> - spin_unlock_irqrestore(&drvdata->spinlock, flags);
> - return;
> - }
> -
> - tmc_etf_disable_hw(drvdata);
> - drvdata->mode = CS_MODE_DISABLED;
> + if (atomic_dec_return(&csdev->refcnt[0]) == 0)
> + if (!drvdata->reading) {
> + tmc_etf_disable_hw(drvdata);
> + drvdata->mode = CS_MODE_DISABLED;
> + dev_dbg(&csdev->dev, "TMC-ETF disabled\n");
> + }
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
> -
> - dev_dbg(&csdev->dev, "TMC-ETF disabled\n");
> }
>
> static void *tmc_alloc_etf_buffer(struct coresight_device *csdev,
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 6453c67a4d01..0bbce0d29158 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -253,9 +253,9 @@ static int coresight_enable_link(struct coresight_device *csdev,
> struct coresight_device *parent,
> struct coresight_device *child)
> {
> - int ret;
> + int ret = 0;
> int link_subtype;
> - int refport, inport, outport;
> + int inport, outport;
>
> if (!parent || !child)
> return -EINVAL;
> @@ -264,29 +264,17 @@ static int coresight_enable_link(struct coresight_device *csdev,
> outport = coresight_find_link_outport(csdev, child);
> link_subtype = csdev->subtype.link_subtype;
>
> - if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG)
> - refport = inport;
> - else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT)
> - refport = outport;
> - else
> - refport = 0;
> -
> - if (refport < 0)
> - return refport;
> + if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG && inport < 0)
> + return inport;
> + if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT && outport < 0)
> + return outport;
>
> - if (atomic_inc_return(&csdev->refcnt[refport]) == 1) {
> - if (link_ops(csdev)->enable) {
> - ret = link_ops(csdev)->enable(csdev, inport, outport);
> - if (ret) {
> - atomic_dec(&csdev->refcnt[refport]);
> - return ret;
> - }
> - }
> - }
> -
> - csdev->enable = true;
> + if (link_ops(csdev)->enable)
> + ret = link_ops(csdev)->enable(csdev, inport, outport);
> + if (!ret)
> + csdev->enable = true;
>
> - return 0;
> + return ret;
> }
>
> static void coresight_disable_link(struct coresight_device *csdev,
> @@ -295,7 +283,7 @@ static void coresight_disable_link(struct coresight_device *csdev,
> {
> int i, nr_conns;
> int link_subtype;
> - int refport, inport, outport;
> + int inport, outport;
>
> if (!parent || !child)
> return;
> @@ -305,20 +293,15 @@ static void coresight_disable_link(struct coresight_device *csdev,
> link_subtype = csdev->subtype.link_subtype;
>
> if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG) {
> - refport = inport;
> nr_conns = csdev->pdata->nr_inport;
> } else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT) {
> - refport = outport;
> nr_conns = csdev->pdata->nr_outport;
> } else {
> - refport = 0;
> nr_conns = 1;
> }
>
> - if (atomic_dec_return(&csdev->refcnt[refport]) == 0) {
> - if (link_ops(csdev)->disable)
> - link_ops(csdev)->disable(csdev, inport, outport);
> - }
> + if (link_ops(csdev)->disable)
> + link_ops(csdev)->disable(csdev, inport, outport);
>
> for (i = 0; i < nr_conns; i++)
> if (atomic_read(&csdev->refcnt[i]) != 0)
> --
> 2.23.0.866.gb869b98d4c-goog
>
On 10/18/2019 07:14 PM, Yabin Cui wrote:
> When tracing etm data of multiple threads on multiple cpus through perf
> interface, some link devices are shared between paths of different cpus.
> It creates race conditions when different cpus wants to enable/disable
> the same link device at the same time.
>
> Example 1:
> Two cpus want to enable different ports of a coresight funnel, thus
> calling the funnel enable operation at the same time. But the funnel
> enable operation isn't reentrantable.
>
> Example 2:
> For an enabled coresight dynamic replicator with refcnt=1, one cpu wants
> to disable it, while another cpu wants to enable it. Ideally we still have
> an enabled replicator with refcnt=1 at the end. But in reality the result
> is uncertain.
>
> Since coresight devices claim themselves when enabled for self-hosted
> usage, the race conditions above usually make the link devices not usable
> after many cycles.
>
> To fix the race conditions, this patch uses spinlocks to serialize
> enabling/disabling link devices.
>
> Fixes: a06ae8609b3d ("coresight: add CoreSight core layer framework")
> Signed-off-by: Yabin Cui <[email protected]>
> ---
>
> v3 -> v4: moved lock from coresight_enable/disable_link() to
> enable/disable functions of each link device.
>
> I also removed lock protection of csdev->enable in v3. Because that
> needs to move csdev->enable inside the enable/disable functions of
> each link device. That's much effort with almost no benefits.
> csdev->enable seems only used for source devices in sysfs interface.
>
> ---
> .../hwtracing/coresight/coresight-funnel.c | 29 ++++++++----
> .../coresight/coresight-replicator.c | 31 +++++++++----
> .../hwtracing/coresight/coresight-tmc-etf.c | 39 ++++++++--------
> drivers/hwtracing/coresight/coresight.c | 45 ++++++-------------
> 4 files changed, 77 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
> index 05f7896c3a01..8326d03a0d03 100644
> --- a/drivers/hwtracing/coresight/coresight-funnel.c
> +++ b/drivers/hwtracing/coresight/coresight-funnel.c
> @@ -44,6 +44,7 @@ struct funnel_drvdata {
> struct clk *atclk;
> struct coresight_device *csdev;
> unsigned long priority;
> + spinlock_t spinlock;
> };
>
> static int dynamic_funnel_enable_hw(struct funnel_drvdata *drvdata, int port)
> @@ -76,12 +77,20 @@ static int funnel_enable(struct coresight_device *csdev, int inport,
> {
> int rc = 0;
> struct funnel_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> + unsigned long flags;
>
> - if (drvdata->base)
> - rc = dynamic_funnel_enable_hw(drvdata, inport);
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_inc_return(&csdev->refcnt[inport]) == 1) {
> + if (drvdata->base)
> + rc = dynamic_funnel_enable_hw(drvdata, inport);
>
> - if (!rc)
> - dev_dbg(&csdev->dev, "FUNNEL inport %d enabled\n", inport);
> + if (rc)
> + atomic_dec(&csdev->refcnt[inport]);
> + else
> + dev_dbg(&csdev->dev, "FUNNEL inport %d enabled\n",
> + inport);
> + }
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> return rc;
> }
>
> @@ -107,11 +116,15 @@ static void funnel_disable(struct coresight_device *csdev, int inport,
> int outport)
> {
> struct funnel_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> + unsigned long flags;
>
> - if (drvdata->base)
> - dynamic_funnel_disable_hw(drvdata, inport);
> -
> - dev_dbg(&csdev->dev, "FUNNEL inport %d disabled\n", inport);
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_dec_return(&csdev->refcnt[inport]) == 0) {
> + if (drvdata->base)
> + dynamic_funnel_disable_hw(drvdata, inport);
> + dev_dbg(&csdev->dev, "FUNNEL inport %d disabled\n", inport);
> + }
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> }
>
> static const struct coresight_ops_link funnel_link_ops = {
> diff --git a/drivers/hwtracing/coresight/coresight-replicator.c b/drivers/hwtracing/coresight/coresight-replicator.c
> index b29ba640eb25..427d8b8d0917 100644
> --- a/drivers/hwtracing/coresight/coresight-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> @@ -36,6 +36,7 @@ struct replicator_drvdata {
> void __iomem *base;
> struct clk *atclk;
> struct coresight_device *csdev;
> + spinlock_t spinlock;
> };
>
> static void dynamic_replicator_reset(struct replicator_drvdata *drvdata)
> @@ -97,11 +98,20 @@ static int replicator_enable(struct coresight_device *csdev, int inport,
> {
> int rc = 0;
> struct replicator_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> -
> - if (drvdata->base)
> - rc = dynamic_replicator_enable(drvdata, inport, outport);
> - if (!rc)
> - dev_dbg(&csdev->dev, "REPLICATOR enabled\n");
> + unsigned long flags;
> +
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_inc_return(&csdev->refcnt[outport]) == 1) {
Since we now have the spinlock to protect us, we could simply do an
atomic_read() and then do the hw_enable() followed by an atomic_inc(),
if we are successful. That way we could make it more cleaner and avoid
the atomic_dec() if we encounter a failure. In fact we could simply get
away with the refcnt and replace them with a simple integer, but that
may be a different patch.
> + if (drvdata->base)
> + rc = dynamic_replicator_enable(drvdata, inport,
> + outport);
> +
> + if (rc)
> + atomic_dec(&csdev->refcnt[outport]);
> + else
> + dev_dbg(&csdev->dev, "REPLICATOR enabled\n");
> + }
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> return rc;
> }
>
> @@ -137,10 +147,15 @@ static void replicator_disable(struct coresight_device *csdev, int inport,
> int outport)
> {
> struct replicator_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> + unsigned long flags;
>
> - if (drvdata->base)
> - dynamic_replicator_disable(drvdata, inport, outport);
> - dev_dbg(&csdev->dev, "REPLICATOR disabled\n");
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_dec_return(&csdev->refcnt[outport]) == 0) {
> + if (drvdata->base)
> + dynamic_replicator_disable(drvdata, inport, outport);
> + dev_dbg(&csdev->dev, "REPLICATOR disabled\n");
> + }
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> }
>
> static const struct coresight_ops_link replicator_link_ops = {
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> index 807416b75ecc..cb4a38541bf8 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -334,23 +334,25 @@ static int tmc_disable_etf_sink(struct coresight_device *csdev)
> static int tmc_enable_etf_link(struct coresight_device *csdev,
> int inport, int outport)
> {
> - int ret;
> + int ret = 0;
> unsigned long flags;
> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>
> spin_lock_irqsave(&drvdata->spinlock, flags);
> - if (drvdata->reading) {
> - spin_unlock_irqrestore(&drvdata->spinlock, flags);
> - return -EBUSY;
> + if (atomic_inc_return(&csdev->refcnt[0]) == 1) {
> + if (drvdata->reading)
> + ret = -EBUSY;
Could we not check the drvdata->reading before the refcount and bail
out early ? We are protected by the spinlock anyway. Similar to the
above case we could check the refcount and only increment when we have
enabled.
Cheers
Suzuki