linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] interconnect: Add helpers for enabling/disabling a path
@ 2020-05-07 12:08 Georgi Djakov
  2020-05-07 15:57 ` Matthias Kaehlcke
  2020-05-12 21:59 ` Bjorn Andersson
  0 siblings, 2 replies; 3+ messages in thread
From: Georgi Djakov @ 2020-05-07 12:08 UTC (permalink / raw)
  To: linux-pm
  Cc: evgreen, bjorn.andersson, mka, akashast, linux-kernel, georgi.djakov

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>
---
v2: https://lore.kernel.org/r/20200428091650.27669-1-georgi.djakov@linaro.org/
* Extract the common code into __icc_enable() (Matthias)


 drivers/interconnect/core.c     | 39 ++++++++++++++++++++++++++++++++-
 drivers/interconnect/internal.h |  2 ++
 include/linux/interconnect.h    | 12 ++++++++++
 3 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index f5699ed34e43..d5e0f93c942d 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;
 }
@@ -571,6 +575,39 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
 }
 EXPORT_SYMBOL_GPL(icc_set_bw);
 
+static int __icc_enable(struct icc_path *path, bool enable)
+{
+	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 = enable;
+
+	mutex_unlock(&icc_lock);
+
+	return icc_set_bw(path, path->reqs[0].avg_bw,
+			  path->reqs[0].peak_bw);
+}
+
+int icc_disable(struct icc_path *path)
+{
+	return __icc_enable(path, false);
+}
+EXPORT_SYMBOL_GPL(icc_disable);
+
+int icc_enable(struct icc_path *path)
+{
+	return __icc_enable(path, true);
+}
+EXPORT_SYMBOL_GPL(icc_enable);
+
 /**
  * icc_get() - return a handle for path between two endpoints
  * @dev: the device requesting the path
diff --git a/drivers/interconnect/internal.h b/drivers/interconnect/internal.h
index bf18cb7239df..f5f82a5c939e 100644
--- a/drivers/interconnect/internal.h
+++ b/drivers/interconnect/internal.h
@@ -14,6 +14,7 @@
  * @req_node: entry in list of requests for the particular @node
  * @node: the interconnect node to which this constraint applies
  * @dev: reference to the device that sets the constraints
+ * @enabled: indicates whether the path with this request is enabled
  * @tag: path tag (optional)
  * @avg_bw: an integer describing the average bandwidth in kBps
  * @peak_bw: an integer describing the peak bandwidth in kBps
@@ -22,6 +23,7 @@ struct icc_req {
 	struct hlist_node req_node;
 	struct icc_node *node;
 	struct device *dev;
+	bool enabled;
 	u32 tag;
 	u32 avg_bw;
 	u32 peak_bw;
diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
index 770692421f4c..2b7b331c9354 100644
--- a/include/linux/interconnect.h
+++ b/include/linux/interconnect.h
@@ -30,6 +30,8 @@ struct icc_path *icc_get(struct device *dev, const int src_id,
 struct icc_path *of_icc_get(struct device *dev, const char *name);
 struct icc_path *devm_of_icc_get(struct device *dev, const char *name);
 void icc_put(struct icc_path *path);
+int icc_disable(struct icc_path *path);
+int icc_enable(struct icc_path *path);
 int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
 void icc_set_tag(struct icc_path *path, u32 tag);
 
@@ -57,6 +59,16 @@ static inline void icc_put(struct icc_path *path)
 {
 }
 
+static inline int icc_disable(struct icc_path *path)
+{
+	return 0;
+}
+
+static inline int icc_enable(struct icc_path *path)
+{
+	return 0;
+}
+
 static inline int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
 {
 	return 0;

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] interconnect: Add helpers for enabling/disabling a path
  2020-05-07 12:08 [PATCH v2] interconnect: Add helpers for enabling/disabling a path Georgi Djakov
@ 2020-05-07 15:57 ` Matthias Kaehlcke
  2020-05-12 21:59 ` Bjorn Andersson
  1 sibling, 0 replies; 3+ messages in thread
From: Matthias Kaehlcke @ 2020-05-07 15:57 UTC (permalink / raw)
  To: Georgi Djakov; +Cc: linux-pm, evgreen, bjorn.andersson, akashast, linux-kernel

On Thu, May 07, 2020 at 03:08:46PM +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>
> ---
> v2: https://lore.kernel.org/r/20200428091650.27669-1-georgi.djakov@linaro.org/
> * Extract the common code into __icc_enable() (Matthias)
> 
> 
>  drivers/interconnect/core.c     | 39 ++++++++++++++++++++++++++++++++-
>  drivers/interconnect/internal.h |  2 ++
>  include/linux/interconnect.h    | 12 ++++++++++
>  3 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index f5699ed34e43..d5e0f93c942d 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;
>  }
> @@ -571,6 +575,39 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
>  }
>  EXPORT_SYMBOL_GPL(icc_set_bw);
>  
> +static int __icc_enable(struct icc_path *path, bool enable)
> +{
> +	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 = enable;
> +
> +	mutex_unlock(&icc_lock);
> +
> +	return icc_set_bw(path, path->reqs[0].avg_bw,
> +			  path->reqs[0].peak_bw);
> +}
> +
> +int icc_disable(struct icc_path *path)
> +{
> +	return __icc_enable(path, false);
> +}
> +EXPORT_SYMBOL_GPL(icc_disable);
> +
> +int icc_enable(struct icc_path *path)
> +{
> +	return __icc_enable(path, true);
> +}
> +EXPORT_SYMBOL_GPL(icc_enable);
> +

uber-nit: my brain expects the order enable/disable, just like lock/unlock
or true/false instead of vice-versa, but it's certainly not really
important :)

>  /**
>   * icc_get() - return a handle for path between two endpoints
>   * @dev: the device requesting the path
> diff --git a/drivers/interconnect/internal.h b/drivers/interconnect/internal.h
> index bf18cb7239df..f5f82a5c939e 100644
> --- a/drivers/interconnect/internal.h
> +++ b/drivers/interconnect/internal.h
> @@ -14,6 +14,7 @@
>   * @req_node: entry in list of requests for the particular @node
>   * @node: the interconnect node to which this constraint applies
>   * @dev: reference to the device that sets the constraints
> + * @enabled: indicates whether the path with this request is enabled
>   * @tag: path tag (optional)
>   * @avg_bw: an integer describing the average bandwidth in kBps
>   * @peak_bw: an integer describing the peak bandwidth in kBps
> @@ -22,6 +23,7 @@ struct icc_req {
>  	struct hlist_node req_node;
>  	struct icc_node *node;
>  	struct device *dev;
> +	bool enabled;
>  	u32 tag;
>  	u32 avg_bw;
>  	u32 peak_bw;
> diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
> index 770692421f4c..2b7b331c9354 100644
> --- a/include/linux/interconnect.h
> +++ b/include/linux/interconnect.h
> @@ -30,6 +30,8 @@ struct icc_path *icc_get(struct device *dev, const int src_id,
>  struct icc_path *of_icc_get(struct device *dev, const char *name);
>  struct icc_path *devm_of_icc_get(struct device *dev, const char *name);
>  void icc_put(struct icc_path *path);
> +int icc_disable(struct icc_path *path);
> +int icc_enable(struct icc_path *path);
>  int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
>  void icc_set_tag(struct icc_path *path, u32 tag);
>  
> @@ -57,6 +59,16 @@ static inline void icc_put(struct icc_path *path)
>  {
>  }
>  
> +static inline int icc_disable(struct icc_path *path)
> +{
> +	return 0;
> +}
> +
> +static inline int icc_enable(struct icc_path *path)
> +{
> +	return 0;
> +}
> +
>  static inline int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
>  {
>  	return 0;

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] interconnect: Add helpers for enabling/disabling a path
  2020-05-07 12:08 [PATCH v2] interconnect: Add helpers for enabling/disabling a path Georgi Djakov
  2020-05-07 15:57 ` Matthias Kaehlcke
@ 2020-05-12 21:59 ` Bjorn Andersson
  1 sibling, 0 replies; 3+ messages in thread
From: Bjorn Andersson @ 2020-05-12 21:59 UTC (permalink / raw)
  To: Georgi Djakov; +Cc: linux-pm, evgreen, mka, akashast, linux-kernel

On Thu 07 May 05:08 PDT 2020, 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>

Nice

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
> v2: https://lore.kernel.org/r/20200428091650.27669-1-georgi.djakov@linaro.org/
> * Extract the common code into __icc_enable() (Matthias)
> 
> 
>  drivers/interconnect/core.c     | 39 ++++++++++++++++++++++++++++++++-
>  drivers/interconnect/internal.h |  2 ++
>  include/linux/interconnect.h    | 12 ++++++++++
>  3 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index f5699ed34e43..d5e0f93c942d 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;
>  }
> @@ -571,6 +575,39 @@ int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
>  }
>  EXPORT_SYMBOL_GPL(icc_set_bw);
>  
> +static int __icc_enable(struct icc_path *path, bool enable)
> +{
> +	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 = enable;
> +
> +	mutex_unlock(&icc_lock);
> +
> +	return icc_set_bw(path, path->reqs[0].avg_bw,
> +			  path->reqs[0].peak_bw);
> +}
> +
> +int icc_disable(struct icc_path *path)
> +{
> +	return __icc_enable(path, false);
> +}
> +EXPORT_SYMBOL_GPL(icc_disable);
> +
> +int icc_enable(struct icc_path *path)
> +{
> +	return __icc_enable(path, true);
> +}
> +EXPORT_SYMBOL_GPL(icc_enable);
> +
>  /**
>   * icc_get() - return a handle for path between two endpoints
>   * @dev: the device requesting the path
> diff --git a/drivers/interconnect/internal.h b/drivers/interconnect/internal.h
> index bf18cb7239df..f5f82a5c939e 100644
> --- a/drivers/interconnect/internal.h
> +++ b/drivers/interconnect/internal.h
> @@ -14,6 +14,7 @@
>   * @req_node: entry in list of requests for the particular @node
>   * @node: the interconnect node to which this constraint applies
>   * @dev: reference to the device that sets the constraints
> + * @enabled: indicates whether the path with this request is enabled
>   * @tag: path tag (optional)
>   * @avg_bw: an integer describing the average bandwidth in kBps
>   * @peak_bw: an integer describing the peak bandwidth in kBps
> @@ -22,6 +23,7 @@ struct icc_req {
>  	struct hlist_node req_node;
>  	struct icc_node *node;
>  	struct device *dev;
> +	bool enabled;
>  	u32 tag;
>  	u32 avg_bw;
>  	u32 peak_bw;
> diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
> index 770692421f4c..2b7b331c9354 100644
> --- a/include/linux/interconnect.h
> +++ b/include/linux/interconnect.h
> @@ -30,6 +30,8 @@ struct icc_path *icc_get(struct device *dev, const int src_id,
>  struct icc_path *of_icc_get(struct device *dev, const char *name);
>  struct icc_path *devm_of_icc_get(struct device *dev, const char *name);
>  void icc_put(struct icc_path *path);
> +int icc_disable(struct icc_path *path);
> +int icc_enable(struct icc_path *path);
>  int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
>  void icc_set_tag(struct icc_path *path, u32 tag);
>  
> @@ -57,6 +59,16 @@ static inline void icc_put(struct icc_path *path)
>  {
>  }
>  
> +static inline int icc_disable(struct icc_path *path)
> +{
> +	return 0;
> +}
> +
> +static inline int icc_enable(struct icc_path *path)
> +{
> +	return 0;
> +}
> +
>  static inline int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
>  {
>  	return 0;

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-05-12 22:00 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-07 12:08 [PATCH v2] interconnect: Add helpers for enabling/disabling a path Georgi Djakov
2020-05-07 15:57 ` Matthias Kaehlcke
2020-05-12 21:59 ` Bjorn Andersson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).