[PATCH] mac80211: notify upper layers after lower

[PATCH] mac80211: notify upper layers after lower

[Reinette Chatre]

When drivers receive change notification they may do work that
will enable the changes to take effect. For example, if new association
the device needs to be programmed with this information.
Give the driver chance to make the changes before notifying the
upper layer - thus preventing race condition where upper layer
attempts to utilize state that may not be configured yet.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 net/mac80211/ieee80211_sta.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index be2ce24..f3c8a21 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -545,13 +545,13 @@ static void ieee80211_set_associated(struct net_device *dev,
 
 		memset(wrqu.ap_addr.sa_data, 0, ETH_ALEN);
 	}
-	wrqu.ap_addr.sa_family = ARPHRD_ETHER;
-	wireless_send_event(dev, SIOCGIWAP, &wrqu, NULL);
 	ifsta->last_probe = jiffies;
 	ieee80211_led_assoc(local, assoc);
 
 	sdata->bss_conf.assoc = assoc;
 	ieee80211_bss_info_change_notify(sdata, changed);
+	wrqu.ap_addr.sa_family = ARPHRD_ETHER;
+	wireless_send_event(dev, SIOCGIWAP, &wrqu, NULL);
 }
 
 static void ieee80211_set_disassoc(struct net_device *dev,
-- 
1.5.3.4

[PATCH] mac80211: notify mac from low level driver (iwlwifi)

[Reinette Chatre]

From: Mohamed Abbas <mabbas@linux.intel.com>

Add new API to MAC80211 to allow low level driver to
notify MAC with driver status like comming from suspend
and HW rfkill

Signed-off-by: Mohamed Abbas <mabbas@linux.intel.com>
Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
 drivers/net/wireless/iwlwifi/iwl3945-base.c |    1 +
 drivers/net/wireless/iwlwifi/iwl4965-base.c |    1 +
 include/net/mac80211.h                      |   21 +++++++++++++++++++++
 net/mac80211/ieee80211_sta.c                |   27 +++++++++++++++++++++++++++
 4 files changed, 50 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wireless/iwlwifi/iwl3945-base.c b/drivers/net/wireless/iwlwifi/iwl3945-base.c
index d4daa04..19a14ad 100644
--- a/drivers/net/wireless/iwlwifi/iwl3945-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl3945-base.c
@@ -5883,6 +5883,7 @@ static void iwl3945_alive_start(struct iwl3945_priv *priv)
 	if (priv->error_recovering)
 		iwl3945_error_recovery(priv);
 
+	ieee80211_notify_mac(priv->hw, IEEE80211_NOTIFY_RE_ASSOC);
 	return;
 
  restart:
diff --git a/drivers/net/wireless/iwlwifi/iwl4965-base.c b/drivers/net/wireless/iwlwifi/iwl4965-base.c
index 4517e4c..4de81d8 100644
--- a/drivers/net/wireless/iwlwifi/iwl4965-base.c
+++ b/drivers/net/wireless/iwlwifi/iwl4965-base.c
@@ -5711,6 +5711,7 @@ static void iwl4965_alive_start(struct iwl_priv *priv)
 		iwl4965_error_recovery(priv);
 
 	iwlcore_low_level_notify(priv, IWLCORE_START_EVT);
+	ieee80211_notify_mac(priv->hw, IEEE80211_NOTIFY_RE_ASSOC);
 	return;
 
  restart:
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 01b3215..9fcda2d 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -74,6 +74,14 @@
  */
 
 /**
+ * enum ieee80211_notification_type - Low level driver notification
+ * @IEEE80211_NOTIFY_RE_ASSOC: start the re-association sequence
+ */
+enum ieee80211_notification_types {
+	IEEE80211_NOTIFY_RE_ASSOC,
+};
+
+/**
  * struct ieee80211_ht_bss_info - describing BSS's HT characteristics
  *
  * This structure describes most essential parameters needed
@@ -1677,4 +1685,17 @@ void ieee80211_stop_tx_ba_cb(struct ieee80211_hw *hw, u8 *ra, u8 tid);
 void ieee80211_stop_tx_ba_cb_irqsafe(struct ieee80211_hw *hw, const u8 *ra,
 				     u16 tid);
 
+/**
+ * ieee80211_notify_mac - low level driver notification
+ * @hw: pointer as obtained from ieee80211_alloc_hw().
+ * @notification_types: enum ieee80211_notification_types
+ *
+ * This function must be called by low level driver to inform mac80211 of
+ * low level driver status change or force mac80211 to re-assoc for low
+ * level driver internal error that require re-assoc. A good example to use
+ * this notification to call mac80211 after suspend/resume, some NIC require
+ * re-assoc.
+ */
+void ieee80211_notify_mac(struct ieee80211_hw *hw,
+			  enum ieee80211_notification_types  notif_type);
 #endif /* MAC80211_H */
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index f3c8a21..e1dd739 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -4235,3 +4235,30 @@ int ieee80211_sta_disassociate(struct net_device *dev, u16 reason)
 	ieee80211_set_disassoc(dev, ifsta, 0);
 	return 0;
 }
