| Message ID | 20220629150102.1582425-2-hch@lst.de |
|---|---|
| State | Accepted |
| Commit | 1045a06724f322ed61f1ffb994427c7bdbe64647 |
| Headers | show |
| Series | remove CONFIG_ANDROID | expand |
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 >
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.
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.
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.
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.
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.
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
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
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
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
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
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
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(-)