Date: Tue, 6 Mar 2012 20:05:15 +0100 From: Bernhard Schmidt <bschmidt@freebsd.org> To: Mitsuru IWASAKI <iwasaki@jp.freebsd.org> Cc: adrian@freebsd.org, freebsd-current@freebsd.org, freebsd-wireless@freebsd.org Subject: Re: patches for if_iwi and wlan for WEP mode Message-ID: <201203062005.15276.bschmidt@freebsd.org> In-Reply-To: <20120307.023046.27956263.iwasaki@jp.FreeBSD.org> References: <20120306.024212.108736612.iwasaki@jp.FreeBSD.org> <201203052314.22050.bschmidt@freebsd.org> <20120307.023046.27956263.iwasaki@jp.FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
>
> ----
> @@ -1357,8 +1365,8 @@
> 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@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@
> keep giving pressure to me `Your src commit bit is still idle.' for
> long time :)
> I just want to stop it.
;)
--
Bernhard
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201203062005.15276.bschmidt>