Re: [GIT pull] irq updates for 4.13

[Linus Torvalds]

On Tue, Jul 11, 2017 at 10:52 AM, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>
> Not completely, because of the free path issues. See the other mail. Tony
> confirmed that it works. I wait for Sebastian and queue it with a proper
> changelog, ok?

Ugh, I absolutely detest your ugly "bool buslock" parameter to
irq_release_resources().

And there seems to be no reason for it.

Why don't you just move the

        chip_bus_sync_unlock(desc);

call in __free_irq() down to just before you release the request_mutex?

In fact, looking at __free_irq(), I note that it's locking is
completely broken shit. Look at the

                if (!action) {
                        WARN(1, "Trying to free already-free IRQ %d\n", irq);

error case, and look for where it unlocks request_mutex. Yeah, it doesn't.

So honestly, I think this code is broken, and it's broken partly
because it has some really bad locking and logic rules.

Why not fix those stupid bugs and clean things up at the same time?
Make the rule be that as you take the request_mutex lock, you then
also do the chip_bus_lock().

And when you release the request_mutex lock, you do
chip_bus_sync_unlock() just before.

And no, I have no idea what the locking rules are for
irq_finalize_oneshot() - it does that chip_bus_lock() without having
any external serialization. Is that ok? Are the chip handlers able to
deal with that? Same seems to go for free_percpu_irq().

Anyway, patch attached (AGAIN, TOTALLY UNTESTED) showing what I mean,
and fixing (well, modulo any bugs I introduced by my untested sh*t)
that definite bug in lack of unlocking.

But that "bool buslock" thing really is too disgusting. Conditional
locking should not be done. It's a sign of serious problems, imnsho.

Comments? Even if they are "Linus, you're way out of line, and you
can't just move that chip_bus_sync_unlock() down like that because of
XYZ, you moron".

For example, it's entirely possible that we can't do the
"synchronize_irq()" waiting while we hold that chip_bus_lock().  But
the ones I looked at seemed to all take sleeping locks (or no locks at
all - doing other things), which implies that they certainly can't be
blocking irq delivery.

So I'm *not* claiming that the attached patch is necessarily right. I
just really don't like your conditional lock thing, and this would
seem to possibly be a clean way around it if it works.

                    Linus
 kernel/irq/manage.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 5624b2dd6b58..c4cbda784ea5 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1168,17 +1168,17 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		new->flags &= ~IRQF_ONESHOT;
 
 	mutex_lock(&desc->request_mutex);
+	chip_bus_lock(desc);
+
 	if (!desc->action) {
 		ret = irq_request_resources(desc);
 		if (ret) {
 			pr_err("Failed to request resources for %s (irq %d) on irqchip %s\n",
 			       new->name, irq, desc->irq_data.chip->name);
-			goto out_mutex;
+			goto out_unlock_chip_bus;
 		}
 	}
 
-	chip_bus_lock(desc);
-
 	/*
 	 * The following block of code has to be executed atomically
 	 */
@@ -1385,12 +1385,11 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 out_unlock:
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
 
-	chip_bus_sync_unlock(desc);
-
 	if (!desc->action)
 		irq_release_resources(desc);
 
-out_mutex:
+out_unlock_chip_bus:
+	chip_bus_sync_unlock(desc);
 	mutex_unlock(&desc->request_mutex);
 
 out_thread:
@@ -1472,6 +1471,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 			WARN(1, "Trying to free already-free IRQ %d\n", irq);
 			raw_spin_unlock_irqrestore(&desc->lock, flags);
 			chip_bus_sync_unlock(desc);
+			mutex_unlock(&desc->request_mutex);
 			return NULL;
 		}
 
@@ -1498,7 +1498,6 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 #endif
 
 	raw_spin_unlock_irqrestore(&desc->lock, flags);
-	chip_bus_sync_unlock(desc);
 
 	unregister_handler_proc(irq, action);
 
@@ -1535,6 +1534,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 		irq_remove_timings(desc);
 	}
 
+	chip_bus_sync_unlock(desc);
 	mutex_unlock(&desc->request_mutex);
 
 	irq_chip_pm_put(&desc->irq_data);