This series introduces interconnect debugfs files that support voting
for any path the framework supports.
We've historically relied on an out-of-tree module for this, which used
the old icc_get() that was recently removed in [0]. The old icc_get()
took integer endpoint IDs, which made identifying paths in our old
implementation non-intuitive. The logical node names typically don't
change much chip-to-chip, but the raw integer IDs do. Take this
opportunity to introduce an icc_get() that uses string names instead,
which allows for a more intuitive and generic debugfs interface.
We rely on this support for debug, test, and verification. Hopefully
it'll be useful for other vendors as well.
[0] commit 7dcdad6f32c9 ("interconnect: drop unused icc_get() interface")
Mike Tipton (3):
debugfs: Add write support to debugfs_create_str()
interconnect: Reintroduce icc_get()
interconnect: Add debugfs test client
drivers/interconnect/Makefile | 2 +-
drivers/interconnect/core.c | 67 +++++++++++
drivers/interconnect/debugfs-client.c | 156 ++++++++++++++++++++++++++
drivers/interconnect/internal.h | 2 +
fs/debugfs/file.c | 48 +++++++-
include/linux/interconnect.h | 6 +
6 files changed, 278 insertions(+), 3 deletions(-)
create mode 100644 drivers/interconnect/debugfs-client.c
--
2.17.1
The original icc_get() that took integer node IDs was removed due to
lack of users. Reintroduce a new version that takes string node names,
which is needed for the debugfs client.
Signed-off-by: Mike Tipton <[email protected]>
---
drivers/interconnect/core.c | 64 ++++++++++++++++++++++++++++++++++++
include/linux/interconnect.h | 6 ++++
2 files changed, 70 insertions(+)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 5fac448c28fd..511152398790 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -147,6 +147,21 @@ static struct icc_node *node_find(const int id)
return idr_find(&icc_idr, id);
}
+static struct icc_node *node_find_by_name(const char *name)
+{
+ struct icc_provider *provider;
+ struct icc_node *n;
+
+ list_for_each_entry(provider, &icc_providers, provider_list) {
+ list_for_each_entry(n, &provider->nodes, node_list) {
+ if (!strcmp(n->name, name))
+ return n;
+ }
+ }
+
+ return NULL;
+}
+
static struct icc_path *path_init(struct device *dev, struct icc_node *dst,
ssize_t num_nodes)
{
@@ -561,6 +576,55 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
}
EXPORT_SYMBOL_GPL(of_icc_get);
+/**
+ * icc_get() - get a path handle between two endpoints
+ * @dev: device pointer for the consumer device
+ * @src: source node name
+ * @dst: destination node name
+ *
+ * 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.
+ *
+ * Return: icc_path pointer on success or ERR_PTR() on error. NULL is returned
+ * when the API is disabled.
+ */
+struct icc_path *icc_get(struct device *dev, const char *src, const char *dst)
+{
+ struct icc_node *src_node, *dst_node;
+ struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
+
+ mutex_lock(&icc_lock);
+
+ src_node = node_find_by_name(src);
+ if (!src_node) {
+ dev_err(dev, "%s: invalid src=%s\n", __func__, src);
+ goto out;
+ }
+
+ dst_node = node_find_by_name(dst);
+ if (!dst_node) {
+ dev_err(dev, "%s: invalid dst=%s\n", __func__, dst);
+ goto out;
+ }
+
+ path = path_find(dev, src_node, dst_node);
+ 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_node->name, dst_node->name);
+ if (!path->name) {
+ kfree(path);
+ path = ERR_PTR(-ENOMEM);
+ }
+out:
+ mutex_unlock(&icc_lock);
+ return path;
+}
+EXPORT_SYMBOL_GPL(icc_get);
+
/**
* icc_set_tag() - set an optional tag on a path
* @path: the path we want to tag
diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
index 97ac253df62c..39a98ddfdfd4 100644
--- a/include/linux/interconnect.h
+++ b/include/linux/interconnect.h
@@ -40,6 +40,7 @@ struct icc_bulk_data {
#if IS_ENABLED(CONFIG_INTERCONNECT)
+struct icc_path *icc_get(struct device *dev, const char *src, const char *dst);
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);
@@ -59,6 +60,11 @@ void icc_bulk_disable(int num_paths, const struct icc_bulk_data *paths);
#else
+struct icc_path *icc_get(struct device *dev, const char *src, const char *dst)
+{
+ return NULL;
+}
+
static inline struct icc_path *of_icc_get(struct device *dev,
const char *name)
{
--
2.17.1
Implementation taken from original patch series that added
debugfs_create_str() [0]. Write support was present in the initial patch
revisions, but was later removed due to lack of users. We have a user
now, so reintroduce it.
[0] https://lore.kernel.org/all/YF3Hv5zXb%[email protected]/
Signed-off-by: Mike Tipton <[email protected]>
---
fs/debugfs/file.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 46 insertions(+), 2 deletions(-)
diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
index b7711888dd17..87b3753aa4b1 100644
--- a/fs/debugfs/file.c
+++ b/fs/debugfs/file.c
@@ -904,8 +904,52 @@ EXPORT_SYMBOL_GPL(debugfs_create_str);
static ssize_t debugfs_write_file_str(struct file *file, const char __user *user_buf,
size_t count, loff_t *ppos)
{
- /* This is really only for read-only strings */
- return -EINVAL;
+ struct dentry *dentry = F_DENTRY(file);
+ char *old, *new = NULL;
+ int pos = *ppos;
+ int r;
+
+ r = debugfs_file_get(dentry);
+ if (unlikely(r))
+ return r;
+
+ old = *(char **)file->private_data;
+
+ /* only allow strict concatenation */
+ r = -EINVAL;
+ if (pos && pos != strlen(old))
+ goto error;
+
+ r = -E2BIG;
+ if (pos + count + 1 > PAGE_SIZE)
+ goto error;
+
+ r = -ENOMEM;
+ new = kmalloc(pos + count + 1, GFP_KERNEL);
+ if (!new)
+ goto error;
+
+ if (pos)
+ memcpy(new, old, pos);
+
+ r = -EFAULT;
+ if (copy_from_user(new + pos, user_buf, count))
+ goto error;
+
+ new[pos + count] = '\0';
+ strim(new);
+
+ rcu_assign_pointer(*(char **)file->private_data, new);
+ synchronize_rcu();
+ kfree(old);
+
+ debugfs_file_put(dentry);
+ return count;
+
+error:
+ kfree(new);
+ debugfs_file_put(dentry);
+ return r;
}
static const struct file_operations fops_str = {
--
2.17.1
It's often useful during test, debug, and development to issue path
votes from shell. Add a debugfs client for this purpose.
Example usage:
cd /sys/kernel/debug/interconnect/test-client/
# Configure node endpoints for the path from CPU to DDR on
# qcom/sm8550.
echo chm_apps > src_node
echo ebi > dst_node
# Get path between src_node and dst_node. This is only
# necessary after updating the node endpoints.
echo 1 > get
# Set desired BW to 1GBps avg and 2GBps peak.
echo 1000000 > avg_bw
echo 2000000 > peak_bw
# Vote for avg_bw and peak_bw on the latest path from "get".
# Voting for multiple paths is possible by repeating this
# process for different nodes endpoints.
echo 1 > commit
Allowing userspace to directly enable and set bus rates can be dangerous
So, following in the footsteps of the regmap [0] and clk [1] frameworks,
keep these userspace controls compile-time disabled without Kconfig
options to enable them. Enabling this will require code changes to
define INTERCONNECT_ALLOW_WRITE_DEBUGFS.
[0] commit 09c6ecd39410 ("regmap: Add support for writing to regmap registers via debugfs")
[1] commit 37215da5553e ("clk: Add support for setting clk_rate via debugfs")
Signed-off-by: Mike Tipton <[email protected]>
---
drivers/interconnect/Makefile | 2 +-
drivers/interconnect/core.c | 3 +
drivers/interconnect/debugfs-client.c | 156 ++++++++++++++++++++++++++
drivers/interconnect/internal.h | 2 +
4 files changed, 162 insertions(+), 1 deletion(-)
create mode 100644 drivers/interconnect/debugfs-client.c
diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
index 5604ce351a9f..d0888babb9a1 100644
--- a/drivers/interconnect/Makefile
+++ b/drivers/interconnect/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
CFLAGS_core.o := -I$(src)
-icc-core-objs := core.o bulk.o
+icc-core-objs := core.o bulk.o debugfs-client.o
obj-$(CONFIG_INTERCONNECT) += icc-core.o
obj-$(CONFIG_INTERCONNECT_IMX) += imx/
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 511152398790..bc82571ff2a8 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -1117,6 +1117,9 @@ static int __init icc_init(void)
icc_debugfs_dir, NULL, &icc_summary_fops);
debugfs_create_file("interconnect_graph", 0444,
icc_debugfs_dir, NULL, &icc_graph_fops);
+
+ icc_debugfs_client_init(icc_debugfs_dir);
+
return 0;
}
diff --git a/drivers/interconnect/debugfs-client.c b/drivers/interconnect/debugfs-client.c
new file mode 100644
index 000000000000..990dd2ff6df7
--- /dev/null
+++ b/drivers/interconnect/debugfs-client.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+#include <linux/debugfs.h>
+#include <linux/interconnect.h>
+#include <linux/platform_device.h>
+#include <linux/uaccess.h>
+
+/*
+ * This can be dangerous, therefore don't provide any real compile time
+ * configuration option for this feature.
+ * People who want to use this will need to modify the source code directly.
+ */
+#undef INTERCONNECT_ALLOW_WRITE_DEBUGFS
+
+#if defined(INTERCONNECT_ALLOW_WRITE_DEBUGFS) && defined(CONFIG_DEBUG_FS)
+
+static LIST_HEAD(debugfs_paths);
+static DEFINE_MUTEX(debugfs_lock);
+
+static struct platform_device *pdev;
+static struct icc_path *cur_path;
+
+static char *src_node;
+static char *dst_node;
+static u32 avg_bw;
+static u32 peak_bw;
+static u32 tag;
+
+struct debugfs_path {
+ const char *src;
+ const char *dst;
+ struct icc_path *path;
+ struct list_head list;
+};
+
+static struct icc_path *get_path(const char *src, const char *dst)
+{
+ struct debugfs_path *path;
+
+ list_for_each_entry(path, &debugfs_paths, list) {
+ if (!strcmp(path->src, src) && !strcmp(path->dst, dst))
+ return path->path;
+ }
+
+ return NULL;
+}
+
+static int icc_get_set(void *data, u64 val)
+{
+ struct debugfs_path *debugfs_path;
+ int ret = 0;
+
+ mutex_lock(&debugfs_lock);
+
+ /*
+ * If we've already looked up a path, then use the existing one instead
+ * of calling icc_get() again. This allows for updating previous BW
+ * votes when "get" is written to multiple times for multiple paths.
+ */
+ cur_path = get_path(src_node, dst_node);
+ if (cur_path)
+ goto out;
+
+ cur_path = icc_get(&pdev->dev, src_node, dst_node);
+ if (IS_ERR(cur_path)) {
+ ret = PTR_ERR(cur_path);
+ goto out;
+ }
+
+ debugfs_path = kzalloc(sizeof(*debugfs_path), GFP_KERNEL);
+ if (!debugfs_path) {
+ ret = -ENOMEM;
+ goto err_put;
+ }
+
+ debugfs_path->path = cur_path;
+ debugfs_path->src = kstrdup(src_node, GFP_KERNEL);
+ debugfs_path->dst = kstrdup(dst_node, GFP_KERNEL);
+ if (!debugfs_path->src || !debugfs_path->dst) {
+ ret = -ENOMEM;
+ goto err_free;
+ }
+
+ list_add_tail(&debugfs_path->list, &debugfs_paths);
+ goto out;
+
+err_free:
+ kfree(debugfs_path->src);
+ kfree(debugfs_path->dst);
+ kfree(debugfs_path);
+err_put:
+ icc_put(cur_path);
+out:
+ mutex_unlock(&debugfs_lock);
+ return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(icc_get_fops, NULL, icc_get_set, "%llu\n");
+
+static int icc_commit_set(void *data, u64 val)
+{
+ int ret;
+
+ mutex_lock(&debugfs_lock);
+
+ if (IS_ERR_OR_NULL(cur_path)) {
+ ret = PTR_ERR(cur_path);
+ goto out;
+ }
+
+ icc_set_tag(cur_path, tag);
+ ret = icc_set_bw(cur_path, avg_bw, peak_bw);
+out:
+ mutex_unlock(&debugfs_lock);
+ return ret;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(icc_commit_fops, NULL, icc_commit_set, "%llu\n");
+
+int icc_debugfs_client_init(struct dentry *icc_dir)
+{
+ struct dentry *client_dir;
+ int ret;
+
+ pdev = platform_device_alloc("icc-debugfs-client", PLATFORM_DEVID_AUTO);
+
+ ret = platform_device_add(pdev);
+ if (ret) {
+ pr_err("%s: failed to add platform device: %d\n", __func__, ret);
+ platform_device_put(pdev);
+ return ret;
+ }
+
+ client_dir = debugfs_create_dir("test_client", icc_dir);
+
+ debugfs_create_str("src_node", 0600, client_dir, &src_node);
+ debugfs_create_str("dst_node", 0600, client_dir, &dst_node);
+ debugfs_create_file("get", 0200, client_dir, NULL, &icc_get_fops);
+ debugfs_create_u32("avg_bw", 0600, client_dir, &avg_bw);
+ debugfs_create_u32("peak_bw", 0600, client_dir, &peak_bw);
+ debugfs_create_u32("tag", 0600, client_dir, &tag);
+ debugfs_create_file("commit", 0200, client_dir, NULL, &icc_commit_fops);
+
+ return 0;
+}
+
+#else
+
+int icc_debugfs_client_init(struct dentry *icc_dir)
+{
+ return 0;
+}
+
+#endif
diff --git a/drivers/interconnect/internal.h b/drivers/interconnect/internal.h
index f5f82a5c939e..87152e70a99b 100644
--- a/drivers/interconnect/internal.h
+++ b/drivers/interconnect/internal.h
@@ -41,4 +41,6 @@ struct icc_path {
struct icc_req reqs[];
};
+int icc_debugfs_client_init(struct dentry *icc_dir);
+
#endif
--
2.17.1
On Mon, Jul 24, 2023 at 06:28:59PM -0700, Mike Tipton wrote:
> It's often useful during test, debug, and development to issue path
> votes from shell. Add a debugfs client for this purpose.
>
> Example usage:
> cd /sys/kernel/debug/interconnect/test-client/
>
> # Configure node endpoints for the path from CPU to DDR on
> # qcom/sm8550.
> echo chm_apps > src_node
> echo ebi > dst_node
>
> # Get path between src_node and dst_node. This is only
> # necessary after updating the node endpoints.
> echo 1 > get
>
> # Set desired BW to 1GBps avg and 2GBps peak.
> echo 1000000 > avg_bw
> echo 2000000 > peak_bw
>
> # Vote for avg_bw and peak_bw on the latest path from "get".
> # Voting for multiple paths is possible by repeating this
> # process for different nodes endpoints.
> echo 1 > commit
>
Can Documentation/driver-api/interconnect.rst be updated with this
information?
> Allowing userspace to directly enable and set bus rates can be dangerous
> So, following in the footsteps of the regmap [0] and clk [1] frameworks,
> keep these userspace controls compile-time disabled without Kconfig
> options to enable them. Enabling this will require code changes to
> define INTERCONNECT_ALLOW_WRITE_DEBUGFS.
>
Completely agree with your argument here.
> [0] commit 09c6ecd39410 ("regmap: Add support for writing to regmap registers via debugfs")
> [1] commit 37215da5553e ("clk: Add support for setting clk_rate via debugfs")
>
> Signed-off-by: Mike Tipton <[email protected]>
> ---
> drivers/interconnect/Makefile | 2 +-
> drivers/interconnect/core.c | 3 +
> drivers/interconnect/debugfs-client.c | 156 ++++++++++++++++++++++++++
> drivers/interconnect/internal.h | 2 +
> 4 files changed, 162 insertions(+), 1 deletion(-)
> create mode 100644 drivers/interconnect/debugfs-client.c
>
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index 5604ce351a9f..d0888babb9a1 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
>
> CFLAGS_core.o := -I$(src)
> -icc-core-objs := core.o bulk.o
> +icc-core-objs := core.o bulk.o debugfs-client.o
>
> obj-$(CONFIG_INTERCONNECT) += icc-core.o
> obj-$(CONFIG_INTERCONNECT_IMX) += imx/
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 511152398790..bc82571ff2a8 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -1117,6 +1117,9 @@ static int __init icc_init(void)
> icc_debugfs_dir, NULL, &icc_summary_fops);
> debugfs_create_file("interconnect_graph", 0444,
> icc_debugfs_dir, NULL, &icc_graph_fops);
> +
> + icc_debugfs_client_init(icc_debugfs_dir);
> +
> return 0;
> }
>
> diff --git a/drivers/interconnect/debugfs-client.c b/drivers/interconnect/debugfs-client.c
> new file mode 100644
> index 000000000000..990dd2ff6df7
> --- /dev/null
> +++ b/drivers/interconnect/debugfs-client.c
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +#include <linux/debugfs.h>
> +#include <linux/interconnect.h>
> +#include <linux/platform_device.h>
> +#include <linux/uaccess.h>
> +
Since you are not dealing with user copy directly, I wonder why this
header inclusion is needed?
> +/*
> + * This can be dangerous, therefore don't provide any real compile time
> + * configuration option for this feature.
> + * People who want to use this will need to modify the source code directly.
> + */
> +#undef INTERCONNECT_ALLOW_WRITE_DEBUGFS
> +
Would it be nice to throw a compile error when DEBUG_FS is not enabled
but INTERCONNECT_ALLOW_WRITE_DEBUGFS is enabled?
> +#if defined(INTERCONNECT_ALLOW_WRITE_DEBUGFS) && defined(CONFIG_DEBUG_FS)
> +
> +static LIST_HEAD(debugfs_paths);
> +static DEFINE_MUTEX(debugfs_lock);
> +
> +static struct platform_device *pdev;
> +static struct icc_path *cur_path;
> +
> +static char *src_node;
> +static char *dst_node;
> +static u32 avg_bw;
> +static u32 peak_bw;
> +static u32 tag;
> +
> +struct debugfs_path {
> + const char *src;
> + const char *dst;
> + struct icc_path *path;
> + struct list_head list;
> +};
> +
> +static struct icc_path *get_path(const char *src, const char *dst)
> +{
> + struct debugfs_path *path;
> +
> + list_for_each_entry(path, &debugfs_paths, list) {
> + if (!strcmp(path->src, src) && !strcmp(path->dst, dst))
> + return path->path;
> + }
> +
> + return NULL;
> +}
> +
> +static int icc_get_set(void *data, u64 val)
> +{
> + struct debugfs_path *debugfs_path;
> + int ret = 0;
> +
> + mutex_lock(&debugfs_lock);
> +
> + /*
> + * If we've already looked up a path, then use the existing one instead
> + * of calling icc_get() again. This allows for updating previous BW
> + * votes when "get" is written to multiple times for multiple paths.
> + */
> + cur_path = get_path(src_node, dst_node);
> + if (cur_path)
> + goto out;
> +
> + cur_path = icc_get(&pdev->dev, src_node, dst_node);
> + if (IS_ERR(cur_path)) {
> + ret = PTR_ERR(cur_path);
> + goto out;
> + }
From the debugfs_write_file_str() implementation introduced in 1/3 of
this series, these values (src_node/dst_node) needs RCU protection.
Othewise, there is a potential UAF issue here, correct?
> +
> + debugfs_path = kzalloc(sizeof(*debugfs_path), GFP_KERNEL);
> + if (!debugfs_path) {
> + ret = -ENOMEM;
> + goto err_put;
> + }
> +
> + debugfs_path->path = cur_path;
> + debugfs_path->src = kstrdup(src_node, GFP_KERNEL);
> + debugfs_path->dst = kstrdup(dst_node, GFP_KERNEL);
> + if (!debugfs_path->src || !debugfs_path->dst) {
> + ret = -ENOMEM;
> + goto err_free;
> + }
> +
> + list_add_tail(&debugfs_path->list, &debugfs_paths);
> + goto out;
> +
> +err_free:
> + kfree(debugfs_path->src);
> + kfree(debugfs_path->dst);
> + kfree(debugfs_path);
> +err_put:
> + icc_put(cur_path);
> +out:
> + mutex_unlock(&debugfs_lock);
> + return ret;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(icc_get_fops, NULL, icc_get_set, "%llu\n");
> +
> +static int icc_commit_set(void *data, u64 val)
> +{
> + int ret;
> +
> + mutex_lock(&debugfs_lock);
> +
> + if (IS_ERR_OR_NULL(cur_path)) {
> + ret = PTR_ERR(cur_path);
> + goto out;
> + }
> +
> + icc_set_tag(cur_path, tag);
> + ret = icc_set_bw(cur_path, avg_bw, peak_bw);
> +out:
> + mutex_unlock(&debugfs_lock);
> + return ret;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(icc_commit_fops, NULL, icc_commit_set, "%llu\n");
> +
> +int icc_debugfs_client_init(struct dentry *icc_dir)
> +{
> + struct dentry *client_dir;
> + int ret;
> +
> + pdev = platform_device_alloc("icc-debugfs-client", PLATFORM_DEVID_AUTO);
> +
In interconnect_summary debugfs file, it appears as
"icc-debugfs-client.0.auto". Since we will only ever support one
instance of this device, would PLATFORM_DEVID_NONE be more appropriate?
> + ret = platform_device_add(pdev);
> + if (ret) {
> + pr_err("%s: failed to add platform device: %d\n", __func__, ret);
> + platform_device_put(pdev);
> + return ret;
> + }
> +
> + client_dir = debugfs_create_dir("test_client", icc_dir);
> +
> + debugfs_create_str("src_node", 0600, client_dir, &src_node);
> + debugfs_create_str("dst_node", 0600, client_dir, &dst_node);
> + debugfs_create_file("get", 0200, client_dir, NULL, &icc_get_fops);
> + debugfs_create_u32("avg_bw", 0600, client_dir, &avg_bw);
> + debugfs_create_u32("peak_bw", 0600, client_dir, &peak_bw);
> + debugfs_create_u32("tag", 0600, client_dir, &tag);
> + debugfs_create_file("commit", 0200, client_dir, NULL, &icc_commit_fops);
> +
> + return 0;
> +}
> +
> +#else
> +
> +int icc_debugfs_client_init(struct dentry *icc_dir)
> +{
> + return 0;
> +}
> +
> +#endif
> diff --git a/drivers/interconnect/internal.h b/drivers/interconnect/internal.h
> index f5f82a5c939e..87152e70a99b 100644
> --- a/drivers/interconnect/internal.h
> +++ b/drivers/interconnect/internal.h
> @@ -41,4 +41,6 @@ struct icc_path {
> struct icc_req reqs[];
> };
>
> +int icc_debugfs_client_init(struct dentry *icc_dir);
> +
> #endif
> --
> 2.17.1
>
Thanks,
Pavan
On Mon, Jul 24, 2023 at 06:28:58PM -0700, Mike Tipton wrote:
> The original icc_get() that took integer node IDs was removed due to
> lack of users. Reintroduce a new version that takes string node names,
> which is needed for the debugfs client.
>
> Signed-off-by: Mike Tipton <[email protected]>
> ---
> drivers/interconnect/core.c | 64 ++++++++++++++++++++++++++++++++++++
> include/linux/interconnect.h | 6 ++++
> 2 files changed, 70 insertions(+)
[...]
> +/**
> + * icc_get() - get a path handle between two endpoints
> + * @dev: device pointer for the consumer device
> + * @src: source node name
> + * @dst: destination node name
> + *
> + * 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.
> + *
> + * Return: icc_path pointer on success or ERR_PTR() on error. NULL is returned
> + * when the API is disabled.
> + */
> +struct icc_path *icc_get(struct device *dev, const char *src, const char *dst)
> +{
> + struct icc_node *src_node, *dst_node;
> + struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
> +
> + mutex_lock(&icc_lock);
> +
> + src_node = node_find_by_name(src);
> + if (!src_node) {
> + dev_err(dev, "%s: invalid src=%s\n", __func__, src);
> + goto out;
> + }
> +
> + dst_node = node_find_by_name(dst);
> + if (!dst_node) {
> + dev_err(dev, "%s: invalid dst=%s\n", __func__, dst);
> + goto out;
> + }
> +
> + path = path_find(dev, src_node, dst_node);
> + 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_node->name, dst_node->name);
> + if (!path->name) {
> + kfree(path);
> + path = ERR_PTR(-ENOMEM);
> + }
> +out:
> + mutex_unlock(&icc_lock);
> + return path;
> +}
> +EXPORT_SYMBOL_GPL(icc_get);
> +
> /**
> * icc_set_tag() - set an optional tag on a path
> * @path: the path we want to tag
> diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
> index 97ac253df62c..39a98ddfdfd4 100644
> --- a/include/linux/interconnect.h
> +++ b/include/linux/interconnect.h
> @@ -40,6 +40,7 @@ struct icc_bulk_data {
>
> #if IS_ENABLED(CONFIG_INTERCONNECT)
>
> +struct icc_path *icc_get(struct device *dev, const char *src, const char *dst);
> 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);
> @@ -59,6 +60,11 @@ void icc_bulk_disable(int num_paths, const struct icc_bulk_data *paths);
>
> #else
>
> +struct icc_path *icc_get(struct device *dev, const char *src, const char *dst)
> +{
> + return NULL;
> +}
> +
Is this API meant to be public()? As you pointed out, it was removed
recently as there were no users. Since debugfs client is part of ICC
frarmwork, should it be made private?
Thanks,
Pavan
On Mon, Jul 24, 2023 at 06:28:56PM -0700, Mike Tipton wrote:
> This series introduces interconnect debugfs files that support voting
> for any path the framework supports.
>
> We've historically relied on an out-of-tree module for this, which used
> the old icc_get() that was recently removed in [0]. The old icc_get()
> took integer endpoint IDs, which made identifying paths in our old
> implementation non-intuitive. The logical node names typically don't
> change much chip-to-chip, but the raw integer IDs do. Take this
> opportunity to introduce an icc_get() that uses string names instead,
> which allows for a more intuitive and generic debugfs interface.
>
> We rely on this support for debug, test, and verification. Hopefully
> it'll be useful for other vendors as well.
Thanks Mike for working on this series. The downstream driver has been
helpful in quickly pushing NOCs to max during throughput issues
debugging. I have tested this series on v6.4 + revert mentioned here on
SM8550 MTP and it works as advertised.
Thanks,
Pavan
Hi Mike,
kernel test robot noticed the following build warnings:
[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.5-rc3 next-20230725]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mike-Tipton/debugfs-Add-write-support-to-debugfs_create_str/20230725-093242
base: driver-core/driver-core-testing
patch link: https://lore.kernel.org/r/20230725012859.18474-3-quic_mdtipton%40quicinc.com
patch subject: [PATCH 2/3] interconnect: Reintroduce icc_get()
config: um-randconfig-m031-20230725 (https://download.01.org/0day-ci/archive/20230725/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230725/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
In file included from drivers/opp/opp.h:15,
from drivers/opp/core.c:23:
>> include/linux/interconnect.h:63:18: warning: no previous prototype for 'icc_get' [-Wmissing-prototypes]
63 | struct icc_path *icc_get(struct device *dev, const char *src, const char *dst)
| ^~~~~~~
vim +/icc_get +63 include/linux/interconnect.h
62
> 63 struct icc_path *icc_get(struct device *dev, const char *src, const char *dst)
64 {
65 return NULL;
66 }
67
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Mike,
kernel test robot noticed the following build errors:
[auto build test ERROR on driver-core/driver-core-testing]
[also build test ERROR on driver-core/driver-core-next driver-core/driver-core-linus linus/master v6.5-rc3 next-20230725]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Mike-Tipton/debugfs-Add-write-support-to-debugfs_create_str/20230725-093242
base: driver-core/driver-core-testing
patch link: https://lore.kernel.org/r/20230725012859.18474-3-quic_mdtipton%40quicinc.com
patch subject: [PATCH 2/3] interconnect: Reintroduce icc_get()
config: arm-integrator_defconfig (https://download.01.org/0day-ci/archive/20230725/[email protected]/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230725/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All error/warnings (new ones prefixed by >>):
In file included from drivers/opp/opp.h:15,
from drivers/opp/core.c:23:
>> include/linux/interconnect.h:63:18: warning: no previous prototype for 'icc_get' [-Wmissing-prototypes]
63 | struct icc_path *icc_get(struct device *dev, const char *src, const char *dst)
| ^~~~~~~
--
arm-linux-gnueabi-ld: drivers/opp/cpu.o: in function `icc_get':
>> cpu.c:(.text+0x2c8): multiple definition of `icc_get'; drivers/opp/core.o:core.c:(.text+0x2260): first defined here
arm-linux-gnueabi-ld: drivers/opp/of.o: in function `icc_get':
of.c:(.text+0x1b20): multiple definition of `icc_get'; drivers/opp/core.o:core.c:(.text+0x2260): first defined here
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On 7/24/2023 10:46 PM, Pavan Kondeti wrote:
> On Mon, Jul 24, 2023 at 06:28:59PM -0700, Mike Tipton wrote:
>> It's often useful during test, debug, and development to issue path
>> votes from shell. Add a debugfs client for this purpose.
>>
>> Example usage:
>> cd /sys/kernel/debug/interconnect/test-client/
>>
>> # Configure node endpoints for the path from CPU to DDR on
>> # qcom/sm8550.
>> echo chm_apps > src_node
>> echo ebi > dst_node
>>
>> # Get path between src_node and dst_node. This is only
>> # necessary after updating the node endpoints.
>> echo 1 > get
>>
>> # Set desired BW to 1GBps avg and 2GBps peak.
>> echo 1000000 > avg_bw
>> echo 2000000 > peak_bw
>>
>> # Vote for avg_bw and peak_bw on the latest path from "get".
>> # Voting for multiple paths is possible by repeating this
>> # process for different nodes endpoints.
>> echo 1 > commit
>>
>
> Can Documentation/driver-api/interconnect.rst be updated with this
> information?
Sure, will add some documentation there.
>
>> Allowing userspace to directly enable and set bus rates can be dangerous
>> So, following in the footsteps of the regmap [0] and clk [1] frameworks,
>> keep these userspace controls compile-time disabled without Kconfig
>> options to enable them. Enabling this will require code changes to
>> define INTERCONNECT_ALLOW_WRITE_DEBUGFS.
>>
>
> Completely agree with your argument here.
>
>> [0] commit 09c6ecd39410 ("regmap: Add support for writing to regmap registers via debugfs")
>> [1] commit 37215da5553e ("clk: Add support for setting clk_rate via debugfs")
>>
>> Signed-off-by: Mike Tipton <[email protected]>
>> ---
>> drivers/interconnect/Makefile | 2 +-
>> drivers/interconnect/core.c | 3 +
>> drivers/interconnect/debugfs-client.c | 156 ++++++++++++++++++++++++++
>> drivers/interconnect/internal.h | 2 +
>> 4 files changed, 162 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/interconnect/debugfs-client.c
>>
>> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
>> index 5604ce351a9f..d0888babb9a1 100644
>> --- a/drivers/interconnect/Makefile
>> +++ b/drivers/interconnect/Makefile
>> @@ -1,7 +1,7 @@
>> # SPDX-License-Identifier: GPL-2.0
>>
>> CFLAGS_core.o := -I$(src)
>> -icc-core-objs := core.o bulk.o
>> +icc-core-objs := core.o bulk.o debugfs-client.o
>>
>> obj-$(CONFIG_INTERCONNECT) += icc-core.o
>> obj-$(CONFIG_INTERCONNECT_IMX) += imx/
>> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
>> index 511152398790..bc82571ff2a8 100644
>> --- a/drivers/interconnect/core.c
>> +++ b/drivers/interconnect/core.c
>> @@ -1117,6 +1117,9 @@ static int __init icc_init(void)
>> icc_debugfs_dir, NULL, &icc_summary_fops);
>> debugfs_create_file("interconnect_graph", 0444,
>> icc_debugfs_dir, NULL, &icc_graph_fops);
>> +
>> + icc_debugfs_client_init(icc_debugfs_dir);
>> +
>> return 0;
>> }
>>
>> diff --git a/drivers/interconnect/debugfs-client.c b/drivers/interconnect/debugfs-client.c
>> new file mode 100644
>> index 000000000000..990dd2ff6df7
>> --- /dev/null
>> +++ b/drivers/interconnect/debugfs-client.c
>> @@ -0,0 +1,156 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +#include <linux/debugfs.h>
>> +#include <linux/interconnect.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/uaccess.h>
>> +
>
> Since you are not dealing with user copy directly, I wonder why this
> header inclusion is needed?
It's not. I'll remove it.
>
>> +/*
>> + * This can be dangerous, therefore don't provide any real compile time
>> + * configuration option for this feature.
>> + * People who want to use this will need to modify the source code directly.
>> + */
>> +#undef INTERCONNECT_ALLOW_WRITE_DEBUGFS
>> +
>
> Would it be nice to throw a compile error when DEBUG_FS is not enabled
> but INTERCONNECT_ALLOW_WRITE_DEBUGFS is enabled?
I don't think we want to consider that a compile error. Someone
#defining this may still want the flexibility of disabling DEBUG_FS
through Kconfig for certain configurations. Certain kernels could
mainline their own changes to define INTERCONNECT_ALLOW_WRITE_DEBUGFS,
and we don't want to preclude them from also disabling DEBUG_FS if desired.
>
>> +#if defined(INTERCONNECT_ALLOW_WRITE_DEBUGFS) && defined(CONFIG_DEBUG_FS)
>> +
>> +static LIST_HEAD(debugfs_paths);
>> +static DEFINE_MUTEX(debugfs_lock);
>> +
>> +static struct platform_device *pdev;
>> +static struct icc_path *cur_path;
>> +
>> +static char *src_node;
>> +static char *dst_node;
>> +static u32 avg_bw;
>> +static u32 peak_bw;
>> +static u32 tag;
>> +
>> +struct debugfs_path {
>> + const char *src;
>> + const char *dst;
>> + struct icc_path *path;
>> + struct list_head list;
>> +};
>> +
>> +static struct icc_path *get_path(const char *src, const char *dst)
>> +{
>> + struct debugfs_path *path;
>> +
>> + list_for_each_entry(path, &debugfs_paths, list) {
>> + if (!strcmp(path->src, src) && !strcmp(path->dst, dst))
>> + return path->path;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +static int icc_get_set(void *data, u64 val)
>> +{
>> + struct debugfs_path *debugfs_path;
>> + int ret = 0;
>> +
>> + mutex_lock(&debugfs_lock);
>> +
>> + /*
>> + * If we've already looked up a path, then use the existing one instead
>> + * of calling icc_get() again. This allows for updating previous BW
>> + * votes when "get" is written to multiple times for multiple paths.
>> + */
>> + cur_path = get_path(src_node, dst_node);
>> + if (cur_path)
>> + goto out;
>> +
>> + cur_path = icc_get(&pdev->dev, src_node, dst_node);
>> + if (IS_ERR(cur_path)) {
>> + ret = PTR_ERR(cur_path);
>> + goto out;
>> + }
>
> From the debugfs_write_file_str() implementation introduced in 1/3 of
> this series, these values (src_node/dst_node) needs RCU protection.
> Othewise, there is a potential UAF issue here, correct?
Yeah, good catch. This could happen if the "get" file is written to at
the same time as the src_node or dst_node files. Will look into this.
>
>> +
>> + debugfs_path = kzalloc(sizeof(*debugfs_path), GFP_KERNEL);
>> + if (!debugfs_path) {
>> + ret = -ENOMEM;
>> + goto err_put;
>> + }
>> +
>> + debugfs_path->path = cur_path;
>> + debugfs_path->src = kstrdup(src_node, GFP_KERNEL);
>> + debugfs_path->dst = kstrdup(dst_node, GFP_KERNEL);
>> + if (!debugfs_path->src || !debugfs_path->dst) {
>> + ret = -ENOMEM;
>> + goto err_free;
>> + }
>> +
>> + list_add_tail(&debugfs_path->list, &debugfs_paths);
>> + goto out;
>> +
>> +err_free:
>> + kfree(debugfs_path->src);
>> + kfree(debugfs_path->dst);
>> + kfree(debugfs_path);
>> +err_put:
>> + icc_put(cur_path);
>> +out:
>> + mutex_unlock(&debugfs_lock);
>> + return ret;
>> +}
>> +
>> +DEFINE_DEBUGFS_ATTRIBUTE(icc_get_fops, NULL, icc_get_set, "%llu\n");
>> +
>> +static int icc_commit_set(void *data, u64 val)
>> +{
>> + int ret;
>> +
>> + mutex_lock(&debugfs_lock);
>> +
>> + if (IS_ERR_OR_NULL(cur_path)) {
>> + ret = PTR_ERR(cur_path);
>> + goto out;
>> + }
>> +
>> + icc_set_tag(cur_path, tag);
>> + ret = icc_set_bw(cur_path, avg_bw, peak_bw);
>> +out:
>> + mutex_unlock(&debugfs_lock);
>> + return ret;
>> +}
>> +
>> +DEFINE_DEBUGFS_ATTRIBUTE(icc_commit_fops, NULL, icc_commit_set, "%llu\n");
>> +
>> +int icc_debugfs_client_init(struct dentry *icc_dir)
>> +{
>> + struct dentry *client_dir;
>> + int ret;
>> +
>> + pdev = platform_device_alloc("icc-debugfs-client", PLATFORM_DEVID_AUTO);
>> +
>
> In interconnect_summary debugfs file, it appears as
> "icc-debugfs-client.0.auto". Since we will only ever support one
> instance of this device, would PLATFORM_DEVID_NONE be more appropriate?
Agreed, no need for AUTO here. Will switch to NONE to make the device
name prettier in the summary.
>
>> + ret = platform_device_add(pdev);
>> + if (ret) {
>> + pr_err("%s: failed to add platform device: %d\n", __func__, ret);
>> + platform_device_put(pdev);
>> + return ret;
>> + }
>> +
>> + client_dir = debugfs_create_dir("test_client", icc_dir);
>> +
>> + debugfs_create_str("src_node", 0600, client_dir, &src_node);
>> + debugfs_create_str("dst_node", 0600, client_dir, &dst_node);
>> + debugfs_create_file("get", 0200, client_dir, NULL, &icc_get_fops);
>> + debugfs_create_u32("avg_bw", 0600, client_dir, &avg_bw);
>> + debugfs_create_u32("peak_bw", 0600, client_dir, &peak_bw);
>> + debugfs_create_u32("tag", 0600, client_dir, &tag);
>> + debugfs_create_file("commit", 0200, client_dir, NULL, &icc_commit_fops);
>> +
>> + return 0;
>> +}
>> +
>> +#else
>> +
>> +int icc_debugfs_client_init(struct dentry *icc_dir)
>> +{
>> + return 0;
>> +}
>> +
>> +#endif
>> diff --git a/drivers/interconnect/internal.h b/drivers/interconnect/internal.h
>> index f5f82a5c939e..87152e70a99b 100644
>> --- a/drivers/interconnect/internal.h
>> +++ b/drivers/interconnect/internal.h
>> @@ -41,4 +41,6 @@ struct icc_path {
>> struct icc_req reqs[];
>> };
>>
>> +int icc_debugfs_client_init(struct dentry *icc_dir);
>> +
>> #endif
>> --
>> 2.17.1
>>
>
> Thanks,
> Pavan
On Tue, Jul 25, 2023 at 11:18:58AM +0530, Pavan Kondeti wrote:
> On Mon, Jul 24, 2023 at 06:28:58PM -0700, Mike Tipton wrote:
> > The original icc_get() that took integer node IDs was removed due to
> > lack of users. Reintroduce a new version that takes string node names,
> > which is needed for the debugfs client.
> >
[..]
>
> Is this API meant to be public()? As you pointed out, it was removed
> recently as there were no users. Since debugfs client is part of ICC
> frarmwork, should it be made private?
With debugfs as the only user, it could technically be made private.
Generally speaking, people should use of_icc_get() instead of this. We
have some other downstream users of icc_get(), but we'll likely push
them to switch to of_icc_get() instead. There are some potential future
use cases that would require exporting this, but none immediate.