Hi, On Tue, Jul 11, 2017 at 11:16:03AM -0700, Linus Torvalds wrote: > On Tue, Jul 11, 2017 at 10:52 AM, Thomas Gleixner <t...@linutronix.de> 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().
So /me will skip testing Thomas' patch for now. > 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. This fixes boot on Droid 4: Tested-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk> -- Sebastian > 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);
signature.asc
Description: PGP signature