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;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Marc Zyngier <maz@kernel.org>, Thomas Pfaff <tpfaff@pcs.com>
Cc:     linux-kernel@vger.kernel.org, linux-rt-users@vger.kernel.org,
        Hillf Danton <hdanton@sina.com>
Subject: Re: [PATCH v2] irq/core: synchronize irq_thread startup
In-Reply-To: <87sfpv98j0.wl-maz@kernel.org>
References: <1e3f96b7-9294-1534-e83b-efe3602f876f@pcs.com>
 <87sfpv98j0.wl-maz@kernel.org>
Date:   Fri, 29 Apr 2022 21:40:32 +0200
Message-ID: <87fslvoez3.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Fri, Apr 29 2022 at 17:08, Marc Zyngier wrote:
> On Fri, 29 Apr 2022 12:52:48 +0100,
> Thomas Pfaff <tpfaff@pcs.com> wrote:

 > +static void wait_for_irq_thread_startup(struct irq_desc *desc,
 > +		struct irqaction *action)

 and this would be wait_for_irq_thread_ready().

which is sill a misnomer as this actually wakes and waits.

>> @@ -1522,6 +1548,8 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>>  		}
>>  	}
>>  
>> +	init_waitqueue_head(&desc->wait_for_threads);
>> +
>
> I'm trying to convince myself that this one is safe.
>
> It was so far only done when registering the first handler of a
> threaded interrupt, while it is now done on every call to
> __setup_irq().  However, this is now done outside of the protection of
> any of the locks, meaning that a concurrent __setup_irq() for a shared
> interrupt can now barge in and corrupt the wait queue.
>
> So I don't think this is right. You may be able to hoist the
> request_lock up, but I haven't checked what could break, if anything.

It can't be moved here, but I can see why Thomas wants to move it. With
a spurious wakeup of the irq thread (should not happen), the thread
would try to invoke wake_up() on a non initialize wait queue head.

Something like this should do the trick.

diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 939d21cd55c3..0099b87dd853 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -407,6 +407,7 @@ static struct irq_desc *alloc_desc(int irq, int node, unsigned int flags,
 	lockdep_set_class(&desc->lock, &irq_desc_lock_class);
 	mutex_init(&desc->request_mutex);
 	init_rcu_head(&desc->rcu);
+	init_waitqueue_head(&desc->wait_for_threads);
 
 	desc_set_defaults(irq, desc, node, affinity, owner);
 	irqd_set(&desc->irq_data, flags);
@@ -575,6 +576,7 @@ int __init early_irq_init(void)
 		raw_spin_lock_init(&desc[i].lock);
 		lockdep_set_class(&desc[i].lock, &irq_desc_lock_class);
 		mutex_init(&desc[i].request_mutex);
+		init_waitqueue_head(&desc[i].wait_for_threads);
 		desc_set_defaults(i, &desc[i], node, NULL, NULL);
 	}
 	return arch_early_irq_init();
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index c03f71d5ec10..6a0942f4d068 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1683,8 +1683,6 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	}
 
 	if (!shared) {
-		init_waitqueue_head(&desc->wait_for_threads);
-
 		/* Setup the type (level, edge polarity) if configured: */
 		if (new->flags & IRQF_TRIGGER_MASK) {
 			ret = __irq_set_trigger(desc,

Thanks,

        tglx