Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 28 Apr 2020 08:28:42 -0700
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Georgi Djakov <georgi.djakov@linaro.org>
Cc:     linux-pm@vger.kernel.org, evgreen@chromium.org,
        bjorn.andersson@linaro.org, akashast@codeaurora.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] interconnect: Add helpers for enabling/disabling a path
Message-ID: <20200428152842.GG4525@google.com>
References: <20200428091650.27669-1-georgi.djakov@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200428091650.27669-1-georgi.djakov@linaro.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@chromium.org>
> Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
> ---
>  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().