[PATCH] mfd: cpcap: Fix interrupt regression with regmap clear_ack

[PATCH] mfd: cpcap: Fix interrupt regression with regmap clear_ack

[Tony Lindgren]

With commit 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack
registers"), the cpcap interrupts are no longer getting acked properly
leading to a very unresponsive device with CPUs fully loaded spinning
in the threaded IRQ handlers.

To me it looks like the clear_ack commit above actually fixed a long
standing bug in regmap_irq_thread() where we unconditionally acked the
interrupts earlier without considering ack_invert. And the issue with
cpcap started happening as we now also consider ack_invert.

Tim Harvey <tharvey@gateworks.com> tried to fix this issue earlier with
"[PATCH v2] regmap: irq: fix ack-invert", but the reading of the ack
register was considered unnecessary for just ack_invert, and we did not
have clear_ack available yet. As the cpcap irqs worked both with and
without ack_invert earlier because of the unconditional ack, the
problem remained hidden until now.

Also, looks like the earlier v3.0.8 based Motorola Android Linux kernel
does clear_ack style read-clear-write with "ireg_val & ~mreg_val" instead
of just ack_invert style write. So let's switch cpcap to use clear_ack
to fix the issue.

Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
Cc: Carl Philipp Klemm <philipp@uvos.xyz>
Cc: Laxminath Kasam <lkasam@codeaurora.org>
Cc: Merlijn Wajer <merlijn@wizzup.org>
Cc: Mark Brown <broonie@kernel.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Sebastian Reichel <sre@kernel.org>
Cc: Tim Harvey <tharvey@gateworks.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/mfd/motorola-cpcap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
--- a/drivers/mfd/motorola-cpcap.c
+++ b/drivers/mfd/motorola-cpcap.c
@@ -97,7 +97,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.ack_base = CPCAP_REG_MI1,
 		.mask_base = CPCAP_REG_MIM1,
 		.use_ack = true,
-		.ack_invert = true,
+		.clear_ack = true,
 	},
 	{
 		.name = "cpcap-m2",
@@ -106,7 +106,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.ack_base = CPCAP_REG_MI2,
 		.mask_base = CPCAP_REG_MIM2,
 		.use_ack = true,
-		.ack_invert = true,
+		.clear_ack = true,
 	},
 	{
 		.name = "cpcap1-4",
@@ -115,7 +115,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
 		.ack_base = CPCAP_REG_INT1,
 		.mask_base = CPCAP_REG_INTM1,
 		.use_ack = true,
-		.ack_invert = true,
+		.clear_ack = true,
 	},
 };
 
-- 
2.29.2

Re: [PATCH] mfd: cpcap: Fix interrupt regression with regmap clear_ack

[Lee Jones]

On Wed, 11 Nov 2020, Tony Lindgren wrote:

> With commit 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack
> registers"), the cpcap interrupts are no longer getting acked properly
> leading to a very unresponsive device with CPUs fully loaded spinning
> in the threaded IRQ handlers.
> 
> To me it looks like the clear_ack commit above actually fixed a long
> standing bug in regmap_irq_thread() where we unconditionally acked the
> interrupts earlier without considering ack_invert. And the issue with
> cpcap started happening as we now also consider ack_invert.
> 
> Tim Harvey <tharvey@gateworks.com> tried to fix this issue earlier with
> "[PATCH v2] regmap: irq: fix ack-invert", but the reading of the ack
> register was considered unnecessary for just ack_invert, and we did not
> have clear_ack available yet. As the cpcap irqs worked both with and
> without ack_invert earlier because of the unconditional ack, the
> problem remained hidden until now.
> 
> Also, looks like the earlier v3.0.8 based Motorola Android Linux kernel
> does clear_ack style read-clear-write with "ireg_val & ~mreg_val" instead
> of just ack_invert style write. So let's switch cpcap to use clear_ack
> to fix the issue.
> 
> Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> Cc: Carl Philipp Klemm <philipp@uvos.xyz>
> Cc: Laxminath Kasam <lkasam@codeaurora.org>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Sebastian Reichel <sre@kernel.org>
> Cc: Tim Harvey <tharvey@gateworks.com>

