Re: [PATCH] interconnect: Add helpers for enabling/disabling a path

From: Matthias Kaehlcke
Date: Tue Apr 28 2020 - 11:28:47 EST


Hi Georgi,

On Tue, Apr 28, 2020 at 12:16:50PM +0300, Georgi Djakov wrote:
> There is a repeated pattern in multiple drivers where they want to switch
> the bandwidth between zero and some other value. This is happening often
> in the suspend/resume callbacks. Let's add helper functions to enable and
> disable the path, so that callers don't have to take care of remembering
> the bandwidth values and handle this in the framework instead.
>
> With this patch the users can call icc_disable() and icc_enable() to lower
> their bandwidth request to zero and then restore it back to it's previous
> value.
>
> Suggested-by: Evan Green <evgreen@xxxxxxxxxxxx>
> Suggested-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> Signed-off-by: Georgi Djakov <georgi.djakov@xxxxxxxxxx>
> ---
> drivers/interconnect/core.c | 50 ++++++++++++++++++++++++++++++++-
> drivers/interconnect/internal.h | 2 ++
> include/linux/interconnect.h | 12 ++++++++
> 3 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 2c6515e3ecf1..6af68e506ac2 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -158,6 +158,7 @@ static struct icc_path *path_init(struct device *dev, struct icc_node *dst,
> hlist_add_head(&path->reqs[i].req_node, &node->req_list);
> path->reqs[i].node = node;
> path->reqs[i].dev = dev;
> + path->reqs[i].enabled = true;
> /* reference to previous node was saved during path traversal */
> node = node->reverse;
> }
> @@ -249,9 +250,12 @@ static int aggregate_requests(struct icc_node *node)
> if (p->pre_aggregate)
> p->pre_aggregate(node);
>
> - hlist_for_each_entry(r, &node->req_list, req_node)
> + hlist_for_each_entry(r, &node->req_list, req_node) {
> + if (!r->enabled)
> + continue;
> p->aggregate(node, r->tag, r->avg_bw, r->peak_bw,
> &node->avg_bw, &node->peak_bw);
> + }
>
> return 0;
> }
> @@ -546,6 +550,50 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> }
> EXPORT_SYMBOL_GPL(icc_set_bw);
>
> +int icc_disable(struct icc_path *path)
> +{
> + int i;
> +
> + if (!path)
> + return 0;
> +
> + if (WARN_ON(IS_ERR(path) || !path->num_nodes))
> + return -EINVAL;
> +
> + mutex_lock(&icc_lock);
> +
> + for (i = 0; i < path->num_nodes; i++)
> + path->reqs[i].enabled = false;
> +
> + mutex_unlock(&icc_lock);
> +
> + return icc_set_bw(path, path->reqs[0].avg_bw,
> + path->reqs[0].peak_bw);
> +}
> +EXPORT_SYMBOL_GPL(icc_disable);
> +
> +int icc_enable(struct icc_path *path)
> +{
> + int i;
> +
> + if (!path)
> + return 0;
> +
> + if (WARN_ON(IS_ERR(path) || !path->num_nodes))
> + return -EINVAL;
> +
> + mutex_lock(&icc_lock);
> +
> + for (i = 0; i < path->num_nodes; i++)
> + path->reqs[i].enabled = true;
> +
> + mutex_unlock(&icc_lock);
> +
> + return icc_set_bw(path, path->reqs[0].avg_bw,
> + path->reqs[0].peak_bw);
> +}

The two functions are identical except for the assignment of the 'enabled'
flags. You could add a helper _icc_enable(struct icc_path *path, bool enable)
and call it from icc_enable/disable().