[PATCH v2 0/4] iio: mark scan_timestamp as __private

[PATCH v2 0/4] iio: mark scan_timestamp as __private

[Vasileios Amoiridis]

Changes in v2:

- Droped patches 1,2.
- DLN2-ADC: zero full struct instead of just the padding
- MAX1363: add the data buffer in the iio_priv()
- SSP_IIO: calculate always the timestamp
- iio_push_to_buffers_with_timestamp(): mark the access of the
  indio_dev->scan_timestamp as ACCESS_PRIVATE()

---
v1: https://lore.kernel.org/linux-iio/20241130002710.18615-1-vassilisamir@gmail.com/

The scan_timestamp value of the struct iio_dev, even though is an
internal variable, it is being used in some drivers. To avoid any
unwanted overwrites of this value, create a getter and when all the
drivers are converted, mark the variable as __private.

The patch is an RFC because the added value might not be considered
high enough by someone to be implemented and/or it might need to be
done in a different way since it touches multiple drivers.
Vasileios Amoiridis (4):
  iio: adc: dln2-adc: zero full struct instead of just the padding
  iio: adc: max1363: make use of iio_is_soft_ts_enabled()
  iio: common: ssp_sensors: drop conditional optimization for simplicity
  iio: core: mark scan_timestamp as __private

 drivers/iio/adc/dln2-adc.c               | 21 ++---------------
 drivers/iio/adc/max1363.c                | 30 +++++++-----------------
 drivers/iio/common/ssp_sensors/ssp_iio.c |  9 +++----
 drivers/iio/industrialio-buffer.c        |  2 +-
 include/linux/iio/buffer.h               |  2 +-
 include/linux/iio/iio.h                  |  2 +-
 6 files changed, 17 insertions(+), 49 deletions(-)


base-commit: a3fb9f5202c3de0ca84848a475f59a0e0584d9fc
-- 
2.43.0

[PATCH v2 1/4] iio: adc: dln2-adc: zero full struct instead of just the padding

[Vasileios Amoiridis]

Drop a minor optimization of zeroing the padding between data and
timestamp and zero the whole structure. This is done in favor of
simpler code, and in order to drop the usage of the internal private
variable "scan_timestamp" of the struct iio_dev.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/adc/dln2-adc.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/adc/dln2-adc.c b/drivers/iio/adc/dln2-adc.c
index 30328626d9be..221a5fdc1eaa 100644
--- a/drivers/iio/adc/dln2-adc.c
+++ b/drivers/iio/adc/dln2-adc.c
@@ -66,8 +66,6 @@ struct dln2_adc {
 	/* Demux table */
 	unsigned int demux_count;
 	struct dln2_adc_demux_table demux[DLN2_ADC_MAX_CHANNELS];
-	/* Precomputed timestamp padding offset and length */
-	unsigned int ts_pad_offset, ts_pad_length;
 };
 
 struct dln2_adc_port_chan {
@@ -111,8 +109,6 @@ static void dln2_adc_update_demux(struct dln2_adc *dln2)
 	if (iio_get_masklength(indio_dev) &&
 	    (*indio_dev->active_scan_mask & 0xff) == 0xff) {
 		dln2_adc_add_demux(dln2, 0, 0, 16);
-		dln2->ts_pad_offset = 0;
-		dln2->ts_pad_length = 0;
 		return;
 	}
 
@@ -127,16 +123,6 @@ static void dln2_adc_update_demux(struct dln2_adc *dln2)
 		out_loc += 2;
 		in_loc += 2;
 	}
-
-	if (indio_dev->scan_timestamp) {
-		size_t ts_offset = indio_dev->scan_bytes / sizeof(int64_t) - 1;
-
-		dln2->ts_pad_offset = out_loc;
-		dln2->ts_pad_length = ts_offset * sizeof(int64_t) - out_loc;
-	} else {
-		dln2->ts_pad_offset = 0;
-		dln2->ts_pad_length = 0;
-	}
 }
 
 static int dln2_adc_get_chan_count(struct dln2_adc *dln2)
@@ -494,6 +480,8 @@ static irqreturn_t dln2_adc_trigger_h(int irq, void *p)
 	if (ret < 0)
 		goto done;
 
+	memset(&data, 0, sizeof(data));
+
 	/* Demux operation */
 	for (i = 0; i < dln2->demux_count; ++i) {
 		t = &dln2->demux[i];
@@ -501,11 +489,6 @@ static irqreturn_t dln2_adc_trigger_h(int irq, void *p)
 		       (void *)dev_data.values + t->from, t->length);
 	}
 
