[PATCH] ASoC: wm8903: disable IRQs during BIAS_OFF

[PATCH] ASoC: wm8903: disable IRQs during BIAS_OFF

[Stephen Warren]

From: Stephen Warren <swarren@nvidia.com>

If the WM8903 IRQ is triggered after the device is suspended, or before
it is resumed, the underlying I2C bus that the WM8903 is attached to may
not be available. This may cause the IRQ handler to emit the following
error:

    wm8903 0-001a: Failed to read IRQ status: -16

... or perhaps even cause a failure inside the I2C driver.

To prevent this, explicitly mask all interrupt sources when entering
BIAS_OFF. Also, modify the IRQ handler to immediately return if the
device is in BIAS_OFF, so that shared interrupt lines don't cause a
similar problem. This requires that WM8903 interrupts be disabled to
avoid interrupt storms.

During resume (actually, the transition back to BIAS_STANDBY), restore
the desired IRQ mask.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
 sound/soc/codecs/wm8903.c | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/sound/soc/codecs/wm8903.c b/sound/soc/codecs/wm8903.c
index eebcb1d..e847bbe 100644
--- a/sound/soc/codecs/wm8903.c
+++ b/sound/soc/codecs/wm8903.c
@@ -1091,6 +1091,22 @@ static const struct snd_soc_dapm_route wm8903_intercon[] = {
 	{ "Right Line Output PGA", NULL, "Charge Pump" },
 };
 