+
+void ieee80211_notify_mac(struct ieee80211_hw *hw,
+			  enum ieee80211_notification_types  notif_type)
+{
+	struct ieee80211_local *local = hw_to_local(hw);
+	struct ieee80211_sub_if_data *sdata;
+
+	switch (notif_type) {
+	case IEEE80211_NOTIFY_RE_ASSOC:
+		rcu_read_lock();
+		list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+
+			/* No need to wake the master device. */
+			if (sdata->dev == local->mdev)
+				continue;
+
+			if (sdata->vif.type == IEEE80211_IF_TYPE_STA) {
+				ieee80211_sta_req_auth(sdata->dev,
+						       &sdata->u.sta);
+			}
+
+		}
+		rcu_read_unlock();
+		break;
+	}
+}
+EXPORT_SYMBOL(ieee80211_notify_mac);
-- 
1.5.3.4

Re: [PATCH] mac80211: notify mac from low level driver (iwlwifi)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]


> + * This function must be called by low level driver to inform mac80211 of
> + * low level driver status change or force mac80211 to re-assoc for low
> + * level driver internal error that require re-assoc. A good example to use
> + * this notification to call mac80211 after suspend/resume, some NIC require
> + * re-assoc.

I think the suspend/resume text should be dropped since IMHO that
requires more work, reprogramming all the hardware crypto keys for
example.

> +			/* No need to wake the master device. */
> +			if (sdata->dev == local->mdev)
> +				continue;

That looks like a copy&paste bug, we're not waking it in any way. Also,
it's of IF_TYPE_AP so it's not necessary to test this at all.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] mac80211: notify mac from low level driver (iwlwifi)

[mohamed salim abbas]

On 4/4/08, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> > + * This function must be called by low level driver to inform mac80211 of
> > + * low level driver status change or force mac80211 to re-assoc for low
> > + * level driver internal error that require re-assoc. A good example to use
> > + * this notification to call mac80211 after suspend/resume, some NIC require
> > + * re-assoc.
>
> I think the suspend/resume text should be dropped since IMHO that
> requires more work, reprogramming all the hardware crypto keys for
> example.
I can remove the suspend/resume text will be that good enough?
>
> > +                     /* No need to wake the master device. */
> > +                     if (sdata->dev == local->mdev)
> > +                             continue;
>
> That looks like a copy&paste bug, we're not waking it in any way. Also,
> it's of IF_TYPE_AP so it's not necessary to test this at all.
>
you are right it is a copy&paste from other part in mac80211, but as
you mentioned this an extra filtering which is not needed. I will
remove it and resubmit again.
> johannes
>
>

Re: [PATCH] mac80211: notify mac from low level driver (iwlwifi)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]


On Fri, 2008-04-04 at 14:35 -0700, mohamed salim abbas wrote:
> On 4/4/08, Johannes Berg <johannes@sipsolutions.net> wrote:
> >
> > > + * This function must be called by low level driver to inform mac80211 of
> > > + * low level driver status change or force mac80211 to re-assoc for low
> > > + * level driver internal error that require re-assoc. A good example to use
> > > + * this notification to call mac80211 after suspend/resume, some NIC require
> > > + * re-assoc.
> >
> > I think the suspend/resume text should be dropped since IMHO that
> > requires more work, reprogramming all the hardware crypto keys for
> > example.

> I can remove the suspend/resume text will be that good enough?

Sure. I hope at some point somebody will implement proper suspend/resume
(i.e. when the suspend callback is invoked all interfaces, stations,
keys etc. are removed from the driver) but that's a lot more work. That
you mention suspend/resume though makes me wonder: does iwlwifi store
all that information locally and restore it to the hardware so that it
can actually work with this across suspend/resume? For b43, I at least
need to reset all WEP keys.

Thanks,
johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] mac80211: notify mac from low level driver (iwlwifi)

[mohamed salim abbas]

