| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3f090ce3-618d-56d1-1ab6-b38de5c86393@arm.com>
Date: Mon, 3 Apr 2023 12:47:38 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: James Clark <james.clark@....com>, coresight@...ts.linaro.org,
quic_jinlmao@...cinc.com, mike.leach@...aro.org
Cc: Mathieu Poirier <mathieu.poirier@...aro.org>,
Leo Yan <leo.yan@...aro.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH v3 10/13] coresight: Make refcount a property of the
connection
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@....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)
Suzuki
Powered by blists - more mailing lists