Development discussion of WireGuard
 help / color / mirror / Atom feed
From: John Stultz <jstultz@google.com>
To: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: "Kalesh Singh" <kaleshsingh@google.com>,
	"Christoph Hellwig" <hch@lst.de>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Arve Hjønnevåg" <arve@android.com>,
	"Todd Kjos" <tkjos@android.com>,
	"Martijn Coenen" <maco@android.com>,
	"Joel Fernandes" <joel@joelfernandes.org>,
	"Christian Brauner" <brauner@kernel.org>,
	"Hridya Valsaraju" <hridya@google.com>,
	"Suren Baghdasaryan" <surenb@google.com>,
	"Theodore Ts'o" <tytso@mit.edu>,
	"David S. Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Alex Xu (Hello71)" <alex_y_xu@yahoo.ca>,
	"Paolo Abeni" <pabeni@redhat.com>,
	"Rob Herring" <robh@kernel.org>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	"Frederic Weisbecker" <frederic@kernel.org>,
	"Neeraj Upadhyay" <quic_neeraju@quicinc.com>,
	"Josh Triplett" <josh@joshtriplett.org>,
	"Steven Rostedt" <rostedt@goodmis.org>,
	"Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>,
	"Lai Jiangshan" <jiangshanlai@gmail.com>,
	"Shuah Khan" <shuah@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	wireguard@lists.zx2c4.com, netdev@vger.kernel.org,
	rcu <rcu@vger.kernel.org>,
	"open list:KERNEL SELFTEST FRAMEWORK"
	<linux-kselftest@vger.kernel.org>,
	sultan@kerneltoast.com,
	android-kernel-team <android-kernel-team@google.com>,
	"Saravana Kannan" <saravanak@google.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>
Subject: Re: [PATCH] remove CONFIG_ANDROID
Date: Wed, 29 Jun 2022 16:52:05 -0700	[thread overview]
Message-ID: <CANDhNCpRzzULaGmEGCbbJgVinA0pJJB-gOP9AY0Hy488n9ZStA@mail.gmail.com> (raw)
In-Reply-To: <YrzaCRl9rwy9DgOC@zx2c4.com>

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:
>
> 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

  parent reply	other threads:[~2022-06-29 23:54 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-29 15:01 Christoph Hellwig
2022-06-29 15:01 ` [PATCH] " Christoph Hellwig
2022-06-29 15:27   ` Greg Kroah-Hartman
2022-06-29 16:07   ` Paul E. McKenney
2022-06-29 16:09   ` Jason A. Donenfeld
2022-06-29 16:10     ` Christoph Hellwig
2022-06-29 16:13       ` Jason A. Donenfeld
2022-06-29 16:15         ` Christoph Hellwig
2022-06-29 16:25           ` Jason A. Donenfeld
2022-06-29 16:30             ` Christoph Hellwig
2022-06-29 16:38               ` Jason A. Donenfeld
2022-06-29 16:45                 ` Christoph Hellwig
2022-06-29 16:52                   ` Jason A. Donenfeld
2022-06-29 17:00                     ` Greg Kroah-Hartman
2022-06-29 17:10                       ` Jason A. Donenfeld
2022-06-29 17:19                         ` Greg Kroah-Hartman
2022-06-29 17:30                           ` Jason A. Donenfeld
2022-06-29 17:35                             ` Christoph Hellwig
2022-06-29 17:42                               ` Jason A. Donenfeld
2022-06-29 18:59                           ` Paul E. McKenney
2022-06-29 16:56                   ` Steven Rostedt
2022-06-29 17:19                     ` Jason A. Donenfeld
2022-06-29 17:34                 ` Jason A. Donenfeld
2022-06-29 19:05                   ` Kalesh Singh
2022-06-29 19:41                     ` Theodore Ts'o
2022-06-29 20:47                     ` Jason A. Donenfeld
2022-06-29 22:26                       ` Kalesh Singh
2022-06-29 23:02                         ` Jason A. Donenfeld
2022-06-29 23:19                           ` Kalesh Singh
2022-06-30  0:36                             ` Joe Perches
2022-06-30  0:50                               ` Jason A. Donenfeld
2022-06-30  1:44                                 ` Joe Perches
2022-06-30  3:02                                   ` Jason A. Donenfeld
2022-06-29 23:52                           ` John Stultz [this message]
2022-06-30  0:24                             ` Jason A. Donenfeld
2022-06-30  0:30                               ` Jason A. Donenfeld
2022-06-30  4:25                                 ` Kalesh Singh
2022-06-30 10:05                                   ` Jason A. Donenfeld
2022-06-30 17:12                                     ` John Stultz
2022-06-30 17:24                                       ` Jason A. Donenfeld
2022-07-01 20:22                               ` Jonathan Corbet
2022-07-01 20:53                                 ` Jason A. Donenfeld
2022-06-29 16:34           ` Paul E. McKenney
2022-06-29 16:37             ` Christoph Hellwig
2022-06-29 16:45               ` Jason A. Donenfeld
2022-06-29 17:04                 ` Paul E. McKenney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CANDhNCpRzzULaGmEGCbbJgVinA0pJJB-gOP9AY0Hy488n9ZStA@mail.gmail.com \
    --to=jstultz@google.com \
    --cc=Jason@zx2c4.com \
    --cc=alex_y_xu@yahoo.ca \
    --cc=android-kernel-team@google.com \
    --cc=arve@android.com \
    --cc=brauner@kernel.org \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=frederic@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@lst.de \
    --cc=hridya@google.com \
    --cc=jiangshanlai@gmail.com \
    --cc=joel@joelfernandes.org \
    --cc=josh@joshtriplett.org \
    --cc=kaleshsingh@google.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=maco@android.com \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=paulmck@kernel.org \
    --cc=quic_neeraju@quicinc.com \
    --cc=rafael@kernel.org \
    --cc=rcu@vger.kernel.org \
    --cc=robh@kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=saravanak@google.com \
    --cc=shuah@kernel.org \
    --cc=sultan@kerneltoast.com \
    --cc=surenb@google.com \
    --cc=tkjos@android.com \
    --cc=tytso@mit.edu \
    --cc=wireguard@lists.zx2c4.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).