Re: [PATCH 2/3] Add hard_irq_disable()
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 <[EMAIL PROTECTED]> > > 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
On Thu, 2007-05-10 at 01:53 -0700, David Miller wrote: > From: Andrew Morton <[EMAIL PROTECTED]> > 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
From: Andrew Morton <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
On Thu, 10 May 2007 18:41:45 +1000 Benjamin Herrenschmidt <[EMAIL PROTECTED]> 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 ;) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
From: Benjamin Herrenschmidt <[EMAIL PROTECTED]> 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
> > 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
On 5/10/07, Benjamin Herrenschmidt <[EMAIL PROTECTED]> 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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
On Thu, 10 May 2007 16:35:51 +1000 Benjamin Herrenschmidt <[EMAIL PROTECTED]>
wrote:
> On Wed, 2007-05-09 at 22:41 -0700, Andrew Morton wrote:
> > On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt <[EMAIL
> > PROTECTED]> wrote:
> >
> > > --- linux-cell.orig/include/linux/interrupt.h 2007-05-10
> > > 14:51:22.0 +1000
> > > +++ linux-cell/include/linux/interrupt.h 2007-05-10 15:18:04.0
> > > +1000
> > > @@ -241,6 +241,16 @@ static inline void __deprecated save_and
> > > #define save_and_cli(x) save_and_cli()
> > > #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
*/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
> 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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
Hi Andrew,
On 5/10/07, Andrew Morton <[EMAIL PROTECTED]> wrote:
On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt <[EMAIL PROTECTED]>
wrote:
> --- linux-cell.orig/include/linux/interrupt.h 2007-05-10 14:51:22.0
+1000
> +++ linux-cell/include/linux/interrupt.h 2007-05-10 15:18:04.0
+1000
> @@ -241,6 +241,16 @@ static inline void __deprecated save_and
> #define save_and_cli(x) save_and_cli()
> #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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
On Wed, 2007-05-09 at 22:41 -0700, Andrew Morton wrote:
> On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt <[EMAIL PROTECTED]>
> wrote:
>
> > --- linux-cell.orig/include/linux/interrupt.h 2007-05-10
> > 14:51:22.0 +1000
> > +++ linux-cell/include/linux/interrupt.h2007-05-10 15:18:04.0
> > +1000
> > @@ -241,6 +241,16 @@ static inline void __deprecated save_and
> > #define save_and_cli(x)save_and_cli()
> > #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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
On Wed, 2007-05-09 at 22:41 -0700, Andrew Morton wrote:
On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt [EMAIL PROTECTED]
wrote:
--- linux-cell.orig/include/linux/interrupt.h 2007-05-10
14:51:22.0 +1000
+++ linux-cell/include/linux/interrupt.h2007-05-10 15:18:04.0
+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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
Hi Andrew,
On 5/10/07, Andrew Morton [EMAIL PROTECTED] wrote:
On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt [EMAIL PROTECTED]
wrote:
--- linux-cell.orig/include/linux/interrupt.h 2007-05-10 14:51:22.0
+1000
+++ linux-cell/include/linux/interrupt.h 2007-05-10 15:18:04.0
+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
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
On Thu, 10 May 2007 16:35:51 +1000 Benjamin Herrenschmidt [EMAIL PROTECTED]
wrote:
On Wed, 2007-05-09 at 22:41 -0700, Andrew Morton wrote:
On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt [EMAIL
PROTECTED] wrote:
--- linux-cell.orig/include/linux/interrupt.h 2007-05-10
14:51:22.0 +1000
+++ linux-cell/include/linux/interrupt.h 2007-05-10 15:18:04.0
+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
*/
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
On 5/10/07, Benjamin Herrenschmidt [EMAIL PROTECTED] 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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
From: Benjamin Herrenschmidt [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
On Thu, 10 May 2007 18:41:45 +1000 Benjamin Herrenschmidt [EMAIL PROTECTED] 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 ;) - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
From: Andrew Morton [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
On Thu, 2007-05-10 at 01:53 -0700, David Miller wrote: From: Andrew Morton [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
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 [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt <[EMAIL PROTECTED]>
wrote:
> --- linux-cell.orig/include/linux/interrupt.h 2007-05-10 14:51:22.0
> +1000
> +++ linux-cell/include/linux/interrupt.h 2007-05-10 15:18:04.0
> +1000
> @@ -241,6 +241,16 @@ static inline void __deprecated save_and
> #define save_and_cli(x) save_and_cli()
> #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 ;)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] Add hard_irq_disable()
On Thu, 10 May 2007 15:25:58 +1000 Benjamin Herrenschmidt [EMAIL PROTECTED]
wrote:
--- linux-cell.orig/include/linux/interrupt.h 2007-05-10 14:51:22.0
+1000
+++ linux-cell/include/linux/interrupt.h 2007-05-10 15:18:04.0
+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 ;)
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
