Re: CVS commit: src/sys/arch/sparc

[David Laight <david%l8s.co.uk@localhost>]

On Sun, Oct 20, 2013 at 11:26:24AM +0200, Martin Husemann wrote:
> On Sun, Oct 20, 2013 at 10:05:30AM +0200, Alan Barrett wrote:
> > For example:
> > 
> > >#define raise_ipi(cpi,lvl) do {                    \
> > >-  volatile int x;                                 \
> > >+  int x;                                          \
> > >   (cpi)->intreg_4m->pi_set = PINTR_SINTRLEV(lvl); \
> > >-  x = (cpi)->intreg_4m->pi_pend;                  \
> > >+  x = (cpi)->intreg_4m->pi_pend; __USE(x);        \
> > >} while (0)
> 
> I don't think this change is incorrect, but I would do this differently.
> 
> If the RHS is not properly marked volatile, the compiler might cache the
> value and repeatadly store the same value again, so the volatile hidden
> somewhere in the RHS is important. I wonder, however, why it does not
> properly warn here (loss of qualifiers), so we should realy investigate
> this closer.
> 
> However, assuming the RHS is ok, it is IMHO better to get rid of "i" 
> completely
> and do:
> 
> #define raise_ipi(cpi,lvl)    do {                    \
>       (cpi)->intreg_4m->pi_set = PINTR_SINTRLEV(lvl); \
>       (void)(cpi)->intreg_4m->pi_pend;                \
> } while (0)

If pi_pend isn't volatile you need something like:
   *(volatile * typeof ((cpi)->intreg_4m->pi_pend))&(cpi)->intreg_4m->pi_pend

You might choose to know the type :-)

        David

-- 
David Laight: david%l8s.co.uk@localhost