Hi, On Tue, Jul 11, 2017 at 11:41:52PM +0200, Thomas Gleixner wrote: > [...] > > Here is a revised version of the previous patch with the conditional > locking removed and a bunch of comments added.
That one also fixes Droid 4 boot.
Tested-by: Sebastian Reichel <sebastian.reic...@collabora.co.uk>
-- Sebastian
> 8<----------------------------
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1090,6 +1090,16 @@ setup_irq_thread(struct irqaction *new,
> /*
> * Internal function to register an irqaction - typically used to
> * allocate special interrupts that are part of the architecture.
> + *
> + * Locking rules:
> + *
> + * desc->request_mutex Provides serialization against a concurrent
> free_irq()
> + * chip_bus_lock Provides serialization for slow bus operations
> + * desc->lock Provides serialization against hard interrupts
> + *
> + * chip_bus_lock and desc->lock are sufficient for all other management and
> + * interrupt related functions. desc->request_mutex solely serializes
> + * request/free_irq().
> */
> static int
> __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
> @@ -1167,20 +1177,35 @@ static int
> if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)
> new->flags &= ~IRQF_ONESHOT;
>
> + /*
> + * Protects against a concurrent __free_irq() call which might wait
> + * for synchronize_irq() to complete without holding the optional
> + * chip bus lock and desc->lock.
> + */
> mutex_lock(&desc->request_mutex);
> +
> + /*
> + * Acquire bus lock as the irq_request_resources() callback below
> + * might rely on the serialization or the magic power management
> + * functions which are abusing the irq_bus_lock() callback,
> + */
> + chip_bus_lock(desc);
> +
> + /* First installed action requests resources. */
> 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_bus_unlock;
> }
> }
>
> - chip_bus_lock(desc);
> -
> /*
> * The following block of code has to be executed atomically
> + * protected against a concurrent interrupt and any of the other
> + * management calls which are not serialized via
> + * desc->request_mutex or the optional bus lock.
> */
> raw_spin_lock_irqsave(&desc->lock, flags);
> old_ptr = &desc->action;
> @@ -1286,10 +1311,8 @@ static int
> ret = __irq_set_trigger(desc,
> new->flags & IRQF_TRIGGER_MASK);
>
> - if (ret) {
> - irq_release_resources(desc);
> + if (ret)
> goto out_unlock;
> - }
> }
>
> desc->istate &= ~(IRQS_AUTODETECT | IRQS_SPURIOUS_DISABLED | \
> @@ -1385,12 +1408,10 @@ static int
> out_unlock:
> raw_spin_unlock_irqrestore(&desc->lock, flags);
>
> - chip_bus_sync_unlock(desc);
> -
> if (!desc->action)
> irq_release_resources(desc);
> -
> -out_mutex:
> +out_bus_unlock:
> + chip_bus_sync_unlock(desc);
> mutex_unlock(&desc->request_mutex);
>
> out_thread:
> @@ -1472,6 +1493,7 @@ static struct irqaction *__free_irq(unsi
> 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,6 +1520,20 @@ static struct irqaction *__free_irq(unsi
> #endif
>
> raw_spin_unlock_irqrestore(&desc->lock, flags);
> + /*
> + * Drop bus_lock here so the changes which were done in the chip
> + * callbacks above are synced out to the irq chips which hang
> + * behind a slow bus (I2C, SPI) before calling synchronize_irq().
> + *
> + * Aside of that the bus_lock can also be taken from the threaded
> + * handler in irq_finalize_oneshot() which results in a deadlock
> + * because synchronize_irq() would wait forever for the thread to
> + * complete, which is blocked on the bus lock.
> + *
> + * The still held desc->request_mutex() protects against a
> + * concurrent request_irq() of this irq so the release of resources
> + * and timing data is properly serialized.
> + */
> chip_bus_sync_unlock(desc);
>
> unregister_handler_proc(irq, action);
> @@ -1530,8 +1566,15 @@ static struct irqaction *__free_irq(unsi
> }
> }
>
> + /* Last action releases resources */
> if (!desc->action) {
> + /*
> + * Reaquire bus lock as irq_release_resources() might
> + * require it to deallocate resources over the slow bus.
> + */
> + chip_bus_lock(desc);
> irq_release_resources(desc);
> + chip_bus_sync_unlock(desc);
> irq_remove_timings(desc);
> }
>
signature.asc
Description: PGP signature
