[v4] net/netvsc: fix parsing of VLAN metadata

Message ID PA4PR83MB0526A80FD64166748D7E1962974B2@PA4PR83MB0526.EURPRD83.prod.outlook.com (mailing list archive)
State Superseded
Delegated to: Ferruh Yigit
Headers
Series [v4] net/netvsc: fix parsing of VLAN metadata |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/iol-abi-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-compile-arm64-testing success Testing PASS

Commit Message

Alan Elder Feb. 9, 2024, 3:50 p.m. UTC
  The previous code incorrectly parsed the VLAN ID and priority.
If the 16-bits of VLAN ID and priority/CFI on the wire was
0123456789ABCDEF the code parsed it as 456789ABCDEF3012.  There
were macros defined to handle this conversion but they were not
used.

Fixes: 4e9c73e96e83 ("net/netvsc: add Hyper-V network device")
Cc: sthemmin@microsoft.com
Cc: stable@dpdk.org

Signed-off-by: Alan Elder <alan.elder@microsoft.com>
---
v4:
* Make consistent with FreeBSD code

 .mailmap                     |  1 +
 drivers/net/netvsc/hn_rxtx.c | 21 +++++++++++++++++++--
 2 files changed, 20 insertions(+), 2 deletions(-)
  

Comments

Long Li Feb. 14, 2024, 10:17 p.m. UTC | #1
> +#define HN_VLAN_CFI_SHIFT	12
> +#define HN_VLAN_PRI_SHIFT	13
> +#define HN_VLAN_PRI_MASK	0xe000 /* Priority Code Point */
> +#define HN_VLAN_CFI_MASK	0x1000 /* Canonical Format Indicator / Drop
> Eligible Indicator */
> +#define HN_VLAN_VID_MASK	0x0fff /* VLAN Identifier */
> +
> +#define HN_VLAN_TCI_ID(vlan_tci)	((vlan_tci) & HN_VLAN_VID_MASK)
> +#define HN_VLAN_TCI_PRI(vlan_tci)	(((vlan_tci) & HN_VLAN_PRI_MASK) >>
> HN_VLAN_PRI_SHIFT)
> +#define HN_VLAN_TCI_CFI(vlan_tci)	(((vlan_tci) & HN_VLAN_CFI_MASK) >>
> HN_VLAN_CFI_SHIFT)
> +#define HN_VLAN_TCI_MAKE(id, pri, cfi)	((id) |
> 	\
> +					 ((pri) << HN_VLAN_PRI_SHIFT) |	\
> +					 ((cfi) << HN_VLAN_CFI_SHIFT))
> +

The patch looks good.

It seems HN_VLAN_TCI_ID, HN_VLAN_TCI_PRI, HN_VLAN_TCI_CFI and HN_VLAN_TCI_MAKE could be useful to other drivers. (at least to MANA)

Ferruh, do you think we should define those common functions in ./lib/net/rte_ether.h?

Thanks

Long
  
Ferruh Yigit Feb. 15, 2024, 11:46 a.m. UTC | #2
On 2/14/2024 10:17 PM, Long Li wrote:
>> +#define HN_VLAN_CFI_SHIFT	12
>> +#define HN_VLAN_PRI_SHIFT	13
>> +#define HN_VLAN_PRI_MASK	0xe000 /* Priority Code Point */
>> +#define HN_VLAN_CFI_MASK	0x1000 /* Canonical Format Indicator / Drop
>> Eligible Indicator */
>> +#define HN_VLAN_VID_MASK	0x0fff /* VLAN Identifier */
>> +
>> +#define HN_VLAN_TCI_ID(vlan_tci)	((vlan_tci) & HN_VLAN_VID_MASK)
>> +#define HN_VLAN_TCI_PRI(vlan_tci)	(((vlan_tci) & HN_VLAN_PRI_MASK) >>
>> HN_VLAN_PRI_SHIFT)
>> +#define HN_VLAN_TCI_CFI(vlan_tci)	(((vlan_tci) & HN_VLAN_CFI_MASK) >>
>> HN_VLAN_CFI_SHIFT)
>> +#define HN_VLAN_TCI_MAKE(id, pri, cfi)	((id) |
>> 	\
>> +					 ((pri) << HN_VLAN_PRI_SHIFT) |	\
>> +					 ((cfi) << HN_VLAN_CFI_SHIFT))
>> +
> 
> The patch looks good.
> 
> It seems HN_VLAN_TCI_ID, HN_VLAN_TCI_PRI, HN_VLAN_TCI_CFI and HN_VLAN_TCI_MAKE could be useful to other drivers. (at least to MANA)
> 
> Ferruh, do you think we should define those common functions in ./lib/net/rte_ether.h?
> 

