cvs commit: src/sys/dev/iwi if_iwi.c if_iwireg.h if_iwivar.h

[Sam Leffler]

Damien Bergamini wrote:
> | You appear to have added a private sta table so that you could have an 
> | index for each node table entry.  However I don't see any place where 
> | you index into the table; only that you need the index to pass to the 
> | firmware.  This seems to say you only need a per-node index and that can 
> | be done differently (see below).
> | 
> | FWIW, the intended way to maange per-node driver-private data like this 
> | is for the driver to extend the node data structure with a private 
> | definition (override the class decl in c++ parlance) and store the index 
> | in the private data area.  This could eliminate the table and avoid the 
> | problems you've introduced by holding uncounted references to node data 
> | structures.  It also avoids likely race conditions that'll result from 
> | having an unlocked data structure that overlaps scope with the net80211 
> | node table (I did something similar in the ath driver only to find out 
> | the hard way it was a mistake).
> 
> No, you missed the point.  This is not a table of ieee80211_node's, but
> just a table of the neighbours mac addresses.  Thus there is no problem
> with reference counting, locking and such.
> The iwi_find_txnode() function just looks for a mac address in the table
> and returns its index (an entry is created if it was not already existing).

Sorry, you're correct, these are mac addresses and not node references. 
  But the suggestion still holds.  You've got a separate piece of 
per-node  information that logically belongs in a driver-private area of 
the node.  Having it there would eliminate the table lookup; you just 
take the node pointer and deref to get the index.  The intent of 
driver-private node storage is to eliminate add-on tables like this.

> 
> | This commit msg didn't indicate that you also made changes for WME 
> | support.  You appear to disallow QoS encapsulation of EAPOL frames but 
> | this is already done in ieee80211_encap.  ieee80211_classify should 
> | assign an AC for all frames so use M_WME_GETAC(m) and don't make 
> | driver-local decisions unless they are required by the h/w (hard to tell 
> | from just looking at a diff).
> 
> No.  I need to decide on which Tx ring the packet will be sent.
> And I must decide this before calling ieee80211_encap() because in case
> the ring is full, I want to put the packet back to the if_snd queue.
> Unfortunately, the check against EAPOL frames is done in ieee80211_encap(),
> not in ieee80211_classify().  If I use M_WME_GETAC(m) and the frame is
> an EAPOL frame, I'll end up checking for free space in the wrong ring.

Thanks again, that'll teach me to read diffs instead of the final code. 
   ath has no limits on the h/w tx queues, the limits are imposed by the 
s/w buffers that back each entry so I never needed this.  Perhaps we 
should move the EAPOL check to the classify code as it's required that 
you classify packets before doing the encap.

> 
> | I suggest that MFC'ing some of these changes so quickly are a bad idea.
> 
> I don't think so.  The WME changes have been in 7-CURRENT for a while now
> and the new changes are not likely to break anything.  Most of the new
> code will not be triggered if IBSS mode is not active.
> I'd like this diff to make its way in BETA5 so it can be tested by many.
> Moreover it fixes the association problems with APs hiding their SSID.
>

Note that when I MFC'd changes in this driver recently that I did not 
MFC any of your WME mods.  My suggestion was that you not MFC _some_ of 
the changes; not things like fixing hidden ap handling.  re is the final 
arbiter of what can be MFC'd.

	Sam