-	/* Zero padding space between values and timestamp */
-	if (dln2->ts_pad_length)
-		memset((void *)data.values + dln2->ts_pad_offset,
-		       0, dln2->ts_pad_length);
-
 	iio_push_to_buffers_with_timestamp(indio_dev, &data,
 					   iio_get_time_ns(indio_dev));
 
-- 
2.43.0

Re: [PATCH v2 1/4] iio: adc: dln2-adc: zero full struct instead of just the padding

[Jonathan Cameron]

On Sat, 14 Dec 2024 20:14:18 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Drop a minor optimization of zeroing the padding between data and
> timestamp and zero the whole structure. This is done in favor of
> simpler code, and in order to drop the usage of the internal private
> variable "scan_timestamp" of the struct iio_dev.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
Applied.

thanks,

Jonathan

> ---
>  drivers/iio/adc/dln2-adc.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/adc/dln2-adc.c b/drivers/iio/adc/dln2-adc.c
> index 30328626d9be..221a5fdc1eaa 100644
> --- a/drivers/iio/adc/dln2-adc.c
> +++ b/drivers/iio/adc/dln2-adc.c
> @@ -66,8 +66,6 @@ struct dln2_adc {
>  	/* Demux table */
>  	unsigned int demux_count;
>  	struct dln2_adc_demux_table demux[DLN2_ADC_MAX_CHANNELS];
> -	/* Precomputed timestamp padding offset and length */
> -	unsigned int ts_pad_offset, ts_pad_length;
>  };
>  
>  struct dln2_adc_port_chan {
> @@ -111,8 +109,6 @@ static void dln2_adc_update_demux(struct dln2_adc *dln2)
>  	if (iio_get_masklength(indio_dev) &&
>  	    (*indio_dev->active_scan_mask & 0xff) == 0xff) {
>  		dln2_adc_add_demux(dln2, 0, 0, 16);
> -		dln2->ts_pad_offset = 0;
> -		dln2->ts_pad_length = 0;
>  		return;
>  	}
>  
> @@ -127,16 +123,6 @@ static void dln2_adc_update_demux(struct dln2_adc *dln2)
>  		out_loc += 2;
>  		in_loc += 2;
>  	}
> -
> -	if (indio_dev->scan_timestamp) {
> -		size_t ts_offset = indio_dev->scan_bytes / sizeof(int64_t) - 1;
> -
> -		dln2->ts_pad_offset = out_loc;
> -		dln2->ts_pad_length = ts_offset * sizeof(int64_t) - out_loc;
> -	} else {
> -		dln2->ts_pad_offset = 0;
> -		dln2->ts_pad_length = 0;
> -	}
>  }
>  
>  static int dln2_adc_get_chan_count(struct dln2_adc *dln2)
> @@ -494,6 +480,8 @@ static irqreturn_t dln2_adc_trigger_h(int irq, void *p)
>  	if (ret < 0)
>  		goto done;
>  
> +	memset(&data, 0, sizeof(data));
> +
>  	/* Demux operation */
>  	for (i = 0; i < dln2->demux_count; ++i) {
>  		t = &dln2->demux[i];
> @@ -501,11 +489,6 @@ static irqreturn_t dln2_adc_trigger_h(int irq, void *p)
>  		       (void *)dev_data.values + t->from, t->length);
>  	}
>  
> -	/* Zero padding space between values and timestamp */
> -	if (dln2->ts_pad_length)
> -		memset((void *)data.values + dln2->ts_pad_offset,
> -		       0, dln2->ts_pad_length);
> -
>  	iio_push_to_buffers_with_timestamp(indio_dev, &data,
>  					   iio_get_time_ns(indio_dev));
>  

[PATCH v2 2/4] iio: adc: max1363: make use of iio_is_soft_ts_enabled()

[Vasileios Amoiridis]

Drop the recurrent allocation of the data buffer from the trigger
handler and put it in the iio_priv(). This way, the maximum amount of
channels is always allocated in favor of simpler code and drop
of usage of the internal private variable "scan_timestamp" of the
struct iio_dev.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/adc/max1363.c | 30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index 9a0baea08ab6..e8d731bc34e0 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -161,6 +161,7 @@ struct max1363_chip_info {
  * @vref_uv:		Actual (external or internal) reference voltage
  * @send:		function used to send data to the chip
  * @recv:		function used to receive data from the chip
+ * @data:		buffer to store channel data and timestamp
  */
 struct max1363_state {
 	struct i2c_client		*client;
@@ -186,6 +187,10 @@ struct max1363_state {
 						const char *buf, int count);
 	int				(*recv)(const struct i2c_client *client,
 						char *buf, int count);
+	struct {
+		u8 buf[MAX1363_MAX_CHANNELS * 2];
+		aligned_s64 ts;
+	} data;
 };
 
