Hello,
It happened that since Emmanuel's commit
3afc2167f60a327a2c1e1e2600ef209a3c2b75b7 (cfg80211/mac80211: ignore
signal if the frame was heard on wrong channel) my atmel-based wifi
device couldn't scan anymore.
I looked at this issue and I found the cause, but currently I have
some doubts, and I would like asking for clarification and comments,
before trying to patch the driver or mac80211 itself....
The problem is triggered because the at76x50x-usb driver does not
provide frequency information in rx_status (and AFAIK it difficultly
could do that during scan right now).
Emmanuel's commit make certain mac80211 functions stop getting the
channel information from the beacon/probe response, and start getting
it from rx_status, but the channel results now NULL, and the frame is
discarded.
The relevant check is done in mlme.c, ieee80211_rx_bss_info
if (!channel)
return;
just after Emmanuel's change.
So my first question is:
Are mac80211 drivers obligated to report this information?
Does this commit just rely on something that should be already in that
way (and the at7950x-usb driver is simply buggy not doing this)?
Reading Emmanuel's commit message it seems he didn't have any
intention to introduce this constraint now.
Said that, I dig in mac80211 code, and I saw Emmanuel commit does
affect frames processed in sta (and ibss, but i didn't look at this)
rx path.
Summarizing it a bit, the call path should be:
__ieee80211_rx_handle_packet()
ieee80211_invoke_rx_handlers()
ieee80211_rx_h_mgmt()
-- work queue --
ieee80211_iface_work()
ieee80211_sta_rx_queued_mgmt()
ieee80211_rx_bss_info() - patched to get channel from driver's rx_status.
And finally
ieee80211_bss_info_update()
is this right?
What is surprising for me is that
__ieee80211_rx_handle_packet()
contains a shorter path for updating BSS information by
ieee80211_scan_rx() that directly calls
ieee80211_bss_info_update()
The interesting thing is that ieee80211_scan_rx was already written in
the way Emmanuel's patched ieee80211_rx_bss_info(): It get channel
info from rx_status.
This suggests that the at79c50x-usb driver, prior to Emmanuel's
commit, was able to scan by getting mgmt frames processed by the
former, longer, RX path, and that the ieee80211_scan_rx() path was
never really used.
If this is correct, my question is:
Isn't this a redundant path duplication (provided that drivers
supplies channel information)?
This seems to be confirmed by the fact that if I modify mac80211 to
avoid discarding frame without channel information (by putting a valid
channel information directly in mac80211 just before the check for
NULL), the scan works by either doing this in ieee80211_scan_rx() or
in ieee80211_rx_bss_info() (reverting Emmanuel change in mlme.c).
Finally my latest question is:
Emmanuel's patch makes cfg80211_inform_bss_width_frame() compare the
RX channel and the actual BSS channel (known from beacon/probe
response), and it does this by comparing the two pointers.
Are we guaranteed that only one instance of every channel object does
exists ? isn't it safer to compare the frequency field value?
Thank you
Andrea
On Mon, 2014-05-19 at 17:42 +0200, Andrea Merello wrote:
> About Johannes patch.. Looks good :) But I already tried to do almost
> the same thing in the at79 driver, but I failed, because despite
> setting the single channel and performing a bunch of HW scan (one for
> each ch), it happened that my HW did several full scans disregarding
> the channel setting.
Well, if it doesn't work we can go back to the mac80211 solution I
suppose. Although the better solution even then might be to at least
detect in the driver that we're in the scan, and then attempt to parse
the DS information in the driver, so that it works regardless of whether
mac80211 has both those long paths or not. That patch would also be
simpler.
johannes
On Mon, 2014-05-19 at 16:29 +0200, Johannes Berg wrote:
> Since there's no "rx channel" information in this driver, but there does
> seem to be a (currently unused) channel field in the scan request, maybe
> we can make the driver request up to 14 single-channel scans (rather
> than a single full scan), and then it can keep track of the current
> channel in software and assign the pointer properly.
>
> This would also allow implementing regulatory for scan properly, which
> seems like a good idea as well.
Maybe something like this works?
http://p.sipsolutions.net/fa75caf17c54747a.txt
johannes
PiBIZWxsbywNCj4gDQo+IEl0IGhhcHBlbmVkIHRoYXQgc2luY2UgRW1tYW51ZWwncyBjb21taXQN
Cj4gM2FmYzIxNjdmNjBhMzI3YTJjMWUxZTI2MDBlZjIwOWEzYzJiNzViNyAoY2ZnODAyMTEvbWFj
ODAyMTE6IGlnbm9yZQ0KPiBzaWduYWwgaWYgdGhlIGZyYW1lIHdhcyBoZWFyZCBvbiB3cm9uZyBj
aGFubmVsKSBteSBhdG1lbC1iYXNlZCB3aWZpDQo+IGRldmljZSBjb3VsZG4ndCBzY2FuIGFueW1v
cmUuDQo+IA0KPiBJIGxvb2tlZCBhdCB0aGlzIGlzc3VlIGFuZCBJIGZvdW5kIHRoZSBjYXVzZSwg
YnV0IGN1cnJlbnRseSBJIGhhdmUNCj4gc29tZSBkb3VidHMsIGFuZCBJIHdvdWxkIGxpa2UgYXNr
aW5nIGZvciBjbGFyaWZpY2F0aW9uIGFuZCBjb21tZW50cywNCj4gYmVmb3JlIHRyeWluZyB0byBw
YXRjaCB0aGUgZHJpdmVyIG9yIG1hYzgwMjExIGl0c2VsZi4uLi4NCj4gDQo+IFRoZSBwcm9ibGVt
IGlzIHRyaWdnZXJlZCBiZWNhdXNlIHRoZSBhdDc2eDUweC11c2IgZHJpdmVyIGRvZXMgbm90DQo+
IHByb3ZpZGUgZnJlcXVlbmN5IGluZm9ybWF0aW9uIGluIHJ4X3N0YXR1cyAoYW5kIEFGQUlLIGl0
IGRpZmZpY3VsdGx5DQo+IGNvdWxkIGRvIHRoYXQgZHVyaW5nIHNjYW4gcmlnaHQgbm93KS4NCg0K
SSdkIHNheSB0aGlzIGlzIGEgYmFkIGJlaGF2aW9yLCBidXQgSSBhbSBub3QgdGhlIGF1dGhvcml0
YXRpdmUgb3BpbmlvbiBoZXJlIDopDQoNCj4gDQo+IEVtbWFudWVsJ3MgY29tbWl0IG1ha2UgY2Vy
dGFpbiBtYWM4MDIxMSBmdW5jdGlvbnMgc3RvcCBnZXR0aW5nIHRoZQ0KPiBjaGFubmVsIGluZm9y
bWF0aW9uIGZyb20gdGhlIGJlYWNvbi9wcm9iZSByZXNwb25zZSwgYW5kIHN0YXJ0IGdldHRpbmcN
Cj4gaXQgZnJvbSByeF9zdGF0dXMsIGJ1dCB0aGUgY2hhbm5lbCByZXN1bHRzIG5vdyBOVUxMLCBh
bmQgdGhlIGZyYW1lIGlzDQo+IGRpc2NhcmRlZC4NCj4gVGhlIHJlbGV2YW50IGNoZWNrIGlzIGRv
bmUgaW4gbWxtZS5jLCBpZWVlODAyMTFfcnhfYnNzX2luZm8NCj4gaWYgKCFjaGFubmVsKQ0KPiAg
ICAgcmV0dXJuOw0KPiBqdXN0IGFmdGVyIEVtbWFudWVsJ3MgY2hhbmdlLg0KPiANCj4gU28gbXkg
Zmlyc3QgcXVlc3Rpb24gaXM6DQo+IEFyZSBtYWM4MDIxMSBkcml2ZXJzIG9ibGlnYXRlZCB0byBy
ZXBvcnQgdGhpcyBpbmZvcm1hdGlvbj8NCj4gRG9lcyB0aGlzIGNvbW1pdCBqdXN0IHJlbHkgb24g
c29tZXRoaW5nIHRoYXQgc2hvdWxkIGJlIGFscmVhZHkgaW4gdGhhdA0KPiB3YXkgKGFuZCB0aGUg
YXQ3OTUweC11c2IgZHJpdmVyIGlzIHNpbXBseSBidWdneSBub3QgZG9pbmcgdGhpcyk/DQo+IFJl
YWRpbmcgRW1tYW51ZWwncyBjb21taXQgbWVzc2FnZSBpdCBzZWVtcyBoZSBkaWRuJ3QgaGF2ZSBh
bnkNCj4gaW50ZW50aW9uIHRvIGludHJvZHVjZSB0aGlzIGNvbnN0cmFpbnQgbm93Lg0KDQpSaWdo
dCAtIGJ1dCB0aGUgYXQ3OTUweC11c2IgZG9lc24ndCBwcm92aWRlIHRoaXMgaW5mb3JtYXRpb24s
IHRoZW4gdGhlIGNoYW5uZWwgaXMgdGFrZW4gZnJvbSB0aGUgRFMgSUUgLSBvbmx5IGlmIGl0IGV4
aXN0cywgc28gd2hhdCBpZiB0aGlzIElFIGRvZXNuJ3QgZXhpc3Q/DQoNCj4gDQo+IFNhaWQgdGhh
dCwgSSBkaWcgaW4gbWFjODAyMTEgY29kZSwgYW5kIEkgc2F3IEVtbWFudWVsIGNvbW1pdCBkb2Vz
DQo+IGFmZmVjdCBmcmFtZXMgcHJvY2Vzc2VkIGluIHN0YSAoYW5kIGlic3MsIGJ1dCBpIGRpZG4n
dCBsb29rIGF0IHRoaXMpDQo+IHJ4IHBhdGguDQo+IA0KPiBTdW1tYXJpemluZyBpdCBhIGJpdCwg
dGhlIGNhbGwgcGF0aCBzaG91bGQgYmU6DQo+ICBfX2llZWU4MDIxMV9yeF9oYW5kbGVfcGFja2V0
KCkNCj4gaWVlZTgwMjExX2ludm9rZV9yeF9oYW5kbGVycygpDQo+IGllZWU4MDIxMV9yeF9oX21n
bXQoKQ0KPiAtLSB3b3JrIHF1ZXVlIC0tDQo+IGllZWU4MDIxMV9pZmFjZV93b3JrKCkNCj4gaWVl
ZTgwMjExX3N0YV9yeF9xdWV1ZWRfbWdtdCgpDQo+IGllZWU4MDIxMV9yeF9ic3NfaW5mbygpIC0g
cGF0Y2hlZCB0byBnZXQgY2hhbm5lbCBmcm9tIGRyaXZlcidzIHJ4X3N0YXR1cy4NCj4gQW5kIGZp
bmFsbHkNCj4gaWVlZTgwMjExX2Jzc19pbmZvX3VwZGF0ZSgpDQo+IA0KPiBpcyB0aGlzIHJpZ2h0
Pw0KPiANCj4gV2hhdCBpcyBzdXJwcmlzaW5nIGZvciBtZSBpcyB0aGF0DQo+ICBfX2llZWU4MDIx
MV9yeF9oYW5kbGVfcGFja2V0KCkNCj4gY29udGFpbnMgYSBzaG9ydGVyIHBhdGggZm9yIHVwZGF0
aW5nIEJTUyBpbmZvcm1hdGlvbiBieQ0KPiBpZWVlODAyMTFfc2Nhbl9yeCgpIHRoYXQgZGlyZWN0
bHkgY2FsbHMNCj4gaWVlZTgwMjExX2Jzc19pbmZvX3VwZGF0ZSgpDQoNClllcyAtIHRoZSBmcmFt
ZSBjb21lcyB0byB0aGUgYnNzIHVwZGF0ZSBmcm9tIDIgcGF0aHMgd2hlbiB3ZSBhcmUgc2Nhbm5p
bmcuDQoNCj4gDQo+IFRoZSBpbnRlcmVzdGluZyB0aGluZyBpcyB0aGF0IGllZWU4MDIxMV9zY2Fu
X3J4IHdhcyBhbHJlYWR5IHdyaXR0ZW4gaW4NCj4gdGhlIHdheSBFbW1hbnVlbCdzIHBhdGNoZWQg
aWVlZTgwMjExX3J4X2Jzc19pbmZvKCk6IEl0IGdldCBjaGFubmVsDQo+IGluZm8gZnJvbSByeF9z
dGF0dXMuDQo+IA0KPiBUaGlzIHN1Z2dlc3RzIHRoYXQgdGhlIGF0NzljNTB4LXVzYiBkcml2ZXIs
IHByaW9yIHRvIEVtbWFudWVsJ3MNCj4gY29tbWl0LCB3YXMgYWJsZSB0byBzY2FuIGJ5IGdldHRp
bmcgbWdtdCBmcmFtZXMgcHJvY2Vzc2VkIGJ5IHRoZQ0KPiBmb3JtZXIsIGxvbmdlciwgUlggcGF0
aCwgYW5kIHRoYXQgdGhlIGllZWU4MDIxMV9zY2FuX3J4KCkgcGF0aCB3YXMNCj4gbmV2ZXIgcmVh
bGx5IHVzZWQuDQoNClllcC4NCg0KPiANCj4gSWYgdGhpcyBpcyBjb3JyZWN0LCBteSBxdWVzdGlv
biBpczoNCj4gSXNuJ3QgdGhpcyBhIHJlZHVuZGFudCBwYXRoIGR1cGxpY2F0aW9uIChwcm92aWRl
ZCB0aGF0IGRyaXZlcnMNCj4gc3VwcGxpZXMgY2hhbm5lbCBpbmZvcm1hdGlvbik/DQoNClRoaXMg
aGFzIGJlZW4gZGlzY3Vzc2VkIGluIHRoZSBtYWlsaW5nIGxpc3QgYWxyZWFkeSwgYnV0IHllcy4N
Cg0KPiANCj4gVGhpcyBzZWVtcyB0byBiZSBjb25maXJtZWQgYnkgdGhlIGZhY3QgdGhhdCBpZiBJ
IG1vZGlmeSBtYWM4MDIxMSB0bw0KPiBhdm9pZCBkaXNjYXJkaW5nIGZyYW1lIHdpdGhvdXQgY2hh
bm5lbCBpbmZvcm1hdGlvbiAoYnkgcHV0dGluZyBhIHZhbGlkDQo+IGNoYW5uZWwgaW5mb3JtYXRp
b24gZGlyZWN0bHkgaW4gbWFjODAyMTEganVzdCBiZWZvcmUgdGhlIGNoZWNrIGZvcg0KPiBOVUxM
KSwgdGhlIHNjYW4gd29ya3MgYnkgZWl0aGVyIGRvaW5nIHRoaXMgaW4gaWVlZTgwMjExX3NjYW5f
cngoKSBvcg0KPiBpbiBpZWVlODAyMTFfcnhfYnNzX2luZm8oKSAocmV2ZXJ0aW5nIEVtbWFudWVs
IGNoYW5nZSBpbiBtbG1lLmMpLg0KDQpDYW4geW91IHRyeSB0aGlzOg0KDQpkaWZmIC0tZ2l0IGEv
bmV0L21hYzgwMjExL21sbWUuYyBiL25ldC9tYWM4MDIxMS9tbG1lLmMNCmluZGV4IDNmYjA3ZTku
LjlkMmYwZDAgMTAwNjQ0DQotLS0gYS9uZXQvbWFjODAyMTEvbWxtZS5jDQorKysgYi9uZXQvbWFj
ODAyMTEvbWxtZS5jDQpAQCAtMjc5MSw4ICsyNzkxLDEzIEBAIHN0YXRpYyB2b2lkIGllZWU4MDIx
MV9yeF9ic3NfaW5mbyhzdHJ1Y3QgaWVlZTgwMjExX3N1Yl9pZl9kYXRhICpzZGF0YSwNCiAgICAg
ICAgc2RhdGFfYXNzZXJ0X2xvY2soc2RhdGEpOw0KIA0KICAgICAgICBjaGFubmVsID0gaWVlZTgw
MjExX2dldF9jaGFubmVsKGxvY2FsLT5ody53aXBoeSwgcnhfc3RhdHVzLT5mcmVxKTsNCi0gICAg
ICAgaWYgKCFjaGFubmVsKQ0KLSAgICAgICAgICAgICAgIHJldHVybjsNCisgICAgICAgaWYgKCFj
aGFubmVsICYmIGVsZW1zLT5kc19wYXJhbXMpIHsNCisgICAgICAgICAgICAgICBpbnQgZnJlcSA9
IGllZWU4MDIxMV9jaGFubmVsX3RvX2ZyZXF1ZW5jeShlbGVtcy0+ZHNfcGFyYW1zWzBdLA0KKyAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIHJ4
X3N0YXR1cy0+YmFuZCk7DQorICAgICAgICAgICAgICAgY2hhbm5lbCA9IGllZWU4MDIxMV9nZXRf
Y2hhbm5lbChsb2NhbC0+aHcud2lwaHksIGZyZXEpOw0KKyAgICAgICAgICAgICAgIGlmICghY2hh
bm5lbCB8fCBjaGFubmVsLT5mbGFncyAmIElFRUU4MDIxMV9DSEFOX0RJU0FCTEVEKQ0KKyAgICAg
ICAgICAgICAgICAgICAgICAgcmV0dXJuOw0KKyAgICAgICB9DQogDQogICAgICAgIGJzcyA9IGll
ZWU4MDIxMV9ic3NfaW5mb191cGRhdGUobG9jYWwsIHJ4X3N0YXR1cywgbWdtdCwgbGVuLCBlbGVt
cywNCiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBjaGFubmVsKTsNCg0K
PiANCj4gRmluYWxseSBteSBsYXRlc3QgcXVlc3Rpb24gaXM6DQo+IEVtbWFudWVsJ3MgcGF0Y2gg
bWFrZXMgY2ZnODAyMTFfaW5mb3JtX2Jzc193aWR0aF9mcmFtZSgpIGNvbXBhcmUgdGhlDQo+IFJY
IGNoYW5uZWwgYW5kIHRoZSBhY3R1YWwgQlNTIGNoYW5uZWwgKGtub3duIGZyb20gYmVhY29uL3By
b2JlDQo+IHJlc3BvbnNlKSwgYW5kIGl0IGRvZXMgdGhpcyBieSBjb21wYXJpbmcgdGhlIHR3byBw
b2ludGVycy4NCj4gDQo+IEFyZSB3ZSBndWFyYW50ZWVkIHRoYXQgb25seSBvbmUgaW5zdGFuY2Ug
b2YgZXZlcnkgY2hhbm5lbCBvYmplY3QgZG9lcw0KPiBleGlzdHMgPyBpc24ndCBpdCBzYWZlciB0
byBjb21wYXJlIHRoZSBmcmVxdWVuY3kgZmllbGQgdmFsdWU/DQoNCk5vIC0gdGhpcyBpcyBmaW5l
LiBXZSBkbyB0aGF0IGFsbCBvdmVyIHRoZSBzdGFjay4NCg0KDQo=
Hello,
Johannes, I tested your patch:
Unfortunately it seems it does the same as mine..
In order to test it, I tried to hack the code (with your patch
applied) with only one fixed channel number hardcoded in the scan
command (thus, the scan will repeat for several times, and the SW
believes to scan one channel per time, but I forced the scan command
to stick only on one channel every time).
What's happened is that I still can see the usual huge BSS list, that
includes even BSS from far channels..
Furthermore, by sniffing on a fixed channel with another reference
card, I could see several probe-requests from the at56x703, some with
a very strong signal, and some other with really weaker signal,
possibly suggesting the card is sweeping, and they have been send on
some nearby channel..
Finally I (quickly) tried monitor mode on at56x703, that uses the same
trick to change channel. Maybe it's worth to do more tests on it to
confirm this, BTW It seems that changing channel in monitor mode does
something, and, by looking at a live capture with wireshark, it seems
even that the card finally stays on a channel (altought I can't say if
it does this after a quick sweep).. But anyway it didn't work as I
expected: I didn't received what I expected, and it seems that, at
least, it sets on the wrong channel.
Indeed there is some confusion about what the scan command with
specified channel does (at least for me)..
If some of the driver's authors (in CC ) that have more knowledge of
the HW/FW can enlight me, then it is maybe possible not only to fix
step-by-step scanning, but also the "monitor" mode (provided that it
does not turn out that it actually works, and that my quick test was
broken..).
If this is not the case (no one knowns/tells) IMHO the two choice are:
- do what we said: parse beacons to extract channel information (and
let monitor mode stay as it is)
- do more investigation trying to fix also monitor mode issue.
I could do some work on this (including opening the dongle to see if I
can put HW probes on the RF chip, to see on with frequency the FW
tunes it when scan commands are sent).
Also AFAIK there is a new firmware in the off-tree Atmel driver, that
AFAIK also support WPA.
If this turns out to be true, then I'm tempted to rework the driver
for using it (hoping the the scan command works better on it).
FYI :
Honestly I'm unsure if it's worth to do these last things:
On one hand it is a very old device, and I think it has few users out
of there.. And probably I could do more interesting things.
On the other hand I have one of this devices and I still use it :) And
indeed no one pay me for my work on Linux drivers, so I have no
deadlines or mandatory jobs to do, and as long as it does amuse me
it's a fair game :)
Andrea
On Mon, May 19, 2014 at 6:05 PM, Andrea Merello
<[email protected]> wrote:
> Yes, I agree.. I thougth the same thing: parsing
> beacons/probe-response in the driver was also my last resort :)
>
> Andrea
>
> On Mon, May 19, 2014 at 6:02 PM, Johannes Berg
> <[email protected]> wrote:
>> On Mon, 2014-05-19 at 17:42 +0200, Andrea Merello wrote:
>>
>>> About Johannes patch.. Looks good :) But I already tried to do almost
>>> the same thing in the at79 driver, but I failed, because despite
>>> setting the single channel and performing a bunch of HW scan (one for
>>> each ch), it happened that my HW did several full scans disregarding
>>> the channel setting.
>>
>> Well, if it doesn't work we can go back to the mac80211 solution I
>> suppose. Although the better solution even then might be to at least
>> detect in the driver that we're in the scan, and then attempt to parse
>> the DS information in the driver, so that it works regardless of whether
>> mac80211 has both those long paths or not. That patch would also be
>> simpler.
>>
>> johannes
>>
Yes, I agree.. I thougth the same thing: parsing
beacons/probe-response in the driver was also my last resort :)
Andrea
On Mon, May 19, 2014 at 6:02 PM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2014-05-19 at 17:42 +0200, Andrea Merello wrote:
>
>> About Johannes patch.. Looks good :) But I already tried to do almost
>> the same thing in the at79 driver, but I failed, because despite
>> setting the single channel and performing a bunch of HW scan (one for
>> each ch), it happened that my HW did several full scans disregarding
>> the channel setting.
>
> Well, if it doesn't work we can go back to the mac80211 solution I
> suppose. Although the better solution even then might be to at least
> detect in the driver that we're in the scan, and then attempt to parse
> the DS information in the driver, so that it works regardless of whether
> mac80211 has both those long paths or not. That patch would also be
> simpler.
>
> johannes
>
Hello,
I found some useful helper function in mac80211, and I used some of
them, but unfortunately I finally parsed information element in
beacon/probe-response by myself:
I found the utility ieee802_11_parse_elems(), but it seems that it
relies on structures that are declared in some .h file located locally
to mac80211.. So I suppose it is not intended for use outside
mac80211, is it?
I did a simpler implementation that only search for channel information..
So.. What about the following patch? :)
Andrea
drivers/net/wireless/at76c50x-usb.c | 57 +++++++++++++++++++++++++++++++++++++
1 file changed, 57 insertions(+)
diff --git a/drivers/net/wireless/at76c50x-usb.c
b/drivers/net/wireless/at76c50x-usb.c
index 10fd12e..06a64f4 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -1502,6 +1502,61 @@ static void at76_work_submit_rx(struct work_struct *work)
mutex_unlock(&priv->mtx);
}
+/* This is a workaround to make scan working:
+ * currently mac80211 does not process frames with no frequency
+ * information.
+ * However during scan the HW performs a sweep by itself, and we
+ * are unable to know where the radio is actually tuned.
+ * This function tries to do its best to guess this information..
+ * If the current frame is a beacon or a probe response, the channel
+ * information is extracted from it.
+ * For other frames, or if it happen that for whatever reason we fail
+ * to parse beacons and probe responses, this function returns
+ * the priv->channel information, that should be valid at least
+ * when we are NOT scanning.
+ */
+static int at76_parse_freq(struct at76_priv *priv)
+{
+ size_t el_off;
+ u8 id;
+ u8 *el;
+ int el_len;
+ int channel = priv->channel;
+ int len = priv->rx_skb->len;
+ struct ieee80211_hdr *hdr = (void *)priv->rx_skb->data;
+
+ if (len < 24)
+ goto exit;
+
+ if (ieee80211_is_probe_resp(hdr->frame_control)) {
+ el_off = offsetof(struct ieee80211_mgmt, u.probe_resp.variable);
+ el = ((struct ieee80211_mgmt *)hdr)->u.probe_resp.variable;
+ } else if (ieee80211_is_beacon(hdr->frame_control)) {
+ el_off = offsetof(struct ieee80211_mgmt, u.beacon.variable);
+ el = ((struct ieee80211_mgmt *)hdr)->u.beacon.variable;
+ } else
+ goto exit;
+
+ len -= el_off;
+
+ while (len >= 2) {
+ id = *el++;
+ el_len = *el++;
+ len -= 2;
+
+ if (id == WLAN_EID_DS_PARAMS) {
+ if ((el_len > 0) && (len >= el_len))
+ channel = *el;
+ break;
+ }
+ len -= el_len;
+ el += el_len;
+ }
+
+exit:
+ return ieee80211_channel_to_frequency(channel, IEEE80211_BAND_2GHZ);
+}
+
static void at76_rx_tasklet(unsigned long param)
{
struct urb *urb = (struct urb *)param;
@@ -1542,6 +1597,8 @@ static void at76_rx_tasklet(unsigned long param)
rx_status.signal = buf->rssi;
rx_status.flag |= RX_FLAG_DECRYPTED;
rx_status.flag |= RX_FLAG_IV_STRIPPED;
+ rx_status.band = IEEE80211_BAND_2GHZ;
+ rx_status.freq = at76_parse_freq(priv);
at76_dbg(DBG_MAC80211, "calling ieee80211_rx_irqsafe(): %d/%d",
priv->rx_skb->len, priv->rx_skb->data_len);
--
1.9.1
On Wed, May 28, 2014 at 4:02 PM, Johannes Berg
<[email protected]> wrote:
> Hi Andrea,
>
>> Unfortunately it seems it does the same as mine..
>
> Ok ... fair enough.
>
>> If this is not the case (no one knowns/tells) IMHO the two choice are:
>> - do what we said: parse beacons to extract channel information (and
>> let monitor mode stay as it is)
>
> Probably the best course of action for now. I'd make this conditional on
> receiving probe response/beacon, there should be helper functions for
> most of this. Since the chip is 2.4 GHz only you also don't have any
> problems with band selection stuff.
>
>> - do more investigation trying to fix also monitor mode issue.
>
> That could be done as a follow-up, I guess.
>
>> I could do some work on this (including opening the dongle to see if I
>> can put HW probes on the RF chip, to see on with frequency the FW
>> tunes it when scan commands are sent).
>>
>> Also AFAIK there is a new firmware in the off-tree Atmel driver, that
>> AFAIK also support WPA.
>> If this turns out to be true, then I'm tempted to rework the driver
>> for using it (hoping the the scan command works better on it).
>>
>> FYI :
>> Honestly I'm unsure if it's worth to do these last things:
>
> Whatever you want to do :-)
>
> johannes
>
Hi all (big cc list...),
> It happened that since Emmanuel's commit
> 3afc2167f60a327a2c1e1e2600ef209a3c2b75b7 (cfg80211/mac80211: ignore
> signal if the frame was heard on wrong channel) my atmel-based wifi
> device couldn't scan anymore.
That driver is a pain! :-)
> The problem is triggered because the at76x50x-usb driver does not
> provide frequency information in rx_status (and AFAIK it difficultly
> could do that during scan right now).
Why is that difficult? Ok actually, looking at the driver, I see. It has
HW scan but nothing indicating the channel.
> So my first question is:
> Are mac80211 drivers obligated to report this information?
> Does this commit just rely on something that should be already in that
> way (and the at7950x-usb driver is simply buggy not doing this)?
> Reading Emmanuel's commit message it seems he didn't have any
> intention to introduce this constraint now.
Yeah, every other driver is always unconditionally setting the channel,
so this is very surprising.
> Said that, I dig in mac80211 code, and I saw Emmanuel commit does
> affect frames processed in sta (and ibss, but i didn't look at this)
> rx path.
>
> Summarizing it a bit, the call path should be:
> __ieee80211_rx_handle_packet()
> ieee80211_invoke_rx_handlers()
> ieee80211_rx_h_mgmt()
> -- work queue --
> ieee80211_iface_work()
> ieee80211_sta_rx_queued_mgmt()
> ieee80211_rx_bss_info() - patched to get channel from driver's rx_status.
> And finally
> ieee80211_bss_info_update()
>
> is this right?
>
> What is surprising for me is that
> __ieee80211_rx_handle_packet()
> contains a shorter path for updating BSS information by
> ieee80211_scan_rx() that directly calls
> ieee80211_bss_info_update()
>
> The interesting thing is that ieee80211_scan_rx was already written in
> the way Emmanuel's patched ieee80211_rx_bss_info(): It get channel
> info from rx_status.
Right - this is the real scan path. The other one is a bit of a
confusing path, I've kinda wanted to get rid of the former one but there
are some corner cases with that as far as I remember. Maybe with careful
analysis we can get rid of it.
> This suggests that the at79c50x-usb driver, prior to Emmanuel's
> commit, was able to scan by getting mgmt frames processed by the
> former, longer, RX path, and that the ieee80211_scan_rx() path was
> never really used.
And had I removed the former path that would have broken the driver :)
> If this is correct, my question is:
> Isn't this a redundant path duplication (provided that drivers
> supplies channel information)?
Yes.
> Finally my latest question is:
> Emmanuel's patch makes cfg80211_inform_bss_width_frame() compare the
> RX channel and the actual BSS channel (known from beacon/probe
> response), and it does this by comparing the two pointers.
>
> Are we guaranteed that only one instance of every channel object does
> exists ? isn't it safer to compare the frequency field value?
This is perfectly fine (though Emmanuel's response that we do it
elsewhere is kinda useless - that might still be a bug :) ) since
there's only one set of channel pointers for each hardware. Those point
to the array entries in (for this driver) at76_channels[].
Since there's no "rx channel" information in this driver, but there does
seem to be a (currently unused) channel field in the scan request, maybe
we can make the driver request up to 14 single-channel scans (rather
than a single full scan), and then it can keep track of the current
channel in software and assign the pointer properly.
This would also allow implementing regulatory for scan properly, which
seems like a good idea as well.
johannes
Emmanuel, Johannes,
Thank you for your answer.
Now that it has been clearly stated that driver must report this
information, I obviously throw away the idea of modifying mac80211 :)
BTW FYI what emmanuel suggested in the last mail seems very similar to
what I tried to do as experiment, and that made the scan works again
:)
About Johannes patch.. Looks good :) But I already tried to do almost
the same thing in the at79 driver, but I failed, because despite
setting the single channel and performing a bunch of HW scan (one for
each ch), it happened that my HW did several full scans disregarding
the channel setting.
But it might be MY bug! I hope it :)
I saw the driver uses the same idea for implementing "monitor mode",
so I thought it should work (but I have not tried yet with "monitor
mode")...
As soon as I have time I'll try Johannes patch (and FYI I will also
try to finish some patches about WEP broken and DMA from stack memory)
and I will post results..
Thank you all :)
Andrea
On Mon, May 19, 2014 at 4:49 PM, Johannes Berg
<[email protected]> wrote:
> On Mon, 2014-05-19 at 16:29 +0200, Johannes Berg wrote:
>
>> Since there's no "rx channel" information in this driver, but there does
>> seem to be a (currently unused) channel field in the scan request, maybe
>> we can make the driver request up to 14 single-channel scans (rather
>> than a single full scan), and then it can keep track of the current
>> channel in software and assign the pointer properly.
>>
>> This would also allow implementing regulatory for scan properly, which
>> seems like a good idea as well.
>
> Maybe something like this works?
>
> http://p.sipsolutions.net/fa75caf17c54747a.txt
>
> johannes
>
Hi Andrea,
> Unfortunately it seems it does the same as mine..
Ok ... fair enough.
> If this is not the case (no one knowns/tells) IMHO the two choice are:
> - do what we said: parse beacons to extract channel information (and
> let monitor mode stay as it is)
Probably the best course of action for now. I'd make this conditional on
receiving probe response/beacon, there should be helper functions for
most of this. Since the chip is 2.4 GHz only you also don't have any
problems with band selection stuff.
> - do more investigation trying to fix also monitor mode issue.
That could be done as a follow-up, I guess.
> I could do some work on this (including opening the dongle to see if I
> can put HW probes on the RF chip, to see on with frequency the FW
> tunes it when scan commands are sent).
>
> Also AFAIK there is a new firmware in the off-tree Atmel driver, that
> AFAIK also support WPA.
> If this turns out to be true, then I'm tempted to rework the driver
> for using it (hoping the the scan command works better on it).
>
> FYI :
> Honestly I'm unsure if it's worth to do these last things:
Whatever you want to do :-)
johannes
Hi Andrea,
On Sat, 2014-05-31 at 20:54 +0200, Andrea Merello wrote:
> Hello,
> I found some useful helper function in mac80211, and I used some of
> them, but unfortunately I finally parsed information element in
> beacon/probe-response by myself:
> I found the utility ieee802_11_parse_elems(), but it seems that it
> relies on structures that are declared in some .h file located locally
> to mac80211.. So I suppose it is not intended for use outside
> mac80211, is it?
Yeah, and it would do much more than what you want. You could use
cfg80211_find_ie() though to simplify the code.
> +/* This is a workaround to make scan working:
> + * currently mac80211 does not process frames with no frequency
> + * information.
> + * However during scan the HW performs a sweep by itself, and we
> + * are unable to know where the radio is actually tuned.
> + * This function tries to do its best to guess this information..
> + * If the current frame is a beacon or a probe response, the channel
> + * information is extracted from it.
> + * For other frames, or if it happen that for whatever reason we fail
> + * to parse beacons and probe responses, this function returns
> + * the priv->channel information, that should be valid at least
> + * when we are NOT scanning.
> + */
> +static int at76_parse_freq(struct at76_priv *priv)
> +{
> + size_t el_off;
> + u8 id;
> + u8 *el;
> + int el_len;
> + int channel = priv->channel;
> + int len = priv->rx_skb->len;
> + struct ieee80211_hdr *hdr = (void *)priv->rx_skb->data;
> +
> + if (len < 24)
> + goto exit;
> +
> + if (ieee80211_is_probe_resp(hdr->frame_control)) {
> + el_off = offsetof(struct ieee80211_mgmt, u.probe_resp.variable);
> + el = ((struct ieee80211_mgmt *)hdr)->u.probe_resp.variable;
> + } else if (ieee80211_is_beacon(hdr->frame_control)) {
> + el_off = offsetof(struct ieee80211_mgmt, u.beacon.variable);
> + el = ((struct ieee80211_mgmt *)hdr)->u.beacon.variable;
> + } else
> + goto exit;
> +
> + len -= el_off;
> +
> + while (len >= 2) {
> + id = *el++;
> + el_len = *el++;
> + len -= 2;
> +
> + if (id == WLAN_EID_DS_PARAMS) {
> + if ((el_len > 0) && (len >= el_len))
> + channel = *el;
> + break;
> + }
> + len -= el_len;
> + el += el_len;
> + }
So the loop can be simplified to
el = cfg80211_find_ie(WLAN_EID_DS_PARAMS, el, el_len);
if (el && el[1] > 0)
channel = el[2];
> @@ -1542,6 +1597,8 @@ static void at76_rx_tasklet(unsigned long param)
> rx_status.signal = buf->rssi;
> rx_status.flag |= RX_FLAG_DECRYPTED;
> rx_status.flag |= RX_FLAG_IV_STRIPPED;
> + rx_status.band = IEEE80211_BAND_2GHZ;
> + rx_status.freq = at76_parse_freq(priv);
You might also invoke this only when you know you're scanning, to avoid
the overhead otherwise, but with this driver it probably doesn't matter
much.
johannes
> So this is the updated patch.
> If you agree, we could push it for merge :)
Yeah, looks fine. Minor code style comments below:
> +static inline int at76_guess_freq(struct at76_priv *priv)
> +{
> + size_t el_off;
should have tabs for indentation
> + const u8 *el;
> + int channel = priv->channel;
> + int len = priv->rx_skb->len;
> + struct ieee80211_hdr *hdr = (void *)priv->rx_skb->data;
> +
> + if (!priv->scanning)
> + goto exit;
> +
> + if (len < 24)
> + goto exit;
> +
> + if (ieee80211_is_probe_resp(hdr->frame_control)) {
> + el_off = offsetof(struct ieee80211_mgmt, u.probe_resp.variable);
> + el = ((struct ieee80211_mgmt *)hdr)->u.probe_resp.variable;
> + } else if (ieee80211_is_beacon(hdr->frame_control)) {
> + el_off = offsetof(struct ieee80211_mgmt, u.beacon.variable);
> + el = ((struct ieee80211_mgmt *)hdr)->u.beacon.variable;
> + } else
> + goto exit;
should have braces around all branches
johannes
Hi Johannes,
> Yeah, and it would do much more than what you want. You could use
> cfg80211_find_ie() though to simplify the code.
Excellent! I missed it..
> So the loop can be simplified to
>
> el = cfg80211_find_ie(WLAN_EID_DS_PARAMS, el, el_len);
> if (el && el[1] > 0)
> channel = el[2];
I guess el_len should be len, but I got it :)
> You might also invoke this only when you know you're scanning, to avoid
> the overhead otherwise, but with this driver it probably doesn't matter
> much.
Probably it doesn't, but you are right that is useless to waste time
parsing those mgmt frames when not scanning.
I modified the function (and I changed also its name) in a way that
when scanning it avoid parsing and it does return priv->channel, and I
made it also inline.
So this is the updated patch.
If you agree, we could push it for merge :)
Thank you
Andrea
drivers/net/wireless/at76c50x-usb.c | 53 +++++++++++++++++++++++++++++++++++++
drivers/net/wireless/at76c50x-usb.h | 1 +
2 files changed, 54 insertions(+)
diff --git a/drivers/net/wireless/at76c50x-usb.c
b/drivers/net/wireless/at76c50x-usb.c
index 10fd12e..6f52633 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -1429,6 +1429,8 @@ static int at76_startup_device(struct at76_priv *priv)
/* remove BSSID from previous run */
memset(priv->bssid, 0, ETH_ALEN);
+ priv->scanning = false;
+
if (at76_set_radio(priv, 1) == 1)
at76_wait_completion(priv, CMD_RADIO_ON);
@@ -1502,6 +1504,52 @@ static void at76_work_submit_rx(struct work_struct *work)
mutex_unlock(&priv->mtx);
}
+/* This is a workaround to make scan working:
+ * currently mac80211 does not process frames with no frequency
+ * information.
+ * However during scan the HW performs a sweep by itself, and we
+ * are unable to know where the radio is actually tuned.
+ * This function tries to do its best to guess this information..
+ * During scan, If the current frame is a beacon or a probe response,
+ * the channel information is extracted from it.
+ * When not scanning, for other frames, or if it happens that for
+ * whatever reason we fail to parse beacons and probe responses, this
+ * function returns the priv->channel information, that should be correct
+ * at least when we are not scanning.
+ */
+static inline int at76_guess_freq(struct at76_priv *priv)
+{
+ size_t el_off;
+ const u8 *el;
+ int channel = priv->channel;
+ int len = priv->rx_skb->len;
+ struct ieee80211_hdr *hdr = (void *)priv->rx_skb->data;
+
+ if (!priv->scanning)
+ goto exit;
+
+ if (len < 24)
+ goto exit;
+
+ if (ieee80211_is_probe_resp(hdr->frame_control)) {
+ el_off = offsetof(struct ieee80211_mgmt, u.probe_resp.variable);
+ el = ((struct ieee80211_mgmt *)hdr)->u.probe_resp.variable;
+ } else if (ieee80211_is_beacon(hdr->frame_control)) {
+ el_off = offsetof(struct ieee80211_mgmt, u.beacon.variable);
+ el = ((struct ieee80211_mgmt *)hdr)->u.beacon.variable;
+ } else
+ goto exit;
+
+ len -= el_off;
+
+ el = cfg80211_find_ie(WLAN_EID_DS_PARAMS, el, len);
+ if (el && el[1] > 0)
+ channel = el[2];
+
+exit:
+ return ieee80211_channel_to_frequency(channel, IEEE80211_BAND_2GHZ);
+}
+
static void at76_rx_tasklet(unsigned long param)
{
struct urb *urb = (struct urb *)param;
@@ -1542,6 +1590,8 @@ static void at76_rx_tasklet(unsigned long param)
rx_status.signal = buf->rssi;
rx_status.flag |= RX_FLAG_DECRYPTED;
rx_status.flag |= RX_FLAG_IV_STRIPPED;
+ rx_status.band = IEEE80211_BAND_2GHZ;
+ rx_status.freq = at76_guess_freq(priv);
at76_dbg(DBG_MAC80211, "calling ieee80211_rx_irqsafe(): %d/%d",
priv->rx_skb->len, priv->rx_skb->data_len);
@@ -1894,6 +1944,8 @@ static void at76_dwork_hw_scan(struct work_struct *work)
if (is_valid_ether_addr(priv->bssid))
at76_join(priv);
+ priv->scanning = false;
+
mutex_unlock(&priv->mtx);
ieee80211_scan_completed(priv->hw, false);
@@ -1948,6 +2000,7 @@ static int at76_hw_scan(struct ieee80211_hw *hw,
goto exit;
}
+ priv->scanning = true;
ieee80211_queue_delayed_work(priv->hw, &priv->dwork_hw_scan,
SCAN_POLL_INTERVAL);
diff --git a/drivers/net/wireless/at76c50x-usb.h
b/drivers/net/wireless/at76c50x-usb.h
index 4718aa5..55090a3 100644
--- a/drivers/net/wireless/at76c50x-usb.h
+++ b/drivers/net/wireless/at76c50x-usb.h
@@ -418,6 +418,7 @@ struct at76_priv {
int scan_max_time; /* scan max channel time */
int scan_mode; /* SCAN_TYPE_ACTIVE, SCAN_TYPE_PASSIVE */
int scan_need_any; /* if set, need to scan for any ESSID */
+ bool scanning; /* if set, the scan is running */
u16 assoc_id; /* current association ID, if associated */
--
1.9.1
since commit 3afc2167f60a327a2c1e1e2600ef209a3c2b75b7 scan in not
working anymore, due to mac80211 requires rx frequency status
information.
This patch makes the driver report this information.
While NOT scanning this is straightforward.
While scanning the firmware performs RF sweep and we cannot track
the actual tuning frequency, so this is guessed by parsing beacons
and probe responses.
This should be enough for ensuring functionality.
Thanks-to: Johannes Berg <[email protected]> [ for suggestions and reviewing ]
Signed-off-by: Andrea Merello <[email protected]>
---
drivers/net/wireless/at76c50x-usb.c | 53 +++++++++++++++++++++++++++++++++++++
drivers/net/wireless/at76c50x-usb.h | 1 +
2 files changed, 54 insertions(+)
diff --git a/drivers/net/wireless/at76c50x-usb.c b/drivers/net/wireless/at76c50x-usb.c
index 10fd12e..d48776e 100644
--- a/drivers/net/wireless/at76c50x-usb.c
+++ b/drivers/net/wireless/at76c50x-usb.c
@@ -1429,6 +1429,8 @@ static int at76_startup_device(struct at76_priv *priv)
/* remove BSSID from previous run */
memset(priv->bssid, 0, ETH_ALEN);
+ priv->scanning = false;
+
if (at76_set_radio(priv, 1) == 1)
at76_wait_completion(priv, CMD_RADIO_ON);
@@ -1502,6 +1504,52 @@ static void at76_work_submit_rx(struct work_struct *work)
mutex_unlock(&priv->mtx);
}
+/* This is a workaround to make scan working:
+ * currently mac80211 does not process frames with no frequency
+ * information.
+ * However during scan the HW performs a sweep by itself, and we
+ * are unable to know where the radio is actually tuned.
+ * This function tries to do its best to guess this information..
+ * During scan, If the current frame is a beacon or a probe response,
+ * the channel information is extracted from it.
+ * When not scanning, for other frames, or if it happens that for
+ * whatever reason we fail to parse beacons and probe responses, this
+ * function returns the priv->channel information, that should be correct
+ * at least when we are not scanning.
+ */
+static inline int at76_guess_freq(struct at76_priv *priv)
+{
+ size_t el_off;
+ const u8 *el;
+ int channel = priv->channel;
+ int len = priv->rx_skb->len;
+ struct ieee80211_hdr *hdr = (void *)priv->rx_skb->data;
+
+ if (!priv->scanning)
+ goto exit;
+
+ if (len < 24)
+ goto exit;
+
+ if (ieee80211_is_probe_resp(hdr->frame_control)) {
+ el_off = offsetof(struct ieee80211_mgmt, u.probe_resp.variable);
+ el = ((struct ieee80211_mgmt *)hdr)->u.probe_resp.variable;
+ } else if (ieee80211_is_beacon(hdr->frame_control)) {
+ el_off = offsetof(struct ieee80211_mgmt, u.beacon.variable);
+ el = ((struct ieee80211_mgmt *)hdr)->u.beacon.variable;
+ } else {
+ goto exit;
+ }
+ len -= el_off;
+
+ el = cfg80211_find_ie(WLAN_EID_DS_PARAMS, el, len);
+ if (el && el[1] > 0)
+ channel = el[2];
+
+exit:
+ return ieee80211_channel_to_frequency(channel, IEEE80211_BAND_2GHZ);
+}
+
static void at76_rx_tasklet(unsigned long param)
{
struct urb *urb = (struct urb *)param;
@@ -1542,6 +1590,8 @@ static void at76_rx_tasklet(unsigned long param)
rx_status.signal = buf->rssi;
rx_status.flag |= RX_FLAG_DECRYPTED;
rx_status.flag |= RX_FLAG_IV_STRIPPED;
+ rx_status.band = IEEE80211_BAND_2GHZ;
+ rx_status.freq = at76_guess_freq(priv);
at76_dbg(DBG_MAC80211, "calling ieee80211_rx_irqsafe(): %d/%d",
priv->rx_skb->len, priv->rx_skb->data_len);
@@ -1894,6 +1944,8 @@ static void at76_dwork_hw_scan(struct work_struct *work)
if (is_valid_ether_addr(priv->bssid))
at76_join(priv);
+ priv->scanning = false;
+
mutex_unlock(&priv->mtx);
ieee80211_scan_completed(priv->hw, false);
@@ -1948,6 +2000,7 @@ static int at76_hw_scan(struct ieee80211_hw *hw,
goto exit;
}
+ priv->scanning = true;
ieee80211_queue_delayed_work(priv->hw, &priv->dwork_hw_scan,
SCAN_POLL_INTERVAL);
diff --git a/drivers/net/wireless/at76c50x-usb.h b/drivers/net/wireless/at76c50x-usb.h
index 4718aa5..55090a3 100644
--- a/drivers/net/wireless/at76c50x-usb.h
+++ b/drivers/net/wireless/at76c50x-usb.h
@@ -418,6 +418,7 @@ struct at76_priv {
int scan_max_time; /* scan max channel time */
int scan_mode; /* SCAN_TYPE_ACTIVE, SCAN_TYPE_PASSIVE */
int scan_need_any; /* if set, need to scan for any ESSID */
+ bool scanning; /* if set, the scan is running */
u16 assoc_id; /* current association ID, if associated */
--
1.9.1