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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7cafafd3-e7a7-96db-0f8f-ac5c8bc94d13@gmail.com>
Date:   Sun, 7 May 2023 19:13:38 +0300
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>,
        "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
Cc:     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 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...

Yours,
	-- Matti.

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~