On Wed, 2019-04-10 at 09:35 +0200, John Crispin wrote:
> The driver
> needs to enable the support on a per vif basis if it finds that all
> pre-reqs are meet.
> + * @IEEE80211_HW_SUPPORTS_80211_ENCAP: Hardware/driver supports 802.11
> + * encap for data frames.
I'm still not sure I can reconcile these ... why do you need the latter
if the driver has to enable per vif anyway?
> +int ieee80211_set_hw_80211_encap(struct ieee80211_vif *vif, bool enable);
Add documentation please. Would be better to return bool too, to be
symmetric.
> +++ b/net/mac80211/cfg.c
> @@ -2379,6 +2379,9 @@ static int ieee80211_set_wiphy_params(struct wiphy *wiphy, u32 changed)
> if (changed & WIPHY_PARAM_FRAG_THRESHOLD) {
> ieee80211_check_fast_xmit_all(local);
>
> + if (ieee80211_is_hw_80211_encap(local))
> + return -EINVAL;
Why not do this like TKIP and disable encap?
> +int ieee80211_set_hw_80211_encap(struct ieee80211_vif *vif, bool enable)
> +{
> + struct ieee80211_sub_if_data *sdata = vif_to_sdata(vif);
> + struct ieee80211_local *local = sdata->local;
> +
> + if (enable == sdata->hw_80211_encap)
> + return enable;
It feels like this is missing some locking? Or at least a lock
assertion, if it's always called with a certain lock held already.
> + if (!sdata->dev)
> + return 0;
> +
> + if (!ieee80211_hw_check(&local->hw, SUPPORTS_80211_ENCAP))
> + enable = 0;
> +
> + switch (vif->type) {
> + case NL80211_IFTYPE_STATION:
> + if (sdata->u.mgd.use_4addr)
> + enable = 0;
> + break;
> + case NL80211_IFTYPE_AP_VLAN:
> + if (sdata->wdev.use_4addr)
> + enable = 0;
> + break;
> + default:
> + break;
> + }
> +
> + if (!ieee80211_hw_check(&local->hw, SUPPORTS_TX_FRAG) &&
> + (local->hw.wiphy->frag_threshold != (u32)-1))
> + enable = 0;
Shouldn't there be some kind of TKIP check here?
> +bool ieee80211_is_hw_80211_encap(struct ieee80211_local *local)
> +{
> + struct ieee80211_sub_if_data *sdata;
> + bool offloaded = false;
> +
> + mutex_lock(&local->iflist_mtx);
> + list_for_each_entry(sdata, &local->interfaces, list) {
> + if (sdata->hw_80211_encap) {
> + offloaded = true;
> + break;
> + }
> + }
> + mutex_unlock(&local->iflist_mtx);
This might be better with RCU, though for proper usage you'd actually
have to call it with the mutex held, otherwise it can change while
you're iterating ...
> + /* TKIP countermeasures wont work on encap offload mode */
> + if (key->conf.cipher == WLAN_CIPHER_SUITE_TKIP)
> + ieee80211_set_hw_80211_encap(&sdata->vif, 0);
false.
> @@ -197,6 +201,9 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
> key->conf.keyidx,
> sta ? sta->sta.addr : bcast_addr, ret);
>
> + if (sdata->hw_80211_encap)
> + return -EINVAL;
Yeah, and this means you're also missing a "do we have a SW crypto key
right now" check above in ieee80211_set_hw_80211_encap().
> + if (ieee80211_hw_check(hw, SUPPORTS_80211_ENCAP)) {
> + /* mac80211 always supports monitor unless we do 802.11
> + * encapsulation offloading.
> + */
> + hw->wiphy->interface_modes |= BIT(NL80211_IFTYPE_MONITOR);
> + hw->wiphy->software_iftypes |= BIT(NL80211_IFTYPE_MONITOR);
> + }
Ok, maybe this addresses my question above about why we need both -
though it's not really clear what happens if the driver actually does
want to still enable monitor mode - which certainly we (iwlwifi) would.
> +void ieee80211_tx_status_8023(struct ieee80211_hw *hw,
> + struct ieee80211_vif *vif,
> + struct sk_buff *skb)
> +{
> + struct ieee80211_local *local = hw_to_local(hw);
> + struct ieee80211_sub_if_data *sdata;
> + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
> + struct sta_info *sta;
> + int retry_count;
> + int rates_idx;
> + bool acked;
> +
> + if (WARN_ON(!ieee80211_hw_check(hw, SUPPORTS_80211_ENCAP)))
> + goto skip_stats_update;
> +
> + sdata = vif_to_sdata(vif);
> +
> + acked = !!(info->flags & IEEE80211_TX_STAT_ACK);
no need for !! with bool :-)
> + /* check for driver support and ieee80211 encap offload */
> + if (!ieee80211_hw_check(&local->hw, SUPPORT_FAST_XMIT) ||
> + sdata->hw_80211_encap)
> return;
That comment isn't very useful? :-)
> @@ -3573,6 +3576,9 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80211_hw *hw,
> goto begin;
> }
>
> + if (info->control.flags & IEEE80211_TX_CTRL_HW_80211_ENCAP)
> + goto encap_out;
> +
> if (test_bit(IEEE80211_TXQ_AMPDU, &txqi->flags))
> info->flags |= IEEE80211_TX_CTL_AMPDU;
Wouldn't you still want this flag, perhaps?
Not with ath10k I guess, which offloads aggregation sessions, but still?
That's not necessarily a requirement for encap offload, is it?
> + ieee80211_tx_stats(dev, skb->len);
> +
> + if (sta) {
> + sta->tx_stats.bytes[skb_get_queue_mapping(skb)] += skb->len;
> + sta->tx_stats.packets[skb_get_queue_mapping(skb)]++;
> + }
This is something to think about, aren't we usually doing that after
encapsulation? So the counters will be off a bit now?
> + if (WARN_ON(unlikely(!sdata->hw_80211_encap))) {
I feel I'm repeating myself - WARN_ON() includes unlikely().
johannes
