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

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

[Yabin Cui]

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 adds a spinlock to serialize
enabling/disabling a link device.

Fixes: a06ae8609b3d ("coresight: add CoreSight core layer framework")
Signed-off-by: Yabin Cui <yabinc@google.com>
---

v2 -> v3: extend lock range to protect csdev->enable for link devices.
          Add fixes tag.

---
 drivers/hwtracing/coresight/coresight.c | 12 +++++++++++-
 include/linux/coresight.h               |  3 +++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
index 55db77f6410b..c069ada151b8 100644
--- a/drivers/hwtracing/coresight/coresight.c
+++ b/drivers/hwtracing/coresight/coresight.c
@@ -256,6 +256,7 @@ static int coresight_enable_link(struct coresight_device *csdev,
 	int ret;
 	int link_subtype;
 	int refport, inport, outport;
+	unsigned long flags;
 
 	if (!parent || !child)
 		return -EINVAL;
@@ -274,17 +275,20 @@ static int coresight_enable_link(struct coresight_device *csdev,
 	if (refport < 0)
 		return refport;
 
+	spin_lock_irqsave(&csdev->spinlock, flags);
 	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]);
+				spin_unlock_irqrestore(&csdev->spinlock, flags);
 				return ret;
 			}
 		}
 	}
 
 	csdev->enable = true;
+	spin_unlock_irqrestore(&csdev->spinlock, flags);
 
 	return 0;
 }
@@ -296,6 +300,7 @@ static void coresight_disable_link(struct coresight_device *csdev,
 	int i, nr_conns;
 	int link_subtype;
 	int refport, inport, outport;
+	unsigned long flags;
 
 	if (!parent || !child)
 		return;
@@ -315,16 +320,20 @@ static void coresight_disable_link(struct coresight_device *csdev,
 		nr_conns = 1;
 	}
 
+	spin_lock_irqsave(&csdev->spinlock, flags);
 	if (atomic_dec_return(&csdev->refcnt[refport]) == 0) {
 		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)
+		if (atomic_read(&csdev->refcnt[i]) != 0) {
+			spin_unlock_irqrestore(&csdev->spinlock, flags);
 			return;
+		}
 
 	csdev->enable = false;
+	spin_unlock_irqrestore(&csdev->spinlock, flags);
 }
 
 static int coresight_enable_source(struct coresight_device *csdev, u32 mode)
@@ -1225,6 +1234,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
 	csdev->subtype = desc->subtype;
 	csdev->ops = desc->ops;
 	csdev->orphan = false;
+	spin_lock_init(&csdev->spinlock);
 
 	csdev->dev.type = &coresight_dev_type[desc->type];
 	csdev->dev.groups = desc->groups;
diff --git a/include/linux/coresight.h b/include/linux/coresight.h
index a2b68823717b..dd28d9ab841d 100644
--- a/include/linux/coresight.h
+++ b/include/linux/coresight.h
@@ -9,6 +9,7 @@
 #include <linux/device.h>
 #include <linux/perf_event.h>
 #include <linux/sched.h>
+#include <linux/spinlock.h>
 
 /* Peripheral id registers (0xFD0-0xFEC) */
 #define CORESIGHT_PERIPHIDR4	0xfd0
