[PATCH 2/3] Add hard_irq_disable()

[PATCH 2/3] Add hard_irq_disable()

[Benjamin Herrenschmidt]

Some architectures, like powerpc, implement lazy disabling of
interrupts. That means that on those, local_irq_disable() doesn't
actually disable interrupts on the CPU, but only sets some per
CPU flag which cause them to be disabled only if an interrupt actually
occurs.

However, in some cases, such as stop_machine, we really want
interrupts to be fully disabled. For example, I have code using
stop machine to do ECC error injection, used to verify operations
of the ECC hardware, that sort of thing. It really needs to make
sure that nothing is actually writing to memory while the injection
happens. Similar examples can be found in other low level bits and
pieces.

This patch implements a generic hard_irq_disable() function which
is meant to be called -after- local_irq_disable() and ensures that
interrupts are fully disabled on that CPU. The default implementation
is a nop, though powerpc does already provide an appropriate one.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

 include/linux/interrupt.h |   10 ++++++++++
 1 file changed, 10 insertions(+)

Index: linux-cell/include/linux/interrupt.h
===================================================================
--- linux-cell.orig/include/linux/interrupt.h	2007-05-10 14:51:22.000000000 +1000
+++ linux-cell/include/linux/interrupt.h	2007-05-10 15:18:04.000000000 +1000
@@ -241,6 +241,16 @@ static inline void __deprecated save_and
 #define save_and_cli(x)	save_and_cli(&x)
 #endif /* CONFIG_SMP */
 
+/* Some architectures might implement lazy enabling/disabling of
+ * interrupts. In some cases, such as stop_machine, we might want
+ * to ensure that after a local_irq_disable(), interrupts have
+ * really been disabled in hardware. Such architectures need to
+ * implement the following hook.
+ */
+#ifndef hard_irq_disable
+#define hard_irq_disable()	do { } while(0)
+#endif
+
 /* PLEASE, avoid to allocate new softirqs, if you need not _really_ high
    frequency threaded job scheduling. For almost all the purposes
    tasklets are more than enough. F.e. all serial device BHs et

Re: [PATCH 2/3] Add hard_irq_disable()

[Andrew Morton]

On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> --- linux-cell.orig/include/linux/interrupt.h	2007-05-10 14:51:22.000000000 +1000
> +++ linux-cell/include/linux/interrupt.h	2007-05-10 15:18:04.000000000 +1000
> @@ -241,6 +241,16 @@ static inline void __deprecated save_and
>  #define save_and_cli(x)	save_and_cli(&x)
>  #endif /* CONFIG_SMP */
>  
> +/* Some architectures might implement lazy enabling/disabling of
> + * interrupts. In some cases, such as stop_machine, we might want
> + * to ensure that after a local_irq_disable(), interrupts have
> + * really been disabled in hardware. Such architectures need to
> + * implement the following hook.
> + */
> +#ifndef hard_irq_disable
> +#define hard_irq_disable()	do { } while(0)
> +#endif

We absolutely require that the architecture's hard_irq_disable() be defined
when we do this.  If it happens that the definition of hard_irq_disable()
is implemented three levels deep in nested includes then we risk getting
into a situation where different .c files see different implementations of
hard_irq_disable(), which could lead to confusing results, to say the least.

Your implementation comes via the inclusion of system.h which then includes
hw_irq.h.  That's perhaps a little fragile and it would be better to

a) include in the comment the name of the arch file which must implement
   hard_irq_disable() and

b) include that file directly from this one.

Oh, and your comment layout style is wrong ;)

Re: [PATCH 2/3] Add hard_irq_disable()

[Benjamin Herrenschmidt]

