Re: [PATCH V2 1/2] wl12xx: consider encryption when configuring auto arp template

[Luciano Coelho <coelho@ti.com>]

Hi Eliad,

On Thu, 2011-04-14 at 12:47 +0300, Eliad Peller wrote:
> When configuring the arp response template, and encryption is enabled,
> we should add some space and set the protected flag bit in the fc.
> 
> In order to track the encryption type, set wl->encryption_type when
> setting an encryption key, and reconfigure the arp response.
> Clear this field on wl1271_join, as keys have to be re-configured
> anyway after a join command.
> 
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---

A couple of style comments.


> diff --git a/drivers/net/wireless/wl12xx/cmd.c b/drivers/net/wireless/wl12xx/cmd.c
> index 2468044..65bcecb 100644
> --- a/drivers/net/wireless/wl12xx/cmd.c
> +++ b/drivers/net/wireless/wl12xx/cmd.c
> @@ -742,28 +742,30 @@ out:
>  
>  int wl1271_cmd_build_arp_rsp(struct wl1271 *wl, __be32 ip_addr)
>  {
> -	int ret;
> -	struct wl12xx_arp_rsp_template tmpl;
> +	int ret, extra = 0;
> +	u16 fc;
> +	struct sk_buff *skb;
> +	struct wl12xx_arp_rsp_template *tmpl;
>  	struct ieee80211_hdr_3addr *hdr;
>  	struct arphdr *arp_hdr;
>  
> -	memset(&tmpl, 0, sizeof(tmpl));
> +	skb = dev_alloc_skb(sizeof(*hdr) + sizeof(*tmpl) + 8);

[...]

> +	skb_reserve(skb, sizeof(*hdr) + 8);

What's 8? Maybe it would be better to make it clear with something like
WL1271_EXTRA_SPACE_MAX?


> @@ -771,12 +773,41 @@ int wl1271_cmd_build_arp_rsp(struct wl1271 *wl, __be32 ip_addr)
>  	arp_hdr->ar_op = cpu_to_be16(ARPOP_REPLY);
>  
>  	/* arp payload */
> -	memcpy(tmpl.sender_hw, wl->vif->addr, ETH_ALEN);
> -	tmpl.sender_ip = ip_addr;
> +	memcpy(tmpl->sender_hw, wl->vif->addr, ETH_ALEN);
> +	tmpl->sender_ip = ip_addr;
> +
> +	/* encryption space */
> +	switch (wl->encryption_type) {
> +	case KEY_TKIP:
> +		extra = WL1271_TKIP_IV_SPACE;
> +		break;
> +	case KEY_AES:
> +		extra = 8;
> +		break;
> +	}

It would be nicer to have WL1271_EXTRA_SPACE_AES here instead.  And
maybe rename the WL1271_TKIP_IV_SPACE to WL1271_EXTRA_SPACE_TKIP, to
make it consistent and clearer?

Maybe we should also complete the switch and explicitly set extra to
zero for KEY_NONE, KEY_WEP, KEY_GEM and default?


> @@ -2734,6 +2748,7 @@ static void wl1271_bss_info_changed_sta(struct wl1271 *wl,
>  
>  		if (bss_conf->arp_addr_cnt == 1 &&
>  		    bss_conf->arp_filter_enabled) {
> +			wl->ip_addr = addr;
>  			/*
>  			 * The template should have been configured only upon
>  			 * association. however, it seems that the correct ip
> @@ -2749,8 +2764,10 @@ static void wl1271_bss_info_changed_sta(struct wl1271 *wl,
>  			ret = wl1271_acx_arp_ip_filter(wl,
>  				ACX_ARP_FILTER_ARP_FILTERING,
>  				addr);
> -		} else
> +		} else {
> +			wl->ip_addr = 0;
>  			ret = wl1271_acx_arp_ip_filter(wl, 0, addr);
> +		}

This is not important at all, but maybe it's a bit cleaner: you could
remove the addr argument in wl1271_acx_arp_ip_filter() and always use
wl->ip_addr instead? You could then also get rid of the local addr here.


-- 
Cheers,
Luca.