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

[PATCH v2] 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.

Signed-off-by: Yabin Cui <yabinc@google.com>
---

v1 -> v2: extend lock range to protect read of refcnt in
coresight_disable_link().

---
 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..e526bdeaeb22 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,15 +275,18 @@ 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;
 			}
 		}
 	}
+	spin_unlock_irqrestore(&csdev->spinlock, flags);
 
 	csdev->enable = true;
 
@@ -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,14 +320,18 @@ 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;
+		}
+	spin_unlock_irqrestore(&csdev->spinlock, flags);
 
 	csdev->enable = false;
 }
@@ -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

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

[Suzuki K Poulose]

Hi Yabin,

On 09/08/2019 22:45, 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 adds a spinlock to serialize
> enabling/disabling a link device.
> 
> Signed-off-by: Yabin Cui <yabinc@google.com>
> ---
> 
> v1 -> v2: extend lock range to protect read of refcnt in
> coresight_disable_link().

Thanks for this. Please find my comments below.

> 
> ---
>   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..e526bdeaeb22 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,15 +275,18 @@ 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;
>   			}
>   		}
>   	}
> +	spin_unlock_irqrestore(&csdev->spinlock, flags);
>   
>   	csdev->enable = true;

Please could we move this inside the spin_lock () too ?

>   
> @@ -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,14 +320,18 @@ 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;
> +		}
> +	spin_unlock_irqrestore(&csdev->spinlock, flags);
>   
>   	csdev->enable = false;
>   }

And this too ? I understand this may not be used right now, but we can avoid any
surprises when we do so.

With the above fixed, :

Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>

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

[Suzuki K Poulose]

Hi Yabin,

On 12/08/2019 11:38, Suzuki K Poulose wrote:
> 
> Hi Yabin,
> 
> On 09/08/2019 22:45, 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 adds a spinlock to serialize
>> enabling/disabling a link device.

Please could you also add :

Fixes : a06ae8609b3dd ("coresight: add CoreSight core layer framework")

Suzuki