On Thu, Dec 14 2023 at 09:54, xiongxin wrote:
在 2023/12/13 22:59, Thomas Gleixner 写道:
Did you actually look at the sequence I gave you?
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 markerHave you verified that this order of invocations is what happens on your machine?
As described earlier, in the current situation, the irq_mask() callback of gpio irq_chip is called in mask_irq(), followed by the disable_irq() in i2c_hid_core_suspend(), unmask_irq() will not be executed.
Which is correct.
Then call enable_irq() in i2c_hid_core_resume(). Since gpio irq_chip does not implement the irq_startup() callback, it ends up calling irq_enable().
The irq_enable() function is then implemented as follows:
irq_state_clr_disabled(desc); if (desc->irq_data.chip->irq_enable) { desc->irq_data.chip->irq_enable(&desc->irq_data); irq_state_clr_masked(desc); } else { unmask_irq(desc); }
Because gpio irq_chip implements irq_enable(), unmask_irq() is not executed, and gpio irq_chip's irq_unmask() callback is not called. Instead, irq_state_clr_masked() was called to clear the masked flag.
The irq masked behavior is actually controlled by the irq_mask()/irq_unmask() callback function pairs in gpio irq_chip. When the whole situation occurs, there is one more irq_mask() operation, or one less irq_unmask() operation. This ends the i2c hid resume and the gpio corresponding i2c hid interrupt is also masked.
Please help confirm whether the current situation is a BUG, or suggest other solutions to fix it.
Again, I already explained to you in great detail why the core code is correct and does not have a bug.
But as you insist that the bug is in the core code you obviously failed to validate what I asked you to validate:
i2c_hid_core_resume() enable_irq(); <- Unmasks interrupt and removes the disabled marker
The keyword to validate here is 'Unmasks'.
As gpio_dwapb implements the irq_enable() callback enable_irq() is not going to end up invoking the irq_unmask() callback. But the irq_enable() callback is required to be a superset of irq_unmask(). I.e. the core code expects it to do:
1) Some preparatory work to enable the interrupt line
2) Unmask the interrupt, which is why the masked state is cleared by the core after invoking the irq_enable() callback.
which is pretty obvious because if an interrupt chip does not implement the irq_enable() callback the core defaults to irq_unmask()
Correspondingly the core expects from the irq_disable() callback:
1) To mask the interrupt
2) To do some extra work to disable the interrupt line
which is obvious again because the core defaults to irq_mask() if the irq_disable() callback is not implemented by the interrupt chip.
I'm pretty sure that with the previous provided information and the extra information above you can figure out yourself that:
1) the core code is correct as is
2) where exactly the problem is located and how to fix it
No?
Thanks,
tglx