 #define MAX1363_MODE_SINGLE(_num, _mask) {				\
@@ -1462,22 +1467,10 @@ static irqreturn_t max1363_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct max1363_state *st = iio_priv(indio_dev);
-	__u8 *rxbuf;
 	int b_sent;
-	size_t d_size;
 	unsigned long numvals = bitmap_weight(st->current_mode->modemask,
 					      MAX1363_MAX_CHANNELS);
 
-	/* Ensure the timestamp is 8 byte aligned */
-	if (st->chip_info->bits != 8)
-		d_size = numvals*2;
-	else
-		d_size = numvals;
-	if (indio_dev->scan_timestamp) {
-		d_size += sizeof(s64);
-		if (d_size % sizeof(s64))
-			d_size += sizeof(s64) - (d_size % sizeof(s64));
-	}
 	/* Monitor mode prevents reading. Whilst not currently implemented
 	 * might as well have this test in here in the meantime as it does
 	 * no harm.
@@ -1485,21 +1478,16 @@ static irqreturn_t max1363_trigger_handler(int irq, void *p)
 	if (numvals == 0)
 		goto done;
 
-	rxbuf = kmalloc(d_size,	GFP_KERNEL);
-	if (rxbuf == NULL)
-		goto done;
 	if (st->chip_info->bits != 8)
-		b_sent = st->recv(st->client, rxbuf, numvals * 2);
+		b_sent = st->recv(st->client, st->data.buf, numvals * 2);
 	else
-		b_sent = st->recv(st->client, rxbuf, numvals);
+		b_sent = st->recv(st->client, st->data.buf, numvals);
 	if (b_sent < 0)
-		goto done_free;
+		goto done;
 
-	iio_push_to_buffers_with_timestamp(indio_dev, rxbuf,
+	iio_push_to_buffers_with_timestamp(indio_dev, &st->data,
 					   iio_get_time_ns(indio_dev));
 
-done_free:
-	kfree(rxbuf);
 done:
 	iio_trigger_notify_done(indio_dev->trig);
 
-- 
2.43.0

Re: [PATCH v2 2/4] iio: adc: max1363: make use of iio_is_soft_ts_enabled()

[Jonathan Cameron]

On Sat, 14 Dec 2024 20:14:19 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

Patch needs a new title.  I'll fix it up.

Use a small fixed size buffer to replace dynamic allocation

with that, applied.

Thanks,


> Drop the recurrent allocation of the data buffer from the trigger
> handler and put it in the iio_priv(). This way, the maximum amount of
> channels is always allocated in favor of simpler code and drop
> of usage of the internal private variable "scan_timestamp" of the
> struct iio_dev.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/adc/max1363.c | 30 +++++++++---------------------
>  1 file changed, 9 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> index 9a0baea08ab6..e8d731bc34e0 100644
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c
> @@ -161,6 +161,7 @@ struct max1363_chip_info {
>   * @vref_uv:		Actual (external or internal) reference voltage
>   * @send:		function used to send data to the chip
>   * @recv:		function used to receive data from the chip
> + * @data:		buffer to store channel data and timestamp
>   */
>  struct max1363_state {
>  	struct i2c_client		*client;
> @@ -186,6 +187,10 @@ struct max1363_state {
>  						const char *buf, int count);
>  	int				(*recv)(const struct i2c_client *client,
>  						char *buf, int count);
> +	struct {
> +		u8 buf[MAX1363_MAX_CHANNELS * 2];
> +		aligned_s64 ts;
> +	} data;
>  };
>  
>  #define MAX1363_MODE_SINGLE(_num, _mask) {				\
> @@ -1462,22 +1467,10 @@ static irqreturn_t max1363_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct max1363_state *st = iio_priv(indio_dev);
> -	__u8 *rxbuf;
>  	int b_sent;
> -	size_t d_size;
>  	unsigned long numvals = bitmap_weight(st->current_mode->modemask,
>  					      MAX1363_MAX_CHANNELS);
>  
> -	/* Ensure the timestamp is 8 byte aligned */
> -	if (st->chip_info->bits != 8)
> -		d_size = numvals*2;
> -	else
> -		d_size = numvals;
> -	if (indio_dev->scan_timestamp) {
> -		d_size += sizeof(s64);
> -		if (d_size % sizeof(s64))
> -			d_size += sizeof(s64) - (d_size % sizeof(s64));
> -	}
>  	/* Monitor mode prevents reading. Whilst not currently implemented
>  	 * might as well have this test in here in the meantime as it does
>  	 * no harm.
> @@ -1485,21 +1478,16 @@ static irqreturn_t max1363_trigger_handler(int irq, void *p)
>  	if (numvals == 0)
>  		goto done;
>  
> -	rxbuf = kmalloc(d_size,	GFP_KERNEL);
> -	if (rxbuf == NULL)
> -		goto done;
>  	if (st->chip_info->bits != 8)
> -		b_sent = st->recv(st->client, rxbuf, numvals * 2);
> +		b_sent = st->recv(st->client, st->data.buf, numvals * 2);
>  	else
> -		b_sent = st->recv(st->client, rxbuf, numvals);
> +		b_sent = st->recv(st->client, st->data.buf, numvals);
>  	if (b_sent < 0)
> -		goto done_free;
> +		goto done;
>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, rxbuf,
> +	iio_push_to_buffers_with_timestamp(indio_dev, &st->data,
>  					   iio_get_time_ns(indio_dev));
>  
> -done_free:
> -	kfree(rxbuf);
>  done:
>  	iio_trigger_notify_done(indio_dev->trig);
>  

[PATCH v2 3/4] iio: common: ssp_sensors: drop conditional optimization for simplicity

[Vasileios Amoiridis]

Drop conditional in favor of always calculating the timestamp value.
This simplifies the code and allows to drop usage of internal private
variable "scan_timestamp" of the struct iio_dev.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/common/ssp_sensors/ssp_iio.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/common/ssp_sensors/ssp_iio.c b/drivers/iio/common/ssp_sensors/ssp_iio.c
index caa404edd9d0..6b86b5315694 100644
--- a/drivers/iio/common/ssp_sensors/ssp_iio.c
+++ b/drivers/iio/common/ssp_sensors/ssp_iio.c
@@ -8,6 +8,8 @@
 #include <linux/iio/kfifo_buf.h>
 #include <linux/module.h>
 #include <linux/slab.h>
+#include <linux/unaligned.h>
+#include <linux/units.h>
 #include "ssp_iio_sensor.h"
 
 /**
@@ -70,7 +72,6 @@ EXPORT_SYMBOL_NS(ssp_common_buffer_postdisable, "IIO_SSP_SENSORS");
 int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
 			    unsigned int len, int64_t timestamp)
 {
-	__le32 time;
 	int64_t calculated_time = 0;
 	struct ssp_sensor_data *spd = iio_priv(indio_dev);
 
@@ -82,11 +83,7 @@ int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
 	 */
 	memcpy(spd->buffer, buf, len);
 
-	if (indio_dev->scan_timestamp) {
-		memcpy(&time, &((char *)buf)[len], SSP_TIME_SIZE);
-		calculated_time =
-			timestamp + (int64_t)le32_to_cpu(time) * 1000000;
-	}
+	calculated_time = timestamp + get_unaligned_le32(buf + len) * MEGA;
 
