Re: [PATCH v5] coresight: Serialize enabling/disabling a link device.

From: Mathieu Poirier
Date: Wed Oct 30 2019 - 11:15:34 EST

Next message: Krzysztof Kozlowski: "Re: [PATCH][next] soc: samsung: exynos-asv: fix potential overflow in multiply"
Previous message: Theodore Y. Ts'o: "Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail"
In reply to: Yabin Cui: "[PATCH v5] coresight: Serialize enabling/disabling a link device."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, 25 Oct 2019 at 17:53, Yabin Cui <yabinc@xxxxxxxxxx> 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 <yabinc@xxxxxxxxxx>
> ---
>
> Sorry for the delay!
>
> v4 -> v5:
> added document for spinlock fields.
> moved dev_dbg() out of lock section, and verified printed debug msgs.
>
> split atomic_inc_return() into atomic_read() and atomic_inc() as
> commented.
> checked drvdata->reading before refcnt.
>
> ---
> .../hwtracing/coresight/coresight-funnel.c | 36 +++++++++++----
> .../coresight/coresight-replicator.c | 35 ++++++++++++---
> .../hwtracing/coresight/coresight-tmc-etf.c | 26 ++++++++---
> drivers/hwtracing/coresight/coresight.c | 45 ++++++-------------
> 4 files changed, 90 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-funnel.c b/drivers/hwtracing/coresight/coresight-funnel.c
> index 05f7896c3a01..b605889b507a 100644
> --- a/drivers/hwtracing/coresight/coresight-funnel.c
> +++ b/drivers/hwtracing/coresight/coresight-funnel.c
> @@ -38,12 +38,14 @@ DEFINE_CORESIGHT_DEVLIST(funnel_devs, "funnel");
> * @atclk: optional clock for the core parts of the funnel.
> * @csdev: component vitals needed by the framework.
> * @priority: port selection order.
> + * @spinlock: serialize enable/disable operations.
> */
> struct funnel_drvdata {
> void __iomem *base;
> 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,11 +78,21 @@ static int funnel_enable(struct coresight_device *csdev, int inport,
> {
> int rc = 0;
> struct funnel_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> -
> - if (drvdata->base)
> - rc = dynamic_funnel_enable_hw(drvdata, inport);
> -
> + unsigned long flags;
> + bool first_enable = false;
> +
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_read(&csdev->refcnt[inport]) == 0) {
> + if (drvdata->base)
> + rc = dynamic_funnel_enable_hw(drvdata, inport);
> + if (!rc)
> + first_enable = true;
> + }
> if (!rc)
> + atomic_inc(&csdev->refcnt[inport]);
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> + if (first_enable)
> dev_dbg(&csdev->dev, "FUNNEL inport %d enabled\n", inport);
> return rc;
> }
> @@ -107,11 +119,19 @@ 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;
> + bool last_disable = false;
> +
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_dec_return(&csdev->refcnt[inport]) == 0) {
> + if (drvdata->base)
> + dynamic_funnel_disable_hw(drvdata, inport);
> + last_disable = true;
> + }
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> - if (drvdata->base)
> - dynamic_funnel_disable_hw(drvdata, inport);
> -
> - dev_dbg(&csdev->dev, "FUNNEL inport %d disabled\n", inport);
> + if (last_disable)
> + dev_dbg(&csdev->dev, "FUNNEL inport %d disabled\n", inport);
> }
>
> 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..43304196a1a6 100644
> --- a/drivers/hwtracing/coresight/coresight-replicator.c
> +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> @@ -31,11 +31,13 @@ DEFINE_CORESIGHT_DEVLIST(replicator_devs, "replicator");
> * whether this one is programmable or not.
> * @atclk: optional clock for the core parts of the replicator.
> * @csdev: component vitals needed by the framework
> + * @spinlock: serialize enable/disable operations.
> */
> 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,10 +99,22 @@ 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);
> + unsigned long flags;
> + bool first_enable = false;
> +
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_read(&csdev->refcnt[outport]) == 0) {
> + if (drvdata->base)
> + rc = dynamic_replicator_enable(drvdata, inport,
> + outport);
> + if (!rc)
> + first_enable = true;
> + }
> if (!rc)
> + atomic_inc(&csdev->refcnt[outport]);
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> + if (first_enable)
> dev_dbg(&csdev->dev, "REPLICATOR enabled\n");
> return rc;
> }
> @@ -137,10 +151,19 @@ 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;
> + bool last_disable = false;
> +
> + spin_lock_irqsave(&drvdata->spinlock, flags);
> + if (atomic_dec_return(&csdev->refcnt[outport]) == 0) {
> + if (drvdata->base)
> + dynamic_replicator_disable(drvdata, inport, outport);
> + last_disable = true;
> + }
> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> - if (drvdata->base)
> - dynamic_replicator_disable(drvdata, inport, outport);
> - dev_dbg(&csdev->dev, "REPLICATOR disabled\n");
> + if (last_disable)
> + dev_dbg(&csdev->dev, "REPLICATOR disabled\n");
> }
>
> 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..d0cc3985b72a 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c
> @@ -334,9 +334,10 @@ 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);
> + bool first_enable = false;
>
> spin_lock_irqsave(&drvdata->spinlock, flags);
> if (drvdata->reading) {
> @@ -344,12 +345,18 @@ static int tmc_enable_etf_link(struct coresight_device *csdev,
> return -EBUSY;
> }
>
> - ret = tmc_etf_enable_hw(drvdata);
> + if (atomic_read(&csdev->refcnt[0]) == 0) {
> + ret = tmc_etf_enable_hw(drvdata);
> + if (!ret) {
> + drvdata->mode = CS_MODE_SYSFS;
> + first_enable = true;
> + }
> + }
> if (!ret)
> - drvdata->mode = CS_MODE_SYSFS;
> + atomic_inc(&csdev->refcnt[0]);
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> - if (!ret)
> + if (first_enable)
> dev_dbg(&csdev->dev, "TMC-ETF enabled\n");
> return ret;
> }
> @@ -359,6 +366,7 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
> {
> unsigned long flags;
> struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> + bool last_disable = false;
>
> spin_lock_irqsave(&drvdata->spinlock, flags);
> if (drvdata->reading) {
> @@ -366,11 +374,15 @@ static void tmc_disable_etf_link(struct coresight_device *csdev,
> return;
> }
>
> - tmc_etf_disable_hw(drvdata);
> - drvdata->mode = CS_MODE_DISABLED;
> + if (atomic_dec_return(&csdev->refcnt[0]) == 0) {
> + tmc_etf_disable_hw(drvdata);
> + drvdata->mode = CS_MODE_DISABLED;
> + last_disable = true;
> + }
> spin_unlock_irqrestore(&drvdata->spinlock, flags);
>
> - dev_dbg(&csdev->dev, "TMC-ETF disabled\n");
> + if (last_disable)
> + 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);

I have picked up you work - thanks,
Mathieu

>
> for (i = 0; i < nr_conns; i++)
> if (atomic_read(&csdev->refcnt[i]) != 0)
> --
> 2.24.0.rc0.303.g954a862665-goog
>

Next message: Krzysztof Kozlowski: "Re: [PATCH][next] soc: samsung: exynos-asv: fix potential overflow in multiply"
Previous message: Theodore Y. Ts'o: "Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail"
In reply to: Yabin Cui: "[PATCH v5] coresight: Serialize enabling/disabling a link device."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]