remove CONFIG_ANDROID

remove CONFIG_ANDROID

[Christoph Hellwig]

Hi Greg,

this series removes the CONFIG_ANDROID.  It just guards the Kconfig
option for binder and then changes a bunch of random defaults and
settings, which makes no sense whatsoever and none of those changes
had any good justifcation in their commit logs either.

[PATCH] remove CONFIG_ANDROID

[Christoph Hellwig]

The ANDROID config symbol is only used to guard the binder config
symbol and to inject completely random config changes.  Remove it
as it is obviously a bad idea.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/Makefile                                    | 2 +-
 drivers/android/Kconfig                             | 9 ---------
 drivers/char/random.c                               | 3 +--
 drivers/net/wireguard/device.c                      | 2 +-
 kernel/configs/android-base.config                  | 1 -
 kernel/rcu/Kconfig.debug                            | 3 +--
 tools/testing/selftests/filesystems/binderfs/config | 1 -
 tools/testing/selftests/sync/config                 | 1 -
 8 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/Makefile b/drivers/Makefile
index 9a30842b22c54..123dce2867583 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -176,7 +176,7 @@ obj-$(CONFIG_USB4)		+= thunderbolt/
 obj-$(CONFIG_CORESIGHT)		+= hwtracing/coresight/
 obj-y				+= hwtracing/intel_th/
 obj-$(CONFIG_STM)		+= hwtracing/stm/
-obj-$(CONFIG_ANDROID)		+= android/
+obj-y				+= android/
 obj-$(CONFIG_NVMEM)		+= nvmem/
 obj-$(CONFIG_FPGA)		+= fpga/
 obj-$(CONFIG_FSI)		+= fsi/
diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
index 53b22e26266c3..07aa8ae0a058c 100644
--- a/drivers/android/Kconfig
+++ b/drivers/android/Kconfig
@@ -1,13 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 menu "Android"
 
-config ANDROID
-	bool "Android Drivers"
-	help
-	  Enable support for various drivers needed on the Android platform
-
-if ANDROID
-
 config ANDROID_BINDER_IPC
 	bool "Android Binder IPC Driver"
 	depends on MMU
@@ -54,6 +47,4 @@ config ANDROID_BINDER_IPC_SELFTEST
 	  exhaustively with combinations of various buffer sizes and
 	  alignments.
 
-endif # if ANDROID
-
 endmenu
diff --git a/drivers/char/random.c b/drivers/char/random.c
index e3dd1dd3dd226..f35ad1a9dff3e 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -755,8 +755,7 @@ static int random_pm_notification(struct notifier_block *nb, unsigned long actio
 	spin_unlock_irqrestore(&input_pool.lock, flags);
 
 	if (crng_ready() && (action == PM_RESTORE_PREPARE ||
-	    (action == PM_POST_SUSPEND &&
-	     !IS_ENABLED(CONFIG_PM_AUTOSLEEP) && !IS_ENABLED(CONFIG_ANDROID)))) {
+	    (action == PM_POST_SUSPEND && !IS_ENABLED(CONFIG_PM_AUTOSLEEP)))) {
 		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 aa9a7a5970fda..de1cc03f7ee86 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))
 		return 0;
 
 	if (action != PM_HIBERNATION_PREPARE && action != PM_SUSPEND_PREPARE)
diff --git a/kernel/configs/android-base.config b/kernel/configs/android-base.config
index eb701b2ac72ff..44b0f0146a3fc 100644
--- a/kernel/configs/android-base.config
+++ b/kernel/configs/android-base.config
@@ -7,7 +7,6 @@
 # CONFIG_OABI_COMPAT is not set
 # CONFIG_SYSVIPC is not set
 # CONFIG_USELIB is not set
-CONFIG_ANDROID=y
 CONFIG_ANDROID_BINDER_IPC=y
 CONFIG_ANDROID_BINDER_DEVICES=binder,hwbinder,vndbinder
 CONFIG_ANDROID_LOW_MEMORY_KILLER=y
diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
index 9b64e55d4f615..e875f4f889656 100644
--- a/kernel/rcu/Kconfig.debug
+++ b/kernel/rcu/Kconfig.debug
@@ -86,8 +86,7 @@ config RCU_EXP_CPU_STALL_TIMEOUT
 	int "Expedited RCU CPU stall timeout in milliseconds"
 	depends on RCU_STALL_COMMON
 	range 0 21000
-	default 20 if ANDROID
-	default 0 if !ANDROID
+	default 0
 	help
 	  If a given expedited RCU grace period extends more than the
 	  specified number of milliseconds, a CPU stall warning is printed.
diff --git a/tools/testing/selftests/filesystems/binderfs/config b/tools/testing/selftests/filesystems/binderfs/config
index 02dd6cc9cf992..7b4fc6ee62057 100644
--- a/tools/testing/selftests/filesystems/binderfs/config
+++ b/tools/testing/selftests/filesystems/binderfs/config
@@ -1,3 +1,2 @@
-CONFIG_ANDROID=y
 CONFIG_ANDROID_BINDERFS=y
 CONFIG_ANDROID_BINDER_IPC=y
diff --git a/tools/testing/selftests/sync/config b/tools/testing/selftests/sync/config
index 47ff5afc37271..64c60f38b4464 100644
--- a/tools/testing/selftests/sync/config
+++ b/tools/testing/selftests/sync/config
@@ -1,3 +1,2 @@
 CONFIG_STAGING=y
-CONFIG_ANDROID=y
 CONFIG_SW_SYNC=y
-- 
2.30.2

Re: [PATCH] remove CONFIG_ANDROID

[Greg Kroah-Hartman]

On Wed, Jun 29, 2022 at 05:01:02PM +0200, Christoph Hellwig wrote:
> The ANDROID config symbol is only used to guard the binder config
> symbol and to inject completely random config changes.  Remove it
> as it is obviously a bad idea.

Ick, rcu and random driver changes?  That's not ok, I'll go queue this
up, if Android devices need to make these core changes, they can do that
in their kernel or submit a patch that makes sense for everyone.

Also, one meta-comment:

>  kernel/configs/android-base.config                  | 1 -

This whole file can be deleted now, with the way Android kernels are now
being managed it makes no sense anymore.  I'll write up a patch to do
that later tonight.

thanks,

greg k-h

Re: [PATCH] remove CONFIG_ANDROID

[Paul E. McKenney]

On Wed, Jun 29, 2022 at 05:01:02PM +0200, Christoph Hellwig wrote:
> The ANDROID config symbol is only used to guard the binder config
> symbol and to inject completely random config changes.  Remove it
> as it is obviously a bad idea.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

And I got confirmation from the Android folks that they have other ways
of getting their 20-millisecond expedited RCU CPU stall warnings, so:

Acked-by: Paul E. McKenney <paulmck@kernel.org>

