From: Thomas Gleixner <tglx@linutronix.de>
To: Brad Harper <bjharper@gmail.com>, Ulf Hansson <ulf.hansson@linaro.org>
Cc: Jerome Brunet <jbrunet@baylibre.com>,
Kevin Hilman <khilman@baylibre.com>,
"open list\:ARM\/Amlogic Meson..."
<linux-amlogic@lists.infradead.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
"linux-mmc\@vger.kernel.org" <linux-mmc@vger.kernel.org>,
Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
Marc Zyngier <maz@kernel.org>
Subject: Re: [PATCH] mmc: meson-gx: remove IRQF_ONESHOT
Date: Tue, 06 Oct 2020 17:33:34 +0200 [thread overview]
Message-ID: <87o8lf74j5.fsf@nanos.tec.linutronix.de> (raw)
In-Reply-To: <e4e6cc45-bc18-40ec-035e-fdb45b9a8f46@gmail.com>
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)) {
next prev parent reply other threads:[~2020-10-06 15:33 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-02 16:49 [PATCH] mmc: meson-gx: remove IRQF_ONESHOT Jerome Brunet
2020-10-05 8:22 ` Ulf Hansson
2020-10-05 8:55 ` Thomas Gleixner
2020-10-05 12:31 ` Jerome Brunet
2020-10-06 12:43 ` Thomas Gleixner
2020-10-06 13:45 ` Brad Harper
2020-10-06 15:33 ` Thomas Gleixner [this message]
2020-10-07 11:32 ` Jerome Brunet
2020-10-08 5:11 ` Brad Harper
2020-10-08 9:08 ` Ulf Hansson
2020-11-10 15:04 ` Jerome Brunet
2020-11-11 10:47 ` Ulf Hansson
2020-11-13 15:25 ` Jerome Brunet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87o8lf74j5.fsf@nanos.tec.linutronix.de \
--to=tglx@linutronix.de \
--cc=bigeasy@linutronix.de \
--cc=bjharper@gmail.com \
--cc=jbrunet@baylibre.com \
--cc=khilman@baylibre.com \
--cc=linux-amlogic@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=maz@kernel.org \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).