[PATCH 01/16] wcn36xx: Add main.c

[Eugene Krasnikov]

> Just a cursory review ...
>

Any review is very welcome;)

>
>> +             .cap = IEEE80211_HT_CAP_GRN_FLD
>> +                     | IEEE80211_HT_CAP_SGI_20
>
> wouldn't that usually be written as
>
> GRN_FLD |
> SGI_20 |
> ...
>
> (multiple similar places)

Will be fixed in the next round.


>> +#ifdef CONFIG_PM
>> +
>> +static const struct wiphy_wowlan_support wowlan_support = {
>> +     .flags = WIPHY_WOWLAN_ANY,
>> +     .n_patterns = 0,
>
> that n_patterns is pretty useless.
>

Will be removed in the next patch set.

>> +#define WCN36XX_SUPPORTED_FILTERS (0)
>> +
>> +static void wcn36xx_configure_filter(struct ieee80211_hw *hw,
>> +                                    unsigned int changed,
>> +                                    unsigned int *total, u64 multicast)
>> +{
>> +     wcn36xx_dbg(WCN36XX_DBG_MAC, "mac configure filter");
>> +
>> +     changed &= WCN36XX_SUPPORTED_FILTERS;
>
> That's pointless

Yes, it is better to remove wcn36xx_configure_filter completely for now.

>> +             if (IEEE80211_KEY_FLAG_PAIRWISE & key_conf->flags) {
>> +                     sta_priv->is_data_encrypted = true;
>> +                     /* Reconfigure bss with encrypt_type */
>> +                     if (NL80211_IFTYPE_STATION == vif->type)
>> +                             wcn36xx_smd_config_bss(wcn,
>> +                                                    vif,
>> +                                                    sta,
>> +                                                    sta->addr,
>> +                                                    true);
>
> It seems to me this should not be here but you should have mac80211 set
> something in e.g. bss_conf that indicates encryption?
>

It's a good idea and I tried to find anything encryption related in
bss_conf but without luck. I do not like this line myself so I would
really appreciate if you can point where exactly in
bss_conf/bss_info_changed information about encryption is located.

>> +     /* Not supported so far*/
>> +     case IEEE80211_AMPDU_TX_STOP_CONT:
>> +             ieee80211_stop_tx_ba_cb_irqsafe(vif, sta->addr, tid);
>> +             break;
>> +     case IEEE80211_AMPDU_TX_STOP_FLUSH:
>> +     case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
>> +             break;
>
> You can't just "not support" them - you have to at least stop the
> aggregation session, see the commit that introduced this.
>

Good point. Will fix this in the next round.

>> +     static const u32 cipher_suites[] = {
>> +             WLAN_CIPHER_SUITE_TKIP,
>> +             WLAN_CIPHER_SUITE_CCMP,
>> +     };
>
> You actually don't want to support WEP, not even in software? Otherwise
> just leave this out and mac80211 will add it.

WEP is supported by HW but wcn36xx does not configure it yet. Is that
ok to add HW WEP encryption in nearest future after wcn36xx is pushed
to upstream?

>> +     wcn->hw->wiphy->iface_combinations = &if_comb;
>> +     wcn->hw->wiphy->n_iface_combinations = 1;
>
> Your code with "wcn->current_vif = " etc. *really* doesn't look like you
> support combinations. Are you positive this is OK?

So far wcn36xx supports only one interface at once. But in the nearest
future it will definitely support more than one. So how about keeping
this for future?;)

>> +     wcn->hw->wiphy->max_scan_ssids = 1;
>
> Really? You don't even have hardware scan, so why?
>

Good catch. wcn36xx used to have hw scan but now it is moved to sw
scan. Will remove this completely.

> johannes
>



-- 
Best regards,
Eugene