Re: [PATCH] d80211: ieee80211_hw handlers should be allowed to sleep

2006-10-19 Thread Jiri Benc 
On Wed, 18 Oct 2006 19:27:37 +0200, Ivo van Doorn wrote:
 Would something like the patch below be better?
 It keeps the flush_scheduled_work() at the same location, but a second
 is added in case local-scan_work.data != sdata-dev

Applied to my tree, thanks for the patch!

 Jiri

-- 
Jiri Benc
SUSE Labs
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] d80211: ieee80211_hw handlers should be allowed to sleep

2006-10-18 Thread Jiri Benc 
On Sat, 7 Oct 2006 11:23:15 +0200, Ivo van Doorn wrote:
 --- a/net/d80211/ieee80211.c
 +++ b/net/d80211/ieee80211.c
 @@ -2075,15 +2075,15 @@ void ieee80211_if_shutdown(struct net_de
   case IEEE80211_IF_TYPE_STA:
   case IEEE80211_IF_TYPE_IBSS:
   sdata-u.sta.state = IEEE80211_DISABLED;
 - del_timer_sync(sdata-u.sta.timer);
 + cancel_delayed_work(sdata-u.sta.work);
   if (local-scan_work.data == sdata-dev) {
   local-sta_scanning = 0;
   cancel_delayed_work(local-scan_work);
 - flush_scheduled_work();
   /* see comment in ieee80211_unregister_hw to
* understand why this works */
   local-scan_work.data = NULL;
   }
 + flush_scheduled_work();

This is racy. local-scan_work.data can be set to NULL only after
flush_scheduled_work().

Other than that, the patch looks good to me.

Thanks,

 Jiri

-- 
Jiri Benc
SUSE Labs
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] d80211: ieee80211_hw handlers should be allowed to sleep

2006-10-18 Thread Ivo van Doorn 
On Wednesday 18 October 2006 15:06, Jiri Benc wrote:
 On Sat, 7 Oct 2006 11:23:15 +0200, Ivo van Doorn wrote:
  --- a/net/d80211/ieee80211.c
  +++ b/net/d80211/ieee80211.c
  @@ -2075,15 +2075,15 @@ void ieee80211_if_shutdown(struct net_de
  case IEEE80211_IF_TYPE_STA:
  case IEEE80211_IF_TYPE_IBSS:
  sdata-u.sta.state = IEEE80211_DISABLED;
  -   del_timer_sync(sdata-u.sta.timer);
  +   cancel_delayed_work(sdata-u.sta.work);
  if (local-scan_work.data == sdata-dev) {
  local-sta_scanning = 0;
  cancel_delayed_work(local-scan_work);
  -   flush_scheduled_work();
  /* see comment in ieee80211_unregister_hw to
   * understand why this works */
  local-scan_work.data = NULL;
  }
  +   flush_scheduled_work();
 
 This is racy. local-scan_work.data can be set to NULL only after
 flush_scheduled_work().

Would something like the patch below be better?
It keeps the flush_scheduled_work() at the same location, but a second
is added in case local-scan_work.data != sdata-dev

Jan, was there any particular reason to move flush_cheduled_work() outside of 
the if-statement?

Signed-off-by Ivo van Doorn [EMAIL PROTECTED]

---

diff --git a/net/d80211/ieee80211.c b/net/d80211/ieee80211.c
index 32a1ba7..cb1180c 100644
--- a/net/d80211/ieee80211.c
+++ b/net/d80211/ieee80211.c
@@ -2075,7 +2075,7 @@ void ieee80211_if_shutdown(struct net_de
case IEEE80211_IF_TYPE_STA:
case IEEE80211_IF_TYPE_IBSS:
sdata-u.sta.state = IEEE80211_DISABLED;
-   del_timer_sync(sdata-u.sta.timer);
+   cancel_delayed_work(sdata-u.sta.work);
if (local-scan_work.data == sdata-dev) {
local-sta_scanning = 0;
cancel_delayed_work(local-scan_work);
@@ -2083,7 +2083,8 @@ void ieee80211_if_shutdown(struct net_de
/* see comment in ieee80211_unregister_hw to
 * understand why this works */
local-scan_work.data = NULL;
-   }
+   } else
+   flush_scheduled_work();
break;
}
 }
@@ -4605,8 +4606,8 @@ void ieee80211_unregister_hw(struct net_
flush_scheduled_work();
/* The scan_work is guaranteed not to be called at this
 * point. It is not scheduled and not running now. It can be
-* scheduled again only by some sta_timer (all of them are
-* stopped by now) or under rtnl lock. */
+* scheduled again only by sta_work (stopped by now) or under
+* rtnl lock. */
}
 
