On Wed, 2018-09-26 at 15:55 +0530, Ajay Singh wrote:
>
> +#define NO_ENCRYPT 0
> +#define ENCRYPT_ENABLED BIT(0)
> +#define WEP BIT(1)
> +#define WEP_EXTENDED BIT(2)
> +#define WPA BIT(3)
> +#define WPA2 BIT(4)
> +#define AES BIT(5)
> +#define TKIP BIT(6)
> +
> +#define FRAME_TYPE_ID 0
> +#define ACTION_CAT_ID 24
> +#define ACTION_SUBTYPE_ID 25
> +#define P2P_PUB_ACTION_SUBTYPE 30
> +
> +#define ACTION_FRAME 0xd0
> +#define GO_INTENT_ATTR_ID 0x04
> +#define CHANLIST_ATTR_ID 0x0b
> +#define OPERCHAN_ATTR_ID 0x11
> +#define PUB_ACTION_ATTR_ID 0x04
> +#define P2PELEM_ATTR_ID 0xdd
> +
> +#define GO_NEG_REQ 0x00
> +#define GO_NEG_RSP 0x01
> +#define GO_NEG_CONF 0x02
> +#define P2P_INV_REQ 0x03
> +#define P2P_INV_RSP 0x04
> +#define PUBLIC_ACT_VENDORSPEC 0x09
> +#define GAS_INITIAL_REQ 0x0a
> +#define GAS_INITIAL_RSP 0x0b
> +
> +#define INVALID_CHANNEL 0
> +
> +#define nl80211_SCAN_RESULT_EXPIRE (3 * HZ)
???
I mentioned namespacing, but you can't steal a different one :-)
> +#define AGING_TIME (9 * 1000)
> +#define DURING_IP_TIME_OUT 15000
Not clear what the units are - should be using HZ?
> +static void clear_shadow_scan(struct wilc_priv *priv)
> +{
> + int i;
> +
> + for (i = 0; i < priv->scanned_cnt; i++) {
> + kfree(priv->scanned_shadow[i].ies);
> + priv->scanned_shadow[i].ies = NULL;
> +
> + kfree(priv->scanned_shadow[i].join_params);
> + priv->scanned_shadow[i].join_params = NULL;
> + }
> + priv->scanned_cnt = 0;
> +}
This seems unlikely to be a good idea - why keep things around in the
driver?
> +static u32 get_rssi_avg(struct network_info *network_info)
> +{
> + u8 i;
> + int rssi_v = 0;
> + u8 num_rssi = (network_info->rssi_history.full) ?
> + NUM_RSSI : (network_info->rssi_history.index);
> +
> + for (i = 0; i < num_rssi; i++)
> + rssi_v += network_info->rssi_history.samples[i];
> +
> + rssi_v /= num_rssi;
> + return rssi_v;
> +}
Why do you need a "real" average rather than EWMA which we have helpers
for?
> +static void refresh_scan(struct wilc_priv *priv, bool direct_scan)
> +{
> + struct wiphy *wiphy = priv->dev->ieee80211_ptr->wiphy;
> + int i;
> +
> + for (i = 0; i < priv->scanned_cnt; i++) {
> + struct network_info *network_info;
> + s32 freq;
> + struct ieee80211_channel *channel;
> + int rssi;
> + struct cfg80211_bss *bss;
> +
> + network_info = &priv->scanned_shadow[i];
> +
> + if (!memcmp("DIRECT-", network_info->ssid, 7) && !direct_scan)
> + continue;
Err, no? Don't do that? What's the point?
I don't know what you need the shadow stuff for, but you should remove
it anyway, and use the cfg80211 functionality instead. If not
sufficient, propose patches to improve it?
> + if (memcmp("DIRECT-", network_info->ssid, 7))
> + return;
same here
> +static int cancel_remain_on_channel(struct wiphy *wiphy,
> + struct wireless_dev *wdev,
> + u64 cookie)
> +{
> + struct wilc_priv *priv = wiphy_priv(wiphy);
> + struct wilc_vif *vif = netdev_priv(priv->dev);
> +
> + return wilc_listen_state_expired(vif,
> + priv->remain_on_ch_params.listen_session_id);
> +}
You really should be using the cookie.
> +static int mgmt_tx(struct wiphy *wiphy,
> + struct wireless_dev *wdev,
> + struct cfg80211_mgmt_tx_params *params,
> + u64 *cookie)
> +{
> + struct ieee80211_channel *chan = params->chan;
> + unsigned int wait = params->wait;
> + const u8 *buf = params->buf;
> + size_t len = params->len;
> + const struct ieee80211_mgmt *mgmt;
> + struct p2p_mgmt_data *mgmt_tx;
> + struct wilc_priv *priv = wiphy_priv(wiphy);
> + struct host_if_drv *wfi_drv = priv->hif_drv;
> + struct wilc_vif *vif = netdev_priv(wdev->netdev);
> + u32 buf_len = len + sizeof(p2p_vendor_spec) +
> sizeof(priv->p2p.local_random);
> + int ret = 0;
> +
> + *cookie = (unsigned long)buf;
Don't use pointers for the cookie, it leaks valuable data about KASLR.
> +static int del_virtual_intf(struct wiphy *wiphy, struct wireless_dev *wdev)
> +{
> + return 0;
> +}
Uh, not a good idea. Well, a good idea would be to actually support it,
but not to pretend to.
> +static struct wireless_dev *wilc_wfi_cfg_alloc(void)
> +{
> + struct wireless_dev *wdev;
> +
> + wdev = kzalloc(sizeof(*wdev), GFP_KERNEL);
> + if (!wdev)
> + goto out;
> +
> + wdev->wiphy = wiphy_new(&wilc_cfg80211_ops, sizeof(struct wilc_priv));
> + if (!wdev->wiphy)
> + goto free_mem;
> +
> + wilc_band_2ghz.ht_cap.ht_supported = 1;
> + wilc_band_2ghz.ht_cap.cap |= (1 << IEEE80211_HT_CAP_RX_STBC_SHIFT);
> + wilc_band_2ghz.ht_cap.mcs.rx_mask[0] = 0xff;
> + wilc_band_2ghz.ht_cap.ampdu_factor = IEEE80211_HT_MAX_AMPDU_8K;
> + wilc_band_2ghz.ht_cap.ampdu_density = IEEE80211_HT_MPDU_DENSITY_NONE;
This kind of static variable use is weird ... you're just initializing
to constant values?
If that's really the case then just put that into the initializer, if
not you need to kmemdup() to have this per device.
johannes
