iio: st_lsm6dsx: Replace scnprintf with sysfs_emit

PATCH Superseded Akshay Bansod 3 participants 6 messages Jul 3, 2025 – Jul 9, 2025

Subsystems (2)

IIO ST LSM6DSx IMU IIO driver

lore

Share & tools

lore

Akshay Bansod

author

2025-07-03 05:39 UTC #0 lore raw

PATCH v2 1/1 iio: st_lsm6dsx: Replace scnprintf with sysfs_emit

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 <[email protected]>

---
changes in v2:
- Fixed indentation for line wrap
- Link to v1: https://lore.kernel.org/linux-iio/[email protected]/

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(-)

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +5 -5

index c65ad4982..7689ca39a 100644
@@ -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;

Signature · 1 line

2.49.0

Andy Shevchenko

2025-07-03 13:40 UTC #1 lore raw

Quoted 3 lines · Akshay Bansod

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?

...

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev, · line 2041

Quoted 11 lines

st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev,

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +3 -3

 	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...

Quoted 1 line

return len;

...

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev, · line 2059

Quoted 7 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +2 -2

 	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.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

akshay bansod

author

2025-07-03 16:58 UTC #2 lore raw

Quoted 12 lines · On Thursday, 3 July 2025 10:12 pm +0530 Andy Shevchenko wrote:

Quoted 3 lines · Akshay Bansod
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?
...
Quoted 1 line
st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev,

Quoted 13 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev, · line 2041
Quoted 9 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +3 -3
 	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.

Quoted 4 lines

Quoted 1 line
return len;
...

Quoted 11 lines

drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev, · line 2059
Quoted 7 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +2 -2
 	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

Jonathan Cameron

maintainer

2025-07-06 10:50 UTC #3 lore raw

Quoted 26 lines · On Thu, 03 Jul 2025 22:28:13 +0530 akshay bansod <[email protected]> wrote:

Quoted 12 lines · On Thursday, 3 July 2025 10:12 pm +0530 Andy Shevchenko wrote:
Quoted 3 lines · Akshay Bansod
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?
...
Quoted 1 line
st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev,
Quoted 13 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev, · line 2041
Quoted 9 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +3 -3
 	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...

Quoted 3 lines

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.

Quoted 23 lines

Quoted 5 lines
Quoted 2 lines
return len;
...
Quoted 11 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c static ssize_t st_lsm6dsx_sysfs_scale_avail(struct device *dev, · line 2059
Quoted 7 lines
drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c +2 -2
 	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

akshay bansod

author

2025-07-08 06:50 UTC #4 lore raw

Quoted 33 lines · On Sunday, 6 July 2025 10:00 am +0530 Jonathan Cameron wrote:

Quoted 26 lines · On Thu, 03 Jul 2025 22:28:13 +0530 akshay bansod <[email protected]> wrote:
Quoted 12 lines · On Thursday, 3 July 2025 10:12 pm +0530 Andy Shevchenko wrote:
Quoted 3 lines · Akshay Bansod
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?
...
Quoted 1 line
st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev,
Quoted 13 lines
Review
Quoted 9 lines
(quoted diff) +3 -3
 	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...
Quoted 3 lines
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.

Quoted 6 lines

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.

drivers/iio/common/st_sensors/st_sensors_core.c +2 ~~-11~~

@@ -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 ?

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
return len;
...

...

Quoted 5 lines

Quoted 4 lines

Regards,
Akshay Bansod

Jonathan Cameron

maintainer

2025-07-09 14:15 UTC #5 lore raw

Quoted 40 lines · On Tue, 08 Jul 2025 12:20:04 +0530 akshay bansod <[email protected]> wrote:

Quoted 33 lines · On Sunday, 6 July 2025 10:00 am +0530 Jonathan Cameron wrote:
Quoted 26 lines · On Thu, 03 Jul 2025 22:28:13 +0530 akshay bansod <[email protected]> wrote:
Quoted 12 lines · On Thursday, 3 July 2025 10:12 pm +0530 Andy Shevchenko wrote:
Quoted 3 lines · Akshay Bansod
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?
...
Quoted 1 line
st_lsm6dsx_sysfs_sampling_frequency_avail(struct device *dev,
Quoted 13 lines
Review
Quoted 9 lines
(quoted diff) +3 -3
 	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...
Quoted 3 lines
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.
Quoted 6 lines
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.

Quoted 9 lines

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

Quoted 82 lines

...
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.
drivers/iio/common/st_sensors/st_sensors_core.c +2 ~~-11~~
@@ -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 ?
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
return len;
...
...
Quoted 5 lines
Quoted 4 lines
Regards,
Akshay Bansod