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

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

[Jan Kiszka]

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();
 
-- 
2.43.0

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

[Florian Bezdeka]

On Thu, 2025-03-20 at 17:41 +0100, Jan Kiszka wrote:
> 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.

How was that tested? I think up to now nobody really cared about
hibernation... Will that really work now when I try to test that or
will I run into (other) problems?


> 
>  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();
>  
> -- 
> 2.43.0

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

[Jan Kiszka]

On 20.03.25 17:50, Florian Bezdeka wrote:
> On Thu, 2025-03-20 at 17:41 +0100, Jan Kiszka wrote:
>> 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.
> 
> How was that tested? I think up to now nobody really cared about
> hibernation... Will that really work now when I try to test that or
> will I run into (other) problems?
> 

This was not tested yet, but the affected code is patched since an
eternity. If we don't want to support this anymore, we must remove the
changes AND declare CONFIG_HIBERNATION incompatible to
CONFIG_IRQ_PIPELINE instead.

Jan

> 
>>
>>  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();
>>  
>> -- 
>> 2.43.0
> 


-- 
Siemens AG, Foundational Technologies
Linux Expert Center

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

[Philippe Gerum]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 20.03.25 17:50, Florian Bezdeka wrote:
>> On Thu, 2025-03-20 at 17:41 +0100, Jan Kiszka wrote:
>>> 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.
>> 
>> How was that tested? I think up to now nobody really cared about
>> hibernation... Will that really work now when I try to test that or
>> will I run into (other) problems?
>> 
>
> This was not tested yet, but the affected code is patched since an
> eternity. If we don't want to support this anymore, we must remove the
> changes AND declare CONFIG_HIBERNATION incompatible to
> CONFIG_IRQ_PIPELINE instead.
>

We may definitely have a system running a real-time core which is
allowed to hibernate at specific times (e.g. a copier running Xenomai
would typically do this). This used to work fine for a couple of
projects I can think of (granted, a long time ago). Let's fix the code.

-- 
Philippe.

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

[Philippe Gerum]

Florian Bezdeka <florian.bezdeka@siemens.com> writes:

> On Thu, 2025-03-20 at 17:41 +0100, Jan Kiszka wrote:
>> 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.
>
> How was that tested? I think up to now nobody really cared about
> hibernation... Will that really work now when I try to test that or
> will I run into (other) problems?

We can write to /sys/power/state to trigger the suspend-to-disk sequence.

-- 
Philippe.

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

[Philippe Gerum]

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.

-- 
Philippe.

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

[Florian Bezdeka]

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.

Florian

> 
> -- 
> Philippe.

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

[Philippe Gerum]

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.

-- 
Philippe.

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

[Philippe Gerum]

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.

-- 
Philippe.

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

[Philippe Gerum]

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.

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

[Florian Bezdeka]

On Thu, 2025-03-20 at 17:41 +0100, Jan Kiszka wrote:
> 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();

General question about ordering here:

The order hard_cond_local_irq_enable() followed by local_irq_enable()
seems wrong to me. We enable hard irqs first, then we unstall the
inband stage.

That means that an hard irq might arrive and will end up in the IRQ
log, which is not processed by local_irq_enable(), so we are delaying
the IRQ processing to an unknown later time.

The other way around would make more sense to me. Is my understanding
right?

>  
> @@ -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();
>  
> -- 
> 2.43.0

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

[Jan Kiszka]

On 24.03.25 12:18, Florian Bezdeka wrote:
> On Thu, 2025-03-20 at 17:41 +0100, Jan Kiszka wrote:
>> 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();
> 
> General question about ordering here:
> 
> The order hard_cond_local_irq_enable() followed by local_irq_enable()
> seems wrong to me. We enable hard irqs first, then we unstall the
> inband stage.

Normally, this is the right ordering because you cannot process pending
Linux IRQs with hard IRQs disabled.

> 
> That means that an hard irq might arrive and will end up in the IRQ
> log, which is not processed by local_irq_enable(), so we are delaying
> the IRQ processing to an unknown later time.
> 

Why should local_irq_enable not process pending Linux IRQs?

Jan

> The other way around would make more sense to me. Is my understanding
> right?
> 
>>  
>> @@ -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();
>>  
>> -- 
>> 2.43.0
> 

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

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

[Philippe Gerum]

Florian Bezdeka <florian.bezdeka@siemens.com> writes:

> On Thu, 2025-03-20 at 17:41 +0100, Jan Kiszka wrote:
>> 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();
>
> General question about ordering here:
>
> The order hard_cond_local_irq_enable() followed by local_irq_enable()
> seems wrong to me. We enable hard irqs first, then we unstall the
> inband stage.
>
> That means that an hard irq might arrive and will end up in the IRQ
> log, which is not processed by local_irq_enable(), so we are delaying
> the IRQ processing to an unknown later time.
>

We are not delaying any IRQ, the implementation would just log any
incoming IRQ into the log, until __inband_irq_enable() eventually
disables them the hard way to check the log content, calling
sync_current_irq_stage() if any is to be played back.

> The other way around would make more sense to me. Is my understanding
> right?
>

Nope. The proper ordering is _always_ hard_local_irq_enable() _then_
local_irq_enable. There is also an historical reason for this (see
comment in inband_irq_enable()).

-- 
Philippe.

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

[Jan Kiszka]

On 21.03.25 14:51, Philippe Gerum wrote:
> 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.
> 

At least in qemu/kvm, hibernation works in the way that linux suspends,
quickly resumes again (might be a virtual firmware issue), and the core
is still working afterwards. But I found some bug in a WARN_ON_ONCE,
patch will follow.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

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

[Florian Bezdeka]

On Mon, 2025-03-24 at 15:19 +0100, Philippe Gerum wrote:
> Florian Bezdeka <florian.bezdeka@siemens.com> writes:
> 
> > On Thu, 2025-03-20 at 17:41 +0100, Jan Kiszka wrote:
> > > 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();
> > 
> > General question about ordering here:
> > 
> > The order hard_cond_local_irq_enable() followed by local_irq_enable()
> > seems wrong to me. We enable hard irqs first, then we unstall the
> > inband stage.
> > 
> > That means that an hard irq might arrive and will end up in the IRQ
> > log, which is not processed by local_irq_enable(), so we are delaying
> > the IRQ processing to an unknown later time.
> > 
> 
> We are not delaying any IRQ, the implementation would just log any
> incoming IRQ into the log, until __inband_irq_enable() eventually
> disables them the hard way to check the log content, calling
> sync_current_irq_stage() if any is to be played back.
> 
> > The other way around would make more sense to me. Is my understanding
> > right?
> > 
> 
> Nope. The proper ordering is _always_ hard_local_irq_enable() _then_
> local_irq_enable. There is also an historical reason for this (see
> comment in inband_irq_enable()).

Thanks, my bad. Seems I overlooked that __inband_irq_enable() is
processing the IRQ log if there is a need to do so.

> 
> -- 
> Philippe.