Re: [PATCH 03/19] wilc: add host_interface.h

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

On 10/8/2018 7:50 PM, Johannes Berg wrote:
> On Wed, 2018-09-26 at 15:55 +0530, Ajay Singh wrote:
>
>> +#include <linux/ieee80211.h>
> you include it
>
>> +#include "coreconfigurator.h"
>> +
>> +#define IDLE_MODE	0x00
>> +#define AP_MODE		0x01
>> +#define STATION_MODE	0x02
>> +#define GO_MODE		0x03
>> +#define CLIENT_MODE	0x04
>> +#define ACTION		0xD0
>> +#define PROBE_REQ	0x40
>> +#define PROBE_RESP	0x50
> please use it too.
>
Ack.

>> +#define ACTION_FRM_IDX				0
>> +#define PROBE_REQ_IDX				1
>> +#define MAX_NUM_STA				9
>> +#define ACTIVE_SCAN_TIME			10
>> +#define PASSIVE_SCAN_TIME			1200
>> +#define MIN_SCAN_TIME				10
>> +#define MAX_SCAN_TIME				1200
>> +#define DEFAULT_SCAN				0
>> +#define USER_SCAN				BIT(0)
>> +#define OBSS_PERIODIC_SCAN			BIT(1)
>> +#define OBSS_ONETIME_SCAN			BIT(2)
>> +#define GTK_RX_KEY_BUFF_LEN			24
>> +#define ADDKEY					0x1
>> +#define REMOVEKEY				0x2
>> +#define DEFAULTKEY				0x4
>> +#define ADDKEY_AP				0x8
>> +#define MAX_NUM_SCANNED_NETWORKS		100
>> +#define MAX_NUM_SCANNED_NETWORKS_SHADOW		130
>> +#define MAX_NUM_PROBED_SSID			10
>> +#define CHANNEL_SCAN_TIME			250
>> +
>> +#define TX_MIC_KEY_LEN				8
>> +#define RX_MIC_KEY_LEN				8
>> +#define PTK_KEY_LEN				16
>> +
>> +#define TX_MIC_KEY_MSG_LEN			26
>> +#define RX_MIC_KEY_MSG_LEN			48
>> +#define PTK_KEY_MSG_LEN				39
>> +
>> +#define PMKSA_KEY_LEN				22
>> +#define ETH_ALEN				6
> umm?
>
>> +#define PMKID_LEN				16
> ??
>
>> +#define WILC_MAX_NUM_PMKIDS			16
>> +#define WILC_ADD_STA_LENGTH			40
>> +#define NUM_CONCURRENT_IFC			2
>> +#define DRV_HANDLER_SIZE			5
>> +#define DRV_HANDLER_MASK			0x000000FF
> Also this file is strangely mixing
>  * 802.11 constants (that you shouldn't have anyway)
>  * driver constants/structs
>  * hardware/firmware-related things (at least it seems like - e.g. the
>    "REMOVEKEY" constant)
>
> Please clean that up, separate the things, and pick a better
> namespace... just having "REMOVEKEY" is probably not a good idea.
>
Will work on this to clean it up.
>> +typedef void (*wilc_remain_on_chan_expired)(void *, u32);
>> +typedef void (*wilc_remain_on_chan_ready)(void *);
> Please no typedefs.
>
I think as per coding style the typedef for function pointer are allowed.

>> +struct rcvd_net_info {
>> +	u8 *buffer;
>> +	u32 len;
>> +};
>> +
>> +struct hidden_net_info {
>> +	u8  *ssid;
>> +	u8 ssid_len;
>> +};
>> +
>> +struct hidden_network {
>> +	struct hidden_net_info *net_info;
>> +	u8 n_ssids;
>> +};
> This seems really odd - what part doesn't cfg80211 already handle?

If I understood your question correctly,  you meant what extra
functionality 'hidden_network' struct is providing.
Actually this structure is just used to keeps list of SSID's requested
in cfg80211 'scan' callback which is passed to firmware. The values are
extracted from 'cfg80211_scan_request[struct cfg80211_ssid *ssids 
-----  int n_ssids] received during scan.

Regards,
Ajay