It would be nice to have you review this Tim.

> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/mfd/motorola-cpcap.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
> --- a/drivers/mfd/motorola-cpcap.c
> +++ b/drivers/mfd/motorola-cpcap.c
> @@ -97,7 +97,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
>  		.ack_base = CPCAP_REG_MI1,
>  		.mask_base = CPCAP_REG_MIM1,
>  		.use_ack = true,
> -		.ack_invert = true,
> +		.clear_ack = true,
>  	},
>  	{
>  		.name = "cpcap-m2",
> @@ -106,7 +106,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
>  		.ack_base = CPCAP_REG_MI2,
>  		.mask_base = CPCAP_REG_MIM2,
>  		.use_ack = true,
> -		.ack_invert = true,
> +		.clear_ack = true,
>  	},
>  	{
>  		.name = "cpcap1-4",
> @@ -115,7 +115,7 @@ static struct regmap_irq_chip cpcap_irq_chip[CPCAP_NR_IRQ_CHIPS] = {
>  		.ack_base = CPCAP_REG_INT1,
>  		.mask_base = CPCAP_REG_INTM1,
>  		.use_ack = true,
> -		.ack_invert = true,
> +		.clear_ack = true,
>  	},
>  };
>  

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] mfd: cpcap: Fix interrupt regression with regmap clear_ack

[Tim Harvey]

On Fri, Nov 13, 2020 at 2:21 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> On Wed, 11 Nov 2020, Tony Lindgren wrote:
>
> > With commit 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack
> > registers"), the cpcap interrupts are no longer getting acked properly
> > leading to a very unresponsive device with CPUs fully loaded spinning
> > in the threaded IRQ handlers.
> >
> > To me it looks like the clear_ack commit above actually fixed a long
> > standing bug in regmap_irq_thread() where we unconditionally acked the
> > interrupts earlier without considering ack_invert. And the issue with
> > cpcap started happening as we now also consider ack_invert.
> >
> > Tim Harvey <tharvey@gateworks.com> tried to fix this issue earlier with
> > "[PATCH v2] regmap: irq: fix ack-invert", but the reading of the ack
> > register was considered unnecessary for just ack_invert, and we did not
> > have clear_ack available yet. As the cpcap irqs worked both with and
> > without ack_invert earlier because of the unconditional ack, the
> > problem remained hidden until now.
> >
> > Also, looks like the earlier v3.0.8 based Motorola Android Linux kernel
> > does clear_ack style read-clear-write with "ireg_val & ~mreg_val" instead
> > of just ack_invert style write. So let's switch cpcap to use clear_ack
> > to fix the issue.
> >
> > Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> > Cc: Carl Philipp Klemm <philipp@uvos.xyz>
> > Cc: Laxminath Kasam <lkasam@codeaurora.org>
> > Cc: Merlijn Wajer <merlijn@wizzup.org>
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Cc: Sebastian Reichel <sre@kernel.org>
> > Cc: Tim Harvey <tharvey@gateworks.com>
>
> It would be nice to have you review this Tim.
>

Tony / Lee,

Thanks for keeping me in the loop on this. I still haven't properly
resolved the issue with my device/driver interrupts
(drivers/mfd/gateworks_gsc.c) which requires ack_invert (writing 0 to
clearing interrupt bits).

3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
appears to not only add the new clear_ack case it also attempts to
resolve the long standing ack_invert issue with this change:

- ret = regmap_write(map, reg, data->status_buf[i]);
+ if (chip->ack_invert)
+      ret = regmap_write(map, reg,
+      ~data->status_buf[i]);
+ else
+     ret = regmap_write(map, reg,
+     data->status_buf[i]);

