2007-04-29 00:32:18

by Jiri Benc

[permalink] [raw]
Subject: [PATCH 1/4] mac80211: remove unused code

Remove code that is commented out and doesn't even compile when uncommented.

Signed-off-by: Jiri Benc <[email protected]>

---
net/mac80211/ieee80211_ioctl.c | 8 --------
1 files changed, 8 deletions(-)

--- dscape.orig/net/mac80211/ieee80211_ioctl.c
+++ dscape/net/mac80211/ieee80211_ioctl.c
@@ -1025,14 +1025,6 @@ static int ieee80211_ioctl_add_if(struct

res = ieee80211_if_add(dev, param->u.if_info.name, NULL,
IEEE80211_IF_TYPE_VLAN);
- if (res)
- return res;
-#if 0
- res = ieee80211_if_update_vlan(new_dev, vlan->id);
- if (res)
- __ieee80211_if_del(wdev_priv(dev->ieee80211_ptr),
- IEEE80211_DEV_TO_SUB_IF(new_dev));
-#endif
return res;
case HOSTAP_IF_BSS:
bss = (struct hostapd_if_bss *) param->u.if_info.data;



2007-04-29 00:33:47

by Jiri Benc

[permalink] [raw]
Subject: [PATCH 3/4] mac80211: document requirement for atomicity of callbacks

Some callbacks must be atomic. This is not documented anywhere.

Signed-off-by: Jiri Benc <[email protected]>

---
include/net/mac80211.h | 24 ++++++++++++++++--------
net/mac80211/ieee80211_i.h | 3 ++-
2 files changed, 18 insertions(+), 9 deletions(-)

--- dscape.orig/include/net/mac80211.h
+++ dscape/include/net/mac80211.h
@@ -582,7 +582,8 @@ struct ieee80211_ops {
/* Handler that 802.11 module calls for each transmitted frame.
* skb contains the buffer starting from the IEEE 802.11 header.
* The low-level driver should send the frame out based on
- * configuration in the TX control data. */
+ * configuration in the TX control data.
+ * Must be atomic. */
int (*tx)(struct ieee80211_hw *hw, struct sk_buff *skb,
struct ieee80211_tx_control *control);

@@ -631,7 +632,8 @@ struct ieee80211_ops {
* we need to combine the multicast lists and flags for multiple
* virtual interfaces), they cannot assign set_multicast_list.
* The parameters here replace dev->flags and dev->mc_count,
- * dev->mc_list is replaced by calling ieee80211_get_mc_list_item. */
+ * dev->mc_list is replaced by calling ieee80211_get_mc_list_item.
+ * Must be atomic. */
void (*set_multicast_list)(struct ieee80211_hw *hw,
unsigned short flags, int mc_count);

@@ -639,7 +641,8 @@ struct ieee80211_ops {
* generation, IEEE 802.11 code uses this function to tell the
* low-level to set (or clear if set==0) TIM bit for the given aid. If
* host system is used to generate beacons, this handler is not used
- * and low-level driver should set it to NULL. */
+ * and low-level driver should set it to NULL.
+ * Must be atomic. */
int (*set_tim)(struct ieee80211_hw *hw, int aid, int set);

/* Set encryption key. IEEE 802.11 module calls this function to set
@@ -647,7 +650,8 @@ struct ieee80211_ops {
* station hwaddr for individual keys. aid of the station is given
* to help low-level driver in selecting which key->hw_key_idx to use
* for this key. TX control data will use the hw_key_idx selected by
- * the low-level driver. */
+ * the low-level driver.
+ * Must be atomic. */
int (*set_key)(struct ieee80211_hw *hw, set_key_cmd cmd,
u8 *addr, struct ieee80211_key_conf *key, int aid);

@@ -675,7 +679,8 @@ struct ieee80211_ops {

/* Ask the hardware to do a passive scan on a new channel. The hardware
* will do what ever is required to nicely leave the current channel
- * including transmit any CTS packets, etc. */
+ * including transmit any CTS packets, etc.
+ * Must be atomic. */
int (*passive_scan)(struct ieee80211_hw *hw, int state,
struct ieee80211_scan_conf *conf);

@@ -712,13 +717,15 @@ struct ieee80211_ops {
int (*set_retry_limit)(struct ieee80211_hw *hw,
u32 short_retry, u32 long_retr);

- /* Number of STAs in STA table notification (NULL = disabled) */
+ /* Number of STAs in STA table notification (NULL = disabled).
+ * Must be atomic. */
void (*sta_table_notification)(struct ieee80211_hw *hw,
int num_sta);

/* Configure TX queue parameters (EDCF (aifs, cw_min, cw_max),
* bursting) for a hardware TX queue.
- * queue = IEEE80211_TX_QUEUE_*. */
+ * queue = IEEE80211_TX_QUEUE_*.
+ * Must be atomic. */
int (*conf_tx)(struct ieee80211_hw *hw, int queue,
const struct ieee80211_tx_queue_params *params);

@@ -733,7 +740,8 @@ struct ieee80211_ops {

/* Get the current TSF timer value from firmware/hardware. Currently,
* this is only used for IBSS mode debugging and, as such, is not a
- * required function. */
+ * required function.
+ * Must be atomic. */
u64 (*get_tsf)(struct ieee80211_hw *hw);

/* Call low level driver with 11n Block Ack action */
--- dscape.orig/net/mac80211/ieee80211_i.h
+++ dscape/net/mac80211/ieee80211_i.h
@@ -495,7 +495,8 @@ struct ieee80211_local {
ieee80211_rx_handler *rx_handlers;
ieee80211_tx_handler *tx_handlers;

- rwlock_t sub_if_lock; /* protects sub_if_list */
+ rwlock_t sub_if_lock; /* Protects sub_if_list. Cannot be taken under
+ * sta_bss_lock or sta_lock. */
struct list_head sub_if_list;
int sta_scanning;
int scan_channel_idx;

2007-04-29 09:15:58

by Michael Büsch

[permalink] [raw]
Subject: Re: [PATCH 3/4] mac80211: document requirement for atomicity of callbacks

On Sunday 29 April 2007 02:33:45 Jiri Benc wrote:
> @@ -631,7 +632,8 @@ struct ieee80211_ops {
> * we need to combine the multicast lists and flags for multiple
> * virtual interfaces), they cannot assign set_multicast_list.
> * The parameters here replace dev->flags and dev->mc_count,
> - * dev->mc_list is replaced by calling ieee80211_get_mc_list_item. */
> + * dev->mc_list is replaced by calling ieee80211_get_mc_list_item.
> + * Must be atomic. */
> void (*set_multicast_list)(struct ieee80211_hw *hw,
> unsigned short flags, int mc_count);

Why is that required to be atomic, actually?

> /* Set encryption key. IEEE 802.11 module calls this function to set
> @@ -647,7 +650,8 @@ struct ieee80211_ops {
> * station hwaddr for individual keys. aid of the station is given
> * to help low-level driver in selecting which key->hw_key_idx to use
> * for this key. TX control data will use the hw_key_idx selected by
> - * the low-level driver. */
> + * the low-level driver.
> + * Must be atomic. */
> int (*set_key)(struct ieee80211_hw *hw, set_key_cmd cmd,
> u8 *addr, struct ieee80211_key_conf *key, int aid);

I think USB drivers and bcm43xx violate that. We can't easily workaround it
without doing the operation async.


--
Greetings Michael.

2007-04-29 00:34:32

by Jiri Benc

[permalink] [raw]
Subject: [PATCH] mac80211: add copyrights

I have been lazy with adding my copyright to some files.

Signed-off-by: Jiri Benc <[email protected]>

---
include/net/mac80211.h | 1 +
net/mac80211/ieee80211.c | 1 +
net/mac80211/ieee80211_i.h | 1 +
net/mac80211/ieee80211_sta.c | 1 +
net/mac80211/sta_info.c | 1 +
5 files changed, 5 insertions(+)

--- dscape.orig/include/net/mac80211.h
+++ dscape/include/net/mac80211.h
@@ -1,6 +1,7 @@
/*
* Low-level hardware driver -- IEEE 802.11 driver (80211.o) interface
* Copyright 2002-2005, Devicescape Software, Inc.
+ * Copyright 2006-2007 Jiri Benc <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
--- dscape.orig/net/mac80211/ieee80211.c
+++ dscape/net/mac80211/ieee80211.c
@@ -1,6 +1,7 @@
/*
* Copyright 2002-2005, Instant802 Networks, Inc.
* Copyright 2005-2006, Devicescape Software, Inc.
+ * Copyright 2006-2007 Jiri Benc <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
--- dscape.orig/net/mac80211/ieee80211_i.h
+++ dscape/net/mac80211/ieee80211_i.h
@@ -1,6 +1,7 @@
/*
* Copyright 2002-2005, Instant802 Networks, Inc.
* Copyright 2005, Devicescape Software, Inc.
+ * Copyright 2006-2007 Jiri Benc <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
--- dscape.orig/net/mac80211/ieee80211_sta.c
+++ dscape/net/mac80211/ieee80211_sta.c
@@ -3,6 +3,7 @@
* Copyright 2003, Jouni Malinen <[email protected]>
* Copyright 2004, Instant802 Networks, Inc.
* Copyright 2005, Devicescape Software, Inc.
+ * Copyright 2006-2007 Jiri Benc <[email protected]>
* Copyright 2007, Michael Wu <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
--- dscape.orig/net/mac80211/sta_info.c
+++ dscape/net/mac80211/sta_info.c
@@ -1,5 +1,6 @@
/*
* Copyright 2002-2005, Instant802 Networks, Inc.
+ * Copyright 2006-2007 Jiri Benc <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as

2007-04-29 12:54:04

by Michael Büsch

[permalink] [raw]
Subject: Re: [PATCH 3/4] mac80211: document requirement for atomicity of callbacks

On Sunday 29 April 2007 12:49:29 Jiri Benc wrote:
> On Sun, 29 Apr 2007 11:15:43 +0200, Michael Buesch wrote:
> > On Sunday 29 April 2007 02:33:45 Jiri Benc wrote:
> > > + * Must be atomic. */
> > > int (*set_key)(struct ieee80211_hw *hw, set_key_cmd cmd,
> > > u8 *addr, struct ieee80211_key_conf *key, int aid);
> >
> > I think USB drivers and bcm43xx violate that. We can't easily workaround it
> > without doing the operation async.
>
> I know. Unfortunately, it's not easy to fix that in mac80211. I really want
> this call not to be atomic and will try to change it later, but it's not a
> priority now.
>
> For now, just always return success and complain to dmesg if something goes
> wrong during setting of the key in a workqueue.

Ok, I see. But I want to note that this might be more problematic
then just a lost error code. It might race.
For example it might race with the EAPOL KEY authentication stuff.
So I think we must also workaround that by disabling TX queues
until the key configuration request has actually happened
on the device.

--
Greetings Michael.

2007-04-29 09:21:49

by Ivo Van Doorn

[permalink] [raw]
Subject: Re: [PATCH 3/4] mac80211: document requirement for atomicity of callbacks

On Sunday 29 April 2007 11:15, Michael Buesch wrote:
> On Sunday 29 April 2007 02:33:45 Jiri Benc wrote:
> > @@ -631,7 +632,8 @@ struct ieee80211_ops {
> > * we need to combine the multicast lists and flags for multiple
> > * virtual interfaces), they cannot assign set_multicast_list.
> > * The parameters here replace dev->flags and dev->mc_count,
> > - * dev->mc_list is replaced by calling ieee80211_get_mc_list_item. */
> > + * dev->mc_list is replaced by calling ieee80211_get_mc_list_item.
> > + * Must be atomic. */
> > void (*set_multicast_list)(struct ieee80211_hw *hw,
> > unsigned short flags, int mc_count);
>
> Why is that required to be atomic, actually?

Because set_multicast_list callback inside the netdevice structure
is also called atomically. So if this should be moved out of atomic context
either the kernel shouldn't call it from atomic context or mac80211 needs to
reschedule the call to the driver.

Ivo

2007-04-29 00:33:05

by Jiri Benc

[permalink] [raw]
Subject: [PATCH 2/4] mac80211: remove test_mode

Testing of a radio is a driver-specific debug feature and doesn't belong to
the stack. Drivers should implement it using debugfs.

Signed-off-by: Jiri Benc <[email protected]>

---
include/net/mac80211.h | 28 ------------------
net/mac80211/hostapd_ioctl.h | 2 -
net/mac80211/ieee80211_ioctl.c | 63 -----------------------------------------
3 files changed, 93 deletions(-)

--- dscape.orig/include/net/mac80211.h
+++ dscape/include/net/mac80211.h
@@ -687,12 +687,6 @@ struct ieee80211_ops {
int (*get_stats)(struct ieee80211_hw *hw,
struct ieee80211_low_level_stats *stats);

- /* Enable/disable test modes; mode = IEEE80211_TEST_* */
- int (*test_mode)(struct ieee80211_hw *hw, int mode);
-
- /* Configuration of test parameters */
- int (*test_param)(struct ieee80211_hw *hw, int param, int value);
-
/* For devices that generate their own beacons and probe response
* or association responses this updates the state of privacy_invoked
* returns 0 for success or an error number */
@@ -1056,28 +1050,6 @@ void ieee80211_scan_completed(struct iee
int ieee80211_radar_status(struct ieee80211_hw *hw, int channel,
int radar, int radar_type);

-/* Test modes */
-enum {
- IEEE80211_TEST_DISABLE = 0 /* terminate testing */,
- IEEE80211_TEST_UNMASK_CHANNELS = 1 /* allow all channels to be used */,
- IEEE80211_TEST_CONTINUOUS_TX = 2,
-};
-
-/* Test parameters */
-enum {
- /* TX power in hardware specific raw value */
- IEEE80211_TEST_PARAM_TX_POWER_RAW = 0,
- /* TX rate in hardware specific raw value */
- IEEE80211_TEST_PARAM_TX_RATE_RAW = 1,
- /* Continuous TX pattern (32-bit) */
- IEEE80211_TEST_PARAM_TX_PATTERN = 2,
- /* TX power in 0.1 dBm, 100 = 10 dBm */
- IEEE80211_TEST_PARAM_TX_POWER = 3,
- /* TX rate in 100 kbps, 540 = 54 Mbps */
- IEEE80211_TEST_PARAM_TX_RATE = 4,
- IEEE80211_TEST_PARAM_TX_ANT_SEL_RAW = 5,
-};
-
/* return a pointer to the source address (SA) */
static inline u8 *ieee80211_get_SA(struct ieee80211_hdr *hdr)
{
--- dscape.orig/net/mac80211/hostapd_ioctl.h
+++ dscape/net/mac80211/hostapd_ioctl.h
@@ -20,7 +20,6 @@
#define PRISM2_IOCTL_PRISM2_PARAM (SIOCIWFIRSTPRIV + 0)
#define PRISM2_IOCTL_GET_PRISM2_PARAM (SIOCIWFIRSTPRIV + 1)
#define PRISM2_IOCTL_HOSTAPD (SIOCIWFIRSTPRIV + 3)
-#define PRISM2_IOCTL_TEST_PARAM (SIOCIWFIRSTPRIV + 4)

/* PRISM2_IOCTL_PRISM2_PARAM ioctl() subtypes:
* This table is no longer added to, the whole sub-ioctl
@@ -41,7 +40,6 @@ enum {
PRISM2_PARAM_DROP_UNENCRYPTED = 1002,
PRISM2_PARAM_PREAMBLE = 1003,
PRISM2_PARAM_SHORT_SLOT_TIME = 1006,
- PRISM2_PARAM_TEST_MODE = 1007,
PRISM2_PARAM_NEXT_MODE = 1008,
PRISM2_PARAM_CLEAR_KEYS = 1009,
PRISM2_PARAM_RADIO_ENABLED = 1010,
--- dscape.orig/net/mac80211/ieee80211_ioctl.c
+++ dscape/net/mac80211/ieee80211_ioctl.c
@@ -2189,38 +2189,6 @@ static int ieee80211_ioctl_giwretry(stru
return 0;
}

-
-static void ieee80211_ioctl_unmask_channels(struct ieee80211_local *local)
-{
- struct ieee80211_hw_mode *mode;
- int c;
-
- list_for_each_entry(mode, &local->modes_list, list) {
- for (c = 0; c < mode->num_channels; c++) {
- struct ieee80211_channel *chan = &mode->channels[c];
- chan->flag |= IEEE80211_CHAN_W_SCAN;
- }
- }
-}
-
-
-static int ieee80211_ioctl_test_mode(struct net_device *dev, int mode)
-{
- struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
- int ret = -EOPNOTSUPP;
-
- if (mode == IEEE80211_TEST_UNMASK_CHANNELS) {
- ieee80211_ioctl_unmask_channels(local);
- ret = 0;
- }
-
- if (local->ops->test_mode)
- ret = local->ops->test_mode(local_to_hw(local), mode);
-
- return ret;
-}
-
-
static int ieee80211_ioctl_clear_keys(struct net_device *dev)
{
struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
@@ -2537,10 +2505,6 @@ static int ieee80211_ioctl_prism2_param(
local_to_hw(local), value);
break;

- case PRISM2_PARAM_TEST_MODE:
- ret = ieee80211_ioctl_test_mode(dev, value);
- break;
-
case PRISM2_PARAM_NEXT_MODE:
local->next_mode = value;
break;
@@ -2871,27 +2835,6 @@ static int ieee80211_ioctl_get_prism2_pa
return ret;
}

-
-static int ieee80211_ioctl_test_param(struct net_device *dev,
- struct iw_request_info *info,
- void *wrqu, char *extra)
-{
- struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
- int *i = (int *) extra;
- int param = *i;
- int value = *(i + 1);
-
- if (!capable(CAP_NET_ADMIN))
- return -EPERM;
-
- if (local->ops->test_param)
- return local->ops->test_param(local_to_hw(local),
- param, value);
-
- return -EOPNOTSUPP;
-}
-
-
static int ieee80211_ioctl_siwmlme(struct net_device *dev,
struct iw_request_info *info,
struct iw_point *data, char *extra)
@@ -3177,8 +3120,6 @@ static const struct iw_priv_args ieee802
{ PRISM2_IOCTL_GET_PRISM2_PARAM,
IW_PRIV_TYPE_INT | IW_PRIV_SIZE_FIXED | 1,
IW_PRIV_TYPE_INT | IW_PRIV_SIZE_FIXED | 1, "get_param" },
- { PRISM2_IOCTL_TEST_PARAM,
- IW_PRIV_TYPE_INT | IW_PRIV_SIZE_FIXED | 2, 0, "test_param" },
};


@@ -3190,10 +3131,6 @@ int ieee80211_ioctl(struct net_device *d
switch (cmd) {
/* Private ioctls (iwpriv) that have not yet been converted
* into new wireless extensions API */
- case PRISM2_IOCTL_TEST_PARAM:
- ret = ieee80211_ioctl_test_param(dev, NULL, &wrq->u,
- (char *) &wrq->u);
- break;
case PRISM2_IOCTL_HOSTAPD:
if (!capable(CAP_NET_ADMIN)) ret = -EPERM;
else ret = ieee80211_ioctl_priv_hostapd(dev, &wrq->u.data);

2007-04-29 10:49:35

by Jiri Benc

[permalink] [raw]
Subject: Re: [PATCH 3/4] mac80211: document requirement for atomicity of callbacks

On Sun, 29 Apr 2007 11:15:43 +0200, Michael Buesch wrote:
> On Sunday 29 April 2007 02:33:45 Jiri Benc wrote:
> > + * Must be atomic. */
> > int (*set_key)(struct ieee80211_hw *hw, set_key_cmd cmd,
> > u8 *addr, struct ieee80211_key_conf *key, int aid);
>
> I think USB drivers and bcm43xx violate that. We can't easily workaround it
> without doing the operation async.

I know. Unfortunately, it's not easy to fix that in mac80211. I really want
this call not to be atomic and will try to change it later, but it's not a
priority now.

For now, just always return success and complain to dmesg if something goes
wrong during setting of the key in a workqueue.

Thanks,

Jiri

--
Jiri Benc
SUSE Labs