Hi,
On Thu, 2020-02-27 at 17:31 +0530, Manikanta Pubbisetty wrote:
> From: Vasanthakumar Thiagarajan <vthiagar@xxxxxxxxxxxxxx>
>
> Implement rx path which does fewer processing on the received data
> frame which has already gone through 802.11 header decapsulation
> and other functionalities which require 802.11 header in the low
> level driver or hardware. Currently this rx path is restricted
> to AP and STA mode, but can be extended for Adhoc mode as well.
>
> It is upto to the low level driver to invoke the correct API and
> make sure if the frame that it passes is in ethernet format and
> the sta pointer is valid.
I guess generally this seems fine...
> +static const u8 pae_group_addr[ETH_ALEN] __aligned(2) = {0x01, 0x80, 0xC2, 0x00,
> + 0x00, 0x03};
The coding style here is a bit weird ...
> +static void
> +ieee80211_rx_handle_decap_offl(struct ieee80211_sub_if_data *sdata,
> + struct sta_info *sta, struct sk_buff *skb,
> + struct napi_struct *napi)
> +{
> + struct ieee80211_local *local = sdata->local;
> + struct ieee80211_vif *vif = &sdata->vif;
> + struct net_device *dev = sdata->dev;
> + struct ieee80211_rx_status *status;
> + struct ieee80211_key *key = NULL;
> + struct ieee80211_rx_data rx;
> + int i;
> + struct ethhdr *ehdr;
> + struct ieee80211_sta_rx_stats *stats = &sta->rx_stats;
> +
> + ehdr = (struct ethhdr *)skb->data;
You need to ensure that this is actually accessible in the skb head.
> + status = IEEE80211_SKB_RXCB(skb);
> +
> + if (ieee80211_hw_check(&local->hw, USES_RSS))
> + stats = this_cpu_ptr(sta->pcpu_rx_stats);
> +
> + /* TODO: Extend ieee80211_rx_decap_offl() with bssid so that Ethernet
> + * encap/decap can be supported in Adhoc interface type as well.
> + * Adhoc interface depends on bssid to update last_rx.
> + */
> + if (vif->type != NL80211_IFTYPE_STATION &&
> + vif->type != NL80211_IFTYPE_AP_VLAN &&
> + vif->type != NL80211_IFTYPE_AP)
> + goto drop;
Is there any value in this TODO? Probably should even WARN_ON() here.
> + stats->bytes += skb->len;
There's a bit of a mismatch here now between frames with 802.11 header
and frames with just ethernet - I don't know if we really need to
consider that, but it's there still?
> + /* Forcing seqno index to -1 so that tid specific stats are
> + * not updated in ieee80211_deliver_skb().
> + */
> + rx.seqno_idx = -1;
I guess that means you should also not advertise them to userspace ...
unless you're assuming that the driver would? But that seems far from
certain, so I guess if the driver intends to use ethernet RX then we
should remove a bunch of things in sta_set_tidstats()?
So I guess my biggest concern is about statistics - not that they need
to be there, but that we shouldn't show missing ones as (close to) zero,
but rather just not have them at all in this case.
johannes