However, this still doesn't resolve the issue I have with my
device/driver because it ends up writing 1's to all the other bits in
the ack register which keeps my device's interrupt from de-asserting.
Perhaps that's a strange behavior of my device that it allows you to
'set' interrupt source bits which causes the interrupt to stay
asserted? I'm also wondering if my issue is that I currently have the
interrupt registered as such:

ret = devm_regmap_add_irq_chip(dev, gsc->regmap, client->irq,
IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_FALLING, 0, &gsc_irq_chip,
&irq_data);

Perhaps this should be IRQF_TRIGGER_LOW as the device will not
de-assert its IRQ# until all source bits are cleared.

Tony, I thought that your pcap issue was that it truly did not have an
inverted ack and the fact that ack_invert did not work was why you
never noticed any issue. If this is true I would think you would want
to disable ack_invert but not necessarily enable clear_ack. Did your
testing show it needed to toggle the ack to clear it?

Best Regards,

Tim

Re: [PATCH] mfd: cpcap: Fix interrupt regression with regmap clear_ack

[Tony Lindgren]

* Tim Harvey <tharvey@gateworks.com> [201113 22:07]:
> 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> appears to not only add the new clear_ack case it also attempts to
> resolve the long standing ack_invert issue with this change:
> 
> - ret = regmap_write(map, reg, data->status_buf[i]);
> + if (chip->ack_invert)
> +      ret = regmap_write(map, reg,
> +      ~data->status_buf[i]);
> + else
> +     ret = regmap_write(map, reg,
> +     data->status_buf[i]);

Yes that's what I noticed too. And that's why cpcap was working for
me with ack_invert and without it earlier.

> However, this still doesn't resolve the issue I have with my
> device/driver because it ends up writing 1's to all the other bits in
> the ack register which keeps my device's interrupt from de-asserting.
> Perhaps that's a strange behavior of my device that it allows you to
> 'set' interrupt source bits which causes the interrupt to stay
> asserted? I'm also wondering if my issue is that I currently have the
> interrupt registered as such:
> 
> ret = devm_regmap_add_irq_chip(dev, gsc->regmap, client->irq,
> IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_FALLING, 0, &gsc_irq_chip,
> &irq_data);
> 
> Perhaps this should be IRQF_TRIGGER_LOW as the device will not
> de-assert its IRQ# until all source bits are cleared.

Yes could be. For cpcap, we have IRQ_TYPE_LEVEL_HIGH configured in the
motorola-cpcap-mapphone.dtsi file.

> Tony, I thought that your pcap issue was that it truly did not have an
> inverted ack and the fact that ack_invert did not work was why you
> never noticed any issue. If this is true I would think you would want
> to disable ack_invert but not necessarily enable clear_ack. Did your
> testing show it needed to toggle the ack to clear it?

Well I looked at the v3.0.8 Motorola Linux Android kernel, it actually
does clear_ack. So I'd rather keep the same logic as we have no proper
documentation for cpcap. I also confirmed still works without ack_invert
too while ack_invert now is broken. But using clear_ack now that we
have it working seems safer.

Regards,

Tony

Re: [PATCH] mfd: cpcap: Fix interrupt regression with regmap clear_ack

[Tim Harvey]

