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