2008-12-08 19:45:02

by Johannes Berg

[permalink] [raw]
Subject: [RFT] mac80211: clean up set_key callback

The set_key callback now seems rather odd, passing a MAC address
instead of a station struct, and a local address instead of a
vif struct. Change that.

Signed-off-by: Johannes Berg <[email protected]>
---
That all-zeroes vs. all-ones for group/tx-only keys was confusing and is
removed, this could possibly affect drivers that support multi-BSS
operation.

iwlwifi said:

if (is_zero_ether_addr(addr))
/* only support pairwise keys */
return -EOPNOTSUPP;

but the comment is obviously wrong since we used to pass in all-ones for
the address for group keys, so it did support them. Change this
accordingly, and ignore the multi-BSS case since iwlwifi doesn't support
AP operation at all.

drivers/net/wireless/ath5k/base.c | 9 ++--
drivers/net/wireless/ath9k/main.c | 20 ++++------
drivers/net/wireless/b43/main.c | 18 ++++-----
drivers/net/wireless/iwlwifi/iwl-agn.c | 10 +++--
drivers/net/wireless/iwlwifi/iwl3945-base.c | 12 +++---
drivers/net/wireless/p54/p54common.c | 6 +--
drivers/net/wireless/rt2x00/rt2x00.h | 2 -
drivers/net/wireless/rt2x00/rt2x00mac.c | 14 +++----
include/net/mac80211.h | 20 ++++------
net/mac80211/key.c | 53 ++++++++++++++--------------
10 files changed, 80 insertions(+), 84 deletions(-)

