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 07/13] coresight: Store pointers to connections rather than an array of them
Date: Mon, 3 Apr 2023 11:16:26 +0100 [thread overview]
Message-ID: <612a9010-6a76-6df3-9223-651e956d7cb8@arm.com> (raw)
In-Reply-To: <c25f0d12-e820-58b0-b139-b2ca322b6b80@arm.com>
On 03/04/2023 09:46, Suzuki K Poulose wrote:
> On 29/03/2023 12:53, James Clark wrote:
>> This will allow the same connection object to be referenced via the
>> input connection list in a later commit rather than duplicating them.
>>
>> Signed-off-by: James Clark <james.clark@arm.com>
>> ---
>> drivers/hwtracing/coresight/coresight-core.c | 45 ++++++++++---------
>> .../hwtracing/coresight/coresight-platform.c | 8 ++--
>> drivers/hwtracing/coresight/coresight-priv.h | 1 +
>> .../hwtracing/coresight/coresight-tmc-etr.c | 2 +-
>> include/linux/coresight.h | 2 +-
>> 5 files changed, 31 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c
>> b/drivers/hwtracing/coresight/coresight-core.c
>> index 12cbb68e8e1c..389f6203c8f0 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -119,7 +119,7 @@ static int coresight_find_link_inport(struct
>> coresight_device *csdev,
>> struct coresight_connection *conn;
>> for (i = 0; i < parent->pdata->nr_outconns; i++) {
>> - conn = &parent->pdata->out_conns[i];
>> + conn = parent->pdata->out_conns[i];
>> if (conn->dest_dev == csdev)
>> return conn->dest_port;
>> }
>> @@ -137,7 +137,7 @@ static int coresight_find_link_outport(struct
>> coresight_device *csdev,
>> struct coresight_connection *conn;
>> for (i = 0; i < csdev->pdata->nr_outconns; i++) {
>> - conn = &csdev->pdata->out_conns[i];
>> + conn = csdev->pdata->out_conns[i];
>> if (conn->dest_dev == child)
>> return conn->src_port;
>> }
>> @@ -606,7 +606,7 @@ coresight_find_enabled_sink(struct
>> coresight_device *csdev)
>> for (i = 0; i < csdev->pdata->nr_outconns; i++) {
>> struct coresight_device *child_dev;
>> - child_dev = csdev->pdata->out_conns[i].dest_dev;
>> + child_dev = csdev->pdata->out_conns[i]->dest_dev;
>> if (child_dev)
>> sink = coresight_find_enabled_sink(child_dev);
>> if (sink)
>> @@ -722,7 +722,7 @@ static int coresight_grab_device(struct
>> coresight_device *csdev)
>> for (i = 0; i < csdev->pdata->nr_outconns; i++) {
>> struct coresight_device *child;
>> - child = csdev->pdata->out_conns[i].dest_dev;
>> + child = csdev->pdata->out_conns[i]->dest_dev;
>> if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
>> if (!coresight_get_ref(child))
>> goto err;
>> @@ -733,7 +733,7 @@ static int coresight_grab_device(struct
>> coresight_device *csdev)
>> for (i--; i >= 0; i--) {
>> struct coresight_device *child;
>> - child = csdev->pdata->out_conns[i].dest_dev;
>> + child = csdev->pdata->out_conns[i]->dest_dev;
>> if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
>> coresight_put_ref(child);
>> }
>> @@ -752,7 +752,7 @@ static void coresight_drop_device(struct
>> coresight_device *csdev)
>> for (i = 0; i < csdev->pdata->nr_outconns; i++) {
>> struct coresight_device *child;
>> - child = csdev->pdata->out_conns[i].dest_dev;
>> + child = csdev->pdata->out_conns[i]->dest_dev;
>> if (child && child->type == CORESIGHT_DEV_TYPE_HELPER)
>> coresight_put_ref(child);
>> }
>> @@ -794,7 +794,7 @@ static int _coresight_build_path(struct
>> coresight_device *csdev,
>> for (i = 0; i < csdev->pdata->nr_outconns; i++) {
>> struct coresight_device *child_dev;
>> - child_dev = csdev->pdata->out_conns[i].dest_dev;
>> + child_dev = csdev->pdata->out_conns[i]->dest_dev;
>> if (child_dev &&
>> _coresight_build_path(child_dev, sink, path) == 0) {
>> found = true;
>> @@ -964,7 +964,7 @@ coresight_find_sink(struct coresight_device
>> *csdev, int *depth)
>> struct coresight_device *child_dev, *sink = NULL;
>> int child_depth = curr_depth;
>> - child_dev = csdev->pdata->out_conns[i].dest_dev;
>> + child_dev = csdev->pdata->out_conns[i]->dest_dev;
>> if (child_dev)
>> sink = coresight_find_sink(child_dev, &child_depth);
>> @@ -1334,7 +1334,7 @@ static int coresight_orphan_match(struct
>> device *dev, void *data)
>> * an orphan connection whose name matches @csdev, link it.
>> */
>> for (i = 0; i < i_csdev->pdata->nr_outconns; i++) {
>> - conn = &i_csdev->pdata->out_conns[i];
>> + conn = i_csdev->pdata->out_conns[i];
>> /* We have found at least one orphan connection */
>> if (conn->dest_dev == NULL) {
>> @@ -1372,7 +1372,7 @@ static int coresight_fixup_device_conns(struct
>> coresight_device *csdev)
>> int i, ret = 0;
>> for (i = 0; i < csdev->pdata->nr_outconns; i++) {
>> - struct coresight_connection *conn = &csdev->pdata->out_conns[i];
>> + struct coresight_connection *conn = csdev->pdata->out_conns[i];
>> conn->dest_dev =
>> coresight_find_csdev_by_fwnode(conn->dest_fwnode);
>> @@ -1406,15 +1406,12 @@ static int coresight_remove_match(struct
>> device *dev, void *data)
>> * a connection whose name matches @csdev, remove it.
>> */
>> for (i = 0; i < iterator->pdata->nr_outconns; i++) {
>> - conn = &iterator->pdata->out_conns[i];
>> + conn = iterator->pdata->out_conns[i];
>> - if (conn->dest_dev == NULL)
>> - continue;
>> -
>> - if (csdev->dev.fwnode == conn->dest_fwnode) {
>> + /* Child_dev being set signifies that the links were made */
>> + if (csdev->dev.fwnode == conn->dest_fwnode && conn->dest_dev) {
>> iterator->orphan = true;
>> coresight_remove_links(iterator, conn);
>> -
>> conn->dest_dev = NULL;
>> /* No need to continue */
>> break;
>> @@ -1534,21 +1531,25 @@ void coresight_write64(struct coresight_device
>> *csdev, u64 val, u32 offset)
>> * to the output port of this device.
>> */
>> void coresight_release_platform_data(struct coresight_device *csdev,
>> + struct device *dev,
>> struct coresight_platform_data *pdata)
>> {
>> int i;
>> - struct coresight_connection *conns = pdata->out_conns;
>> + struct coresight_connection **conns = pdata->out_conns;
>> for (i = 0; i < pdata->nr_outconns; i++) {
>> /* If we have made the links, remove them now */
>> - if (csdev && conns[i].dest_dev)
>> - coresight_remove_links(csdev, &conns[i]);
>> + if (csdev && conns[i]->dest_dev)
>> + coresight_remove_links(csdev, conns[i]);
>> /*
>> * Drop the refcount and clear the handle as this device
>> * is going away
>> */
>> - fwnode_handle_put(conns[i].dest_fwnode);
>> + fwnode_handle_put(conns[i]->dest_fwnode);
>> + devm_kfree(dev, conns[i]);
>> }
>> + devm_kfree(dev, pdata->out_conns);
>> + devm_kfree(dev, pdata);
>
> Is there any particular reason, why we need to do this ? This should
> be done automatically at device teardown, which is bound to happen
> right after this call ?
>
I think they're not actually freed because connections are added before
the coresight device is created using the 'parent' of the device which
never goes away:
struct coresight_device *coresight_register(struct coresight_desc *desc) {
...
csdev->dev.parent = desc->dev;
Another reason is that's is a good way to catch errors. There are so
many different devices floating around that without an explicit free,
it's very easy to pass the wrong one and be inconsistent. With the free
it gives you a warning if it's mismatched.
But either way, now that we know that they're not freed maybe we should
fix that isntead? Although apart from doing a full copy of pdata into
the coresight device I'm not sure how it can be done. Then I suppose we
could remove these frees, although I still think the error catching is
good. It's also not strictly related to this fix so maybe it can be a
second fix.
>
>> if (csdev)
>> coresight_remove_conns_sysfs_group(csdev);
>> }
>> @@ -1665,7 +1666,7 @@ struct coresight_device
>> *coresight_register(struct coresight_desc *desc)
>> err_out:
>> /* Cleanup the connection information */
>> - coresight_release_platform_data(NULL, desc->pdata);
>> + coresight_release_platform_data(NULL, desc->dev, desc->pdata);
>> return ERR_PTR(ret);
>> }
>> EXPORT_SYMBOL_GPL(coresight_register);
>> @@ -1678,7 +1679,7 @@ void coresight_unregister(struct
>> coresight_device *csdev)
>> cti_assoc_ops->remove(csdev);
>> coresight_remove_conns(csdev);
>> coresight_clear_default_sink(csdev);
>> - coresight_release_platform_data(csdev, csdev->pdata);
>> + coresight_release_platform_data(csdev, csdev->dev.parent,
>> csdev->pdata);
>> device_unregister(&csdev->dev);
>> }
>> EXPORT_SYMBOL_GPL(coresight_unregister);
>> diff --git a/drivers/hwtracing/coresight/coresight-platform.c
>> b/drivers/hwtracing/coresight/coresight-platform.c
>> index 80ed2e74620b..bea8f1ba309a 100644
>> --- a/drivers/hwtracing/coresight/coresight-platform.c
>> +++ b/drivers/hwtracing/coresight/coresight-platform.c
>> @@ -36,7 +36,7 @@ int coresight_add_out_conn(struct device *dev,
>> * Warn on any existing duplicate output port.
>> */
>> for (i = 0; i < pdata->nr_outconns; ++i) {
>> - conn = &pdata->out_conns[i];
>> + conn = pdata->out_conns[i];
>> /* Output == -1 means ignore the port for example for
>> helpers */
>> if (conn->src_port != -1 &&
>> conn->src_port == new_conn->src_port) {
>> @@ -53,7 +53,9 @@ int coresight_add_out_conn(struct device *dev,
>> if (!pdata->out_conns)
>> return -ENOMEM;
>> - pdata->out_conns[pdata->nr_outconns - 1] = *new_conn;
>> + pdata->out_conns[pdata->nr_outconns - 1] = devm_kmalloc(
>> + dev, sizeof(struct coresight_connection), GFP_KERNEL);
>> + *pdata->out_conns[pdata->nr_outconns - 1] = *new_conn;
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(coresight_add_out_conn);
>> @@ -859,7 +861,7 @@ coresight_get_platform_data(struct device *dev)
>> error:
>> if (!IS_ERR_OR_NULL(pdata))
>> /* Cleanup the connection information */
>> - coresight_release_platform_data(NULL, pdata);
>> + coresight_release_platform_data(NULL, dev, pdata);
>> return ERR_PTR(ret);
>> }
>> EXPORT_SYMBOL_GPL(coresight_get_platform_data);
>> diff --git a/drivers/hwtracing/coresight/coresight-priv.h
>> b/drivers/hwtracing/coresight/coresight-priv.h
>> index 788ff19c60f6..65ae6d161c57 100644
>> --- a/drivers/hwtracing/coresight/coresight-priv.h
>> +++ b/drivers/hwtracing/coresight/coresight-priv.h
>> @@ -207,6 +207,7 @@ static inline void *coresight_get_uci_data(const
>> struct amba_id *id)
>> }
>> void coresight_release_platform_data(struct coresight_device *csdev,
>> + struct device *dev,
>> struct coresight_platform_data *pdata);
>> struct coresight_device *
>> coresight_find_csdev_by_fwnode(struct fwnode_handle *r_fwnode);
>> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> index 61234cb8052a..1bbe5410a23d 100644
>> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
>> @@ -782,7 +782,7 @@ tmc_etr_get_catu_device(struct tmc_drvdata *drvdata)
>> return NULL;
>> for (i = 0; i < etr->pdata->nr_outconns; i++) {
>> - tmp = etr->pdata->out_conns[i].dest_dev;
>> + tmp = etr->pdata->out_conns[i]->dest_dev;
>> if (tmp && coresight_is_catu_device(tmp))
>> return tmp;
>> }
>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
>> index ccbc5eafcb6b..7197b07deede 100644
>> --- a/include/linux/coresight.h
>> +++ b/include/linux/coresight.h
>> @@ -111,7 +111,7 @@ struct coresight_platform_data {
>> int high_outport;
>> int nr_inconns;
>> int nr_outconns;
>> - struct coresight_connection *out_conns;
>> + struct coresight_connection **out_conns;
>
> minor nit: Do we need to update the comment too ?
>
Will do
> Otherwise looks good to me
> Suzuki
>
>> };
>> /**
>
next prev parent reply other threads:[~2023-04-03 10:16 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 [this message]
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
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=612a9010-6a76-6df3-9223-651e956d7cb8@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).