On 4/4/08, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Fri, 2008-04-04 at 14:35 -0700, mohamed salim abbas wrote:
> > On 4/4/08, Johannes Berg <johannes@sipsolutions.net> wrote:
> > >
> > > > + * This function must be called by low level driver to inform mac80211 of
> > > > + * low level driver status change or force mac80211 to re-assoc for low
> > > > + * level driver internal error that require re-assoc. A good example to use
> > > > + * this notification to call mac80211 after suspend/resume, some NIC require
> > > > + * re-assoc.
> > >
> > > I think the suspend/resume text should be dropped since IMHO that
> > > requires more work, reprogramming all the hardware crypto keys for
> > > example.
>
> > I can remove the suspend/resume text will be that good enough?
>
> Sure. I hope at some point somebody will implement proper suspend/resume
> (i.e. when the suspend callback is invoked all interfaces, stations,
> keys etc. are removed from the driver) but that's a lot more work. That
> you mention suspend/resume though makes me wonder: does iwlwifi store
> all that information locally and restore it to the hardware so that it
> can actually work with this across suspend/resume? For b43, I at least
> need to reset all WEP keys.

for iwl the driver on suspend behave like the device is going down, on
resume the driver reload the firmware and start fresh. the driver does
save some data to restore on resume.
I guess we can enhance on this to handle more scenario, but atleast
this is good enough for iwl driver.

>
> Thanks,
> johannes
>
>

Re: [PATCH] mac80211: notify mac from low level driver (iwlwifi)

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 729 bytes --]


> for iwl the driver on suspend behave like the device is going down, on
> resume the driver reload the firmware and start fresh. the driver does
> save some data to restore on resume.

Right, I think most drivers/hw behave that way.

> I guess we can enhance on this to handle more scenario, but atleast
> this is good enough for iwl driver.

What happens, for example, when you enable hardware crypto and set a WEP
key? With b43, after suspend/resume, mac80211 and the driver think the
key is set in hardware, but it was obviously cleared. It would of course
be possible to handle that in the driver, but I'd prefer it to be done
in mac80211 because then the driver doesn't need all the extra code.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] mac80211: notify mac from low level driver (iwlwifi)

[mohamed salim abbas]

On 4/4/08, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> > for iwl the driver on suspend behave like the device is going down, on
> > resume the driver reload the firmware and start fresh. the driver does
> > save some data to restore on resume.
>
> Right, I think most drivers/hw behave that way.
>
> > I guess we can enhance on this to handle more scenario, but atleast
> > this is good enough for iwl driver.
>
> What happens, for example, when you enable hardware crypto and set a WEP
> key? With b43, after suspend/resume, mac80211 and the driver think the
> key is set in hardware, but it was obviously cleared. It would of course
> be possible to handle that in the driver, but I'd prefer it to be done
> in mac80211 because then the driver doesn't need all the extra code.
>
> johannes
>
>
That exactly what I think should happen and that why iwl need to
notify the mac80211 to re-associate again, we can have the driver do
that but it is cleaner and ii is more safe to have mac80211 to drive
thing than have the driver try extra hard to restore to pre-suspend. I
never involved in the security part of the driver so I need to look it
up.

Re: [PATCH] mac80211: notify upper layers after lower

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1482 bytes --]


On Thu, 2008-04-03 at 16:08 -0700, Reinette Chatre wrote:
> When drivers receive change notification they may do work that
> will enable the changes to take effect. For example, if new association
> the device needs to be programmed with this information.
> Give the driver chance to make the changes before notifying the
> upper layer - thus preventing race condition where upper layer
> attempts to utilize state that may not be configured yet.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>

Looks good to me

Acked-by: Johannes Berg <johannes@sipsolutions.net>

> ---
>  net/mac80211/ieee80211_sta.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
> index be2ce24..f3c8a21 100644
> --- a/net/mac80211/ieee80211_sta.c
> +++ b/net/mac80211/ieee80211_sta.c
> @@ -545,13 +545,13 @@ static void ieee80211_set_associated(struct net_device *dev,
>  
>  		memset(wrqu.ap_addr.sa_data, 0, ETH_ALEN);
>  	}
> -	wrqu.ap_addr.sa_family = ARPHRD_ETHER;
> -	wireless_send_event(dev, SIOCGIWAP, &wrqu, NULL);
>  	ifsta->last_probe = jiffies;
>  	ieee80211_led_assoc(local, assoc);
>  
>  	sdata->bss_conf.assoc = assoc;
>  	ieee80211_bss_info_change_notify(sdata, changed);
> +	wrqu.ap_addr.sa_family = ARPHRD_ETHER;
> +	wireless_send_event(dev, SIOCGIWAP, &wrqu, NULL);
>  }
>  
>  static void ieee80211_set_disassoc(struct net_device *dev,

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]