Date: Tue, 6 Mar 2012 12:12:55 -0800 From: Adrian Chadd <adrian@freebsd.org> To: Bernhard Schmidt <bschmidt@freebsd.org> Cc: freebsd-current@freebsd.org, freebsd-wireless@freebsd.org Subject: Re: patches for if_iwi and wlan for WEP mode Message-ID: <CAJ-Vmo=P7LNm4rUpcoUPgerssBkSVifQ7Gdge3=Zps3v77pFhg@mail.gmail.com> In-Reply-To: <201203062005.15276.bschmidt@freebsd.org> References: <20120306.024212.108736612.iwasaki@jp.FreeBSD.org> <201203052314.22050.bschmidt@freebsd.org> <20120307.023046.27956263.iwasaki@jp.FreeBSD.org> <201203062005.15276.bschmidt@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
.. except that the default if_transmit handling breaks fragments. Sigh.
So we're going to have to implement if_transmit for all net80211
drivers soon and fix fragment handling.
Adrian
On 6 March 2012 11:05, Bernhard Schmidt <bschmidt@freebsd.org> wrote:
> 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. =A0Any 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 parameter=
s
>> > 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 @@
>> =A0 =A0 =A0 frm +=3D 2;
>>
>> =A0 =A0 =A0 wme =3D NULL;
>> - =A0 =A0 while (frm < efrm) {
>> - =A0 =A0 =A0 =A0 =A0 =A0 IEEE80211_VERIFY_LENGTH(efrm - frm, frm[1], re=
turn);
>> + =A0 =A0 while (efrm - frm > 1) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 IEEE80211_VERIFY_LENGTH(efrm - frm, frm[1] + 2=
, return);
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 switch (*frm) {
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 case IEEE80211_ELEMID_VENDOR:
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 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.
>>
>> =A0 ether_output
>> =A0 =A0 ether_output_frame
>> =A0 =A0 =A0 IFQ_HANDOFF/IFQ_HANDOFF_ADJ
>> =A0 =A0 =A0 =A0 if_start
>> =A0 =A0 =A0 =A0 =A0 iwi_start
>> =A0 =A0 =A0 =A0 =A0 =A0 iwi_tx_start
>>
>> After 8.0-RELEASE, device specific if_transmit() is called via net80211 =
layer.
>>
>> =A0 ether_output
>> =A0 =A0 ether_output_frame
>> =A0 =A0 =A0 if_transmit
>> =A0 =A0 =A0 =A0 IFQ_HANDOFF/IFQ_HANDOFF_ADJ
>> =A0 =A0 =A0 =A0 =A0 if_start
>> =A0 =A0 =A0 =A0 =A0 =A0 ieee80211_start
>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 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 >=3D 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?CAJ-Vmo=P7LNm4rUpcoUPgerssBkSVifQ7Gdge3=Zps3v77pFhg>