Re: [RFT] mac80211: clean up set_key callback

[Ivo van Doorn <ivdoorn@gmail.com>]

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 <johannes@sipsolutions.net>

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