drivers/base/power/wakeup.c | 75 ++++++++++++++++++++++++++++++++++++- include/linux/suspend.h | 2 + kernel/power/suspend.c | 1 + 3 files changed, 77 insertions(+), 1 deletion(-)
Sometimes kernel suspend transitions can be aborted unconditionally by
manipulating pm_abort_suspend value using "hard" wakeup triggers or
through "pm_system_wakeup()".
There is no way to trace the source path of module or subsystem which
aborted the suspend transitions. This change will create a list of
wakeup sources aborting suspend in progress through "hard" events as
well as subsytems aborting suspend using "pm_system_wakeup()".
Example: Existing suspend failure logs:
[ 349.708359] PM: Some devices failed to suspend, or early wake event detected
[ 350.327842] PM: suspend exit
Suspend failure logs with this change:
[ 518.761835] PM: Some devices failed to suspend, or early wake event detected
[ 519.486939] Abort: ws or subsystem uart_suspend_port aborted suspend
[ 519.500594] PM: suspend exit
Here we can clearly identify the module triggerring abort suspend.
Co-developed-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
Co-developed-by: Mintu Patel <mintupatel89@gmail.com>
Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
Co-developed-by: Vishal Badole <badolevishal1116@gmail.com>
Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>
---
drivers/base/power/wakeup.c | 75 ++++++++++++++++++++++++++++++++++++-
include/linux/suspend.h | 2 +
kernel/power/suspend.c | 1 +
3 files changed, 77 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
index a917219feea6..f640034cab6d 100644
--- a/drivers/base/power/wakeup.c
+++ b/drivers/base/power/wakeup.c
@@ -73,6 +73,13 @@ static struct wakeup_source deleted_ws = {
static DEFINE_IDA(wakeup_ida);
+struct pm_abort_suspend_source {
+ struct list_head list; //linux kernel list implementation
+ char *source_triggering_abort_suspend;
+};
+
+LIST_HEAD(pm_abort_suspend_list);
+
/**
* wakeup_source_create - Create a struct wakeup_source object.
* @name: Name of the new wakeup source.
@@ -575,6 +582,53 @@ static void wakeup_source_activate(struct wakeup_source *ws)
trace_wakeup_source_activate(ws->name, cec);
}
+/**
+ * clear_abort_suspend_list: To clear the list containing sources which
+ * aborted suspend transitions.
+ * Functionality: The list will be cleared every time system PM exits as we
+ * can find sources which aborted suspend in the current suspend transisions.
+ */
+void clear_abort_suspend_list(void)
+{
+ struct pm_abort_suspend_source *info, *tmp;
+
+ if (!list_empty(&pm_abort_suspend_list))
+ list_for_each_entry_safe(info, tmp, &pm_abort_suspend_list, list) {
+ list_del(&info->list);
+ kfree(info);
+ }
+}
+EXPORT_SYMBOL_GPL(clear_abort_suspend_list);
+
+/**
+ * pm_add_abort_suspend_source: add sources who aborted system suspend transitions.
+ * @func_name: Name of the WS or subsystem which needs to added in the list
+ */
+void pm_add_abort_suspend_source(const char *source_name)
+{
+ struct pm_abort_suspend_source *info = NULL;
+
+ info = kmalloc(sizeof(struct pm_abort_suspend_source), GFP_KERNEL);
+ if (unlikely(!info)) {
+ pr_err("Failed to alloc memory for pm_abort_suspend_source info\n");
+ return;
+ }
+
+ /* Initialize the list within the struct if it's not already initialized */
+ if (list_empty(&info->list))
+ INIT_LIST_HEAD(&info->list);
+
+ info->source_triggering_abort_suspend = kstrdup(source_name, GFP_KERNEL);
+ if (unlikely(!info->source_triggering_abort_suspend)) {
+ pr_err("Failed to get abort_suspend source_name\n");
+ kfree(info);
+ return;
+ }
+
+ list_add_tail(&info->list, &pm_abort_suspend_list);
+}
+EXPORT_SYMBOL_GPL(pm_add_abort_suspend_source);
+
/**
* wakeup_source_report_event - Report wakeup event using the given source.
* @ws: Wakeup source to report the event for.
@@ -590,8 +644,11 @@ static void wakeup_source_report_event(struct wakeup_source *ws, bool hard)
if (!ws->active)
wakeup_source_activate(ws);
- if (hard)
+ if (hard) {
+ if (pm_suspend_target_state != PM_SUSPEND_ON)
+ pm_add_abort_suspend_source(ws->name);
pm_system_wakeup();
+ }
}
/**
@@ -893,12 +950,28 @@ bool pm_wakeup_pending(void)
pm_print_active_wakeup_sources();
}
+ if (atomic_read(&pm_abort_suspend) > 0) {
+ if (!list_empty(&pm_abort_suspend_list))
+ list_for_each_entry(info, &pm_abort_suspend_list, list) {
+ log_suspend_abort_reason("ws or subsystem %s aborted suspend\n",
+ info->source_triggering_abort_suspend);
+ }
+ }
+
return ret || atomic_read(&pm_abort_suspend) > 0;
}
EXPORT_SYMBOL_GPL(pm_wakeup_pending);
void pm_system_wakeup(void)
{
+ char buf[MAX_SUSPEND_ABORT_LEN];
+
+ if (pm_suspend_target_state != PM_SUSPEND_ON) {
+ sprintf(buf, "%ps", __builtin_return_address(0));
+ if (strcmp(buf, "pm_wakeup_ws_event"))
+ pm_add_abort_suspend_source(buf);
+ }
+
atomic_inc(&pm_abort_suspend);
s2idle_wake();
}
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index ef503088942d..803071f664e7 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -528,6 +528,7 @@ extern void pm_print_active_wakeup_sources(void);
extern unsigned int lock_system_sleep(void);
extern void unlock_system_sleep(unsigned int);
+extern void clear_abort_suspend_list(void);
#else /* !CONFIG_PM_SLEEP */
@@ -556,6 +557,7 @@ static inline void pm_system_irq_wakeup(unsigned int irq_number) {}
static inline unsigned int lock_system_sleep(void) { return 0; }
static inline void unlock_system_sleep(unsigned int flags) {}
+static inline void clear_abort_suspend_list(void) {}
#endif /* !CONFIG_PM_SLEEP */
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index fa3bf161d13f..64a99f26bd51 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -623,6 +623,7 @@ int pm_suspend(suspend_state_t state)
} else {
suspend_stats.success++;
}
+ clear_abort_suspend_list();
pr_info("suspend exit\n");
return error;
}
--
2.25.1Hi Vimal,
kernel test robot noticed the following build errors:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/acpi-bus linus/master rafael-pm/devprop v6.7-rc4 next-20231208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Vimal-Kumar/PM-sleep-Mechanism-to-find-source-aborting-kernel-suspend-transition/20231209-161237
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20231209081056.1497-1-vimal.kumar32%40gmail.com
patch subject: [PATCH] PM / sleep: Mechanism to find source aborting kernel suspend transition
config: arm-lpc32xx_defconfig (https://download.01.org/0day-ci/archive/20231210/202312100547.4pJ5OE1O-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231210/202312100547.4pJ5OE1O-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312100547.4pJ5OE1O-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/base/power/wakeup.c:607:6: warning: no previous prototype for function 'pm_add_abort_suspend_source' [-Wmissing-prototypes]
607 | void pm_add_abort_suspend_source(const char *source_name)
| ^
drivers/base/power/wakeup.c:607:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
607 | void pm_add_abort_suspend_source(const char *source_name)
| ^
| static
>> drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^
>> drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'
>> drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'
>> drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'
>> drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'; did you mean 'int'?
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^~~~
| int
include/linux/list.h:780:29: note: expanded from macro 'list_for_each_entry'
780 | pos = list_next_entry(pos, member))
| ^
include/linux/list.h:645:14: note: expanded from macro 'list_next_entry'
645 | list_entry((pos)->member.next, typeof(*(pos)), member)
| ^
include/linux/list.h:601:15: note: expanded from macro 'list_entry'
601 | container_of(ptr, type, member)
| ^
include/linux/container_of.h:19:26: note: expanded from macro 'container_of'
19 | void *__mptr = (void *)(ptr); \
| ^
>> drivers/base/power/wakeup.c:955:4: error: reference to overloaded function could not be resolved; did you mean to call it?
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/list.h:780:13: note: expanded from macro 'list_for_each_entry'
780 | pos = list_next_entry(pos, member))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/list.h:645:13: note: expanded from macro 'list_next_entry'
645 | list_entry((pos)->member.next, typeof(*(pos)), member)
| ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/list.h:601:15: note: expanded from macro 'list_entry'
601 | container_of(ptr, type, member)
| ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
include/linux/container_of.h:19:26: note: expanded from macro 'container_of'
19 | void *__mptr = (void *)(ptr); \
| ^~~
>> drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'; did you mean 'int'?
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^~~~
| int
include/linux/list.h:780:29: note: expanded from macro 'list_for_each_entry'
780 | pos = list_next_entry(pos, member))
| ^
include/linux/list.h:645:14: note: expanded from macro 'list_next_entry'
645 | list_entry((pos)->member.next, typeof(*(pos)), member)
| ^
include/linux/list.h:601:15: note: expanded from macro 'list_entry'
601 | container_of(ptr, type, member)
| ^
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
include/linux/compiler_types.h:376:63: note: expanded from macro '__same_type'
376 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
| ^
include/linux/build_bug.h:77:50: note: expanded from macro 'static_assert'
77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
| ^
include/linux/build_bug.h:78:56: note: expanded from macro '__static_assert'
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^
>> drivers/base/power/wakeup.c:955:4: error: reference to overloaded function could not be resolved; did you mean to call it?
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/list.h:780:13: note: expanded from macro 'list_for_each_entry'
780 | pos = list_next_entry(pos, member))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/list.h:645:13: note: expanded from macro 'list_next_entry'
645 | list_entry((pos)->member.next, typeof(*(pos)), member)
| ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/list.h:601:15: note: expanded from macro 'list_entry'
601 | container_of(ptr, type, member)
| ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
include/linux/compiler_types.h:376:63: note: expanded from macro '__same_type'
376 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
| ^
include/linux/build_bug.h:77:50: note: expanded from macro 'static_assert'
77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
| ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:78:56: note: expanded from macro '__static_assert'
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^~~~
>> drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'; did you mean 'int'?
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^~~~
| int
include/linux/list.h:780:29: note: expanded from macro 'list_for_each_entry'
780 | pos = list_next_entry(pos, member))
| ^
include/linux/list.h:645:14: note: expanded from macro 'list_next_entry'
645 | list_entry((pos)->member.next, typeof(*(pos)), member)
| ^
include/linux/list.h:601:15: note: expanded from macro 'list_entry'
601 | container_of(ptr, type, member)
| ^
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
include/linux/compiler_types.h:376:63: note: expanded from macro '__same_type'
376 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
| ^
include/linux/build_bug.h:77:50: note: expanded from macro 'static_assert'
77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
| ^
include/linux/build_bug.h:78:56: note: expanded from macro '__static_assert'
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^
>> drivers/base/power/wakeup.c:955:4: error: reference to overloaded function could not be resolved; did you mean to call it?
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/list.h:780:13: note: expanded from macro 'list_for_each_entry'
780 | pos = list_next_entry(pos, member))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/list.h:645:13: note: expanded from macro 'list_next_entry'
645 | list_entry((pos)->member.next, typeof(*(pos)), member)
| ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/list.h:601:15: note: expanded from macro 'list_entry'
601 | container_of(ptr, type, member)
| ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~
note: (skipping 1 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
include/linux/compiler_types.h:376:63: note: expanded from macro '__same_type'
376 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
| ^
include/linux/build_bug.h:77:50: note: expanded from macro 'static_assert'
77 | #define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
| ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:78:56: note: expanded from macro '__static_assert'
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^~~~
>> drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'; did you mean 'int'?
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^~~~
| int
include/linux/list.h:780:29: note: expanded from macro 'list_for_each_entry'
780 | pos = list_next_entry(pos, member))
| ^
include/linux/list.h:645:42: note: expanded from macro 'list_next_entry'
645 | list_entry((pos)->member.next, typeof(*(pos)), member)
| ^
include/linux/list.h:601:20: note: expanded from macro 'list_entry'
601 | container_of(ptr, type, member)
| ^
include/linux/container_of.h:23:4: note: expanded from macro 'container_of'
23 | ((type *)(__mptr - offsetof(type, member))); })
| ^
>> drivers/base/power/wakeup.c:955:4: error: reference to overloaded function could not be resolved; did you mean to call it?
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/list.h:780:13: note: expanded from macro 'list_for_each_entry'
780 | pos = list_next_entry(pos, member))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/list.h:645:41: note: expanded from macro 'list_next_entry'
645 | list_entry((pos)->member.next, typeof(*(pos)), member)
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
include/linux/list.h:601:20: note: expanded from macro 'list_entry'
601 | container_of(ptr, type, member)
| ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
include/linux/container_of.h:23:4: note: expanded from macro 'container_of'
23 | ((type *)(__mptr - offsetof(type, member))); })
| ^~~~
>> drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'; did you mean 'int'?
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^~~~
| int
include/linux/list.h:780:29: note: expanded from macro 'list_for_each_entry'
780 | pos = list_next_entry(pos, member))
| ^
include/linux/list.h:645:42: note: expanded from macro 'list_next_entry'
645 | list_entry((pos)->member.next, typeof(*(pos)), member)
| ^
include/linux/list.h:601:20: note: expanded from macro 'list_entry'
601 | container_of(ptr, type, member)
| ^
include/linux/container_of.h:23:30: note: expanded from macro 'container_of'
23 | ((type *)(__mptr - offsetof(type, member))); })
| ^
include/linux/stddef.h:16:51: note: expanded from macro 'offsetof'
16 | #define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER)
| ^
>> drivers/base/power/wakeup.c:955:4: error: reference to overloaded function could not be resolved; did you mean to call it?
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/list.h:780:13: note: expanded from macro 'list_for_each_entry'
780 | pos = list_next_entry(pos, member))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/list.h:645:41: note: expanded from macro 'list_next_entry'
645 | list_entry((pos)->member.next, typeof(*(pos)), member)
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
include/linux/list.h:601:20: note: expanded from macro 'list_entry'
601 | container_of(ptr, type, member)
| ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
include/linux/container_of.h:23:30: note: expanded from macro 'container_of'
23 | ((type *)(__mptr - offsetof(type, member))); })
| ~~~~~~~~~^~~~~~~~~~~~~
include/linux/stddef.h:16:51: note: expanded from macro 'offsetof'
16 | #define offsetof(TYPE, MEMBER) __builtin_offsetof(TYPE, MEMBER)
| ^~~~
>> drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^
>> drivers/base/power/wakeup.c:956:5: error: call to undeclared function 'log_suspend_abort_reason'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
956 | log_suspend_abort_reason("ws or subsystem %s aborted suspend\n",
| ^
drivers/base/power/wakeup.c:957:7: error: use of undeclared identifier 'info'
957 | info->source_triggering_abort_suspend);
| ^
>> drivers/base/power/wakeup.c:967:11: error: use of undeclared identifier 'MAX_SUSPEND_ABORT_LEN'
967 | char buf[MAX_SUSPEND_ABORT_LEN];
| ^
1 warning and 18 errors generated.
vim +/info +955 drivers/base/power/wakeup.c
924
925 /**
926 * pm_wakeup_pending - Check if power transition in progress should be aborted.
927 *
928 * Compare the current number of registered wakeup events with its preserved
929 * value from the past and return true if new wakeup events have been registered
930 * since the old value was stored. Also return true if the current number of
931 * wakeup events being processed is different from zero.
932 */
933 bool pm_wakeup_pending(void)
934 {
935 unsigned long flags;
936 bool ret = false;
937
938 raw_spin_lock_irqsave(&events_lock, flags);
939 if (events_check_enabled) {
940 unsigned int cnt, inpr;
941
942 split_counters(&cnt, &inpr);
943 ret = (cnt != saved_count || inpr > 0);
944 events_check_enabled = !ret;
945 }
946 raw_spin_unlock_irqrestore(&events_lock, flags);
947
948 if (ret) {
949 pm_pr_dbg("Wakeup pending, aborting suspend\n");
950 pm_print_active_wakeup_sources();
951 }
952
953 if (atomic_read(&pm_abort_suspend) > 0) {
954 if (!list_empty(&pm_abort_suspend_list))
> 955 list_for_each_entry(info, &pm_abort_suspend_list, list) {
> 956 log_suspend_abort_reason("ws or subsystem %s aborted suspend\n",
957 info->source_triggering_abort_suspend);
958 }
959 }
960
961 return ret || atomic_read(&pm_abort_suspend) > 0;
962 }
963 EXPORT_SYMBOL_GPL(pm_wakeup_pending);
964
965 void pm_system_wakeup(void)
966 {
> 967 char buf[MAX_SUSPEND_ABORT_LEN];
968
969 if (pm_suspend_target_state != PM_SUSPEND_ON) {
970 sprintf(buf, "%ps", __builtin_return_address(0));
971 if (strcmp(buf, "pm_wakeup_ws_event"))
972 pm_add_abort_suspend_source(buf);
973 }
974
975 atomic_inc(&pm_abort_suspend);
976 s2idle_wake();
977 }
978 EXPORT_SYMBOL_GPL(pm_system_wakeup);
979
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Vimal,
kernel test robot noticed the following build warnings:
[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/acpi-bus linus/master rafael-pm/devprop v6.7-rc4 next-20231208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Vimal-Kumar/PM-sleep-Mechanism-to-find-source-aborting-kernel-suspend-transition/20231209-161237
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20231209081056.1497-1-vimal.kumar32%40gmail.com
patch subject: [PATCH] PM / sleep: Mechanism to find source aborting kernel suspend transition
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20231210/202312100146.Nv4INLR5-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231210/202312100146.Nv4INLR5-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312100146.Nv4INLR5-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/base/power/wakeup.c:607:6: warning: no previous prototype for 'pm_add_abort_suspend_source' [-Wmissing-prototypes]
607 | void pm_add_abort_suspend_source(const char *source_name)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/rculist.h:10,
from include/linux/pid.h:5,
from include/linux/sched.h:14,
from include/linux/ratelimit.h:6,
from include/linux/dev_printk.h:16,
from include/linux/device.h:15,
from drivers/base/power/wakeup.c:9:
drivers/base/power/wakeup.c: In function 'pm_wakeup_pending':
drivers/base/power/wakeup.c:955:45: error: 'info' undeclared (first use in this function)
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^~~~
include/linux/list.h:778:14: note: in definition of macro 'list_for_each_entry'
778 | for (pos = list_first_entry(head, typeof(*pos), member); \
| ^~~
drivers/base/power/wakeup.c:955:45: note: each undeclared identifier is reported only once for each function it appears in
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^~~~
include/linux/list.h:778:14: note: in definition of macro 'list_for_each_entry'
778 | for (pos = list_first_entry(head, typeof(*pos), member); \
| ^~~
In file included from include/linux/bits.h:21,
from include/linux/ratelimit_types.h:5,
from include/linux/ratelimit.h:5,
from include/linux/dev_printk.h:16,
from include/linux/device.h:15,
from drivers/base/power/wakeup.c:9:
include/linux/compiler_types.h:376:27: error: expression in static assertion is not an integer
376 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:78:56: note: in definition of macro '__static_assert'
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^~~~
include/linux/container_of.h:20:9: note: in expansion of macro 'static_assert'
20 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \
| ^~~~~~~~~~~~~
include/linux/container_of.h:20:23: note: in expansion of macro '__same_type'
20 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \
| ^~~~~~~~~~~
include/linux/list.h:601:9: note: in expansion of macro 'container_of'
601 | container_of(ptr, type, member)
| ^~~~~~~~~~~~
include/linux/list.h:612:9: note: in expansion of macro 'list_entry'
612 | list_entry((ptr)->next, type, member)
| ^~~~~~~~~~
include/linux/list.h:778:20: note: in expansion of macro 'list_first_entry'
778 | for (pos = list_first_entry(head, typeof(*pos), member); \
| ^~~~~~~~~~~~~~~~
drivers/base/power/wakeup.c:955:25: note: in expansion of macro 'list_for_each_entry'
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^~~~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:376:27: error: expression in static assertion is not an integer
376 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:78:56: note: in definition of macro '__static_assert'
78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg)
| ^~~~
include/linux/container_of.h:20:9: note: in expansion of macro 'static_assert'
20 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \
| ^~~~~~~~~~~~~
include/linux/container_of.h:20:23: note: in expansion of macro '__same_type'
20 | static_assert(__same_type(*(ptr), ((type *)0)->member) || \
| ^~~~~~~~~~~
include/linux/list.h:601:9: note: in expansion of macro 'container_of'
601 | container_of(ptr, type, member)
| ^~~~~~~~~~~~
include/linux/list.h:645:9: note: in expansion of macro 'list_entry'
645 | list_entry((pos)->member.next, typeof(*(pos)), member)
| ^~~~~~~~~~
include/linux/list.h:780:20: note: in expansion of macro 'list_next_entry'
780 | pos = list_next_entry(pos, member))
| ^~~~~~~~~~~~~~~
drivers/base/power/wakeup.c:955:25: note: in expansion of macro 'list_for_each_entry'
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^~~~~~~~~~~~~~~~~~~
drivers/base/power/wakeup.c:956:33: error: implicit declaration of function 'log_suspend_abort_reason' [-Werror=implicit-function-declaration]
956 | log_suspend_abort_reason("ws or subsystem %s aborted suspend\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~
drivers/base/power/wakeup.c: In function 'pm_system_wakeup':
drivers/base/power/wakeup.c:967:18: error: 'MAX_SUSPEND_ABORT_LEN' undeclared (first use in this function)
967 | char buf[MAX_SUSPEND_ABORT_LEN];
| ^~~~~~~~~~~~~~~~~~~~~
>> drivers/base/power/wakeup.c:967:14: warning: unused variable 'buf' [-Wunused-variable]
967 | char buf[MAX_SUSPEND_ABORT_LEN];
| ^~~
cc1: some warnings being treated as errors
vim +/pm_add_abort_suspend_source +607 drivers/base/power/wakeup.c
602
603 /**
604 * pm_add_abort_suspend_source: add sources who aborted system suspend transitions.
605 * @func_name: Name of the WS or subsystem which needs to added in the list
606 */
> 607 void pm_add_abort_suspend_source(const char *source_name)
608 {
609 struct pm_abort_suspend_source *info = NULL;
610
611 info = kmalloc(sizeof(struct pm_abort_suspend_source), GFP_KERNEL);
612 if (unlikely(!info)) {
613 pr_err("Failed to alloc memory for pm_abort_suspend_source info\n");
614 return;
615 }
616
617 /* Initialize the list within the struct if it's not already initialized */
618 if (list_empty(&info->list))
619 INIT_LIST_HEAD(&info->list);
620
621 info->source_triggering_abort_suspend = kstrdup(source_name, GFP_KERNEL);
622 if (unlikely(!info->source_triggering_abort_suspend)) {
623 pr_err("Failed to get abort_suspend source_name\n");
624 kfree(info);
625 return;
626 }
627
628 list_add_tail(&info->list, &pm_abort_suspend_list);
629 }
630 EXPORT_SYMBOL_GPL(pm_add_abort_suspend_source);
631
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Vimal,
kernel test robot noticed the following build warnings:
[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/acpi-bus linus/master rafael-pm/devprop v6.7-rc4 next-20231208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Vimal-Kumar/PM-sleep-Mechanism-to-find-source-aborting-kernel-suspend-transition/20231209-161237
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20231209081056.1497-1-vimal.kumar32%40gmail.com
patch subject: [PATCH] PM / sleep: Mechanism to find source aborting kernel suspend transition
config: powerpc-randconfig-r071-20231209 (https://download.01.org/0day-ci/archive/20231210/202312100106.hPaGgy7C-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231210/202312100106.hPaGgy7C-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312100106.hPaGgy7C-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/base/power/wakeup.c:607:6: warning: no previous prototype for function 'pm_add_abort_suspend_source' [-Wmissing-prototypes]
607 | void pm_add_abort_suspend_source(const char *source_name)
| ^
drivers/base/power/wakeup.c:607:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
607 | void pm_add_abort_suspend_source(const char *source_name)
| ^
| static
drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'
955 | list_for_each_entry(info, &pm_abort_suspend_list, list) {
| ^
drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'
drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'
drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'
drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'
drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'
drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'
drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'
drivers/base/power/wakeup.c:955:24: error: use of undeclared identifier 'info'
drivers/base/power/wakeup.c:956:5: error: call to undeclared function 'log_suspend_abort_reason'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
956 | log_suspend_abort_reason("ws or subsystem %s aborted suspend\n",
| ^
drivers/base/power/wakeup.c:957:7: error: use of undeclared identifier 'info'
957 | info->source_triggering_abort_suspend);
| ^
drivers/base/power/wakeup.c:967:11: error: use of undeclared identifier 'MAX_SUSPEND_ABORT_LEN'
967 | char buf[MAX_SUSPEND_ABORT_LEN];
| ^
1 warning and 12 errors generated.
vim +/pm_add_abort_suspend_source +607 drivers/base/power/wakeup.c
602
603 /**
604 * pm_add_abort_suspend_source: add sources who aborted system suspend transitions.
605 * @func_name: Name of the WS or subsystem which needs to added in the list
606 */
> 607 void pm_add_abort_suspend_source(const char *source_name)
608 {
609 struct pm_abort_suspend_source *info = NULL;
610
611 info = kmalloc(sizeof(struct pm_abort_suspend_source), GFP_KERNEL);
612 if (unlikely(!info)) {
613 pr_err("Failed to alloc memory for pm_abort_suspend_source info\n");
614 return;
615 }
616
617 /* Initialize the list within the struct if it's not already initialized */
618 if (list_empty(&info->list))
619 INIT_LIST_HEAD(&info->list);
620
621 info->source_triggering_abort_suspend = kstrdup(source_name, GFP_KERNEL);
622 if (unlikely(!info->source_triggering_abort_suspend)) {
623 pr_err("Failed to get abort_suspend source_name\n");
624 kfree(info);
625 return;
626 }
627
628 list_add_tail(&info->list, &pm_abort_suspend_list);
629 }
630 EXPORT_SYMBOL_GPL(pm_add_abort_suspend_source);
631
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Le 09/12/2023 à 09:10, Vimal Kumar a écrit :
> Sometimes kernel suspend transitions can be aborted unconditionally by
> manipulating pm_abort_suspend value using "hard" wakeup triggers or
> through "pm_system_wakeup()".
>
> There is no way to trace the source path of module or subsystem which
> aborted the suspend transitions. This change will create a list of
> wakeup sources aborting suspend in progress through "hard" events as
> well as subsytems aborting suspend using "pm_system_wakeup()".
>
> Example: Existing suspend failure logs:
> [ 349.708359] PM: Some devices failed to suspend, or early wake event detected
> [ 350.327842] PM: suspend exit
>
> Suspend failure logs with this change:
> [ 518.761835] PM: Some devices failed to suspend, or early wake event detected
> [ 519.486939] Abort: ws or subsystem uart_suspend_port aborted suspend
> [ 519.500594] PM: suspend exit
>
> Here we can clearly identify the module triggerring abort suspend.
>
> Co-developed-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> Co-developed-by: Mintu Patel <mintupatel89@gmail.com>
> Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
> Co-developed-by: Vishal Badole <badolevishal1116@gmail.com>
> Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
> Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>
> ---
...
> +void pm_add_abort_suspend_source(const char *source_name)
> +{
> + struct pm_abort_suspend_source *info = NULL;
No need to init.
> +
> + info = kmalloc(sizeof(struct pm_abort_suspend_source), GFP_KERNEL);
Could be sizeof(*info).
> + if (unlikely(!info)) {
> + pr_err("Failed to alloc memory for pm_abort_suspend_source info\n");
No need to log a message if kmalloc() fails, a trace is already logged.
> + return;
> + }
> +
> + /* Initialize the list within the struct if it's not already initialized */
> + if (list_empty(&info->list))
> + INIT_LIST_HEAD(&info->list);
> +
> + info->source_triggering_abort_suspend = kstrdup(source_name, GFP_KERNEL);
> + if (unlikely(!info->source_triggering_abort_suspend)) {
> + pr_err("Failed to get abort_suspend source_name\n");
Same here.
> + kfree(info);
> + return;
> + }
> +
> + list_add_tail(&info->list, &pm_abort_suspend_list);
...
CJ
On Sat, Dec 09, 2023 at 01:40:54PM +0530, Vimal Kumar wrote:
> Sometimes kernel suspend transitions can be aborted unconditionally by
> manipulating pm_abort_suspend value using "hard" wakeup triggers or
> through "pm_system_wakeup()".
>
> There is no way to trace the source path of module or subsystem which
> aborted the suspend transitions. This change will create a list of
> wakeup sources aborting suspend in progress through "hard" events as
> well as subsytems aborting suspend using "pm_system_wakeup()".
>
> Example: Existing suspend failure logs:
> [ 349.708359] PM: Some devices failed to suspend, or early wake event detected
> [ 350.327842] PM: suspend exit
>
> Suspend failure logs with this change:
> [ 518.761835] PM: Some devices failed to suspend, or early wake event detected
> [ 519.486939] Abort: ws or subsystem uart_suspend_port aborted suspend
> [ 519.500594] PM: suspend exit
>
> Here we can clearly identify the module triggerring abort suspend.
>
> Co-developed-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> Co-developed-by: Mintu Patel <mintupatel89@gmail.com>
> Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
> Co-developed-by: Vishal Badole <badolevishal1116@gmail.com>
> Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
> Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>
> ---
> drivers/base/power/wakeup.c | 75 ++++++++++++++++++++++++++++++++++++-
> include/linux/suspend.h | 2 +
> kernel/power/suspend.c | 1 +
> 3 files changed, 77 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index a917219feea6..f640034cab6d 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -73,6 +73,13 @@ static struct wakeup_source deleted_ws = {
>
> static DEFINE_IDA(wakeup_ida);
>
> +struct pm_abort_suspend_source {
> + struct list_head list; //linux kernel list implementation
Nit, we know this is a list implementation, no need to comment that.
Also did you run checkpatch on this? You need a ' ' after "//" to be
correct.
> + char *source_triggering_abort_suspend;
> +};
> +
> +LIST_HEAD(pm_abort_suspend_list);
No blank line needed, and also, shouldn't this be static?
> +
> /**
> * wakeup_source_create - Create a struct wakeup_source object.
> * @name: Name of the new wakeup source.
> @@ -575,6 +582,53 @@ static void wakeup_source_activate(struct wakeup_source *ws)
> trace_wakeup_source_activate(ws->name, cec);
> }
>
> +/**
> + * clear_abort_suspend_list: To clear the list containing sources which
> + * aborted suspend transitions.
> + * Functionality: The list will be cleared every time system PM exits as we
> + * can find sources which aborted suspend in the current suspend transisions.
This isn't the correct way to write kernel doc formats, please see the
documentation for how to do it.
> + */
> +void clear_abort_suspend_list(void)
> +{
> + struct pm_abort_suspend_source *info, *tmp;
> +
> + if (!list_empty(&pm_abort_suspend_list))
Why check this, doesn't the list loop work properly here?
> + list_for_each_entry_safe(info, tmp, &pm_abort_suspend_list, list) {
> + list_del(&info->list);
> + kfree(info);
> + }
No locking at all for this list?
> +}
> +EXPORT_SYMBOL_GPL(clear_abort_suspend_list);
Global functions should be "subsystem_action", not "action_something"
like you did here. Otherwise this gets really messy very quickly.
> +
> +/**
> + * pm_add_abort_suspend_source: add sources who aborted system suspend transitions.
> + * @func_name: Name of the WS or subsystem which needs to added in the list
> + */
> +void pm_add_abort_suspend_source(const char *source_name)
> +{
> + struct pm_abort_suspend_source *info = NULL;
> +
> + info = kmalloc(sizeof(struct pm_abort_suspend_source), GFP_KERNEL);
> + if (unlikely(!info)) {
Only ever use unlikely/likely if you have documented proof that the code
is faster (i.e. you can measure it.) For normal calls like this, the
compiler and the processor knows better than you, so no need to do
anything.
> + pr_err("Failed to alloc memory for pm_abort_suspend_source info\n");
> + return;
> + }
> +
> + /* Initialize the list within the struct if it's not already initialized */
> + if (list_empty(&info->list))
> + INIT_LIST_HEAD(&info->list);
> +
> + info->source_triggering_abort_suspend = kstrdup(source_name, GFP_KERNEL);
> + if (unlikely(!info->source_triggering_abort_suspend)) {
Again, don't use likely/unlikely
> + pr_err("Failed to get abort_suspend source_name\n");
> + kfree(info);
> + return;
> + }
> +
> + list_add_tail(&info->list, &pm_abort_suspend_list);
> +}
> +EXPORT_SYMBOL_GPL(pm_add_abort_suspend_source);
> +
> /**
> * wakeup_source_report_event - Report wakeup event using the given source.
> * @ws: Wakeup source to report the event for.
> @@ -590,8 +644,11 @@ static void wakeup_source_report_event(struct wakeup_source *ws, bool hard)
> if (!ws->active)
> wakeup_source_activate(ws);
>
> - if (hard)
> + if (hard) {
> + if (pm_suspend_target_state != PM_SUSPEND_ON)
> + pm_add_abort_suspend_source(ws->name);
> pm_system_wakeup();
> + }
> }
>
> /**
> @@ -893,12 +950,28 @@ bool pm_wakeup_pending(void)
> pm_print_active_wakeup_sources();
> }
>
> + if (atomic_read(&pm_abort_suspend) > 0) {
> + if (!list_empty(&pm_abort_suspend_list))
> + list_for_each_entry(info, &pm_abort_suspend_list, list) {
> + log_suspend_abort_reason("ws or subsystem %s aborted suspend\n",
What is "ws"?
And again, no locking is just not going to work.
But step back, are you _sure_ you really need this information? Who is
going to use it? Where are they going to use it? And when are they
going to use it?
thanks,
greg k-h
Hi Greg k-h,
On Sat, Dec 09, 2023 at 10:21:12AM +0100, Greg Kroah-Hartman wrote:
> On Sat, Dec 09, 2023 at 01:40:54PM +0530, Vimal Kumar wrote:
> > Sometimes kernel suspend transitions can be aborted unconditionally by
> > manipulating pm_abort_suspend value using "hard" wakeup triggers or
> > through "pm_system_wakeup()".
> >
> > There is no way to trace the source path of module or subsystem which
> > aborted the suspend transitions. This change will create a list of
> > wakeup sources aborting suspend in progress through "hard" events as
> > well as subsytems aborting suspend using "pm_system_wakeup()".
> >
> > Example: Existing suspend failure logs:
> > [ 349.708359] PM: Some devices failed to suspend, or early wake event detected
> > [ 350.327842] PM: suspend exit
> >
> > Suspend failure logs with this change:
> > [ 518.761835] PM: Some devices failed to suspend, or early wake event detected
> > [ 519.486939] Abort: ws or subsystem uart_suspend_port aborted suspend
> > [ 519.500594] PM: suspend exit
> >
> > Here we can clearly identify the module triggerring abort suspend.
> >
> > Co-developed-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> > Signed-off-by: Chinmoy Ghosh <chinmoyghosh2001@gmail.com>
> > Co-developed-by: Mintu Patel <mintupatel89@gmail.com>
> > Signed-off-by: Mintu Patel <mintupatel89@gmail.com>
> > Co-developed-by: Vishal Badole <badolevishal1116@gmail.com>
> > Signed-off-by: Vishal Badole <badolevishal1116@gmail.com>
> > Signed-off-by: Vimal Kumar <vimal.kumar32@gmail.com>
> > ---
> > drivers/base/power/wakeup.c | 75 ++++++++++++++++++++++++++++++++++++-
> > include/linux/suspend.h | 2 +
> > kernel/power/suspend.c | 1 +
> > 3 files changed, 77 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> > index a917219feea6..f640034cab6d 100644
> > --- a/drivers/base/power/wakeup.c
> > +++ b/drivers/base/power/wakeup.c
> > @@ -73,6 +73,13 @@ static struct wakeup_source deleted_ws = {
> >
> > static DEFINE_IDA(wakeup_ida);
> >
> > +struct pm_abort_suspend_source {
> > + struct list_head list; //linux kernel list implementation
>
> Nit, we know this is a list implementation, no need to comment that.
>
> Also did you run checkpatch on this? You need a ' ' after "//" to be
> correct.
>
I do run checkpatch but it was not highlighted. I will remove the comment
> > + char *source_triggering_abort_suspend;
> > +};
> > +
> > +LIST_HEAD(pm_abort_suspend_list);
>
> No blank line needed, and also, shouldn't this be static?
>
I missed it, this should be static.
> > +
> > /**
> > * wakeup_source_create - Create a struct wakeup_source object.
> > * @name: Name of the new wakeup source.
> > @@ -575,6 +582,53 @@ static void wakeup_source_activate(struct wakeup_source *ws)
> > trace_wakeup_source_activate(ws->name, cec);
> > }
> >
> > +/**
> > + * clear_abort_suspend_list: To clear the list containing sources which
> > + * aborted suspend transitions.
> > + * Functionality: The list will be cleared every time system PM exits as we
> > + * can find sources which aborted suspend in the current suspend transisions.
>
> This isn't the correct way to write kernel doc formats, please see the
> documentation for how to do it.
>
I will correct it in the next version.
> > + */
> > +void clear_abort_suspend_list(void)
> > +{
> > + struct pm_abort_suspend_source *info, *tmp;
> > +
> > + if (!list_empty(&pm_abort_suspend_list))
>
> Why check this, doesn't the list loop work properly here?
>
The list worked properly, just added for extra check. It will be removed in the next version.
> > + list_for_each_entry_safe(info, tmp, &pm_abort_suspend_list, list) {
> > + list_del(&info->list);
> > + kfree(info);
> > + }
>
> No locking at all for this list?
>
I will gaurd it in the next version.
> > +}
> > +EXPORT_SYMBOL_GPL(clear_abort_suspend_list);
>
> Global functions should be "subsystem_action", not "action_something"
> like you did here. Otherwise this gets really messy very quickly.
>
I will take care of this in future.
> > +
> > +/**
> > + * pm_add_abort_suspend_source: add sources who aborted system suspend transitions.
> > + * @func_name: Name of the WS or subsystem which needs to added in the list
> > + */
> > +void pm_add_abort_suspend_source(const char *source_name)
> > +{
> > + struct pm_abort_suspend_source *info = NULL;
> > +
> > + info = kmalloc(sizeof(struct pm_abort_suspend_source), GFP_KERNEL);
> > + if (unlikely(!info)) {
>
> Only ever use unlikely/likely if you have documented proof that the code
> is faster (i.e. you can measure it.) For normal calls like this, the
> compiler and the processor knows better than you, so no need to do
> anything.
>
I will remove it in the next version.
> > + pr_err("Failed to alloc memory for pm_abort_suspend_source info\n");
> > + return;
> > + }
> > +
> > + /* Initialize the list within the struct if it's not already initialized */
> > + if (list_empty(&info->list))
> > + INIT_LIST_HEAD(&info->list);
> > +
> > + info->source_triggering_abort_suspend = kstrdup(source_name, GFP_KERNEL);
> > + if (unlikely(!info->source_triggering_abort_suspend)) {
>
> Again, don't use likely/unlikely
>
It will be fixed in the next version.
> > + pr_err("Failed to get abort_suspend source_name\n");
> > + kfree(info);
> > + return;
> > + }
> > +
> > + list_add_tail(&info->list, &pm_abort_suspend_list);
> > +}
> > +EXPORT_SYMBOL_GPL(pm_add_abort_suspend_source);
> > +
> > /**
> > * wakeup_source_report_event - Report wakeup event using the given source.
> > * @ws: Wakeup source to report the event for.
> > @@ -590,8 +644,11 @@ static void wakeup_source_report_event(struct wakeup_source *ws, bool hard)
> > if (!ws->active)
> > wakeup_source_activate(ws);
> >
> > - if (hard)
> > + if (hard) {
> > + if (pm_suspend_target_state != PM_SUSPEND_ON)
> > + pm_add_abort_suspend_source(ws->name);
> > pm_system_wakeup();
> > + }
> > }
> >
> > /**
> > @@ -893,12 +950,28 @@ bool pm_wakeup_pending(void)
> > pm_print_active_wakeup_sources();
> > }
> >
> > + if (atomic_read(&pm_abort_suspend) > 0) {
> > + if (!list_empty(&pm_abort_suspend_list))
> > + list_for_each_entry(info, &pm_abort_suspend_list, list) {
> > + log_suspend_abort_reason("ws or subsystem %s aborted suspend\n",
>
> What is "ws"?
>
This is for "wakeup_source", we will add it instead of "ws".
> And again, no locking is just not going to work.
>
I will add lock and gaurd the list in the next version.
> But step back, are you _sure_ you really need this information? Who is
> going to use it? Where are they going to use it? And when are they
> going to use it?
>
> thanks,
>
> greg k-h
Suspend-to-RAM and Suspend-to-Idle are widely used features. Sometimes, we have encountered
suspend failures, but the cause of these issues is unknown.
This occurred whenever the suspend was unconditionally aborted using the functionality below:
- Suspend can be aborted via "hard" wakeup triggers using either "pm_wakeup_ws_event", or "pm_wakeup_dev_event".
- Any module can abort the suspend directly using "pm_system_wakeup"
This change can give developers an upper hand by tracing any such aborted suspend calls and
pointing them in the right direction to debug further.
Warm Regards,
Vimal Kumar
© 2016 - 2025 Red Hat, Inc.