* [PATCH v2] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit @ 2025-07-03 5:38 Akshay Bansod 2025-07-03 13:40 ` Andy Shevchenko 0 siblings, 1 reply; 6+ messages in thread From: Akshay Bansod @ 2025-07-03 5:38 UTC (permalink / raw) To: Lorenzo Bianconi, Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko Cc: linux-kernel-mentees, skhan, linux-iio, linux-kernel 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.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit 2025-07-03 5:38 [PATCH v2] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit Akshay Bansod @ 2025-07-03 13:40 ` Andy Shevchenko 2025-07-03 16:58 ` akshay bansod 0 siblings, 1 reply; 6+ messages in thread From: Andy Shevchenko @ 2025-07-03 13:40 UTC (permalink / raw) To: Akshay Bansod Cc: Lorenzo Bianconi, Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, linux-kernel-mentees, skhan, linux-iio, linux-kernel 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... > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit 2025-07-03 13:40 ` Andy Shevchenko @ 2025-07-03 16:58 ` akshay bansod 2025-07-06 10:50 ` Jonathan Cameron 0 siblings, 1 reply; 6+ messages in thread From: akshay bansod @ 2025-07-03 16:58 UTC (permalink / raw) To: Andy Shevchenko Cc: Lorenzo Bianconi, Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, linux-kernel-mentees, skhan, linux-iio, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit 2025-07-03 16:58 ` akshay bansod @ 2025-07-06 10:50 ` Jonathan Cameron 2025-07-08 6:50 ` akshay bansod 0 siblings, 1 reply; 6+ messages in thread From: Jonathan Cameron @ 2025-07-06 10:50 UTC (permalink / raw) To: akshay bansod Cc: Andy Shevchenko, Lorenzo Bianconi, David Lechner, Nuno Sá, Andy Shevchenko, linux-kernel-mentees, skhan, linux-iio, linux-kernel 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 > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit 2025-07-06 10:50 ` Jonathan Cameron @ 2025-07-08 6:50 ` akshay bansod 2025-07-09 14:15 ` Jonathan Cameron 0 siblings, 1 reply; 6+ messages in thread From: akshay bansod @ 2025-07-08 6:50 UTC (permalink / raw) To: Jonathan Cameron Cc: Andy Shevchenko, Lorenzo Bianconi, David Lechner, Nuno Sá, Andy Shevchenko, linux-kernel-mentees, skhan, linux-iio, linux-kernel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit 2025-07-08 6:50 ` akshay bansod @ 2025-07-09 14:15 ` Jonathan Cameron 0 siblings, 0 replies; 6+ messages in thread From: Jonathan Cameron @ 2025-07-09 14:15 UTC (permalink / raw) To: akshay bansod Cc: Andy Shevchenko, Lorenzo Bianconi, David Lechner, Nuno Sá, Andy Shevchenko, linux-kernel-mentees, skhan, linux-iio, linux-kernel 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 > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-07-09 14:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-07-03 5:38 [PATCH v2] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit Akshay Bansod 2025-07-03 13:40 ` Andy Shevchenko 2025-07-03 16:58 ` akshay bansod 2025-07-06 10:50 ` Jonathan Cameron 2025-07-08 6:50 ` akshay bansod 2025-07-09 14:15 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).