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/
[PATCH 2/3] Add hard_irq_disable()
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 <[EMAIL PROTECTED]> 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.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 + /* 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 - 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/
[PATCH 2/3] Add hard_irq_disable()
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 [EMAIL PROTECTED] 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.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 + /* 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 - 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/