[PATCH RESEND v2] interconnect: drop unused icc_get() interface

[PATCH RESEND v2] interconnect: drop unused icc_get() interface

[Johan Hovold]

The icc_get() interface can be used to lookup an interconnect path based
on global node ids. There has never been any users of this interface and
all lookups are currently done from the devicetree.

Remove the unused icc_get() interface.

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---

Hi Georgi,

I just noticed that this patch never made it into 6.4 along with the
rest of the series:

	https://lore.kernel.org/lkml/20230306075651.2449-23-johan+linaro@kernel.org/

This interface is still unused in mainline and should be removed so
resending the patch again.

Johan


 drivers/interconnect/core.c  | 52 ++----------------------------------
 include/linux/interconnect.h |  8 ------
 2 files changed, 2 insertions(+), 58 deletions(-)

diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index ec46bcb16d5e..5fac448c28fd 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -587,7 +587,7 @@ EXPORT_SYMBOL_GPL(icc_set_tag);
 
 /**
  * icc_get_name() - Get name of the icc path
- * @path: reference to the path returned by icc_get()
+ * @path: interconnect path
  *
  * This function is used by an interconnect consumer to get the name of the icc
  * path.
@@ -605,7 +605,7 @@ EXPORT_SYMBOL_GPL(icc_get_name);
 
 /**
  * icc_set_bw() - set bandwidth constraints on an interconnect path
- * @path: reference to the path returned by icc_get()
+ * @path: interconnect path
  * @avg_bw: average bandwidth in kilobytes per second
  * @peak_bw: peak bandwidth in kilobytes per second
  *
@@ -704,54 +704,6 @@ int icc_disable(struct icc_path *path)
 }
 EXPORT_SYMBOL_GPL(icc_disable);
 
-/**
- * icc_get() - return a handle for path between two endpoints
- * @dev: the device requesting the path
- * @src_id: source device port id
- * @dst_id: destination device port id
- *
- * This function will search for a path between two endpoints and return an
- * icc_path handle on success. Use icc_put() to release
- * constraints when they are not needed anymore.
- * If the interconnect API is disabled, NULL is returned and the consumer
- * drivers will still build. Drivers are free to handle this specifically,
- * but they don't have to.
- *
- * Return: icc_path pointer on success, ERR_PTR() on error or NULL if the
- * interconnect API is disabled.
- */
-struct icc_path *icc_get(struct device *dev, const int src_id, const int dst_id)
-{
-	struct icc_node *src, *dst;
-	struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
-
-	mutex_lock(&icc_lock);
-
-	src = node_find(src_id);
-	if (!src)
-		goto out;
-
-	dst = node_find(dst_id);
-	if (!dst)
-		goto out;
-
-	path = path_find(dev, src, dst);
-	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);
-	if (!path->name) {
-		kfree(path);
-		path = ERR_PTR(-ENOMEM);
-	}
-out:
-	mutex_unlock(&icc_lock);
-	return path;
-}
-EXPORT_SYMBOL_GPL(icc_get);
-
 /**
  * icc_put() - release the reference to the icc_path
  * @path: interconnect path
diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
index 2b0e784ba771..97ac253df62c 100644
--- a/include/linux/interconnect.h
+++ b/include/linux/interconnect.h
@@ -40,8 +40,6 @@ struct icc_bulk_data {
 
 #if IS_ENABLED(CONFIG_INTERCONNECT)
 
-struct icc_path *icc_get(struct device *dev, const int src_id,
-			 const int dst_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);
 int devm_of_icc_bulk_get(struct device *dev, int num_paths, struct icc_bulk_data *paths);
@@ -61,12 +59,6 @@ void icc_bulk_disable(int num_paths, const struct icc_bulk_data *paths);
 
 #else
 
-static inline struct icc_path *icc_get(struct device *dev, const int src_id,
-				       const int dst_id)
-{
-	return NULL;
-}
-
 static inline struct icc_path *of_icc_get(struct device *dev,
 					  const char *name)
 {
-- 
2.39.3

Re: [PATCH RESEND v2] interconnect: drop unused icc_get() interface

[Bjorn Andersson]

On Tue, May 23, 2023 at 11:52:48AM +0200, Johan Hovold wrote:
> The icc_get() interface can be used to lookup an interconnect path based
> on global node ids. There has never been any users of this interface and
> all lookups are currently done from the devicetree.
> 
> Remove the unused icc_get() interface.
> 

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

Regards,
Bjorn

> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
> 
> Hi Georgi,
> 
> I just noticed that this patch never made it into 6.4 along with the
> rest of the series:
> 
> 	https://lore.kernel.org/lkml/20230306075651.2449-23-johan+linaro@kernel.org/
> 
> This interface is still unused in mainline and should be removed so
> resending the patch again.
> 
> Johan
> 
> 
>  drivers/interconnect/core.c  | 52 ++----------------------------------
>  include/linux/interconnect.h |  8 ------
>  2 files changed, 2 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index ec46bcb16d5e..5fac448c28fd 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -587,7 +587,7 @@ EXPORT_SYMBOL_GPL(icc_set_tag);
>  
>  /**
>   * icc_get_name() - Get name of the icc path
> - * @path: reference to the path returned by icc_get()
> + * @path: interconnect path
>   *
>   * This function is used by an interconnect consumer to get the name of the icc
>   * path.
> @@ -605,7 +605,7 @@ EXPORT_SYMBOL_GPL(icc_get_name);
>  
>  /**
>   * icc_set_bw() - set bandwidth constraints on an interconnect path
> - * @path: reference to the path returned by icc_get()
> + * @path: interconnect path
>   * @avg_bw: average bandwidth in kilobytes per second
>   * @peak_bw: peak bandwidth in kilobytes per second
>   *
> @@ -704,54 +704,6 @@ int icc_disable(struct icc_path *path)
>  }
>  EXPORT_SYMBOL_GPL(icc_disable);
>  
> -/**
> - * icc_get() - return a handle for path between two endpoints
> - * @dev: the device requesting the path
> - * @src_id: source device port id
> - * @dst_id: destination device port id
> - *
> - * This function will search for a path between two endpoints and return an
> - * icc_path handle on success. Use icc_put() to release
> - * constraints when they are not needed anymore.
> - * If the interconnect API is disabled, NULL is returned and the consumer
> - * drivers will still build. Drivers are free to handle this specifically,
> - * but they don't have to.
> - *
> - * Return: icc_path pointer on success, ERR_PTR() on error or NULL if the
> - * interconnect API is disabled.
> - */
> -struct icc_path *icc_get(struct device *dev, const int src_id, const int dst_id)
> -{
> -	struct icc_node *src, *dst;
> -	struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
> -
> -	mutex_lock(&icc_lock);
> -
> -	src = node_find(src_id);
> -	if (!src)
> -		goto out;
> -
> -	dst = node_find(dst_id);
> -	if (!dst)
> -		goto out;
> -
> -	path = path_find(dev, src, dst);
> -	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);
> -	if (!path->name) {
> -		kfree(path);
> -		path = ERR_PTR(-ENOMEM);
> -	}
> -out:
> -	mutex_unlock(&icc_lock);
> -	return path;
> -}
> -EXPORT_SYMBOL_GPL(icc_get);
> -
>  /**
>   * icc_put() - release the reference to the icc_path
>   * @path: interconnect path
> diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
> index 2b0e784ba771..97ac253df62c 100644
> --- a/include/linux/interconnect.h
> +++ b/include/linux/interconnect.h
> @@ -40,8 +40,6 @@ struct icc_bulk_data {
>  
>  #if IS_ENABLED(CONFIG_INTERCONNECT)
>  
> -struct icc_path *icc_get(struct device *dev, const int src_id,
> -			 const int dst_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);
>  int devm_of_icc_bulk_get(struct device *dev, int num_paths, struct icc_bulk_data *paths);
> @@ -61,12 +59,6 @@ void icc_bulk_disable(int num_paths, const struct icc_bulk_data *paths);
>  
>  #else
>  
> -static inline struct icc_path *icc_get(struct device *dev, const int src_id,
> -				       const int dst_id)
> -{
> -	return NULL;
> -}
> -
>  static inline struct icc_path *of_icc_get(struct device *dev,
>  					  const char *name)
>  {
> -- 
> 2.39.3
> 

Re: [PATCH RESEND v2] interconnect: drop unused icc_get() interface

[Mike Tipton]

On 5/23/2023 2:52 AM, Johan Hovold wrote:
> The icc_get() interface can be used to lookup an interconnect path based
> on global node ids. There has never been any users of this interface and
> all lookups are currently done from the devicetree.
> 
> Remove the unused icc_get() interface.
> 
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
> 
> Hi Georgi,
> 
> I just noticed that this patch never made it into 6.4 along with the
> rest of the series:
> 
> 	https://lore.kernel.org/lkml/20230306075651.2449-23-johan+linaro@kernel.org/
> 
> This interface is still unused in mainline and should be removed so
> resending the patch again.
> 
> Johan
> 
> 
>   drivers/interconnect/core.c  | 52 ++----------------------------------
>   include/linux/interconnect.h |  8 ------
>   2 files changed, 2 insertions(+), 58 deletions(-)
> 

We have downstream debug/test modules that removing icc_get() will 
break. I'd like to get equivalent debug support in mainline, but until 
then I'd prefer we not remove this.

Our debug module adds debugfs files for requesting a path and voting BW 
to it. The path is defined by supplying the raw src_id / dst_id and 
calling icc_get() for them. This allows us to issue any request to any 
path from debugfs without needing to recompile or reflash devicetree. We 
use this extensively.

That said, I don't like the current implementation of icc_get(). It 
should take strings for src/dst rather than integer IDs. This would be 
similar to other interfaces like clk_get() and regulator_get(). And 
would allow users to specify their path in debugfs using logical names 
rather than raw integers.

I suspect having a mainline approach for voting paths from debugfs would 
be useful to others as well. There are similar debugfs control 
mechanisms in other frameworks already, e.g. clock.

Instead of removing icc_get() immediately, can we wait for a future 
patch series that adds debugfs as a consumer?

Re: [PATCH RESEND v2] interconnect: drop unused icc_get() interface

[Johan Hovold]

On Tue, Jun 20, 2023 at 02:57:17PM -0700, Mike Tipton wrote:
> On 5/23/2023 2:52 AM, Johan Hovold wrote:
> > The icc_get() interface can be used to lookup an interconnect path based
> > on global node ids. There has never been any users of this interface and
> > all lookups are currently done from the devicetree.
> > 
> > Remove the unused icc_get() interface.
> > 
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

> We have downstream debug/test modules that removing icc_get() will 
> break. I'd like to get equivalent debug support in mainline, but until 
> then I'd prefer we not remove this.

I'm sure you've heard this before, but if it's not in mainline it does
not count. We don't carry code upstream for the sole benefit of
out-of-tree users.

> I suspect having a mainline approach for voting paths from debugfs would 
> be useful to others as well. There are similar debugfs control 
> mechanisms in other frameworks already, e.g. clock.
> 
> Instead of removing icc_get() immediately, can we wait for a future 
> patch series that adds debugfs as a consumer?

This function was merged over four years ago and has never been used in
mainline and I doubt a user will suddenly show up in the near future if
we were to keep it.

I guess you can just carry one more patch out-of-tree until you can
mainline a proper debugfs interface (which should probably not even use
this function, as you mentioned yourself).

Johan

Re: [PATCH RESEND v2] interconnect: drop unused icc_get() interface

[Mike Tipton]

On 6/21/2023 1:34 AM, Johan Hovold wrote:
> On Tue, Jun 20, 2023 at 02:57:17PM -0700, Mike Tipton wrote:
>> On 5/23/2023 2:52 AM, Johan Hovold wrote:
>>> The icc_get() interface can be used to lookup an interconnect path based
>>> on global node ids. There has never been any users of this interface and
>>> all lookups are currently done from the devicetree.
>>>
>>> Remove the unused icc_get() interface.
>>>
>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
>> We have downstream debug/test modules that removing icc_get() will
>> break. I'd like to get equivalent debug support in mainline, but until
>> then I'd prefer we not remove this.
> 
> I'm sure you've heard this before, but if it's not in mainline it does
> not count. We don't carry code upstream for the sole benefit of
> out-of-tree users.

Yeah, I understand that in general.

> 
>> I suspect having a mainline approach for voting paths from debugfs would
>> be useful to others as well. There are similar debugfs control
>> mechanisms in other frameworks already, e.g. clock.
>>
>> Instead of removing icc_get() immediately, can we wait for a future
>> patch series that adds debugfs as a consumer?
> 
> This function was merged over four years ago and has never been used in
> mainline and I doubt a user will suddenly show up in the near future if
> we were to keep it.

I guess I'm hoping if it's already been unused in mainline for four 
years, that we can keep it a little longer until the mainline debugfs 
user can be added.

We can prepare that series. Not entirely sure when it'll be ready, but 
can try to prioritize it.

> 
> I guess you can just carry one more patch out-of-tree until you can
> mainline a proper debugfs interface (which should probably not even use
> this function, as you mentioned yourself).
That's not really possible in this case. We're limited to our DLKMs 
running on top of the Android kernel. We can't modify the interconnect 
framework itself.

> 
> Johan