On Wed, 2007-05-09 at 22:41 -0700, Andrew Morton wrote:
> On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > --- linux-cell.orig/include/linux/interrupt.h	2007-05-10 14:51:22.000000000 +1000
> > +++ linux-cell/include/linux/interrupt.h	2007-05-10 15:18:04.000000000 +1000
> > @@ -241,6 +241,16 @@ static inline void __deprecated save_and
> >  #define save_and_cli(x)	save_and_cli(&x)
> >  #endif /* CONFIG_SMP */
> >  
> > +/* Some architectures might implement lazy enabling/disabling of
> > + * interrupts. In some cases, such as stop_machine, we might want
> > + * to ensure that after a local_irq_disable(), interrupts have
> > + * really been disabled in hardware. Such architectures need to
> > + * implement the following hook.
> > + */
> > +#ifndef hard_irq_disable
> > +#define hard_irq_disable()	do { } while(0)
> > +#endif
> 
> We absolutely require that the architecture's hard_irq_disable() be defined
> when we do this.  If it happens that the definition of hard_irq_disable()
> is implemented three levels deep in nested includes then we risk getting
> into a situation where different .c files see different implementations of
> hard_irq_disable(), which could lead to confusing results, to say the least.

Yes, I'm indeed a bit worried about that... I've been wondering what's
the best include path here... I tried to follow who gets to hw_irq.h and
didn't come to any conclusive results.

powerpc gets it from asm/system.h but I haven't verified other arch
(though it only matters on arch that have their own here).

I've verified that a #error on ppc up there will not trigger thus it's
fine on powerpc, but I agree it's a bit fragile.

> Your implementation comes via the inclusion of system.h which then includes
> hw_irq.h.  That's perhaps a little fragile and it would be better to
> 
> a) include in the comment the name of the arch file which must implement
>    hard_irq_disable() and
> 
> b) include that file directly from this one.

Fair enough. I was just worried that including hw_irq.h here might cause
trouble for some archs though (as I said, we get it indirectly on
powerpc via some other asm thingy, not via some linux/*.h). I've looked
around and seen all sort of horrors in arch include dependencies
(including some circular stuff that must work by mere luck).

> Oh, and your comment layout style is wrong ;)

What about my comment layout style ? I've been using that forever ... Or
do you mean I should use a function documentation style layout there ?

Cheers,
Ben.

Re: [PATCH 2/3] Add hard_irq_disable()

[Andrew Morton]

On Thu, 10 May 2007 16:35:51 +1000 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Wed, 2007-05-09 at 22:41 -0700, Andrew Morton wrote:
> > On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> > 
> > > --- linux-cell.orig/include/linux/interrupt.h	2007-05-10 14:51:22.000000000 +1000
> > > +++ linux-cell/include/linux/interrupt.h	2007-05-10 15:18:04.000000000 +1000
> > > @@ -241,6 +241,16 @@ static inline void __deprecated save_and
> > >  #define save_and_cli(x)	save_and_cli(&x)
> > >  #endif /* CONFIG_SMP */
> > >  
> > > +/* Some architectures might implement lazy enabling/disabling of
> > > + * interrupts. In some cases, such as stop_machine, we might want
> > > + * to ensure that after a local_irq_disable(), interrupts have
> > > + * really been disabled in hardware. Such architectures need to
> > > + * implement the following hook.
> > > + */
> > > +#ifndef hard_irq_disable
> > > +#define hard_irq_disable()	do { } while(0)
> > > +#endif
> > 
> > We absolutely require that the architecture's hard_irq_disable() be defined
> > when we do this.  If it happens that the definition of hard_irq_disable()
> > is implemented three levels deep in nested includes then we risk getting
> > into a situation where different .c files see different implementations of
> > hard_irq_disable(), which could lead to confusing results, to say the least.
> 
> Yes, I'm indeed a bit worried about that... I've been wondering what's
> the best include path here... I tried to follow who gets to hw_irq.h and
> didn't come to any conclusive results.
> 
> powerpc gets it from asm/system.h but I haven't verified other arch
> (though it only matters on arch that have their own here).
> 
> I've verified that a #error on ppc up there will not trigger thus it's
> fine on powerpc, but I agree it's a bit fragile.

I think saying "system.h must provide this" is reasonable.  The fact that
powerpc does that via another inclusion is a powerpc detail - just don't
break it ;)