On Sun, Nov 15, 2020 at 12:43 AM Tony Lindgren <tony@atomide.com> wrote:
>
> * Tim Harvey <tharvey@gateworks.com> [201113 22:07]:
> > 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> > appears to not only add the new clear_ack case it also attempts to
> > resolve the long standing ack_invert issue with this change:
> >
> > - ret = regmap_write(map, reg, data->status_buf[i]);
> > + if (chip->ack_invert)
> > +      ret = regmap_write(map, reg,
> > +      ~data->status_buf[i]);
> > + else
> > +     ret = regmap_write(map, reg,
> > +     data->status_buf[i]);
>
> Yes that's what I noticed too. And that's why cpcap was working for
> me with ack_invert and without it earlier.
>
> > However, this still doesn't resolve the issue I have with my
> > device/driver because it ends up writing 1's to all the other bits in
> > the ack register which keeps my device's interrupt from de-asserting.
> > Perhaps that's a strange behavior of my device that it allows you to
> > 'set' interrupt source bits which causes the interrupt to stay
> > asserted? I'm also wondering if my issue is that I currently have the
> > interrupt registered as such:
> >
> > ret = devm_regmap_add_irq_chip(dev, gsc->regmap, client->irq,
> > IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_FALLING, 0, &gsc_irq_chip,
> > &irq_data);
> >
> > Perhaps this should be IRQF_TRIGGER_LOW as the device will not
> > de-assert its IRQ# until all source bits are cleared.
>
> Yes could be. For cpcap, we have IRQ_TYPE_LEVEL_HIGH configured in the
> motorola-cpcap-mapphone.dtsi file.
>
> > Tony, I thought that your pcap issue was that it truly did not have an
> > inverted ack and the fact that ack_invert did not work was why you
> > never noticed any issue. If this is true I would think you would want
> > to disable ack_invert but not necessarily enable clear_ack. Did your
> > testing show it needed to toggle the ack to clear it?
>
> Well I looked at the v3.0.8 Motorola Linux Android kernel, it actually
> does clear_ack. So I'd rather keep the same logic as we have no proper
> documentation for cpcap. I also confirmed still works without ack_invert
> too while ack_invert now is broken. But using clear_ack now that we
> have it working seems safer.
>
> Regards,
>
> Tony

Tony,

Ok - sounds like the right thing for your device.

Reviewed-By: Tim Harvey <tharvey@gateworks.com>

Tim

Re: [PATCH] mfd: cpcap: Fix interrupt regression with regmap clear_ack

[Mark Brown]

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

On Fri, Nov 13, 2020 at 02:06:29PM -0800, Tim Harvey wrote:

> asserted? I'm also wondering if my issue is that I currently have the
> interrupt registered as such:

> ret = devm_regmap_add_irq_chip(dev, gsc->regmap, client->irq,
> IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_FALLING, 0, &gsc_irq_chip,
> &irq_data);

> Perhaps this should be IRQF_TRIGGER_LOW as the device will not
> de-assert its IRQ# until all source bits are cleared.

That's clearly an active low interrupt, it will break things if it's
registered as edge triggered.

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

Re: [PATCH] mfd: cpcap: Fix interrupt regression with regmap clear_ack

[Tim Harvey]

On Mon, Nov 16, 2020 at 10:59 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Nov 13, 2020 at 02:06:29PM -0800, Tim Harvey wrote:
>
> > asserted? I'm also wondering if my issue is that I currently have the
> > interrupt registered as such:
>
> > ret = devm_regmap_add_irq_chip(dev, gsc->regmap, client->irq,
> > IRQF_ONESHOT | IRQF_SHARED | IRQF_TRIGGER_FALLING, 0, &gsc_irq_chip,
> > &irq_data);
>
> > Perhaps this should be IRQF_TRIGGER_LOW as the device will not
> > de-assert its IRQ# until all source bits are cleared.
>
> That's clearly an active low interrupt, it will break things if it's
> registered as edge triggered.

Mark,

Agreed - I will post a fix for my driver that changes it to IRQF_TRIGGER_LOW

What are your thoughts regarding the issue of  regmap_irq_sync_unlock
ack_invert ack'ing by writing ~d->mask_buf[i] which ends up setting
all the other bits not trying to be awk'd? I would say that the device
allowing an interrupt status to be 'set' and keeping it from releasing
its IRQ is strange/broken for sure, but I'll need to work around it
somehow.

Best Regards,

Tim

Re: [PATCH] mfd: cpcap: Fix interrupt regression with regmap clear_ack

[Mark Brown]

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

On Mon, Nov 16, 2020 at 12:43:57PM -0800, Tim Harvey wrote:

