All of lore.kernel.org
 help / color / mirror / Atom feed
From: jamie@shareable.org (Jamie Lokier)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq
Date: Sun, 29 Nov 2009 15:18:40 +0000	[thread overview]
Message-ID: <20091129151840.GA30813@shareable.org> (raw)
In-Reply-To: <20091129102635.GA22653@pengutronix.de>

Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Sun, Nov 29, 2009 at 02:31:18AM +0000, Jamie Lokier wrote:
> > Thomas Gleixner wrote:
> > > What about analysing the code and verifying that the setup order is
> > > correct ?
> > > 
> > > Adding save/restore_irq just because you have no clue what the code
> > > does is utter nonsense.
> > 
> > Wouldn't it be quite a lot nicer if generic setup moved the
> > IRQF_DISABLED handler to be first in the list, if that actually works
> > in a useful way rather than simply being a quirk that irqs are
> > disabled for the first one?
> Hmm, what happens if an ISR runs with irqs disabled even though it
> doesn't expect it?  I wouldn't bet that nothing breaks.

Moving the IRQF_DISABLED handler to be first will run an ISR with
interrupts disabled which *does* expect it, so that's good.

According to this thread, at the moment when you have multiple
IRQF_DISABLED|IRQF_SHARED ISRs, only the first one is run with
interrupts disabled.

In fact I don't see why the kernel cannot put _all_ of the
IRQ_DISABLED handlers at the beginning of the list, traverse those
with interrupts disabled, then enable interrupts them for the
remaining handlers.

> IMHO the best is if a warning is printed or registering fails if shared
> irq actions don't agree about wanting IRQF_DISABLED.

On the hardware in question, the debug and timer interrupts share a
line, and I'm guessing only the timer interrupt should have IRQF_DISABLED.

Or we could do away with this silliness and just switch everything to
threaded interrupts with RT-priorities ;-)

-- Jamie

WARNING: multiple messages have this Message-ID (diff)
From: Jamie Lokier <jamie@shareable.org>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Rusty Russell <rusty@rustcorp.com.au>,
	David Brownell <dbrownell@users.sourceforge.net>,
	Eric Miao <eric.y.miao@gmail.com>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>,
	John Stultz <johnstul@us.ibm.com>,
	Nicolas Pitre <nico@marvell.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Ingo Molnar <mingo@elte.hu>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq
Date: Sun, 29 Nov 2009 15:18:40 +0000	[thread overview]
Message-ID: <20091129151840.GA30813@shareable.org> (raw)
In-Reply-To: <20091129102635.GA22653@pengutronix.de>

Uwe Kleine-König wrote:
> Hello,
> 
> On Sun, Nov 29, 2009 at 02:31:18AM +0000, Jamie Lokier wrote:
> > Thomas Gleixner wrote:
> > > What about analysing the code and verifying that the setup order is
> > > correct ?
> > > 
> > > Adding save/restore_irq just because you have no clue what the code
> > > does is utter nonsense.
> > 
> > Wouldn't it be quite a lot nicer if generic setup moved the
> > IRQF_DISABLED handler to be first in the list, if that actually works
> > in a useful way rather than simply being a quirk that irqs are
> > disabled for the first one?
> Hmm, what happens if an ISR runs with irqs disabled even though it
> doesn't expect it?  I wouldn't bet that nothing breaks.

Moving the IRQF_DISABLED handler to be first will run an ISR with
interrupts disabled which *does* expect it, so that's good.

According to this thread, at the moment when you have multiple
IRQF_DISABLED|IRQF_SHARED ISRs, only the first one is run with
interrupts disabled.

In fact I don't see why the kernel cannot put _all_ of the
IRQ_DISABLED handlers at the beginning of the list, traverse those
with interrupts disabled, then enable interrupts them for the
remaining handlers.

> IMHO the best is if a warning is printed or registering fails if shared
> irq actions don't agree about wanting IRQF_DISABLED.

On the hardware in question, the debug and timer interrupts share a
line, and I'm guessing only the timer interrupt should have IRQF_DISABLED.

Or we could do away with this silliness and just switch everything to
threaded interrupts with RT-priorities ;-)

-- Jamie

  reply	other threads:[~2009-11-29 15:18 UTC|newest]

