linux-kernel - Re: [PATCH] regmap: irq: do not allow setting irq bits during ack

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: <20201231133041.GA4720@sirena.org.uk>
Date:   Thu, 31 Dec 2020 13:30:41 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Tim Harvey <tharvey@...eworks.com>
Cc:     "Rafael J . Wysocki" <rafael@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        open list <linux-kernel@...r.kernel.org>,
        Laxminath Kasam <lkasam@...eaurora.org>,
        Tony Lindgren <tony@...mide.com>,
        Lee Jones <lee.jones@...aro.org>,
        Robert Jones <rjones@...eworks.com>
Subject: Re: [PATCH] regmap: irq: do not allow setting irq bits during ack

On Wed, Dec 30, 2020 at 08:37:17AM -0800, Tim Harvey wrote:

> It 'is' inverted ack because the device I have requires a '0' to be
> written to clear the interrupt instead of a '1'.

Right, yes - misremembered there.

> The chip I'm using has a status register where bit values of 1
> indicate an interrupt assertion and to clear it you write a 0 (where
> as the typical non-ack-invert case you write a 1 to clear). The chip
> I'm using also allows you to 'set' (by writing a 1) to bits that were
> not already set which would keep it from de-asserting it's interrupt.

> Honestly I thought the commit message was very clear. What exactly is
> your suggestion? It is of course confusing when talking about code
> that handles both ack invert and the normal case (let alone the new
> case of 'clear_ack').

First you need to write a commit message which explains what the change
is supposed to do.  Like I said it's things like talking about how "bits
are set" without specifying which bits you are talking about - which
bits?  You mean other bits in the status/ack register but especially
given all the talk about ack_invert in the commit message and the fact
that it is very unusual to be able to assert an interrupt by writing to
the status/ack register it's a bit of a jump to get there.  It could be
something to do with masking non-ack/status bits in the register, it
could be something to do with confusion about what inversion means, or
something else.  Something like your above explanation is much clearer
than what you wrote since it explains the unusual behaviour of your chip
which causes problems which makes it clear which bits you are talking
about.

The behaviour you are trying to implement here also needs to be opt in
since it will be harmful for other controllers due to it being racy, as
far as I can see with your controller there is no way to acknowledge a
single interrupt, we have to acknowledge them all since the only
sensible thing to write for any bit is an acknowledgement.  This means
that if handling an interrupt races with a different one being asserted
then the new interrupt will be acknowledged before it is seen as part of
acknowleding the original interrupt.  You could also express this as
doing a read/modify/write to clear just the bits that are asserted but
the effect is the same so probably an ack all mode would be easier.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)