Re: [Question] iforce_serio lockup

[Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>]

On 2022/07/30 4:24, Greg Tulli wrote:
> I was advised to use a vanilla kernel with CONFIG_PROVE_LOCKING and
> decode the symbols in the report. So I followed
> https://kernelnewbies.org/OutreachyfirstpatchSetup and here's the
> result. Thanks, Tetsuo Handa.
> 
> WARNING: possible circular locking dependency detected
> 5.19.0-rc6iforce-00194-gf3a76018dd55 #2 Not tainted

OK. You confirmed that this is a real deadlock.

> 2022-07-11 11:32 GMT+02:00, Greg T <greg.iforce@gmail.com>:
> 
>> That problem vanishes if we don't call iforce_process_packet directly
>> from iforce_serio_irq, but from a tasklet. Is that a right approach?

Does untested diff shown below solve the problem?
I don't know about flush ordering in iforce_serio_disconnect().

 drivers/input/joystick/iforce/iforce-serio.c | 51 +++++++++++++++++++-
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/input/joystick/iforce/iforce-serio.c b/drivers/input/joystick/iforce/iforce-serio.c
index f95a81b9fac7..5ff650ce05ab 100644
--- a/drivers/input/joystick/iforce/iforce-serio.c
+++ b/drivers/input/joystick/iforce/iforce-serio.c
@@ -124,8 +124,7 @@ static void iforce_serio_write_wakeup(struct serio *serio)
 	iforce_serio_xmit(iforce);
 }
 
-static irqreturn_t iforce_serio_irq(struct serio *serio,
-				    unsigned char data, unsigned int flags)
+static void do_iforce_serio(struct serio *serio, unsigned char data)
 {
 	struct iforce_serio *iforce_serio = serio_get_drvdata(serio);
 	struct iforce *iforce = &iforce_serio->iforce;
@@ -183,6 +182,53 @@ static irqreturn_t iforce_serio_irq(struct serio *serio,
 		iforce_serio->csum = 0;
 	}
 out:
+	return;
+}
+
+struct iforce_serio_irq {
+	struct list_head list;
+	struct serio *serio;
+	unsigned char data;
+};
+static LIST_HEAD(iforce_serio_irq_list);
+static DEFINE_SPINLOCK(iforce_serio_irq_list_lock);
+
+static void iforce_serio_workfn(struct work_struct *work)
+{
+	static DEFINE_MUTEX(lock);
+	struct iforce_serio_irq *ev;
+
+	mutex_lock(&lock);
+	spin_lock_irq(&iforce_serio_irq_list_lock);
+	while (!list_empty(&iforce_serio_irq_list)) {
+		ev = list_first_entry(&iforce_serio_irq_list, struct iforce_serio_irq, list);
+		list_del(&ev->list);
+		spin_unlock_irq(&iforce_serio_irq_list_lock);
+		do_iforce_serio(ev->serio, ev->data);
+		kfree(ev);
+		spin_lock_irq(&iforce_serio_irq_list_lock);
+	}
+	spin_unlock_irq(&iforce_serio_irq_list_lock);
+	mutex_unlock(&lock);
+}
+static DECLARE_WORK(iforce_serio_work, iforce_serio_workfn);
+
+static irqreturn_t iforce_serio_irq(struct serio *serio,
+				    unsigned char data, unsigned int flags)
+{
+	bool was_empty;
+	struct iforce_serio_irq *ev = kmalloc(sizeof(*ev), GFP_ATOMIC | __GFP_NOWARN);
+
+	if (ev) {
+		ev->serio = serio;
+		ev->data = data;
+		spin_lock(&iforce_serio_irq_list_lock);
+		was_empty = list_empty(&iforce_serio_irq_list);
+		list_add_tail(&ev->list, &iforce_serio_irq_list);
+		spin_unlock(&iforce_serio_irq_list_lock);
+		if (was_empty)
+			queue_work(system_highpri_wq, &iforce_serio_work);
+	}
 	return IRQ_HANDLED;
 }
 
@@ -220,6 +266,7 @@ static void iforce_serio_disconnect(struct serio *serio)
 {
 	struct iforce_serio *iforce_serio = serio_get_drvdata(serio);
 
+	flush_work(&iforce_serio_work);
 	input_unregister_device(iforce_serio->iforce.dev);
 	serio_close(serio);
 	serio_set_drvdata(serio, NULL);
-- 
2.18.4