Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-27 Thread Herbert Xu
Alexey Dobriyan <[EMAIL PROTECTED]> wrote: > >irq_flags_t flags; > >flags = spin_lock_irqXXX(&lock); >spin_unlock_irqYYY(&lock, flags); > > where XXX and YYY are still to be found good names :^) It's also a solution How about flags? flags = spin_lock_irqflags(&lo

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-27 Thread Roman Zippel
Hi, On Sun, 28 Oct 2007, Alexey Dobriyan wrote: > > If it's just about the type checking, something like below should pretty > > much do the same. > > It won't catch, the following if both variables are unsigned long: > > spin_lock_irqsave(&lock, flags); > [stuff] > s

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-27 Thread Peter Zijlstra
On Sun, 2007-10-28 at 00:14 +0400, Alexey Dobriyan wrote: > On Sat, Oct 27, 2007 at 09:20:43PM +0200, Roman Zippel wrote: > > On Sun, 21 Oct 2007, Alexey Dobriyan wrote: > > > > > So far remedies were: > > > a) grep(1) -- obviously fragile. I tried at some point grepping for > > >spin_lock_irq

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-27 Thread Jan Engelhardt
On Oct 28 2007 00:24, Alexey Dobriyan wrote: > >> (Which would be the logical choice if it were a function and not a >> macro...) That would flag up all violations ("without cast to different >> pointer" or so) while usually not breaking compilation. >> >> Of course, irq_flags_t is probably the

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-27 Thread Alexey Dobriyan
On Thu, Oct 25, 2007 at 05:40:03PM +0200, Jan Engelhardt wrote: > On Oct 21 2007, Alexey Dobriyan wrote: > > > >One of type of bugs steadily being fought is the following one: > > > > unsigned int flags; > > spin_lock_irqsave(&lock, flags); > > > >where "flags" should be "unsigned long". He

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-27 Thread Alexey Dobriyan
On Sat, Oct 27, 2007 at 09:20:43PM +0200, Roman Zippel wrote: > On Sun, 21 Oct 2007, Alexey Dobriyan wrote: > > > So far remedies were: > > a) grep(1) -- obviously fragile. I tried at some point grepping for > >spin_lock_irqsave(), found quite a few, but it became bring quickly. > > b) BUI

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-27 Thread Roman Zippel
Hi, On Sun, 21 Oct 2007, Alexey Dobriyan wrote: > So far remedies were: > a) grep(1) -- obviously fragile. I tried at some point grepping for >spin_lock_irqsave(), found quite a few, but it became bring quickly. > b) BUILD_BUG_ON(sizeof(flags) != sizeof(unsigned long)) -- was tried, >

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-25 Thread Jan Engelhardt
Hi, On Oct 21 2007, Alexey Dobriyan wrote: > >One of type of bugs steadily being fought is the following one: > > unsigned int flags; > spin_lock_irqsave(&lock, flags); > >where "flags" should be "unsigned long". Here is far from complete list >of commits fixing such bugs: > How abou

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-24 Thread Arnd Bergmann
On Wednesday 24 October 2007, Jeff Garzik wrote: > > > > Could we add a debug option that warned if spin_lock_irq is > > executed with IRQs turned off already? > > Seems reasonable but perhaps arch-specific? > > Also, I think someone (akpm?) mentioned an effort had been made before, > and run i

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-23 Thread Jeff Garzik
Herbert Xu wrote: Jeff Garzik <[EMAIL PROTECTED]> wrote: Let me add to the chorus of voices: I continually see two cases where real bugs crop up: 1) hacker uses spin_lock_irq() in incorrect context (where it is not safe to do a blind enable/disable) 2) hacker uses spin_lock_irq() correctly

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-22 Thread Herbert Xu
Jeff Garzik <[EMAIL PROTECTED]> wrote: > > Let me add to the chorus of voices: I continually see two cases where > real bugs crop up: > > 1) hacker uses spin_lock_irq() in incorrect context (where it is not > safe to do a blind enable/disable) > > 2) hacker uses spin_lock_irq() correctly, but

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-22 Thread Arnd Bergmann
On Monday 22 October 2007, Thomas Gleixner wrote: > On Mon, 22 Oct 2007, Arnd Bergmann wrote: > > > I tried this as well a few years ago, and I think I hit a few places in > > the early initialization, but nothing unfixable. > > Hmm, lockdep checks this already. If it does not catch it, we need to

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-22 Thread David Miller
From: Jeff Garzik <[EMAIL PROTECTED]> Date: Mon, 22 Oct 2007 16:47:16 -0400 > Let me add to the chorus of voices: I continually see two cases where > real bugs crop up: > > 1) hacker uses spin_lock_irq() in incorrect context (where it is not > safe to do a blind enable/disable) > > 2) hacker

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-22 Thread Thomas Gleixner
On Mon, 22 Oct 2007, Arnd Bergmann wrote: > On Monday 22 October 2007, Andrew Morton wrote: > > It's almost always a bug to do spin_lock_irq() when local interrupts are > > disabled.  However iirc when we've tried to add runtime debugging to catch > > that, it triggered false-positives which made

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-22 Thread Arnd Bergmann
On Monday 22 October 2007, Andrew Morton wrote: > It's almost always a bug to do spin_lock_irq() when local interrupts are > disabled.  However iirc when we've tried to add runtime debugging to catch > that, it triggered false-positives which made the idea unworkable.  I forget > where. I tried th

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-22 Thread Jeff Garzik
Andrew Morton wrote: Linus Torvalds <[EMAIL PROTECTED]> wrote: > On Mon, 22 Oct 2007, Matthew Wilcox wrote: > > We certainly don't want to encourage people to blindly make those > > conversions ... and I've seen the results of encouraging kernel janitors > > to do things a certain way. > Th

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-22 Thread Andrew Morton
On Mon, 22 Oct 2007 12:56:29 -0700 (PDT) Linus Torvalds <[EMAIL PROTECTED]> wrote: > > > On Mon, 22 Oct 2007, Matthew Wilcox wrote: > > > > We certainly don't want to encourage people to blindly make those > > conversions ... and I've seen the results of encouraging kernel janitors > > to do th

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-22 Thread Linus Torvalds
On Mon, 22 Oct 2007, Matthew Wilcox wrote: > > We certainly don't want to encourage people to blindly make those > conversions ... and I've seen the results of encouraging kernel janitors > to do things a certain way. There's another issue: the "irqsave/irqrestore" versions are much safer than

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-22 Thread Matthew Wilcox
On Mon, Oct 22, 2007 at 09:10:34PM +0200, Arnd Bergmann wrote: > On a related note, should we encourage the use of spin_lock() and > spin_lock_irq() instead of spin_lock_irqsave() where possible? spin_lock(), certainly. On PowerPC, I'm reliably informed it's fewer instructions to save/restore tha

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-22 Thread Arnd Bergmann
On Monday 22 October 2007, Andrew Morton wrote: > Yes, it's always been ugly that we use unsigned long for this rather than > abstracting it properly. > > However I'd prefer that we have some really good reason for introducing > irq_flags_t now.  Simply so that I don't needlessly spend the next tw

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-22 Thread Geert Uytterhoeven
On Mon, 22 Oct 2007, Andrew Morton wrote: > On Mon, 22 Oct 2007 16:29:12 +0100 Ralf Baechle <[EMAIL PROTECTED]> wrote: > > On Sun, Oct 21, 2007 at 01:30:42PM +0400, Alexey Dobriyan wrote: > > > > > irq_flags_t > > > > > > New type for use with spin_lock_irqsave() and friends.

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-22 Thread Andrew Morton
On Mon, 22 Oct 2007 16:29:12 +0100 Ralf Baechle <[EMAIL PROTECTED]> wrote: > On Sun, Oct 21, 2007 at 01:30:42PM +0400, Alexey Dobriyan wrote: > > > irq_flags_t > > > > New type for use with spin_lock_irqsave() and friends. > > Talking about it, why did we ever require th

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-22 Thread Ralf Baechle
On Sun, Oct 21, 2007 at 01:30:42PM +0400, Alexey Dobriyan wrote: > irq_flags_t > > New type for use with spin_lock_irqsave() and friends. Talking about it, why did we ever require this to be a long anyway? I could get away with a single bit for MIPS; the rest of this

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-21 Thread Alexey Dobriyan
nditional > closer to the end of conversion. OK! Resending patch #1, patch #2 stays the same. [PATCH 1/2] irq_flags_t: intro and core annotations One type of bugs steadily being fought is the following one: unsigned int flags; spin_lock_irqsave(&lock, flags); where &q

Re: [PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-20 Thread Al Viro
On Sun, Oct 21, 2007 at 03:55:46AM +0400, Alexey Dobriyan wrote: > * irq_flags_t is marked with __bitwise__ which means sparse(1) will warn > developer when he accidently uses wrong type or totally wrong variable. > * irq_flags_t allows conversion to struct instead of typedef without flag day. >

[PATCH 1/2] irq_flags_t: intro and core annotations

2007-10-20 Thread Alexey Dobriyan
One of type of bugs steadily being fought is the following one: unsigned int flags; spin_lock_irqsave(&lock, flags); where "flags" should be "unsigned long". Here is far from complete list of commits fixing such bugs: 099575b6cb7eaf18211ba72de56264f67651b90b 5efee174f8a101cafb160