Re: [PATCH] irq: Resolve that mask_irq/unmask_irq may not be called in pairs

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, Dec 11 2023 at 11:10, xiongxin@kylinos.cn wrote:
> 在 2023/12/8 21:52, Thomas Gleixner 写道:
>> On Thu, Dec 07 2023 at 09:40, xiongxin@kylinos.cn wrote:
>> Disabled interrupts are disabled and can only be reenabled by the
>> corresponding enable call. The existing code is entirely correct.
>> 
>> What you are trying to do is unmasking a disabled interrupt, which
>> results in inconsistent state.
>> 
>> Which interrupt chip is involved here?
>
> i2c hid driver use gpio interrupt controller like 
> drivers/gpio/gpio-dwapb.c, The gpio interrupt controller code implements 
> handle_level_irq() and irq_disabled().

No it does not. handle_level_irq() is implemented in the interrupt core
code and irq_disabled() is not a function at all.

Please describe things precisely and not by fairy tales.

> Normally, when using the i2c hid device, the gpio interrupt controller's 
> mask_irq() and unmask_irq() are called in pairs.

Sure. That's how the core code works.

> But when doing a sleep process, such as suspend to RAM, 
> i2c_hid_core_suspend() of the i2c hid driver is called, which implements 
> the disable_irq() function,

IOW, i2c_hid_core_suspend() disables the interrupt of the client device.

> which finally calls __irq_disable(). Because 
> the desc parameter is set to the __irq_disabled() function without a 
> lock (desk->lock), the __irq_disabled() function can be called during

That's nonsense.

disable_irq(irq)
  if (!__disable_irq_nosync(irq)
     desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);

            ^^^^^^^^^^^^^^^^^^^^ This locks the interrupt descriptor

And yes disable_irq() can be invoked when the interrupt is handled
concurrently. That's legitimate and absolutely correct, but that has
absolutely nothing to do with the locking.

The point is that after disable_irq() returns the interrupt handler is
guaranteed not to be running and not to be invoked anymore until
something invokes enable_irq().

The fact that disable_irq() marks the interrupt disabled prevents the
hard interrupt handler and the threaded handler to unmask the interrupt.
That's correct and fundamental to ensure that the interrupt is and stays
truly disabled.

> if (!irqd_irq_disabled() && irqd_irq_masked())
> 	unmask_irq();

> In this scenario, unmask_irq() will not be called, and then gpio 
> corresponding interrupt pin will be masked.

It _cannot_ be called because the interrupt is _disabled_, which means
the interrupt stays masked. Correctly so.

> Finally, in the suspend() process driven by gpio interrupt controller,
> the interrupt mask register will be saved, and then masked will
> continue to be read when resuming () process. After the kernel
> resumed, the i2c hid gpio interrupt was masked and the i2c hid device
> was unavailable.

That's just wrong again.

Suspend:

       i2c_hid_core_suspend()
          disable_irq();       <- Marks it disabled and eventually
                                  masks it.

       gpio_irq_suspend()
          save_registers();    <- Saves masked interrupt

Resume:

       gpio_irq_resume()
          restore_registers(); <- Restores masked interrupt

       i2c_hid_core_resume()
          enable_irq();        <- Unmasks interrupt and removes the
                                  disabled marker

As I explained you before, disable_irq() can only be undone by
enable_irq() and not by ignoring the disabled state somewhere
else. Disabled state is well defined.

So if the drivers behave correctly in terms of suspend/resume ordering
as shown above, then this all should just work.

If it does not then please figure out what's the actual underlying
problem instead of violating well defined constraints in the core code
and telling me fairy tales about the code.

Thanks,

        tglx