[RFC PATCHv3 0/1] mac80211: Support for HW connection recovery

[RFC PATCHv3 0/1] mac80211: Support for HW connection recovery

[Juuso Oikarinen]

When PSM is enabled, the wl1271 performs connection recovery independently by
sending probe-requests to the associated-to AP if it detects beacon loss.

The wl1271 only indicates connection problem after the probe-requests have
failed. At this stage, it's useless for the mac80211 to send further
probe requests in attempt to recover the connection.

As the wl1271 is also capable of transmitting periodic keep-alive frames to
the AP, I realised these functionalities should be bundled.

Instead of any new functions, this v2 of this proposed feature does not add
any functions, instead it introduces the IEEE80211_HW_CONNECTION_MONITOR flag,
which prevents mac80211 from sending periodic keep-alive probe-requests to the
AP and prevents it from sending probe-requests on beacon loss.

This patch has been tested with the wl1271 driver.

I'm hoping to be close to completion with these patches, but obviously,
comments and suggestions are welcomed.

Juuso Oikarinen (1):
  mac80211: Add support connection monitor in hardware

 include/net/mac80211.h |   10 ++++++++++
 net/mac80211/mlme.c    |   38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 47 insertions(+), 1 deletions(-)

[RFC PATCHv3 1/1] mac80211: Add support connection monitor in hardware

[Juuso Oikarinen]

This patch is based on a RFC patch by Kalle Valo.

The wl1271 has a feature which handles the connection monitor logic
in hardware, basically sending periodically nullfunc frames and reporting
to the host if AP is lost, after attempting to recover by sending
probe-requests to the AP.

Add support to mac80211 by adding a new flag IEEE80211_HW_CONNECTION_MONITOR
which prevents conn_mon_timer from triggering during idle periods, and
prevents sending probe-requests to the AP if beacon-loss is indicated by the
hardware.