Hi Long,

That is good idea indeed, so others can benefit from them. Thanks.

@Alan, can you please move above macros to './lib/net/rte_ether.h', with
RTE_VLAN_ prefix? And use them from net library in the driver.

Btw, CFI seems renamed to DEI (Drop Eligible Indicator), perhaps can be
good to go with that acronym.
  

Patch

diff --git a/.mailmap b/.mailmap
index a0756974e2..eca02318d6 100644
--- a/.mailmap
+++ b/.mailmap
@@ -33,6 +33,7 @@  Alain Leon <xerebz@gmail.com>
 Alan Brady <alan.brady@intel.com>
 Alan Carew <alan.carew@intel.com>
 Alan Dewar <alan.dewar@att.com> <adewar@brocade.com>
+Alan Elder <alan.elder@microsoft.com>
 Alan Liu <zaoxingliu@gmail.com>
 Alan Winkowski <walan@marvell.com>
 Alejandro Lucero <alejandro.lucero@netronome.com>
diff --git a/drivers/net/netvsc/hn_rxtx.c b/drivers/net/netvsc/hn_rxtx.c
index e4f5015aa3..1cbb8df03f 100644
--- a/drivers/net/netvsc/hn_rxtx.c
+++ b/drivers/net/netvsc/hn_rxtx.c
@@ -42,6 +42,19 @@ 
 #define HN_TXD_CACHE_SIZE	32 /* per cpu tx_descriptor pool cache */
 #define HN_RXQ_EVENT_DEFAULT	2048
 
+#define HN_VLAN_CFI_SHIFT	12
+#define HN_VLAN_PRI_SHIFT	13
+#define HN_VLAN_PRI_MASK	0xe000 /* Priority Code Point */
+#define HN_VLAN_CFI_MASK	0x1000 /* Canonical Format Indicator / Drop Eligible Indicator */
+#define HN_VLAN_VID_MASK	0x0fff /* VLAN Identifier */
+
+#define HN_VLAN_TCI_ID(vlan_tci)	((vlan_tci) & HN_VLAN_VID_MASK)
+#define HN_VLAN_TCI_PRI(vlan_tci)	(((vlan_tci) & HN_VLAN_PRI_MASK) >> HN_VLAN_PRI_SHIFT)
+#define HN_VLAN_TCI_CFI(vlan_tci)	(((vlan_tci) & HN_VLAN_CFI_MASK) >> HN_VLAN_CFI_SHIFT)
+#define HN_VLAN_TCI_MAKE(id, pri, cfi)	((id) |				\
+					 ((pri) << HN_VLAN_PRI_SHIFT) |	\
+					 ((cfi) << HN_VLAN_CFI_SHIFT))
+
 struct hn_rxinfo {
 	uint32_t	vlan_info;
 	uint32_t	csum_info;
@@ -612,7 +625,9 @@  static void hn_rxpkt(struct hn_rx_queue *rxq, struct hn_rx_bufinfo *rxb,
 					   RTE_PTYPE_L4_MASK);
 
 	if (info->vlan_info != HN_NDIS_VLAN_INFO_INVALID) {
-		m->vlan_tci = info->vlan_info;
+		m->vlan_tci = HN_VLAN_TCI_MAKE(NDIS_VLAN_INFO_ID(info->vlan_info),
+					       NDIS_VLAN_INFO_PRI(info->vlan_info),
+					       NDIS_VLAN_INFO_CFI(info->vlan_info));
 		m->ol_flags |= RTE_MBUF_F_RX_VLAN_STRIPPED | RTE_MBUF_F_RX_VLAN;
 
 		/* NDIS always strips tag, put it back if necessary */
@@ -1332,7 +1347,9 @@  static void hn_encap(struct rndis_packet_msg *pkt,
 	if (m->ol_flags & RTE_MBUF_F_TX_VLAN) {
 		pi_data = hn_rndis_pktinfo_append(pkt, NDIS_VLAN_INFO_SIZE,
 						  NDIS_PKTINFO_TYPE_VLAN);
-		*pi_data = m->vlan_tci;
+		*pi_data = NDIS_VLAN_INFO_MAKE(HN_VLAN_TCI_ID(m->vlan_tci),
+					       HN_VLAN_TCI_PRI(m->vlan_tci),
+					       HN_VLAN_TCI_CFI(m->vlan_tci));
 	}
 
 	if (m->ol_flags & RTE_MBUF_F_TX_TCP_SEG) {