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

[Ajay Singh <ajay.kathat@microchip.com>]

Hi,

Firstly, thanks alot for taking out time and reviewing our driver.
I will work on review comment and submit the updated code changes.

On 10/8/2018 7:46 PM, Johannes Berg wrote:
>> +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().
>
Sure, I will change these to make use of get_unalinged_le16() API.
>> +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()
>
Ack.
>> +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()
>
Ack
>> +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.
>
Did you mean to use '& IEEE80211_FCTL_STYPE' to get the frame sub type,
instead of adding this API.
>> +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
>
Ack.  
>> +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.
>
Got your point but currently  a copy of 'network_info' is maintained for
the shadow buffer. As we have to work on removing the use of shadow
buffer so after those changes this extra memcpy can be avoided.

>> +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().
>
Same as above.
>> +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.
>
Will check and modify to make use of ssid_len instead of using '\0' as
terminator.
>> +	*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.
>
Ack.
>> +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.

As suggested, will modify this code to make use of ieee80211.h header.

Regards,
Ajay