Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 19d39a38 authored by Thomas Gleixner's avatar Thomas Gleixner
Browse files

genirq: Keep chip buslock across irq_request/release_resources() 



Moving the irq_request/release_resources() callbacks out of the spinlocked,
irq disabled and bus locked region, unearthed an interesting abuse of the
irq_bus_lock/irq_bus_sync_unlock() callbacks.

The OMAP GPIO driver does merily power management inside of them. The
irq_request_resources() callback of this GPIO irqchip calls a function
which reads a GPIO register. That read aborts now because the clock of the
GPIO block is not magically enabled via the irq_bus_lock() callback.

Move the callbacks under the bus lock again to prevent this. In the
free_irq() path this requires to drop the bus_lock before calling
synchronize_irq() and reaquiring it before calling the
irq_release_resources() callback.

The bus lock can't be held because:

   1) The data which has been changed between bus_lock/un_lock is cached in
      the irq chip driver private data and needs to go out to the irq chip
      via the slow bus (usually SPI or I2C) before calling
      synchronize_irq().

      That's the reason why this bus_lock/unlock magic exists in the first
      place, as you cannot do SPI/I2C transactions while holding desc->lock
      with interrupts disabled.

   2) synchronize_irq() will actually deadlock, if there is a handler on
      flight. These chips use threaded handlers for obvious reasons, as
      they allow to do SPI/I2C communication. When the threaded handler
      returns then bus_lock needs to be taken in irq_finalize_oneshot() as
      we need to talk to the actual irq chip once more. After that the
      threaded handler is marked done, which makes synchronize_irq() return.

      So if we hold bus_lock accross the synchronize_irq() call, the
      handler cannot mark itself done because it blocks on the bus
      lock. That in turn makes synchronize_irq() wait forever on the
      threaded handler to complete....

Add the missing unlock of desc->request_mutex in the error path of
__free_irq() and add a bunch of comments to explain the locking and
protection rules.

Fixes: 46e48e25 ("genirq: Move irq resource handling out of spinlocked region")
Reported-and-tested-by: default avatarSebastian Reichel <sebastian.reichel@collabora.co.uk>
Reported-and-tested-by: default avatarTony Lindgren <tony@atomide.com>
Reported-by: default avatarPavel Machek <pavel@ucw.cz>
Signed-off-by: default avatarThomas Gleixner <tglx@linutronix.de>
Not-longer-ranted-at-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
parent c5c601c4

kernel/irq/manage.c

+53 −10
Original line number Diff line number Diff line
@@ -1090,6 +1090,16 @@ setup_irq_thread(struct irqaction *new, unsigned int irq, bool secondary) 
/*

 * 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 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) 
	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,11 +1311,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new) 
			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 | \

				  IRQS_ONESHOT | IRQS_WAITING);
@@ -1385,12 +1408,10 @@ __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_bus_unlock:

	chip_bus_sync_unlock(desc);

	mutex_unlock(&desc->request_mutex);



out_thread:
@@ -1472,6 +1493,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,6 +1520,20 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id) 
#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(unsigned int irq, void *dev_id) 
		}

	}



	/* 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);

	}