> ---
>  drivers/Makefile                                    | 2 +-
>  drivers/android/Kconfig                             | 9 ---------
>  drivers/char/random.c                               | 3 +--
>  drivers/net/wireguard/device.c                      | 2 +-
>  kernel/configs/android-base.config                  | 1 -
>  kernel/rcu/Kconfig.debug                            | 3 +--
>  tools/testing/selftests/filesystems/binderfs/config | 1 -
>  tools/testing/selftests/sync/config                 | 1 -
>  8 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 9a30842b22c54..123dce2867583 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -176,7 +176,7 @@ obj-$(CONFIG_USB4)		+= thunderbolt/
>  obj-$(CONFIG_CORESIGHT)		+= hwtracing/coresight/
>  obj-y				+= hwtracing/intel_th/
>  obj-$(CONFIG_STM)		+= hwtracing/stm/
> -obj-$(CONFIG_ANDROID)		+= android/
> +obj-y				+= android/
>  obj-$(CONFIG_NVMEM)		+= nvmem/
>  obj-$(CONFIG_FPGA)		+= fpga/
>  obj-$(CONFIG_FSI)		+= fsi/
> diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
> index 53b22e26266c3..07aa8ae0a058c 100644
> --- a/drivers/android/Kconfig
> +++ b/drivers/android/Kconfig
> @@ -1,13 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  menu "Android"
>  
> -config ANDROID
> -	bool "Android Drivers"
> -	help
> -	  Enable support for various drivers needed on the Android platform
> -
> -if ANDROID
> -
>  config ANDROID_BINDER_IPC
>  	bool "Android Binder IPC Driver"
>  	depends on MMU
> @@ -54,6 +47,4 @@ config ANDROID_BINDER_IPC_SELFTEST
>  	  exhaustively with combinations of various buffer sizes and
>  	  alignments.
>  
> -endif # if ANDROID
> -
>  endmenu
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index e3dd1dd3dd226..f35ad1a9dff3e 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -755,8 +755,7 @@ static int random_pm_notification(struct notifier_block *nb, unsigned long actio
>  	spin_unlock_irqrestore(&input_pool.lock, flags);
>  
>  	if (crng_ready() && (action == PM_RESTORE_PREPARE ||
> -	    (action == PM_POST_SUSPEND &&
> -	     !IS_ENABLED(CONFIG_PM_AUTOSLEEP) && !IS_ENABLED(CONFIG_ANDROID)))) {
> +	    (action == PM_POST_SUSPEND && !IS_ENABLED(CONFIG_PM_AUTOSLEEP)))) {
>  		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 aa9a7a5970fda..de1cc03f7ee86 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))
>  		return 0;
>  
>  	if (action != PM_HIBERNATION_PREPARE && action != PM_SUSPEND_PREPARE)
> diff --git a/kernel/configs/android-base.config b/kernel/configs/android-base.config
> index eb701b2ac72ff..44b0f0146a3fc 100644
> --- a/kernel/configs/android-base.config
> +++ b/kernel/configs/android-base.config
> @@ -7,7 +7,6 @@
>  # CONFIG_OABI_COMPAT is not set
>  # CONFIG_SYSVIPC is not set
>  # CONFIG_USELIB is not set
> -CONFIG_ANDROID=y
>  CONFIG_ANDROID_BINDER_IPC=y
>  CONFIG_ANDROID_BINDER_DEVICES=binder,hwbinder,vndbinder
>  CONFIG_ANDROID_LOW_MEMORY_KILLER=y
> diff --git a/kernel/rcu/Kconfig.debug b/kernel/rcu/Kconfig.debug
> index 9b64e55d4f615..e875f4f889656 100644
> --- a/kernel/rcu/Kconfig.debug
> +++ b/kernel/rcu/Kconfig.debug
> @@ -86,8 +86,7 @@ config RCU_EXP_CPU_STALL_TIMEOUT
>  	int "Expedited RCU CPU stall timeout in milliseconds"
>  	depends on RCU_STALL_COMMON
>  	range 0 21000
> -	default 20 if ANDROID
> -	default 0 if !ANDROID
> +	default 0
>  	help
>  	  If a given expedited RCU grace period extends more than the
>  	  specified number of milliseconds, a CPU stall warning is printed.
> diff --git a/tools/testing/selftests/filesystems/binderfs/config b/tools/testing/selftests/filesystems/binderfs/config
> index 02dd6cc9cf992..7b4fc6ee62057 100644
> --- a/tools/testing/selftests/filesystems/binderfs/config
> +++ b/tools/testing/selftests/filesystems/binderfs/config
> @@ -1,3 +1,2 @@
> -CONFIG_ANDROID=y
>  CONFIG_ANDROID_BINDERFS=y
>  CONFIG_ANDROID_BINDER_IPC=y
> diff --git a/tools/testing/selftests/sync/config b/tools/testing/selftests/sync/config
> index 47ff5afc37271..64c60f38b4464 100644
> --- a/tools/testing/selftests/sync/config
> +++ b/tools/testing/selftests/sync/config
> @@ -1,3 +1,2 @@
>  CONFIG_STAGING=y
> -CONFIG_ANDROID=y
>  CONFIG_SW_SYNC=y
> -- 
> 2.30.2
> 

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

Hi Christoph,

On Wed, Jun 29, 2022 at 05:01:02PM +0200, Christoph Hellwig wrote:
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index e3dd1dd3dd226..f35ad1a9dff3e 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -755,8 +755,7 @@ static int random_pm_notification(struct notifier_block *nb, unsigned long actio
>  	spin_unlock_irqrestore(&input_pool.lock, flags);
>  
>  	if (crng_ready() && (action == PM_RESTORE_PREPARE ||
> -	    (action == PM_POST_SUSPEND &&
> -	     !IS_ENABLED(CONFIG_PM_AUTOSLEEP) && !IS_ENABLED(CONFIG_ANDROID)))) {
> +	    (action == PM_POST_SUSPEND && !IS_ENABLED(CONFIG_PM_AUTOSLEEP)))) {
>  		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 aa9a7a5970fda..de1cc03f7ee86 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))
>  		return 0;
>  
>  	if (action != PM_HIBERNATION_PREPARE && action != PM_SUSPEND_PREPARE)
 
CONFIG_ANDROID is used here for a reason. As somebody suggested in
another thread of which you were a participant, it acts as a proxy for
"probably running on Android hardware", which in turn is a proxy for,
"suspend happens all the time on this machine, so don't do fancy key
clearing stuff every time the user clicks the power button."

You can see the history of that in these two commits here:
https://git.zx2c4.com/wireguard-linux-compat/commit/?id=36f81c83674e0fd7c18e5b15499d1a275b6d4d7f
https://git.zx2c4.com/wireguard-linux-compat/commit/?id=a89d53098dbde43f56e4d1e16ba5e24ef807c03b

The former commit was done when I first got this running on an Android
device (a Oneplus 3T, IIRC) and I encountered this problem. The latter
was a refinement after suggestions on LKML during WireGuard's
upstreaming.

So there *is* a reason to have that kind of conditionalization in the
code. The question is: does CONFIG_ANDROID actually represent something
interesting here? Is this already taken care of by CONFIG_PM_AUTOSLEEP
on all CONFIG_ANDROID devices? That is, do the base Android configs set
CONFIG_PM_AUTOSLEEP already so this isn't necessary? Or is there some
*other* proxy config value that should be used? Or is there a different
solution entirely that should be considered?

I don't know the answers to these questions, because I haven't done a
recent analysis. Obviously at one point in time I did, and that's why
the code is how it is. It sounds like you'd now like to revisit that
original decision. That's fine with me. But you need to conduct a new
analysis and write down your findings inside of a commit message. I must
see that you've at least thought about the problem and looked into it
substantially enough that making this change is safe. Your "let's delete
it; it's not doing much" alone seems more expedient than thorough.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Christoph Hellwig]

On Wed, Jun 29, 2022 at 06:09:18PM +0200, Jason A. Donenfeld wrote:
> CONFIG_ANDROID is used here for a reason. As somebody suggested in
> another thread of which you were a participant, it acts as a proxy for
> "probably running on Android hardware",

No, it does not in any way.

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

On Wed, Jun 29, 2022 at 06:10:20PM +0200, Christoph Hellwig wrote:
> On Wed, Jun 29, 2022 at 06:09:18PM +0200, Jason A. Donenfeld wrote:
> > CONFIG_ANDROID is used here for a reason. As somebody suggested in
> > another thread of which you were a participant, it acts as a proxy for
> > "probably running on Android hardware",
> 
> No, it does not in any way.

Good! It sounds like you're starting to develop opinions on the matter.
Please weave these into some analysis of the issue and put this in your
v2 patch. To be clear, the thing I care about is that this won't make
the behavior worse for any kernels. If you feel like you've got that
covered, say why in your patch, and then it's fine by me.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Christoph Hellwig]

On Wed, Jun 29, 2022 at 06:13:05PM +0200, Jason A. Donenfeld wrote:
> Good! It sounds like you're starting to develop opinions on the matter.

No, I provide facts.  Look at both the definition of the symbol, and
various distribution kernel that enabled it and think hard if they run
on "Android" hardware.  Not just primarily, but at all.

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

On Wed, Jun 29, 2022 at 06:15:27PM +0200, Christoph Hellwig wrote:
> On Wed, Jun 29, 2022 at 06:13:05PM +0200, Jason A. Donenfeld wrote:
> > Good! It sounds like you're starting to develop opinions on the matter.
> 
> No, I provide facts.

Lol.

> Look at both the definition of the symbol, and
> various distribution kernel that enabled it and think hard if they run
> on "Android" hardware.  Not just primarily, but at all.

There are two failure modes:

1) Key clearing code is skipped when it shouldn't be.
2) Key clearing code is run when it shouldn't be.

You've identified (well, Alex in the other thread I think?) a case of
(1). I was sort of thinking the fix to that would be that distros
shouldn't enable that option, but it doesn't really matter to me.

However, what I'm pointing out is the potential for (2). A (2)-style
regression means that WireGuard basically doesn't work, because, for
example, qcacld's packet-triggered wakeups tend to be too short to
renegotiate a handshake.

Anyway, instead of the slow drip of "facts" and ≤three sentence emails,
can you just write up a paragraph that indicates this is safe to do (for
both (1) and (2)) in your v+1?

I don't really want to argue about it, because I don't have anything to
argue about. Your change is probably fine. I'd just like it to be
spelled out why this is safe to do from somebody who has looked into it.
I have not looked into it, but it sounds like you have or are in the
process of doing so. Just write down what you find, please.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Christoph Hellwig]

On Wed, Jun 29, 2022 at 06:25:32PM +0200, Jason A. Donenfeld wrote:
> Anyway, instead of the slow drip of "facts" and ≤three sentence emails,
> can you just write up a paragraph that indicates this is safe to do (for
> both (1) and (2)) in your v+1?

Why would I care?  If your config wakeups up so often that you need
special casing find a way to deal with it.  In the upstream kernel
CONFIG_ANDROID is a very strong indicator for a desktop kernel as
that is much more common than someone actually running upstream Linux
on one of the very few Android devices actually fully supported by
upstream Linux.

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

