Re: [PATCH v2 5/7] wl12xx: add RX data filter ACX commands

[Luciano Coelho <coelho@ti.com>]

On Tue, 2012-01-31 at 16:44 +0200, Eliad Peller wrote: 
> From: Eyal Shapira <eyal@wizery.com>
> 
> (based on Pontus' patch)
> 
> Added commands for setting a specific filter
> and controlling the behaviour RX data filters
> implemented by the FW.
> 
> Signed-off-by: Pontus Fuchs <pontus.fuchs@gmail.com>
> Signed-off-by: Ido Reis <idor@ti.com>
> Signed-off-by: Eyal Shapira <eyal@wizery.com>
> Signed-off-by: Eliad Peller <eliad@wizery.com>
> ---

Why is Ido's sign-off here? (just curious)


> diff --git a/drivers/net/wireless/wl12xx/acx.c b/drivers/net/wireless/wl12xx/acx.c
> index bc96db0..668d337 100644
> --- a/drivers/net/wireless/wl12xx/acx.c
> +++ b/drivers/net/wireless/wl12xx/acx.c
> @@ -1740,3 +1740,108 @@ out:
>  	return ret;
>  
>  }
> +
> +int wl1271_acx_toggle_rx_data_filter(struct wl1271 *wl, bool enable,
> +				     u8 default_action)

Instead of "toggle" we usually have "enable" in the function name.  This
is not very important, obviously, but if we get a v3 of this patch, you
could change it for a little more consistency. ;)


> {
> +	struct acx_rx_data_filter_state *acx;
> +	int ret;
> +
> +	wl1271_debug(DEBUG_ACX, "acx toggle rx data filter en: %d act: %d",
> +		     enable, default_action);
> +
> +	acx = kzalloc(sizeof(*acx), GFP_KERNEL);
> +	if (!acx) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	acx->enable = enable ? 1 : 0;

Do we really need this? We usually trust that enable will be either true
(1) or false (0).


> +int wl1271_acx_set_rx_data_filter(struct wl1271 *wl, u8 index, bool enable,
> +				  struct wl12xx_rx_data_filter *filter)
> +{
> +	struct acx_rx_data_filter_cfg *acx;
> +	int fields_size = 0;
> +	int acx_size;
> +	int ret;
> +
> +	if (enable && !filter) {
> +		wl1271_warning("acx_set_rx_data_filter: enable but no filter");
> +		return -EINVAL;
> +	}
> +
> +	if (index >= WL1271_MAX_RX_DATA_FILTERS) {
> +		wl1271_warning("acx_set_rx_data_filter: invalid filter idx(%d)",
> +			       index);
> +		return -EINVAL;
> +	}

Should we use BUG_ON instead? This is only used internally in the
driver, so if it get here, it's a bug.  And if the filters come from
userspace, we should validate them before continuing anyway.


> +	if (filter) {
> +		if (filter->action < FILTER_DROP ||
> +		    filter->action > FILTER_FW_HANDLE) {
> +			wl1271_warning("invalid filter action (%d)",
> +				       filter->action);
> +			return -EINVAL;
> +		}
> +
> +		if (filter->num_fields != 1 &&
> +		    filter->num_fields != 2) {
> +			wl1271_warning("invalid filter num_fields (%d)",
> +				       filter->num_fields);
> +			return -EINVAL;
> +		}

Same for these two.



> +	acx_size = roundup(sizeof(*acx) + fields_size, 4);

ALIGN()?


> +	acx = kzalloc(acx_size, GFP_KERNEL);
> +
> +	if (!acx)
> +		return -ENOMEM;
> +
> +	acx->enable = enable ? 1 : 0;

Same as above.


> diff --git a/drivers/net/wireless/wl12xx/wl12xx.h b/drivers/net/wireless/wl12xx/wl12xx.h
> index 1463341..c18ad0a 100644
> --- a/drivers/net/wireless/wl12xx/wl12xx.h
> +++ b/drivers/net/wireless/wl12xx/wl12xx.h
> @@ -277,6 +277,39 @@ struct wl1271_link {
>  	u8 ba_bitmap;
>  };
>  
> +#define WL1271_MAX_RX_DATA_FILTERS 4
> +#define WL1271_RX_DATA_FILTER_MAX_FIELD_PATTERNS 8

This is too long for a macro?


> +/* FW MAX FILTER SIZE is 98 bytes. The MAX_PATTERN_SIZE is imposed
> + * after taking into account the mask bytes and other structs members
> + */

This is slightly off according to the coding-style. ;)

Also s/structs/struct/


> +#define WL1271_RX_DATA_FILTER_MAX_PATTERN_SIZE 43
> +#define WL1271_RX_DATA_FILTER_ETH_HEADER_SIZE 14
> +
> +#define WL1271_RX_DATA_FILTER_FLAG_MASK                BIT(0)
> +#define WL1271_RX_DATA_FILTER_FLAG_IP_HEADER           0
> +#define WL1271_RX_DATA_FILTER_FLAG_ETHERNET_HEADER     BIT(1)

It would also be nice to find some smaller names for these.


> +struct wl12xx_rx_data_filter_field {
> +	__le16 offset;
> +	u8 len;
> +	u8 flags;
> +	u8 pattern[0];
> +} __packed;
> +
> +struct wl12xx_rx_data_filter {
> +	u8 action;
> +	int num_fields;
> +	int fields_size;
> +	struct wl12xx_rx_data_filter_field fields[0];
> +} __packed;

No need for __packed here.  These are internal structs and not part of
the FW API.


-- 
Cheers,
Luca.