+static void wm8903_setup_mic_det_irqs(struct snd_soc_codec *codec)
+{
+	struct wm8903_priv *wm8903 = snd_soc_codec_get_drvdata(codec);
+	int irq_mask = WM8903_MICDET_EINT | WM8903_MICSHRT_EINT;
+
+	/* Enable interrupts we've got a report configured for */
+	if (wm8903->mic_det)
+		irq_mask &= ~WM8903_MICDET_EINT;
+	if (wm8903->mic_short)
+		irq_mask &= ~WM8903_MICSHRT_EINT;
+
+	snd_soc_update_bits(codec, WM8903_INTERRUPT_STATUS_1_MASK,
+			    WM8903_MICDET_EINT | WM8903_MICSHRT_EINT,
+			    irq_mask);
+}
+
 static int wm8903_set_bias_level(struct snd_soc_codec *codec,
 				 enum snd_soc_bias_level level)
 {
@@ -1169,6 +1185,8 @@ static int wm8903_set_bias_level(struct snd_soc_codec *codec,
 					    WM8903_CP_DYN_V,
 					    WM8903_CP_DYN_FREQ |
 					    WM8903_CP_DYN_V);
+
+			wm8903_setup_mic_det_irqs(codec);
 		}
 
 		snd_soc_update_bits(codec, WM8903_VMID_CONTROL_0,
@@ -1177,6 +1195,8 @@ static int wm8903_set_bias_level(struct snd_soc_codec *codec,
 		break;
 
 	case SND_SOC_BIAS_OFF:
+		snd_soc_write(codec, WM8903_INTERRUPT_STATUS_1_MASK, 0xffff);
+
 		snd_soc_update_bits(codec, WM8903_BIAS_CONTROL_0,
 				    WM8903_BIAS_ENA, 0);
 
@@ -1600,7 +1620,6 @@ int wm8903_mic_detect(struct snd_soc_codec *codec, struct snd_soc_jack *jack,
 		      int det, int shrt)
 {
 	struct wm8903_priv *wm8903 = snd_soc_codec_get_drvdata(codec);
-	int irq_mask = WM8903_MICDET_EINT | WM8903_MICSHRT_EINT;
 
 	dev_dbg(codec->dev, "Enabling microphone detection: %x %x\n",
 		det, shrt);
@@ -1610,15 +1629,7 @@ int wm8903_mic_detect(struct snd_soc_codec *codec, struct snd_soc_jack *jack,
 	wm8903->mic_det = det;
 	wm8903->mic_short = shrt;
 
-	/* Enable interrupts we've got a report configured for */
-	if (det)
-		irq_mask &= ~WM8903_MICDET_EINT;
-	if (shrt)
-		irq_mask &= ~WM8903_MICSHRT_EINT;
-
-	snd_soc_update_bits(codec, WM8903_INTERRUPT_STATUS_1_MASK,
-			    WM8903_MICDET_EINT | WM8903_MICSHRT_EINT,
-			    irq_mask);
+	wm8903_setup_mic_det_irqs(codec);
 
 	if (det || shrt) {
 		/* Enable mic detection, this may not have been set through
@@ -1642,6 +1653,13 @@ static irqreturn_t wm8903_irq(int irq, void *data)
 	int mic_report, ret;
 	unsigned int int_val, mask, int_pol;
 
+	/*
+	 * During suspend, a shared IRQ may be triggered by some other device.
+	 * If so, just ignore it; WM8903 shouldn't be generating interrupts.
+	 */
+	if (wm8903->codec->dapm.bias_level == SND_SOC_BIAS_OFF)
+		return IRQ_NONE;
+
 	ret = regmap_read(wm8903->regmap, WM8903_INTERRUPT_STATUS_1_MASK,
 			  &mask);
 	if (ret != 0) {
-- 
1.8.1.5

Re: [PATCH] ASoC: wm8903: disable IRQs during BIAS_OFF

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1954 bytes --]

On Mon, Sep 09, 2013 at 12:44:49PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> If the WM8903 IRQ is triggered after the device is suspended, or before
> it is resumed, the underlying I2C bus that the WM8903 is attached to may
> not be available. This may cause the IRQ handler to emit the following
> error:
> 
>     wm8903 0-001a: Failed to read IRQ status: -16
> 
> ... or perhaps even cause a failure inside the I2C driver.
> 
> To prevent this, explicitly mask all interrupt sources when entering
> BIAS_OFF. Also, modify the IRQ handler to immediately return if the
> device is in BIAS_OFF, so that shared interrupt lines don't cause a
> similar problem. This requires that WM8903 interrupts be disabled to
> avoid interrupt storms.
> 
> During resume (actually, the transition back to BIAS_STANDBY), restore
> the desired IRQ mask.

This should be being handled in at a framework level, there's a generic
problem here that affects essentially every device out there which might
need interrupts over suspend (and causes relatively frequent problems).
Your fix also won't help if someone has arranged for the wm8903 mic
detect to be a wake source which you normally want in phone or tablet
type applications but is where this sort of issue most frequently bites
since we'll never turn the device off the interrupt does need to be
asserted to kick off the wake.

Drivers usually work around this by disabling their interrupt with
disable_irq() between suspend() and suspend_late(), then doing the same
thing again between resume_noirq() and resume() covering both the
windows where this can happen.  This preserves the ability to wake from
the interrupt but it's not awesome; ideally genirq would be able to
understand that it can't deliver interrupts to a suspended device but
then of course at the minute genirq has no idea that a device is even
associated with the interrupt.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: wm8903: disable IRQs during BIAS_OFF

[Stephen Warren]

On 09/09/2013 02:27 PM, Mark Brown wrote:
> On Mon, Sep 09, 2013 at 12:44:49PM -0600, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>> 
>> If the WM8903 IRQ is triggered after the device is suspended, or
>> before it is resumed, the underlying I2C bus that the WM8903 is
>> attached to may not be available. This may cause the IRQ handler
>> to emit the following error:
>> 
>> wm8903 0-001a: Failed to read IRQ status: -16
>> 
>> ... or perhaps even cause a failure inside the I2C driver.
>> 
>> To prevent this, explicitly mask all interrupt sources when
>> entering BIAS_OFF. Also, modify the IRQ handler to immediately
>> return if the device is in BIAS_OFF, so that shared interrupt
>> lines don't cause a similar problem. This requires that WM8903
>> interrupts be disabled to avoid interrupt storms.
>> 
>> During resume (actually, the transition back to BIAS_STANDBY),
>> restore the desired IRQ mask.
> 
> This should be being handled in at a framework level, there's a
> generic problem here that affects essentially every device out
> there which might need interrupts over suspend (and causes
> relatively frequent problems). Your fix also won't help if someone
> has arranged for the wm8903 mic detect to be a wake source which
> you normally want in phone or tablet type applications but is where
> this sort of issue most frequently bites since we'll never turn the
> device off the interrupt does need to be asserted to kick off the
> wake.
> 
> Drivers usually work around this by disabling their interrupt with 
> disable_irq() between suspend() and suspend_late(), then doing the
> same thing again between resume_noirq() and resume() covering both
> the windows where this can happen.  This preserves the ability to
> wake from the interrupt but it's not awesome; ideally genirq would
> be able to understand that it can't deliver interrupts to a
> suspended device but then of course at the minute genirq has no
> idea that a device is even associated with the interrupt.

So I suppose request_irq() could be enhanced to associate IRQs with
devices, which would at least give genirq the knowledge it needs to
mask interrupts before devices were resumed.

However, I'm not sure it's possible for genirq to handle this
transparently in all situations.

When would the IRQ be re-enabled again?

If it the IRQ is enabled immediately before the device's resume() is
called, then IRQs can still fire while the device is suspended. That
would solve the issue at hand here; that the I2C device used for
register IO wasn't resumed when the IRQ fired, but could presumably
still be problematic for the device itself, in general if not in this
case. Perhaps drivers just need to support this?

If the IRQ is enabled immediately after the device's resume() is
called, then a device wouldn't be able to receive IRQs during the
execution of resume(), which I assume would completely break some drivers?

This gets more complicated with shared IRQ lines, since it wouldn't be
possible to enable the IRQ immediately before/after *a* device's
resume() was called, but rather, this could only happen immediately
before/after the resume() for *all* devices using that IRQ were called.

It seems like the only way a driver can really manage IRQ handling in
the general case is to mask interrupts at the source, like this patch
does, rather than mask them at the interrupt controller, where the IRQ
may already be shared.

I guess that means that for wakeup interrupts, the driver does indeed
need to disable the interrupt at the source between resume_noirq() and
resume(), although I have no idea how that could possibly work, if the
register IO is over an I2C device that itself uses its own interrupts...

Re: [PATCH] ASoC: wm8903: disable IRQs during BIAS_OFF

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 3609 bytes --]

On Wed, Sep 11, 2013 at 10:16:56AM -0600, Stephen Warren wrote:
> On 09/09/2013 02:27 PM, Mark Brown wrote:

> > Drivers usually work around this by disabling their interrupt with 
> > disable_irq() between suspend() and suspend_late(), then doing the
> > same thing again between resume_noirq() and resume() covering both
> > the windows where this can happen.  This preserves the ability to

> However, I'm not sure it's possible for genirq to handle this
> transparently in all situations.

> When would the IRQ be re-enabled again?

See above - during the period between the device being suspended and
interrupts being globally disabled, and then during the period between
interrupts being reenabled and the device resumed.

> If it the IRQ is enabled immediately before the device's resume() is
> called, then IRQs can still fire while the device is suspended. That
> would solve the issue at hand here; that the I2C device used for
> register IO wasn't resumed when the IRQ fired, but could presumably
> still be problematic for the device itself, in general if not in this
> case. Perhaps drivers just need to support this?

Yes, that's a problem if it's an issue in the driver/device itself - in
that case it seems reasonable for the device to be responsible for
quiescing itself (as they mostly do already).

> If the IRQ is enabled immediately after the device's resume() is
> called, then a device wouldn't be able to receive IRQs during the
> execution of resume(), which I assume would completely break some drivers?

I think you're right there.

> This gets more complicated with shared IRQ lines, since it wouldn't be
> possible to enable the IRQ immediately before/after *a* device's
> resume() was called, but rather, this could only happen immediately
> before/after the resume() for *all* devices using that IRQ were called.

Yeah.  Fortunately I've only ever run into this with threaded interrupts
so they can't be shared in the first place (which to be fair tends to
come along with the buses that cause issues here).

> It seems like the only way a driver can really manage IRQ handling in
> the general case is to mask interrupts at the source, like this patch
> does, rather than mask them at the interrupt controller, where the IRQ
> may already be shared.

The problem is that this doesn't actually fix the problem in some quite
common cases, you can still trigger it so long as the device is still
supposed to act as a wake source (and hence won't actually be suspended)
- masking the interrupts at the source would actively break those
systems since they do want to be woken by the device.  To make matters
worse if the device is the wake source then the interrupt will be
asserted during resume making it more likely that the resume side of the
issue will be triggered.

> I guess that means that for wakeup interrupts, the driver does indeed
> need to disable the interrupt at the source between resume_noirq() and
> resume(), although I have no idea how that could possibly work, if the
> register IO is over an I2C device that itself uses its own interrupts...

The disable needs to be done with disable_irq() rather than by
interacting with the device, just stop the interrupts being delivered
until the device is capable of handling them rather than stopping the
hardware asserting them.  Fortunately since genirq doesn't allow
threaded interrupts to be shared I2C devices won't normally have to
worry about how rude this is for shared interrupts and generally the
buses affected by this will be threaded.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]