On Wed, Jun 29, 2022 at 06:30:07PM +0200, Christoph Hellwig wrote:
> On Wed, Jun 29, 2022 at 06:25:32PM +0200, Jason A. Donenfeld wrote:
> > Anyway, instead of the slow drip of "facts" and ≤three sentence emails,
> > can you just write up a paragraph that indicates this is safe to do (for
> > both (1) and (2)) in your v+1?
> 
> Why would I care?  If your config wakeups up so often that you need
> special casing find a way to deal with it. 

You should care, because the change you're suggesting might break code
that I maintain. If you don't care about breaking my code with your
change, than just have my "nack", and it'll be up to Greg to decide
whether he wants to apply this despite the nack for two separate
affected code spots.

On the technical topic, an Android developer friend following this
thread just pointed out to me that Android doesn't use PM_AUTOSLEEP and
just has userspace causing suspend frequently. So by his rough
estimation your patch actually *will* break Android devices. Zoinks.
Maybe he's right, maybe he's not -- I don't know -- but you should
probably look into this if you want this patch to land without breakage.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Christoph Hellwig]

On Wed, Jun 29, 2022 at 06:38:09PM +0200, Jason A. Donenfeld wrote:
> On the technical topic, an Android developer friend following this
> thread just pointed out to me that Android doesn't use PM_AUTOSLEEP and
> just has userspace causing suspend frequently. So by his rough
> estimation your patch actually *will* break Android devices. Zoinks.
> Maybe he's right, maybe he's not -- I don't know -- but you should
> probably look into this if you want this patch to land without breakage.

And it will also "break" anyone else doing frequent suspends from
userspace, as that behavior is still in no way related to
CONFIG_ANDROID.

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

On Wed, Jun 29, 2022 at 6:45 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Wed, Jun 29, 2022 at 06:38:09PM +0200, Jason A. Donenfeld wrote:
> > On the technical topic, an Android developer friend following this
> > thread just pointed out to me that Android doesn't use PM_AUTOSLEEP and
> > just has userspace causing suspend frequently. So by his rough
> > estimation your patch actually *will* break Android devices. Zoinks.
> > Maybe he's right, maybe he's not -- I don't know -- but you should
> > probably look into this if you want this patch to land without breakage.
>
> And it will also "break" anyone else doing frequent suspends from
> userspace, as that behavior is still in no way related to
> CONFIG_ANDROID.

I don't know of any actual systems that do this for which
CONFIG_PM_AUTOSLEEP and CONFIG_ANDROID are both disabled. At least
that was what I concluded back in 2017-2018 when I looked at this
last. And so far, no other-handset-users have reported bugs.

But of course I agree that this all could be improved with something
more granular somehow, somewhere. I don't really have any developed
opinions on what that looks like or what form that should take.

However, the thing I do have a strong opinion on is that the change
you're proposing shouldn't break things. And that's what your patch
currently might do (or not!).

In other words, instead of making the current situation worse,
justified by your observation that in theory the current mechanism is
imperfect, please make the situation better. Or conclude that it
doesn't make the situation worse, and put the reason why inside your
commit message, which is the first thing I asked.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Greg Kroah-Hartman]

On Wed, Jun 29, 2022 at 06:52:08PM +0200, Jason A. Donenfeld wrote:
> On Wed, Jun 29, 2022 at 6:45 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > On Wed, Jun 29, 2022 at 06:38:09PM +0200, Jason A. Donenfeld wrote:
> > > On the technical topic, an Android developer friend following this
> > > thread just pointed out to me that Android doesn't use PM_AUTOSLEEP and
> > > just has userspace causing suspend frequently. So by his rough
> > > estimation your patch actually *will* break Android devices. Zoinks.
> > > Maybe he's right, maybe he's not -- I don't know -- but you should
> > > probably look into this if you want this patch to land without breakage.
> >
> > And it will also "break" anyone else doing frequent suspends from
> > userspace, as that behavior is still in no way related to
> > CONFIG_ANDROID.
> 
> I don't know of any actual systems that do this for which
> CONFIG_PM_AUTOSLEEP and CONFIG_ANDROID are both disabled. At least
> that was what I concluded back in 2017-2018 when I looked at this
> last. And so far, no other-handset-users have reported bugs.
> 
> But of course I agree that this all could be improved with something
> more granular somehow, somewhere. I don't really have any developed
> opinions on what that looks like or what form that should take.
> 
> However, the thing I do have a strong opinion on is that the change
> you're proposing shouldn't break things. And that's what your patch
> currently might do (or not!).

I think that by the time the next kernel release comes out, and
percolates to a real Android device, the years gone by will have caused
those who care about this to fix it.

In the meantime, this might actually fix issues in desktop distros that
were enabling this option, thinking it only affected the building of a
driver, not core power management functionality.

So it's nothing to worry about now, I agree with Christoph, this config
option should not be used for power management policy decisions like
this.  This should be controlled by userspace properly in the Android
userspace framework, like all other Linux distros/systems do this.

And worst case, Android kernels sometimes _do_ have a not-upstream
config option that you can use to trigger off of horrible hacks like
this.  I'll leave the answer to what that is as an exercise for the
reader :)

thanks,

greg k-h

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

On Wed, Jun 29, 2022 at 07:00:25PM +0200, Greg Kroah-Hartman wrote:
> I think that by the time the next kernel release comes out, and
> percolates to a real Android device, the years gone by will have caused
> those who care about this to fix it.

You assume that there aren't Android devices using kernels outside of
the ones you're referring to. That's a rather Google-centric
perspective. It's still breakage, even if Google has the ability to fix
it locally after "years gone by". If you want Android things to be
upstream, this is the way you must think about it; otherwise, what's the
point? By your logic, upstream should probably remove the Android code
everywhere and let Google handle it downstream. Except nobody wants
that; we want Android upstream. So let's keep it working upstream, not
intentionally break it.

> In the meantime, this might actually fix issues in desktop distros that
> were enabling this option, thinking it only affected the building of a
> driver

That sounds like a false dichotomy. It's not about "fix Android" vs "fix
distros". What I'm suggesting is fixing Android AND fixing distros, by
looking at the problem holistically. Trading a bad problem on Android
(wg connections are broken) for a manageable problem on distros (something
something theoretical warm boot attack something) doesn't sound like a
nice trade off. Let's instead get this all fixed at the same time.

> So it's nothing to worry about now, I agree with Christoph, this config
> option should not be used for power management policy decisions like
> this.  This should be controlled by userspace properly in the Android
> userspace framework, like all other Linux distros/systems do this.

Except right now it is. So if it's going to be removed, the code that
was depending on it will need to be updated coherently.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Greg Kroah-Hartman]

On Wed, Jun 29, 2022 at 07:10:43PM +0200, Jason A. Donenfeld wrote:
> On Wed, Jun 29, 2022 at 07:00:25PM +0200, Greg Kroah-Hartman wrote:
> > I think that by the time the next kernel release comes out, and
> > percolates to a real Android device, the years gone by will have caused
> > those who care about this to fix it.
> 
> You assume that there aren't Android devices using kernels outside of
> the ones you're referring to. That's a rather Google-centric
> perspective. It's still breakage, even if Google has the ability to fix
> it locally after "years gone by". If you want Android things to be
> upstream, this is the way you must think about it; otherwise, what's the
> point? By your logic, upstream should probably remove the Android code
> everywhere and let Google handle it downstream. Except nobody wants
> that; we want Android upstream. So let's keep it working upstream, not
> intentionally break it.

I would be totally and completly amazed if there are any Android kernels
in real devices in the world that are not at the very least, based on
LTS releases.  But maybe there is, this patch series isn't going to land
until 5.20, and by then, I think the "define behavior, not hardware" fix
for random and wg will be properly resolved :)

> > In the meantime, this might actually fix issues in desktop distros that
> > were enabling this option, thinking it only affected the building of a
> > driver
> 
> That sounds like a false dichotomy. It's not about "fix Android" vs "fix
> distros". What I'm suggesting is fixing Android AND fixing distros, by
> looking at the problem holistically. Trading a bad problem on Android
> (wg connections are broken) for a manageable problem on distros (something
> something theoretical warm boot attack something) doesn't sound like a
> nice trade off. Let's instead get this all fixed at the same time.

Agreed, so what should we use instead in the wg code?  What userspace
functionality are you trying to trigger off of here in the current
CONFIG_ANDROID check?

The RCU stuff is already handled as Paul has stated, so that's not an
issue.

thanks,

greg k-h

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

On Wed, Jun 29, 2022 at 07:19:36PM +0200, Greg Kroah-Hartman wrote:
> I would be totally and completly amazed if there are any Android kernels
> in real devices in the world that are not at the very least, based on
> LTS releases.  But maybe there is, this patch series isn't going to land
> until 5.20, and by then, I think the "define behavior, not hardware" fix
> for random and wg will be properly resolved :)

Properly resolved by whom? It sounds like you're up for intentionally
allowing a userspace regression, and also volunteering other people's
time into fixing that regression? The way I understand the kernel
development process is that the person proposing a change is responsible
for not intentionally causing regressions, and if one is pointed out, a
v+1 of that patch is provided that doesn't cause the regression.