 	return iio_push_to_buffers_with_timestamp(indio_dev, spd->buffer,
 						  calculated_time);
-- 
2.43.0

Re: [PATCH v2 3/4] iio: common: ssp_sensors: drop conditional optimization for simplicity

[David Lechner]

On 12/14/24 1:14 PM, Vasileios Amoiridis wrote:
> Drop conditional in favor of always calculating the timestamp value.
> This simplifies the code and allows to drop usage of internal private
> variable "scan_timestamp" of the struct iio_dev.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> ---
>  drivers/iio/common/ssp_sensors/ssp_iio.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/common/ssp_sensors/ssp_iio.c b/drivers/iio/common/ssp_sensors/ssp_iio.c
> index caa404edd9d0..6b86b5315694 100644
> --- a/drivers/iio/common/ssp_sensors/ssp_iio.c
> +++ b/drivers/iio/common/ssp_sensors/ssp_iio.c
> @@ -8,6 +8,8 @@
>  #include <linux/iio/kfifo_buf.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
>  #include "ssp_iio_sensor.h"
>  
>  /**
> @@ -70,7 +72,6 @@ EXPORT_SYMBOL_NS(ssp_common_buffer_postdisable, "IIO_SSP_SENSORS");
>  int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
>  			    unsigned int len, int64_t timestamp)
>  {
> -	__le32 time;
>  	int64_t calculated_time = 0;
>  	struct ssp_sensor_data *spd = iio_priv(indio_dev);
>  
> @@ -82,11 +83,7 @@ int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
>  	 */
>  	memcpy(spd->buffer, buf, len);
>  
> -	if (indio_dev->scan_timestamp) {
> -		memcpy(&time, &((char *)buf)[len], SSP_TIME_SIZE);
> -		calculated_time =
> -			timestamp + (int64_t)le32_to_cpu(time) * 1000000;
> -	}
> +	calculated_time = timestamp + get_unaligned_le32(buf + len) * MEGA;

Don't we still need to cast to 64 bit to avoid multiplication overflow?

>  
>  	return iio_push_to_buffers_with_timestamp(indio_dev, spd->buffer,
>  						  calculated_time);

Re: [PATCH v2 3/4] iio: common: ssp_sensors: drop conditional optimization for simplicity

[Vasileios Amoiridis]

On Mon, Dec 16, 2024 at 03:57:44PM -0600, David Lechner wrote:
> On 12/14/24 1:14 PM, Vasileios Amoiridis wrote:
> > Drop conditional in favor of always calculating the timestamp value.
> > This simplifies the code and allows to drop usage of internal private
> > variable "scan_timestamp" of the struct iio_dev.
> > 
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > ---
> >  drivers/iio/common/ssp_sensors/ssp_iio.c | 9 +++------
> >  1 file changed, 3 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/common/ssp_sensors/ssp_iio.c b/drivers/iio/common/ssp_sensors/ssp_iio.c
> > index caa404edd9d0..6b86b5315694 100644
> > --- a/drivers/iio/common/ssp_sensors/ssp_iio.c
> > +++ b/drivers/iio/common/ssp_sensors/ssp_iio.c
> > @@ -8,6 +8,8 @@
> >  #include <linux/iio/kfifo_buf.h>
> >  #include <linux/module.h>
> >  #include <linux/slab.h>
> > +#include <linux/unaligned.h>
> > +#include <linux/units.h>
> >  #include "ssp_iio_sensor.h"
> >  
> >  /**
> > @@ -70,7 +72,6 @@ EXPORT_SYMBOL_NS(ssp_common_buffer_postdisable, "IIO_SSP_SENSORS");
> >  int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
> >  			    unsigned int len, int64_t timestamp)
> >  {
> > -	__le32 time;
> >  	int64_t calculated_time = 0;
> >  	struct ssp_sensor_data *spd = iio_priv(indio_dev);
> >  
> > @@ -82,11 +83,7 @@ int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
> >  	 */
> >  	memcpy(spd->buffer, buf, len);
> >  
> > -	if (indio_dev->scan_timestamp) {
> > -		memcpy(&time, &((char *)buf)[len], SSP_TIME_SIZE);
> > -		calculated_time =
> > -			timestamp + (int64_t)le32_to_cpu(time) * 1000000;
> > -	}
> > +	calculated_time = timestamp + get_unaligned_le32(buf + len) * MEGA;
> 
> Don't we still need to cast to 64 bit to avoid multiplication overflow?
>

Hi David,

Thanks for your message!

Aren't we already covered by the fact that MEGA is defined as an
unsigned long?