Thread overview: 110+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-09-21  7:39 [PATCH] Don't disable irqs in set_next_event and set_mode callbacks Uwe Kleine-König
2009-09-21  9:04 ` Russell King - ARM Linux
2009-09-21  9:16   ` Uwe Kleine-König
2009-09-21  9:30     ` Russell King - ARM Linux
2009-09-21 10:48       ` Alessandro Rubini
2009-09-21 12:32 ` Kristoffer Ericson
2009-09-23 19:01   ` Eric Miao
2009-09-23 21:04 ` Remy Bohmer
2009-11-26 10:26 ` [RESENT PATCH] " Uwe Kleine-König
2009-11-26 10:50   ` Russell King - ARM Linux
2009-11-26 11:31     ` Russell King - ARM Linux
2009-11-27 10:44       ` Uwe Kleine-König
2009-11-27 19:08         ` Thomas Gleixner
2009-11-27 19:58         ` Russell King - ARM Linux
2009-11-27 20:38           ` Uwe Kleine-König
2009-11-27 20:44             ` Russell King - ARM Linux
2009-11-27 21:31               ` Thomas Gleixner
2009-11-27 21:59                 ` Russell King - ARM Linux
2009-11-27 22:20                   ` Thomas Gleixner
2009-11-27 21:10           ` [PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq Uwe Kleine-König
2009-11-27 21:10             ` Uwe Kleine-König
2009-11-27 22:18             ` Thomas Gleixner
2009-11-27 22:18               ` Thomas Gleixner
2009-11-28 20:03               ` Uwe Kleine-König
2009-11-28 20:03                 ` Uwe Kleine-König
2009-11-28 21:50                 ` Thomas Gleixner
2009-11-28 21:50                   ` Thomas Gleixner
2009-11-28 22:13                   ` David Brownell
2009-11-28 22:13                     ` David Brownell
2009-11-29  2:31                   ` Jamie Lokier
2009-11-29  2:31                     ` Jamie Lokier
2009-11-29 10:26                     ` Uwe Kleine-König
2009-11-29 10:26                       ` Uwe Kleine-König
2009-11-29 15:18                       ` Jamie Lokier [this message]
2009-11-29 15:18                         ` Jamie Lokier
2009-11-29 15:27                         ` Russell King - ARM Linux
2009-11-29 15:27                           ` Russell King - ARM Linux
2009-11-30 20:39                           ` Jamie Lokier
2009-11-30 20:39                             ` Jamie Lokier
2009-11-30  9:28                         ` Uwe Kleine-König
2009-11-30  9:28                           ` Uwe Kleine-König
2009-11-30  9:54                         ` Thomas Gleixner
2009-11-30  9:54                           ` Thomas Gleixner
2009-11-28 22:09                 ` David Brownell
2009-11-28 22:09                   ` David Brownell
2009-11-30 10:47             ` [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED at the right place Uwe Kleine-König
2009-11-30 10:47               ` Uwe Kleine-König
2009-11-30 13:54               ` Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED) Thomas Gleixner
2009-11-30 13:54                 ` Thomas Gleixner
2009-11-30 14:03                 ` Peter Zijlstra
2009-11-30 14:03                   ` Peter Zijlstra
2009-11-30 14:24                   ` Thomas Gleixner
2009-11-30 14:24                     ` Thomas Gleixner
2009-11-30 14:47                     ` Alan Cox
2009-11-30 14:47                       ` Alan Cox
2009-11-30 15:01                       ` Russell King - ARM Linux
2009-11-30 15:01                         ` Russell King - ARM Linux
2009-11-30 15:32                         ` Alan Cox
2009-11-30 15:32                           ` Alan Cox
2009-11-30 15:43                           ` Russell King - ARM Linux
2009-11-30 15:43                             ` Russell King - ARM Linux
2009-11-30 20:15                         ` Andrew Victor
2009-11-30 20:15                           ` Andrew Victor
2009-11-30 20:53                         ` David Brownell
2009-11-30 20:53                           ` David Brownell
2009-11-30 20:38                       ` David Brownell
2009-11-30 20:38                         ` David Brownell
2009-12-01  1:42                         ` Andy Walls
2009-12-01  1:42                           ` Andy Walls
2009-11-30 19:59                     ` Benjamin Herrenschmidt
2009-11-30 19:59                       ` Benjamin Herrenschmidt
2009-11-30 21:31                       ` Thomas Gleixner
2009-11-30 21:31                         ` Thomas Gleixner
2009-11-30 21:42                         ` Benjamin Herrenschmidt
2009-11-30 21:42                           ` Benjamin Herrenschmidt
2009-11-30 21:54                           ` Thomas Gleixner
2009-11-30 21:54                             ` Thomas Gleixner
2009-11-30 14:37                 ` Russell King - ARM Linux
2009-11-30 14:37                   ` Russell King - ARM Linux
2009-11-30 14:39                   ` Russell King - ARM Linux
2009-11-30 14:39                     ` Russell King - ARM Linux
2009-11-30 17:48                     ` Thomas Gleixner
2009-11-30 17:48                       ` Thomas Gleixner
2009-11-30 14:51                   ` Alan Cox
2009-11-30 14:51                     ` Alan Cox
2009-11-30 21:59                     ` Thomas Gleixner
2009-11-30 21:59                       ` Thomas Gleixner
2009-11-30 23:30                       ` Alan Cox
2009-11-30 23:30                         ` Alan Cox
2009-11-30 15:38                   ` Nicolas Pitre
2009-11-30 15:38                     ` Nicolas Pitre
2009-11-30 17:46                   ` Thomas Gleixner
2009-11-30 17:46                     ` Thomas Gleixner
2009-11-30 19:51                 ` Uwe Kleine-König
2009-11-30 19:51                   ` Uwe Kleine-König
2009-11-30 21:23                   ` Thomas Gleixner
2009-11-30 21:23                     ` Thomas Gleixner
2009-11-30 20:21               ` [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED at the right place David Brownell
2009-11-30 20:21                 ` David Brownell
2009-11-30 20:27                 ` Uwe Kleine-König
2009-11-30 20:27                   ` Uwe Kleine-König
2010-01-12 15:42               ` [RESEND PATCH] " Uwe Kleine-König
2010-01-12 15:42                 ` Uwe Kleine-König
2009-11-26 10:51   ` [RESENT PATCH] Don't disable irqs in set_next_event and set_mode callbacks Eric Miao
2009-12-17 13:33   ` [PATCH 1/2] arm/at91: " Uwe Kleine-König
2009-12-17 13:33     ` [PATCH 2/2] arm/{pxa, sa1100, nomadik}: Don't disable irqs in set_next_event and set_mode Uwe Kleine-König
2010-01-22 16:08     ` [PATCH 1/2] arm/at91: Don't disable irqs in set_next_event and set_mode callbacks Uwe Kleine-König
2010-01-22 16:36       ` Russell King - ARM Linux
2010-01-22 16:52         ` Uwe Kleine-König
2010-02-12 10:35           ` Uwe Kleine-König

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=20091129151840.GA30813@shareable.org \
    --to=jamie@shareable.org \
    --cc=linux-arm-kernel@lists.infradead.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.