* [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).