	include/linux/units.h:12:#define MEGA 1000000UL

Cheers,
Vasilis

> >  
> >  	return iio_push_to_buffers_with_timestamp(indio_dev, spd->buffer,
> >  						  calculated_time);
> 

Re: [PATCH v2 3/4] iio: common: ssp_sensors: drop conditional optimization for simplicity

[David Lechner]

On 12/17/24 5:41 PM, Vasileios Amoiridis wrote:
> On Mon, Dec 16, 2024 at 03:57:44PM -0600, David Lechner wrote:
>> On 12/14/24 1:14 PM, Vasileios Amoiridis wrote:
>>> Drop conditional in favor of always calculating the timestamp value.
>>> This simplifies the code and allows to drop usage of internal private
>>> variable "scan_timestamp" of the struct iio_dev.
>>>
>>> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
>>> ---
>>>  drivers/iio/common/ssp_sensors/ssp_iio.c | 9 +++------
>>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/iio/common/ssp_sensors/ssp_iio.c b/drivers/iio/common/ssp_sensors/ssp_iio.c
>>> index caa404edd9d0..6b86b5315694 100644
>>> --- a/drivers/iio/common/ssp_sensors/ssp_iio.c
>>> +++ b/drivers/iio/common/ssp_sensors/ssp_iio.c
>>> @@ -8,6 +8,8 @@
>>>  #include <linux/iio/kfifo_buf.h>
>>>  #include <linux/module.h>
>>>  #include <linux/slab.h>
>>> +#include <linux/unaligned.h>
>>> +#include <linux/units.h>
>>>  #include "ssp_iio_sensor.h"
>>>  
>>>  /**
>>> @@ -70,7 +72,6 @@ EXPORT_SYMBOL_NS(ssp_common_buffer_postdisable, "IIO_SSP_SENSORS");
>>>  int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
>>>  			    unsigned int len, int64_t timestamp)
>>>  {
>>> -	__le32 time;
>>>  	int64_t calculated_time = 0;
>>>  	struct ssp_sensor_data *spd = iio_priv(indio_dev);
>>>  
>>> @@ -82,11 +83,7 @@ int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
>>>  	 */
>>>  	memcpy(spd->buffer, buf, len);
>>>  
>>> -	if (indio_dev->scan_timestamp) {
>>> -		memcpy(&time, &((char *)buf)[len], SSP_TIME_SIZE);
>>> -		calculated_time =
>>> -			timestamp + (int64_t)le32_to_cpu(time) * 1000000;
>>> -	}
>>> +	calculated_time = timestamp + get_unaligned_le32(buf + len) * MEGA;
>>
>> Don't we still need to cast to 64 bit to avoid multiplication overflow?
>>
> 
> Hi David,
> 
> Thanks for your message!
> 
> Aren't we already covered by the fact that MEGA is defined as an
> unsigned long?

That is only 64-bits on 64-bit architectures, so could still overflow on
32-bit architectures where long is 32-bit.

> 
> 	include/linux/units.h:12:#define MEGA 1000000UL
> 
> Cheers,
> Vasilis
> 
>>>  
>>>  	return iio_push_to_buffers_with_timestamp(indio_dev, spd->buffer,
>>>  						  calculated_time);
>>

Re: [PATCH v2 3/4] iio: common: ssp_sensors: drop conditional optimization for simplicity

[Vasileios Amoiridis]

On Wed, Dec 18, 2024 at 09:17:44AM -0600, David Lechner wrote:
> On 12/17/24 5:41 PM, Vasileios Amoiridis wrote:
> > On Mon, Dec 16, 2024 at 03:57:44PM -0600, David Lechner wrote:
> >> On 12/14/24 1:14 PM, Vasileios Amoiridis wrote:
> >>> Drop conditional in favor of always calculating the timestamp value.
> >>> This simplifies the code and allows to drop usage of internal private
> >>> variable "scan_timestamp" of the struct iio_dev.
> >>>
> >>> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> >>> ---
> >>>  drivers/iio/common/ssp_sensors/ssp_iio.c | 9 +++------
> >>>  1 file changed, 3 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/common/ssp_sensors/ssp_iio.c b/drivers/iio/common/ssp_sensors/ssp_iio.c
> >>> index caa404edd9d0..6b86b5315694 100644
> >>> --- a/drivers/iio/common/ssp_sensors/ssp_iio.c
> >>> +++ b/drivers/iio/common/ssp_sensors/ssp_iio.c
> >>> @@ -8,6 +8,8 @@
> >>>  #include <linux/iio/kfifo_buf.h>
> >>>  #include <linux/module.h>
> >>>  #include <linux/slab.h>
> >>> +#include <linux/unaligned.h>
> >>> +#include <linux/units.h>
> >>>  #include "ssp_iio_sensor.h"
> >>>  
> >>>  /**
> >>> @@ -70,7 +72,6 @@ EXPORT_SYMBOL_NS(ssp_common_buffer_postdisable, "IIO_SSP_SENSORS");
> >>>  int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
> >>>  			    unsigned int len, int64_t timestamp)
> >>>  {
> >>> -	__le32 time;
> >>>  	int64_t calculated_time = 0;
> >>>  	struct ssp_sensor_data *spd = iio_priv(indio_dev);
> >>>  
> >>> @@ -82,11 +83,7 @@ int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
> >>>  	 */
> >>>  	memcpy(spd->buffer, buf, len);
> >>>  
> >>> -	if (indio_dev->scan_timestamp) {
> >>> -		memcpy(&time, &((char *)buf)[len], SSP_TIME_SIZE);
> >>> -		calculated_time =
> >>> -			timestamp + (int64_t)le32_to_cpu(time) * 1000000;
> >>> -	}
> >>> +	calculated_time = timestamp + get_unaligned_le32(buf + len) * MEGA;
> >>
> >> Don't we still need to cast to 64 bit to avoid multiplication overflow?
> >>
> > 
> > Hi David,
> > 
> > Thanks for your message!
> > 
> > Aren't we already covered by the fact that MEGA is defined as an
> > unsigned long?
> 
> That is only 64-bits on 64-bit architectures, so could still overflow on
> 32-bit architectures where long is 32-bit.
> 

Hi David,

Hmmm, I think you are right. I can fix it in next, iteration. I will
wait also for Jonathan's comments on the rest of the series.

Cheers,
Vasilis

> > 
> > 	include/linux/units.h:12:#define MEGA 1000000UL
> > 
> > Cheers,
> > Vasilis
> > 
> >>>  
> >>>  	return iio_push_to_buffers_with_timestamp(indio_dev, spd->buffer,
> >>>  						  calculated_time);
> >>
> 

Re: [PATCH v2 3/4] iio: common: ssp_sensors: drop conditional optimization for simplicity

[Jonathan Cameron]

On Wed, 18 Dec 2024 22:13:55 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> On Wed, Dec 18, 2024 at 09:17:44AM -0600, David Lechner wrote:
> > On 12/17/24 5:41 PM, Vasileios Amoiridis wrote:  
> > > On Mon, Dec 16, 2024 at 03:57:44PM -0600, David Lechner wrote:  
> > >> On 12/14/24 1:14 PM, Vasileios Amoiridis wrote:  
> > >>> Drop conditional in favor of always calculating the timestamp value.
> > >>> This simplifies the code and allows to drop usage of internal private
> > >>> variable "scan_timestamp" of the struct iio_dev.
> > >>>
> > >>> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
> > >>> ---
> > >>>  drivers/iio/common/ssp_sensors/ssp_iio.c | 9 +++------
> > >>>  1 file changed, 3 insertions(+), 6 deletions(-)
> > >>>
> > >>> diff --git a/drivers/iio/common/ssp_sensors/ssp_iio.c b/drivers/iio/common/ssp_sensors/ssp_iio.c
> > >>> index caa404edd9d0..6b86b5315694 100644
> > >>> --- a/drivers/iio/common/ssp_sensors/ssp_iio.c
> > >>> +++ b/drivers/iio/common/ssp_sensors/ssp_iio.c
> > >>> @@ -8,6 +8,8 @@
> > >>>  #include <linux/iio/kfifo_buf.h>
> > >>>  #include <linux/module.h>
> > >>>  #include <linux/slab.h>
> > >>> +#include <linux/unaligned.h>
> > >>> +#include <linux/units.h>
> > >>>  #include "ssp_iio_sensor.h"
> > >>>  
> > >>>  /**
> > >>> @@ -70,7 +72,6 @@ EXPORT_SYMBOL_NS(ssp_common_buffer_postdisable, "IIO_SSP_SENSORS");
> > >>>  int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
> > >>>  			    unsigned int len, int64_t timestamp)
> > >>>  {
> > >>> -	__le32 time;
> > >>>  	int64_t calculated_time = 0;
> > >>>  	struct ssp_sensor_data *spd = iio_priv(indio_dev);
> > >>>  
> > >>> @@ -82,11 +83,7 @@ int ssp_common_process_data(struct iio_dev *indio_dev, void *buf,
> > >>>  	 */
> > >>>  	memcpy(spd->buffer, buf, len);
> > >>>  
> > >>> -	if (indio_dev->scan_timestamp) {
> > >>> -		memcpy(&time, &((char *)buf)[len], SSP_TIME_SIZE);
> > >>> -		calculated_time =
> > >>> -			timestamp + (int64_t)le32_to_cpu(time) * 1000000;
> > >>> -	}
> > >>> +	calculated_time = timestamp + get_unaligned_le32(buf + len) * MEGA;  
> > >>
> > >> Don't we still need to cast to 64 bit to avoid multiplication overflow?
> > >>  
> > > 
> > > Hi David,
> > > 
> > > Thanks for your message!
> > > 
> > > Aren't we already covered by the fact that MEGA is defined as an
> > > unsigned long?  
> > 
> > That is only 64-bits on 64-bit architectures, so could still overflow on
> > 32-bit architectures where long is 32-bit.
> >   
> 
> Hi David,
> 
> Hmmm, I think you are right. I can fix it in next, iteration. I will
> wait also for Jonathan's comments on the rest of the series.
> 
I tweaked it and applied.  Also dropped the initial assignment of calculated_time given it is now
always set before use.

Thanks,

Jonathan


> Cheers,
> Vasilis
> 
> > > 
> > > 	include/linux/units.h:12:#define MEGA 1000000UL
> > > 
> > > Cheers,
> > > Vasilis
> > >   
> > >>>  
> > >>>  	return iio_push_to_buffers_with_timestamp(indio_dev, spd->buffer,
> > >>>  						  calculated_time);  
> > >>  
> >   

[PATCH v2 4/4] iio: core: mark scan_timestamp as __private

[Vasileios Amoiridis]

Since there are no more direct accesses to the indio_dev->scan_timestamp
value, it can be marked as __private and use the macro ACCESS_PRIVATE()
in order to access it. Like this, static checkers will be able to inform
in case someone tries to either write to the value, or read its value
directly.

Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
---
 drivers/iio/industrialio-buffer.c | 2 +-
 include/linux/iio/buffer.h        | 2 +-
 include/linux/iio/iio.h           | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index 2708f87df719..a80f7cc25a27 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -1137,7 +1137,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
 	int ret;
 
