Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191025235321.181897-1-yabinc@google.com>
In-Reply-To: <20191025235321.181897-1-yabinc@google.com>
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
Date:   Wed, 30 Oct 2019 09:15:19 -0600
Message-ID: <CANLsYkxexrP_uuNS6SUmj1DyE0q8whj4i=ZdZs+PRTazxzCgaA@mail.gmail.com>
Subject: Re: [PATCH v5] coresight: Serialize enabling/disabling a link device.
To:     Yabin Cui <yabinc@google.com>
Cc:     Suzuki K Poulose <suzuki.poulose@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 25 Oct 2019 at 17:53, Yabin Cui <yabinc@google.com> 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@google.com>
> ---
>
> 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
>