Re: [PATCH 02/19] wilc: add coreconfigurator.c

[Johannes Berg <johannes@sipsolutions.net>]

> +static inline u16 get_beacon_period(u8 *data)
> +{
> +	u16 bcn_per;
> +
> +	bcn_per  = data[0];
> +	bcn_per |= (data[1] << 8);
> +
> +	return bcn_per;
> +}

Remove and use get_unaligned_le16().

> +static inline u32 get_beacon_timestamp_lo(u8 *data)
> +{
> +	u32 time_stamp = 0;
> +	u32 index    = MAC_HDR_LEN;
> +
> +	time_stamp |= data[index++];
> +	time_stamp |= (data[index++] << 8);
> +	time_stamp |= (data[index++] << 16);
> +	time_stamp |= (data[index]   << 24);
> +
> +	return time_stamp;
> +}

get_unaligned_le32()

> +static inline u32 get_beacon_timestamp_hi(u8 *data)
> +{
> +	u32 time_stamp = 0;
> +	u32 index    = (MAC_HDR_LEN + 4);
> +
> +	time_stamp |= data[index++];
> +	time_stamp |= (data[index++] << 8);
> +	time_stamp |= (data[index++] << 16);
> +	time_stamp |= (data[index]   << 24);
> +
> +	return time_stamp;
> +}

get_unaligned_le32()

> +static inline enum sub_frame_type get_sub_type(u8 *header)
> +{
> +	return ((enum sub_frame_type)(header[0] & 0xFC));
> +}

remove, use include/linux/ieee80211.h.

> +static inline u8 get_to_ds(u8 *header)
> +{
> +	return (header[1] & 0x01);
> +}
> +
> +static inline u8 get_from_ds(u8 *header)
> +{
> +	return ((header[1] & 0x02) >> 1);
> +}

dito

> +static inline void get_address1(u8 *msa, u8 *addr)
> +{
> +	memcpy(addr, msa + 4, 6);
> +}
> +
> +static inline void get_address2(u8 *msa, u8 *addr)
> +{
> +	memcpy(addr, msa + 10, 6);
> +}
> +
> +static inline void get_address3(u8 *msa, u8 *addr)

you probably shouldn't memcpy() here, that's expensive.

> +static inline void get_bssid(u8 *data, u8 *bssid)
> +{
> +	if (get_from_ds(data) == 1)
> +		get_address2(data, bssid);
> +	else if (get_to_ds(data) == 1)
> +		get_address1(data, bssid);
> +	else
> +		get_address3(data, bssid);
> +}

I just noticed we don't have this in ieee80211.h, but perhaps we should.

I think you should just return a pointer though, there's no point in
memcpy().

> +static inline void get_ssid(u8 *data, u8 *ssid, u8 *p_ssid_len)
> +{
> +	u8 i, j, len;
> +
> +	len = data[TAG_PARAM_OFFSET + 1];
> +	j   = TAG_PARAM_OFFSET + 2;
> +
> +	if (len >= MAX_SSID_LEN)
> +		len = 0;
> +
> +	for (i = 0; i < len; i++, j++)
> +		ssid[i] = data[j];
> +
> +	ssid[len] = '\0';

SSIDs are *NOT* strings, don't pretend they are.

> +	*p_ssid_len = len;
> +}

Uh, no, we have helper functions for finding elements.

Again, return something, don't just fill out parameters.

> +static inline u16 get_cap_info(u8 *data)
> +{
> +	u16 cap_info = 0;
> +	u16 index    = MAC_HDR_LEN;
> +	enum sub_frame_type st;
> +
> +	st = get_sub_type(data);
> +
> +	if (st == BEACON || st == PROBE_RSP)
> +		index += TIME_STAMP_LEN + BEACON_INTERVAL_LEN;
> +
> +	cap_info  = data[index];
> +	cap_info |= (data[index + 1] << 8);
> +
> +	return cap_info;
> +}

Umm, no, ieee80211.h.

> +static inline u16 get_asoc_status(u8 *data)
> +{
> +	u16 asoc_status;
> +
> +	asoc_status = data[3];
> +	return (asoc_status << 8) | data[2];
> +}

I'll just stop here - you need to replace almost all of this file with
ieee80211.h stuff instead.

johannes