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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Sat, 12 Mar 2016 11:14:28 +0000
From:	Jonathan Cameron <jic23@...nel.org>
To:	Vlad Banea <vlad@...ea.ca>, Lars-Peter Clausen <lars@...afoo.de>
Cc:	Vlad Banea <vladb@...ux.com>, Michael.Hennerich@...log.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling
 IRQ

On 09/03/16 17:06, Vlad Banea wrote:
> Hi,
> 
> Ok, I'll change the patch to keep the old behaviour as the default one
> but add a device tree settings for selecting the DIO and polarity as
> you said.
Cool.  As a process related aside, please make sure to cc the list
linux-iio@...r.kernel.org

Jonathan
> 
> Vlad
> 
> On Wed, Mar 9, 2016 at 11:41 AM, Lars-Peter Clausen <lars@...afoo.de> wrote:
>> Hi,
>>
>> The intention was to use DIO1 by default, which the driver does and which
>> has been tested. With your patch it uses DIO2 by default, which is the
>> correct setting for your board.
>>
>> - Lars
>>
>> On 03/09/2016 04:27 PM, Vlad Banea wrote:
>>>
>>> Hi,
>>>
>>> Since the driver is never setting the FNCTIO_CTRL register, I'd say the
>>> intention is to use the default settings.
>>> The clearing of all the other bits as we toggle the interrupt seems
>>> unintentional, and that's what the proposed patch addresses. The ADIS16485
>>> device is not usable without this patch with the most up to date driver, and
>>> becomes fully functional when the patch is applied..
>>>
>>> I wouldn't mind contributing to allow the driver to fully configure this
>>> register, but I'm not sure what user interface this should go through.
>>>
>>> Thanks
>>> Vlad
>>>
>>>
>>>
>>> On Wed, Mar 9, 2016 at 7:21 AM, Lars-Peter Clausen <lars@...afoo.de
>>> <mailto:lars@...afoo.de>> wrote:
>>>
>>>     Hi,
>>>
>>>     Hm, right. But the intention of the driver is to use DIO1. Changing this to
>>>     DIO2 by default will break all existing users.
>>>
>>>     This change should be part of a patch which allows to configure which
>>>     interrupt output to use and also the interrupt polarity.
>>>
>>>     - Lars
>>>
>>>     On 03/09/2016 12:39 PM, Vlad Banea wrote:
>>>     > Hi,
>>>     >
>>>     > Thanks for your answer. I'm using this driver with the ADIS16485
>>>     device and
>>>     > the default value for this register is 0x000D:
>>>     >
>>>     http://www.analog.com/media/en/technical-documentation/data-sheets/ADIS16485.pdf
>>>     > (Table 89)
>>>     >
>>>     > When the driver enables the interrupt, the Data Ready Line selection and
>>>     > polarity are changed, and I never receive the interrupt.
>>>     >
>>>     > Vlad
>>>     >
>>>     >
>>>     > On Wed, Mar 9, 2016 at 4:22 AM, Lars-Peter Clausen <lars@...afoo.de
>>>     <mailto:lars@...afoo.de>
>>>     > <mailto:lars@...afoo.de <mailto:lars@...afoo.de>>> wrote:
>>>     >
>>>     >     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)
>>>     >     >
>>>     >
>>>     >
>>>
>>>
>>