Re: [RFCv2 1/2] mac80211: add receive path for ethernet frame format

[Johannes Berg <johannes@sipsolutions.net>]

Hi,

On Thu, 2020-02-27 at 17:31 +0530, Manikanta Pubbisetty wrote:
> From: Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>
> 
> 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