Re: [Question] iforce_serio lockup

From: Tetsuo Handa
Date: Sun Aug 28 2022 - 02:10:57 EST


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@xxxxxxxxx>:
>
>> 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