linux-kernel - Re: [PATCH 1/3] iio: light: Add driver for ap3216c

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]

Date:   Mon, 18 Feb 2019 15:22:24 +0000
From:   Jonathan Cameron <jonathan.cameron@...wei.com>
To:     Sven Van Asbroeck <thesven73@...il.com>
CC:     Robert Eshleman <bobbyeshleman@...il.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald-Stadler <pmeerw@...erw.net>,
        "Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
        <linux-iio@...r.kernel.org>
Subject: Re: [PATCH 1/3] iio: light: Add driver for ap3216c

On Tue, 12 Feb 2019 22:25:39 -0500
Sven Van Asbroeck <thesven73@...il.com> wrote:

> Hi Bobby,
> 
> On Tue, Feb 12, 2019 at 9:17 PM Robert Eshleman <bobbyeshleman@...il.com> wrote:
> >
> > First, thank you for the feedback.  
> 
> First of all, thank _you_ for doing the hard work on this driver !
> I very much respect what you've done here.
> 
> >
> > I had initially went with a similar design, but there is
> > the case in which the interrupt fires and then before the status
> > register is read by the handler a user process reads the data and
> > clears the interrupt.  When the handler continues execution it will
> > read a zero status and return IRQ_NONE.  My understanding of how
> > Linux handles IRQ_NONE is pretty poor, but I felt that this behavior
> > is incorrect even if inconsequential.  This could be avoided by
> > doing a status register read with every data read, and buffering
> > that as well, but then we lose the benefit altogether by increasing
> > I2C reads.
> >
> > In the approach you describe here, it seems like that would
> > work if this driver wasn't supporting shared interrupts.  In the
> > case that a user-space read happens to clear the interrupt before
> > the handler reads the status register, I think we would end up
> > falsely returning IRQ_NONE.
> >
> > Is my understanding of this correct?  It's very possible I'm
> > misunderstanding IRQ_NONE and shared interrupts.
> >  
> 
> Yes, I can see how one can run into those issues.
> 
> I believe that this whole class of problems goes away if PS/ALS
> are _exclusively_ read inside the interrupt, and cached.
> 
> Then, whenever a user process wants to read the data, the function
> does not touch the h/w, but simply return the cached value.
> 
> But hang on, I will have more to say on this when replying to Jonathan's
> feedback.

As mentioned in the other thread.  Can't do that.  Then we don't get a readout for
a normal read of the value.  If we aren't above the threshold then
we don't an interrupt, hence no value is ever read.

So, what I'm reading above is worrying.   The interrupt is cleared
by the read of the data registers?  I thought the datasheet allowed
for an explicit clear?

If it clears on a read, then we are on one of those rare and really
annoying devices where we have to deal with them not really being able
to do monitoring and polled reading at the same time and also not
providing a dataready interrupt (which allows for the solution Sven
gives)

Basically we've only ever had one of these that I can recall and on that
max1363 it was more a case of the format being really nasty to decode
than the hardware not being capable of safely do it, so we didn't bother.

Jonathan