Re: [PATCH 12/19] wilc: add wilc_wfi_cfgoperations.c
On 10/29/18 1:11 PM, Johannes Berg wrote: > Hi, > > Sorry for the late reply. No worries! Thanks for taking the time to review the driver. > > On Fri, 2018-10-19 at 21:47 +, adham.aboza...@microchip.com wrote: >> Johannes, shadow buffer has 2 more usage that I missed in my first email: >> 1- It keeps a copy of scan results to be able to auto-select from if >> the cfg80211 didn't supply a specific bssid in cfg's connect request >> (struct cfg80211_connect_params). >> In this case the driver will select a network that matches the ssid, >> while having the highest rssi. > You should be able to find this from cfg80211's BSS list as well. > > If we lack some API in this area, which is possible, then we can add it. Frankly, I wasn't aware that the cfg80211's BSS list is accessible from the driver! I'll do some research on that and let you know if these APIs didn't serve our purpose completely. >> 2- It keeps network parameters that the device will need to connect >> to a network, since the device doesn't keep the scan results >> internally. >> These parameters are stored in struct join_bss_param, and passed to >> the device when a connect request is received. >> Some of these parameters can be extracted from cfg's >> cfg80211_connect_params (like cap_info.. etc), but others (like bssid, >> beacon period.. etc) are still required. > Again though, you should be able to extract these from struct > cfg80211_bss, I'd argue? > > johannes >
Re: [PATCH 12/19] wilc: add wilc_wfi_cfgoperations.c
Hi, Sorry for the late reply. On Fri, 2018-10-19 at 21:47 +, adham.aboza...@microchip.com wrote: > > Johannes, shadow buffer has 2 more usage that I missed in my first email: > 1- It keeps a copy of scan results to be able to auto-select from if > the cfg80211 didn't supply a specific bssid in cfg's connect request > (struct cfg80211_connect_params). > In this case the driver will select a network that matches the ssid, > while having the highest rssi. You should be able to find this from cfg80211's BSS list as well. If we lack some API in this area, which is possible, then we can add it. > 2- It keeps network parameters that the device will need to connect > to a network, since the device doesn't keep the scan results > internally. > These parameters are stored in struct join_bss_param, and passed to > the device when a connect request is received. > Some of these parameters can be extracted from cfg's > cfg80211_connect_params (like cap_info.. etc), but others (like bssid, > beacon period.. etc) are still required. Again though, you should be able to extract these from struct cfg80211_bss, I'd argue? johannes
Re: [PATCH 12/19] wilc: add wilc_wfi_cfgoperations.c
On 10/09/2018 10:15 AM, Adham Abozaeid wrote: > > > On 10/09/2018 12:55 AM, Johannes Berg wrote: >> On Tue, 2018-10-09 at 04:23 +, adham.aboza...@microchip.com wrote: >>> 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? >>> >>> The point behind using a shadow buffer was to keep the scan results >>> consistent between consecutive scans, and smooth out the cases where >>> an AP isn't found momentarily during scanning. >>> In this implementation, APs found during scanning are added to the >>> shadow list, and removed if not found again for a period of time. >>> >>> I'm not much in favour of this implementation neither since it >>> complicates the driver's logic, but it was serving the purpose. >> >> You really should remove it - cfg80211 *and* wpa_s already do this if >> required. >> >> johannes >> > > Thanks Johannes for the tip. I did some research, and I believe you are > referring to using bss_expire_age and bss_expire_count. > We'll go ahead and remove that. Johannes, shadow buffer has 2 more usage that I missed in my first email: 1- It keeps a copy of scan results to be able to auto-select from if the cfg80211 didn't supply a specific bssid in cfg's connect request (struct cfg80211_connect_params). In this case the driver will select a network that matches the ssid, while having the highest rssi. 2- It keeps network parameters that the device will need to connect to a network, since the device doesn't keep the scan results internally. These parameters are stored in struct join_bss_param, and passed to the device when a connect request is received. Some of these parameters can be extracted from cfg's cfg80211_connect_params (like cap_info.. etc), but others (like bssid, beacon period.. etc) are still required. I think I can remove the functionality where it keeps older networks that didn't appear in the latest scan results, and let cfg take care of that, but will still need it to keep a copy of the latest scan results. Let me know what you think. Thanks, Adham
Re: [PATCH 12/19] wilc: add wilc_wfi_cfgoperations.c
On 10/09/2018 12:55 AM, Johannes Berg wrote: > On Tue, 2018-10-09 at 04:23 +, adham.aboza...@microchip.com wrote: >> >>> 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? >> >> The point behind using a shadow buffer was to keep the scan results >> consistent between consecutive scans, and smooth out the cases where >> an AP isn't found momentarily during scanning. >> In this implementation, APs found during scanning are added to the >> shadow list, and removed if not found again for a period of time. >> >> I'm not much in favour of this implementation neither since it >> complicates the driver's logic, but it was serving the purpose. > > You really should remove it - cfg80211 *and* wpa_s already do this if > required. > > johannes > Thanks Johannes for the tip. I did some research, and I believe you are referring to using bss_expire_age and bss_expire_count. We'll go ahead and remove that. Thanks, Adham
Re: [PATCH 12/19] wilc: add wilc_wfi_cfgoperations.c
On Tue, 2018-10-09 at 04:23 +, adham.aboza...@microchip.com wrote: > > > 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? > > The point behind using a shadow buffer was to keep the scan results > consistent between consecutive scans, and smooth out the cases where > an AP isn't found momentarily during scanning. > In this implementation, APs found during scanning are added to the > shadow list, and removed if not found again for a period of time. > > I'm not much in favour of this implementation neither since it > complicates the driver's logic, but it was serving the purpose. You really should remove it - cfg80211 *and* wpa_s already do this if required. johannes
Re: [PATCH 12/19] wilc: add wilc_wfi_cfgoperations.c
On 10/08/2018 07:57 AM, Johannes Berg wrote:
> On Wed, 2018-09-26 at 15:55 +0530, Ajay Singh wrote:
>>
>> +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 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?
The point behind using a shadow buffer was to keep the scan results consistent
between consecutive scans, and smooth out the cases where an AP isn't found
momentarily during scanning.
In this implementation, APs found during scanning are added to the shadow list,
and removed if not found again for a period of time.
I'm not much in favour of this implementation neither since it complicates the
driver's logic, but it was serving the purpose.
>
>> +if (memcmp("DIRECT-", network_info->ssid, 7))
>> +return;
>
> same here
>
>
> johannes
>
Thanks,
Adham
Re: [PATCH 12/19] wilc: add wilc_wfi_cfgoperations.c
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_ID0
> +#define ACTION_CAT_ID24
> +#define ACTION_SUBTYPE_ID25
> +#define P2P_PUB_ACTION_SUBTYPE 30
> +
> +#define ACTION_FRAME 0xd0
> +#define GO_INTENT_ATTR_ID0x04
> +#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_VENDORSPEC0x09
> +#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
