[PATCH v2 0/2] Fixups for cancelled hibernate

[PATCH v2 0/2] Fixups for cancelled hibernate

[Mario Limonciello (AMD)]

Muhammad Usama Anjun's recent series for being able to cancel
the hibernate sequence [1] exposes a bug with amdgpu handling for
skipping the thaw step.

Because the thaw step is skipped in most cases, cancelling the
hibernate means that the device is left in an inconsistent
state.

Add support to the PM core to let a driver track it's frozen
state when an error code is returned during thaw(). This will
ensure that the poweroff() and restore() callbacks get run.

Cc: Muhammad Usama Anjum <usama.anjum@collabora.com>

I suggest this patch series merge through linux-pm.

v2:
 * pick up tag
 * rebase on linux-pm/bleeding-edge
 * fix lkp robot issue

Mario Limonciello (AMD) (2):
  PM: Allow device drivers to manage the frozen state of a device
  drm/amd: Manage frozen state internally

 Documentation/driver-api/pm/devices.rst    |  8 ++++++++
 drivers/base/power/main.c                  |  7 +++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 ++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  2 +-
 include/linux/pm.h                         |  3 +++
 5 files changed, 29 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH v2 1/2] PM: Allow device drivers to manage the frozen state of a device

[Mario Limonciello (AMD)]

During a normal successful hibernate sequence devices will go through
the freeze() callbacks create an image, go through the thaw() callbacks,
and poweroff() callbacks.

During a successful hibernate sequence some device drivers may want to
skip the thaw() callbacks.  This confuses the PM core though because it
thinks the device is no longer suspended.

To accommodate drivers that want to do this, introduce a new is_frozen
bit that the driver can set and manage.  From the driver perspective
any thaw() or restore() callbacks that are being skipped should set
is_frozen and return an error code.  The PM core will then put the
device back into the list of devices to resume for any aborted hibernate.

Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v2:
 * add tag
 * fix lkp robot issue
 * rebase on linux-pm/bleeding-edge
---
 Documentation/driver-api/pm/devices.rst | 8 ++++++++
 drivers/base/power/main.c               | 7 +++++++
 include/linux/pm.h                      | 3 +++
 3 files changed, 18 insertions(+)

diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst
index 36d5c9c9fd113..55c6337271086 100644
--- a/Documentation/driver-api/pm/devices.rst
+++ b/Documentation/driver-api/pm/devices.rst
@@ -578,6 +578,14 @@ should already have been stored during the ``freeze``, ``freeze_late`` or
 the entire system, so it is not necessary for the callback to put the device in
 a low-power state.
 
+Skipping thaw phase
+-------------------
+In some rare situations, it may be desirable to skip the thaw phases
+(``thaw_noirq``, ``thaw_early``, ``thaw``) of a device entirely.  This can be
+achieved by a device driver returning an error code from any of it's thaw
+callbacks but also setting dev->power.is_frozen to true.  This indicates to the
+PM core that the device is still in the frozen state.  The PM core will consider
+this when resuming the device in later phases such as `restore` or `poweroff`.
 
 Leaving Hibernation
 -------------------
diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 7a8807ec9a5d0..c5a192fc04344 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1110,6 +1110,13 @@ static void device_resume(struct device *dev, pm_message_t state, bool async)
 
  End:
 	error = dpm_run_callback(callback, dev, state, info);
+#ifdef CONFIG_HIBERNATE_CALLBACKS
+	/* device manages frozen state */
+	if (error && dev->power.is_frozen) {
+		dev->power.is_suspended = true;
+		error = 0;
+	}
+#endif
 
 	device_unlock(dev);
 	dpm_watchdog_clear(&wd);
