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)) {
