| Message ID | 1372319520-29087-1-git-send-email-cedric.voncken@acksys.fr (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Subject line should have a "wireless: " (or "cfg80211: ") prefix. > If the VLAN tci is set in skb->vlan_tci use the priority field to determine the WMM priority. > > V2 modifications : > Fix indentation > Use symbolic constant > include the header linux/if_vlan.h This doesn't belong into the changelog, it should be after --- with the diffstat. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> + vlan_priority = (skb->vlan_tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT; > + if (vlan_priority > 0) > + return vlan_priority; Is this really correct? For once, checking vlan_priority for non-zero seems weird since that ought to be a valid value -- is there no other way to determine that the value is valid? Also, this gives you a 2-bit value, while the return value should be a 3-bit value, maybe you need to shift this up by one to spread into the correct buckets? johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >+ vlan_priority = (skb->vlan_tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT; > >+ if (vlan_priority > 0) >> + return vlan_priority; >Is this really correct? For once, checking vlan_priority for non-zero seems weird since that ought to be a valid value -- is there no other way to determine that the value is valid? The vlan Tag contain three bit for priority. The value 0 indicate no priority (on this case the VLAN tag contain only VID). The vlan_tci field is set to zero if the frame do not contain the vlan tag. So if we have not a vlan tag or no priority in VLAN tag the priority value is always 0. >Also, this gives you a 2-bit value, while the return value should be a 3-bit value, maybe you need to shift this up by one to spread into the correct buckets? Sorry but I don't understand. The vlan_tci field it is a __u16 value (defined in include/linux/skbuff.h), the VLAN_PRIO_MASK is set to 0xE000 and VLAN_PRIO_SHIFT is set to 13 (defined in include/linux/if_vlan.h), the vlan_priority is an unsigned char. For me the vlan_priority contain a 3-bit value (0xE000 >>13 = 0x0003), why 2 ? If you are agree with me, I'll resend a V3 patch with a correct subject. Cedric >johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-07-05 at 16:39 +0200, voncken wrote: > > >+ vlan_priority = (skb->vlan_tci & VLAN_PRIO_MASK) >> > VLAN_PRIO_SHIFT; > > >+ if (vlan_priority > 0) > >> + return vlan_priority; > > > Is this really correct? For once, checking vlan_priority for > non-zero seems > > weird since that ought to be a valid value -- is there no other way > to > > determine that the value is valid? > The vlan Tag contain three bit for priority. The value 0 indicate no > priority (on this case the VLAN tag contain only VID). The vlan_tci > field is set to zero if the frame do not contain the vlan tag. So if > we have not a vlan tag or no priority in VLAN tag the priority value > is always 0. Yes but don't we know that the vlan_tci field is valid? I don't think you're correct in that 0 means "no priority present", it actually means "best effort" as far as I can tell. Ignoring the VLAN tag when the field is 0 would mean we could use a higher priority from the contents of the frame, which would not be desired? > >Also, this gives you a 2-bit value, while the return value should be > a 3-bit value, maybe you need to shift this up by one to spread into > the correct buckets? > Sorry but I don't understand. The vlan_tci field it is a __u16 value > (defined in include/linux/skbuff.h), the VLAN_PRIO_MASK is set to > 0xE000 and VLAN_PRIO_SHIFT is set to 13 (defined in > include/linux/if_vlan.h), the vlan_priority is an unsigned char. For > me the vlan_priority contain a 3-bit value (0xE000 >>13 = 0x0003), why > 2 ? Umm? No? Think again about what hweight(0x0003) is. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > The vlan Tag contain three bit for priority. The value 0 indicate no > > priority (on this case the VLAN tag contain only VID). The vlan_tci > > field is set to zero if the frame do not contain the vlan tag. So if > > we have not a vlan tag or no priority in VLAN tag the priority value > > is always 0. > Yes but don't we know that the vlan_tci field is valid? > I don't think you're correct in that 0 means "no priority present", it actually means "best effort" as far as I can tell. Ignoring the VLAN tag when the field is 0 would mean we could use a higher priority from the contents of the frame, which would not be desired? I can add a test with the macro vlan_tx_tag_present() to verify if the vlan_tci field is valid. I test the value 0 to skip the VLAN priority and use the dscp priority in this case. The priority 0 in VLAN tag is often use to turn off the QOS, because not bit is allowed for it. For me is it correct. Nevertheless, if you prefer, I can test only the vlan_tci validity and in this case always use the VLAN priority. > > Sorry but I don't understand. The vlan_tci field it is a __u16 value > > (defined in include/linux/skbuff.h), the VLAN_PRIO_MASK is set to > > 0xE000 and VLAN_PRIO_SHIFT is set to 13 (defined in > > include/linux/if_vlan.h), the vlan_priority is an unsigned char. For > > me the vlan_priority contain a 3-bit value (0xE000 >>13 = 0x0003), why > > 2 ? > Umm? No? Think again about what hweight(0x0003) is. Sorry I made a mistake 0xE000 >>13 = 0x0007 and not 0x0003, and 7 is a 3 bits value. Cedric -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-07-08 at 12:39 +0200, voncken wrote: > > > The vlan Tag contain three bit for priority. The value 0 indicate > no > > > priority (on this case the VLAN tag contain only VID). The > vlan_tci > > > field is set to zero if the frame do not contain the vlan tag. So > if > > > we have not a vlan tag or no priority in VLAN tag the priority > value > > > is always 0. > > > Yes but don't we know that the vlan_tci field is valid? > > > I don't think you're correct in that 0 means "no priority present", > it actually means "best effort" as far as I can tell. Ignoring the > VLAN tag when the field is 0 would mean we could use a higher priority > from the contents of the frame, which would not be desired? > > I can add a test with the macro vlan_tx_tag_present() to verify if the > vlan_tci field is valid. > I test the value 0 to skip the VLAN priority and use the dscp priority > in this case. The priority 0 in VLAN tag is often use to turn off the > QOS, because not bit is allowed for it. What do you mean by "is often used"? I don't see how that would be the case? Are you saying routers commonly ignore the VLAN priority value if it's 0? That would seem odd? > For me is it correct. Nevertheless, if you prefer, I can test only the > vlan_tci validity and in this case always use the VLAN priority. I don't know! Since you don't seem to really know either, we should ask somebody who knows, I think. Maybe you should Cc netdev with this question on the patch or so? > Sorry I made a mistake 0xE000 >>13 = 0x0007 and not 0x0003, and 7 is > a 3 bits value. Ah yes, I made the same mistake, sorry. johannes -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/wireless/util.c b/net/wireless/util.c index 74458b7..13937db 100644 --- a/net/wireless/util.c +++ b/net/wireless/util.c @@ -10,6 +10,7 @@ #include <net/cfg80211.h> #include <net/ip.h> #include <net/dsfield.h> +#include <linux/if_vlan.h> #include "core.h" #include "rdev-ops.h" @@ -685,6 +686,7 @@ EXPORT_SYMBOL(ieee80211_amsdu_to_8023s); unsigned int cfg80211_classify8021d(struct sk_buff *skb) { unsigned int dscp; + unsigned char vlan_priority; /* skb->priority values from 256->263 are magic values to * directly indicate a specific 802.1d priority. This is used @@ -694,6 +696,10 @@ unsigned int cfg80211_classify8021d(struct sk_buff *skb) if (skb->priority >= 256 && skb->priority <= 263) return skb->priority - 256; + vlan_priority = (skb->vlan_tci & VLAN_PRIO_MASK) >> VLAN_PRIO_SHIFT; + if (vlan_priority > 0) + return vlan_priority; + switch (skb->protocol) { case htons(ETH_P_IP): dscp = ipv4_get_dsfield(ip_hdr(skb)) & 0xfc;