Cc: Kalle Valo <kalle.valo@nokia.com>
Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
---
 include/net/mac80211.h |   10 ++++++++++
 net/mac80211/mlme.c    |   38 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 47 insertions(+), 1 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 936bc41..ab138d2 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -954,6 +954,11 @@ enum ieee80211_tkip_key_type {
  *	Hardware can provide ack status reports of Tx frames to
  *	the stack.
  *
+ * @IEEE80211_HW_CONNECTION_MONITOR:
+ *      The hardware performs its own connection monitoring, including
+ *      periodic keep-alives to the AP and probing the AP on beacon loss.
+ *      When this flag is set, signaling beacon-loss will cause an immediate
+ *      change to disassociated state.
  */
 enum ieee80211_hw_flags {
 	IEEE80211_HW_HAS_RATE_CONTROL			= 1<<0,
@@ -975,6 +980,7 @@ enum ieee80211_hw_flags {
 	IEEE80211_HW_SUPPORTS_DYNAMIC_SMPS		= 1<<16,
 	IEEE80211_HW_SUPPORTS_UAPSD			= 1<<17,
 	IEEE80211_HW_REPORTS_TX_ACK_STATUS		= 1<<18,
+	IEEE80211_HW_CONNECTION_MONITOR			= 1<<19,
 };
 
 /**
@@ -2367,6 +2373,10 @@ void ieee80211_sta_block_awake(struct ieee80211_hw *hw,
  * When beacon filtering is enabled with IEEE80211_HW_BEACON_FILTERING and
  * IEEE80211_CONF_PS is set, the driver needs to inform whenever the
  * hardware is not receiving beacons with this function.
+ *
+ * When hardware connection monitoring is enabled with
+ * IEEE80211_HW_CONNECTION_MONITOR, this function will cause immediate change
+ * to disassociated state, without connection recovery attempts.
  */
 void ieee80211_beacon_loss(struct ieee80211_vif *vif);
 
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index be5f723..312a210 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -854,6 +854,9 @@ void ieee80211_sta_rx_notify(struct ieee80211_sub_if_data *sdata,
 	if (is_multicast_ether_addr(hdr->addr1))
 		return;
 
+	if (sdata->local->hw.flags & IEEE80211_HW_CONNECTION_MONITOR)
+		return;
+
 	mod_timer(&sdata->u.mgd.conn_mon_timer,
 		  round_jiffies_up(jiffies + IEEE80211_CONNECTION_IDLE_TIME));
 }
@@ -931,13 +934,46 @@ static void ieee80211_mgd_probe_ap(struct ieee80211_sub_if_data *sdata,
 	mutex_unlock(&ifmgd->mtx);
 }
 
+static void ieee80211_beacon_loss_disassoc(struct ieee80211_sub_if_data *sdata)
+{
+	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
+	struct ieee80211_local *local = sdata->local;
+	u8 bssid[ETH_ALEN];
+
+	mutex_lock(&ifmgd->mtx);
+	if (!ifmgd->associated) {
+		mutex_unlock(&ifmgd->mtx);
+		return;
+	}
+
+	memcpy(bssid, ifmgd->associated->bssid, ETH_ALEN);
+
+	printk(KERN_DEBUG "Beacon loss from AP %pM, "
+	       "disconnected.\n", bssid);
+
+	ieee80211_set_disassoc(sdata);
+	ieee80211_recalc_idle(local);
+	mutex_unlock(&ifmgd->mtx);
+	/*
+	 * must be outside lock due to cfg80211,
+	 * but that's not a problem.
+	 */
+	ieee80211_send_deauth_disassoc(sdata, bssid,
+				       IEEE80211_STYPE_DEAUTH,
+				       WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
+				       NULL);
+}
+
 void ieee80211_beacon_loss_work(struct work_struct *work)
 {
 	struct ieee80211_sub_if_data *sdata =
 		container_of(work, struct ieee80211_sub_if_data,
 			     u.mgd.beacon_loss_work);
 
-	ieee80211_mgd_probe_ap(sdata, true);
+	if (sdata->local->hw.flags & IEEE80211_HW_CONNECTION_MONITOR)
+		ieee80211_beacon_loss_disassoc(sdata);
+	else
+		ieee80211_mgd_probe_ap(sdata, true);
 }
 
 void ieee80211_beacon_loss(struct ieee80211_vif *vif)
-- 
1.6.3.3

Re: [RFC PATCHv3 1/1] mac80211: Add support connection monitor in hardware

[Johannes Berg]

> + *
> + * When hardware connection monitoring is enabled with
> + * IEEE80211_HW_CONNECTION_MONITOR, this function will cause immediate change
> + * to disassociated state, without connection recovery attempts.

Tiny detail: putting a % in front of constants formats them nicer in the
html output.

> +static void ieee80211_beacon_loss_disassoc(struct ieee80211_sub_if_data *sdata)
> +{

I can see where you're coming from, but maybe this should be more like
ieee80211_driver_notify_disconnect() or something like that?


> +	printk(KERN_DEBUG "Beacon loss from AP %pM, "
> +	       "disconnected.\n", bssid);

printk lines are allowed to pass 80 chars to make grep easier. Also I
really think this should be more like "Connection to AP %pM lost." or
something like that?

>  void ieee80211_beacon_loss_work(struct work_struct *work)
>  {
>  	struct ieee80211_sub_if_data *sdata =
>  		container_of(work, struct ieee80211_sub_if_data,
>  			     u.mgd.beacon_loss_work);
>  
> -	ieee80211_mgd_probe_ap(sdata, true);
> +	if (sdata->local->hw.flags & IEEE80211_HW_CONNECTION_MONITOR)
> +		ieee80211_beacon_loss_disassoc(sdata);
> +	else
> +		ieee80211_mgd_probe_ap(sdata, true);
>  }

And I'm actually wondering now if using the same API is a good idea.
Yes, it makes some sense, but it's quite different yet? Maybe we should
have something like this:

static inline void ieee80211_beacon_loss(hw, vif)
{
	WARN_ON(hw->flags & IEEE80211_HW_CONNECTION_MONITOR);
	__ieee80211_beacon_connection_loss(vif);
}

static inline void ieee80211_connection_loss(hw, vif)
{
	WARN_ON(!(hw->flags & IEEE80211_HW_CONNECTION_MONITOR));
	__ieee80211_beacon_connection_loss(vif);
}

to make at least the external API easier to understand?

johannes

Re: [RFC PATCHv3 1/1] mac80211: Add support connection monitor in hardware

[Juuso Oikarinen]

On Wed, 2010-03-17 at 17:15 +0100, ext Johannes Berg wrote:
> > + *
> > + * When hardware connection monitoring is enabled with
> > + * IEEE80211_HW_CONNECTION_MONITOR, this function will cause immediate change
> > + * to disassociated state, without connection recovery attempts.
> 
> Tiny detail: putting a % in front of constants formats them nicer in the
> html output.
> 
> > +static void ieee80211_beacon_loss_disassoc(struct ieee80211_sub_if_data *sdata)
> > +{
> 
> I can see where you're coming from, but maybe this should be more like
> ieee80211_driver_notify_disconnect() or something like that?
> 
> 
> > +	printk(KERN_DEBUG "Beacon loss from AP %pM, "
> > +	       "disconnected.\n", bssid);
> 
> printk lines are allowed to pass 80 chars to make grep easier. Also I
> really think this should be more like "Connection to AP %pM lost." or
> something like that?
> 
> >  void ieee80211_beacon_loss_work(struct work_struct *work)
> >  {
> >  	struct ieee80211_sub_if_data *sdata =
> >  		container_of(work, struct ieee80211_sub_if_data,
> >  			     u.mgd.beacon_loss_work);
> >  
> > -	ieee80211_mgd_probe_ap(sdata, true);
> > +	if (sdata->local->hw.flags & IEEE80211_HW_CONNECTION_MONITOR)
> > +		ieee80211_beacon_loss_disassoc(sdata);
> > +	else
> > +		ieee80211_mgd_probe_ap(sdata, true);
> >  }
> 
> And I'm actually wondering now if using the same API is a good idea.
> Yes, it makes some sense, but it's quite different yet? Maybe we should
> have something like this:
> 
> static inline void ieee80211_beacon_loss(hw, vif)
> {
> 	WARN_ON(hw->flags & IEEE80211_HW_CONNECTION_MONITOR);
> 	__ieee80211_beacon_connection_loss(vif);
> }
> 
> static inline void ieee80211_connection_loss(hw, vif)
> {
> 	WARN_ON(!(hw->flags & IEEE80211_HW_CONNECTION_MONITOR));
> 	__ieee80211_beacon_connection_loss(vif);
> }
> 
> to make at least the external API easier to understand?

We actually had a debate about this with Luciano Coelho, and I opted for
a separate API, and Luca opted for just adding a flag to the existing
beacon loss function. To be noted: Luca, two against one now! ;)

So I'll be sending v4 soon, with a separate API. Worrisomely, the amount
of comments seems to increase on every round :D

-Juuso

> johannes
> 

Re: [RFC PATCHv3 1/1] mac80211: Add support connection monitor in hardware

[Johannes Berg]

On Thu, 2010-03-18 at 07:06 +0200, Juuso Oikarinen wrote:

> We actually had a debate about this with Luciano Coelho, and I opted for
> a separate API, and Luca opted for just adding a flag to the existing
> beacon loss function. To be noted: Luca, two against one now! ;)

:P

> So I'll be sending v4 soon, with a separate API. Worrisomely, the amount
> of comments seems to increase on every round :D

Heh. Well I think I reached the limit of comments for a single patch ;)
But yeah, sorry about that. Sometimes I just don't think about things in
too much detail when there are other things that distract me I guess.

Mind you, the API I proposed requires changing the prototype. If you
don't do the warn_on, which I guess is fine, then you don't have to
change the prototype ... and in any cas eI think I'm off to bed now,
even if that's not really related :)

johannes

Re: [RFC PATCHv3 1/1] mac80211: Add support connection monitor in hardware

[Luciano Coelho]

On Thu, 2010-03-18 at 06:06 +0100, Oikarinen Juuso (Nokia-D/Tampere)
wrote:
> On Wed, 2010-03-17 at 17:15 +0100, ext Johannes Berg wrote:
> > >  void ieee80211_beacon_loss_work(struct work_struct *work)
> > >  {
> > >  	struct ieee80211_sub_if_data *sdata =
> > >  		container_of(work, struct ieee80211_sub_if_data,
> > >  			     u.mgd.beacon_loss_work);
> > >  
> > > -	ieee80211_mgd_probe_ap(sdata, true);
> > > +	if (sdata->local->hw.flags & IEEE80211_HW_CONNECTION_MONITOR)
> > > +		ieee80211_beacon_loss_disassoc(sdata);
> > > +	else
> > > +		ieee80211_mgd_probe_ap(sdata, true);
> > >  }
> > 
> > And I'm actually wondering now if using the same API is a good idea.
> > Yes, it makes some sense, but it's quite different yet? Maybe we should
> > have something like this:
> > 
> > static inline void ieee80211_beacon_loss(hw, vif)
> > {
> > 	WARN_ON(hw->flags & IEEE80211_HW_CONNECTION_MONITOR);
> > 	__ieee80211_beacon_connection_loss(vif);
> > }
> > 
> > static inline void ieee80211_connection_loss(hw, vif)
> > {
> > 	WARN_ON(!(hw->flags & IEEE80211_HW_CONNECTION_MONITOR));
> > 	__ieee80211_beacon_connection_loss(vif);
> > }
> > 
> > to make at least the external API easier to understand?
> 
> We actually had a debate about this with Luciano Coelho, and I opted for
> a separate API, and Luca opted for just adding a flag to the existing
> beacon loss function. To be noted: Luca, two against one now! ;)

Heh, damn you guys! Lots of things have changed since Juuso and I
discussed this, so I'm not 100% sure that my previous comment about the
flag really still applies. :P

My point at the time was that the driver should inform mac80211 that it
has tried to send probe_reqs automatically but they were not answered,
and let mac80211 itself decide whether that meant a connection loss or
not.


-- 
Luca, the one who won't admit losing the discussion. :)