[PATCH] regmap: irq: do not allow setting irq bits during ack

[PATCH] regmap: irq: do not allow setting irq bits during ack

[Tim Harvey]

Some interrupt controllers may not de-assert their interrupt if
bits are set when acknowledging the bits that caused the interrupt.

Take care to not apply the mask to the status until we are done
acknowledging the interrupt and take care to mask the bits according
for the ack_invert state.

This is needed to avoid a stuck interrupt case for the Gateworks
System Controller which uses ack_invert. If the status has the mask
applied before clearing the bits it will end up setting bits that
are enabled but were not the cause the interrupt which will keep
the GSC from ever de-asserting its interrupt.

Cc: Tony Lindgren <tony@atomide.com>
Cc: Laxminath Kasam <lkasam@codeaurora.org>
Cc: Robert Jones <rjones@gateworks.com>
Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/base/regmap/regmap-irq.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index ad5c2de..560c641 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -496,29 +496,29 @@ static irqreturn_t regmap_irq_thread(int irq, void *d)
 	 * doing a write per register.
 	 */
 	for (i = 0; i < data->chip->num_regs; i++) {
-		data->status_buf[i] &= ~data->mask_buf[i];
-
-		if (data->status_buf[i] && (chip->ack_base || chip->use_ack)) {
+		if ((data->status_buf[i] && ~data->mask_buf[i]) &&
+		    (chip->ack_base || chip->use_ack)) {
 			reg = chip->ack_base +
 				(i * map->reg_stride * data->irq_reg_stride);
 			if (chip->ack_invert)
 				ret = regmap_write(map, reg,
-						~data->status_buf[i]);
+						~data->status_buf[i] & data->mask_buf[i]);
 			else
 				ret = regmap_write(map, reg,
-						data->status_buf[i]);
+						data->status_buf[i] & ~data->mask_buf[i]);
 			if (chip->clear_ack) {
 				if (chip->ack_invert && !ret)
 					ret = regmap_write(map, reg,
-							data->status_buf[i]);
+							data->status_buf[i] & ~data->mask_buf[i]);
 				else if (!ret)
 					ret = regmap_write(map, reg,
-							~data->status_buf[i]);
+							~data->status_buf[i] & data->mask_buf[i]);
 			}
 			if (ret != 0)
 				dev_err(map->dev, "Failed to ack 0x%x: %d\n",
 					reg, ret);
 		}
+		data->status_buf[i] &= ~data->mask_buf[i];
 	}
 
 	for (i = 0; i < chip->num_irqs; i++) {
-- 
2.7.4

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 631 bytes --]

On Mon, Dec 28, 2020 at 01:45:51PM -0800, Tim Harvey wrote:

> Some interrupt controllers may not de-assert their interrupt if
> bits are set when acknowledging the bits that caused the interrupt.

> Take care to not apply the mask to the status until we are done
> acknowledging the interrupt and take care to mask the bits according
> for the ack_invert state.

I can't understand what this commit message is trying to say, sorry.
Which bits are you talking about when you say "if bits are set"?  Isn't
acknowleding the interrupt clearing the bits asserting the interrupt?  I
can't tell what the problem you're trying to fix is.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Tim Harvey]

On Tue, Dec 29, 2020 at 5:06 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Dec 28, 2020 at 01:45:51PM -0800, Tim Harvey wrote:
>
> > Some interrupt controllers may not de-assert their interrupt if
> > bits are set when acknowledging the bits that caused the interrupt.
>
> > Take care to not apply the mask to the status until we are done
> > acknowledging the interrupt and take care to mask the bits according
> > for the ack_invert state.
>
> I can't understand what this commit message is trying to say, sorry.
> Which bits are you talking about when you say "if bits are set"?  Isn't
> acknowleding the interrupt clearing the bits asserting the interrupt?  I
> can't tell what the problem you're trying to fix is.

Mark,

If for example status=0x01 the current code for ack_invert will write
a 0xfe to clear that bit which ends up setting all other interrupt
bits keeping the interrupt from ever being de-asserted. With the patch
applied a status=0x01 will result in a 0x00 written to clear that bit
and keep other's from being set.

Tim

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

On Tue, Dec 29, 2020 at 08:23:00AM -0800, Tim Harvey wrote:
> On Tue, Dec 29, 2020 at 5:06 AM Mark Brown <broonie@kernel.org> wrote:

> > I can't understand what this commit message is trying to say, sorry.
> > Which bits are you talking about when you say "if bits are set"?  Isn't
> > acknowleding the interrupt clearing the bits asserting the interrupt?  I
> > can't tell what the problem you're trying to fix is.

> If for example status=0x01 the current code for ack_invert will write
> a 0xfe to clear that bit which ends up setting all other interrupt
> bits keeping the interrupt from ever being de-asserted. With the patch
> applied a status=0x01 will result in a 0x00 written to clear that bit
> and keep other's from being set.

But that's not an inverted ack as far as I can see?  That's writing back
the bit you're trying to clear which is the default ack.  Why do you
believe this is an inverted ack?  In any case the changelog for the
patch needs to be clear.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Tim Harvey]

On Wed, Dec 30, 2020 at 5:14 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Tue, Dec 29, 2020 at 08:23:00AM -0800, Tim Harvey wrote:
> > On Tue, Dec 29, 2020 at 5:06 AM Mark Brown <broonie@kernel.org> wrote:
>
> > > I can't understand what this commit message is trying to say, sorry.
> > > Which bits are you talking about when you say "if bits are set"?  Isn't
> > > acknowleding the interrupt clearing the bits asserting the interrupt?  I
> > > can't tell what the problem you're trying to fix is.
>
> > If for example status=0x01 the current code for ack_invert will write
> > a 0xfe to clear that bit which ends up setting all other interrupt
> > bits keeping the interrupt from ever being de-asserted. With the patch
> > applied a status=0x01 will result in a 0x00 written to clear that bit
> > and keep other's from being set.
>
> But that's not an inverted ack as far as I can see?  That's writing back
> the bit you're trying to clear which is the default ack.  Why do you
> believe this is an inverted ack?  In any case the changelog for the
> patch needs to be clear.

Mark,

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

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').

Best regards,

Tim

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2369 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]