> What are your thoughts regarding the issue of  regmap_irq_sync_unlock
> ack_invert ack'ing by writing ~d->mask_buf[i] which ends up setting
> all the other bits not trying to be awk'd? I would say that the device
> allowing an interrupt status to be 'set' and keeping it from releasing
> its IRQ is strange/broken for sure, but I'll need to work around it
> somehow.

My initial assumption with ack_invert would be that any bits we're not
acking are inverted as well - if we just write 0 to those bits then we
might be spuriously acking them.  I'm not sure if there's something that
is being missed with how this hardware is modelled or if it's just a not
entirely thought through hardware design, if I'm understanding it
correctly I can't see anything that can safely be written to the bits we
don't want to ack.  Either we cause them to be asserted or we might
clear them incorrectly if we race with that interrupt asserting.

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

Re: [PATCH] mfd: cpcap: Fix interrupt regression with regmap clear_ack

[Pavel Machek]

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

Hi!

> With commit 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack
> registers"), the cpcap interrupts are no longer getting acked properly
> leading to a very unresponsive device with CPUs fully loaded spinning
> in the threaded IRQ handlers.
> 
> To me it looks like the clear_ack commit above actually fixed a long
> standing bug in regmap_irq_thread() where we unconditionally acked the
> interrupts earlier without considering ack_invert. And the issue with
> cpcap started happening as we now also consider ack_invert.
> 
> Tim Harvey <tharvey@gateworks.com> tried to fix this issue earlier with
> "[PATCH v2] regmap: irq: fix ack-invert", but the reading of the ack
> register was considered unnecessary for just ack_invert, and we did not
> have clear_ack available yet. As the cpcap irqs worked both with and
> without ack_invert earlier because of the unconditional ack, the
> problem remained hidden until now.
> 
> Also, looks like the earlier v3.0.8 based Motorola Android Linux kernel
> does clear_ack style read-clear-write with "ireg_val & ~mreg_val" instead
> of just ack_invert style write. So let's switch cpcap to use clear_ack
> to fix the issue.
> 
> Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")

Tested-by: Pavel Machek <pavel@ucw.cz>

Thank you, culomb counter issue is now gone.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

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

Re: [PATCH] mfd: cpcap: Fix interrupt regression with regmap clear_ack

[Lee Jones]

On Wed, 11 Nov 2020, Tony Lindgren wrote:

> With commit 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack
> registers"), the cpcap interrupts are no longer getting acked properly
> leading to a very unresponsive device with CPUs fully loaded spinning
> in the threaded IRQ handlers.
> 
> To me it looks like the clear_ack commit above actually fixed a long
> standing bug in regmap_irq_thread() where we unconditionally acked the
> interrupts earlier without considering ack_invert. And the issue with
> cpcap started happening as we now also consider ack_invert.
> 
> Tim Harvey <tharvey@gateworks.com> tried to fix this issue earlier with
> "[PATCH v2] regmap: irq: fix ack-invert", but the reading of the ack
> register was considered unnecessary for just ack_invert, and we did not
> have clear_ack available yet. As the cpcap irqs worked both with and
> without ack_invert earlier because of the unconditional ack, the
> problem remained hidden until now.
> 
> Also, looks like the earlier v3.0.8 based Motorola Android Linux kernel
> does clear_ack style read-clear-write with "ireg_val & ~mreg_val" instead
> of just ack_invert style write. So let's switch cpcap to use clear_ack
> to fix the issue.
> 
> Fixes: 3a6f0fb7b8eb ("regmap: irq: Add support to clear ack registers")
> Cc: Carl Philipp Klemm <philipp@uvos.xyz>
> Cc: Laxminath Kasam <lkasam@codeaurora.org>
> Cc: Merlijn Wajer <merlijn@wizzup.org>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Sebastian Reichel <sre@kernel.org>
> Cc: Tim Harvey <tharvey@gateworks.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/mfd/motorola-cpcap.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog