power: suspend: Add suspend timeout handler

PATCH In review [email protected] 2 participants 2 messages Oct 20, 2020

lore

Share & tools

lore

[email protected]

author

2020-10-20 09:01 UTC #0 lore raw

PATCH 1/1 power: suspend: Add suspend timeout handler

Quoted 63 lines · On Tue, Oct 20, 2020 at 08:15:38AM +0000, [email protected] wrote:

Quoted 4 lines
Quoted 4 lines · Joseph Jang
Quoted 1 line
Add sleep timer and timeout handler to prevent device stuck during
suspend/
Quoted 1 line
resume process. The timeout handler will dump disk sleep task at
first
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
round timeout and trigger kernel panic at second round timeout. The default timer for each round is defined in CONFIG_PM_SLEEP_TIMER_TIMEOUT.
Signed-off-by Joseph Jang <[email protected]>
---
 MAINTAINERS                   |  2 +
 include/linux/console.h       |  1 +
 include/linux/suspend_timer.h | 90  
+++++++++++++++++++++++++++++++++++
Quoted 2 lines
Quoted 2 lines
Why is this file in include/linux/ if you only ever call it from
one .c
Quoted 3 lines
Quoted 1 line
file?
I just refer to include/linux/suspend.h and create a new header file in
the
Quoted 2 lines
same folder.
If you have a better location for the new header file, please feel free
to
Quoted 1 line
let me know.
Only put .h files that are needed by different .c files in the include/linux/ directory. Otherwise it should be local to where the .c file is.
Great, use that!
Quoted 1 line
But we really hit the suspend hang issue that DPM_WATCHDOG cannot cover.
What issue is that?
Quoted 8 lines
We propose a wide coverage debug feature like PM_SLEEP_MONITOR which not only covers PM but also core PM hang issues.
And DPM_WATCHDOG is for device driver power management in drivers/base/power/main.c
and PM_SLEEP_MONITOR locate is for core power management in kernel/power/suspend.c.
I think it is fine for users to select whether they need device PM only
or
Quoted 1 line
not.
How will a user know which they should use?
Why not just fix whatever is wrong with the watchdog code instead of creating a new one?
Quoted 4 lines
Quoted 2 lines
And why isn't the watchdog sufficient for you? Why are you "open coding" a watchdog timer logic here at all???
Yes, we refer to DPM_WATCHDOG to extend the watchdog debugging for core
PM.
Quoted 1 line
Because we really hit a real case that was not covered by DPM_WATCHDOG.
Then fix that!
Quoted 1 line
I think PM_SLEEP_MONITOR is an extension debug feature from

Quoted 8 lines

DPM_WATCHDOG.
Please just fix the watchdog, as obviously it is not working properly. Don't create something new because of that.
thanks,
greg k-h

Thank you Greq for promptly responding.
I am okay to fix the DPM_WATCHDOG feature, but would like to have quick sync up before start. Could you help?

1. Can we change the kernel config name ? DPM_WATCHDOG stands for Device Power Management.
We propose PM_SLEEP_MONITOR is to cover Core PM and Device PM.

2. Can we create a new data structure instead of using the following struct dpm_watchdog?

struct dpm_watchdog {
	struct device		*dev;
	struct task_struct	*tsk;
	struct timer_list	timer;
};

I list some reasons by following ...

static int device_resume(struct device *dev, pm_message_t state, bool async)

{
	pm_callback_t callback = NULL;
	const char *info = NULL;
	int error = 0;
	DECLARE_DPM_WATCHDOG_ON_STACK(wd);  <== dpm_watchdog use stack memory for

watchdog timer struct, but sleep timer use global memory.

...<SNIP>

	if (!dpm_wait_for_superior(dev, async))
		goto Complete;

	dpm_watchdog_set(&wd, dev);  <== dpm_watchdog need "struct device", but  
sleep timer doesn't need it.
	device_lock(dev);

...<SNIP>

Unlock:

	device_unlock(dev);
	dpm_watchdog_clear(&wd);

Complete:

	complete_all(&dev->power.completion);

	TRACE_RESUME(error);

	return error;
}

Thank you,
Joseph.

Greg KH

2020-10-20 09:24 UTC #1 lore raw

Note, your response is not threading well, just hit 'reply' in your email client...

Quoted 78 lines · On Tue, Oct 20, 2020 at 09:01:05AM +0000, [email protected] wrote:

Quoted 63 lines · On Tue, Oct 20, 2020 at 08:15:38AM +0000, [email protected] wrote:
Quoted 4 lines
Quoted 4 lines · Joseph Jang
Quoted 1 line
Add sleep timer and timeout handler to prevent device stuck during
suspend/
Quoted 1 line
resume process. The timeout handler will dump disk sleep task at
first
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
round timeout and trigger kernel panic at second round timeout. The default timer for each round is defined in CONFIG_PM_SLEEP_TIMER_TIMEOUT.
Signed-off-by Joseph Jang <[email protected]>
---
 MAINTAINERS                   |  2 +
 include/linux/console.h       |  1 +
 include/linux/suspend_timer.h | 90
+++++++++++++++++++++++++++++++++++
Quoted 2 lines
Quoted 2 lines
Why is this file in include/linux/ if you only ever call it from one
.c
Quoted 3 lines
Quoted 1 line
file?
I just refer to include/linux/suspend.h and create a new header file
in the
Quoted 2 lines
same folder.
If you have a better location for the new header file, please feel
free to
Quoted 1 line
let me know.
Only put .h files that are needed by different .c files in the include/linux/ directory. Otherwise it should be local to where the .c file is.
Great, use that!
Quoted 1 line
But we really hit the suspend hang issue that DPM_WATCHDOG cannot cover.
What issue is that?
Quoted 8 lines
We propose a wide coverage debug feature like PM_SLEEP_MONITOR which not only covers PM but also core PM hang issues.
And DPM_WATCHDOG is for device driver power management in drivers/base/power/main.c
and PM_SLEEP_MONITOR locate is for core power management in kernel/power/suspend.c.
I think it is fine for users to select whether they need device PM
only or
Quoted 1 line
not.
How will a user know which they should use?
Why not just fix whatever is wrong with the watchdog code instead of creating a new one?
Quoted 4 lines
Quoted 2 lines
And why isn't the watchdog sufficient for you? Why are you "open coding" a watchdog timer logic here at all???
Yes, we refer to DPM_WATCHDOG to extend the watchdog debugging for
core PM.
Quoted 1 line
Because we really hit a real case that was not covered by DPM_WATCHDOG.
Then fix that!
Quoted 1 line
I think PM_SLEEP_MONITOR is an extension debug feature from
Quoted 8 lines
DPM_WATCHDOG.
Please just fix the watchdog, as obviously it is not working properly. Don't create something new because of that.
thanks,
greg k-h
Thank you Greq for promptly responding.
I am okay to fix the DPM_WATCHDOG feature, but would like to have quick sync up before start.
Could you help?

Quoted 3 lines

1. Can we change the kernel config name ? DPM_WATCHDOG stands for Device Power Management.
We propose PM_SLEEP_MONITOR is to cover Core PM and Device PM.

That's fine.

Quoted 7 lines

2. Can we create a new data structure instead of using the following struct dpm_watchdog?
struct dpm_watchdog {
	struct device		*dev;
	struct task_struct	*tsk;
	struct timer_list	timer;
};

Why not use the existing one?

Quoted 10 lines

I list some reasons by following ...
static int device_resume(struct device *dev, pm_message_t state, bool async)
{
	pm_callback_t callback = NULL;
	const char *info = NULL;
	int error = 0;
	DECLARE_DPM_WATCHDOG_ON_STACK(wd);  <== dpm_watchdog use stack memory for
watchdog timer struct, but sleep timer use global memory.

Then move it off of the stack.

Quoted 8 lines

...<SNIP>

	if (!dpm_wait_for_superior(dev, async))
		goto Complete;

	dpm_watchdog_set(&wd, dev);  <== dpm_watchdog need "struct device", but

sleep timer doesn't need it.

That's fine, what are you trying to optimize for?

Quoted 8 lines

	device_lock(dev);

...<SNIP>

Unlock:

	device_unlock(dev);
	dpm_watchdog_clear(&wd);

Quoted 7 lines

Complete:

	complete_all(&dev->power.completion);

	TRACE_RESUME(error);

	return error;
}

Submit a patch that shows what you are doing and we will be glad to review that.

thanks,

greg k-h