> > Your implementation comes via the inclusion of system.h which then includes
> > hw_irq.h.  That's perhaps a little fragile and it would be better to
> > 
> > a) include in the comment the name of the arch file which must implement
> >    hard_irq_disable() and
> > 
> > b) include that file directly from this one.
> 
> Fair enough. I was just worried that including hw_irq.h here might cause
> trouble for some archs though (as I said, we get it indirectly on
> powerpc via some other asm thingy, not via some linux/*.h). I've looked
> around and seen all sort of horrors in arch include dependencies
> (including some circular stuff that must work by mere luck).
> 
> > Oh, and your comment layout style is wrong ;)
> 
> What about my comment layout style ? I've been using that forever ... Or
> do you mean I should use a function documentation style layout there ?

/* This
 * is
 * wrong
 */

/*
 * This
 * is
 * right
 */

Re: [PATCH 2/3] Add hard_irq_disable()

[Benjamin Herrenschmidt]

> > What about my comment layout style ? I've been using that forever ... Or
> > do you mean I should use a function documentation style layout there ?
> 
> /* This
>  * is
>  * wrong
>  */
> 
> /*
>  * This
>  * is
>  * right
>  */

Hrm... how bad are you about that one ? I must say I prefer my style :-)

Cheers,
Ben.

Re: [PATCH 2/3] Add hard_irq_disable()

[David Miller]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Thu, 10 May 2007 18:41:45 +1000

> 
> > > What about my comment layout style ? I've been using that forever ... Or
> > > do you mean I should use a function documentation style layout there ?
> > 
> > /* This
> >  * is
> >  * wrong
> >  */
> > 
> > /*
> >  * This
> >  * is
> >  * right
> >  */
> 
> Hrm... how bad are you about that one ? I must say I prefer my style :-)

I think the "wrong" one is right because screen real-estate matters.
In fact that's the one I use and enforce throughout the networking
and sparc ports.

I even hand edit out those empty comment lines in patches submitted
to me when I spot them.

Re: [PATCH 2/3] Add hard_irq_disable()

[Andrew Morton]

On Thu, 10 May 2007 18:41:45 +1000 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> > > What about my comment layout style ? I've been using that forever ... Or
> > > do you mean I should use a function documentation style layout there ?
> > 
> > /* This
> >  * is
> >  * wrong
> >  */
> > 
> > /*
> >  * This
> >  * is
> >  * right
> >  */
> 
> Hrm... how bad are you about that one ? I must say I prefer my style :-)
> 

I usually shrug and ignore it.  It's more a matter of informing people
about it than going in there and fixing them all.

We discussed this a couple of months back.  davem landed firmly in the
second camp and everyone then shut up ;)

Re: [PATCH 2/3] Add hard_irq_disable()

[David Miller]

From: Andrew Morton <akpm@linux-foundation.org>
Date: Thu, 10 May 2007 01:50:36 -0700

> We discussed this a couple of months back.  davem landed firmly in the
> second camp and everyone then shut up ;)

No I landed in the first :-)))

I think the empty lines are a waste and only serve to eat
up precious screen real-estate when reading code.

It is possible that I used to use the empty line thing in
the past, but I definitely don't do that any more.

Re: [PATCH 2/3] Add hard_irq_disable()

[Benjamin Herrenschmidt]

On Thu, 2007-05-10 at 01:53 -0700, David Miller wrote:
> From: Andrew Morton <akpm@linux-foundation.org>
> Date: Thu, 10 May 2007 01:50:36 -0700
> 
> > We discussed this a couple of months back.  davem landed firmly in the
> > second camp and everyone then shut up ;)
> 
> No I landed in the first :-)))
> 
> I think the empty lines are a waste and only serve to eat
> up precious screen real-estate when reading code.
> 
> It is possible that I used to use the empty line thing in
> the past, but I definitely don't do that any more.

