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]

Message-ID: <20190213021753.GA19621@bobby.localdomain>
Date:   Tue, 12 Feb 2019 18:17:53 -0800
From:   Robert Eshleman <bobbyeshleman@...il.com>
To:     Sven Van Asbroeck <thesven73@...il.com>
Cc:     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 Mon, Feb 11, 2019 at 02:29:58PM -0500, Sven Van Asbroeck wrote:
> On Sun, Feb 10, 2019 at 3:39 PM Robert Eshleman <bobbyeshleman@...il.com> wrote:
> >
> > This patch adds support for the ap3216c ambient light and proximity
> > sensor.
> 
> PS
> 
> Why not use the chip in the mode where the interrupt is automatically cleared by
> reading the data? This could work if you read the data in the
> interrupt routine, store
> it in a buffer, then send the event to iio. Then when userspace wants
> to read out the
> value, don't actually touch the hardware, but return the buffered value.
> 
> I don't think you then need any synchronization primitives to
> accomplish this, such as
> mutexes, spin locks, etc. Because the interrupt routine is single-threaded.
> 
> You don't even need a memory barrier for the buffered value,
> because the iio core uses a waitqueue internally, which automatically issues
> an mb(). As far as I know.

Hey Sven,

First, thank you for the feedback.

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.

Again, thank you for the feedback.

-Bobby