Re: [PATCH] genirq/PM: Fix IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND if depth > 1

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, Jun 10 2025 at 09:43, Doug Anderson wrote:
> On Tue, May 13, 2025 at 4:02 PM Brian Norris <briannorris@chromium.org> wrote:
>> It seems to me (again, not an expert) that maybe you need to solve your
>> problems by dodging the disable-depth entirely. But I'm not sure the
>> best way to do that.
>
> I can give a shot at spinning the patch, but before doing so I'd love
> to get agreement that this problem is worth solving. As I said above,
> we're not actually hitting this in any real cases and the issue was
> just found during code review. To me it feels like it's a real
> (potential) bug and worth solving before it bites someone in the
> future, but I won't force the issue and I'll drop the patch if that's
> what everyone wants.

I don't have a strong opinion either way.

> If it's agreed that I should move forward, I'd love advice on which
> approach I should use. Should I do as Brian says and try to sidestep
> disable-depth entirely in this case? I could factor out the "case 1"
> case of __enable_irq() and call it directly and then make sure that
> all I do is count "depth" while `IRQD_IRQ_ENABLED_ON_SUSPEND` is set.
> That doesn't seem like it would be too ugly...

No. That's creating inconstent state. It's already ugly enough. So if we
go there then we make it explicit like we did for the managed case,
i.e. something like this

void enable_wakeup_irq(struct irq_desc *desc)
{
	irqd_set(irqd, IRQD_IRQ_ENABLED_ON_SUSPEND);
	desc->saved_depth = desc->depth;
	desc->depth = 1;
	__enable_irq(desc);
}

and then have a counterpart which disables and restores the state.

Thanks,

        tglx