linux-kernel - Re: [PATCH v1 1/1] iio: accel: adxl367: fix setting odr for activity time update

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] [day] [month] [year] [list]

Message-ID: <20250222150329.66d6e79b@jic23-huawei>
Date: Sat, 22 Feb 2025 15:03:29 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: lars@...afoo.de, Michael.Hennerich@...log.com,
 cosmin.tanislav@...log.com, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/1] iio: accel: adxl367: fix setting odr for
 activity time update

On Fri, 21 Feb 2025 20:33:52 +0000
Lothar Rubusch <l.rubusch@...il.com> wrote:

> Fix setting the odr value to update activity time based on frequency
> derrived by recent odr, and not by obsolete odr value.
> 
> The [small] bug: When _adxl367_set_odr() is called with a new odr value,
> it first writes the new odr value to the hardware register
> ADXL367_REG_FILTER_CTL.
> Second, it calls _adxl367_set_act_time_ms(), which calls
> adxl367_time_ms_to_samples(). Here st->odr still holds the old odr value.
> This st->odr member is used to derrive a frequency value, which is
> applied to update ADXL367_REG_TIME_ACT. Hence, the idea is to update
> activity time, based on possibilities and power consumption by the
> current ODR rate.
> Finally, when the function calls return, again in _adxl367_set_odr() the
> new ODR is assigned to st->odr.
> 
> The fix: When setting a new ODR value is set to ADXL367_REG_FILTER_CTL,
> also ADXL367_REG_TIME_ACT should probably be updated with a frequency
> based on the recent ODR value and not the old one. Changing the location
> of the assignment to st->odr fixes this.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
Fixes tag?

Otherwise looks good to me.

> ---
>  drivers/iio/accel/adxl367.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl367.c b/drivers/iio/accel/adxl367.c
> index add4053e7a02..0c04b2bb7efb 100644
> --- a/drivers/iio/accel/adxl367.c
> +++ b/drivers/iio/accel/adxl367.c
> @@ -601,18 +601,14 @@ static int _adxl367_set_odr(struct adxl367_state *st, enum adxl367_odr odr)
>  	if (ret)
>  		return ret;
>  
> +	st->odr = odr;
> +
>  	/* Activity timers depend on ODR */
>  	ret = _adxl367_set_act_time_ms(st, st->act_time_ms);
>  	if (ret)
>  		return ret;
>  
> -	ret = _adxl367_set_inact_time_ms(st, st->inact_time_ms);
> -	if (ret)
> -		return ret;
> -
> -	st->odr = odr;
> -
> -	return 0;
> +	return _adxl367_set_inact_time_ms(st, st->inact_time_ms);
>  }
>  
>  static int adxl367_set_odr(struct iio_dev *indio_dev, enum adxl367_odr odr)