| 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: <a25804a5-c949-098e-43de-9c9046fdb2de@arm.com>
Date: Mon, 3 Apr 2023 11:17:52 +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 09/13] coresight: Store in-connections as well as
out-connections
On 29/03/2023 12:53, James Clark wrote:
> This will allow CATU to get its associated ETR in a generic way where
> currently the enable path has some hard coded searches which avoid
> the need to store input connections.
>
> This also means that the full search for connected devices on removal
> can be replaced with a loop through only the input and output devices.
>
> Signed-off-by: James Clark <james.clark@....com>
> ---
> drivers/hwtracing/coresight/coresight-core.c | 79 ++++++++-----------
> .../hwtracing/coresight/coresight-platform.c | 31 +++++++-
> drivers/hwtracing/coresight/coresight-sysfs.c | 7 --
> include/linux/coresight.h | 26 ++++++
> 4 files changed, 90 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 2f4aa15ef8f9..be1e8be2459f 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1349,6 +1349,16 @@ static int coresight_orphan_match(struct device *dev, void *data)
> if (ret)
> return ret;
>
> + /*
> + * Install the device connection. This also indicates that
> + * the links are operational on both ends.
> + */
> + conn->dest_dev = csdev;
> + conn->src_dev = i_csdev;
> +
> + ret = coresight_add_in_conn(conn);
> + if (ret)
> + return ret;
Do we need to clean up this "conn" in case of an error here ?
> } else {
> /* This component still has an orphan */
> still_orphan = true;
> @@ -1370,58 +1380,36 @@ static int coresight_fixup_orphan_conns(struct coresight_device *csdev)
> csdev, coresight_orphan_match);
> }
>
> -static int coresight_remove_match(struct device *dev, void *data)
> +/* coresight_remove_conns - Remove other device's references to this device */
> +static void coresight_remove_conns(struct coresight_device *csdev)
> {
> - int i;
> - struct coresight_device *csdev, *iterator;
> + int i, j;
> struct coresight_connection *conn;
>
> - csdev = data;
> - iterator = to_coresight_device(dev);
> -
> - /* No need to check oneself */
> - if (csdev == iterator)
> - return 0;
> + /* Remove input references on output devices */
minor nit: The term "output devices" is a bit confusing.
Could we simplify it as :
/*
* Remove the input connection references from the
* destination device for each output connection.
*/
> + for (i = 0; i < csdev->pdata->nr_outconns; i++) {
> + conn = csdev->pdata->out_conns[i];
> + if (!conn->dest_dev)
> + continue;
>
> - /*
> - * Circle throuch all the connection of that component. If we find
> - * a connection whose name matches @csdev, remove it.
> - */
> - for (i = 0; i < iterator->pdata->nr_outconns; i++) {
> - conn = iterator->pdata->out_conns[i];
> -
> - /* 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;
> - }
> + for (j = 0; j < conn->dest_dev->pdata->nr_inconns; ++j)
> + if (conn->dest_dev->pdata->in_conns[j] == conn) {
> + conn->dest_dev->pdata->in_conns[j] = NULL;
> + break;
> + }
> }
>
> - /*
> - * Returning '0' ensures that all known component on the
> - * bus will be checked.
> - */
> - return 0;
> -}
> + /* Remove output connections on input devices */
Similarly here :
/*
* For all input connections, remove references in
* the output connection.
*/
> + for (i = 0; i < csdev->pdata->nr_inconns; ++i) {
> + conn = csdev->pdata->in_conns[i];
> + /* Input conns array is sparse */
> + if (!conn)
> + continue;
>
> -/*
> - * coresight_remove_conns - Remove references to this given devices
> - * from the connections of other devices.
> - */
> -static void coresight_remove_conns(struct coresight_device *csdev)
> -{
> - /*
> - * Another device will point to this device only if there is
> - * an output port connected to this one. i.e, if the device
> - * doesn't have at least one input port, there is no point
> - * in searching all the devices.
> - */
> - if (csdev->pdata->high_inport)
> - bus_for_each_dev(&coresight_bustype, NULL,
> - csdev, coresight_remove_match);
> + conn->src_dev->orphan = true;
> + coresight_remove_links(conn->src_dev, conn);
> + conn->dest_dev = NULL;
> + }
> }
>
> /**
> @@ -1531,6 +1519,7 @@ void coresight_release_platform_data(struct coresight_device *csdev,
> devm_kfree(dev, conns[i]);
> }
> devm_kfree(dev, pdata->out_conns);
> + devm_kfree(dev, pdata->in_conns);
As mentioned earlier, this is not required.
> devm_kfree(dev, pdata);
> if (csdev)
> coresight_remove_conns_sysfs_group(csdev);
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> index bea8f1ba309a..59583df2dc44 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -60,6 +60,35 @@ int coresight_add_out_conn(struct device *dev,
> }
> EXPORT_SYMBOL_GPL(coresight_add_out_conn);
>
> +/*
> + * Add an input connection reference to @out_conn in the target's in_conns array
> + *
> + * @out_conn: Existing output connection to store as an input on the
> + * connection's remote device.
> + */
> +int coresight_add_in_conn(struct coresight_connection *out_conn)
> +{
> + int i;
> + struct device *dev = out_conn->dest_dev->dev.parent;
> + struct coresight_platform_data *pdata = out_conn->dest_dev->pdata;
> +
> + for (i = 0; i < pdata->nr_inconns; ++i)
> + if (!pdata->in_conns[i]) {
> + pdata->in_conns[i] = out_conn;
> + return 0;
> + }
> +
> + pdata->nr_inconns++;
> + pdata->in_conns =
> + devm_krealloc_array(dev, pdata->in_conns, pdata->nr_inconns,
> + sizeof(*pdata->in_conns), GFP_KERNEL);
> + if (!pdata->in_conns)
> + return -ENOMEM;
> + pdata->in_conns[pdata->nr_inconns - 1] = out_conn;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(coresight_add_in_conn);
> +
> static struct device *
> coresight_find_device_by_fwnode(struct fwnode_handle *fwnode)
> {
> @@ -230,7 +259,7 @@ static int of_coresight_get_cpu(struct device *dev)
>
> /*
> * of_coresight_parse_endpoint : Parse the given output endpoint @ep
> - * and fill the connection information in @conn
> + * and fill the connection information in @in_conn and @out_conn
We don't do anything about the in_conn here ? So this looks a bit
odd.
Rest looks good to me.
Suzuki
> *
> * Parses the local port, remote device name and the remote port.
> *
> diff --git a/drivers/hwtracing/coresight/coresight-sysfs.c b/drivers/hwtracing/coresight/coresight-sysfs.c
> index a4a8e8e642e8..464ba5e1343b 100644
> --- a/drivers/hwtracing/coresight/coresight-sysfs.c
> +++ b/drivers/hwtracing/coresight/coresight-sysfs.c
> @@ -173,12 +173,6 @@ int coresight_make_links(struct coresight_device *orig,
> break;
>
> conn->link = link;
> -
> - /*
> - * Install the device connection. This also indicates that
> - * the links are operational on both ends.
> - */
> - conn->dest_dev = target;
> return 0;
> } while (0);
>
> @@ -202,5 +196,4 @@ void coresight_remove_links(struct coresight_device *orig,
> devm_kfree(&orig->dev, conn->link->orig_name);
> devm_kfree(&orig->dev, conn->link);
> conn->link = NULL;
> - conn->dest_dev = NULL;
> }
> diff --git a/include/linux/coresight.h b/include/linux/coresight.h
> index 7197b07deede..aa36680fd264 100644
> --- a/include/linux/coresight.h
> +++ b/include/linux/coresight.h
> @@ -105,6 +105,9 @@ union coresight_dev_subtype {
> * @nr_inconns: Number of elements for the input connections.
> * @nr_outconns: Number of elements for the output connections.
> * @out_conns: Array of nr_outconns connections from this component.
> + * @in_conns: Sparse array of in_conns. Sparse because the source
> + * device owns the connection so when it's unloaded the
> + * connection leaves an empty slot.
> */
> struct coresight_platform_data {
> int high_inport;
> @@ -112,6 +115,7 @@ struct coresight_platform_data {
> int nr_inconns;
> int nr_outconns;
> struct coresight_connection **out_conns;
> + struct coresight_connection **in_conns;
> };
>
> /**
> @@ -172,6 +176,26 @@ struct coresight_desc {
> * @dest_dev: a @coresight_device representation of the component
> connected to @src_port. NULL until the device is created
> * @link: Representation of the connection as a sysfs link.
> + *
> + * The full connection structure looks like this, where in_conns store
> + * references to same connection as the source device's out_conns.
> + *
> + * +-----------------------------+ +-----------------------------+
> + * |coresight_device | |coresight_connection |
> + * |-----------------------------| |-----------------------------|
> + * | | | |
> + * | | | remote_dev*|<--
> + * |pdata->out_conns[nr_outconns]|<->|src_dev* | |
> + * | | | | |
> + * +-----------------------------+ +-----------------------------+ |
> + * |
> + * +-----------------------------+ |
> + * |coresight_device | |
> + * |------------------------------ |
> + * | | |
> + * | pdata->in_conns[nr_inconns]|<--
> + * | |
> + * +-----------------------------+
> */
> struct coresight_connection {
> int src_port;
> @@ -179,6 +203,7 @@ struct coresight_connection {
> struct fwnode_handle *dest_fwnode;
> struct coresight_device *dest_dev;
> struct coresight_sysfs_link *link;
> + struct coresight_device *src_dev;
> };
>
> /**
> @@ -614,5 +639,6 @@ struct coresight_platform_data *coresight_get_platform_data(struct device *dev);
> int coresight_add_out_conn(struct device *dev,
> struct coresight_platform_data *pdata,
> const struct coresight_connection *new_conn);
> +int coresight_add_in_conn(struct coresight_connection *conn);
>
> #endif /* _LINUX_COREISGHT_H */
Powered by blists - more mailing lists