linux-kernel - Re: [PATCH] iio: invensense: fix multiple odr switch when FIFO is off

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241019153912.43e050a0@jic23-huawei>
Date: Sat, 19 Oct 2024 15:39:12 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Jean-Baptiste Maneyrol via B4 Relay
 <devnull+jean-baptiste.maneyrol.tdk.com@...nel.org>
Cc: jean-baptiste.maneyrol@....com, Lars-Peter Clausen <lars@...afoo.de>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] iio: invensense: fix multiple odr switch when FIFO is
 off

On Thu, 17 Oct 2024 16:06:28 +0200
Jean-Baptiste Maneyrol via B4 Relay <devnull+jean-baptiste.maneyrol.tdk.com@...nel.org> wrote:

> From: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>
> 
> When multiple ODR switch happens during FIFO off, the change could
> not be taken into account if you get back to previous FIFO on value.
> For example, if you run sensor buffer at 50Hz, stop, change to
> 200Hz, then back to 50Hz and restart buffer, data will be timestamped
> at 200Hz. This due to testing against mult and not new_mult.
> 
> To prevent this, let's just run apply_odr automatically when FIFO is
> off. It will also simplify driver code.
> 
> Update inv_mpu6050 and inv_icm42600 to delete now useless apply_odr.
> 
> Fixes: 95444b9eeb8c ("iio: invensense: fix odr switching to same value")
> Cc: stable@...r.kernel.org
> Signed-off-by: Jean-Baptiste Maneyrol <jean-baptiste.maneyrol@....com>

In at least some of the cases ts is no longer used:

  CHECK   drivers/iio/common/inv_sensors/inv_sensors_timestamp.c
drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c: In function ‘inv_icm42600_gyro_update_scan_mode’:
drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c:103:39: warning: unused variable ‘ts’ [-Wunused-variable]
  103 |         struct inv_sensors_timestamp *ts = &gyro_st->ts;
      |                                       ^~
drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c: In function ‘inv_icm42600_accel_update_scan_mode’:
drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c:203:39: warning: unused variable ‘ts’ [-Wunused-variable]
  203 |         struct inv_sensors_timestamp *ts = &accel_st->ts;
      |                                       ^~

So drop those as well for v2.


> ---
>  drivers/iio/common/inv_sensors/inv_sensors_timestamp.c | 4 ++++
>  drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c      | 1 -
>  drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c       | 1 -
>  drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c          | 1 -
>  4 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c
> index f44458c380d92823ce2e7e5f78ca877ea4c06118..37d0bdaa8d824f79dcd2f341be7501d249926951 100644
> --- a/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c
> +++ b/drivers/iio/common/inv_sensors/inv_sensors_timestamp.c
> @@ -70,6 +70,10 @@ int inv_sensors_timestamp_update_odr(struct inv_sensors_timestamp *ts,
>  	if (mult != ts->mult)
>  		ts->new_mult = mult;
>  
> +	/* When FIFO is off, directly apply the new ODR */
> +	if (!fifo)
> +		inv_sensors_timestamp_apply_odr(ts, 0, 0, 0);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(inv_sensors_timestamp_update_odr, IIO_INV_SENSORS_TIMESTAMP);
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index 56ac198142500a2e1fc40b62cdd465cc736d8bf0..d061a64ebbf71859a3bc44644a14137dff0f9efe 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> @@ -229,7 +229,6 @@ static int inv_icm42600_accel_update_scan_mode(struct iio_dev *indio_dev,
>  	}
>  
>  	/* update data FIFO write */
> -	inv_sensors_timestamp_apply_odr(ts, 0, 0, 0);
>  	ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
>  
>  out_unlock:
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> index 938af5b640b00f58d2b8185f752c4755edfb0d25..f1e5a9648c4f5dd34f40136d02c72c90473eff37 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> @@ -128,7 +128,6 @@ static int inv_icm42600_gyro_update_scan_mode(struct iio_dev *indio_dev,
>  	}
>  
>  	/* update data FIFO write */
> -	inv_sensors_timestamp_apply_odr(ts, 0, 0, 0);
>  	ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
>  
>  out_unlock:
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> index 3bfeabab0ec4f6fa28fbbcd47afe92af5b8a58e2..5b1088cc3704f1ad1288a0d65b2f957b91455d7f 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c
> @@ -112,7 +112,6 @@ int inv_mpu6050_prepare_fifo(struct inv_mpu6050_state *st, bool enable)
>  	if (enable) {
>  		/* reset timestamping */
>  		inv_sensors_timestamp_reset(&st->timestamp);
> -		inv_sensors_timestamp_apply_odr(&st->timestamp, 0, 0, 0);
>  		/* reset FIFO */
>  		d = st->chip_config.user_ctrl | INV_MPU6050_BIT_FIFO_RST;
>  		ret = regmap_write(st->map, st->reg->user_ctrl, d);
> 
> ---
> base-commit: c3e9df514041ec6c46be83801b1891392f4522f7
> change-id: 20241017-invn-inv-sensors-timestamp-fix-switch-fifo-off-3f29110e95d0
> 
> Best regards,