> 
> > > In the meantime, this might actually fix issues in desktop distros that
> > > were enabling this option, thinking it only affected the building of a
> > > driver
> > 
> > That sounds like a false dichotomy. It's not about "fix Android" vs "fix
> > distros". What I'm suggesting is fixing Android AND fixing distros, by
> > looking at the problem holistically. Trading a bad problem on Android
> > (wg connections are broken) for a manageable problem on distros (something
> > something theoretical warm boot attack something) doesn't sound like a
> > nice trade off. Let's instead get this all fixed at the same time.
> 
> Agreed, so what should we use instead in the wg code?  What userspace
> functionality are you trying to trigger off of here in the current
> CONFIG_ANDROID check?

It's systems that go to sleep all the time, nonstop, like Android
handsets do. I'm fine with a compile time constant for this, or some
runtime sysctl, or whatever else. It doesn't really matter to me. The
only concern is that the Android people are okay with it and enable it
(and that the distros don't enable it, I guess). So whatever they want
is fine.  Or maybe such a runtime indicator already exists. I'm not
sure. But I think a v+1 of this patchset needs to work that out in one
direction or another.

To put it simply,

    - I highly suspect that this patch will cause a regression.
    - I don't have a ready-made solution offhand to fix said regression.
    - The submitter of the patch, now aware of regression potential,
      should look into not causing that regression.

That seems pretty clear cut: you're welcome to improve it, but please
don't *intentionally* break my code.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Christoph Hellwig]

On Wed, Jun 29, 2022 at 07:30:35PM +0200, Jason A. Donenfeld wrote:
> Properly resolved by whom? It sounds like you're up for intentionally
> allowing a userspace regression, and also volunteering other people's
> time into fixing that regression? The way I understand the kernel
> development process is that the person proposing a change is responsible
> for not intentionally causing regressions, and if one is pointed out, a
> v+1 of that patch is provided that doesn't cause the regression.

If you think the code does not work when the system frequently suspends
and resumes, then well it is broken already, as that can happen just
as much on non-Android systems.  So maybe we should just remove it if
it is so broken that you fear about regressions on the 3 and a half
Android systems in the world running an upstream kernel?

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

On Wed, Jun 29, 2022 at 07:35:45PM +0200, Christoph Hellwig wrote:
> On Wed, Jun 29, 2022 at 07:30:35PM +0200, Jason A. Donenfeld wrote:
> > Properly resolved by whom? It sounds like you're up for intentionally
> > allowing a userspace regression, and also volunteering other people's
> > time into fixing that regression? The way I understand the kernel
> > development process is that the person proposing a change is responsible
> > for not intentionally causing regressions, and if one is pointed out, a
> > v+1 of that patch is provided that doesn't cause the regression.
> 
> If you think the code does not work when the system frequently suspends
> and resumes, then well it is broken already, as that can happen just
> as much on non-Android systems.

I don't know how you arrived at that sentence or conclusion. The
regression I'm referring to in that paragraph is the one that *your*
patch would introduce were it to be applied.

The code currently does work well on Android devices. These very
messages are transiting through it, even.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Paul E. McKenney]

On Wed, Jun 29, 2022 at 07:19:36PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jun 29, 2022 at 07:10:43PM +0200, Jason A. Donenfeld wrote:
> > On Wed, Jun 29, 2022 at 07:00:25PM +0200, Greg Kroah-Hartman wrote:

[ . . . ]

> > That sounds like a false dichotomy. It's not about "fix Android" vs "fix
> > distros". What I'm suggesting is fixing Android AND fixing distros, by
> > looking at the problem holistically. Trading a bad problem on Android
> > (wg connections are broken) for a manageable problem on distros (something
> > something theoretical warm boot attack something) doesn't sound like a
> > nice trade off. Let's instead get this all fixed at the same time.
> 
> Agreed, so what should we use instead in the wg code?  What userspace
> functionality are you trying to trigger off of here in the current
> CONFIG_ANDROID check?
> 
> The RCU stuff is already handled as Paul has stated, so that's not an
> issue.

To be fair, one advantage that the RCU stuff has is that it only very
recently hit mainline, so people are only just now starting to make use
of it.  Jason's code has been in for some time, so he has more existing
practice to deal with.

							Thanx, Paul

Re: [PATCH] remove CONFIG_ANDROID

[Steven Rostedt]

[ Note, I'm not on the Android team and my response has nothing to do with
  my employer. I would say the same thing with my previous employer. ]

On Wed, 29 Jun 2022 18:45:43 +0200
Christoph Hellwig <hch@lst.de> wrote:

> On Wed, Jun 29, 2022 at 06:38:09PM +0200, Jason A. Donenfeld wrote:
> > On the technical topic, an Android developer friend following this
> > thread just pointed out to me that Android doesn't use PM_AUTOSLEEP and
> > just has userspace causing suspend frequently. So by his rough
> > estimation your patch actually *will* break Android devices. Zoinks.
> > Maybe he's right, maybe he's not -- I don't know -- but you should
> > probably look into this if you want this patch to land without breakage.  
> 
> And it will also "break" anyone else doing frequent suspends from
> userspace, as that behavior is still in no way related to
> CONFIG_ANDROID.

Should there then be a CONFIG_FREQUENT_SUSPENDS ?

That is, if you have system where you know that there will be a lot of
frequent suspends coming from user space, then you would enable it.

I agree, calling this ANDROID is not related to the functionality change.
But if there was a config that was related, would that be acceptable?

Then it would not just be Android that could enabled this change, but any
other system that is doing frequent suspends?

-- Steve

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

On Wed, Jun 29, 2022 at 12:56:43PM -0400, Steven Rostedt wrote:
> > And it will also "break" anyone else doing frequent suspends from
> > userspace, as that behavior is still in no way related to
> > CONFIG_ANDROID.
> 
> Should there then be a CONFIG_FREQUENT_SUSPENDS ?

That'd be fine by me. It could be selected by PM_AUTOSLEEP as well.

[ Bikeshed: maybe CONFIG_PM_CONTINUOUS_SUSPENDS would make more sense,
  to really drive home how often these suspends are to make the option a
  reasonable thing to turn on. ]

I think Christoph had in mind a runtime switch instead (like a sysctl or
something), but it doesn't make a difference to me whether it's runtime
or compile time. If CONFIG_ANDROID is to go away, the code using it now
needs *some* replacement that's taken up by the Android people. So
whatever they agree to works for me, for what my concerns are.

Maybe v2 of this patchset can propose such an option and the right
Android people can be CC'd on it. (Who are they, by the way? There's no
android-kernel@vger mailing list, right?)

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

On Wed, Jun 29, 2022 at 06:38:09PM +0200, Jason A. Donenfeld wrote:
> On the technical topic, an Android developer friend following this
> thread just pointed out to me that Android doesn't use PM_AUTOSLEEP and
> just has userspace causing suspend frequently. So by his rough
> estimation your patch actually *will* break Android devices. Zoinks.
> Maybe he's right, maybe he's not -- I don't know -- but you should
> probably look into this if you want this patch to land without breakage.

More details: https://cs.android.com/android/platform/superproject/+/master:system/core/libsuspend/autosuspend_wakeup_count.cpp;bpv=1;bpt=1;l=52?q=%22%2Fsys%2Fpower%2Fstate%22&start=51&gsn=sys_power_state&gs=kythe%3A%2F%2Fandroid.googlesource.com%2Fplatform%2Fsuperproject%3Flang%3Dc%252B%252B%3Fpath%3Dsystem%2Fcore%2Flibsuspend%2Fautosuspend_wakeup_count.cpp%23ftWlDJuOhS_2fn3Ri7rClxA30blj_idGgT12aoUHd1o

So indeed it looks like it's userspace controlled. If you want this to
be a runtime, rather than a compiletime, switch, maybe
autosuspend_init() of that file could write to a sysctl.

Who at Google "owns" that code? Can somebody CC them in?

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Kalesh Singh]

On Wed, Jun 29, 2022 at 07:34:58PM +0200, Jason A. Donenfeld wrote:
> On Wed, Jun 29, 2022 at 06:38:09PM +0200, Jason A. Donenfeld wrote:
> > On the technical topic, an Android developer friend following this
> > thread just pointed out to me that Android doesn't use PM_AUTOSLEEP and
> > just has userspace causing suspend frequently. So by his rough
> > estimation your patch actually *will* break Android devices. Zoinks.
> > Maybe he's right, maybe he's not -- I don't know -- but you should
> > probably look into this if you want this patch to land without breakage.
> 
> More details: https://cs.android.com/android/platform/superproject/+/master:system/core/libsuspend/autosuspend_wakeup_count.cpp;bpv=1;bpt=1;l=52?q=%22%2Fsys%2Fpower%2Fstate%22&start=51&gsn=sys_power_state&gs=kythe%3A%2F%2Fandroid.googlesource.com%2Fplatform%2Fsuperproject%3Flang%3Dc%252B%252B%3Fpath%3Dsystem%2Fcore%2Flibsuspend%2Fautosuspend_wakeup_count.cpp%23ftWlDJuOhS_2fn3Ri7rClxA30blj_idGgT12aoUHd1o
> 
> So indeed it looks like it's userspace controlled. If you want this to
> be a runtime, rather than a compiletime, switch, maybe
> autosuspend_init() of that file could write to a sysctl.
> 
> Who at Google "owns" that code? Can somebody CC them in?

