Re: [PATCH] synchronize_irq needs a barrier

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

> > Index: linux-work/kernel/irq/manage.c
> > ===================================================================
> > --- linux-work.orig/kernel/irq/manage.c	2007-10-18 11:22:16.000000000 +1000
> > +++ linux-work/kernel/irq/manage.c	2007-10-18 11:22:20.000000000 +1000
> > @@ -33,6 +33,7 @@ void synchronize_irq(unsigned int irq)
> >  	if (irq >= NR_IRQS)
> >  		return;
> >  
> > +	smp_mb();
> >  	while (desc->status & IRQ_INPROGRESS)
> >  		cpu_relax();
> >  }
> 
> Anyone reading this code is going to ask "wtf is that for".  It needs a
> comment telling them.
> 
> 
> mb() is the new lock_kernel().  Sigh.

Ugh ?

That sounds fairly obvious to me :-) we are reading a value, that is
totally unordered, nothing to do about lock kernel or whatever, if we
want the above statement to make any sense in any kind of usage
scenario, it needs to be ordered vs. what happens before.

For example, take a construct like:

	device->my_hw_is_off = 1;
	synchronize_irq();
	turn_off_hardware();

That basically makes sure the irq either sees device->my_hw_is_off
being set to 1, or if an irq handler is already in progress and hasn't
seen it, we wait for it to complete.

(You can replace "hw_is_off" with anything that we want to set and make
sure the IRQ handler sees it before proceeding. It could be clearing a
pointer to something and make sure the irq sees it before freeing the
data, etc...).

I think pretty much any use of synchronize_irq() I can imagine needs
such kind of ordering... or it simply doesn't synchronize anything :-)

Cheers,
Ben.