[PATCH] Keep a shadow copy of HFA384X_INTEN_OFF

[Denis Vlasenko]

On 19 May 2003 16:33, Jouni Malinen wrote:
> On Mon, May 19, 2003 at 10:17:38AM +0300, Denis Vlasenko wrote:
> > This patch against today's CVS keeps a shadow copy
> > of last value written to HFA384X_INTEN register,
> > replacing expensive INWs with main memory fetches.
> >
> > Compile tested.
>
> Did you solve the race condition that I fixed by more or less the
> exact reverse of this patch? local->event_mask used to be a shadow
> copy of IntEn, but it had a race condition and would have needed
> heave locking to fix it.. Removing the shadow copy was the easiest
> way of getting rid of that issue.

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;

we are almost inevitably want to protect some surrounding code too,
because setting IntEn is usually only one stage of some larger
hw manipulation.

In short: if we are worried about this race, we already have
locking problem (lack of locking) to fix.

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.

Random example:

hostap_ioctl -> prism2_ioctl_siwrate -> hostap_set_rate,
no locks/semaphores held, we arrive here:

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()?


hostap_set_word -> set_rid -> hfa384x_set_rid:
...
        res = down_interruptible(&local->rid_bap_sem);
	*** works with BAP and cmd ***
        up(&local->rid_bap_sem);
...
        if (res == -ETIMEDOUT)
                prism2_hw_reset(dev);  <--- (see below)
        return res;
}

Again, no protection between up(&local->rid_bap_sem) and
prism2_hw_reset(dev).


static int prism2_reset_port(struct net_device *dev)
...
        res = hfa384x_cmd(dev, HFA384X_CMDCODE_DISABLE, 0,
                          NULL, NULL);
        if (res)
                printk...
        else {
                res = hfa384x_cmd(dev, HFA384X_CMDCODE_ENABLE, 0,
                                  NULL, NULL);
...
}

No protection between two hfa384x_cmd() calls


static int hfa384x_cmd(...)
{
...
        if (local->cmd_queue_len >= HOSTAP_CMD_QUEUE_MAX_LEN) {
                printk(KERN_DEBUG "%s: hfa384x_cmd: cmd_queue full\n",
                       dev->name);
                return -1;
        }

No protection against local->cmd_queue_len++ on another CPU.


static void prism2_hw_reset(struct net_device *dev)
{
...
        if (local->hw_resetting) {
                printk(KERN_WARNING "%s: %s: already resetting card - "
                       "ignoring reset request\n", dev_info, dev->name);
                return;
        }
...
        local->hw_resetting = 1;

Race against reset on another CPU.
--
vda