| Message ID | 20220629150102.1582425-2-hch@lst.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | remove CONFIG_ANDROID | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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
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 >
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
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: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
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: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
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 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
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: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
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 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
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
[ 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
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 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
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
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 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
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
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
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?
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
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
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 >
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
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
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
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,
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, >
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
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
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
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?
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
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,
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
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
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
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
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
"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
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
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(-)