[RFC] mac80211: at76x50x_usb driver broken by commit 3afc216.. and RX path involved in scan

Commit Message

Emmanuel Grumbach May 18, 2014, 7:10 a.m. UTC

> 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).


I'd say this is a bad behavior, but I am not the authoritative opinion here :)

> 

> 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.


Right - but the at7950x-usb doesn't provide this information, then the channel is taken from the DS IE - only if it exists, so what if this IE doesn't exist?

> 

> 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()


Yes - the frame comes to the bss update from 2 paths when we are scanning.

> 

> 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.


Yep.

> 

> If this is correct, my question is:

> Isn't this a redundant path duplication (provided that drivers

> supplies channel information)?


This has been discussed in the mailing list already, but yes.

> 

> 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).


Can you try this:


> 

> 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?


No - this is fine. We do that all over the stack.

Patch

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 3fb07e9..9d2f0d0 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -2791,8 +2791,13 @@  static void ieee80211_rx_bss_info(struct ieee80211_sub_if_data *sdata,
        sdata_assert_lock(sdata);
 
        channel = ieee80211_get_channel(local->hw.wiphy, rx_status->freq);
-       if (!channel)
-               return;
+       if (!channel && elems->ds_params) {
+               int freq = ieee80211_channel_to_frequency(elems->ds_params[0],
+                                                         rx_status->band);
+               channel = ieee80211_get_channel(local->hw.wiphy, freq);
+               if (!channel || channel->flags & IEEE80211_CHAN_DISABLED)
+                       return;
+       }
 
        bss = ieee80211_bss_info_update(local, rx_status, mgmt, len, elems,
                                        channel);