Yup, I used to do the other one too but nowadays, I much prefer not
wasting that additional line unless specific circumstances, like I
want a kind of "title" in front of a whole block of other definitions
with their own comments.

Something like:


/*
 * foo management stuff
 */


/* This puts the bar in the foo
 */
code code code code

/* This does something you don't want to know about
 */
code code code code


But does it realy matter that much ? :-)

Cheers,
Ben.

Re: [PATCH 2/3] Add hard_irq_disable()

[Josh Boyer]

On Thu, 2007-05-10 at 19:29 +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2007-05-10 at 01:53 -0700, David Miller wrote:
> > From: Andrew Morton <akpm@linux-foundation.org>
> > Date: Thu, 10 May 2007 01:50:36 -0700
> > 
> > > We discussed this a couple of months back.  davem landed firmly in the
> > > second camp and everyone then shut up ;)
> > 
> > No I landed in the first :-)))
> > 
> > I think the empty lines are a waste and only serve to eat
> > up precious screen real-estate when reading code.
> > 
> > It is possible that I used to use the empty line thing in
> > the past, but I definitely don't do that any more.
> 
> Yup, I used to do the other one too but nowadays, I much prefer not
> wasting that additional line unless specific circumstances, like I
> want a kind of "title" in front of a whole block of other definitions
> with their own comments.
> 
> Something like:
> 
> 
> /*
>  * foo management stuff
>  */
> 
> 
> /* This puts the bar in the foo
>  */
> code code code code
> 
> /* This does something you don't want to know about
>  */
> code code code code

Now your examples are just wrong.  One liners should be:

/* This puts the bar in the foo */

Are we having fun yet? ;)

/me goes to look for something better to do

josh

Re: [PATCH 2/3] Add hard_irq_disable()

[Satyam Sharma]

Hi Andrew,

On 5/10/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> > --- linux-cell.orig/include/linux/interrupt.h 2007-05-10 14:51:22.000000000 +1000
> > +++ linux-cell/include/linux/interrupt.h      2007-05-10 15:18:04.000000000 +1000
> > @@ -241,6 +241,16 @@ static inline void __deprecated save_and
> >  #define save_and_cli(x)      save_and_cli(&x)
> >  #endif /* CONFIG_SMP */
> >
> > +/* Some architectures might implement lazy enabling/disabling of
> > + * interrupts. In some cases, such as stop_machine, we might want
> > + * to ensure that after a local_irq_disable(), interrupts have
> > + * really been disabled in hardware. Such architectures need to
> > + * implement the following hook.
> > + */
> > +#ifndef hard_irq_disable
> > +#define hard_irq_disable()   do { } while(0)
> > +#endif
>
> We absolutely require that the architecture's hard_irq_disable() be defined
> when we do this.  If it happens that the definition of hard_irq_disable()
> is implemented three levels deep in nested includes then we risk getting
> into a situation where different .c files see different implementations of
> hard_irq_disable(), which could lead to confusing results, to say the least.

So you're saying that this mechanism forces the arch (that really
wants hard_irq_disable) to _#define_ hard_irq_disable (as a macro),
and if it implements it as an inline function, for example, then we're
screwed?

> Your implementation comes via the inclusion of system.h which then includes
> hw_irq.h.  That's perhaps a little fragile and it would be better to
>
> a) include in the comment the name of the arch file which must implement
>    hard_irq_disable() and
>
> b) include that file directly from this one.

Hmmm. How about:

1. Introduce some CONFIG_WANTS_HARD_IRQ_DISABLE that is #defined (or
left undefined) by the arch/.../defconfig (depending upon whether or
not that arch implements a hard_irq_disable() for itself or not)

