diff mbox series

regmap-irq: Set IRQCHIP_MASK_ON_SUSPEND if no wake_base

Message ID 20230717191654.1303285-1-samuel.holland@sifive.com
State New
Headers
Series regmap-irq: Set IRQCHIP_MASK_ON_SUSPEND if no wake_base |

Commit Message

Samuel Holland July 17, 2023, 7:16 p.m. UTC 
  If hardware provides no way to control which IRQs are wakeup enabled,
then software needs to mask non-wakeup-enabled IRQs when going to sleep.

Fixes: 55ac85e942c6 ("regmap: irq: enable wake support by default")
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

 drivers/base/regmap/regmap-irq.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Samuel Holland July 17, 2023, 7:54 p.m. UTC | #1 
On Jul 17, 2023, at 2:22 PM, Mark Brown <broonie@kernel.org> wrote:
> 
> On Mon, Jul 17, 2023 at 12:16:54PM -0700, Samuel Holland wrote:
> 
>> If hardware provides no way to control which IRQs are wakeup enabled,
>> then software needs to mask non-wakeup-enabled IRQs when going to sleep.
> 
> This isn't an unambigiously clear statement, especially for MFDs where
> there might be a desire to have some function on the device be able to
> wake the system (eg, headset button press on an audio CODEC or a RTC on
> a PMIC) even if there's no control within the device...
> 
>> Fixes: 55ac85e942c6 ("regmap: irq: enable wake support by default")
> 
> ...as the commit log does hint at.  If there's a problem I think we need
> some finer grained control here.

The current problem is that if wakeup is enabled for any IRQ in the chip
(say, the PMIC's power button), then we enable wakeup for the parent IRQ,
and now suddenly all (enabled) IRQs on the PMIC are also inadvertently
wakeup-enabled.

But I realize this patch does not actually solve the issue, for a couple
of reasons:
1) regmap-irq does not implement .irq_mask, just .irq_disable.
2) The __disable_irq() call in suspend_device_irq() should be sufficient,
   except that we fail the irq_settings_is_nested_thread() check in
   suspend_device_irqs().

So maybe the real issue is that commit 3c646f2c6aa9 ("genirq: Don't
suspend nested_thread irqs over system suspend") missed the case where
the child IRQ should be suspended, but the parent IRQ should not.

If that is fixed, then suspend_device_irq() should do the right thing
without any changes to the regmap-irq code.

Sorry for the noise.
Mark Brown July 17, 2023, 9:05 p.m. UTC | #2 
On Mon, Jul 17, 2023 at 02:54:18PM -0500, Samuel Holland wrote:

> The current problem is that if wakeup is enabled for any IRQ in the chip
> (say, the PMIC's power button), then we enable wakeup for the parent IRQ,
> and now suddenly all (enabled) IRQs on the PMIC are also inadvertently
> wakeup-enabled.

Yeah, I can see the issue.

> But I realize this patch does not actually solve the issue, for a couple
> of reasons:
> 1) regmap-irq does not implement .irq_mask, just .irq_disable.

It's possible we're doing the wrong thing here?  It's been a decade or
something since I wrote this code.

> 2) The __disable_irq() call in suspend_device_irq() should be sufficient,
>    except that we fail the irq_settings_is_nested_thread() check in
>    suspend_device_irqs().

> So maybe the real issue is that commit 3c646f2c6aa9 ("genirq: Don't
> suspend nested_thread irqs over system suspend") missed the case where
> the child IRQ should be suspended, but the parent IRQ should not.

> If that is fixed, then suspend_device_irq() should do the right thing
> without any changes to the regmap-irq code.

> Sorry for the noise.

No worries, I can see the logic and it feels like there might be
something to look at here, I'm just not sure that's it.
diff mbox series

Patch

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index ced0dcf86e0b..d7e076cff6e1 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -728,6 +728,8 @@  int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
 
 	d->irq_chip = regmap_irq_chip;
 	d->irq_chip.name = chip->name;
+	if (!chip->wake_base)
+		d->irq_chip.flags |= IRQCHIP_MASK_ON_SUSPEND;
 	d->irq = irq;
 	d->map = map;
 	d->chip = chip;