| Message ID | 20230807142914.12480-1-quic_mdtipton@quicinc.com |
|---|---|
| Headers | show |
| Series | Add interconnect debugfs client | expand |
On 7.08.23 17:29, Mike Tipton wrote: > Currently, debugfs_create_str() only supports reading strings from > debugfs. Add support for writing them as well. > > Based on original implementation by Peter Zijlstra [0]. Write support > was present in the initial patch version, but dropped in v2 due to lack > of users. We have a user now, so reintroduce it. > > [0] https://lore.kernel.org/all/YF3Hv5zXb%2F6lauzs@hirez.programming.kicks-ass.net/ > Hi Greg, Looks like the original code was reviewed two years ago (not sure if it counts). But in any case, i need an ack from you to apply this. There is no build dependency with the rest of the patches (but there is a functional one). It should be also fine if you apply it directly, if you prefer so? Thanks, Georgi > Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com> > --- > 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 = {
On Sat, Aug 12, 2023 at 12:36:12PM +0200, Greg KH wrote: > On Fri, Aug 11, 2023 at 10:46:34AM +0300, Georgi Djakov wrote: > > On 7.08.23 17:29, Mike Tipton wrote: > > > Currently, debugfs_create_str() only supports reading strings from > > > debugfs. Add support for writing them as well. > > > > > > Based on original implementation by Peter Zijlstra [0]. Write support > > > was present in the initial patch version, but dropped in v2 due to lack > > > of users. We have a user now, so reintroduce it. > > > > > > [0] https://lore.kernel.org/all/YF3Hv5zXb%2F6lauzs@hirez.programming.kicks-ass.net/ > > > > > > > Hi Greg, > > > > Looks like the original code was reviewed two years ago (not sure if it > > counts). But in any case, i need an ack from you to apply this. > > > > There is no build dependency with the rest of the patches (but there is a > > functional one). It should be also fine if you apply it directly, if you > > prefer so? > > Feel free for this to go with the code that uses it: > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Wait, no, this isn't ok, this ACK is now rescinded: NACKED-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Hi Greg, Thanks for the comments! On 12.08.23 13:40, Greg KH wrote: > On Mon, Aug 07, 2023 at 07:29:12AM -0700, Mike Tipton wrote: >> Currently, debugfs_create_str() only supports reading strings from >> debugfs. Add support for writing them as well. >> >> Based on original implementation by Peter Zijlstra [0]. Write support >> was present in the initial patch version, but dropped in v2 due to lack >> of users. We have a user now, so reintroduce it. >> >> [0] https://lore.kernel.org/all/YF3Hv5zXb%2F6lauzs@hirez.programming.kicks-ass.net/ >> >> Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com> >> --- >> 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; >> } > > So you just added write support for ALL debugfs files that use the > string interface, what did you just allow to break? Not really. According to the existing code, the write support for strings is enabled only when the file is created with +w permissions. For read-only files, we use fops_str_ro, which is the case for all existing string files: $ git grep -w debugfs_create_str | egrep -v "fs/debugfs/file.c|include/linux/debugfs.h" drivers/firmware/arm_scmi/driver.c: debugfs_create_str("instance_name", 0400, top_dentry, drivers/firmware/arm_scmi/driver.c: debugfs_create_str("type", 0400, trans, (char **)&dbg->type); drivers/opp/debugfs.c: debugfs_create_str("of_name", S_IRUGO, d, (char **)&opp->of_name); For fops_str_ro, the .write function is not implemented, so nothing should break? > I recommend just using your own debugfs file function instead, as this > could cause bad problems, right? Agree, and that should be exactly what this patch does. > Are you sure that all string calls can > handle the variable be freed underneath it like this call will allow to > happen? Looks fine, at least for this patch-set. Thanks, Georgi
Hi Greg, On 12.08.23 13:40, Greg KH wrote: > On Mon, Aug 07, 2023 at 07:29:12AM -0700, Mike Tipton wrote: >> Currently, debugfs_create_str() only supports reading strings from >> debugfs. Add support for writing them as well. >> >> Based on original implementation by Peter Zijlstra [0]. Write support >> was present in the initial patch version, but dropped in v2 due to lack >> of users. We have a user now, so reintroduce it. >> >> [0] https://lore.kernel.org/all/YF3Hv5zXb%2F6lauzs@hirez.programming.kicks-ass.net/ >> >> Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com> >> --- >> 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; >> } > > So you just added write support for ALL debugfs files that use the > string interface, what did you just allow to break? Not true. Write support is added only for debugfs string files that are created with +w permissions. All existing files are created as read-only and use the fops_str_ro ops. > I recommend just using your own debugfs file function instead, as this > could cause bad problems, right? Are you sure that all string calls can > handle the variable be freed underneath it like this call will allow to > happen? > > So I wouldn't recommend doing this, sorry. > Maybe you missed the fact that the different file ops are already there and are selected based on permissions: > static const struct file_operations fops_str = { > .read = debugfs_read_file_str, > .write = debugfs_write_file_str, > .open = simple_open, > .llseek = default_llseek, > }; > > static const struct file_operations fops_str_ro = { > .read = debugfs_read_file_str, > .open = simple_open, > .llseek = default_llseek, > }; > > static const struct file_operations fops_str_wo = { > .write = debugfs_write_file_str, > .open = simple_open, > .llseek = default_llseek, > }; ...so this patch is doing exactly what you suggested? If you agree, could you ack it again please? Thanks, Georgi
On Fri, Aug 18, 2023 at 01:05:57PM +0300, Georgi Djakov wrote: > Hi Greg, > > On 12.08.23 13:40, Greg KH wrote: > > On Mon, Aug 07, 2023 at 07:29:12AM -0700, Mike Tipton wrote: > > > Currently, debugfs_create_str() only supports reading strings from > > > debugfs. Add support for writing them as well. > > > > > > Based on original implementation by Peter Zijlstra [0]. Write support > > > was present in the initial patch version, but dropped in v2 due to lack > > > of users. We have a user now, so reintroduce it. > > > > > > [0] https://lore.kernel.org/all/YF3Hv5zXb%2F6lauzs@hirez.programming.kicks-ass.net/ > > > > > > Signed-off-by: Mike Tipton <quic_mdtipton@quicinc.com> > > > --- > > > 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; > > > } > > > > So you just added write support for ALL debugfs files that use the > > string interface, what did you just allow to break? > > Not true. Write support is added only for debugfs string files that are > created with +w permissions. All existing files are created as read-only > and use the fops_str_ro ops. > > > I recommend just using your own debugfs file function instead, as this > > could cause bad problems, right? Are you sure that all string calls can > > handle the variable be freed underneath it like this call will allow to > > happen? > > > > So I wouldn't recommend doing this, sorry. > > > > Maybe you missed the fact that the different file ops are already there > and are selected based on permissions: > > > static const struct file_operations fops_str = { > > .read = debugfs_read_file_str, > > .write = debugfs_write_file_str, > > .open = simple_open, > > .llseek = default_llseek, > > }; > > > > static const struct file_operations fops_str_ro = { > > .read = debugfs_read_file_str, > > .open = simple_open, > > .llseek = default_llseek, > > }; > > > > static const struct file_operations fops_str_wo = { > > .write = debugfs_write_file_str, > > .open = simple_open, > > .llseek = default_llseek, > > }; > > ...so this patch is doing exactly what you suggested? If you agree, > could you ack it again please? Yes, I did miss that, sorry, my appologies for dragging this out so long: Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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") Changes in v3: - Use GFP_ATOMIC when allocating with the rcu lock held. Changes in v2: - Make icc_get() an internal interface. - RCU-protect src_node and dst_node. - Replace PLATFORM_DEVID_AUTO with PLATFORM_DEVID_NONE. - Remove unnecessary #include. - Add debugfs client documentation. Mike Tipton (3): debugfs: Add write support to debugfs_create_str() interconnect: Reintroduce icc_get() interconnect: Add debugfs test client Documentation/driver-api/interconnect.rst | 25 ++++ drivers/interconnect/Makefile | 2 +- drivers/interconnect/core.c | 66 +++++++++ drivers/interconnect/debugfs-client.c | 168 ++++++++++++++++++++++ drivers/interconnect/internal.h | 3 + fs/debugfs/file.c | 48 ++++++- 6 files changed, 309 insertions(+), 3 deletions(-) create mode 100644 drivers/interconnect/debugfs-client.c