2. Then pull-in that code into include/linux/interrupt.h somehow
(through some known / fixed header file, or through asm/system.h, or
anyhow -- it doesn't really matter)

3. And:

#ifndef CONFIG_WANTS_HARD_IRQ_DISABLE
#define hard_irq_disable() do { } while(0)
#endif

We don't need to standardize on some particular arch-specific header
filename in this case.

Comments?

Satyam

Re: [PATCH 2/3] Add hard_irq_disable()

[Benjamin Herrenschmidt]

> So you're saying that this mechanism forces the arch (that really
> wants hard_irq_disable) to _#define_ hard_irq_disable (as a macro),
> and if it implements it as an inline function, for example, then we're
> screwed?

No. The idea is to do like we did for a few other things already
(according to Linus request in fact), which is to write

static inline void hard_irq_disable(void)
{
	.../...
}
#define hard_irq_disable hard_irq_disable

This is nicer than having an ARCH_HAS_xxx

> 1. Introduce some CONFIG_WANTS_HARD_IRQ_DISABLE that is #defined (or
> left undefined) by the arch/.../defconfig (depending upon whether or
> not that arch implements a hard_irq_disable() for itself or not)
>
> 2. Then pull-in that code into include/linux/interrupt.h somehow
> (through some known / fixed header file, or through asm/system.h, or
> anyhow -- it doesn't really matter)
> 
> 3. And:
> 
> #ifndef CONFIG_WANTS_HARD_IRQ_DISABLE
> #define hard_irq_disable() do { } while(0)
> #endif

Well, last time I tried that, Linus NACKed it in favor of what I
described above.

> We don't need to standardize on some particular arch-specific header
> filename in this case.

True, that's my main problem here. Though really only the archs who
actually implement something special here need to be careful.

Ben.

Re: [PATCH 2/3] Add hard_irq_disable()

[Satyam Sharma]

On 5/10/07, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> > So you're saying that this mechanism forces the arch (that really
> > wants hard_irq_disable) to _#define_ hard_irq_disable (as a macro),
> > and if it implements it as an inline function, for example, then we're
> > screwed?
>
> No. The idea is to do like we did for a few other things already
> (according to Linus request in fact), which is to write
>
> static inline void hard_irq_disable(void)
> {
>         .../...
> }
> #define hard_irq_disable hard_irq_disable
>
> This is nicer than having an ARCH_HAS_xxx

Ok, that's reasonable, we don't want to end up with zillions of
ARCH_HAS_THIS and ARCH_HAS_THAT.

But then, what _is_ the problem with your approach above? An arch that
wants (and implements) hard_irq_disable will also #define that dummy
macro, so we just need to pull in the appropriate header (directly,
indirectly, anyhow -- we don't really care) into
include/linux/interrupt.h and then just do the exact same "#ifndef
hard_irq_disable" check that you're doing right now. I must be missing
something trivial (either that or I need to go and have a coffee :-)
because I don't see the possibility of hitting multiple _different_
definitions with the approach you mentioned just now.

Re: [PATCH 2/3] Add hard_irq_disable()

[Benjamin Herrenschmidt]

On Thu, 2007-05-10 at 13:24 +0530, Satyam Sharma wrote:
> But then, what _is_ the problem with your approach above? An arch that
> wants (and implements) hard_irq_disable will also #define that dummy
> macro, so we just need to pull in the appropriate header (directly,
> indirectly, anyhow -- we don't really care) into
> include/linux/interrupt.h and then just do the exact same "#ifndef
> hard_irq_disable" check that you're doing right now. I must be missing
> something trivial (either that or I need to go and have a coffee :-)
> because I don't see the possibility of hitting multiple _different_
> definitions with the approach you mentioned just now.

Sure, the only problem is that I don't want to pull asm/hw_irq.h
directly from linux/interrupts.h unless all arch maintainers around
verify it's ok, because those headers are a bit of a can of worm at the
moment ...

So I'd rather say that if your arch has a custom version of
hard_irq_disable(), make sure that asm/system.h pulls it in a way or
another. And that's already included.

Ben.