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)))) {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=36f81c83674e0fd7c18e... https://git.zx2c4.com/wireguard-linux-compat/commit/?id=a89d53098dbde43f56e4...
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