[PATCH] Keep a shadow copy of HFA384X_INTEN_OFF

[Pavel Roskin]

On Fri, 23 May 2003, Denis Vlasenko wrote:

> > A lot of races can be avoided by enabling interrupts after such
> > manipulation and disabling them before doing anything drastic (e.g.
> > reset), provided that the hardware behaves properly and obeys those
> > registers.
>
> This is not enough considering SMP/preempt. IMHO current code
> does not take this into consideration.

I don't understand.  We started the discussion about interrupts and now it
seems you are talking about locking between two user contexts.

As I understand it, the code executed in user context is not critical to
performance, so it should be OK to put locks there as long as they work
correctly.  Just be careful not to introduce deadlocks - they are much
worse than a couple of lost packets per million.

> He is worried about this:
>
> 	HFA384X_OUTW(v, HFA384X_INTEN_OFF);
> 	interrupt --------------------> handlers reads
> 					shadow_hfa384x_inten
> 					and sees wrong value
>         shadow_hfa384x_inten = v;
>
> This indeed can happen. But the same bad thing can happen in
> tons of places.

Correct.  If we move setting the shadow variable before changing the
hardware register, then the interrupt routine will ignore the disabled
events.  Note that the interrupts are not normally disabled.  We a talking
about user intervention here, and losing an interrupt is not a big deal,
especially because we are going to ignore all interrupts microseconds
later.

> > The question is what is can happen and how can it affect correctness
> > of the operation.  I cannot think of anything bad in this case.  RID
> > settings become affective on reset, i.e. atomically.
>
> But IRQs and, worse, other CPUs can bite us:
>
> CPU1: doing ioctl			CPU2: doing ioctl
> -----------------			-------------------------
> hostap_set_word(...)
> IRQ---->				hostap_set_word(...)
> 	processing IRQ...		hostap_set_word(...)
> <-------
> hostap_set_word(...)
> reset_port(...)
> 					reset_port(...)
>
> There is locking inside set_word(), but we need locking around whole
> {set_word(),set_word(),reset_port()} thing.

But why?  I'm not aware of any reason why it's bad.  Are you afraid to
lose one of the settings?  But each of them if written correctly, and they
are kept separately in the MAC chip.

-- 
Regards,
Pavel Roskin