From: Jan Kiszka <jan.kiszka@web.de>
To: Ivo van Doorn <ivdoorn@gmail.com>
Cc: Jiri Benc <jbenc@suse.cz>, netdev@vger.kernel.org
Subject: Re: [PATCH] d80211: ieee80211_hw handlers should be allowed to sleep
Date: Wed, 18 Oct 2006 20:10:02 +0200 [thread overview]
Message-ID: <45366DFA.7030707@web.de> (raw)
In-Reply-To: <200610181927.37471.IvDoorn@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 9116 bytes --]
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 <IvDoorn@gmail.com>
>
> ---
>
> 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 ieee80211_associate(struct n
>
> ieee80211_send_assoc(dev, ifsta);
>
> - mod_timer(&ifsta->timer, jiffies + IEEE80211_ASSOC_TIMEOUT);
> + schedule_delayed_work(&ifsta->work, IEEE80211_ASSOC_TIMEOUT);
> }
>
>
> @@ -735,11 +735,11 @@ static void ieee80211_associated(struct
> memset(wrqu.ap_addr.sa_data, 0, ETH_ALEN);
> wrqu.ap_addr.sa_family = ARPHRD_ETHER;
> wireless_send_event(dev, SIOCGIWAP, &wrqu, NULL);
> - mod_timer(&ifsta->timer,
> - jiffies + IEEE80211_MONITORING_INTERVAL + 30 * HZ);
> + schedule_delayed_work(&ifsta->work,
> + IEEE80211_MONITORING_INTERVAL + 30 * HZ);
> } else {
> - mod_timer(&ifsta->timer,
> - jiffies + IEEE80211_MONITORING_INTERVAL);
> + schedule_delayed_work(&ifsta->work,
> + IEEE80211_MONITORING_INTERVAL);
> }
> }
>
> @@ -1019,8 +1019,8 @@ static void ieee80211_rx_mgmt_deauth(str
> ifsta->state == IEEE80211_ASSOCIATE ||
> ifsta->state == IEEE80211_ASSOCIATED) {
> ifsta->state = IEEE80211_AUTHENTICATE;
> - mod_timer(&ifsta->timer,
> - jiffies + IEEE80211_RETRY_AUTH_INTERVAL);
> + schedule_delayed_work(&ifsta->work,
> + IEEE80211_RETRY_AUTH_INTERVAL);
> }
>
> ieee80211_set_associated(dev, ifsta, 0);
> @@ -1062,8 +1062,8 @@ static void ieee80211_rx_mgmt_disassoc(s
>
> if (ifsta->state == IEEE80211_ASSOCIATED) {
> ifsta->state = IEEE80211_ASSOCIATE;
> - mod_timer(&ifsta->timer,
> - jiffies + IEEE80211_RETRY_AUTH_INTERVAL);
> + schedule_delayed_work(&ifsta->work,
> + IEEE80211_RETRY_AUTH_INTERVAL);
> }
>
> ieee80211_set_associated(dev, ifsta, 0);
> @@ -1829,7 +1829,7 @@ static void ieee80211_sta_expire(struct
> static void ieee80211_sta_merge_ibss(struct net_device *dev,
> struct ieee80211_if_sta *ifsta)
> {
> - mod_timer(&ifsta->timer, jiffies + IEEE80211_IBSS_MERGE_INTERVAL);
> + schedule_delayed_work(&ifsta->work, IEEE80211_IBSS_MERGE_INTERVAL);
>
> ieee80211_sta_expire(dev);
> if (ieee80211_sta_active_ibss(dev))
> @@ -1841,20 +1841,19 @@ static void ieee80211_sta_merge_ibss(str
> }
>
>
> -void ieee80211_sta_timer(unsigned long ptr)
> +void ieee80211_sta_work(void *ptr)
> {
> - struct net_device *dev;
> + struct net_device *dev = ptr;
> struct ieee80211_sub_if_data *sdata;
> struct ieee80211_if_sta *ifsta;
>
> - dev = (struct net_device *) ptr;
> if (!netif_running(dev))
> return;
>
> sdata = IEEE80211_DEV_TO_SUB_IF(dev);
> if (sdata->type != IEEE80211_IF_TYPE_STA &&
> sdata->type != IEEE80211_IF_TYPE_IBSS) {
> - printk(KERN_DEBUG "%s: ieee80211_sta_timer: non-STA interface "
> + printk(KERN_DEBUG "%s: ieee80211_sta_work: non-STA interface "
> "(type=%d)\n", dev->name, sdata->type);
> return;
> }
> @@ -1879,7 +1878,7 @@ void ieee80211_sta_timer(unsigned long p
> ieee80211_sta_merge_ibss(dev, ifsta);
> break;
> default:
> - printk(KERN_DEBUG "ieee80211_sta_timer: Unknown state %d\n",
> + printk(KERN_DEBUG "ieee80211_sta_work: Unknown state %d\n",
> ifsta->state);
> break;
> }
> @@ -2107,7 +2106,7 @@ static int ieee80211_sta_join_ibss(struc
> }
>
> ifsta->state = IEEE80211_IBSS_JOINED;
> - mod_timer(&ifsta->timer, jiffies + IEEE80211_IBSS_MERGE_INTERVAL);
> + schedule_delayed_work(&ifsta->work, IEEE80211_IBSS_MERGE_INTERVAL);
>
> ieee80211_rx_bss_put(dev, bss);
>
> @@ -2224,8 +2223,8 @@ #endif /* CONFIG_D80211_IBSS_DEBUG */
> /* Selected IBSS not found in current scan results - try to scan */
> if (ifsta->state == IEEE80211_IBSS_JOINED &&
> !ieee80211_sta_active_ibss(dev)) {
> - mod_timer(&ifsta->timer,
> - jiffies + IEEE80211_IBSS_MERGE_INTERVAL);
> + schedule_delayed_work(&ifsta->work,
> + IEEE80211_IBSS_MERGE_INTERVAL);
> } else if (time_after(jiffies, local->last_scan_completed +
> IEEE80211_SCAN_INTERVAL)) {
> printk(KERN_DEBUG "%s: Trigger new scan to find an IBSS to "
> @@ -2253,7 +2252,7 @@ #endif /* CONFIG_D80211_IBSS_DEBUG */
> }
>
> ifsta->state = IEEE80211_IBSS_SEARCH;
> - mod_timer(&ifsta->timer, jiffies + interval);
> + schedule_delayed_work(&ifsta->work, interval);
> return 0;
> }
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]
next prev parent reply other threads:[~2006-10-18 18:10 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-10-07 9:23 [PATCH] d80211: ieee80211_hw handlers should be allowed to sleep Ivo van Doorn
2006-10-18 13:06 ` Jiri Benc
2006-10-18 17:27 ` Ivo van Doorn
2006-10-18 18:10 ` Jan Kiszka [this message]
2006-10-19 16:01 ` Jiri Benc
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=45366DFA.7030707@web.de \
--to=jan.kiszka@web.de \
--cc=ivdoorn@gmail.com \
--cc=jbenc@suse.cz \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.