linux-kernel - Re: [PATCH v3 4/5] iio: light: ROHM BU27008 color sensor

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: <20230513182629.398447b4@jic23-huawei>
Date:   Sat, 13 May 2023 18:26:29 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Matti Vaittinen <mazziesaccount@...il.com>
Cc:     "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Shreeya Patel <shreeya.patel@...labora.com>,
        Zhigang Shi <Zhigang.Shi@...eon.com>,
        Paul Gazzillo <paul@...zz.com>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Andi Shyti <andi.shyti@...nel.org>
Subject: Re: [PATCH v3 4/5] iio: light: ROHM BU27008 color sensor

On Sun, 7 May 2023 19:13:38 +0300
Matti Vaittinen <mazziesaccount@...il.com> wrote:

> On 5/7/23 17:22, Jonathan Cameron wrote:
> 
> >>> If there is nothing to do in the actual interrupt as it's a data ready
> >>> only signal, then you should just call iio_trigger_poll() in the top half and
> >>> use devm_request_irq() only as there is no thread in this interrupt (though
> >>> there is one for the interrupt below the software interrupt chip).  
> >>
> >> I haven't tested this yet so please ignore me if I am writing nonsense -
> >> but... The BU27008 will keep the IRQ line asserted until a register is
> >> read. We can't read the register form HW-IRQ so we need to keep the IRQ
> >> disabled until the threaded trigger handler is ran. With the setup we
> >> have here, the IRQF_ONESHOT, took care of this. I assume that changing
> >> to call the iio_poll_trigger() from top-half means I need to explicitly
> >> disable the IRQ and re-enable it at the end of the trigger thread after
> >> reading the register which debounces the IRQ line?  
> > 
> > Hmm. I'm trying to remember how this works (wrote this a very long time ago).
> > I'm fairly sure it's not an issue because we use IRQF_ONESHOT down one level
> > so exercise the same prevention of the threads triggering multiple times etc >   
> https://elixir.bootlin.com/linux/latest/source/drivers/iio/buffer/industrialio-triggered-buffer.c#L57
> > 
> > It doesn't matter if the device interrupt fires again as it will still be masked
> > at our software irqchip level and will then get queued up and the thread will
> > run again.  
> 
> After reading this I am not at all sure I am using the trigger 
> correctly. I see the iio_trigger_attach_poll_func() registering threaded 
> handler with the IRQF_ONESHOT which is stored in the 
> iio_alloc_pollfunc() as you pointed above.
> 
> The iio_trigger_attach_poll_func() is called from sysfs callback when 
> trigger is enabled. So, if this is supposed to be the device IRQ, then I 
> am not at all sure the driver should be requesting the IRQ at the 
> probe(). If it is not the device's IRQ, then I guess the IRQF_ONESHOT 
> passed in here won't help. I need to try seeing some examples how other 
> drivers are using the triggers. Getting back to this tomorrow...

I think you are confusing two different irqs here / I managed to confuse
you even more.

The one in iio_trigger_attach_pollfunc() is a child of the trigger - it's
called when the trigger 'fires' along with any other children attached
(if multiple IIO devices have current_trigger set tot his one).
The one you are registering for is the parent of the trigger and calls
iio_poll_trigger() in it's handler which in turn will result in interrupts
for any children of the trigger (that is the pollfunc's which are
interrupt handlers underneath).

Not sure this horrible diagram will help

    Data Ready Interrupt
            |
 iio_trigger_poll() after checking it's right int etc. Easy here.
            |
    ________|__________________________________________________________________
    |                             |                                  |         |
 Pollfunc 1 Int              Pollfunc 2 Int
    |                             |
 Top half of PF 1           Top half of PF2 return IRQ_THREAD etc
    |                             |
.....................................
    |                             |
 Thread PF 1                 Thread PF 2
    |                             |
iio_push_to_buffers..()    iio_push_to_bufffers..()
For trigger consumer       For trigger consumer
device 1                   device 2.

So in the pollfunc interrupts have IRQF_ONESHOT set so the masking happens
in the internal irq_chip that IIO uses to handle this demux of the
trigger signal to multiple different sensor drivers to make them grab
data to push into buffers.  This whole thing is built to enable
sync capture of multiple signals where possible (hardware restrictions
mean this only works for some combinations).
Years ago, we had our own registration scheme that didn't use interrupts
internally and it was pointed out (Arnd Bergmann I think...) that we
were just replicating what goes on in an interrupt chip so should use
that instead.

Jonathan

> 
> Yours,
> 	-- Matti.
>