| Message ID | 20200220170726.9580-1-alexandru.tachici@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: industrialio-core: Fix debugfs read | expand |
On Thu, 2020-02-20 at 19:07 +0200, Alexandru Tachici wrote: > [External] > > Currently iio_debugfs_read_reg calls debugfs_reg_access > every time it is ran. Reading the same hardware register > multiple times during the same reading of a debugfs file > can cause unintended effects. > > For example for each: cat iio:device0/direct_reg_access > the file_operations.read function will be called at least > twice. First will return the full length of the string in > bytes and the second will return 0. > > This patch makes iio_debugfs_read_reg to call debugfs_reg_access > only when the user's buffer position (*ppos) is 0. (meaning > it is the beginning of a new reading of the debugfs file). > > Fixes: e553f182d55b ("staging: iio: core: Introduce debugfs support, add > support for direct register access") > Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com> > --- > drivers/iio/industrialio-core.c | 34 +++++++++++++++++++++++---------- > include/linux/iio/iio.h | 2 ++ > 2 files changed, 26 insertions(+), 10 deletions(-) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index 65ff0d067018..637cea14afdb 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -297,26 +297,40 @@ static void __exit iio_exit(void) > } > > #if defined(CONFIG_DEBUG_FS) > -static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf, > - size_t count, loff_t *ppos) > +static int iio_debugfs_get_reg_string(struct iio_dev *indio_dev) > { > - struct iio_dev *indio_dev = file->private_data; > - char buf[20]; > + const struct iio_info *info = indio_dev->info; > unsigned val = 0; > - ssize_t len; > int ret; > > - ret = indio_dev->info->debugfs_reg_access(indio_dev, > - indio_dev->cached_reg_addr, > - 0, &val); > + ret = info->debugfs_reg_access(indio_dev, indio_dev->cached_reg_addr, > + 0, &val); > if (ret) { > dev_err(indio_dev->dev.parent, "%s: read failed\n", __func__); > return ret; > } > + indio_dev->read_buf_len = snprintf(indio_dev->read_buf, > + sizeof(indio_dev->read_buf), > + "0x%X\n", val); > + return 0; > +} > > - len = snprintf(buf, sizeof(buf), "0x%X\n", val); > +static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf, > + size_t count, loff_t *ppos) > +{ > + struct iio_dev *indio_dev = file->private_data; > + loff_t pos = *ppos; > + int ret; > + > + if (pos == 0) { > + ret = iio_debugfs_get_reg_string(indio_dev); > + if (ret) > + return ret; > + } > > - return simple_read_from_buffer(userbuf, count, ppos, buf, len); I'm wondering now, if it would be sufficient here to just do: if (len == simple_read_from_buffer(userbuf, count, ppos, buf, len)) return 0; > + return simple_read_from_buffer(userbuf, count, ppos, > + indio_dev->read_buf, > + indio_dev->read_buf_len); > } > > static ssize_t iio_debugfs_write_reg(struct file *file, > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > index 862ce0019eba..eed58ed2f368 100644 > --- a/include/linux/iio/iio.h > +++ b/include/linux/iio/iio.h > @@ -568,6 +568,8 @@ struct iio_dev { > #if defined(CONFIG_DEBUG_FS) > struct dentry *debugfs_dentry; > unsigned cached_reg_addr; > + char read_buf[20]; > + unsigned int read_buf_len; > #endif > }; >
On Thu, 2020-02-20 at 19:07 +0200, Alexandru Tachici wrote: > Currently iio_debugfs_read_reg calls debugfs_reg_access > every time it is ran. Reading the same hardware register > multiple times during the same reading of a debugfs file > can cause unintended effects. > > For example for each: cat iio:device0/direct_reg_access > the file_operations.read function will be called at least > twice. First will return the full length of the string in > bytes and the second will return 0. > > This patch makes iio_debugfs_read_reg to call debugfs_reg_access > only when the user's buffer position (*ppos) is 0. (meaning > it is the beginning of a new reading of the debugfs file). Please disregard this. Will send a V2. > Fixes: e553f182d55b ("staging: iio: core: Introduce debugfs support, > add support for direct register access") > Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com> > --- > drivers/iio/industrialio-core.c | 34 +++++++++++++++++++++++------ > ---- > include/linux/iio/iio.h | 2 ++ > 2 files changed, 26 insertions(+), 10 deletions(-) > > diff --git a/drivers/iio/industrialio-core.c > b/drivers/iio/industrialio-core.c > index 65ff0d067018..637cea14afdb 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -297,26 +297,40 @@ static void __exit iio_exit(void) > } > > #if defined(CONFIG_DEBUG_FS) > -static ssize_t iio_debugfs_read_reg(struct file *file, char __user > *userbuf, > - size_t count, loff_t *ppos) > +static int iio_debugfs_get_reg_string(struct iio_dev *indio_dev) > { > - struct iio_dev *indio_dev = file->private_data; > - char buf[20]; > + const struct iio_info *info = indio_dev->info; > unsigned val = 0; > - ssize_t len; > int ret; > > - ret = indio_dev->info->debugfs_reg_access(indio_dev, > - indio_dev- > >cached_reg_addr, > - 0, &val); > + ret = info->debugfs_reg_access(indio_dev, indio_dev- > >cached_reg_addr, > + 0, &val); > if (ret) { > dev_err(indio_dev->dev.parent, "%s: read failed\n", > __func__); > return ret; > } > + indio_dev->read_buf_len = snprintf(indio_dev->read_buf, > + sizeof(indio_dev->read_buf), > + "0x%X\n", val); > + return 0; > +} > > - len = snprintf(buf, sizeof(buf), "0x%X\n", val); > +static ssize_t iio_debugfs_read_reg(struct file *file, char __user > *userbuf, > + size_t count, loff_t *ppos) > +{ > + struct iio_dev *indio_dev = file->private_data; > + loff_t pos = *ppos; > + int ret; > + > + if (pos == 0) { > + ret = iio_debugfs_get_reg_string(indio_dev); > + if (ret) > + return ret; > + } > > - return simple_read_from_buffer(userbuf, count, ppos, buf, len); > + return simple_read_from_buffer(userbuf, count, ppos, > + indio_dev->read_buf, > + indio_dev->read_buf_len); > } > > static ssize_t iio_debugfs_write_reg(struct file *file, > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > index 862ce0019eba..eed58ed2f368 100644 > --- a/include/linux/iio/iio.h > +++ b/include/linux/iio/iio.h > @@ -568,6 +568,8 @@ struct iio_dev { > #if defined(CONFIG_DEBUG_FS) > struct dentry *debugfs_dentry; > unsigned cached_reg_addr; > + char read_buf[20]; > + unsigned int read_buf_len; > #endif > }; >
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 65ff0d067018..637cea14afdb 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -297,26 +297,40 @@ static void __exit iio_exit(void) } #if defined(CONFIG_DEBUG_FS) -static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf, - size_t count, loff_t *ppos) +static int iio_debugfs_get_reg_string(struct iio_dev *indio_dev) { - struct iio_dev *indio_dev = file->private_data; - char buf[20]; + const struct iio_info *info = indio_dev->info; unsigned val = 0; - ssize_t len; int ret; - ret = indio_dev->info->debugfs_reg_access(indio_dev, - indio_dev->cached_reg_addr, - 0, &val); + ret = info->debugfs_reg_access(indio_dev, indio_dev->cached_reg_addr, + 0, &val); if (ret) { dev_err(indio_dev->dev.parent, "%s: read failed\n", __func__); return ret; } + indio_dev->read_buf_len = snprintf(indio_dev->read_buf, + sizeof(indio_dev->read_buf), + "0x%X\n", val); + return 0; +} - len = snprintf(buf, sizeof(buf), "0x%X\n", val); +static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf, + size_t count, loff_t *ppos) +{ + struct iio_dev *indio_dev = file->private_data; + loff_t pos = *ppos; + int ret; + + if (pos == 0) { + ret = iio_debugfs_get_reg_string(indio_dev); + if (ret) + return ret; + } - return simple_read_from_buffer(userbuf, count, ppos, buf, len); + return simple_read_from_buffer(userbuf, count, ppos, + indio_dev->read_buf, + indio_dev->read_buf_len); } static ssize_t iio_debugfs_write_reg(struct file *file, diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h index 862ce0019eba..eed58ed2f368 100644 --- a/include/linux/iio/iio.h +++ b/include/linux/iio/iio.h @@ -568,6 +568,8 @@ struct iio_dev { #if defined(CONFIG_DEBUG_FS) struct dentry *debugfs_dentry; unsigned cached_reg_addr; + char read_buf[20]; + unsigned int read_buf_len; #endif };
Currently iio_debugfs_read_reg calls debugfs_reg_access every time it is ran. Reading the same hardware register multiple times during the same reading of a debugfs file can cause unintended effects. For example for each: cat iio:device0/direct_reg_access the file_operations.read function will be called at least twice. First will return the full length of the string in bytes and the second will return 0. This patch makes iio_debugfs_read_reg to call debugfs_reg_access only when the user's buffer position (*ppos) is 0. (meaning it is the beginning of a new reading of the debugfs file). Fixes: e553f182d55b ("staging: iio: core: Introduce debugfs support, add support for direct register access") Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com> --- drivers/iio/industrialio-core.c | 34 +++++++++++++++++++++++---------- include/linux/iio/iio.h | 2 ++ 2 files changed, 26 insertions(+), 10 deletions(-)