@@ -153,6 +154,7 @@ struct coresight_connection {
  *		activated but not yet enabled.  Enabling for a _sink_
  *		appens when a source has been selected for that it.
  * @ea:		Device attribute for sink representation under PMU directory.
+ * @spinlock:	Serialize enabling/disabling this device.
  */
 struct coresight_device {
 	struct coresight_platform_data *pdata;
@@ -166,6 +168,7 @@ struct coresight_device {
 	/* sink specific fields */
 	bool activated;	/* true only if a sink is part of a path */
 	struct dev_ext_attribute *ea;
+	spinlock_t spinlock;
 };
 
 /*
-- 
2.23.0.rc1.153.gdeed80330f-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mathieu Poirier]

Hi Yabin,

On Mon, 12 Aug 2019 at 13:57, 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 adds a spinlock to serialize
> enabling/disabling a link device.
>
> Fixes: a06ae8609b3d ("coresight: add CoreSight core layer framework")

When "a06ae8609b3d" was introduced there wasn't any race condition as
all access to links were guarded by the coresight_mutex in
coresight_enable/disable() functions.  The problem was really
introduced when integration with the perf subsystem was added, though
it would have been really difficult to trigger due to the HW
topologies available at the time.

So, to be exact:

Fixes: 0bcbf2e30ff2 ("coresight: etm-perf: new PMU driver for ETM tracers")

> Signed-off-by: Yabin Cui <yabinc@google.com>
> ---
>
> v2 -> v3: extend lock range to protect csdev->enable for link devices.
>           Add fixes tag.
>
> ---
>  drivers/hwtracing/coresight/coresight.c | 12 +++++++++++-
>  include/linux/coresight.h               |  3 +++
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c
> index 55db77f6410b..c069ada151b8 100644
> --- a/drivers/hwtracing/coresight/coresight.c
> +++ b/drivers/hwtracing/coresight/coresight.c
> @@ -256,6 +256,7 @@ static int coresight_enable_link(struct coresight_device *csdev,
>         int ret;
>         int link_subtype;
>         int refport, inport, outport;
> +       unsigned long flags;
>
>         if (!parent || !child)
>                 return -EINVAL;
> @@ -274,17 +275,20 @@ static int coresight_enable_link(struct coresight_device *csdev,
>         if (refport < 0)
>                 return refport;
>
> +       spin_lock_irqsave(&csdev->spinlock, flags);
>         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]);
> +                               spin_unlock_irqrestore(&csdev->spinlock, flags);
>                                 return ret;
>                         }
>                 }
>         }
>
>         csdev->enable = true;
> +       spin_unlock_irqrestore(&csdev->spinlock, flags);
>
>         return 0;
>  }
> @@ -296,6 +300,7 @@ static void coresight_disable_link(struct coresight_device *csdev,
>         int i, nr_conns;
>         int link_subtype;
>         int refport, inport, outport;
> +       unsigned long flags;
>
>         if (!parent || !child)
>                 return;
> @@ -315,16 +320,20 @@ static void coresight_disable_link(struct coresight_device *csdev,
>                 nr_conns = 1;
>         }
>
> +       spin_lock_irqsave(&csdev->spinlock, flags);
>         if (atomic_dec_return(&csdev->refcnt[refport]) == 0) {
>                 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)
> +               if (atomic_read(&csdev->refcnt[i]) != 0) {
> +                       spin_unlock_irqrestore(&csdev->spinlock, flags);
>                         return;
> +               }
>
>         csdev->enable = false;
> +       spin_unlock_irqrestore(&csdev->spinlock, flags);
>  }
>
>  static int coresight_enable_source(struct coresight_device *csdev, u32 mode)
> @@ -1225,6 +1234,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc)
>         csdev->subtype = desc->subtype;
>         csdev->ops = desc->ops;
>         csdev->orphan = false;
> +       spin_lock_init(&csdev->spinlock);

Your patch will work and I see the problem it is addressing.  I
contemplated doing exactly the same thing for sink devices but
couldn't due to memory management issues.  In the end I moved the
reference counting inside each driver where it could be guarded by
drvdata->spinlock.

I suggest to do the same thing for links to avoid dealing with two
different ways of doing things.  As such we could get rid of the
"refport" thing I never liked in coresight_enable_link(), invariably
call link_ops()->enable() and let each driver deal with its own port
management.  The same logic applies to the disable() path.

The patch isn't difficult to do but does go deeper in each link
drivers (ETF, funnel, replicator).  Let me know if you are not
comfortable with the idea and I will see what I can do on my side.

Thanks,
Mathieu

>
>         csdev->dev.type = &coresight_dev_type[desc->type];
>         csdev->dev.groups = desc->groups;
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index a2b68823717b..dd28d9ab841d 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -9,6 +9,7 @@
>  #include <linux/device.h>
>  #include <linux/perf_event.h>
>  #include <linux/sched.h>
> +#include <linux/spinlock.h>
>
>  /* Peripheral id registers (0xFD0-0xFEC) */
>  #define CORESIGHT_PERIPHIDR4   0xfd0
> @@ -153,6 +154,7 @@ struct coresight_connection {
>   *             activated but not yet enabled.  Enabling for a _sink_
>   *             appens when a source has been selected for that it.
>   * @ea:                Device attribute for sink representation under PMU directory.
> + * @spinlock:  Serialize enabling/disabling this device.
>   */
>  struct coresight_device {
>         struct coresight_platform_data *pdata;
> @@ -166,6 +168,7 @@ struct coresight_device {
>         /* sink specific fields */
>         bool activated; /* true only if a sink is part of a path */
>         struct dev_ext_attribute *ea;
> +       spinlock_t spinlock;
>  };
>
>  /*
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Yabin Cui]

> The patch isn't difficult to do but does go deeper in each link
> drivers (ETF, funnel, replicator).  Let me know if you are not
> comfortable with the idea and I will see what I can do on my side.

I am comfortable with the idea. I chose to add lock in coresight_enable_link() just because it
is the smallest change. But a refactor probably is better for maintainance.

Feel free to upload a new patch fixing the problem. I can help to test it.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Mathieu Poirier]

On Tue, 13 Aug 2019 at 18:36, Yabin Cui <yabinc@google.com> wrote:
>
> > The patch isn't difficult to do but does go deeper in each link
> > drivers (ETF, funnel, replicator).  Let me know if you are not
> > comfortable with the idea and I will see what I can do on my side.
>
> I am comfortable with the idea. I chose to add lock in coresight_enable_link() just because it
> is the smallest change. But a refactor probably is better for maintainance.

Yes, that is  my prime concern.

>
> Feel free to upload a new patch fixing the problem. I can help to test it.

I have a few things on my plate at this time so you might beat me to
the punch.  I'll see how the next few days go.

Mathieu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel