Re: [PATCH] mmc: meson-gx: remove IRQF_ONESHOT

[Thomas Gleixner]

Brad,

On Wed, Oct 07 2020 at 00:45, Brad Harper wrote:
> I'm happy to test anything on a range of amlogic hardware with standard 
> / rt and  multiple mmc devices.  Ill test Jerome's patch in next 24 
> hours to report the results.

please do not top-post and trim your replies.

> On 6/10/2020 11:43 pm, Thomas Gleixner wrote:
>>       We rather should make interrupts which need to have their primary
>>       handler in hard interrupt context to set IRQF_NO_THREAD. That
>>       should at the same time confirm that the primary handler is RT
>>       safe.
>>
>>       Let me stare at the core code and the actual usage sites some more.

So there are a few nasties in there and I faintly remember that there
was an assumption that interrupts which are requested with both a
primary and a secondary handler should quiesce the device interrupt in
the primary handler if needed. OTOH, this also enforces that the primary
handler is RT safe, which is after a quick scan of all the usage sites
not a given and quite some of the users rely on IRQF_ONESHOT.

The below untested patch should cure the problem and keep the interrupt
line masked if requested with IRQF_ONESHOT.

Thanks,

        tglx
---
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -967,8 +967,7 @@ static int irq_wait_for_interrupt(struct
 static void irq_finalize_oneshot(struct irq_desc *desc,
 				 struct irqaction *action)
 {
-	if (!(desc->istate & IRQS_ONESHOT) ||
-	    action->handler == irq_forced_secondary_handler)
+	if (!(action->flags & IRQF_ONESHOT))
 		return;
 again:
 	chip_bus_lock(desc);
@@ -1073,10 +1072,6 @@ irq_forced_thread_fn(struct irq_desc *de
 
 	local_bh_disable();
 	ret = action->thread_fn(action->irq, action->dev_id);
-	if (ret == IRQ_HANDLED)
-		atomic_inc(&desc->threads_handled);
-
-	irq_finalize_oneshot(desc, action);
 	local_bh_enable();
 	return ret;
 }
@@ -1089,14 +1084,7 @@ irq_forced_thread_fn(struct irq_desc *de
 static irqreturn_t irq_thread_fn(struct irq_desc *desc,
 		struct irqaction *action)
 {
-	irqreturn_t ret;
-
-	ret = action->thread_fn(action->irq, action->dev_id);
-	if (ret == IRQ_HANDLED)
-		atomic_inc(&desc->threads_handled);
-
-	irq_finalize_oneshot(desc, action);
-	return ret;
+	return action->thread_fn(action->irq, action->dev_id);
 }
 
 static void wake_threads_waitq(struct irq_desc *desc)
@@ -1172,9 +1160,14 @@ static int irq_thread(void *data)
 		irq_thread_check_affinity(desc, action);
 
 		action_ret = handler_fn(desc, action);
+		if (action_ret == IRQ_HANDLED)
+			atomic_inc(&desc->threads_handled);
+
 		if (action_ret == IRQ_WAKE_THREAD)
 			irq_wake_secondary(desc, action);
 
+		irq_finalize_oneshot(desc, action);
+
 		wake_threads_waitq(desc);
 	}
 
@@ -1219,7 +1212,7 @@ static int irq_setup_forced_threading(st
 {
 	if (!force_irqthreads)
 		return 0;
-	if (new->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT))
+	if (new->flags & (IRQF_NO_THREAD | IRQF_PERCPU))
 		return 0;
 
 	/*
@@ -1229,8 +1222,6 @@ static int irq_setup_forced_threading(st
 	if (new->handler == irq_default_primary_handler)
 		return 0;
 
-	new->flags |= IRQF_ONESHOT;
-
 	/*
 	 * Handle the case where we have a real primary handler and a
 	 * thread handler. We force thread them as well by creating a
@@ -1246,8 +1237,11 @@ static int irq_setup_forced_threading(st
 		new->secondary->dev_id = new->dev_id;
 		new->secondary->irq = new->irq;
 		new->secondary->name = new->name;
+		/* Preserve the requested IRQF_ONESHOT */
+		new->secondary->flags = new->flags & IRQF_ONESHOT;
 	}
 	/* Deal with the primary handler */
+	new->flags |= IRQF_ONESHOT;
 	set_bit(IRQTF_FORCED_THREAD, &new->thread_flags);
 	new->thread_fn = new->handler;
 	new->handler = irq_default_primary_handler;
@@ -1341,6 +1335,38 @@ setup_irq_thread(struct irqaction *new,
 	return 0;
 }
 
+static unsigned long thread_mask_alloc(unsigned long thread_mask)
+{
+	/*
+	 * Unlikely to have 32 resp 64 irqs sharing one line,
+	 * but who knows.
+	 */
+	if (thread_mask == ~0UL)
+		return 0;
+
+	/*
+	 * The thread_mask for the action is or'ed to
+	 * desc->thread_active to indicate that the
+	 * IRQF_ONESHOT thread handler has been woken, but not
+	 * yet finished. The bit is cleared when a thread
+	 * completes. When all threads of a shared interrupt
+	 * line have completed desc->threads_active becomes
+	 * zero and the interrupt line is unmasked. See
+	 * handle.c:irq_wake_thread() for further information.
+	 *
+	 * If no thread is woken by primary (hard irq context)
+	 * interrupt handlers, then desc->threads_active is
+	 * also checked for zero to unmask the irq line in the
+	 * affected hard irq flow handlers
+	 * (handle_[fasteoi|level]_irq).
+	 *
+	 * The new action gets the first zero bit of
+	 * thread_mask assigned. See the loop above which or's
+	 * all existing action->thread_mask bits.
+	 */
+	return 1UL << ffz(thread_mask);
+}
+
 /*
  * Internal function to register an irqaction - typically used to
  * allocate special interrupts that are part of the architecture.
@@ -1525,35 +1551,18 @@ static int
 	 * conditional in irq_wake_thread().
 	 */
 	if (new->flags & IRQF_ONESHOT) {
-		/*
-		 * Unlikely to have 32 resp 64 irqs sharing one line,
-		 * but who knows.
-		 */
-		if (thread_mask == ~0UL) {
-			ret = -EBUSY;
+		ret = -EBUSY;
+		new->thread_mask = thread_mask_alloc(thread_mask);
+		if (!new->thread_mask)
 			goto out_unlock;
+
+		if (new->secondary && (new->secondary->flags & IRQF_ONESHOT)) {
+			thread_mask |= new->thread_mask;
+			new->secondary->thread_mask =
+				thread_mask_alloc(thread_mask);
+			if (!new->secondary->thread_mask)
+				goto out_unlock;
 		}
-		/*
-		 * The thread_mask for the action is or'ed to
-		 * desc->thread_active to indicate that the
-		 * IRQF_ONESHOT thread handler has been woken, but not
-		 * yet finished. The bit is cleared when a thread
-		 * completes. When all threads of a shared interrupt
-		 * line have completed desc->threads_active becomes
-		 * zero and the interrupt line is unmasked. See
-		 * handle.c:irq_wake_thread() for further information.
-		 *
-		 * If no thread is woken by primary (hard irq context)
-		 * interrupt handlers, then desc->threads_active is
-		 * also checked for zero to unmask the irq line in the
-		 * affected hard irq flow handlers
-		 * (handle_[fasteoi|level]_irq).
-		 *
-		 * The new action gets the first zero bit of
-		 * thread_mask assigned. See the loop above which or's
-		 * all existing action->thread_mask bits.
-		 */
-		new->thread_mask = 1UL << ffz(thread_mask);
 
 	} else if (new->handler == irq_default_primary_handler &&
 		   !(desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE)) {