Received-SPF: pass (google.com: domain of linux-wireless+bounces-871-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sun, 17 Dec 2023 11:42:05 +0200
From: Jouni Malinen <j@w1.fi>
To: Miri Korenblit <miriam.rachel.korenblit@intel.com>
Cc: johannes@sipsolutions.net, linux-wireless@vger.kernel.org,
	Ilan Peer <ilan.peer@intel.com>,
	Gregory Greenman <gregory.greenman@intel.com>,
	Johannes Berg <johannes.berg@intel.com>
Subject: Re: [PATCH 06/14] wifi: cfg80211: Update the default DSCP-to-UP
 mapping
Message-ID: <ZX7CbSCEv7Zvv476@w1.fi>
References: <20231211070532.2458539-1-miriam.rachel.korenblit@intel.com>
 <20231211085121.8a1c7d1f0034.I50aed38be78ae9aea052938e2cb6b5800010ecd4@changeid>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231211085121.8a1c7d1f0034.I50aed38be78ae9aea052938e2cb6b5800010ecd4@changeid>

On Mon, Dec 11, 2023 at 09:05:24AM +0200, Miri Korenblit wrote:
> The default DSCP-to-UP mapping method defined in RFC8325
> applied to packets marked per recommendations in RFC4594 and
> destined to 802.11 WLAN clients will yield a number of inconsistent
> QoS mappings.
> 
> To handle this, modify the mapping of specific DSCP values for
> which the default mapping will create inconsistencies, based on
> the recommendations in section 4 in RFC8325.

Could this commit message give some more justification for why these
exact specifications are the best source of this information? For
example, should this also reference the Wi-Fi QoS Management
specification?

> diff --git a/net/wireless/util.c b/net/wireless/util.c
> @@ -980,7 +980,53 @@ unsigned int cfg80211_classify8021d(struct sk_buff *skb,
>  		}
>  	}
>  
> +	/* The default mapping as defined in RFC8325 */
>  	ret = dscp >> 5;

Could you point to the exact location in RFC 8325 that has that as the
default mapping? Figure 1 has this note: "Note: All unused codepoints
are RECOMMENDED to be mapped to UP 0".

Section 2.4 does describe what many APs do, but that section is called
"Default UP-to-DSCP Mappings and Conflicts" which does not sound like a
recommended default mapping in RFC 8325..

> +	/* Handle specific DSCP values for which the default mapping doesn't
> +	 * adhere to the intended usage of the DSCP value. See section 4 in
> +	 * RFC8325.
> +	 */
> +	switch (dscp >> 2) {

What about "Standard: DF" (0 --> 0) and "Low-Priority Data: CS1" (8 ->
1)? Are those omitted here because the "dscp >> 5" happens to have same
value? It would seem to be good to have at least a comment here pointing
that out in particular taken into account that comment above about the
status of that "default mapping" and how it relates to RFC 8325.

> +	case 10:
> +	case 12:
> +	case 14:
> +		/* High throughput data: AF11, AF12, AF13 */
> +		ret = 0;
> +		break;
> +	case 16:
> +		/* Operations, Administration, and Maintenance and Provisioning:
> +		 * CS2
> +		 */
> +		ret = 0;
> +		break;
> +	case 18:
> +	case 20:
> +	case 22:
> +		/* Low latency data: AF21, AF22, AF23 */
> +		ret = 3;
> +		break;
> +	case 24:
> +		/* Broadcasting video: CS23 */
> +		ret = 4;
> +		break;

Typo.. Should be "CS3" instead of "CS23".

What about "Multimedia Streaming: AF31, AF32, AF33"? Shouldn't those
values 26, 28, 30 be mapped to 4? If not, it would be good to add a
comment explaining why that is not here while it is included in RFC 8325
Figure 1.

What about "Real-Time Interactive: CS4"? Shouldn't that value 32 be
mapped to 4? If not, it would be good to add a comment explaining why
that is not here while it is included in RFC 8325 Figure 1.

What about "Multimedia Conferencing: AF41, AF42, AF43"? Shouldn't those
values 34, 36, 38 be mapped to 4? If not, it would be good to add a
comment explaining why that is not here while it is included in RFC 8325
Figure 1.

> +	case 40:
> +		/* Signaling: CS5 */
> +		ret = 5;
> +		break;
> +	case 44:
> +		/* Voice Admit */
> +		ret = 6;
> +		break;

To be consistent, that comment should be "VOICE-ADMIT: VA".

> +	case 46:
> +		/* Telephony traffic: EF */
> +		ret = 6;
> +		break;
> +	case 48:
> +		/* Network Control Traffic: CS6 */
> +		ret = 7;
> +		break;
> +	}

This does not include "Network Control: CS7". Is there a reason for not
covering that case 56 now? I know it is "reserved for future use", but
RFC 8325 does provide mapping for it as well.

-- 
Jouni Malinen                                            PGP id EFC895FA