diff --git a/include/linux/pm.h b/include/linux/pm.h
index a72e42eec1303..852902fc72158 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -689,6 +689,9 @@ struct dev_pm_info {
 #else
 	bool			should_wakeup:1;
 #endif
+#ifdef CONFIG_HIBERNATE_CALLBACKS
+	bool			is_frozen:1;	/* Owned by the driver */
+#endif
 #ifdef CONFIG_PM
 	struct hrtimer		suspend_timer;
 	u64			timer_expires;
-- 
2.43.0

Re: [PATCH v2 1/2] PM: Allow device drivers to manage the frozen state of a device

[Rafael J. Wysocki]

On Sat, Oct 25, 2025 at 3:01 AM Mario Limonciello (AMD)
<superm1@kernel.org> wrote:
>
> During a normal successful hibernate sequence devices will go through
> the freeze() callbacks create an image, go through the thaw() callbacks,
> and poweroff() callbacks.
>
> During a successful hibernate sequence some device drivers may want to
> skip the thaw() callbacks.  This confuses the PM core though because it
> thinks the device is no longer suspended.

The problem only really occurs if hibernation is aborted before or
during the final "poweroff" transition.

What happens is that if a driver decides to leave the device in the
"frozen" state during its "thaw" callback and its "poweroff" callback
is not invoked because hibernation is aborted earlier, the device will
be left in the "frozen" state going forward.

The goal of the change should be to make the core detect that
situation and "thaw" the device.

> To accommodate drivers that want to do this, introduce a new is_frozen
> bit that the driver can set and manage.  From the driver perspective
> any thaw() or restore() callbacks that are being skipped should set
> is_frozen and return an error code.

"Restore" has nothing to do with this, it is just about "freeze".

> The PM core will then put the device back into the list of devices to resume for any aborted hibernate.

That's not what the patch does, though (see below).

All of this is mainly about what the core should do when it sees the
"is_frozen" flag set, so a few observations to that end:

1. That flag is only relevant when hibernation is aborted before
invoking the given driver's "poweroff" callback (because that callback
must be prepared to deal with a "frozen" device).

2. If the final "poweroff" transition is aborted during its "prepare"
phase, the "frozen" device may need to be "thawed" even if its
driver's "prepare" callback is not invoked.

3. There is a possibility, not taken into account so far, that
hibernation is aborted because of a failure to save the image.  Then,
"frozen" devices will need to be "thawed" before starting the final
"poweroff" transition.

Moreover, I'm not sure if it really makes sense to invoke "complete"
callbacks during the "thaw" transition for the devices left in the
"frozen" state.

All of the above means that the approach needs to be rethought and my
advice is to revert the commit causing the AMD driver to leave its
device in the "frozen" for 6.18 (and previous kernels).

> Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
> ---
> v2:
>  * add tag
>  * fix lkp robot issue
>  * rebase on linux-pm/bleeding-edge
> ---
>  Documentation/driver-api/pm/devices.rst | 8 ++++++++
>  drivers/base/power/main.c               | 7 +++++++
>  include/linux/pm.h                      | 3 +++
>  3 files changed, 18 insertions(+)
>
> diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst
> index 36d5c9c9fd113..55c6337271086 100644
> --- a/Documentation/driver-api/pm/devices.rst
> +++ b/Documentation/driver-api/pm/devices.rst
> @@ -578,6 +578,14 @@ should already have been stored during the ``freeze``, ``freeze_late`` or
>  the entire system, so it is not necessary for the callback to put the device in
>  a low-power state.
>
> +Skipping thaw phase
> +-------------------
> +In some rare situations, it may be desirable to skip the thaw phases
> +(``thaw_noirq``, ``thaw_early``, ``thaw``) of a device entirely.  This can be
> +achieved by a device driver returning an error code from any of it's thaw
> +callbacks but also setting dev->power.is_frozen to true.

Returning an error code should not be necessary.

Also this needs to be done in "thaw_noirq" or maybe even in
"freeze_noirq" because "thaw_noirq" may involve some bus type actions
changing the state of the device before the driver gets to it.

So the driver would opt in for leaving the device in the "frozen"
state at the "noirq" stage of the preceding "freeze" transition.  That
can be achieved by setting a "leave_in_freeze" flag, so no callbacks
would be run for any devices with that flag set during the subsequent
"thaw" transition.

If hibernation is aborted before the final "poweroff" transition
begins, the "thaw*" and "complete" callbacks will have to be run for
all of those devices (I'm wondering if any ordering issues may arise
at that point; presumably, devices that depend on the "frozen" ones
would also need to be "frozen"?).

