On Thursday 15 December 2016 02:59 PM, Johannes Berg wrote:
>
>> There is a field, no_80211_encap, added to ieee80211_tx_info:control
>> to mark if the 802.11 encapsulation is offloaded to driver.
>> There is also a new callback for tx completion status indication
>> to handle data frames using 802.11 encap offload.
>
> I'm not sure I see the need for this? Maybe I'll find out in this patch
> :)
>
I think we may not need this if we make the design in such a way that all
the interfaces will use uniform encap/decap mode for the data packet.
>> + /* XXX: This frame is not encaptulated with
>> 802.11
>> + * header. Should this be added to
>> %IEEE80211_TX_CTRL_*
>> + * flags?.
>> + */
>> + bool no_80211_encap;
>> + /* 3 bytes free */
>> } control;
>
> probably - just to preserve more space.
Correct.
>
>> + * @IEEE80211_CONF_CHANGE_80211_HDR_OFFL: Offload configuration
>> + * implementing 802.11 encap/decap for data frames changed.
>> */
>> enum ieee80211_conf_changed {
>> IEEE80211_CONF_CHANGE_SMPS = BIT(1),
>> @@ -1279,6 +1286,7 @@ enum ieee80211_conf_changed {
>> IEEE80211_CONF_CHANGE_CHANNEL = BIT(6),
>> IEEE80211_CONF_CHANGE_RETRY_LIMITS = BIT(7),
>> IEEE80211_CONF_CHANGE_IDLE = BIT(8),
>> + IEEE80211_CONF_CHANGE_80211_HDR_OFFL = BIT(9),
>> };
>
> Given the requirements (PN check, etc.) I'm not sure how this can
> change dynamically?
I agree. Dynamic switch part is buggy, we can start with not allowing
interfaces resulting in dynamic switch.
>
>> + * @encap_decap_80211_offloaded: Whether 802.11 header encap/decap
>> offload
>> + * is enabled
>> */
>> struct ieee80211_conf {
>> u32 flags;
>> @@ -1346,6 +1357,7 @@ struct ieee80211_conf {
>> struct cfg80211_chan_def chandef;
>> bool radar_enabled;
>> enum ieee80211_smps_mode smps_mode;
>> + bool encap_decap_80211_offloaded;
>
> Please don't add anything here that's interface specific.
Ok, this is mainly hw configuration of encap/decap mode, not
vif specific per say. Any pointers where this should belong to?
>
>> --- a/net/mac80211/cfg.c
>> +++ b/net/mac80211/cfg.c
>> @@ -107,6 +107,10 @@ static int ieee80211_change_iface(struct wiphy
>> *wiphy,
>> }
>> }
>>
>> + ieee80211_if_check_80211_hdr_offl(sdata,
>> + params ? params->use_4addr
>> : false,
>> + true);
>> +
>> return 0;
>> }
>
> Wouldn't it be better to simply prohibit changing this while the
> interface is up, and re-init it later when it goes up?
I agree.
>
>> +++ b/net/mac80211/ieee80211_i.h
>> @@ -1373,6 +1373,8 @@ struct ieee80211_local {
>> /* TDLS channel switch */
>> struct work_struct tdls_chsw_work;
>> struct sk_buff_head skb_queue_tdls_chsw;
>> +
>> + bool data_80211_hdr_offloaded;
>
> Again, don't put interface specific things into device structures.
Ok, this is also current hw configuration of encap/decap mode, not vif
specific.
>
>> +int ieee80211_lookup_ra_sta(struct ieee80211_sub_if_data *sdata,
>> + struct sk_buff *skb,
>> + struct sta_info **sta_out);
>
> Return the sta_info pointer, and ERR_PTR() if needed.
Correct, sta_out is checked for ERR_PTR() as well before using it.
>
>> +++ b/net/mac80211/iface.c
>> @@ -698,6 +698,11 @@ int ieee80211_do_open(struct wireless_dev *wdev,
>> bool coming_up)
>> rcu_assign_pointer(local->p2p_sdata, sdata);
>> }
>>
>> + if (local->open_count == 0 && local-
>>> data_80211_hdr_offloaded) {
>> + local->hw.conf.encap_decap_80211_offloaded = true;
>> + hw_reconf_flags |=
>> IEEE80211_CONF_CHANGE_80211_HDR_OFFL;
>> + }
>
> I don't see how this helps anything, I think you should remove it.
Yeah, I think this was added for dynamic switch.
>
>> +void ieee80211_if_config_80211_hdr_offl(struct ieee80211_local
>> *local,
>> + bool enable_80211_hdr_offl)
>> +{
>> + struct ieee80211_sub_if_data *sdata;
>> + unsigned long flags;
>> + int n_acs = IEEE80211_NUM_ACS;
>> + int ac;
>> +
>> + ASSERT_RTNL();
>> +
>> + if (!ieee80211_hw_check(&local->hw,
>> SUPPORTS_80211_ENCAP_DECAP) ||
>> + !(ieee80211_hw_check(&local->hw, HAS_RATE_CONTROL)))
>> + return;
>> +
>> + if (local->hw.wiphy->frag_threshold != (u32)-1 &&
>> + !local->ops->set_frag_threshold)
>> + return;
>> +
>> + mutex_lock(&local->iflist_mtx);
>> +
>> + list_for_each_entry(sdata, &local->interfaces, list) {
>> + if (!sdata->dev)
>> + continue;
>> +
>> + netif_tx_stop_all_queues(sdata->dev);
>> +
>> + if (enable_80211_hdr_offl)
>> + sdata->dev->netdev_ops =
>> &ieee80211_dataif_8023_ops;
>> + else
>> + sdata->dev->netdev_ops =
>> &ieee80211_dataif_ops;
>> + }
>> +
>> + mutex_unlock(&local->iflist_mtx);
>> +
>> + local->data_80211_hdr_offloaded = enable_80211_hdr_offl;
>> +
>> + if (local->started) {
>> + if (enable_80211_hdr_offl)
>> + local->hw.conf.encap_decap_80211_offloaded =
>> true;
>> + else
>> + local->hw.conf.encap_decap_80211_offloaded =
>> false;
>> + ieee80211_hw_config(local,
>> + IEEE80211_CONF_CHANGE_80211_HDR_
>> OFFL);
>> + }
>> +
>> + mutex_lock(&local->iflist_mtx);
>> +
>> + list_for_each_entry(sdata, &local->interfaces, list) {
>> + if (!sdata->dev)
>> + continue;
>> +
>> + if (local->hw.queues < IEEE80211_NUM_ACS)
>> + n_acs = 1;
>> +
>> + spin_lock_irqsave(&local->queue_stop_reason_lock,
>> flags);
>> + if (sdata->vif.cab_queue == IEEE80211_INVAL_HW_QUEUE
>> ||
>> + (local->queue_stop_reasons[sdata->vif.cab_queue]
>> == 0 &&
>> + skb_queue_empty(&local->pending[sdata-
>>> vif.cab_queue]))) {
>> + for (ac = 0; ac < n_acs; ac++) {
>> + int ac_queue = sdata-
>>> vif.hw_queue[ac];
>> +
>> + if (local-
>>> queue_stop_reasons[ac_queue] == 0 &&
>> + skb_queue_empty(&local-
>>> pending[ac_queue]))
>> + netif_start_subqueue(sdata-
>>> dev, ac);
>> + }
>> + }
>> + spin_unlock_irqrestore(&local-
>>> queue_stop_reason_lock, flags);
>> + }
>> +
>> + mutex_unlock(&local->iflist_mtx);
>> +}
>
> I really would prefer we could simply avoid doing these manipulations
> while the interface is UP and can have data queued.
Agreed.
>
>> +++ b/net/mac80211/key.c
>> @@ -208,13 +208,25 @@ static int ieee80211_key_enable_hw_accel(struct
>> ieee80211_key *key)
>> case WLAN_CIPHER_SUITE_GCMP_256:
>> /* all of these we can do in software - if driver
>> can */
>> if (ret == 1)
>> - return 0;
>> + goto check_8023_txrx;
>> if (ieee80211_hw_check(&key->local->hw,
>> SW_CRYPTO_CONTROL))
>> return -EINVAL;
>> - return 0;
>> + goto check_8023_txrx;
>> default:
>> return -EINVAL;
>> }
>> +
>> +check_8023_txrx:
>> + /* When sw crypto is enabled make sure data tx/rx happens
>> + * in 802.11 format.
>> + */
>> + if (key->local->data_80211_hdr_offloaded) {
>> + rtnl_lock();
>> + ieee80211_if_config_80211_hdr_offl(key->local,
>> false);
>> + rtnl_unlock();
>> + }
>> +
>> + return 0;
>> }
>
> Why not just refuse the key instead? It also seems wrong to do anything
> with local-> here, it should be per interface.
Correct. Here also encap/decap type switch happens, this can be avoided just
refusing the key.
>
>> +++ b/net/mac80211/status.c
>> @@ -506,12 +506,14 @@ static void ieee80211_report_used_skb(struct
>> ieee80211_local *local,
>> struct sk_buff *skb, bool
>> dropped)
>> {
>> struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
>> - struct ieee80211_hdr *hdr = (void *)skb->data;
>> + struct ieee80211_hdr *hdr;
>> bool acked = info->flags & IEEE80211_TX_STAT_ACK;
>>
>> if (dropped)
>> acked = false;
>>
>> + hdr = (void *)skb->data;
>
> That change make no sense.
Yes, I forgot to remove this.
>
>> if (info->flags & IEEE80211_TX_INTFL_MLME_CONN_TX) {
>> struct ieee80211_sub_if_data *sdata;
>>
>> @@ -945,6 +947,85 @@ void ieee80211_tx_status(struct ieee80211_hw
>> *hw, struct sk_buff *skb)
>> }
>> EXPORT_SYMBOL(ieee80211_tx_status);
>>
>> +void ieee80211_tx_status_8023(struct ieee80211_hw *hw,
>> + struct ieee80211_vif *vif,
>> + struct sk_buff *skb)
>
> I think this could share some code with the 802.11 version?
>
>> + /* XXX: Add a generic helper for this */
>> + if (sdata->vif.type == NL80211_IFTYPE_AP ||
>> + sdata->vif.type == NL80211_IFTYPE_AP_VLAN ||
>> + sdata->vif.type == NL80211_IFTYPE_ADHOC)
>> + ether_addr_copy(ra_addr, ehdr->h_dest);
>
> nit, but the "A" in "RA" already means address ... :)
Sure, thanks.
>
> You also don't need to copy it - just keeping a pointer should be fine?
Right.
>
>> + /* TODO: Handle frames requiring wifi tx status to be
>> notified */
>> + if (skb->sk && skb_shinfo(skb)->tx_flags &
>> SKBTX_WIFI_STATUS)
>> + goto out_free;
>
> Surely you shouldn't free them, even if you don't handle the status?!
Correct, I think we can still pass the frame to go through even if tx status
is not handled.
Vasanth