ieee80211_rx_bss_list_deinit(dev);
diff --git a/net/d80211/ieee80211_i.h b/net/d80211/ieee80211_i.h
index 89666ec..5b48ce2 100644
--- a/net/d80211/ieee80211_i.h
+++ b/net/d80211/ieee80211_i.h
@@ -240,7 +240,7 @@ struct ieee80211_if_sta {
IEEE80211_ASSOCIATE, IEEE80211_ASSOCIATED,
IEEE80211_IBSS_SEARCH, IEEE80211_IBSS_JOINED
} state;
-   struct timer_list timer;
+   struct work_struct work;
u8 bssid[ETH_ALEN], prev_bssid[ETH_ALEN];
u8 ssid[IEEE80211_MAX_SSID_LEN];
size_t ssid_len;
@@ -621,7 +621,7 @@ int ieee80211_set_compression(struct iee
  struct net_device *dev, struct sta_info *sta);
 int ieee80211_init_client(struct net_device *dev);
 /* ieee80211_sta.c */
-void ieee80211_sta_timer(unsigned long ptr);
+void ieee80211_sta_work(void *ptr);
 void ieee80211_sta_rx_mgmt(struct net_device *dev, struct sk_buff *skb,
   struct ieee80211_rx_status *rx_status);
 int ieee80211_sta_set_ssid(struct net_device *dev, char *ssid, size_t len);
