Re: patches for if_iwi and wlan for WEP mode

On Tuesday 06 March 2012 18:30:46 Mitsuru IWASAKI wrote:
> Thanks Bernhard and Adrian, I think the problem seems to be solved.
> 
> > > My patches set IEEE80211_NODE_ASSOCID bit only if ni->ni_associd
> > > is set.  Any suggestions on this part are welcome.
> > 
> > Are you sure the net80211 part is correct? It looks to me as if you
> > are just masking the real issue. The IEEE80211_NODE_ASSOCID flag is
> > ment to be used to verify that an associd has actually been set, not
> > doing so will break other things I guess. iwi(4) is a bit tricky in
> > that regard, as it sets the associd itself, check iwi_checkforqos().
> > I'd verify that function is actually called and if so if the parameters
> > are correct. I fumbled around there once, might have wrong WEP..
> 
> As you suggested, iwi_checkforqos() has problems, wrong asresp
> frame parsing.
> 
> ----
> _at__at_ -1357,8 +1365,8 _at__at_
>  	frm += 2;
>  
>  	wme = NULL;
> -	while (frm < efrm) {
> -		IEEE80211_VERIFY_LENGTH(efrm - frm, frm[1], return);
> +	while (efrm - frm > 1) {
> +		IEEE80211_VERIFY_LENGTH(efrm - frm, frm[1] + 2, return);
>  		switch (*frm) {
>  		case IEEE80211_ELEMID_VENDOR:
>  			if (iswmeoui(frm))
> ----
> 
> Bacause of the condition `while (frm < efrm)',
> IEEE80211_VERIFY_LENGTH() was checking item length beyond the
> ieee80211_frame region, and returned from iwi_checkforqos() without
> setting flags, capinfo and associd!
> I made above changes referring to net80211 code such as
> ieee80211_sta.c.
> 
> Today's version of patches at:
> http://people.freebsd.org/~iwasaki/iwi/iwi-20120306.diff
> 
> This one don't have changes on net80211 part at all.

Looks good to me, please get that into the tree.

> > What's the reason behing adding if_qflush()/if_transmit()?
> 
> In RELENG_7, data frame is transmitted by iwi_tx_start() like this.
> 
>   ether_output
>     ether_output_frame
>       IFQ_HANDOFF/IFQ_HANDOFF_ADJ
>         if_start
>           iwi_start
>             iwi_tx_start
> 
> After 8.0-RELEASE, device specific if_transmit() is called via net80211 layer.
> 
>   ether_output
>     ether_output_frame
>       if_transmit
>         IFQ_HANDOFF/IFQ_HANDOFF_ADJ
>           if_start
>             ieee80211_start
>               parent->if_transmit(ie. iwi_transmit())
> 
> There was not if_transmit method in iwi(4), so I add it.
> On if_qflush(), CURRENT kernel complains that `transmit and qflush
> must both either be set or both be NULL' from if.c.
> I wrote iwi_qflush(), but actually never tested it...

Hmm, it still is the case for >= 8 afaik, there is a default
if_transmit() which is used for all wireless drivers which seems to
work pretty well. That's why I'm wondering, iwi(4) would be the first
driver to have it's own if_transmit() function. I'm not aware of any
technical reason for adding one, or did I miss something? If not I'd
rather not have one added, for sake of consistency.

> From: Adrian Chadd <adrian_at_freebsd.org>
> > Would you please open a PR with this particular issue and then attach
> > the patch to it?
> 
> I prefer committing changes on iwi(4) by myself, because grimreaper_at_
> keep giving pressure to me `Your src commit bit is still idle.' for
> long time :)
> I just want to stop it.

;)

-- 
Bernhard