Hi Jason,

Thanks for raising this.

Android no longer uses PM_AUTOSLEEP, is correct. libsuspend is
also now deprecated. Android autosuspend is initiatiated from the
userspace system suspend service [1].

A runtime config sounds more reasonable since in the !PM_AUTOSLEEP
case, it is userspace which decides the suspend policy.

[1] https://cs.android.com/android/platform/superproject/+/bf3906ecb33c98ff8edd96c852b884dbccb73295:system/hardware/interfaces/suspend/1.0/default/SystemSuspend.cpp;l=265

--Kalesh

> 
> Jason
> 

Re: [PATCH] remove CONFIG_ANDROID

[Theodore Ts'o]

On Wed, Jun 29, 2022 at 12:05:23PM -0700, Kalesh Singh wrote:
> 
> Android no longer uses PM_AUTOSLEEP, is correct. libsuspend is
> also now deprecated. Android autosuspend is initiatiated from the
> userspace system suspend service [1].

Is anything still using CONFIG_PM_AUTOSLEEP?  Is it perhaps time to
consider deprecating it?

						- Ted

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

Hi Kalesh,

On Wed, Jun 29, 2022 at 12:05:23PM -0700, Kalesh Singh wrote:
> Thanks for raising this.
> 
> Android no longer uses PM_AUTOSLEEP, is correct. libsuspend is
> also now deprecated. Android autosuspend is initiatiated from the
> userspace system suspend service [1].
> 
> A runtime config sounds more reasonable since in the !PM_AUTOSLEEP
> case, it is userspace which decides the suspend policy.
> 
> [1] https://cs.android.com/android/platform/superproject/+/bf3906ecb33c98ff8edd96c852b884dbccb73295:system/hardware/interfaces/suspend/1.0/default/SystemSuspend.cpp;l=265

Bingo, thanks for the pointer. So looking at this, I'm trying to tease
out some heuristic that wouldn't require any changes, but I don't really
see anything _too_ perfect. One fragment of an idea would be that the
kernel treats things in autosuspending mode if anybody is holding open a
fd to /sys/power/state. But I worry this would interact with
non-autosuspending userspaces that also hold open the file. So barring
that, I'm not quite sure.

If you also can't think of something, maybe we should talk about adding
something that requires code changes. In that line of thinking, how
would you feel about opening /sys/power/userspace_autosuspender and
keeping that fd open. Then the kernel API would have
`bool pm_has_userspace_autosuspender(void)` that code could check.
Alternatively, if you don't want refcounting fd semantics for that, just
writing a "1" into a similar file seems fine?

Any strong opinions about it? Personally it doesn't make much of a
difference to me. The important thing is just that it'd be something
you're willing to implement in that SystemSuspend.cpp file.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Kalesh Singh]

On Wed, Jun 29, 2022 at 1:47 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Kalesh,
>
> On Wed, Jun 29, 2022 at 12:05:23PM -0700, Kalesh Singh wrote:
> > Thanks for raising this.
> >
> > Android no longer uses PM_AUTOSLEEP, is correct. libsuspend is
> > also now deprecated. Android autosuspend is initiatiated from the
> > userspace system suspend service [1].
> >
> > A runtime config sounds more reasonable since in the !PM_AUTOSLEEP
> > case, it is userspace which decides the suspend policy.
> >
> > [1] https://cs.android.com/android/platform/superproject/+/bf3906ecb33c98ff8edd96c852b884dbccb73295:system/hardware/interfaces/suspend/1.0/default/SystemSuspend.cpp;l=265
>
> Bingo, thanks for the pointer. So looking at this, I'm trying to tease
> out some heuristic that wouldn't require any changes, but I don't really
> see anything _too_ perfect. One fragment of an idea would be that the
> kernel treats things in autosuspending mode if anybody is holding open a
> fd to /sys/power/state. But I worry this would interact with
> non-autosuspending userspaces that also hold open the file. So barring
> that, I'm not quite sure.
>
> If you also can't think of something, maybe we should talk about adding
> something that requires code changes. In that line of thinking, how
> would you feel about opening /sys/power/userspace_autosuspender and
> keeping that fd open. Then the kernel API would have
> `bool pm_has_userspace_autosuspender(void)` that code could check.
> Alternatively, if you don't want refcounting fd semantics for that, just
> writing a "1" into a similar file seems fine?
>
> Any strong opinions about it? Personally it doesn't make much of a
> difference to me. The important thing is just that it'd be something
> you're willing to implement in that SystemSuspend.cpp file.

Hi Jason,

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.

Thanks,
Kalesh

>
> Jason

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

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);
+
 #ifdef CONFIG_SUSPEND
 static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
 			      char *buf)
@@ -869,6 +886,7 @@ static struct attribute * g[] = {
 #ifdef CONFIG_PM_SLEEP
 	&pm_async_attr.attr,
 	&wakeup_count_attr.attr,
+	&pm_userspace_autosleeper.attr,
 #ifdef CONFIG_SUSPEND
 	&mem_sleep_attr.attr,
 	&sync_on_suspend_attr.attr,

Re: [PATCH] remove CONFIG_ANDROID

[Kalesh Singh]

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?

Sounds like a plan. I'll clean up and repost your patch once the
Gerrit change is ready.

Thanks,
Kalesh
>
> 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);
> +
>  #ifdef CONFIG_SUSPEND
>  static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
>                               char *buf)
> @@ -869,6 +886,7 @@ static struct attribute * g[] = {
>  #ifdef CONFIG_PM_SLEEP
>         &pm_async_attr.attr,
>         &wakeup_count_attr.attr,
> +       &pm_userspace_autosleeper.attr,
>  #ifdef CONFIG_SUSPEND
>         &mem_sleep_attr.attr,
>         &sync_on_suspend_attr.attr,
>

Re: [PATCH] remove CONFIG_ANDROID

[Joe Perches]

On Wed, 2022-06-29 at 16:19 -0700, Kalesh Singh wrote:
> On Wed, Jun 29, 2022 at 4:02 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > 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?
> 
> Sounds like a plan. I'll clean up and repost your patch once the
> Gerrit change is ready.

trivial note:

> > diff --git 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);

This should use sysfs_emit no?

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

On Wed, Jun 29, 2022 at 05:36:57PM -0700, Joe Perches wrote:
> > > +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);
> 
> This should use sysfs_emit no?

Probably, yea. Note that I just copy and pasted a nearby function,
pm_async_show, `:%s/`d the variable name, and then promptly `git diff |
clip`d it and plonked it into my email. Looking at the file, it uses
sprintf all over the place in this fashion. So you may want to submit a
cleanup to Rafael on this if you're right about sysfs_emit() being
universally preferred.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Joe Perches]

On Thu, 2022-06-30 at 02:50 +0200, Jason A. Donenfeld wrote:
> On Wed, Jun 29, 2022 at 05:36:57PM -0700, Joe Perches wrote:
> > > > +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);
> > 
> > This should use sysfs_emit no?
> 
> Probably, yea. Note that I just copy and pasted a nearby function,
> pm_async_show, `:%s/`d the variable name, and then promptly `git diff |
> clip`d it and plonked it into my email. Looking at the file, it uses
> sprintf all over the place in this fashion. So you may want to submit a
> cleanup to Rafael on this if you're right about sysfs_emit() being
> universally preferred.

Perhaps:

(trivial refactored and added a missing newline in autosleep_show)

---
 kernel/power/main.c | 102 ++++++++++++++++++++++++++--------------------------
 1 file changed, 52 insertions(+), 50 deletions(-)

diff --git a/kernel/power/main.c b/kernel/power/main.c
index e3694034b7536..c8a030319b15c 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -100,7 +100,7 @@ int pm_async_enabled = 1;
 static ssize_t pm_async_show(struct kobject *kobj, struct kobj_attribute *attr,
 			     char *buf)
 {
-	return sprintf(buf, "%d\n", pm_async_enabled);
+	return sysfs_emit(buf, "%d\n", pm_async_enabled);
 }
 
 static ssize_t pm_async_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -124,27 +124,25 @@ power_attr(pm_async);
 static ssize_t mem_sleep_show(struct kobject *kobj, struct kobj_attribute *attr,
 			      char *buf)
 {
-	char *s = buf;
+	int len = 0;
 	suspend_state_t i;
 
 	for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++) {
 		if (i >= PM_SUSPEND_MEM && cxl_mem_active())
 			continue;
-		if (mem_sleep_states[i]) {
-			const char *label = mem_sleep_states[i];
+		if (!mem_sleep_states[i])
+			continue;
 
-			if (mem_sleep_current == i)
-				s += sprintf(s, "[%s] ", label);
-			else
-				s += sprintf(s, "%s ", label);
-		}
+		len += sysfs_emit_at(buf, len,
+				     mem_sleep_current == i ? "[%s] " : "%s ",
+				     mem_sleep_states[i]);
 	}
 
