Hi,
> -----Original Message-----
> From: Jouni Malinen <j@xxxxx>
> Sent: Sunday, December 17, 2023 11:42
> To: Korenblit, Miriam Rachel <miriam.rachel.korenblit@xxxxxxxxx>
> Cc: johannes@xxxxxxxxxxxxxxxx; linux-wireless@xxxxxxxxxxxxxxx; Peer, Ilan
> <ilan.peer@xxxxxxxxx>; Greenman, Gregory <gregory.greenman@xxxxxxxxx>;
> Berg, Johannes <johannes.berg@xxxxxxxxx>
> Subject: Re: [PATCH 06/14] wifi: cfg80211: Update the default DSCP-to-UP
> mapping
>
> 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?
>
I was using these specifications since they were mentioned in the Wi-Fi QoS Management specification
and because RFC8325 was specifically mentioned in the WFA test plan specifications.
I'll update the commit message.
> > 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..
>
The term "Default DSCP-to-UP" is defined in section 2.3 in RFC8325. As noted there, this is
not a specification definition, but more a term to describe what is a "common practice in the
networking industry".
> > + /* 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.
>
Yes. I thought my comment above was clear about it, but I'll rephrase it to make
It clearer.
> > + 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".
>
Fixed.
> 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.
>
The default mapping maps AF31, AF32 and AF33 to 4 which is the recommended mapping.
I'll add a comment.
> 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.
>
Same here. The default mapping is used. I'll add a comment.
> 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.
>
Same.
> > + case 40:
> > + /* Signaling: CS5 */
> > + ret = 5;
> > + break;
> > + case 44:
> > + /* Voice Admit */
> > + ret = 6;
> > + break;
>
> To be consistent, that comment should be "VOICE-ADMIT: VA".
>
Fixed.
> > + 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.
>
Same.
Regards,
Ilan.