During the final "poweroff" transition, since "complete" has not been
called for any of the "frozen" devices, it should not be necessary to
call "prepare" for any of them, so that one can be skipped.

Again, if hibernation is aborted at this point, all of the "thaw*" and
complete callbacks need to be run for all of the "frozen" devices.

Now, "poweroff", "poweroff" and "poweroff_noirq" callbacks for the
"frozen" devices need to be prepared to deal with them, but the exact
rules there will need some consideration.  They kind of need to assume
that "freeze" may be changed into "poweroff" transparently without a
"thaw" in between and that generally depends on the bus type/PM domain
involved.

> This indicates to the
> +PM core that the device is still in the frozen state.  The PM core will consider
> +this when resuming the device in later phases such as `restore` or `poweroff`.
>
>  Leaving Hibernation
>  -------------------
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 7a8807ec9a5d0..c5a192fc04344 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1110,6 +1110,13 @@ static void device_resume(struct device *dev, pm_message_t state, bool async)
>
>   End:
>         error = dpm_run_callback(callback, dev, state, info);
> +#ifdef CONFIG_HIBERNATE_CALLBACKS

CONFIG_HIBERNATION should be sufficient.

> +       /* device manages frozen state */
> +       if (error && dev->power.is_frozen) {
> +               dev->power.is_suspended = true;
> +               error = 0;
> +       }

This assumes that the callback will run, but what if hibernation is
aborted before running it?  Isn't that really the problem at hand?

