Re: [PATCH v4 2/3] interconnect: Add a name to struct icc_path

[Bjorn Andersson <bjorn.andersson@linaro.org>]

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@linaro.org>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
>  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@linaro.org>

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[];
>  };