On Wed, Jun 29, 2022 at 4:02 PM Jason A. Donenfeld Jason@zx2c4.com wrote:
Hi Kalesh,
On Wed, Jun 29, 2022 at 03:26:33PM -0700, Kalesh Singh wrote:
Thanks for taking a look. I'm concerned holding the sys/power/state open would have unintentional side effects. Adding the /sys/power/userspace_autosuspender seems more appropriate. We don't have a use case for the refcounting, so would prefer the simpler writing '0' / '1' to toggle semantics.
Alright. So I've cooked you up some code that you can submit, since I assume based on Christoph's bristliness that he won't do so. The below adds /sys/power/pm_userspace_autosleeper, which you can write a 0 or a 1 into, and fixes up wireguard and random.c to use it. The code is untested, but should generally be the correct thing, I think.
So in order of operations:
You write a patch for SystemSuspend.cpp and post it on Gerrit.
You take the diff below, clean it up or bikeshed the naming a bit or do whatever there, and submit it to Rafael's PM tree, including as a `Link: ...` this thread and the Gerrit link.
When/if Rafael accepts the patch, you submit the Gerrit CL.
When both have landed, Christoph moves forward with his CONFIG_ANDROID removal.
Does that seem like a reasonable way forward?
Jason
diff --git a/drivers/char/random.c b/drivers/char/random.c index e3dd1dd3dd22..c25e3be10d9c 100644 --- a/drivers/char/random.c +++ b/drivers/char/random.c @@ -756,7 +756,7 @@ static int random_pm_notification(struct notifier_block *nb, unsigned long actio
if (crng_ready() && (action == PM_RESTORE_PREPARE || (action == PM_POST_SUSPEND &&
!IS_ENABLED(CONFIG_PM_AUTOSLEEP) && !IS_ENABLED(CONFIG_ANDROID)))) {
!IS_ENABLED(CONFIG_PM_AUTOSLEEP) && !pm_userspace_autosleeper_enabled))) { crng_reseed(); pr_notice("crng reseeded on system resumption\n"); }diff --git a/drivers/net/wireguard/device.c b/drivers/net/wireguard/device.c index aa9a7a5970fd..1983e0fadb6e 100644 --- a/drivers/net/wireguard/device.c +++ b/drivers/net/wireguard/device.c @@ -69,7 +69,7 @@ static int wg_pm_notification(struct notifier_block *nb, unsigned long action, v * its normal operation rather than as a somewhat rare event, then we * don't actually want to clear keys. */
if (IS_ENABLED(CONFIG_PM_AUTOSLEEP) || IS_ENABLED(CONFIG_ANDROID))
if (IS_ENABLED(CONFIG_PM_AUTOSLEEP) || pm_userspace_autosleeper_enabled) return 0; if (action != PM_HIBERNATION_PREPARE && action != PM_SUSPEND_PREPARE)diff --git a/include/linux/suspend.h b/include/linux/suspend.h index 70f2921e2e70..0acff26f87b4 100644 --- a/include/linux/suspend.h +++ b/include/linux/suspend.h @@ -498,6 +498,7 @@ extern void ksys_sync_helper(void); /* drivers/base/power/wakeup.c */ extern bool events_check_enabled; extern suspend_state_t pm_suspend_target_state; +extern bool pm_userspace_autosleeper_enabled;
extern bool pm_wakeup_pending(void); extern void pm_system_wakeup(void); @@ -537,6 +538,8 @@ static inline void pm_system_irq_wakeup(unsigned int irq_number) {} static inline void lock_system_sleep(void) {} static inline void unlock_system_sleep(void) {}
+#define pm_userspace_autosleeper_enabled (false)
#endif /* !CONFIG_PM_SLEEP */
#ifdef CONFIG_PM_SLEEP_DEBUG diff --git a/kernel/power/main.c b/kernel/power/main.c index e3694034b753..08f32a281010 100644 --- a/kernel/power/main.c +++ b/kernel/power/main.c @@ -120,6 +120,23 @@ static ssize_t pm_async_store(struct kobject *kobj, struct kobj_attribute *attr,
power_attr(pm_async);
+bool pm_userspace_autosleeper_enabled;
+static ssize_t pm_userspace_autosleeper_show(struct kobject *kobj,
struct kobj_attribute *attr, char *buf)+{
return sprintf(buf, "%d\n", pm_userspace_autosleeper_enabled);+}
+static ssize_t pm_userspace_autosleeper_store(struct kobject *kobj,
struct kobj_attribute *attr,const char *buf, size_t n)+{
return kstrtobool(buf, &pm_userspace_autosleeper_enabled);+}
+power_attr(pm_userspace_autosleeper);
Jason: Thanks for raising this issue and sharing this patch to avoid breakage! I really appreciate it.
My only concern with this change introducting a userspace knob set at runtime, vs a (hopefully more specific than _ANDROID) kernel config is that it's not exactly clear what the flag really means (which is the same issue CONFIG_ANDROID has). And more problematic, with this it would be an ABI.
So for this we probably need to have a very clear description of what userland is telling the kernel. Because I'm sure userlands behavior will drift and shift and we'll end up litigating what kind of behavior is really userspace_autosleeping vs userspace_sortof_autosleeping. :)
Alternatively, maybe we should switch it to describe what behavior change we are wanting the kernel take (instead of it hinting to the kernel what to expect from userland's behavior)? That way it might be more specific.
Again, really appreciate your efforts here!
thanks -john