Re: [PATCH] dovetail: Fix interrupt re-enabling after hibernation

[Philippe Gerum <rpm@xenomai.org>]

Philippe Gerum <rpm@xenomai.org> writes:

> Philippe Gerum <rpm@xenomai.org> writes:
>
>> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
>>
>>> On Fri, 2025-03-21 at 08:51 +0100, Philippe Gerum wrote:
>>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>> 
>>>> > From: Jan Kiszka <jan.kiszka@siemens.com>
>>>> > 
>>>> > We had unbalanced hard_cond_local_irq_disable here so far.
>>>> > 
>>>> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> > ---
>>>> > 
>>>> > Minus one hunk that is already in current 6.12.y, this needs to be 
>>>> > applied to older stable versions as well.
>>>> > 
>>>> >  kernel/power/hibernate.c | 3 +++
>>>> >  1 file changed, 3 insertions(+)
>>>> > 
>>>> > diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
>>>> > index 733b1a196f09e..e1c05f276a453 100644
>>>> > --- a/kernel/power/hibernate.c
>>>> > +++ b/kernel/power/hibernate.c
>>>> > @@ -352,6 +352,7 @@ static int create_image(int platform_mode)
>>>> >  	syscore_resume();
>>>> >  
>>>> >   Enable_irqs:
>>>> > +	hard_cond_local_irq_enable();
>>>> >  	system_state = SYSTEM_RUNNING;
>>>> >  	local_irq_enable();
>>>> >  
>>>> > @@ -522,6 +523,7 @@ static int resume_target_kernel(bool platform_mode)
>>>> >  	syscore_resume();
>>>> >  
>>>> >   Enable_irqs:
>>>> > +	hard_cond_local_irq_enable();
>>>> >  	system_state = SYSTEM_RUNNING;
>>>> >  	local_irq_enable();
>>>> >  
>>>> > @@ -628,6 +630,7 @@ int hibernation_platform_enter(void)
>>>> >   Power_up:
>>>> >  	syscore_resume();
>>>> >   Enable_irqs:
>>>> > +	hard_cond_local_irq_enable();
>>>> >  	system_state = SYSTEM_RUNNING;
>>>> >  	local_irq_enable();
>>>> 
>>>> Merged into v6.12 and backported to v6.1.y-cip, thanks.
>>>
>>> Sorry, but I have to highlight that this patch does not help at all.
>>> Testing shows that we already "hang" when trying to hibernate. We never
>>> make it into the "wakeup" path.
>>>
>>> I think the "process" we followed here is broken. First we have to
>>> implement and *TEST* things in latest development branches and once we
>>> are sure we fully addressed the problem back porting can happen.
>>
>> The point is that this has never been a new and/or untested feature, we
>> may be seeing a regression on all versions of a feature that did work
>> for ages, the story does not start with v6.14. Now, the tricky issue
>> with hibernation is not necessarily in the code paths above, but most
>> likely in one of the many suspend/resume handlers, which are per-driver
>> beasts. First of all, we need to define a test configuration with
>> suspend/resume code which is known to work.
>
> Known to work when irq pipelining is enabled, I mean.

Food for thought: since the weak point of hibernation is definitely in
those suspend/resume handlers, a way to make any suspend-to-* sequence
stable without incurring a truckload of changes sprinkled all over the
map in those handlers, would be to temporarily switch the implementation
of the inband stall/unstall helpers to hard irq disabling/enabling
_while_ a suspend/resume sequence is ongoing. A bit like Dovetail's
so-called 'hybrid' spinlocks (hybrid_spinlock_t) are working for
irq_desc->lock.

i.e. use static branching in __inband_irq_enable(), inband_irq_save()
and alike in order to pair virtual masking using the stall bit with
hardware-level masking in the CPU.

With that scheme in place, interrupt bit virtualization would be
actually disabled while traversing the sensitive paths during
suspend/resume.

-- 
Philippe.