Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 29 Jun 2022 18:09:18 +0200
From:   "Jason A. Donenfeld" <Jason@zx2c4.com>
To:     Christoph Hellwig <hch@lst.de>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Arve =?utf-8?B?SGrDuG5uZXbDpWc=?= <arve@android.com>,
        Todd Kjos <tkjos@android.com>,
        Martijn Coenen <maco@android.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Christian Brauner <brauner@kernel.org>,
        Hridya Valsaraju <hridya@google.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Theodore Ts'o <tytso@mit.edu>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        "Alex Xu (Hello71)" <alex_y_xu@yahoo.ca>,
        Paolo Abeni <pabeni@redhat.com>, Rob Herring <robh@kernel.org>,
        "Paul E. McKenney" <paulmck@kernel.org>,
        Frederic Weisbecker <frederic@kernel.org>,
        Neeraj Upadhyay <quic_neeraju@quicinc.com>,
        Josh Triplett <josh@joshtriplett.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Shuah Khan <shuah@kernel.org>, linux-kernel@vger.kernel.org,
        wireguard@lists.zx2c4.com, netdev@vger.kernel.org,
        rcu@vger.kernel.org, linux-kselftest@vger.kernel.org
Subject: Re: [PATCH] remove CONFIG_ANDROID
Message-ID: <Yrx5Lt7jrk5BiHXx@zx2c4.com>
References: <20220629150102.1582425-1-hch@lst.de>
 <20220629150102.1582425-2-hch@lst.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20220629150102.1582425-2-hch@lst.de>
Precedence: bulk

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