From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id C8169C32750 for ; Tue, 13 Aug 2019 21:30:43 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 98AE12067D for ; Tue, 13 Aug 2019 21:30:43 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Xfhuiu7F"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="rYu0s+BQ" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 98AE12067D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=Q77ys2qNToGxb8SmDnfRM+p0m4zaBXWBMmw0rCzberg=; b=Xfhuiu7F/PQ0W6 NMfaTw7pVkni3IzjHJBdobUmF11U4K04uAYpPljUWbcQ3vsn+PvEUifw8S+2uHz8PRySxKGgCJPqR Z+CRXCSlnNpBF9lKY1oPL4Je4U3s73k3KaPbDlz8mJ0heu3/H8eydcbjzipkwCUWuJlsNpONjJQ+K SmUkal8LiQIi+ZTSRlcLnH83JhHCeOcSlyAfc80NJzEqCH6IV87Gz11FSBb/OAlTBg430QwyQDKmc WAP5fceJ7syzx+jDW6uJ6+2av18w50hbEFHd2Eu92POvZeExYrsUyHaCt7F4yh1vUTl+hQqQqt3LQ v2c4G2KGSES39jufze7A==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1hxeNK-0000Pk-IM; Tue, 13 Aug 2019 21:30:38 +0000 Received: from mail-ot1-x342.google.com ([2607:f8b0:4864:20::342]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1hxeNG-0000Ov-JG for linux-arm-kernel@lists.infradead.org; Tue, 13 Aug 2019 21:30:36 +0000 Received: by mail-ot1-x342.google.com with SMTP id f17so42173640otq.4 for ; Tue, 13 Aug 2019 14:30:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=u+G2zy96VnmH+zRcEgTdWubjTvGo5IMvJNYRtMTxw04=; b=rYu0s+BQvtmdoikVCTb6tLYsN1sDtbMeBmIDG0wOuMSsygNONs/e3TDuYfwpva1Xoo h7NPf+gsUEGZg+HcPEdFBtswVYcZFgAYfl0C+xFtm6G9biCG5VYrhGI8aUuMmp4bkinC tECbByKVkOb0JCEfQ+5w/kqI6rYmLMZO25nNzwdo5Hp18mLUscDNGI+Fr9W97fQKi/RO 9/LredGzO8VPBZpt2FRHvwohU7FC+k2MA4YwfdaHIyu3EAxv1rRvzCK17PRSkK50aBjL pVB3OmCESjWKYPrZSs43tbZEyJ0BhBuXZgDis1mTNti1OcjPR7+ePRuwvv0O8BA8GkFd UH0w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=u+G2zy96VnmH+zRcEgTdWubjTvGo5IMvJNYRtMTxw04=; b=g1nUcgfd0PMZtcs87Q8TpC8+ccCQdIedUmEmziJNCLNSQWQ3qcDgsmVIxWp4PaonrF 1PqRQCPAUukXg5ZU613WQEqo10Orh0FOaFrcfnGNvgxfpjIJx86GuktNzrs64dsIEdJR kUWupJDCpi8S8AbN9LhSYm15TWy5ujpE6EzrjqGRkHG70Y8v83nCleu9BEgpxYfkJa1R lBUFgL43T4WfrREnkmP1ILVkcaHIg1FpamVs8OKwMTKhDrxY6wUjpyY/pOpMu3jjfZQe Eb8m2blYw3x4T+k5z1zaAYxNiBrOjtsx3ieZwf6N1GMo7rO5Ju2OiAJRWKtEdGYKfw7Q 74KA== X-Gm-Message-State: APjAAAWTCbYdMOn8vCkBa+c9NF/DLjvns+3f27sI9Cv0S5dhw5WwGzt7 pdG5FCGychtIpW3iZ0bM+INsUXSmPnMUVhL6vFerE93LGoI= X-Google-Smtp-Source: APXvYqy2TmxV0nrsRl3QgD8/+Drr+1lMWGMeydxHOJ6S+P2RASj4gQx4YYrpxwDsLLfnwq1Yg5HYVQW1KKzejLihI8Q= X-Received: by 2002:a5e:db0a:: with SMTP id q10mr23818923iop.58.1565731832847; Tue, 13 Aug 2019 14:30:32 -0700 (PDT) MIME-Version: 1.0 References: <20190812195725.11793-1-yabinc@google.com> In-Reply-To: <20190812195725.11793-1-yabinc@google.com> From: Mathieu Poirier Date: Tue, 13 Aug 2019 15:30:21 -0600 Message-ID: Subject: Re: [PATCH v3] coresight: Serialize enabling/disabling a link device. To: Yabin Cui X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190813_143034_646405_6D74BC23 X-CRM114-Status: GOOD ( 26.90 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Alexander Shishkin , Linux Kernel Mailing List , linux-arm-kernel , Suzuki K Poulose Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Yabin, On Mon, 12 Aug 2019 at 13:57, 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. > > 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 > --- > > 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 > #include > #include > +#include > > /* 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