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