-	/* Convert the last space to a newline if needed. */
-	if (s != buf)
-		*(s-1) = '\n';
+	/* Convert the last space to a newline if needed */
+	if (len)
+		sysfs_emit_at(buf, len - 1, "\n");
 
-	return (s - buf);
+	return len;
 }
 
 static suspend_state_t decode_suspend_state(const char *buf, size_t n)
@@ -205,7 +203,7 @@ bool sync_on_suspend_enabled = !IS_ENABLED(CONFIG_SUSPEND_SKIP_SYNC);
 static ssize_t sync_on_suspend_show(struct kobject *kobj,
 				   struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%d\n", sync_on_suspend_enabled);
+	return sysfs_emit(buf, "%d\n", sync_on_suspend_enabled);
 }
 
 static ssize_t sync_on_suspend_store(struct kobject *kobj,
@@ -242,22 +240,22 @@ static const char * const pm_tests[__TEST_AFTER_LAST] = {
 static ssize_t pm_test_show(struct kobject *kobj, struct kobj_attribute *attr,
 				char *buf)
 {
-	char *s = buf;
+	int len = 0;
 	int level;
 
-	for (level = TEST_FIRST; level <= TEST_MAX; level++)
-		if (pm_tests[level]) {
-			if (level == pm_test_level)
-				s += sprintf(s, "[%s] ", pm_tests[level]);
-			else
-				s += sprintf(s, "%s ", pm_tests[level]);
-		}
+	for (level = TEST_FIRST; level <= TEST_MAX; level++) {
+		if (!pm_tests[level])
+			continue;
+		len += sysfs_emit_at(buf, len,
+				     level == pm_test_level ? "[%s] " : "%s ",
+				     pm_tests[level]);
+	}
 
-	if (s != buf)
-		/* convert the last space to a newline */
-		*(s-1) = '\n';
+	/* convert the last space to a newline if needed */
+	if (len)
+		sysfs_emit_at(buf, len - 1, "\n");
 
-	return (s - buf);
+	return len;
 }
 
 static ssize_t pm_test_store(struct kobject *kobj, struct kobj_attribute *attr,
@@ -314,7 +312,7 @@ static char *suspend_step_name(enum suspend_stat_step step)
 static ssize_t _name##_show(struct kobject *kobj,		\
 		struct kobj_attribute *attr, char *buf)		\
 {								\
-	return sprintf(buf, "%d\n", suspend_stats._name);	\
+	return sysfs_emit(buf, "%d\n", suspend_stats._name);	\
 }								\
 static struct kobj_attribute _name = __ATTR_RO(_name)
 
@@ -339,7 +337,7 @@ static ssize_t last_failed_dev_show(struct kobject *kobj,
 	index %= REC_FAILED_NUM;
 	last_failed_dev = suspend_stats.failed_devs[index];
 
-	return sprintf(buf, "%s\n", last_failed_dev);
+	return sysfs_emit(buf, "%s\n", last_failed_dev);
 }
 static struct kobj_attribute last_failed_dev = __ATTR_RO(last_failed_dev);
 
@@ -353,7 +351,7 @@ static ssize_t last_failed_errno_show(struct kobject *kobj,
 	index %= REC_FAILED_NUM;
 	last_failed_errno = suspend_stats.errno[index];
 
-	return sprintf(buf, "%d\n", last_failed_errno);
+	return sysfs_emit(buf, "%d\n", last_failed_errno);
 }
 static struct kobj_attribute last_failed_errno = __ATTR_RO(last_failed_errno);
 
@@ -369,7 +367,7 @@ static ssize_t last_failed_step_show(struct kobject *kobj,
 	step = suspend_stats.failed_steps[index];
 	last_failed_step = suspend_step_name(step);
 
-	return sprintf(buf, "%s\n", last_failed_step);
+	return sysfs_emit(buf, "%s\n", last_failed_step);
 }
 static struct kobj_attribute last_failed_step = __ATTR_RO(last_failed_step);
 
@@ -477,7 +475,7 @@ bool pm_print_times_enabled;
 static ssize_t pm_print_times_show(struct kobject *kobj,
 				   struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%d\n", pm_print_times_enabled);
+	return sysfs_emit(buf, "%d\n", pm_print_times_enabled);
 }
 
 static ssize_t pm_print_times_store(struct kobject *kobj,
@@ -510,7 +508,7 @@ static ssize_t pm_wakeup_irq_show(struct kobject *kobj,
 	if (!pm_wakeup_irq())
 		return -ENODATA;
 
-	return sprintf(buf, "%u\n", pm_wakeup_irq());
+	return sysfs_emit(buf, "%u\n", pm_wakeup_irq());
 }
 
 power_attr_ro(pm_wakeup_irq);
@@ -520,7 +518,7 @@ bool pm_debug_messages_on __read_mostly;
 static ssize_t pm_debug_messages_show(struct kobject *kobj,
 				      struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%d\n", pm_debug_messages_on);
+	return sysfs_emit(buf, "%d\n", pm_debug_messages_on);
 }
 
 static ssize_t pm_debug_messages_store(struct kobject *kobj,
@@ -568,21 +566,24 @@ struct kobject *power_kobj;
 static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
 			  char *buf)
 {
-	char *s = buf;
+	int len = 0;
 #ifdef CONFIG_SUSPEND
 	suspend_state_t i;
 
-	for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++)
+	for (i = PM_SUSPEND_MIN; i < PM_SUSPEND_MAX; i++) {
 		if (pm_states[i])
-			s += sprintf(s,"%s ", pm_states[i]);
+			len += sysfs_emit_at(buf, len, "%s ", pm_states[i]);
+	}
 
 #endif
 	if (hibernation_available())
-		s += sprintf(s, "disk ");
-	if (s != buf)
-		/* convert the last space to a newline */
-		*(s-1) = '\n';
-	return (s - buf);
+		len += sysfs_emit_at(buf, len, "disk ");
+
+	/* convert the last space to a newline if needed */
+	if (len)
+		sysfs_emit_at(buf, len - 1, "\n");
+
+	return len;
 }
 
 static suspend_state_t decode_state(const char *buf, size_t n)
@@ -681,8 +682,10 @@ static ssize_t wakeup_count_show(struct kobject *kobj,
 {
 	unsigned int val;
 
-	return pm_get_wakeup_count(&val, true) ?
-		sprintf(buf, "%u\n", val) : -EINTR;
+	if (!pm_get_wakeup_count(&val, true))
+		return -EINTR;
+
+	return sysfs_emit(buf, "%u\n", val);
 }
 
 static ssize_t wakeup_count_store(struct kobject *kobj,
@@ -724,17 +727,16 @@ static ssize_t autosleep_show(struct kobject *kobj,
 	suspend_state_t state = pm_autosleep_state();
 
 	if (state == PM_SUSPEND_ON)
-		return sprintf(buf, "off\n");
+		return sysfs_emit(buf, "off\n");
 
 #ifdef CONFIG_SUSPEND
 	if (state < PM_SUSPEND_MAX)
-		return sprintf(buf, "%s\n", pm_states[state] ?
-					pm_states[state] : "error");
+		return sysfs_emit(buf, "%s\n", pm_states[state] ?: "error");
 #endif
 #ifdef CONFIG_HIBERNATION
-	return sprintf(buf, "disk\n");
+	return sysfs_emit(buf, "disk\n");
 #else
-	return sprintf(buf, "error");
+	return sysfs_emit(buf, "error\n");
 #endif
 }
 
@@ -803,7 +805,7 @@ int pm_trace_enabled;
 static ssize_t pm_trace_show(struct kobject *kobj, struct kobj_attribute *attr,
 			     char *buf)
 {
-	return sprintf(buf, "%d\n", pm_trace_enabled);
+	return sysfs_emit(buf, "%d\n", pm_trace_enabled);
 }
 
 static ssize_t
@@ -840,7 +842,7 @@ power_attr_ro(pm_trace_dev_match);
 static ssize_t pm_freeze_timeout_show(struct kobject *kobj,
 				      struct kobj_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%u\n", freeze_timeout_msecs);
+	return sysfs_emit(buf, "%u\n", freeze_timeout_msecs);
 }
 
 static ssize_t pm_freeze_timeout_store(struct kobject *kobj,

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

On Wed, Jun 29, 2022 at 06:44:14PM -0700, Joe Perches wrote:
> On Thu, 2022-06-30 at 02:50 +0200, Jason A. Donenfeld wrote:
> > On Wed, Jun 29, 2022 at 05:36:57PM -0700, Joe Perches wrote:
> > > > > +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);
> > > 
> > > This should use sysfs_emit no?
> > 
> > Probably, yea. Note that I just copy and pasted a nearby function,
> > pm_async_show, `:%s/`d the variable name, and then promptly `git diff |
> > clip`d it and plonked it into my email. Looking at the file, it uses
> > sprintf all over the place in this fashion. So you may want to submit a
> > cleanup to Rafael on this if you're right about sysfs_emit() being
> > universally preferred.
> 
> Perhaps:
> 
> (trivial refactored and added a missing newline in autosleep_show)
> 
> ---
>  kernel/power/main.c | 102 ++++++++++++++++++++++++++--------------------------
>  1 file changed, 52 insertions(+), 50 deletions(-)

You should probably post a proper patch to the PM people. At least I'm
not going to look at that here, as it's not really relevant at all to
this discussion.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[John Stultz]

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

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

Hi John,

On Wed, Jun 29, 2022 at 04:52:05PM -0700, John Stultz wrote:
> 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

I'd also be okay with a compile-time knob. The details make no
difference to me, so long as there's just *something* there.

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

I guess what I have in mind is the answer to these being "yes":
- "Is it very common to be asleep for only 2 seconds before being woken?"
- "Is it very common to be awake for only 2 seconds before sleeping?"

I think it'd be easiest to have a knob somewhere (compiletime,
runtime, wherever) that describes a device that exhibits those
properties. Then wireguard and other things will make a decision on how
to handle the crypto during relevant events.

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

As a general rule, I don't expose knobs like that in wireguard /itself/,
but wireguard has no problem with adapting to whatever machine properties
it finds itself on. And besides, this *is* a very definite device
property, something really particular and peculiar about the machine
the kernel is running on. It's a concrete thing that the kernel should
know about. So let's go with your "very clear description idea", above,
instead.

So taken together, I guess there are two approaches to this:

1) Introduce a simple CONFIG_PM_CONTINUOUS_AUTOSLEEPING Kconfig thing
   with lots of discouraging help text.

2) Go with the /sys/power tunable and bikeshed the naming of that a bit
   to get it to something that reflects this better, and document it as
   being undesirable except for Android phones.

It seems like in both cases, the key will be getting the naming right.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

Hey again,

On Thu, Jun 30, 2022 at 2:24 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> 1) Introduce a simple CONFIG_PM_CONTINUOUS_AUTOSLEEPING Kconfig thing
>    with lots of discouraging help text.
>
> 2) Go with the /sys/power tunable and bikeshed the naming of that a bit
>    to get it to something that reflects this better, and document it as
>    being undesirable except for Android phones.

