linux-kernel - Re: [PATCH] Fix IRQ issue by setting IRQ_DISABLE

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: <2a108acdd79682f47e3ac923fe005b943a4a00c0.camel@gmail.com>
Date:   Mon, 27 Mar 2023 14:28:02 +0200
From:   Nuno Sá <noname.nuno@...il.com>
To:     Masahiro Honda <honda@...hatrax.com>
Cc:     Lars-Peter Clausen <lars@...afoo.de>,
        Michael Hennerich <Michael.Hennerich@...log.com>,
        Jonathan Cameron <jic23@...nel.org>, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Fix IRQ issue by setting IRQ_DISABLE_UNLAZY flag

On Mon, 2023-03-27 at 18:01 +0900, Masahiro Honda wrote:

Hi Masahiro,

Thanks for looking in more detail into this...

> Hi, Nuno
> 
> > On Mon, Mar 6, 2023 at 10:30 PM Nuno Sá <noname.nuno@...il.com>
> > wrote:
> > > Another thing that came to my mind is if the data is totally
> > > garbage/wrong or is it just some outstanding sample?
> 
> I'm sorry for not answering your question. The data is the same as
> the previous conversion even if the input voltage is changed. At this
> time, a logic analyzer shows that the read operation is performed
> immediately after the conversion is performed. The read operation
> returns the previous conversion result because it is performed before
> the completion of the conversion.
> 

So, my suspicion was right... We are getting stalled data (which is
obviously not good). AFAIU, when disabling the IRQ, we don't
immediately mask the IRQ and we only do it in the next time an
interrupt (sample) comes which means (I think) we'll process (right
away) that outstanding interrupt next time we enable the IRQ.

> > > Some research on this also seems to point that we should (need?)
> > > call
> > > irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the
> > > IRQ.
> 
> I have understood that I need to call irq_clear_status_flags.
> However,
> I cannot find a code to free the IRQ in ad_sigma_delta.c and other
> Sigma-Delta ADC driver source files. So, I would like to implement
> only irq_set_status_flags.

Well, that's because we are using devm_request_irq() which is a device
managed API. So, I can see two options in here... 

1) You do not use devm_request_irq() and use request_irq() +
devm_add_action_or_reset() and in your release() function you would
call irq_clear_status_flags() + free_irq().

2) You add a devm_add_action_or_reset() after devm_request_irq() and
your release() function would only clear the flag. But in here we would
likely also have to be careful in the case where devm_request_irq()
fails. So option 2) seems a bit more "ugly".

I would likely go to option 1) but maybe Jonathan or others have better
ideas.

- Nuno Sá