On Thu 28 Nov 06:18 PST 2019, Georgi Djakov wrote:
> When debugging interconnect things, it turned out that saving the path
> name and including it in the traces is quite useful, especially for
> devices with multiple paths.
>
> For the path name we use the one specified in DT, or if we use platform
> data, the name is based on the source and destination node names.
>
> Suggested-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> Signed-off-by: Georgi Djakov <georgi.djakov@xxxxxxxxxx>
> ---
> drivers/interconnect/core.c | 18 +++++++++++++++---
> drivers/interconnect/internal.h | 2 ++
> 2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index f30a326dc7ce..c9e16bc1331e 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -356,9 +356,17 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
>
> mutex_lock(&icc_lock);
> path = path_find(dev, src_node, dst_node);
> - if (IS_ERR(path))
> - dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
> mutex_unlock(&icc_lock);
> + if (IS_ERR(path)) {
> + dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
> + return path;
> + }
> +
> + if (name)
> + path->name = kstrdup(name, GFP_KERNEL);
path->name is declared as const and name is likely to be rodata, so
using kstrdup_const() here instead have a good chance of avoiding an
unnecessary allocation.
> + else
> + path->name = kasprintf(GFP_KERNEL, "%s-%s",
> + src_node->name, dst_node->name);
>
> return path;
> }
> @@ -481,9 +489,12 @@ struct icc_path *icc_get(struct device *dev, const int src_id, const int dst_id)
> goto out;
>
> path = path_find(dev, src, dst);
> - if (IS_ERR(path))
> + if (IS_ERR(path)) {
> dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
> + goto out;
> + }
>
> + path->name = kasprintf(GFP_KERNEL, "%s-%s", src->name, dst->name);
> out:
> mutex_unlock(&icc_lock);
> return path;
> @@ -519,6 +530,7 @@ void icc_put(struct icc_path *path)
> }
> mutex_unlock(&icc_lock);
>
> + kfree(path->name);
And then kfree_const() here (which will handle both the rodata and
dynamically allocated case).
Apart from this I think the patch looks good.
Reviewed-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
Regards,
Bjorn
> kfree(path);
> }
> EXPORT_SYMBOL_GPL(icc_put);
> diff --git a/drivers/interconnect/internal.h b/drivers/interconnect/internal.h
> index 5853e8faf223..bf18cb7239df 100644
> --- a/drivers/interconnect/internal.h
> +++ b/drivers/interconnect/internal.h
> @@ -29,10 +29,12 @@ struct icc_req {
>
> /**
> * struct icc_path - interconnect path structure
> + * @name: a string name of the path (useful for ftrace)
> * @num_nodes: number of hops (nodes)
> * @reqs: array of the requests applicable to this path of nodes
> */
> struct icc_path {
> + const char *name;
> size_t num_nodes;
> struct icc_req reqs[];
> };