One other quick thought, which I had mentioned earlier to Kalesh:

3) Make the semantics a process holding open a file descriptor, rather
   than writing 0/1 into a file. It'd be called /sys/power/
   userspace_autosleep_ctrl, or something, and it'd enable this behavior
   while it's opened. And maybe down the line somebody will want to add
   ioctls to it for a different purpose. This way it's less of a tunable
   and more of an indication that there's a userspace app doing/controlling
   something.

This idea (3) may be a lot of added complexity for basically nothing,
but it might fit the usage semantics concerns a bit better than (2). But
anyway, just an idea. Any one of those three are fine with me.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Kalesh Singh]

On Wed, Jun 29, 2022 at 5:30 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hey again,
>
> On Thu, Jun 30, 2022 at 2:24 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > 1) Introduce a simple CONFIG_PM_CONTINUOUS_AUTOSLEEPING Kconfig thing
> >    with lots of discouraging help text.
> >
> > 2) Go with the /sys/power tunable and bikeshed the naming of that a bit
> >    to get it to something that reflects this better, and document it as
> >    being undesirable except for Android phones.
>
> One other quick thought, which I had mentioned earlier to Kalesh:
>
> 3) Make the semantics a process holding open a file descriptor, rather
>    than writing 0/1 into a file. It'd be called /sys/power/
>    userspace_autosleep_ctrl, or something, and it'd enable this behavior
>    while it's opened. And maybe down the line somebody will want to add
>    ioctls to it for a different purpose. This way it's less of a tunable
>    and more of an indication that there's a userspace app doing/controlling
>    something.
>
> This idea (3) may be a lot of added complexity for basically nothing,
> but it might fit the usage semantics concerns a bit better than (2). But
> anyway, just an idea. Any one of those three are fine with me.

Two concerns John raised:
  1) Adding new ABI we need to maintain
  2) Having unclear config options

Another idea, I think, is to add the Kconfig option as
CONFIG_SUSPEND_SKIP_RNG_RESEED? Similar to existing
CONFIG_SUSPEND_SKIP_SYNC and I think it would address those concerns.

--Kalesh

> Jason

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

Hi Kalesh,

On Wed, Jun 29, 2022 at 09:25:32PM -0700, Kalesh Singh wrote:
> On Wed, Jun 29, 2022 at 5:30 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > Hey again,
> >
> > On Thu, Jun 30, 2022 at 2:24 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > > 1) Introduce a simple CONFIG_PM_CONTINUOUS_AUTOSLEEPING Kconfig thing
> > >    with lots of discouraging help text.
> > >
> > > 2) Go with the /sys/power tunable and bikeshed the naming of that a bit
> > >    to get it to something that reflects this better, and document it as
> > >    being undesirable except for Android phones.
> >
> > One other quick thought, which I had mentioned earlier to Kalesh:
> >
> > 3) Make the semantics a process holding open a file descriptor, rather
> >    than writing 0/1 into a file. It'd be called /sys/power/
> >    userspace_autosleep_ctrl, or something, and it'd enable this behavior
> >    while it's opened. And maybe down the line somebody will want to add
> >    ioctls to it for a different purpose. This way it's less of a tunable
> >    and more of an indication that there's a userspace app doing/controlling
> >    something.
> >
> > This idea (3) may be a lot of added complexity for basically nothing,
> > but it might fit the usage semantics concerns a bit better than (2). But
> > anyway, just an idea. Any one of those three are fine with me.
> 
> Two concerns John raised:
>   1) Adding new ABI we need to maintain
>   2) Having unclear config options
> 
> Another idea, I think, is to add the Kconfig option as
> CONFIG_SUSPEND_SKIP_RNG_RESEED? Similar to existing
> CONFIG_SUSPEND_SKIP_SYNC and I think it would address those concerns.

I mentioned in my reply to him that this doesn't really work for me:

| As a general rule, I don't expose knobs like that in wireguard /itself/,
| but wireguard has no problem with adapting to whatever machine properties
| it finds itself on. And besides, this *is* a very definite device
| property, something really particular and peculiar about the machine
| the kernel is running on. It's a concrete thing that the kernel should
| know about. So let's go with your "very clear description idea", above,
| instead.

IOW, we're not going to add a tunable on every possible place this is
used.

Anyway if you don't want a runtime switch, make a compiletime switch
called CONFIG_PM_CONTINUOUS_RAPID_AUTOSLEEPING or whatever, write some
very discouraging help text, and call it a day. And this way you don't
have to worry about ABI and we can change this later on and do the whole
thing as a no-big-deal change that somebody can tweak later without
issue.

The below diff is some boiler plate to help you get started with that
direction. Similar order of operations for this one:

