[PATCH] Keep a shadow copy of HFA384X_INTEN_OFF

[Denis Vlasenko]

On 22 May 2003 09:04, Pavel Roskin wrote:
> On Tue, 20 May 2003, Denis Vlasenko wrote:
> > Hmm... this is a small bit of a bigger problem. If we are to guard
> > against interrupt / schedule / other CPU hitting us between these
> > two lines of code:
> >
> >         HFA384X_OUTW(HFA384X_EVENT_MASK, HFA384X_INTEN_OFF);
> >         ((local_info_t*)dev->priv)->shadow_hfa384x_inten =
> > HFA384X_EVENT_MASK;
>
> First of all, please make it an inline function or another macro -
> that's the best way to keep data in-sync.

Agreed.

> > we are almost inevitably want to protect some surrounding code too,
> > because setting IntEn is usually only one stage of some larger
> > hw manipulation.
>
> 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.

> > In short: if we are worried about this race, we already have
> > locking problem (lack of locking) to fix.
>
> I'm not sure that your understanding of Jouni's words is correct.  I
> don't know what race he is worried about.  Please give a realistic
> scenario.

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.

> > I won't pretend that I have experience in writing good
> > SMP/IRQ/preempt safe code, but I think I see rather incomplete
> > locking in hostap.
>
> It may be, but hostap has 4 locks, which is quite impressive.

Actually, number of locks is not indicative of lock correctness.
I'm afraid 

> > hostap_ioctl -> prism2_ioctl_siwrate -> hostap_set_rate,
> > no locks/semaphores held, we arrive here:
>
> The orinoco driver holds a lock when processing ioctl.  You may want
> to check that code.  David Gibson is religious about locking.

Hmm? I'm not talking about orinoco code...

> > ret = (hostap_set_word(dev, HFA384X_RID_TXRATECONTROL,
> >                        local->tx_rate_control) ||
> >        hostap_set_word(dev, HFA384X_RID_CNFSUPPORTEDRATES,
> >                        local->tx_rate_control) ||
> >        local->func->reset_port(dev));   <--- (see below)
> >
> > What protects us between two set_port() calls or second set_port()
> > and reset_port()?
>
> 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.

And this is not a particularly bad piece of code, there are worse places.

So what about starting with one per-device semaphore to cover
interruptible/sleepable code and one spinlock for critical sections,
and then refining it if we'll see performance regression?

I can code it up but I'm afraid I can only compile-test it.
--
vda