diff --git a/net/d80211/ieee80211_iface.c b/net/d80211/ieee80211_iface.c
index 9a187af..4dd480f 100644
--- a/net/d80211/ieee80211_iface.c
+++ b/net/d80211/ieee80211_iface.c
@@ -194,9 +194,7 @@ void ieee80211_if_set_type(struct net_de
struct ieee80211_if_sta *ifsta;
 
ifsta = sdata-u.sta;
-   init_timer(ifsta-timer);
-   ifsta-timer.data = (unsigned long) dev;
-   ifsta-timer.function = ieee80211_sta_timer;
+   INIT_WORK(ifsta-work, ieee80211_sta_work, dev);
 
ifsta-capab = WLAN_CAPABILITY_ESS;
ifsta-auth_algs = IEEE80211_AUTH_ALG_OPEN |
diff --git a/net/d80211/ieee80211_sta.c b/net/d80211/ieee80211_sta.c
index cc336bd..bf74b6b 100644
--- a/net/d80211/ieee80211_sta.c
+++ b/net/d80211/ieee80211_sta.c
@@ -457,7 +457,7 @@ static void ieee80211_authenticate(struc
 
ieee80211_send_auth(dev, ifsta, 1, NULL, 0, 0);
 
-   mod_timer(ifsta-timer, jiffies + IEEE80211_AUTH_TIMEOUT);
+   schedule_delayed_work(ifsta-work, IEEE80211_AUTH_TIMEOUT);
 }
 
 
@@ -677,7 +677,7 @@ static void

Re: [PATCH] d80211: ieee80211_hw handlers should be allowed to sleep

2006-10-18 Thread Jan Kiszka 
Ivo van Doorn wrote:
 On Wednesday 18 October 2006 15:06, Jiri Benc wrote:
 On Sat, 7 Oct 2006 11:23:15 +0200, Ivo van Doorn wrote:
 --- a/net/d80211/ieee80211.c
 +++ b/net/d80211/ieee80211.c
 @@ -2075,15 +2075,15 @@ void ieee80211_if_shutdown(struct net_de
 case IEEE80211_IF_TYPE_STA:
 case IEEE80211_IF_TYPE_IBSS:
 sdata-u.sta.state = IEEE80211_DISABLED;
 -   del_timer_sync(sdata-u.sta.timer);
 +   cancel_delayed_work(sdata-u.sta.work);
 if (local-scan_work.data == sdata-dev) {
 local-sta_scanning = 0;
 cancel_delayed_work(local-scan_work);
 -   flush_scheduled_work();
 /* see comment in ieee80211_unregister_hw to
  * understand why this works */
 local-scan_work.data = NULL;
 }
 +   flush_scheduled_work();
 This is racy. local-scan_work.data can be set to NULL only after
 flush_scheduled_work().
 
 Would something like the patch below be better?
 It keeps the flush_scheduled_work() at the same location, but a second
 is added in case local-scan_work.data != sdata-dev
 
 Jan, was there any particular reason to move flush_cheduled_work() outside of 
 the if-statement?

It is needed unconditionally now, so I moved it out without knowing
about this side effect. Your approach looks good to me.

 
 Signed-off-by Ivo van Doorn [EMAIL PROTECTED]
 
 ---
 
 diff --git a/net/d80211/ieee80211.c b/net/d80211/ieee80211.c
 index 32a1ba7..cb1180c 100644
 --- a/net/d80211/ieee80211.c
 +++ b/net/d80211/ieee80211.c
 @@ -2075,7 +2075,7 @@ void ieee80211_if_shutdown(struct net_de
   case IEEE80211_IF_TYPE_STA:
   case IEEE80211_IF_TYPE_IBSS:
   sdata-u.sta.state = IEEE80211_DISABLED;
 - del_timer_sync(sdata-u.sta.timer);
 + cancel_delayed_work(sdata-u.sta.work);
   if (local-scan_work.data == sdata-dev) {
   local-sta_scanning = 0;
   cancel_delayed_work(local-scan_work);
 @@ -2083,7 +2083,8 @@ void ieee80211_if_shutdown(struct net_de
   /* see comment in ieee80211_unregister_hw to
* understand why this works */
   local-scan_work.data = NULL;
 - }
 + } else
 + flush_scheduled_work();
   break;
   }
  }
 @@ -4605,8 +4606,8 @@ void ieee80211_unregister_hw(struct net_
   flush_scheduled_work();
   /* The scan_work is guaranteed not to be called at this
* point. It is not scheduled and not running now. It can be
 -  * scheduled again only by some sta_timer (all of them are
 -  * stopped by now) or under rtnl lock. */
 +  * scheduled again only by sta_work (stopped by now) or under
 +  * rtnl lock. */
   }
  
   ieee80211_rx_bss_list_deinit(dev);
 diff --git a/net/d80211/ieee80211_i.h b/net/d80211/ieee80211_i.h
 index 89666ec..5b48ce2 100644
 --- a/net/d80211/ieee80211_i.h
 +++ b/net/d80211/ieee80211_i.h
 @@ -240,7 +240,7 @@ struct ieee80211_if_sta {
   IEEE80211_ASSOCIATE, IEEE80211_ASSOCIATED,
   IEEE80211_IBSS_SEARCH, IEEE80211_IBSS_JOINED
   } state;
 - struct timer_list timer;
 + struct work_struct work;
   u8 bssid[ETH_ALEN], prev_bssid[ETH_ALEN];
   u8 ssid[IEEE80211_MAX_SSID_LEN];
   size_t ssid_len;
 @@ -621,7 +621,7 @@ int ieee80211_set_compression(struct iee
 struct net_device *dev, struct sta_info *sta);
  int ieee80211_init_client(struct net_device *dev);
  /* ieee80211_sta.c */
 -void ieee80211_sta_timer(unsigned long ptr);
 +void ieee80211_sta_work(void *ptr);
  void ieee80211_sta_rx_mgmt(struct net_device *dev, struct sk_buff *skb,
  struct ieee80211_rx_status *rx_status);
  int ieee80211_sta_set_ssid(struct net_device *dev, char *ssid, size_t len);
 diff --git a/net/d80211/ieee80211_iface.c b/net/d80211/ieee80211_iface.c
 index 9a187af..4dd480f 100644
 --- a/net/d80211/ieee80211_iface.c
 +++ b/net/d80211/ieee80211_iface.c
 @@ -194,9 +194,7 @@ void ieee80211_if_set_type(struct net_de
   struct ieee80211_if_sta *ifsta;
  
   ifsta = sdata-u.sta;
 - init_timer(ifsta-timer);
 - ifsta-timer.data = (unsigned long) dev;
 - ifsta-timer.function = ieee80211_sta_timer;
 + INIT_WORK(ifsta-work, ieee80211_sta_work, dev);
  
   ifsta-capab = WLAN_CAPABILITY_ESS;
   ifsta-auth_algs = IEEE80211_AUTH_ALG_OPEN |
 diff --git a/net/d80211/ieee80211_sta.c b/net/d80211/ieee80211_sta.c
 index cc336bd..bf74b6b 100644
 --- a/net/d80211/ieee80211_sta.c
 +++ b/net/d80211/ieee80211_sta.c
 @@ -457,7 +457,7 @@ static void ieee80211_authenticate(struc
  
   ieee80211_send_auth(dev, ifsta, 1, NULL, 0, 0);
  
 - mod_timer(ifsta-timer, jiffies +