1. You write a patch for Android's base config to enable this option 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 the kernel, including as a `Link:
   ...` this thread and the Gerrit link.

3. When the patch lands, you submit the Gerrit CL.

4. When both have landed, Christoph moves forward with his
   CONFIG_ANDROID removal.

So really, just pick an option here -- the runtime switch or the
compiletime switch or the crazy fd thing I mentioned -- and run with it.

Jason

diff --git a/drivers/char/random.c b/drivers/char/random.c
index e3dd1dd3dd22..5332236cb1ad 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) && !IS_ENABLED(CONFIG_PM_RAPID_USERSPACE_AUTOSLEEP)))) {
 		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..b93171f2e6c9 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) || IS_ENABLED(CONFIG_PM_RAPID_USERSPACE_AUTOSLEEP))
 		return 0;

 	if (action != PM_HIBERNATION_PREPARE && action != PM_SUSPEND_PREPARE)
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index a12779650f15..bcbfbeb39d4f 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -150,6 +150,25 @@ config PM_WAKELOCKS
 	Allow user space to create, activate and deactivate wakeup source
 	objects with the help of a sysfs-based interface.

+config PM_RAPID_USERSPACE_AUTOSLEEP
+	bool "Tune for rapid and consistent userspace calls to sleep"
+	depends on PM_SLEEP
+	help
+	Change the behavior of various sleep-sensitive code to deal with
+	userspace autosuspend daemons that put the machine to sleep and wake it
+	up extremely often and for short periods of time.
+
+	This option mostly disables code paths that most users really should
+	keep enabled. In particular, only enable this if:
+
+	- It is very common to be asleep for only 2 seconds before being woken;	and
+	- It is very common to be awake for only 2 seconds before sleeping.
+
+	This likely only applies to Android devices, and not other machines.
+	Therefore, you should say N here, unless you're extremely certain that
+	this is what you want. The option otherwise has bad, undesirable
+	effects, and should not be enabled just for fun.
+
 config PM_WAKELOCKS_LIMIT
 	int "Maximum number of user space wakeup sources (0 = no limit)"
 	range 0 100000

Re: [PATCH] remove CONFIG_ANDROID

[John Stultz]

On Thu, Jun 30, 2022 at 3:06 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Wed, Jun 29, 2022 at 09:25:32PM -0700, Kalesh Singh wrote:
> > Two concerns John raised:
> >   1) Adding new ABI we need to maintain
> >   2) Having unclear config options
> >
> > Another idea, I think, is to add the Kconfig option as
> > CONFIG_SUSPEND_SKIP_RNG_RESEED? Similar to existing
> > CONFIG_SUSPEND_SKIP_SYNC and I think it would address those concerns.
>
> I mentioned in my reply to him that this doesn't really work for me:
>
> | As a general rule, I don't expose knobs like that in wireguard /itself/,
> | but wireguard has no problem with adapting to whatever machine properties
> | it finds itself on. And besides, this *is* a very definite device
> | property, something really particular and peculiar about the machine
> | the kernel is running on. It's a concrete thing that the kernel should
> | know about. So let's go with your "very clear description idea", above,
> | instead.
>
> IOW, we're not going to add a tunable on every possible place this is
> used.

Hrm. Can you explain a bit more on why you're particularly adamant
against scoping the config to describe the behavior we want from the
kernel rather than describing a "machine property"?  As personally, I
greatly prefer Kalesh's suggestion (as it avoids the same critique one
could make of the CONFIG_ANDROID "flag"), but admittedly this is
bikeshed territory.

Does this preference come out of the too-many-options-in-gpg
antipattern? Or is there something else?


> Anyway if you don't want a runtime switch, make a compiletime switch
> called CONFIG_PM_CONTINUOUS_RAPID_AUTOSLEEPING or whatever, write some
> very discouraging help text, and call it a day. And this way you don't
> have to worry about ABI and we can change this later on and do the whole
> thing as a no-big-deal change that somebody can tweak later without
> issue.

Yeah, this is ok with me, as I don't see much benefit to creating a
userland ABI, as I don't think at this point we expect the behavior to
shift or oscillate at runtime.

thanks
-john

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

Hi John,

On Thu, Jun 30, 2022 at 10:12:30AM -0700, John Stultz wrote:
> Does this preference come out of the too-many-options-in-gpg
> antipattern? Or is there something else?

There are numerous presentations and threads galore on why WireGuard
doesn't do knobs. Not worth rehashing here; it's not a bikeshed I really
want to have yet again, and I'd appreciate you respecting my time by not
going down that route. Sorry.

> > Anyway if you don't want a runtime switch, make a compiletime switch
> > called CONFIG_PM_CONTINUOUS_RAPID_AUTOSLEEPING or whatever, write some
> > very discouraging help text, and call it a day. And this way you don't
> > have to worry about ABI and we can change this later on and do the whole
> > thing as a no-big-deal change that somebody can tweak later without
> > issue.
> 
> Yeah, this is ok with me, as I don't see much benefit to creating a
> userland ABI, as I don't think at this point we expect the behavior to
> shift or oscillate at runtime.

Okay, fine by me. You have my sample patch for this. Feel free to CC me
on Gerrit and on the cleaned up patch and I'll offer my acks there.

No need to keep this mega thread going longer here. I'll keep my eyes
open for Gerrit notifications and such though.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Jonathan Corbet]

"Jason A. Donenfeld" <Jason@zx2c4.com> writes:

> I guess what I have in mind is the answer to these being "yes":
> - "Is it very common to be asleep for only 2 seconds before being woken?"
> - "Is it very common to be awake for only 2 seconds before sleeping?"
>
> I think it'd be easiest to have a knob somewhere (compiletime,
> runtime, wherever) that describes a device that exhibits those
> properties. Then wireguard and other things will make a decision on how
> to handle the crypto during relevant events.

So please forgive the noise from the peanut gallery, but I do find
myself wondering...do you really need a knob for this?  The kernel
itself can observe how often (and for how long) the system is suspended,
and might well be able to do the right thing without explicit input from
user space.  If it works it would eliminate a potential configuration
problem and also perhaps respond correctly to changing workloads.

For example, rather than testing a knob, avoid resetting keys on resume
if the suspend time is less than (say) 30s?

Educate me on what I'm missing here, please :)

Thanks,

jon

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

Hi Jon,

On Fri, Jul 01, 2022 at 02:22:38PM -0600, Jonathan Corbet wrote:
> So please forgive the noise from the peanut gallery

Yuh oh, I sure hope this isn't newsworthy for LWN. This has already
consumed me for two days...

> myself wondering...do you really need a knob for this?  The kernel
> itself can observe how often (and for how long) the system is suspended,
> and might well be able to do the right thing without explicit input from
> user space.  If it works it would eliminate a potential configuration
> problem and also perhaps respond correctly to changing workloads.
> 
> For example, rather than testing a knob, avoid resetting keys on resume
> if the suspend time is less than (say) 30s?
> 
> Educate me on what I'm missing here, please :)

What you're missing is that wireguard needs to do this before going to
sleep, not when waking up, because one of the objectives is forward
secrecy. See
https://git.zx2c4.com/wireguard-linux/tree/drivers/net/wireguard/device.c#n63

	if (IS_ENABLED(CONFIG_PM_AUTOSLEEP) || IS_ENABLED(CONFIG_ANDROID))
		return 0;
	if (action != PM_HIBERNATION_PREPARE && action != PM_SUSPEND_PREPARE)
		return 0;
	[...]
	wg_noise_handshake_clear(&peer->handshake);
	wg_noise_keypairs_clear(&peer->keypairs);

Somebody asked the same question on wgml here -
https://lore.kernel.org/wireguard/CAHmME9p2OYSTX2C5M0faKtw2N8jiyohvRqnAPKa=e7BWynF7fQ@mail.gmail.com/T/
- and then eventually suggested that I should wake up computers from
sleep to clear that memory. No way jose.

Anyway, this matter has been resolved in this thread here:
https://lore.kernel.org/lkml/20220630191230.235306-1-kaleshsingh@google.com/T/
And this Android change:
https://android-review.googlesource.com/c/kernel/common/+/2142693/1
Resulting in these two commits landing in Greg's tree:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/?id=261e224d6a5c43e2bb8a07b7662f9b4ec425cfec
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/commit/?id=1045a06724f322ed61f1ffb994427c7bdbe64647
So hopefully this thread can come to an end and I can get back to work.

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Paul E. McKenney]

On Wed, Jun 29, 2022 at 06:15:27PM +0200, Christoph Hellwig wrote:
> On Wed, Jun 29, 2022 at 06:13:05PM +0200, Jason A. Donenfeld wrote:
> > Good! It sounds like you're starting to develop opinions on the matter.
> 
> No, I provide facts.  Look at both the definition of the symbol, and
> various distribution kernel that enabled it and think hard if they run
> on "Android" hardware.  Not just primarily, but at all.

So you are OK if your patch is accepted, and then CONFIG_ANDROID is
re-introduced but used only for building kernels intended to run on
Android systems?

Asking for a friend.  ;-)

							Thanx, Paul

Re: [PATCH] remove CONFIG_ANDROID

[Christoph Hellwig]

On Wed, Jun 29, 2022 at 09:34:44AM -0700, Paul E. McKenney wrote:
> So you are OK if your patch is accepted, and then CONFIG_ANDROID is
> re-introduced but used only for building kernels intended to run on
> Android systems?

I don't think that is a good config.  In general you want APIs
to express behavior, and in that case that is goes to sleep and
wakes all the time.  So we need go figure out what causes this in
Android systems - I don't think it can be the hardware, so it must
be a policy set somewhere either in the kernel or fed into the kernel
by userspace.  Then we can key it off that, and I suspect it is
probably going to be a runtime variable and not a config option.

Re: [PATCH] remove CONFIG_ANDROID

[Jason A. Donenfeld]

On Wed, Jun 29, 2022 at 06:37:01PM +0200, Christoph Hellwig wrote:
> be a policy set somewhere either in the kernel or fed into the kernel
> by userspace.  Then we can key it off that, and I suspect it is
> probably going to be a runtime variable and not a config option.

Right, this would be a good way of addressing it.

Maybe some Android people on the list have a good idea off hand of what
Android uses at runtime to control this, and how it'd be accessible in
the kernel?

Jason

Re: [PATCH] remove CONFIG_ANDROID

[Paul E. McKenney]

On Wed, Jun 29, 2022 at 06:45:48PM +0200, Jason A. Donenfeld wrote:
> On Wed, Jun 29, 2022 at 06:37:01PM +0200, Christoph Hellwig wrote:
> > be a policy set somewhere either in the kernel or fed into the kernel
> > by userspace.  Then we can key it off that, and I suspect it is
> > probably going to be a runtime variable and not a config option.
> 
> Right, this would be a good way of addressing it.
> 
> Maybe some Android people on the list have a good idea off hand of what
> Android uses at runtime to control this, and how it'd be accessible in
> the kernel?

In case it helps, in the case of CONFIG_RCU_EXP_CPU_STALL_TIMEOUT,
the Android guys can use things like defconfig at kernel-build time or
the rcu_exp_cpu_stall_timeout kernel-boot/sysfs parameter at boot time
or runtime.

							Thanx, Paul