Received-SPF: pass (google.com: domain of linux-kernel+bounces-108299-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Wed, 20 Mar 2024 02:09:21 +0100
From: "Jason A. Donenfeld" <Jason@zx2c4.com>
To: Guoyong Wang <guoyong.wang@mediatek.com>
Cc: Theodore Ts'o <tytso@mit.edu>, Tejun Heo <tj@kernel.org>,
	Lai Jiangshan <jiangshanlai@gmail.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>,
	linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, wsd_upstream@mediatek.com
Subject: Re: [PATCH] random: Fix the issue of '_might_sleep' function running
 in an atomic contex
Message-ID: <Zfo3QUFEy4wHkLEB@zx2c4.com>
References: <Zfidal8CEZStp3R7@zx2c4.com>
 <20240319093055.3252-1-guoyong.wang@mediatek.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240319093055.3252-1-guoyong.wang@mediatek.com>

On Tue, Mar 19, 2024 at 05:30:55PM +0800, Guoyong Wang wrote:
> On Mon, 18 Mar 2024 21:00:42 +0100, Jason A. Donenfeld wrote:
> > I'm wondering, though, rather than introducing a second function, maybe
> > execute_in_process_context() should just gain a `&& !in_atomic()`.
> > That'd make things a bit simpler.  
> 
> > However, I'm pretty sure in_atomic() isn't actually a reliable way of
> > determining that, depending on config. So maybe this should just call
> > the worker always (if system_wq isn't null).
> 
> > Alternatively, any chance the call to add_input_randomness() could be
> > moved outside the spinlock, or does this not look possible?
> 
> Hi Jason,
> 
> Thanks for your suggestions. 
> 
> I am inclined to accept your second suggestion. My reluctance to accept 
> the first is due to the concern that "&& !in_atomic()" could potentially 
> alter the original meaning of the 'execute_in_process_context' interface. 
> Regarding the third suggestion, modifying the logic associated with 'input' 
> is not recommended.

Doesn't something like the below seem simplest? Just move the call out
of the spinlock and we're done.

diff --git a/drivers/input/input-core-private.h b/drivers/input/input-core-private.h
index 116834cf8868..717f239e28d0 100644
--- a/drivers/input/input-core-private.h
+++ b/drivers/input/input-core-private.h
@@ -10,7 +10,7 @@
 struct input_dev;

 void input_mt_release_slots(struct input_dev *dev);
-void input_handle_event(struct input_dev *dev,
+bool input_handle_event(struct input_dev *dev,
 			unsigned int type, unsigned int code, int value);

 #endif /* _INPUT_CORE_PRIVATE_H */
diff --git a/drivers/input/input.c b/drivers/input/input.c
index f71ea4fb173f..2faf46218c66 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -391,20 +391,22 @@ static void input_event_dispose(struct input_dev *dev, int disposition,
 	}
 }

-void input_handle_event(struct input_dev *dev,
+bool input_handle_event(struct input_dev *dev,
 			unsigned int type, unsigned int code, int value)
 {
 	int disposition;
+	bool should_contribute_to_rng = false;

 	lockdep_assert_held(&dev->event_lock);

 	disposition = input_get_disposition(dev, type, code, &value);
 	if (disposition != INPUT_IGNORE_EVENT) {
 		if (type != EV_SYN)
-			add_input_randomness(type, code, value);
+			should_contribute_to_rng = true;

 		input_event_dispose(dev, disposition, type, code, value);
 	}
+	return should_contribute_to_rng;
 }

 /**
@@ -428,12 +430,15 @@ void input_event(struct input_dev *dev,
 		 unsigned int type, unsigned int code, int value)
 {
 	unsigned long flags;
+	bool should_contribute_to_rng;

 	if (is_event_supported(type, dev->evbit, EV_MAX)) {

 		spin_lock_irqsave(&dev->event_lock, flags);
-		input_handle_event(dev, type, code, value);
+		should_contribute_to_rng = input_handle_event(dev, type, code, value);
 		spin_unlock_irqrestore(&dev->event_lock, flags);
+		if (should_contribute_to_rng)
+			add_input_randomness(type, code, value);
 	}
 }
 EXPORT_SYMBOL(input_event);