--- everything.orig/include/net/mac80211.h 2008-12-08 17:25:24.000000000 +0100
+++ everything/include/net/mac80211.h 2008-12-08 17:58:51.000000000 +0100
@@ -710,8 +710,8 @@ enum ieee80211_key_flags {
* - Temporal Encryption Key (128 bits)
* - Temporal Authenticator Tx MIC Key (64 bits)
* - Temporal Authenticator Rx MIC Key (64 bits)
- * @icv_len: FIXME
- * @iv_len: FIXME
+ * @icv_len: The ICV length for this key type
+ * @iv_len: The IV length for this key type
*/
struct ieee80211_key_conf {
enum ieee80211_key_alg alg;
@@ -1017,16 +1017,12 @@ ieee80211_get_alt_retry_rate(const struc
*
* The set_key() callback in the &struct ieee80211_ops for a given
* device is called to enable hardware acceleration of encryption and
- * decryption. The callback takes an @address parameter that will be
- * the broadcast address for default keys, the other station's hardware
- * address for individual keys or the zero address for keys that will
- * be used only for transmission.
+ * decryption. The callback takes a @sta parameter that will be NULL
+ * for default keys or keys used for transmission only, or point to
+ * the station information for the peer for individual keys.
* Multiple transmission keys with the same key index may be used when
* VLANs are configured for an access point.
*
- * The @local_address parameter will always be set to our own address,
- * this is only relevant if you support multiple local addresses.
- *
* When transmitting, the TX control data will use the @hw_key_idx
* selected by the driver by modifying the &struct ieee80211_key_conf
* pointed to by the @key parameter to the set_key() function.
@@ -1231,8 +1227,8 @@ enum ieee80211_ampdu_mlme_action {
*
* @set_key: See the section "Hardware crypto acceleration"
* This callback can sleep, and is only called between add_interface
- * and remove_interface calls, i.e. while the interface with the
- * given local_address is enabled.
+ * and remove_interface calls, i.e. while the given virtual interface
+ * is enabled.
*
* @update_tkip_key: See the section "Hardware crypto acceleration"
* This callback will be called in the context of Rx. Called for drivers
@@ -1311,7 +1307,7 @@ struct ieee80211_ops {
int (*set_tim)(struct ieee80211_hw *hw, struct ieee80211_sta *sta,
bool set);
int (*set_key)(struct ieee80211_hw *hw, enum set_key_cmd cmd,
- const u8 *local_address, const u8 *address,
+ struct ieee80211_vif *vif, struct ieee80211_sta *sta,
struct ieee80211_key_conf *key);
void (*update_tkip_key)(struct ieee80211_hw *hw,
struct ieee80211_key_conf *conf, const u8 *address,
--- everything.orig/net/mac80211/key.c 2008-12-08 17:26:21.000000000 +0100
+++ everything/net/mac80211/key.c 2008-12-08 17:38:32.000000000 +0100
@@ -47,7 +47,6 @@
*/

static const u8 bcast_addr[ETH_ALEN] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
-static const u8 zero_addr[ETH_ALEN];

/* key mutex: used to synchronise todo runners */
static DEFINE_MUTEX(key_mutex);
@@ -108,29 +107,18 @@ static void assert_key_lock(void)
WARN_ON(!mutex_is_locked(&key_mutex));
}

-static const u8 *get_mac_for_key(struct ieee80211_key *key)
+static struct ieee80211_sta *get_sta_for_key(struct ieee80211_key *key)
{
- const u8 *addr = bcast_addr;
-
- /*
- * If we're an AP we won't ever receive frames with a non-WEP
- * group key so we tell the driver that by using the zero MAC
- * address to indicate a transmit-only key.
- */
- if (key->conf.alg != ALG_WEP &&
- (key->sdata->vif.type == NL80211_IFTYPE_AP ||
- key->sdata->vif.type == NL80211_IFTYPE_AP_VLAN))
- addr = zero_addr;
-
if (key->sta)
- addr = key->sta->sta.addr;
+ return &key->sta->sta;

- return addr;
+ return NULL;
}

static void ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
{
- const u8 *addr;
+ struct ieee80211_sub_if_data *sdata;
+ struct ieee80211_sta *sta;
int ret;
DECLARE_MAC_BUF(mac);

@@ -140,11 +128,16 @@ static void ieee80211_key_enable_hw_acce
if (!key->local->ops->set_key)
return;

- addr = get_mac_for_key(key);
+ sta = get_sta_for_key(key);
+
+ sdata = key->sdata;
+ if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
+ sdata = container_of(sdata->bss,
+ struct ieee80211_sub_if_data,
+ u.ap);

ret = key->local->ops->set_key(local_to_hw(key->local), SET_KEY,
- key->sdata->dev->dev_addr, addr,
- &key->conf);
+ &sdata->vif, sta, &key->conf);

if (!ret) {
spin_lock(&todo_lock);
@@ -156,12 +149,14 @@ static void ieee80211_key_enable_hw_acce
printk(KERN_ERR "mac80211-%s: failed to set key "
"(%d, %s) to hardware (%d)\n",
wiphy_name(key->local->hw.wiphy),
- key->conf.keyidx, print_mac(mac, addr), ret);
+ key->conf.keyidx,
+ print_mac(mac, sta ? sta->addr : bcast_addr), ret);
}

static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
{
- const u8 *addr;
+ struct ieee80211_sub_if_data *sdata;
+ struct ieee80211_sta *sta;
int ret;
DECLARE_MAC_BUF(mac);

@@ -178,17 +173,23 @@ static void ieee80211_key_disable_hw_acc
}
spin_unlock(&todo_lock);

- addr = get_mac_for_key(key);
+ sta = get_sta_for_key(key);
+ sdata = key->sdata;
+
+ if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
+ sdata = container_of(sdata->bss,
+ struct ieee80211_sub_if_data,
+ u.ap);

ret = key->local->ops->set_key(local_to_hw(key->local), DISABLE_KEY,
- key->sdata->dev->dev_addr, addr,
- &key->conf);
+ &sdata->vif, sta, &key->conf);

if (ret)
printk(KERN_ERR "mac80211-%s: failed to remove key "
"(%d, %s) from hardware (%d)\n",
wiphy_name(key->local->hw.wiphy),
- key->conf.keyidx, print_mac(mac, addr), ret);
+ key->conf.keyidx,
+ print_mac(mac, sta ? sta->addr : bcast_addr), ret);

spin_lock(&todo_lock);
key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
--- everything.orig/drivers/net/wireless/ath5k/base.c 2008-12-08 17:39:05.000000000 +0100
+++ everything/drivers/net/wireless/ath5k/base.c 2008-12-08 17:41:03.000000000 +0100
@@ -232,7 +232,7 @@ static void ath5k_configure_filter(struc
int mc_count, struct dev_mc_list *mclist);
static int ath5k_set_key(struct ieee80211_hw *hw,
enum set_key_cmd cmd,
- const u8 *local_addr, const u8 *addr,
+ struct ieee80211_vif *vif, struct ieee80211_sta *sta,
struct ieee80211_key_conf *key);
static int ath5k_get_stats(struct ieee80211_hw *hw,
struct ieee80211_low_level_stats *stats);
@@ -2986,8 +2986,8 @@ static void ath5k_configure_filter(struc

static int
ath5k_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
- const u8 *local_addr, const u8 *addr,
- struct ieee80211_key_conf *key)
+ struct ieee80211_vif *vif, struct ieee80211_sta *sta,
+ struct ieee80211_key_conf *key)
{
struct ath5k_softc *sc = hw->priv;
int ret = 0;
@@ -3010,7 +3010,8 @@ ath5k_set_key(struct ieee80211_hw *hw, e

switch (cmd) {
case SET_KEY:
- ret = ath5k_hw_set_key(sc->ah, key->keyidx, key, addr);
+ ret = ath5k_hw_set_key(sc->ah, key->keyidx, key,
+ sta ? sta->addr : NULL);
if (ret) {
ATH5K_ERR(sc, "can't set the key\n");
goto unlock;
--- everything.orig/drivers/net/wireless/ath9k/main.c 2008-12-08 17:41:24.000000000 +0100
+++ everything/drivers/net/wireless/ath9k/main.c 2008-12-08 18:10:07.000000000 +0100
@@ -724,9 +724,10 @@ static int ath_key_config(struct ath_sof
{
struct ieee80211_vif *vif;
struct ath9k_keyval hk;
- const u8 *mac = NULL;
int ret = 0;
enum nl80211_iftype opmode;
+ static const u8 bcast_addr[ETH_ALEN] =
+ { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, };

memset(&hk, 0, sizeof(hk));

@@ -767,29 +768,26 @@ static int ath_key_config(struct ath_sof
* BUT we will plumb a cleartext key so that we can do
* per-Sta default key table lookup in software.
*/
- if (is_broadcast_ether_addr(addr)) {
+ if (!addr) {
switch (opmode) {
case NL80211_IFTYPE_STATION:
/* default key: could be group WPA key
* or could be static WEP key */
- mac = NULL;
break;
case NL80211_IFTYPE_ADHOC:
- break;
case NL80211_IFTYPE_AP:
+ addr = bcast_addr;
break;
default:
ASSERT(0);
break;
}
- } else {
- mac = addr;
}

if (key->alg == ALG_TKIP)
- ret = ath_setkey_tkip(sc, key, &hk, mac);
+ ret = ath_setkey_tkip(sc, key, &hk, addr);
else
- ret = ath_keyset(sc, key->keyidx, &hk, mac);
+ ret = ath_keyset(sc, key->keyidx, &hk, addr);

if (!ret)
return -EIO;
@@ -2302,8 +2300,8 @@ static int ath9k_conf_tx(struct ieee8021

static int ath9k_set_key(struct ieee80211_hw *hw,
enum set_key_cmd cmd,
- const u8 *local_addr,
- const u8 *addr,
+ struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
struct ieee80211_key_conf *key)
{
struct ath_softc *sc = hw->priv;
@@ -2313,7 +2311,7 @@ static int ath9k_set_key(struct ieee8021

switch (cmd) {
case SET_KEY:
- ret = ath_key_config(sc, addr, key);
+ ret = ath_key_config(sc, sta ? sta->addr : NULL, key);
if (!ret) {
set_bit(key->keyidx, sc->sc_keymap);
key->hw_key_idx = key->keyidx;
--- everything.orig/drivers/net/wireless/b43/main.c 2008-12-08 17:42:05.000000000 +0100
+++ everything/drivers/net/wireless/b43/main.c 2008-12-08 17:49:40.000000000 +0100
@@ -3508,8 +3508,8 @@ static void b43_op_bss_info_changed(stru
}

static int b43_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
- const u8 *local_addr, const u8 *addr,
- struct ieee80211_key_conf *key)
+ struct ieee80211_vif *vif, struct ieee80211_sta *sta,
+ struct ieee80211_key_conf *key)
{
struct b43_wl *wl = hw_to_b43_wl(hw);
struct b43_wldev *dev;
@@ -3567,17 +3567,13 @@ static int b43_op_set_key(struct ieee802
goto out_unlock;
}

- if (is_broadcast_ether_addr(addr)) {
+ if (!sta) {
/* addr is FF:FF:FF:FF:FF:FF for default keys */
err = b43_key_write(dev, index, algorithm,
key->key, key->keylen, NULL, key);
} else {
- /*
- * either pairwise key or address is 00:00:00:00:00:00
- * for transmit-only keys
- */
- err = b43_key_write(dev, -1, algorithm,
- key->key, key->keylen, addr, key);
+ err = b43_key_write(dev, -1, algorithm, key->key,
+ key->keylen, sta->addr, key);
}
if (err)
goto out_unlock;
@@ -3604,10 +3600,12 @@ out_unlock:
spin_unlock_irqrestore(&wl->irq_lock, flags);
mutex_unlock(&wl->mutex);
if (!err) {
+ static const u8 bcast_addr[ETH_ALEN] =
+ { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
b43dbg(wl, "%s hardware based encryption for keyidx: %d, "
"mac: %s\n",
cmd == SET_KEY ? "Using" : "Disabling", key->keyidx,
- print_mac(mac, addr));
+ print_mac(mac, sta ? sta->addr : bcast_addr));
}
return err;
}
--- everything.orig/drivers/net/wireless/iwlwifi/iwl-agn.c 2008-12-08 17:46:45.000000000 +0100
+++ everything/drivers/net/wireless/iwlwifi/iwl-agn.c 2008-12-08 18:13:37.000000000 +0100
@@ -3039,7 +3039,8 @@ static void iwl_mac_update_tkip_key(stru
}

static int iwl_mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
- const u8 *local_addr, const u8 *addr,
+ struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
struct ieee80211_key_conf *key)
{
struct iwl_priv *priv = hw->priv;
@@ -3047,6 +3048,9 @@ static int iwl_mac_set_key(struct ieee80
int ret = 0;
u8 sta_id = IWL_INVALID_STATION;
u8 is_default_wep_key = 0;
+ static const u8 bcast_addr[ETH_ALEN] =
+ { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, };
+ static const u8 *addr;

IWL_DEBUG_MAC80211("enter\n");

@@ -3055,9 +3059,7 @@ static int iwl_mac_set_key(struct ieee80
return -EOPNOTSUPP;
}

- if (is_zero_ether_addr(addr))
- /* only support pairwise keys */
- return -EOPNOTSUPP;
+ addr = sta ? sta->addr : bcast_addr;

sta_id = iwl_find_station(priv, addr);
if (sta_id == IWL_INVALID_STATION) {
--- everything.orig/drivers/net/wireless/iwlwifi/iwl3945-base.c 2008-12-08 17:46:16.000000000 +0100
+++ everything/drivers/net/wireless/iwlwifi/iwl3945-base.c 2008-12-08 18:14:45.000000000 +0100
@@ -7033,12 +7033,16 @@ out_unlock:
}

static int iwl3945_mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
- const u8 *local_addr, const u8 *addr,
- struct ieee80211_key_conf *key)
+ struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta,
+ struct ieee80211_key_conf *key)
{
struct iwl3945_priv *priv = hw->priv;
int rc = 0;
u8 sta_id;
+ static const u8 bcast_addr[ETH_ALEN] =
+ { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, };
+ const u8 *addr;

IWL_DEBUG_MAC80211("enter\n");

@@ -7047,9 +7051,7 @@ static int iwl3945_mac_set_key(struct ie
return -EOPNOTSUPP;
}

- if (is_zero_ether_addr(addr))
- /* only support pairwise keys */
- return -EOPNOTSUPP;
+ addr = sta ? sta->addr : bcast_addr;

sta_id = iwl3945_hw_find_station(priv, addr);
if (sta_id == IWL_INVALID_STATION) {
--- everything.orig/drivers/net/wireless/p54/p54common.c 2008-12-08 17:47:11.000000000 +0100
+++ everything/drivers/net/wireless/p54/p54common.c 2008-12-08 17:53:09.000000000 +0100
@@ -1938,7 +1938,7 @@ static void p54_bss_info_changed(struct
}

static int p54_set_key(struct ieee80211_hw *dev, enum set_key_cmd cmd,
- const u8 *local_address, const u8 *address,
+ struct ieee80211_vif *vif, struct ieee80211_sta *sta,
struct ieee80211_key_conf *key)
{
struct p54_common *priv = dev->priv;
@@ -2002,8 +2002,8 @@ static int p54_set_key(struct ieee80211_
rxkey->entry = key->keyidx;
rxkey->key_id = key->keyidx;
rxkey->key_type = algo;
- if (address)
- memcpy(rxkey->mac, address, ETH_ALEN);
+ if (sta)
+ memcpy(rxkey->mac, sta->addr, ETH_ALEN);
else
memset(rxkey->mac, ~0, ETH_ALEN);
if (key->alg != ALG_TKIP) {
--- everything.orig/drivers/net/wireless/rt2x00/rt2x00.h 2008-12-08 17:47:42.000000000 +0100
+++ everything/drivers/net/wireless/rt2x00/rt2x00.h 2008-12-08 17:47:52.000000000 +0100
@@ -982,7 +982,7 @@ void rt2x00mac_configure_filter(struct i
int mc_count, struct dev_addr_list *mc_list);
#ifdef CONFIG_RT2X00_LIB_CRYPTO
int rt2x00mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
- const u8 *local_address, const u8 *address,
+ struct ieee80211_vif *vif, struct ieee80211_sta *sta,
struct ieee80211_key_conf *key);
#else
#define rt2x00mac_set_key NULL
--- everything.orig/drivers/net/wireless/rt2x00/rt2x00mac.c 2008-12-08 17:48:03.000000000 +0100
+++ everything/drivers/net/wireless/rt2x00/rt2x00mac.c 2008-12-08 17:57:43.000000000 +0100
@@ -483,15 +483,17 @@ EXPORT_SYMBOL_GPL(rt2x00mac_configure_fi

#ifdef CONFIG_RT2X00_LIB_CRYPTO
int rt2x00mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
- const u8 *local_address, const u8 *address,
+ struct ieee80211_vif *vif, struct ieee80211_sta *sta,
struct ieee80211_key_conf *key)
{
struct rt2x00_dev *rt2x00dev = hw->priv;
- struct ieee80211_sta *sta;
+ struct rt2x00_intf *intf = vif_to_intf(vif);
int (*set_key) (struct rt2x00_dev *rt2x00dev,
struct rt2x00lib_crypto *crypto,
struct ieee80211_key_conf *key);
struct rt2x00lib_crypto crypto;
+ static const u8 bcast_addr[ETH_ALEN] =
+ { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, };

if (!test_bit(DEVICE_STATE_PRESENT, &rt2x00dev->flags))
return 0;
@@ -509,15 +511,14 @@ int rt2x00mac_set_key(struct ieee80211_h
if (rt2x00dev->intf_sta_count)
crypto.bssidx = 0;
else
- crypto.bssidx =
- local_address[5] & (rt2x00dev->ops->max_ap_intf - 1);
+ crypto.bssidx = intf->mac[5] & (rt2x00dev->ops->max_ap_intf - 1);

crypto.cipher = rt2x00crypto_key_to_cipher(key);
if (crypto.cipher == CIPHER_NONE)
return -EOPNOTSUPP;

crypto.cmd = cmd;
- crypto.address = address;
+ crypto.address = sta ? sta->addr : bcast_addr;

if (crypto.cipher == CIPHER_TKIP) {
if (key->keylen > NL80211_TKIP_DATA_OFFSET_ENCR_KEY)
@@ -542,11 +543,8 @@ int rt2x00mac_set_key(struct ieee80211_h
* Some drivers need this information when updating the
* hardware key (either adding or removing).
*/
- rcu_read_lock();
- sta = ieee80211_find_sta(hw, address);
if (sta)
crypto.aid = sta->aid;
- rcu_read_unlock();

/*
* Each BSS has a maximum of 4 shared keys.




2008-12-08 20:14:14

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFT] mac80211: clean up set_key callback

On Mon, 2008-12-08 at 21:11 +0100, Michael Buesch wrote:

> > - if (is_broadcast_ether_addr(addr)) {
> > + if (!sta) {
> > /* addr is FF:FF:FF:FF:FF:FF for default keys */
>
> Please also remove this comment.

Ok.

> > @@ -3604,10 +3600,12 @@ out_unlock:
> > spin_unlock_irqrestore(&wl->irq_lock, flags);
> > mutex_unlock(&wl->mutex);
> > if (!err) {
> > + static const u8 bcast_addr[ETH_ALEN] =
> > + { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
> > b43dbg(wl, "%s hardware based encryption for keyidx: %d, "
> > "mac: %s\n",
> > cmd == SET_KEY ? "Using" : "Disabling", key->keyidx,
> > - print_mac(mac, addr));
> > + print_mac(mac, sta ? sta->addr : bcast_addr));
>
> Will throw "unused variable bcast_addr" for !DEBUG.
> Use something like
> sta ? print_mac(mac, sta->addr) : "group/tx-only");

Crap. What you're proposing won't work well either because then this
code cannot be converted to %pM... I guess I'll have mark the variable
__maybe_unused or #ifdef it.

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-12-09 10:42:51

by Samuel Ortiz

[permalink] [raw]
Subject: Re: [RFT] mac80211: clean up set_key callback

On Mon, Dec 08, 2008 at 06:23:55PM +0100, Johannes Berg wrote:
> The set_key callback now seems rather odd, passing a MAC address
> instead of a station struct, and a local address instead of a
> vif struct. Change that.
Seems to work for iwl3945, thanks.

Cheers,
Samuel.


> Signed-off-by: Johannes Berg <[email protected]>
> ---
> That all-zeroes vs. all-ones for group/tx-only keys was confusing and is
> removed, this could possibly affect drivers that support multi-BSS
> operation.
>
> iwlwifi said:
>
> if (is_zero_ether_addr(addr))
> /* only support pairwise keys */
> return -EOPNOTSUPP;
>
> but the comment is obviously wrong since we used to pass in all-ones for
> the address for group keys, so it did support them. Change this
> accordingly, and ignore the multi-BSS case since iwlwifi doesn't support
> AP operation at all.
>
> drivers/net/wireless/ath5k/base.c | 9 ++--
> drivers/net/wireless/ath9k/main.c | 20 ++++------
> drivers/net/wireless/b43/main.c | 18 ++++-----
> drivers/net/wireless/iwlwifi/iwl-agn.c | 10 +++--
> drivers/net/wireless/iwlwifi/iwl3945-base.c | 12 +++---
> drivers/net/wireless/p54/p54common.c | 6 +--
> drivers/net/wireless/rt2x00/rt2x00.h | 2 -
> drivers/net/wireless/rt2x00/rt2x00mac.c | 14 +++----
> include/net/mac80211.h | 20 ++++------
> net/mac80211/key.c | 53 ++++++++++++++--------------
> 10 files changed, 80 insertions(+), 84 deletions(-)
>
> --- everything.orig/include/net/mac80211.h 2008-12-08 17:25:24.000000000 +0100
> +++ everything/include/net/mac80211.h 2008-12-08 17:58:51.000000000 +0100
> @@ -710,8 +710,8 @@ enum ieee80211_key_flags {
> * - Temporal Encryption Key (128 bits)
> * - Temporal Authenticator Tx MIC Key (64 bits)
> * - Temporal Authenticator Rx MIC Key (64 bits)
> - * @icv_len: FIXME
> - * @iv_len: FIXME
> + * @icv_len: The ICV length for this key type
> + * @iv_len: The IV length for this key type
> */
> struct ieee80211_key_conf {
> enum ieee80211_key_alg alg;
> @@ -1017,16 +1017,12 @@ ieee80211_get_alt_retry_rate(const struc
> *
> * The set_key() callback in the &struct ieee80211_ops for a given
> * device is called to enable hardware acceleration of encryption and
> - * decryption. The callback takes an @address parameter that will be
> - * the broadcast address for default keys, the other station's hardware
> - * address for individual keys or the zero address for keys that will
> - * be used only for transmission.
> + * decryption. The callback takes a @sta parameter that will be NULL
> + * for default keys or keys used for transmission only, or point to
> + * the station information for the peer for individual keys.
> * Multiple transmission keys with the same key index may be used when
> * VLANs are configured for an access point.
> *
> - * The @local_address parameter will always be set to our own address,
> - * this is only relevant if you support multiple local addresses.
> - *
> * When transmitting, the TX control data will use the @hw_key_idx
> * selected by the driver by modifying the &struct ieee80211_key_conf
> * pointed to by the @key parameter to the set_key() function.
> @@ -1231,8 +1227,8 @@ enum ieee80211_ampdu_mlme_action {
> *
> * @set_key: See the section "Hardware crypto acceleration"
> * This callback can sleep, and is only called between add_interface
> - * and remove_interface calls, i.e. while the interface with the
> - * given local_address is enabled.
> + * and remove_interface calls, i.e. while the given virtual interface
> + * is enabled.
> *
> * @update_tkip_key: See the section "Hardware crypto acceleration"
> * This callback will be called in the context of Rx. Called for drivers
> @@ -1311,7 +1307,7 @@ struct ieee80211_ops {
> int (*set_tim)(struct ieee80211_hw *hw, struct ieee80211_sta *sta,
> bool set);
> int (*set_key)(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> - const u8 *local_address, const u8 *address,
> + struct ieee80211_vif *vif, struct ieee80211_sta *sta,
> struct ieee80211_key_conf *key);
> void (*update_tkip_key)(struct ieee80211_hw *hw,
> struct ieee80211_key_conf *conf, const u8 *address,
> --- everything.orig/net/mac80211/key.c 2008-12-08 17:26:21.000000000 +0100
> +++ everything/net/mac80211/key.c 2008-12-08 17:38:32.000000000 +0100
> @@ -47,7 +47,6 @@
> */
>
> static const u8 bcast_addr[ETH_ALEN] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF };
> -static const u8 zero_addr[ETH_ALEN];
>
> /* key mutex: used to synchronise todo runners */
> static DEFINE_MUTEX(key_mutex);
> @@ -108,29 +107,18 @@ static void assert_key_lock(void)
> WARN_ON(!mutex_is_locked(&key_mutex));
> }
>
> -static const u8 *get_mac_for_key(struct ieee80211_key *key)
> +static struct ieee80211_sta *get_sta_for_key(struct ieee80211_key *key)
> {
> - const u8 *addr = bcast_addr;
> -
> - /*
> - * If we're an AP we won't ever receive frames with a non-WEP
> - * group key so we tell the driver that by using the zero MAC
> - * address to indicate a transmit-only key.
> - */
> - if (key->conf.alg != ALG_WEP &&
> - (key->sdata->vif.type == NL80211_IFTYPE_AP ||
> - key->sdata->vif.type == NL80211_IFTYPE_AP_VLAN))
> - addr = zero_addr;
> -
> if (key->sta)
> - addr = key->sta->sta.addr;
> + return &key->sta->sta;
>
> - return addr;
> + return NULL;
> }
>
> static void ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
> {
> - const u8 *addr;
> + struct ieee80211_sub_if_data *sdata;
> + struct ieee80211_sta *sta;
> int ret;
> DECLARE_MAC_BUF(mac);
>
> @@ -140,11 +128,16 @@ static void ieee80211_key_enable_hw_acce
> if (!key->local->ops->set_key)
> return;
>
> - addr = get_mac_for_key(key);
> + sta = get_sta_for_key(key);
> +
> + sdata = key->sdata;
> + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
> + sdata = container_of(sdata->bss,
> + struct ieee80211_sub_if_data,
> + u.ap);
>
> ret = key->local->ops->set_key(local_to_hw(key->local), SET_KEY,
> - key->sdata->dev->dev_addr, addr,
> - &key->conf);
> + &sdata->vif, sta, &key->conf);
>
> if (!ret) {
> spin_lock(&todo_lock);
> @@ -156,12 +149,14 @@ static void ieee80211_key_enable_hw_acce
> printk(KERN_ERR "mac80211-%s: failed to set key "
> "(%d, %s) to hardware (%d)\n",
> wiphy_name(key->local->hw.wiphy),
> - key->conf.keyidx, print_mac(mac, addr), ret);
> + key->conf.keyidx,
> + print_mac(mac, sta ? sta->addr : bcast_addr), ret);
> }
>
> static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
> {
> - const u8 *addr;
> + struct ieee80211_sub_if_data *sdata;
> + struct ieee80211_sta *sta;
> int ret;
> DECLARE_MAC_BUF(mac);
>
> @@ -178,17 +173,23 @@ static void ieee80211_key_disable_hw_acc
> }
> spin_unlock(&todo_lock);
>
> - addr = get_mac_for_key(key);
> + sta = get_sta_for_key(key);
> + sdata = key->sdata;
> +
> + if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
> + sdata = container_of(sdata->bss,
> + struct ieee80211_sub_if_data,
> + u.ap);
>
> ret = key->local->ops->set_key(local_to_hw(key->local), DISABLE_KEY,
> - key->sdata->dev->dev_addr, addr,
> - &key->conf);
> + &sdata->vif, sta, &key->conf);
>
> if (ret)
> printk(KERN_ERR "mac80211-%s: failed to remove key "
> "(%d, %s) from hardware (%d)\n",
> wiphy_name(key->local->hw.wiphy),
> - key->conf.keyidx, print_mac(mac, addr), ret);
> + key->conf.keyidx,
> + print_mac(mac, sta ? sta->addr : bcast_addr), ret);
>
> spin_lock(&todo_lock);
> key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
> --- everything.orig/drivers/net/wireless/ath5k/base.c 2008-12-08 17:39:05.000000000 +0100
> +++ everything/drivers/net/wireless/ath5k/base.c 2008-12-08 17:41:03.000000000 +0100
> @@ -232,7 +232,7 @@ static void ath5k_configure_filter(struc
> int mc_count, struct dev_mc_list *mclist);
> static int ath5k_set_key(struct ieee80211_hw *hw,
> enum set_key_cmd cmd,
> - const u8 *local_addr, const u8 *addr,
> + struct ieee80211_vif *vif, struct ieee80211_sta *sta,
> struct ieee80211_key_conf *key);
> static int ath5k_get_stats(struct ieee80211_hw *hw,
> struct ieee80211_low_level_stats *stats);
> @@ -2986,8 +2986,8 @@ static void ath5k_configure_filter(struc
>
> static int
> ath5k_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> - const u8 *local_addr, const u8 *addr,
> - struct ieee80211_key_conf *key)
> + struct ieee80211_vif *vif, struct ieee80211_sta *sta,
> + struct ieee80211_key_conf *key)
> {
> struct ath5k_softc *sc = hw->priv;
> int ret = 0;
> @@ -3010,7 +3010,8 @@ ath5k_set_key(struct ieee80211_hw *hw, e
>
> switch (cmd) {
> case SET_KEY:
> - ret = ath5k_hw_set_key(sc->ah, key->keyidx, key, addr);
> + ret = ath5k_hw_set_key(sc->ah, key->keyidx, key,
> + sta ? sta->addr : NULL);
> if (ret) {
> ATH5K_ERR(sc, "can't set the key\n");
> goto unlock;
> --- everything.orig/drivers/net/wireless/ath9k/main.c 2008-12-08 17:41:24.000000000 +0100
> +++ everything/drivers/net/wireless/ath9k/main.c 2008-12-08 18:10:07.000000000 +0100
> @@ -724,9 +724,10 @@ static int ath_key_config(struct ath_sof
> {
> struct ieee80211_vif *vif;
> struct ath9k_keyval hk;
> - const u8 *mac = NULL;
> int ret = 0;
> enum nl80211_iftype opmode;
> + static const u8 bcast_addr[ETH_ALEN] =
> + { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, };
>
> memset(&hk, 0, sizeof(hk));
>
> @@ -767,29 +768,26 @@ static int ath_key_config(struct ath_sof
> * BUT we will plumb a cleartext key so that we can do
> * per-Sta default key table lookup in software.
> */
> - if (is_broadcast_ether_addr(addr)) {
> + if (!addr) {
> switch (opmode) {
> case NL80211_IFTYPE_STATION:
> /* default key: could be group WPA key
> * or could be static WEP key */
> - mac = NULL;
> break;
> case NL80211_IFTYPE_ADHOC:
> - break;
> case NL80211_IFTYPE_AP:
> + addr = bcast_addr;
> break;
> default:
> ASSERT(0);
> break;
> }
> - } else {
> - mac = addr;
> }
>
> if (key->alg == ALG_TKIP)
> - ret = ath_setkey_tkip(sc, key, &hk, mac);
> + ret = ath_setkey_tkip(sc, key, &hk, addr);
> else
> - ret = ath_keyset(sc, key->keyidx, &hk, mac);
> + ret = ath_keyset(sc, key->keyidx, &hk, addr);
>
> if (!ret)
> return -EIO;
> @@ -2302,8 +2300,8 @@ static int ath9k_conf_tx(struct ieee8021
>
> static int ath9k_set_key(struct ieee80211_hw *hw,
> enum set_key_cmd cmd,
> - const u8 *local_addr,
> - const u8 *addr,
> + struct ieee80211_vif *vif,
> + struct ieee80211_sta *sta,
> struct ieee80211_key_conf *key)
> {
> struct ath_softc *sc = hw->priv;
> @@ -2313,7 +2311,7 @@ static int ath9k_set_key(struct ieee8021
>
> switch (cmd) {
> case SET_KEY:
> - ret = ath_key_config(sc, addr, key);
> + ret = ath_key_config(sc, sta ? sta->addr : NULL, key);
> if (!ret) {
> set_bit(key->keyidx, sc->sc_keymap);
> key->hw_key_idx = key->keyidx;
> --- everything.orig/drivers/net/wireless/b43/main.c 2008-12-08 17:42:05.000000000 +0100
> +++ everything/drivers/net/wireless/b43/main.c 2008-12-08 17:49:40.000000000 +0100
> @@ -3508,8 +3508,8 @@ static void b43_op_bss_info_changed(stru
> }
>
> static int b43_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> - const u8 *local_addr, const u8 *addr,
> - struct ieee80211_key_conf *key)
> + struct ieee80211_vif *vif, struct ieee80211_sta *sta,
> + struct ieee80211_key_conf *key)
> {
> struct b43_wl *wl = hw_to_b43_wl(hw);
> struct b43_wldev *dev;
> @@ -3567,17 +3567,13 @@ static int b43_op_set_key(struct ieee802
> goto out_unlock;
> }
>
> - if (is_broadcast_ether_addr(addr)) {
> + if (!sta) {
> /* addr is FF:FF:FF:FF:FF:FF for default keys */
> err = b43_key_write(dev, index, algorithm,
> key->key, key->keylen, NULL, key);
> } else {
> - /*
> - * either pairwise key or address is 00:00:00:00:00:00
> - * for transmit-only keys
> - */
> - err = b43_key_write(dev, -1, algorithm,
> - key->key, key->keylen, addr, key);
> + err = b43_key_write(dev, -1, algorithm, key->key,
> + key->keylen, sta->addr, key);
> }
> if (err)
> goto out_unlock;
> @@ -3604,10 +3600,12 @@ out_unlock:
> spin_unlock_irqrestore(&wl->irq_lock, flags);
> mutex_unlock(&wl->mutex);
> if (!err) {
> + static const u8 bcast_addr[ETH_ALEN] =
> + { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
> b43dbg(wl, "%s hardware based encryption for keyidx: %d, "
> "mac: %s\n",
> cmd == SET_KEY ? "Using" : "Disabling", key->keyidx,
> - print_mac(mac, addr));
> + print_mac(mac, sta ? sta->addr : bcast_addr));
> }
> return err;
> }
> --- everything.orig/drivers/net/wireless/iwlwifi/iwl-agn.c 2008-12-08 17:46:45.000000000 +0100
> +++ everything/drivers/net/wireless/iwlwifi/iwl-agn.c 2008-12-08 18:13:37.000000000 +0100
> @@ -3039,7 +3039,8 @@ static void iwl_mac_update_tkip_key(stru
> }
>
> static int iwl_mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> - const u8 *local_addr, const u8 *addr,
> + struct ieee80211_vif *vif,
> + struct ieee80211_sta *sta,
> struct ieee80211_key_conf *key)
> {
> struct iwl_priv *priv = hw->priv;
> @@ -3047,6 +3048,9 @@ static int iwl_mac_set_key(struct ieee80
> int ret = 0;
> u8 sta_id = IWL_INVALID_STATION;
> u8 is_default_wep_key = 0;
> + static const u8 bcast_addr[ETH_ALEN] =
> + { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, };
> + static const u8 *addr;
>
> IWL_DEBUG_MAC80211("enter\n");
>
> @@ -3055,9 +3059,7 @@ static int iwl_mac_set_key(struct ieee80
> return -EOPNOTSUPP;
> }
>
> - if (is_zero_ether_addr(addr))
> - /* only support pairwise keys */
> - return -EOPNOTSUPP;
> + addr = sta ? sta->addr : bcast_addr;
>
> sta_id = iwl_find_station(priv, addr);
> if (sta_id == IWL_INVALID_STATION) {
> --- everything.orig/drivers/net/wireless/iwlwifi/iwl3945-base.c 2008-12-08 17:46:16.000000000 +0100
> +++ everything/drivers/net/wireless/iwlwifi/iwl3945-base.c 2008-12-08 18:14:45.000000000 +0100
> @@ -7033,12 +7033,16 @@ out_unlock:
> }
>
> static int iwl3945_mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> - const u8 *local_addr, const u8 *addr,
> - struct ieee80211_key_conf *key)
> + struct ieee80211_vif *vif,
> + struct ieee80211_sta *sta,
> + struct ieee80211_key_conf *key)
> {
> struct iwl3945_priv *priv = hw->priv;
> int rc = 0;
> u8 sta_id;
> + static const u8 bcast_addr[ETH_ALEN] =
> + { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, };
> + const u8 *addr;
>
> IWL_DEBUG_MAC80211("enter\n");
>
> @@ -7047,9 +7051,7 @@ static int iwl3945_mac_set_key(struct ie
> return -EOPNOTSUPP;
> }
>
> - if (is_zero_ether_addr(addr))
> - /* only support pairwise keys */
> - return -EOPNOTSUPP;
> + addr = sta ? sta->addr : bcast_addr;
>
> sta_id = iwl3945_hw_find_station(priv, addr);
> if (sta_id == IWL_INVALID_STATION) {
> --- everything.orig/drivers/net/wireless/p54/p54common.c 2008-12-08 17:47:11.000000000 +0100
> +++ everything/drivers/net/wireless/p54/p54common.c 2008-12-08 17:53:09.000000000 +0100
> @@ -1938,7 +1938,7 @@ static void p54_bss_info_changed(struct
> }
>
> static int p54_set_key(struct ieee80211_hw *dev, enum set_key_cmd cmd,
> - const u8 *local_address, const u8 *address,
> + struct ieee80211_vif *vif, struct ieee80211_sta *sta,
> struct ieee80211_key_conf *key)
> {
> struct p54_common *priv = dev->priv;
> @@ -2002,8 +2002,8 @@ static int p54_set_key(struct ieee80211_
> rxkey->entry = key->keyidx;
> rxkey->key_id = key->keyidx;
> rxkey->key_type = algo;
> - if (address)
> - memcpy(rxkey->mac, address, ETH_ALEN);
> + if (sta)
> + memcpy(rxkey->mac, sta->addr, ETH_ALEN);
> else
> memset(rxkey->mac, ~0, ETH_ALEN);
> if (key->alg != ALG_TKIP) {
> --- everything.orig/drivers/net/wireless/rt2x00/rt2x00.h 2008-12-08 17:47:42.000000000 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt2x00.h 2008-12-08 17:47:52.000000000 +0100
> @@ -982,7 +982,7 @@ void rt2x00mac_configure_filter(struct i
> int mc_count, struct dev_addr_list *mc_list);
> #ifdef CONFIG_RT2X00_LIB_CRYPTO
> int rt2x00mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> - const u8 *local_address, const u8 *address,
> + struct ieee80211_vif *vif, struct ieee80211_sta *sta,
> struct ieee80211_key_conf *key);
> #else
> #define rt2x00mac_set_key NULL
> --- everything.orig/drivers/net/wireless/rt2x00/rt2x00mac.c 2008-12-08 17:48:03.000000000 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt2x00mac.c 2008-12-08 17:57:43.000000000 +0100
> @@ -483,15 +483,17 @@ EXPORT_SYMBOL_GPL(rt2x00mac_configure_fi
>
> #ifdef CONFIG_RT2X00_LIB_CRYPTO
> int rt2x00mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> - const u8 *local_address, const u8 *address,
> + struct ieee80211_vif *vif, struct ieee80211_sta *sta,
> struct ieee80211_key_conf *key)
> {
> struct rt2x00_dev *rt2x00dev = hw->priv;
> - struct ieee80211_sta *sta;
> + struct rt2x00_intf *intf = vif_to_intf(vif);
> int (*set_key) (struct rt2x00_dev *rt2x00dev,
> struct rt2x00lib_crypto *crypto,
> struct ieee80211_key_conf *key);
> struct rt2x00lib_crypto crypto;
> + static const u8 bcast_addr[ETH_ALEN] =
> + { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, };
>
> if (!test_bit(DEVICE_STATE_PRESENT, &rt2x00dev->flags))
> return 0;
> @@ -509,15 +511,14 @@ int rt2x00mac_set_key(struct ieee80211_h
> if (rt2x00dev->intf_sta_count)
> crypto.bssidx = 0;
> else
> - crypto.bssidx =
> - local_address[5] & (rt2x00dev->ops->max_ap_intf - 1);
> + crypto.bssidx = intf->mac[5] & (rt2x00dev->ops->max_ap_intf - 1);
>
> crypto.cipher = rt2x00crypto_key_to_cipher(key);
> if (crypto.cipher == CIPHER_NONE)
> return -EOPNOTSUPP;
>
> crypto.cmd = cmd;
> - crypto.address = address;
> + crypto.address = sta ? sta->addr : bcast_addr;
>
> if (crypto.cipher == CIPHER_TKIP) {
> if (key->keylen > NL80211_TKIP_DATA_OFFSET_ENCR_KEY)
> @@ -542,11 +543,8 @@ int rt2x00mac_set_key(struct ieee80211_h
> * Some drivers need this information when updating the
> * hardware key (either adding or removing).
> */
> - rcu_read_lock();
> - sta = ieee80211_find_sta(hw, address);
> if (sta)
> crypto.aid = sta->aid;
> - rcu_read_unlock();
>
> /*
> * Each BSS has a maximum of 4 shared keys.
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Intel Open Source Technology Centre
http://oss.intel.com/

2008-12-08 20:11:50

by Michael Büsch

[permalink] [raw]
Subject: Re: [RFT] mac80211: clean up set_key callback

On Monday 08 December 2008 18:23:55 Johannes Berg wrote:
> --- everything.orig/drivers/net/wireless/b43/main.c 2008-12-08 17:42:05.000000000 +0100
> +++ everything/drivers/net/wireless/b43/main.c 2008-12-08 17:49:40.000000000 +0100
> @@ -3508,8 +3508,8 @@ static void b43_op_bss_info_changed(stru
> }
>
> static int b43_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> - const u8 *local_addr, const u8 *addr,
> - struct ieee80211_key_conf *key)
> + struct ieee80211_vif *vif, struct ieee80211_sta *sta,
> + struct ieee80211_key_conf *key)
> {
> struct b43_wl *wl = hw_to_b43_wl(hw);
> struct b43_wldev *dev;
> @@ -3567,17 +3567,13 @@ static int b43_op_set_key(struct ieee802
> goto out_unlock;
> }
>
> - if (is_broadcast_ether_addr(addr)) {
> + if (!sta) {
> /* addr is FF:FF:FF:FF:FF:FF for default keys */

Please also remove this comment.

> err = b43_key_write(dev, index, algorithm,
> key->key, key->keylen, NULL, key);
> } else {
> - /*
> - * either pairwise key or address is 00:00:00:00:00:00
> - * for transmit-only keys
> - */
> - err = b43_key_write(dev, -1, algorithm,
> - key->key, key->keylen, addr, key);
> + err = b43_key_write(dev, -1, algorithm, key->key,
> + key->keylen, sta->addr, key);
> }
> if (err)
> goto out_unlock;
> @@ -3604,10 +3600,12 @@ out_unlock:
> spin_unlock_irqrestore(&wl->irq_lock, flags);
> mutex_unlock(&wl->mutex);
> if (!err) {
> + static const u8 bcast_addr[ETH_ALEN] =
> + { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff };
> b43dbg(wl, "%s hardware based encryption for keyidx: %d, "
> "mac: %s\n",
> cmd == SET_KEY ? "Using" : "Disabling", key->keyidx,
> - print_mac(mac, addr));
> + print_mac(mac, sta ? sta->addr : bcast_addr));

Will throw "unused variable bcast_addr" for !DEBUG.
Use something like
sta ? print_mac(mac, sta->addr) : "group/tx-only");

--
Greetings Michael.

2008-12-09 01:07:57

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFT] mac80211: clean up set_key callback

On Mon, 2008-12-08 at 20:01 -0500, Bob Copeland wrote:
> On Mon, Dec 08, 2008 at 06:23:55PM +0100, Johannes Berg wrote:
> > The set_key callback now seems rather odd, passing a MAC address
> > instead of a station struct, and a local address instead of a
> > vif struct. Change that.
> >
> > Signed-off-by: Johannes Berg <[email protected]>
>
> Works here... ath5k changes:
>
> Acked-by: Bob Copeland <[email protected]>

Thanks for testing.

> But ath5k probably wants this one-liner too (feel free to just
> roll it up into yours):

Right, I'll roll it up so you don't have to resend it.

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-12-08 22:54:03

by Ivo Van Doorn

[permalink] [raw]
Subject: Re: [RFT] mac80211: clean up set_key callback

On Monday 08 December 2008, Johannes Berg wrote:
> The set_key callback now seems rather odd, passing a MAC address
> instead of a station struct, and a local address instead of a
> vif struct. Change that.
>
> Signed-off-by: Johannes Berg <[email protected]>

ACK for the rt2x00 changes,
I'm especially happy with the ieee80211_find_sta() removal from rt2x00. :)

I'll see if I can test this in a few days,
One small change request though.

> --- everything.orig/drivers/net/wireless/rt2x00/rt2x00mac.c 2008-12-08 17:48:03.000000000 +0100
> +++ everything/drivers/net/wireless/rt2x00/rt2x00mac.c 2008-12-08 17:57:43.000000000 +0100
> @@ -483,15 +483,17 @@ EXPORT_SYMBOL_GPL(rt2x00mac_configure_fi
>
> #ifdef CONFIG_RT2X00_LIB_CRYPTO
> int rt2x00mac_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> - const u8 *local_address, const u8 *address,
> + struct ieee80211_vif *vif, struct ieee80211_sta *sta,
> struct ieee80211_key_conf *key)
> {
> struct rt2x00_dev *rt2x00dev = hw->priv;
> - struct ieee80211_sta *sta;
> + struct rt2x00_intf *intf = vif_to_intf(vif);
> int (*set_key) (struct rt2x00_dev *rt2x00dev,
> struct rt2x00lib_crypto *crypto,
> struct ieee80211_key_conf *key);
> struct rt2x00lib_crypto crypto;
> + static const u8 bcast_addr[ETH_ALEN] =
> + { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, };
>
> if (!test_bit(DEVICE_STATE_PRESENT, &rt2x00dev->flags))
> return 0;
> @@ -509,15 +511,14 @@ int rt2x00mac_set_key(struct ieee80211_h
> if (rt2x00dev->intf_sta_count)
> crypto.bssidx = 0;
> else
> - crypto.bssidx =
> - local_address[5] & (rt2x00dev->ops->max_ap_intf - 1);
> + crypto.bssidx = intf->mac[5] & (rt2x00dev->ops->max_ap_intf - 1);
>
> crypto.cipher = rt2x00crypto_key_to_cipher(key);
> if (crypto.cipher == CIPHER_NONE)
> return -EOPNOTSUPP;
>
> crypto.cmd = cmd;
> - crypto.address = address;
> + crypto.address = sta ? sta->addr : bcast_addr;
>
> if (crypto.cipher == CIPHER_TKIP) {
> if (key->keylen > NL80211_TKIP_DATA_OFFSET_ENCR_KEY)
> @@ -542,11 +543,8 @@ int rt2x00mac_set_key(struct ieee80211_h
> * Some drivers need this information when updating the
> * hardware key (either adding or removing).
> */
> - rcu_read_lock();
> - sta = ieee80211_find_sta(hw, address);
> if (sta)
> crypto.aid = sta->aid;
> - rcu_read_unlock();

Please move this bit up a bit to merge it with the address if-statement:

if (sta) {
crypto.address = sta->addr;
crypto.aid = sta->aid
} else
crypto.address = bcast_addr;

that would group the sta dependencies together so we only need to check
if sta is valid once.

Thanks!

Ivo

2008-12-09 12:36:32

by Christian Lamparter

[permalink] [raw]
Subject: Re: [RFT] mac80211: clean up set_key callback

On Monday 08 December 2008 18:23:55 Johannes Berg wrote:
> The set_key callback now seems rather odd, passing a MAC address
> instead of a station struct, and a local address instead of a
> vif struct. Change that.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
Acked-by: Christian Lamparter <[email protected]>

> --- everything.orig/drivers/net/wireless/p54/p54common.c 2008-12-08 17:47:11.000000000 +0100
> +++ everything/drivers/net/wireless/p54/p54common.c 2008-12-08 17:53:09.000000000 +0100
> @@ -1938,7 +1938,7 @@ static void p54_bss_info_changed(struct
> }
>
> static int p54_set_key(struct ieee80211_hw *dev, enum set_key_cmd cmd,
> - const u8 *local_address, const u8 *address,
> + struct ieee80211_vif *vif, struct ieee80211_sta *sta,
> struct ieee80211_key_conf *key)
> {
> struct p54_common *priv = dev->priv;
> @@ -2002,8 +2002,8 @@ static int p54_set_key(struct ieee80211_
> rxkey->entry = key->keyidx;
> rxkey->key_id = key->keyidx;
> rxkey->key_type = algo;
> - if (address)
> - memcpy(rxkey->mac, address, ETH_ALEN);
> + if (sta)
> + memcpy(rxkey->mac, sta->addr, ETH_ALEN);
> else
> memset(rxkey->mac, ~0, ETH_ALEN);
> if (key->alg != ALG_TKIP) {

2008-12-08 23:02:38

by Johannes Berg

[permalink] [raw]
Subject: Re: [RFT] mac80211: clean up set_key callback

On Mon, 2008-12-08 at 23:53 +0100, Ivo van Doorn wrote:
> On Monday 08 December 2008, Johannes Berg wrote:
> > The set_key callback now seems rather odd, passing a MAC address
> > instead of a station struct, and a local address instead of a
> > vif struct. Change that.
> >
> > Signed-off-by: Johannes Berg <[email protected]>
>
> ACK for the rt2x00 changes,
> I'm especially happy with the ieee80211_find_sta() removal from rt2x00. :)
>
> I'll see if I can test this in a few days,

Thanks

> One small change request though.

> Please move this bit up a bit to merge it with the address if-statement:
>
> if (sta) {
> crypto.address = sta->addr;
> crypto.aid = sta->aid
> } else
> crypto.address = bcast_addr;
>
> that would group the sta dependencies together so we only need to check
> if sta is valid once.

Sure, changed, see my patches folder, will resend after more
comments/testing.

johannes


Attachments:
signature.asc (836.00 B)
This is a digitally signed message part

2008-12-09 01:05:13

by Bob Copeland

[permalink] [raw]
Subject: Re: [RFT] mac80211: clean up set_key callback

On Mon, Dec 08, 2008 at 06:23:55PM +0100, Johannes Berg wrote:
> The set_key callback now seems rather odd, passing a MAC address
> instead of a station struct, and a local address instead of a
> vif struct. Change that.
>
> Signed-off-by: Johannes Berg <[email protected]>

Works here... ath5k changes:

Acked-by: Bob Copeland <[email protected]>

But ath5k probably wants this one-liner too (feel free to just
roll it up into yours):

From: Bob Copeland <[email protected]>
Date: Mon, 8 Dec 2008 18:16:59 -0500
Subject: [PATCH] ath5k: remove unlikely from null check in set_key_lladdr

Default keys use NULL for the mac too, so mac == NULL really
isn't unlikely.

Changes-licensed-under: ISC

Signed-off-by: Bob Copeland <[email protected]>
---
drivers/net/wireless/ath5k/pcu.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath5k/pcu.c b/drivers/net/wireless/ath5k/pcu.c
index dabe422..1ab0275 100644
--- a/drivers/net/wireless/ath5k/pcu.c
+++ b/drivers/net/wireless/ath5k/pcu.c
@@ -1139,7 +1139,7 @@ int ath5k_hw_set_key_lladdr(struct ath5k_hw *ah, u16 entry, const u8 *mac)

/* MAC may be NULL if it's a broadcast key. In this case no need to
* to compute AR5K_LOW_ID and AR5K_HIGH_ID as we already know it. */
- if (unlikely(mac == NULL)) {
+ if (mac == NULL) {
low_id = 0xffffffff;
high_id = 0xffff | AR5K_KEYTABLE_VALID;
} else {
--
1.6.0.4


--
Bob Copeland %% http://www.bobcopeland.com


2008-12-09 10:24:41

by Kalle Valo

[permalink] [raw]
Subject: Re: [RFT] mac80211: clean up set_key callback

"Johannes Berg" <[email protected]> writes:

> The set_key callback now seems rather odd, passing a MAC address
> instead of a station struct, and a local address instead of a
> vif struct. Change that.
>
> Signed-off-by: Johannes Berg <[email protected]>

Tested with iwl3945 on Lenove X60s. WEP works fine, I can test TKIP or
AES later if you want.

Tested-by: Kalle Valo <[email protected]>

--
Kalle Valo