drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Update the sysfs interface for sampling frequency and scale attributes.
Replace `scnprintf()` with `sysfs_emit_at()` which is PAGE_SIZE-aware
and recommended for use in sysfs.
Signed-off-by: Akshay Bansod <akbansd@gmail.com>
---
changes in v2:
- Fixed indentation for line wrap
- Link to v1: https://lore.kernel.org/linux-iio/20250702135855.59955-1-akbansd@gmail.com/
Testing:
- Built the driver (`st_lsm6dsx_i2c`) as a module.
- Tested using `i2c-stub` to mock the device.
- Verified that reading sysfs attributes like `sampling_frequency_available`
works correctly and shows no change in functionality.
---
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index c65ad4982..7689ca39a 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -2035,9 +2035,9 @@ st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev,
odr_table = &sensor->hw->settings->odr_table[sensor->id];
for (i = 0; i < odr_table->odr_len; i++)
- len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%03d ",
- odr_table->odr_avl[i].milli_hz / 1000,
- odr_table->odr_avl[i].milli_hz % 1000);
+ len += sysfs_emit_at(buf, len, "%d.%03d ",
+ odr_table->odr_avl[i].milli_hz / 1000,
+ odr_table->odr_avl[i].milli_hz % 1000);
buf[len - 1] = '\n';
return len;
@@ -2054,8 +2054,8 @@ static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev,
fs_table = &hw->settings->fs_table[sensor->id];
for (i = 0; i < fs_table->fs_len; i++)
- len += scnprintf(buf + len, PAGE_SIZE - len, "0.%09u ",
- fs_table->fs_avl[i].gain);
+ len += sysfs_emit_at(buf, len, "0.%09u ",
+ fs_table->fs_avl[i].gain);
buf[len - 1] = '\n';
return len;
--
2.49.0On Thu, Jul 03, 2025 at 11:08:59AM +0530, Akshay Bansod wrote: > Update the sysfs interface for sampling frequency and scale attributes. > Replace `scnprintf()` with `sysfs_emit_at()` which is PAGE_SIZE-aware > and recommended for use in sysfs. 'must' is stronger than 'recommendation'. Of has the documentation been changed lately? ... > st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev, > odr_table = &sensor->hw->settings->odr_table[sensor->id]; > for (i = 0; i < odr_table->odr_len; i++) > - len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%03d ", > - odr_table->odr_avl[i].milli_hz / 1000, > - odr_table->odr_avl[i].milli_hz % 1000); > + len += sysfs_emit_at(buf, len, "%d.%03d ", > + odr_table->odr_avl[i].milli_hz / 1000, > + odr_table->odr_avl[i].milli_hz % 1000); > buf[len - 1] = '\n'; My gosh, this is error prone. I'm wondering when some CIs will start to complain on this line. But this was already before your change... > return len; ... > fs_table = &hw->settings->fs_table[sensor->id]; > for (i = 0; i < fs_table->fs_len; i++) > - len += scnprintf(buf + len, PAGE_SIZE - len, "0.%09u ", > - fs_table->fs_avl[i].gain); > + len += sysfs_emit_at(buf, len, "0.%09u ", > + fs_table->fs_avl[i].gain); > buf[len - 1] = '\n'; Ditto. -- With Best Regards, Andy Shevchenko
On Thursday, 3 July 2025 10:12 pm +0530 Andy Shevchenko wrote: > On Thu, Jul 03, 2025 at 11:08:59AM +0530, Akshay Bansod wrote: > > Update the sysfs interface for sampling frequency and scale attributes. > > Replace `scnprintf()` with `sysfs_emit_at()` which is PAGE_SIZE-aware > > and recommended for use in sysfs. > > 'must' is stronger than 'recommendation'. > Of has the documentation been changed lately? > > ... > > > st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev, > > > odr_table = &sensor->hw->settings->odr_table[sensor->id]; > > for (i = 0; i < odr_table->odr_len; i++) > > - len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%03d ", > > - odr_table->odr_avl[i].milli_hz / 1000, > > - odr_table->odr_avl[i].milli_hz % 1000); > > + len += sysfs_emit_at(buf, len, "%d.%03d ", > > + odr_table->odr_avl[i].milli_hz / 1000, > > + odr_table->odr_avl[i].milli_hz % 1000); > > buf[len - 1] = '\n'; > > My gosh, this is error prone. I'm wondering when some CIs will start to > complain on this line. But this was already before your change... > I'm planning to drop It entirely or should I replace it with another `sysfs_emit_at()` ? I've seen other device driver returning space terminated buffers. Maybe I'm overlooking something. > > return len; > > ... > > > fs_table = &hw->settings->fs_table[sensor->id]; > > for (i = 0; i < fs_table->fs_len; i++) > > - len += scnprintf(buf + len, PAGE_SIZE - len, "0.%09u ", > > - fs_table->fs_avl[i].gain); > > + len += sysfs_emit_at(buf, len, "0.%09u ", > > + fs_table->fs_avl[i].gain); > > buf[len - 1] = '\n'; > > Ditto. > > regards, Akshay Bansod
On Thu, 03 Jul 2025 22:28:13 +0530 akshay bansod <akbansd@gmail.com> wrote: > On Thursday, 3 July 2025 10:12 pm +0530 Andy Shevchenko wrote: > > On Thu, Jul 03, 2025 at 11:08:59AM +0530, Akshay Bansod wrote: > > > Update the sysfs interface for sampling frequency and scale attributes. > > > Replace `scnprintf()` with `sysfs_emit_at()` which is PAGE_SIZE-aware > > > and recommended for use in sysfs. > > > > 'must' is stronger than 'recommendation'. > > Of has the documentation been changed lately? > > > > ... > > > > > st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev, > > > > > odr_table = &sensor->hw->settings->odr_table[sensor->id]; > > > for (i = 0; i < odr_table->odr_len; i++) > > > - len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%03d ", > > > - odr_table->odr_avl[i].milli_hz / 1000, > > > - odr_table->odr_avl[i].milli_hz % 1000); > > > + len += sysfs_emit_at(buf, len, "%d.%03d ", > > > + odr_table->odr_avl[i].milli_hz / 1000, > > > + odr_table->odr_avl[i].milli_hz % 1000); > > > buf[len - 1] = '\n'; > > > > My gosh, this is error prone. I'm wondering when some CIs will start to > > complain on this line. But this was already before your change... > > > I'm planning to drop It entirely or should I replace it with another `sysfs_emit_at()` ? > I've seen other device driver returning space terminated buffers. Maybe I'm overlooking > something. It is rather ugly currently but not a bug as such as we know we don't actually run out of space in the page (it would just overwrite last byte in that case so odd output, but not a bug) and that we always print something so just as you suggest sysfs_emit_at(buf, len - 1, "\n"); is safe. It also checks under and overflow so that safe + hopefully won't trip up static analysis tools. > > > > return len; > > > > ... > > > > > fs_table = &hw->settings->fs_table[sensor->id]; > > > for (i = 0; i < fs_table->fs_len; i++) > > > - len += scnprintf(buf + len, PAGE_SIZE - len, "0.%09u ", > > > - fs_table->fs_avl[i].gain); > > > + len += sysfs_emit_at(buf, len, "0.%09u ", > > > + fs_table->fs_avl[i].gain); > > > buf[len - 1] = '\n'; > > > > Ditto. > > > > > > regards, > Akshay Bansod > > > >
On Sunday, 6 July 2025 10:00 am +0530 Jonathan Cameron wrote:
> On Thu, 03 Jul 2025 22:28:13 +0530
> akshay bansod <akbansd@gmail.com> wrote:
>
> > On Thursday, 3 July 2025 10:12 pm +0530 Andy Shevchenko wrote:
> > > On Thu, Jul 03, 2025 at 11:08:59AM +0530, Akshay Bansod wrote:
> > > > Update the sysfs interface for sampling frequency and scale attributes.
> > > > Replace `scnprintf()` with `sysfs_emit_at()` which is PAGE_SIZE-aware
> > > > and recommended for use in sysfs.
> > >
> > > 'must' is stronger than 'recommendation'.
> > > Of has the documentation been changed lately?
> > >
> > > ...
> > >
> > > > st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev,
> > >
> > > > odr_table = &sensor->hw->settings->odr_table[sensor->id];
> > > > for (i = 0; i < odr_table->odr_len; i++)
> > > > - len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%03d ",
> > > > - odr_table->odr_avl[i].milli_hz / 1000,
> > > > - odr_table->odr_avl[i].milli_hz % 1000);
> > > > + len += sysfs_emit_at(buf, len, "%d.%03d ",
> > > > + odr_table->odr_avl[i].milli_hz / 1000,
> > > > + odr_table->odr_avl[i].milli_hz % 1000);
> > > > buf[len - 1] = '\n';
> > >
> > > My gosh, this is error prone. I'm wondering when some CIs will start to
> > > complain on this line. But this was already before your change...
> > >
> > I'm planning to drop It entirely or should I replace it with another `sysfs_emit_at()` ?
> > I've seen other device driver returning space terminated buffers. Maybe I'm overlooking
> > something.
>
> It is rather ugly currently but not a bug as such as we know we don't actually run
> out of space in the page (it would just overwrite last byte in that case so odd
> output, but not a bug) and that we always print something so just as you suggest
> sysfs_emit_at(buf, len - 1, "\n"); is safe. It also checks under and overflow
> so that safe + hopefully won't trip up static analysis tools.
>
understood. I'll revise the patch.
On a sidenode, I see a lot of repetitive code trying to write to a sysfs buffer
from a static array. for example
drivers/iio/common/st_sensors/st_sensors_core.c:629
drivers/iio/adc/vf610_adc.c:614
drivers/iio/accel/adxl372.c:972
...
What if we export a symbol from industrialio-core.c which does something
similar to
drivers/iio/industrialio-core.c:815
'iio_format_avail_list(char *buf, const int *vals,
int type, int length)'
but rather than taking integer array, it take `void* ptr` and `int stride` as
parameter. Then iterates from `vals` by `stride` for `count` times and typecast
the pointer and 'sysfs_emit` it.
static ssize_t iio_format_avail_list(char *buf, void *vals,
int stride, int type, int count) {
// iterate (void*) vals by stride and perform `sysfs_emit`
void* ref = vals;
for(int i = 0; i < count; i++){
ref += stride;
// typecast and write to buf using sysfs_emit
...
}
};
Thus, drivers can use this as follows.
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -618,20 +618,11 @@ EXPORT_SYMBOL_NS(st_sensors_verify_id, "IIO_ST_SENSORS");
ssize_t st_sensors_sysfs_sampling_frequency_avail(struct device *dev,
struct device_attribute *attr, char *buf)
{
- int i, len = 0;
struct iio_dev *indio_dev = dev_to_iio_dev(dev);
struct st_sensor_data *sdata = iio_priv(indio_dev);
- for (i = 0; i < ST_SENSORS_ODR_LIST_MAX; i++) {
- if (sdata->sensor_settings->odr.odr_avl[i].hz == 0)
- break;
-
- len += scnprintf(buf + len, PAGE_SIZE - len, "%d ",
- sdata->sensor_settings->odr.odr_avl[i].hz);
- }
- buf[len - 1] = '\n';
-
- return len;
+ return iio_format_avail_list(buf, &sdata->sensor_settings->odr.odr_avl[0].hz,
+ sizeof(st_sensor_odr_avl), IIO_VAL_INT, ST_SENSORS_ODR_LIST_MAX);
}
The details about the various types to cover is still unclear.
But does this sounds feasible ?
> >
> > > > return len;
> > >
> > > ...
> > >
...
> >
> >
> >
> >
>
Regards,
Akshay Bansod
On Tue, 08 Jul 2025 12:20:04 +0530
akshay bansod <akbansd@gmail.com> wrote:
> On Sunday, 6 July 2025 10:00 am +0530 Jonathan Cameron wrote:
> > On Thu, 03 Jul 2025 22:28:13 +0530
> > akshay bansod <akbansd@gmail.com> wrote:
> >
> > > On Thursday, 3 July 2025 10:12 pm +0530 Andy Shevchenko wrote:
> > > > On Thu, Jul 03, 2025 at 11:08:59AM +0530, Akshay Bansod wrote:
> > > > > Update the sysfs interface for sampling frequency and scale attributes.
> > > > > Replace `scnprintf()` with `sysfs_emit_at()` which is PAGE_SIZE-aware
> > > > > and recommended for use in sysfs.
> > > >
> > > > 'must' is stronger than 'recommendation'.
> > > > Of has the documentation been changed lately?
> > > >
> > > > ...
> > > >
> > > > > st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev,
> > > >
> > > > > odr_table = &sensor->hw->settings->odr_table[sensor->id];
> > > > > for (i = 0; i < odr_table->odr_len; i++)
> > > > > - len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%03d ",
> > > > > - odr_table->odr_avl[i].milli_hz / 1000,
> > > > > - odr_table->odr_avl[i].milli_hz % 1000);
> > > > > + len += sysfs_emit_at(buf, len, "%d.%03d ",
> > > > > + odr_table->odr_avl[i].milli_hz / 1000,
> > > > > + odr_table->odr_avl[i].milli_hz % 1000);
> > > > > buf[len - 1] = '\n';
> > > >
> > > > My gosh, this is error prone. I'm wondering when some CIs will start to
> > > > complain on this line. But this was already before your change...
> > > >
> > > I'm planning to drop It entirely or should I replace it with another `sysfs_emit_at()` ?
> > > I've seen other device driver returning space terminated buffers. Maybe I'm overlooking
> > > something.
> >
> > It is rather ugly currently but not a bug as such as we know we don't actually run
> > out of space in the page (it would just overwrite last byte in that case so odd
> > output, but not a bug) and that we always print something so just as you suggest
> > sysfs_emit_at(buf, len - 1, "\n"); is safe. It also checks under and overflow
> > so that safe + hopefully won't trip up static analysis tools.
> >
> understood. I'll revise the patch.
>
> On a sidenode, I see a lot of repetitive code trying to write to a sysfs buffer
> from a static array. for example
>
> drivers/iio/common/st_sensors/st_sensors_core.c:629
> drivers/iio/adc/vf610_adc.c:614
> drivers/iio/accel/adxl372.c:972
>
It is a more complex conversion but in at least some of these cases
they should really move to using a read_avail callback rather
than a custom attribute.
The internals of that functionality indeed look somewhat like your
suggested function but with added benefit of being useable by
in kernel consumers of the channels and with rigid enforcement of naming etc.
It does need to be done a little carefully though as there are some
messy lifetime issues when in kernel consumers use that data.
Jonathan
> ...
>
> What if we export a symbol from industrialio-core.c which does something
> similar to
>
> drivers/iio/industrialio-core.c:815
>
> 'iio_format_avail_list(char *buf, const int *vals,
> int type, int length)'
>
>
> but rather than taking integer array, it take `void* ptr` and `int stride` as
> parameter. Then iterates from `vals` by `stride` for `count` times and typecast
> the pointer and 'sysfs_emit` it.
>
> static ssize_t iio_format_avail_list(char *buf, void *vals,
> int stride, int type, int count) {
>
> // iterate (void*) vals by stride and perform `sysfs_emit`
>
> void* ref = vals;
> for(int i = 0; i < count; i++){
>
> ref += stride;
>
> // typecast and write to buf using sysfs_emit
> ...
>
> }
> };
>
>
> Thus, drivers can use this as follows.
>
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -618,20 +618,11 @@ EXPORT_SYMBOL_NS(st_sensors_verify_id, "IIO_ST_SENSORS");
> ssize_t st_sensors_sysfs_sampling_frequency_avail(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - int i, len = 0;
> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> struct st_sensor_data *sdata = iio_priv(indio_dev);
>
> - for (i = 0; i < ST_SENSORS_ODR_LIST_MAX; i++) {
> - if (sdata->sensor_settings->odr.odr_avl[i].hz == 0)
> - break;
> -
> - len += scnprintf(buf + len, PAGE_SIZE - len, "%d ",
> - sdata->sensor_settings->odr.odr_avl[i].hz);
> - }
> - buf[len - 1] = '\n';
> -
> - return len;
> + return iio_format_avail_list(buf, &sdata->sensor_settings->odr.odr_avl[0].hz,
> + sizeof(st_sensor_odr_avl), IIO_VAL_INT, ST_SENSORS_ODR_LIST_MAX);
> }
>
> The details about the various types to cover is still unclear.
> But does this sounds feasible ?
>
> > >
> > > > > return len;
> > > >
> > > > ...
> > > >
>
> ...
>
> > >
> > >
> > >
> > >
> >
>
> Regards,
> Akshay Bansod
>
>
>
>
>
© 2016 - 2025 Red Hat, Inc.