[PATCH] Keep a shadow copy of HFA384X_INTEN_OFF
Thu May 22 22:43:47 PDT 2003
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.
> > 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
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
He is worried about this:
interrupt --------------------> handlers reads
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.
> > 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
processing IRQ... hostap_set_word(...)
There is locking inside set_word(), but we need locking around whole
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.
More information about the Hostap