On Thu, 2009-07-16 at 11:09 +0200, Helmut Schaa wrote:
Looks nice! Some nitpicks ;)
> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.>
Missing "com" :)
> +/**
> + * enum mac80211_scan_flag - currently active scan mode
> + *
> + * @SCAN_SW_SCANNING: We're off our operating channel for scanning
> + * @SCAN_HW_SCANNING: The hardware is scanning for us, we have no way to
> + * determine if we are on the operating channel or not
> + * @SCAN_BG_SCANNING: We're currently in the process of scanning but may
> + * as well be on the operating channel
> + */
> enum mac80211_scan_flag {
> SCAN_SW_SCANNING = 1,"SCAN_ENTER_OPER_CHANNEL"
> SCAN_HW_SCANNING = 2,
> + SCAN_BG_SCANNING = 4,
There's some random stuff in there that doesn't belong. Also I would
prefer you used BIT(0) etc. or maybe __test_bit().
> --- a/net/mac80211/iface.c
> +++ b/net/mac80211/iface.c
> @@ -513,7 +513,7 @@ static int ieee80211_stop(struct net_device *dev)
> * the scan_sdata is NULL already don't send out a
> * scan event to userspace -- the scan is incomplete.
> */
> - if (local->scanning & SCAN_SW_SCANNING)
> + if (local->scanning & SCAN_BG_SCANNING)
> ieee80211_scan_completed(&local->hw, true);
> }
That doesn't seem correct -- it should be kept I think.
> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index a1b4887..a87522f 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -198,7 +198,7 @@ void ieee80211_bss_info_change_notify(struct ieee80211_sub_if_data *sdata,
> }
>
> if (changed & BSS_CHANGED_BEACON_ENABLED) {
> - if (local->scanning & SCAN_SW_SCANNING) {
> + if (local->scanning & SCAN_BG_SCANNING) {
> sdata->vif.bss_conf.enable_beacon = false;
That too.
> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index 3df8a6e..24739ab 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -2136,7 +2136,7 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
> return;
> }
>
> - if (unlikely(local->scanning))
> + if (unlikely((local->scanning & SCAN_HW_SCANNING) || (local->scanning & SCAN_SW_SCANNING)))
I would prefer
if (unlikely(local->scanning & (SCAN_HW_SCANNING | SCAN_SW_SCANNING)))
> rx.flags |= IEEE80211_RX_IN_SCAN;
>
> ieee80211_parse_qos(&rx);
> diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
> index b4cc556..8f33fb5 100644
> --- a/net/mac80211/scan.c
> +++ b/net/mac80211/scan.c
> @@ -282,7 +282,7 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted)
> cfg80211_scan_done(local->scan_req, aborted);
> local->scan_req = NULL;
>
> - was_hw_scan = local->scanning & SCAN_HW_SCANNING;
> + was_hw_scan = !!(local->scanning & SCAN_HW_SCANNING);
Should that be in the other patch?
> @@ -435,7 +434,7 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
> if (local->ops->hw_scan)
> local->scanning |= SCAN_HW_SCANNING;
> else
> - local->scanning |= SCAN_SW_SCANNING;
> + local->scanning |= SCAN_BG_SCANNING;
Ok now I'm confused. Did you really intend to replace all these?
Based on your initial description I thought it was going to be
scanning == SW_SCANNING
or
scanning == SW_SCANNING | BG_SCANNING
> + if (local->scan_channel) {
> + /*
> + * we're currently scanning a different channel, let's
> + * switch back to the operating channel now if at least
> + * one interface is associated. Otherwise just scan the
> + * next channel
> + */
> + if (associated)
> + local->scan_state = SCAN_ENTER_OPER_CHANNEL;
> + else
> + local->scan_state = SCAN_SET_CHANNEL;
:)
> + /* advance to the next channel to be scanned */
> + *next_delay = HZ / 10;
> + local->scan_state = SCAN_SET_CHANNEL;
Maybe we should rename scan_state to next_scan_state.
> + if (ieee80211_hw_config(local,
> + IEEE80211_CONF_CHANGE_CHANNEL))
That looks weird. I don't think you really need to care about the return
value anyway, now that we have rfkill integrated it shouldn't ever be
nonzero (and if it is, while rfkill is being activated, it doesn't
really matter since we're taking down interfaces)
> + /*
> + * notify the AP about us being back and restart all STA interfaces
> + */
> + mutex_lock(&local->iflist_mtx);
> + list_for_each_entry(sdata, &local->interfaces, list) {
> + if (!netif_running(sdata->dev))
> + continue;
> +
> + /* Tell AP we're back */
> + if (sdata->vif.type == NL80211_IFTYPE_STATION) {
> + if (sdata->u.mgd.associated) {
> + ieee80211_scan_ps_disable(sdata);
> + }
Could drop a set of {} here.
> @@ -657,7 +760,7 @@ void ieee80211_scan_cancel(struct ieee80211_local *local)
> * queued -- mostly at suspend under RTNL.
> */
> mutex_lock(&local->scan_mtx);
> - swscan = !!(local->scanning & SCAN_SW_SCANNING);
> + swscan = !!(local->scanning & SCAN_BG_SCANNING);
and another one -- please explain?
Anyway looks pretty good to me! How does it fare during ping -f or
something?
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
