Messages in this thread | | | Subject | Re: [PATCH v2] coresight: Serialize enabling/disabling a link device. | From | Suzuki K Poulose <> | Date | Mon, 12 Aug 2019 11:38:59 +0100 |
| |
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>
| |