From: James Clark <james.clark@arm.com>
To: Suzuki K Poulose <suzuki.poulose@arm.com>,
coresight@lists.linaro.org, quic_jinlmao@quicinc.com,
mike.leach@linaro.org
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>,
Leo Yan <leo.yan@linaro.org>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Maxime Coquelin <mcoquelin.stm32@gmail.com>,
Alexandre Torgue <alexandre.torgue@foss.st.com>,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org,
linux-stm32@st-md-mailman.stormreply.com
Subject: Re: [PATCH v3 10/13] coresight: Make refcount a property of the connection
Date: Mon, 3 Apr 2023 15:13:44 +0100 [thread overview]
Message-ID: <2ed464b9-1754-b649-050d-14c070faa67b@arm.com> (raw)
In-Reply-To: <3f090ce3-618d-56d1-1ab6-b38de5c86393@arm.com>
On 03/04/2023 12:47, Suzuki K Poulose wrote:
> On 29/03/2023 12:53, James Clark wrote:
>> This removes the need to do an additional lookup for the total number
>> of ports used and also removes the need to allocate an array of
>> refcounts which is just another representation of a connection array.
>>
>> This was only used for link type devices, for regular devices a single
>> refcount on the coresight device is used.
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>> drivers/hwtracing/coresight/coresight-core.c | 96 +++++++--------
>> drivers/hwtracing/coresight/coresight-etb10.c | 10 +-
>> .../hwtracing/coresight/coresight-funnel.c | 26 +++--
>> .../hwtracing/coresight/coresight-platform.c | 109 +-----------------
>> .../coresight/coresight-replicator.c | 23 ++--
>> .../hwtracing/coresight/coresight-tmc-etf.c | 24 ++--
>> .../hwtracing/coresight/coresight-tmc-etr.c | 12 +-
>> drivers/hwtracing/coresight/coresight-tpda.c | 23 ++--
>> drivers/hwtracing/coresight/coresight-tpiu.c | 4 +-
>> drivers/hwtracing/coresight/ultrasoc-smb.c | 8 +-
>> include/linux/coresight.h | 13 ++-
>> 11 files changed, 124 insertions(+), 224 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c
>> b/drivers/hwtracing/coresight/coresight-core.c
>> index be1e8be2459f..baa23aa53971 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -112,8 +112,9 @@ struct coresight_device
>> *coresight_get_percpu_sink(int cpu)
>> }
>> EXPORT_SYMBOL_GPL(coresight_get_percpu_sink);
>> -static int coresight_find_link_inport(struct coresight_device *csdev,
>> - struct coresight_device *parent)
>> +static struct coresight_connection *
>> +coresight_find_link_inport(struct coresight_device *csdev,
>> + struct coresight_device *parent)
>> {
>> int i;
>> struct coresight_connection *conn;
>> @@ -121,17 +122,18 @@ static int coresight_find_link_inport(struct
>> coresight_device *csdev,
>> for (i = 0; i < parent->pdata->nr_outconns; i++) {
>> conn = parent->pdata->out_conns[i];
>> if (conn->dest_dev == csdev)
>> - return conn->dest_port;
>> + return conn;
>> }
>> dev_err(&csdev->dev, "couldn't find inport, parent: %s, child:
>> %s\n",
>> dev_name(&parent->dev), dev_name(&csdev->dev));
>> - return -ENODEV;
>> + return ERR_PTR(-ENODEV);
>> }
>> -static int coresight_find_link_outport(struct coresight_device *csdev,
>> - struct coresight_device *child)
>> +static struct coresight_connection *
>> +coresight_find_link_outport(struct coresight_device *csdev,
>> + struct coresight_device *child)
>> {
>> int i;
>> struct coresight_connection *conn;
>> @@ -139,13 +141,13 @@ static int coresight_find_link_outport(struct
>> coresight_device *csdev,
>> for (i = 0; i < csdev->pdata->nr_outconns; i++) {
>> conn = csdev->pdata->out_conns[i];
>> if (conn->dest_dev == child)
>> - return conn->src_port;
>> + return conn;
>> }
>> dev_err(&csdev->dev, "couldn't find outport, parent: %s,
>> child: %s\n",
>> dev_name(&csdev->dev), dev_name(&child->dev));
>> - return -ENODEV;
>> + return ERR_PTR(-ENODEV);
>> }
>
> minor nit: I think the above two functions are exactly similar except
> for the error message ? And could be unified by fixing the caller ?
>
> coresight_find_connection(src_dev, dst_dev)
>
>
>
>> static inline u32 coresight_read_claim_tags(struct
>> coresight_device *csdev)
>> @@ -352,24 +354,24 @@ static int coresight_enable_link(struct
>> coresight_device *csdev,
>> {
>> int ret = 0;
>> int link_subtype;
>> - int inport, outport;
>> + struct coresight_connection *inconn, *outconn;
>> if (!parent || !child)
>> return -EINVAL;
>> - inport = coresight_find_link_inport(csdev, parent);
>> - outport = coresight_find_link_outport(csdev, child);
>> + inconn = coresight_find_link_inport(csdev, parent);
>> + outconn = coresight_find_link_outport(csdev, child);
>
> here :
>
>
> outconn = coresight_find_connection(csdev, child);
> inconn = coresight_find_connection(parent, csdev);
>
>> link_subtype = csdev->subtype.link_subtype;
>> - 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 (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG &&
>> IS_ERR(inconn))
>> + return PTR_ERR(inconn);
>> + if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT &&
>> IS_ERR(outconn))
>> + return PTR_ERR(outconn);
>> if (link_ops(csdev)->enable) {
>> ret = coresight_control_assoc_ectdev(csdev, true);
>> if (!ret) {
>> - ret = link_ops(csdev)->enable(csdev, inport, outport);
>> + ret = link_ops(csdev)->enable(csdev, inconn, outconn);
>> if (ret)
>> coresight_control_assoc_ectdev(csdev, false);
>> }
>> @@ -385,33 +387,36 @@ static void coresight_disable_link(struct
>> coresight_device *csdev,
>> struct coresight_device *parent,
>> struct coresight_device *child)
>> {
>> - int i, nr_conns;
>> + int i;
>> int link_subtype;
>> - int inport, outport;
>> + struct coresight_connection *inconn, *outconn;
>> if (!parent || !child)
>> return;
>> - inport = coresight_find_link_inport(csdev, parent);
>> - outport = coresight_find_link_outport(csdev, child);
>> + inconn = coresight_find_link_inport(csdev, parent);
>> + outconn = coresight_find_link_outport(csdev, child);
>
> similarly here
>
>> link_subtype = csdev->subtype.link_subtype;
>> - if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG) {
>> - nr_conns = csdev->pdata->high_inport;
>> - } else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT) {
>> - nr_conns = csdev->pdata->high_outport;
>> - } else {
>> - nr_conns = 1;
>> - }
>> -
>> if (link_ops(csdev)->disable) {
>> - link_ops(csdev)->disable(csdev, inport, outport);
>> + link_ops(csdev)->disable(csdev, inconn, outconn);
>> coresight_control_assoc_ectdev(csdev, false);
>> }
>> - for (i = 0; i < nr_conns; i++)
>> - if (atomic_read(&csdev->refcnt[i]) != 0)
>> + if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG) {
>> + for (i = 0; i < csdev->pdata->nr_inconns; i++)
>> + if (atomic_read(&csdev->pdata->in_conns[i]->refcnt) !=
>> + 0)
>> + return;
>> + } else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT) {
>> + for (i = 0; i < csdev->pdata->nr_outconns; i++)
>> + if (atomic_read(&csdev->pdata->out_conns[i]->refcnt) !=
>> + 0)
>> + return;
>> + } else {
>> + if (atomic_read(&csdev->refcnt) != 0)
>> return;
>> + }
>
> I am slightly concerned about a case where a (practically a corner case,
> but not impossible) replicator-out could be connected to a funnel-in. In
> which case, we might be operating on the same "connection" and could
> mess up with the refcounting and may not do the right thing (e.g, enable
> the funnel - depends on in_conn ref, when the Replicator was enabled -
> touches out_conn ref)
>
Yeah I think you are right. It's possible that if an output increases
its refcount first, then the input will skip the ..._enable_hw() call
because it will look like that connection is already enabled.
It looks like it would be solved by splitting the refcounts into
separate in and out refcount members. The other alternative is not
sharing the connection object between inputs and outputs which is a big
change and didn't feel like it was the right thing to do before.
Splitting them actually feels like it would be the right thing to do
anyway, and it would be excatly the same as the old version then.
> Suzuki
>
>
next prev parent reply other threads:[~2023-04-03 14:13 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-29 11:53 [PATCH v3 00/13] coresight: Fix CTI module refcount leak by making it a helper device James Clark
2023-03-29 11:53 ` [PATCH v3 01/13] coresight: Use enum type for cs_mode wherever possible James Clark
2023-03-29 11:53 ` [PATCH v3 02/13] coresight: Change name of pdata->conns James Clark
2023-03-29 11:53 ` [PATCH v3 03/13] coresight: Rename nr_outports to nr_outconns James Clark
2023-03-29 11:53 ` [PATCH v3 04/13] coresight: Rename connection members to make the direction explicit James Clark
2023-03-29 11:53 ` [PATCH v3 05/13] coresight: Dynamically add connections James Clark
2023-03-29 11:53 ` [PATCH v3 06/13] coresight: Fix loss of connection info when a module is unloaded James Clark
2023-03-30 12:42 ` Suzuki K Poulose
2023-03-30 14:01 ` Suzuki K Poulose
2023-03-29 11:53 ` [PATCH v3 07/13] coresight: Store pointers to connections rather than an array of them James Clark
2023-04-03 8:46 ` Suzuki K Poulose
2023-04-03 10:16 ` James Clark
2023-04-03 11:11 ` Suzuki K Poulose
2023-03-29 11:53 ` [PATCH v3 08/13] coresight: Simplify connection fixup mechanism James Clark
2023-04-03 9:18 ` Suzuki K Poulose
2023-03-29 11:53 ` [PATCH v3 09/13] coresight: Store in-connections as well as out-connections James Clark
2023-04-03 10:17 ` Suzuki K Poulose
2023-03-29 11:53 ` [PATCH v3 10/13] coresight: Make refcount a property of the connection James Clark
2023-04-03 11:47 ` Suzuki K Poulose
2023-04-03 14:13 ` James Clark [this message]
2023-03-29 11:53 ` [PATCH v3 11/13] coresight: Refactor out buffer allocation function for ETR James Clark
2023-03-29 11:53 ` [PATCH v3 12/13] coresight: Enable and disable helper devices adjacent to the path James Clark
2023-04-03 17:54 ` Suzuki K Poulose
2023-03-29 11:53 ` [PATCH v3 13/13] coresight: Fix CTI module refcount leak by making it a helper device James Clark
2023-04-04 9:21 ` Suzuki K Poulose
2023-04-04 12:55 ` James Clark
2023-04-04 13:04 ` James Clark
2023-04-04 13:59 ` Suzuki K Poulose
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2ed464b9-1754-b649-050d-14c070faa67b@arm.com \
--to=james.clark@arm.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=alexandre.torgue@foss.st.com \
--cc=coresight@lists.linaro.org \
--cc=leo.yan@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=mathieu.poirier@linaro.org \
--cc=mcoquelin.stm32@gmail.com \
--cc=mike.leach@linaro.org \
--cc=quic_jinlmao@quicinc.com \
--cc=suzuki.poulose@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).