Message-ID: <20250709151552.6ccde08b@jic23-huawei>
Date: Wed, 9 Jul 2025 15:15:52 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: akshay bansod <akbansd@...il.com>
Cc: Andy Shevchenko <andriy.shevchenko@...el.com>, Lorenzo Bianconi
 <lorenzo@...nel.org>, David Lechner <dlechner@...libre.com>, Nuno
 Sá <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
 linux-kernel-mentees@...ts.linuxfoundation.org, skhan@...uxfoundation.org,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] iio: st_lsm6dsx: Replace scnprintf with sysfs_emit

On Tue, 08 Jul 2025 12:20:04 +0530
akshay bansod <akbansd@...il.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@...il.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
> 
> 
> 
> 
> 

Powered by blists - more mailing lists