From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7E87DCCA481 for ; Wed, 29 Jun 2022 23:52:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231604AbiF2XwW (ORCPT ); Wed, 29 Jun 2022 19:52:22 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56282 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229948AbiF2XwR (ORCPT ); Wed, 29 Jun 2022 19:52:17 -0400 Received: from mail-yw1-x1136.google.com (mail-yw1-x1136.google.com [IPv6:2607:f8b0:4864:20::1136]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9AE8B22BF6 for ; Wed, 29 Jun 2022 16:52:16 -0700 (PDT) Received: by mail-yw1-x1136.google.com with SMTP id 00721157ae682-31bf327d4b5so74364327b3.13 for ; Wed, 29 Jun 2022 16:52:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=jj2bErexLgV1A9YIdvzx3PGjusAMI02AE7tzIDJ1M5A=; b=Y2Tu2OdDzMgyWgz7uI6wnDGVdXiBPwT0tk13f7mVdOKDysVRR/kbruRKXKz2I5+Mni GQx6RMKlgx3jTU8tqKusqnJzX0RL/HtsP7Km6NeANpCcKuitYurgmkGIZxyd81D0b7c4 M6hgkp/CS/Lan4c8MCYn+6RBduyK3MXC1Q5hpln09l+yFeWTrPqOilPtanfcysyDmduD 1QqYZFTcDH7Rj6fKdLgMxiOspYiBUK9DkBCSjd2QsNvE7OGJUY1fr6YAUMEzUMi16PcV Pr19C5xIMVz6otGKDdAXazhClNibtSR2AR9TjfOL7WIpf1a3vQmUpvWxoW405WHuZuXJ zzZg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=jj2bErexLgV1A9YIdvzx3PGjusAMI02AE7tzIDJ1M5A=; b=nNuL5MsKKmuag4LFlEUyKg1yxPR1mgxM3dWxnYR6kEXejX8xIV6lho8Bx5zODpptPC qBZNfWDbPM5KaHCZ6l06jRC3t7YQ6/p3toIf+HP8dg2i1oeKqd3HgjerrGeqO7ScyroD qmwAvCTiICNDJo6xe6aJNI9fwYMYBq/s9+bgp6bMFEIFeS1i4N4cnSgnvKP7ztfGUr6+ CnjPK8hV29ZfDg3/6KZ5TS3l/MYMt4RnglYG8bDp5C+qtFrBxgETNhKmGHlSrkl+si+/ 7UWep3n2yiH33uoDvLrk9qdltaUvItzU/Zi+UrOyiDAJ5sDGJwTJ5B/PEJ7aAIpPkmMj iWLA== X-Gm-Message-State: AJIora8FVqmEqLb68HGgOHNsQZxjK3FLS0nuKFqui6C7jsOU7sxfcNo3 OFdARkgqzU2uEPh94FK2eN28N9yVBpdz+sKCIZPc X-Google-Smtp-Source: AGRyM1v1n7EWs3fcbJobS/0qoUyUpaOGXwj1JxcVbNWxYIvJDEF/meItbXW0LeimAccioancoaiWdGtPrhVNyLhYnGY= X-Received: by 2002:a81:8d08:0:b0:317:a4cd:d65d with SMTP id d8-20020a818d08000000b00317a4cdd65dmr6750642ywg.329.1656546735690; Wed, 29 Jun 2022 16:52:15 -0700 (PDT) MIME-Version: 1.0 References: <20220629161020.GA24891@lst.de> <20220629161527.GA24978@lst.de> <20220629163007.GA25279@lst.de> In-Reply-To: From: John Stultz Date: Wed, 29 Jun 2022 16:52:05 -0700 Message-ID: Subject: Re: [PATCH] remove CONFIG_ANDROID To: "Jason A. Donenfeld" Cc: Kalesh Singh , Christoph Hellwig , Greg Kroah-Hartman , =?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= , Todd Kjos , Martijn Coenen , Joel Fernandes , Christian Brauner , Hridya Valsaraju , Suren Baghdasaryan , "Theodore Ts'o" , "David S. Miller" , Eric Dumazet , Jakub Kicinski , "Alex Xu (Hello71)" , Paolo Abeni , Rob Herring , "Paul E. McKenney" , Frederic Weisbecker , Neeraj Upadhyay , Josh Triplett , Steven Rostedt , Mathieu Desnoyers , Lai Jiangshan , Shuah Khan , LKML , wireguard@lists.zx2c4.com, netdev@vger.kernel.org, rcu , "open list:KERNEL SELFTEST FRAMEWORK" , sultan@kerneltoast.com, android-kernel-team , Saravana Kannan , "Rafael J. Wysocki" Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jun 29, 2022 at 4:02 PM Jason A. Donenfeld 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: > > 1. You write a patch for SystemSuspend.cpp and post it on Gerrit. > > 2. 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. > > 3. When/if Rafael accepts the patch, you submit the Gerrit CL. > > 4. 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