Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Philipp Borgers <borgers@mi.fu-berlin.de>,
        Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org
References: <20210518110755.43077-1-borgers@mi.fu-berlin.de>
From:   Felix Fietkau <nbd@nbd.name>
Subject: Re: [PATCH 1/2] mac80211: do not use low data rates for data frames
 with no ack flag
Message-ID: <f80ba91c-88d4-3064-6861-5f9510b4fb69@nbd.name>
Date:   Tue, 18 May 2021 13:17:34 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <20210518110755.43077-1-borgers@mi.fu-berlin.de>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2021-05-18 13:07, Philipp Borgers wrote:
> Data Frames with no ack flag set should be handle by the rate controler.
> Make sure we reach the rate controler by returning early from
> rate_control_send_low if the frame is a data frame with no ack flag.
> 
> Signed-off-by: Philipp Borgers <borgers@mi.fu-berlin.de>
> ---
>  net/mac80211/rate.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
> index 63652c39c8e0..4f5b35a0ea28 100644
> --- a/net/mac80211/rate.c
> +++ b/net/mac80211/rate.c
> @@ -391,11 +391,16 @@ static bool rate_control_send_low(struct ieee80211_sta *pubsta,
>  				  struct ieee80211_tx_rate_control *txrc)
>  {
>  	struct ieee80211_tx_info *info = IEEE80211_SKB_CB(txrc->skb);
> +	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) txrc->skb->data;
>  	struct ieee80211_supported_band *sband = txrc->sband;
>  	struct sta_info *sta;
>  	int mcast_rate;
>  	bool use_basicrate = false;
>  
> +	if (ieee80211_is_data(hdr->frame_control) &&
> +			(info->flags & IEEE80211_TX_CTL_NO_ACK))
> +		return false;
Frames with IEEE80211_TX_CTL_HW_80211_ENCAP set have no 802.11 header,
so please change the check something like this:

	if ((info->flags & IEEE80211_TX_CTL_NO_ACK) &&
	    ((info->flags & IEEE80211_TX_CTL_HW_80211_ENCAP) ||
	     ieee80211_is_data(hdr->frame_control)))

Same applies to the other patch.
There are other places in the code where this kind of change needs to be
done, but it would be good if at least newly added code handles it properly.

- Felix