> +#endif
>
>         device_unlock(dev);
>         dpm_watchdog_clear(&wd);
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index a72e42eec1303..852902fc72158 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -689,6 +689,9 @@ struct dev_pm_info {
>  #else
>         bool                    should_wakeup:1;
>  #endif
> +#ifdef CONFIG_HIBERNATE_CALLBACKS
> +       bool                    is_frozen:1;    /* Owned by the driver */
> +#endif
>  #ifdef CONFIG_PM
>         struct hrtimer          suspend_timer;
>         u64                     timer_expires;
> --

Re: [PATCH v2 1/2] PM: Allow device drivers to manage the frozen state of a device

[Muhammad Usama Anjum]

On 11/4/25 12:50 AM, Rafael J. Wysocki wrote:
> On Sat, Oct 25, 2025 at 3:01 AM Mario Limonciello (AMD)
> <superm1@kernel.org> wrote:
>>
>> During a normal successful hibernate sequence devices will go through
>> the freeze() callbacks create an image, go through the thaw() callbacks,
>> and poweroff() callbacks.
>>
>> During a successful hibernate sequence some device drivers may want to
>> skip the thaw() callbacks.  This confuses the PM core though because it
>> thinks the device is no longer suspended.

With RFC [1] applied and hibernation is cancelled until or from inside of
dpm_suspend(), this series restores the amdgpu driver. 

hibernate()
-> hibernation_snapshot()
   -> dpm_suspend()

> 
> The problem only really occurs if hibernation is aborted before or
> during the final "poweroff" transition.
This isn't supported yet. I'm working on a new patch to cancel hibernation
after dpm_suspend() and before power_down(). But it causes missed resumption
of amdgpu even after applying this series. Probably some 
dpm_resume_start(PMSG_RECOVER) and dpm_resume_end(PMSG_RECOVER) are missing.
I've not been able to sort it out. 

Its a very late stage and I'm not getting console logs. I don't have serial
connection to get those logs as well.

I'll send the series without this patch in coming days if I'm not able to sort
it out.

[1] https://lore.kernel.org/all/20251018142114.897445-1-usama.anjum@collabora.com 

> 
> What happens is that if a driver decides to leave the device in the
> "frozen" state during its "thaw" callback and its "poweroff" callback
> is not invoked because hibernation is aborted earlier, the device will
> be left in the "frozen" state going forward.
> 
> The goal of the change should be to make the core detect that
> situation and "thaw" the device.
> 
>> To accommodate drivers that want to do this, introduce a new is_frozen
>> bit that the driver can set and manage.  From the driver perspective
>> any thaw() or restore() callbacks that are being skipped should set
>> is_frozen and return an error code.
> 
> "Restore" has nothing to do with this, it is just about "freeze".
> 
>> The PM core will then put the device back into the list of devices to resume for any aborted hibernate.
> 
> That's not what the patch does, though (see below).
> 
> All of this is mainly about what the core should do when it sees the
> "is_frozen" flag set, so a few observations to that end:
> 
> 1. That flag is only relevant when hibernation is aborted before
> invoking the given driver's "poweroff" callback (because that callback
> must be prepared to deal with a "frozen" device).
> 
> 2. If the final "poweroff" transition is aborted during its "prepare"
> phase, the "frozen" device may need to be "thawed" even if its
> driver's "prepare" callback is not invoked.
> 
> 3. There is a possibility, not taken into account so far, that
> hibernation is aborted because of a failure to save the image.  Then,
> "frozen" devices will need to be "thawed" before starting the final
> "poweroff" transition.
> 
> Moreover, I'm not sure if it really makes sense to invoke "complete"
> callbacks during the "thaw" transition for the devices left in the
> "frozen" state.
> 
> All of the above means that the approach needs to be rethought and my
> advice is to revert the commit causing the AMD driver to leave its
> device in the "frozen" for 6.18 (and previous kernels).
> 
>> Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>> Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
>> ---
>> v2:
>>  * add tag
>>  * fix lkp robot issue
>>  * rebase on linux-pm/bleeding-edge
>> ---
>>  Documentation/driver-api/pm/devices.rst | 8 ++++++++
>>  drivers/base/power/main.c               | 7 +++++++
>>  include/linux/pm.h                      | 3 +++
>>  3 files changed, 18 insertions(+)
>>
>> diff --git a/Documentation/driver-api/pm/devices.rst b/Documentation/driver-api/pm/devices.rst
>> index 36d5c9c9fd113..55c6337271086 100644
>> --- a/Documentation/driver-api/pm/devices.rst
>> +++ b/Documentation/driver-api/pm/devices.rst
>> @@ -578,6 +578,14 @@ should already have been stored during the ``freeze``, ``freeze_late`` or
>>  the entire system, so it is not necessary for the callback to put the device in
>>  a low-power state.
>>
>> +Skipping thaw phase
>> +-------------------
>> +In some rare situations, it may be desirable to skip the thaw phases
>> +(``thaw_noirq``, ``thaw_early``, ``thaw``) of a device entirely.  This can be
>> +achieved by a device driver returning an error code from any of it's thaw
>> +callbacks but also setting dev->power.is_frozen to true.
> 
> Returning an error code should not be necessary.
> 
> Also this needs to be done in "thaw_noirq" or maybe even in
> "freeze_noirq" because "thaw_noirq" may involve some bus type actions
> changing the state of the device before the driver gets to it.
> 
> So the driver would opt in for leaving the device in the "frozen"
> state at the "noirq" stage of the preceding "freeze" transition.  That
> can be achieved by setting a "leave_in_freeze" flag, so no callbacks
> would be run for any devices with that flag set during the subsequent
> "thaw" transition.
> 
> If hibernation is aborted before the final "poweroff" transition
> begins, the "thaw*" and "complete" callbacks will have to be run for
> all of those devices (I'm wondering if any ordering issues may arise
> at that point; presumably, devices that depend on the "frozen" ones
> would also need to be "frozen"?).
> 
> During the final "poweroff" transition, since "complete" has not been
> called for any of the "frozen" devices, it should not be necessary to
> call "prepare" for any of them, so that one can be skipped.
> 
> Again, if hibernation is aborted at this point, all of the "thaw*" and
> complete callbacks need to be run for all of the "frozen" devices.
> 
> Now, "poweroff", "poweroff" and "poweroff_noirq" callbacks for the
> "frozen" devices need to be prepared to deal with them, but the exact
> rules there will need some consideration.  They kind of need to assume
> that "freeze" may be changed into "poweroff" transparently without a
> "thaw" in between and that generally depends on the bus type/PM domain
> involved.
> 
>> This indicates to the
>> +PM core that the device is still in the frozen state.  The PM core will consider
>> +this when resuming the device in later phases such as `restore` or `poweroff`.
>>
>>  Leaving Hibernation
>>  -------------------
>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> index 7a8807ec9a5d0..c5a192fc04344 100644
>> --- a/drivers/base/power/main.c
>> +++ b/drivers/base/power/main.c
>> @@ -1110,6 +1110,13 @@ static void device_resume(struct device *dev, pm_message_t state, bool async)
>>
>>   End:
>>         error = dpm_run_callback(callback, dev, state, info);
>> +#ifdef CONFIG_HIBERNATE_CALLBACKS
> 
> CONFIG_HIBERNATION should be sufficient.
> 
>> +       /* device manages frozen state */
>> +       if (error && dev->power.is_frozen) {
>> +               dev->power.is_suspended = true;
>> +               error = 0;
>> +       }
> 
> This assumes that the callback will run, but what if hibernation is
> aborted before running it?  Isn't that really the problem at hand?
> 
>> +#endif
>>
>>         device_unlock(dev);
>>         dpm_watchdog_clear(&wd);
>> diff --git a/include/linux/pm.h b/include/linux/pm.h
>> index a72e42eec1303..852902fc72158 100644
>> --- a/include/linux/pm.h
>> +++ b/include/linux/pm.h
>> @@ -689,6 +689,9 @@ struct dev_pm_info {
>>  #else
>>         bool                    should_wakeup:1;
>>  #endif
>> +#ifdef CONFIG_HIBERNATE_CALLBACKS
>> +       bool                    is_frozen:1;    /* Owned by the driver */
>> +#endif
>>  #ifdef CONFIG_PM
>>         struct hrtimer          suspend_timer;
>>         u64                     timer_expires;
>> --


-- 
---
Thanks,
Usama

[PATCH v2 2/2] drm/amd: Manage frozen state internally

[Mario Limonciello (AMD)]

[Why]
On a normal hibernate sequence amdgpu will skip the thaw step due to
commit 530694f54dd5e ("drm/amdgpu: do not resume device in thaw for
normal hibernation").

If the hibernate sequence has been aborted however after this thawed
step runs the PM core will think the device is suspended and will skip
the restore() sequence for amdgpu.  This leads to accessing the device
while in a low power state and will freeze the system.

[How]
Set `dev->power.is_frozen` to indicate to the PM core that an error
code will be returned for thaw() callback because driver managed the
frozen state.  If the restore() callback is called by the PM core the
driver will resume the device.

Tested-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
Signed-off-by: Mario Limonciello (AMD) <superm1@kernel.org>
---
v2:
 * add tag
 * rebase on linux-pm/bleeding-edge
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 ++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 3d032c4e2dce1..693347eb6861b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -5247,6 +5247,11 @@ int amdgpu_device_suspend(struct drm_device *dev, bool notify_clients)
 	if (r)
 		return r;
 
+#ifdef CONFIG_HIBERNATE_CALLBACKS
+	if (adev->in_s4)
+		dev->dev->power.is_frozen = 1;
+#endif
+
 	return 0;
 }
 
@@ -5385,6 +5390,11 @@ int amdgpu_device_resume(struct drm_device *dev, bool notify_clients)
 	if (amdgpu_acpi_smart_shift_update(adev, AMDGPU_SS_DEV_D0))
 		dev_warn(adev->dev, "smart shift update failed\n");
 
+#ifdef CONFIG_HIBERNATE_CALLBACKS
+	if (adev->in_s4)
+		dev->dev->power.is_frozen = 0;
+#endif
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 61268aa82df4d..d40af069f24dd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2681,7 +2681,7 @@ static int amdgpu_pmops_thaw(struct device *dev)
 
 	/* do not resume device if it's normal hibernation */
 	if (!pm_hibernate_is_recovering() && !pm_hibernation_mode_is_suspend())
-		return 0;
+		return -EBUSY;
 
 	return amdgpu_device_resume(drm_dev, true);
 }
-- 
2.43.0