 	indio_dev->active_scan_mask = config->scan_mask;
-	indio_dev->scan_timestamp = config->scan_timestamp;
+	ACCESS_PRIVATE(indio_dev, scan_timestamp) = config->scan_timestamp;
 	indio_dev->scan_bytes = config->scan_bytes;
 	iio_dev_opaque->currentmode = config->mode;
 
diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
index 418b1307d3f2..3b8d618bb3df 100644
--- a/include/linux/iio/buffer.h
+++ b/include/linux/iio/buffer.h
@@ -37,7 +37,7 @@ int iio_pop_from_buffer(struct iio_buffer *buffer, void *data);
 static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev,
 	void *data, int64_t timestamp)
 {
-	if (indio_dev->scan_timestamp) {
+	if (ACCESS_PRIVATE(indio_dev, scan_timestamp)) {
 		size_t ts_offset = indio_dev->scan_bytes / sizeof(int64_t) - 1;
 		((int64_t *)data)[ts_offset] = timestamp;
 	}
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index ae65890d4567..56161e02f002 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -611,7 +611,7 @@ struct iio_dev {
 	const unsigned long		*available_scan_masks;
 	unsigned int			__private masklength;
 	const unsigned long		*active_scan_mask;
-	bool				scan_timestamp;
+	bool				__private scan_timestamp;
 	struct iio_trigger		*trig;
 	struct iio_poll_func		*pollfunc;
 	struct iio_poll_func		*pollfunc_event;
-- 
2.43.0

Re: [PATCH v2 4/4] iio: core: mark scan_timestamp as __private

[Jonathan Cameron]

On Sat, 14 Dec 2024 20:14:21 +0100
Vasileios Amoiridis <vassilisamir@gmail.com> wrote:

> Since there are no more direct accesses to the indio_dev->scan_timestamp
> value, it can be marked as __private and use the macro ACCESS_PRIVATE()
> in order to access it. Like this, static checkers will be able to inform
> in case someone tries to either write to the value, or read its value
> directly.
> 
> Signed-off-by: Vasileios Amoiridis <vassilisamir@gmail.com>
Applied.  Thanks

I'll push this out as testing shortly and we can see if 0-day finds
any problems with it.

Jonathan



> ---
>  drivers/iio/industrialio-buffer.c | 2 +-
>  include/linux/iio/buffer.h        | 2 +-
>  include/linux/iio/iio.h           | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> index 2708f87df719..a80f7cc25a27 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -1137,7 +1137,7 @@ static int iio_enable_buffers(struct iio_dev *indio_dev,
>  	int ret;
>  
>  	indio_dev->active_scan_mask = config->scan_mask;
> -	indio_dev->scan_timestamp = config->scan_timestamp;
> +	ACCESS_PRIVATE(indio_dev, scan_timestamp) = config->scan_timestamp;
>  	indio_dev->scan_bytes = config->scan_bytes;
>  	iio_dev_opaque->currentmode = config->mode;
>  
> diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h
> index 418b1307d3f2..3b8d618bb3df 100644
> --- a/include/linux/iio/buffer.h
> +++ b/include/linux/iio/buffer.h
> @@ -37,7 +37,7 @@ int iio_pop_from_buffer(struct iio_buffer *buffer, void *data);
>  static inline int iio_push_to_buffers_with_timestamp(struct iio_dev *indio_dev,
>  	void *data, int64_t timestamp)
>  {
> -	if (indio_dev->scan_timestamp) {
> +	if (ACCESS_PRIVATE(indio_dev, scan_timestamp)) {
>  		size_t ts_offset = indio_dev->scan_bytes / sizeof(int64_t) - 1;
>  		((int64_t *)data)[ts_offset] = timestamp;
>  	}
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index ae65890d4567..56161e02f002 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -611,7 +611,7 @@ struct iio_dev {
>  	const unsigned long		*available_scan_masks;
>  	unsigned int			__private masklength;
>  	const unsigned long		*active_scan_mask;
> -	bool				scan_timestamp;
> +	bool				__private scan_timestamp;
>  	struct iio_trigger		*trig;
>  	struct iio_poll_func		*pollfunc;
>  	struct iio_poll_func		*pollfunc_event;