linux-kernel - Re: [PATCH] iio: adis16480: fix FNCTIO

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <56DFEB6D.90604@metafoo.de>
Date:	Wed, 9 Mar 2016 10:22:53 +0100
From:	Lars-Peter Clausen <lars@...afoo.de>
To:	Vlad Banea <vladb@...ux.com>, jic23@...nel.org,
	Michael.Hennerich@...log.com
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling
 IRQ

On 03/09/2016 06:28 AM, Vlad Banea wrote:
> Enabling the IRQ should leave all other settings in the FNCTIO_CTRL
> register untouched: read the whole register, toggle just the enable bit,
> before writing it back.

Hi,

Thanks for the patch. Looks good in general, but it's not a fix. The driver
does not write this register anywhere else and the reset default value is
0x00. So we don't corrupt any other settings since, 0x00 and BIT(3) are the
only two settings the driver does at the moment.

If the patch is in preparation of future changes that are going to set/clear
other bits of the register this should be noted in the commit message.

The reason I'm so pedantic here is because fix generally means that the
patch needs to be backported to older kernel versions, which is not the case
here.

- Lars


> ---
>  drivers/iio/imu/adis16480.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index b94bfd3..8473859 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -738,8 +738,19 @@ static int adis16480_stop_device(struct iio_dev *indio_dev)
>  
>  static int adis16480_enable_irq(struct adis *adis, bool enable)
>  {
> -	return adis_write_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL,
> -		enable ? BIT(3) : 0);
> +	u16 fnctio_ctrl;
> +	int ret;
> +
> +	ret = adis_read_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, &fnctio_ctrl);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (enable)
> +		fnctio_ctrl |= BIT(3);
> +	else
> +		fnctio_ctrl &= ~BIT(3);
> +
> +	return adis_write_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, fnctio_ctrl);
>  }
>  
>  static int adis16480_initial_setup(struct iio_dev *indio_dev)
>