[PATCH net] ice: Fix undersized tx_flags variable

[PATCH net] ice: Fix undersized tx_flags variable

[Tony Nguyen]

From: Jan Sokolowski <jan.sokolowski@intel.com>

As not all ICE_TX_FLAGS_* fit in current 16-bit limited
tx_flags field, some VLAN-related flags would not properly apply.

Fix that by refactoring tx_flags variable into flags only and
a separate variable that holds VLAN ID. As there is some space left,
type variable can fit between those two. Pahole reports no size
change to ice_tx_buf struct.

Fixes: aa1d3faf71a6 ("ice: Robustify cleaning/completing XDP Tx buffers")
Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
Tested-by: Michal Schmidt <mschmidt@redhat.com>
Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_dcb_lib.c |  6 +++---
 drivers/net/ethernet/intel/ice/ice_txrx.c    |  8 +++-----
 drivers/net/ethernet/intel/ice/ice_txrx.h    | 12 ++++++------
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
index c6d4926f0fcf..6c212d0dbef3 100644
--- a/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_dcb_lib.c
@@ -932,10 +932,10 @@ ice_tx_prepare_vlan_flags_dcb(struct ice_tx_ring *tx_ring,
 	if ((first->tx_flags & ICE_TX_FLAGS_HW_VLAN ||
 	     first->tx_flags & ICE_TX_FLAGS_HW_OUTER_SINGLE_VLAN) ||
 	    skb->priority != TC_PRIO_CONTROL) {
-		first->tx_flags &= ~ICE_TX_FLAGS_VLAN_PR_M;
+		first->vid &= ~ICE_TX_VLAN_PR_M;
 		/* Mask the lower 3 bits to set the 802.1p priority */
-		first->tx_flags |= (skb->priority & 0x7) <<
-				   ICE_TX_FLAGS_VLAN_PR_S;
+		first->vid |= (skb->priority << ICE_TX_VLAN_PR_S) &
+			      ICE_TX_VLAN_PR_M;
 		/* if this is not already set it means a VLAN 0 + priority needs
 		 * to be offloaded
 		 */
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
index 4fcf2d07eb85..059bd911c51d 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.c
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
@@ -1664,8 +1664,7 @@ ice_tx_map(struct ice_tx_ring *tx_ring, struct ice_tx_buf *first,
 
 	if (first->tx_flags & ICE_TX_FLAGS_HW_VLAN) {
 		td_cmd |= (u64)ICE_TX_DESC_CMD_IL2TAG1;
-		td_tag = (first->tx_flags & ICE_TX_FLAGS_VLAN_M) >>
-			  ICE_TX_FLAGS_VLAN_S;
+		td_tag = first->vid;
 	}
 
 	dma = dma_map_single(tx_ring->dev, skb->data, size, DMA_TO_DEVICE);
@@ -1998,7 +1997,7 @@ ice_tx_prepare_vlan_flags(struct ice_tx_ring *tx_ring, struct ice_tx_buf *first)
 	 * VLAN offloads exclusively so we only care about the VLAN ID here
 	 */
 	if (skb_vlan_tag_present(skb)) {
-		first->tx_flags |= skb_vlan_tag_get(skb) << ICE_TX_FLAGS_VLAN_S;
+		first->vid = skb_vlan_tag_get(skb);
 		if (tx_ring->flags & ICE_TX_FLAGS_RING_VLAN_L2TAG2)
 			first->tx_flags |= ICE_TX_FLAGS_HW_OUTER_SINGLE_VLAN;
 		else
@@ -2388,8 +2387,7 @@ ice_xmit_frame_ring(struct sk_buff *skb, struct ice_tx_ring *tx_ring)
 		offload.cd_qw1 |= (u64)(ICE_TX_DESC_DTYPE_CTX |
 					(ICE_TX_CTX_DESC_IL2TAG2 <<
 					ICE_TXD_CTX_QW1_CMD_S));
-		offload.cd_l2tag2 = (first->tx_flags & ICE_TX_FLAGS_VLAN_M) >>
-			ICE_TX_FLAGS_VLAN_S;
+		offload.cd_l2tag2 = first->vid;
 	}
 
 	/* set up TSO offload */
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
index fff0efe28373..76a34d435025 100644
--- a/drivers/net/ethernet/intel/ice/ice_txrx.h
+++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
@@ -127,10 +127,9 @@ static inline int ice_skb_pad(void)
 #define ICE_TX_FLAGS_IPV6	BIT(6)
 #define ICE_TX_FLAGS_TUNNEL	BIT(7)
 #define ICE_TX_FLAGS_HW_OUTER_SINGLE_VLAN	BIT(8)
-#define ICE_TX_FLAGS_VLAN_M	0xffff0000
-#define ICE_TX_FLAGS_VLAN_PR_M	0xe0000000
-#define ICE_TX_FLAGS_VLAN_PR_S	29
-#define ICE_TX_FLAGS_VLAN_S	16
+
+#define ICE_TX_VLAN_PR_M	0xe000
+#define ICE_TX_VLAN_PR_S	13
 
 #define ICE_XDP_PASS		0
 #define ICE_XDP_CONSUMED	BIT(0)
@@ -182,8 +181,9 @@ struct ice_tx_buf {
 		unsigned int gso_segs;
 		unsigned int nr_frags;	/* used for mbuf XDP */
 	};
-	u32 type:16;			/* &ice_tx_buf_type */
-	u32 tx_flags:16;
+	u32 tx_flags:12;
+	u32 type:4;			/* &ice_tx_buf_type */
+	u32 vid:16;
 	DEFINE_DMA_UNMAP_LEN(len);
 	DEFINE_DMA_UNMAP_ADDR(dma);
 };
-- 
2.38.1

Re: [PATCH net] ice: Fix undersized tx_flags variable

[Simon Horman]

On Mon, May 08, 2023 at 10:42:25AM -0700, Tony Nguyen wrote:
> From: Jan Sokolowski <jan.sokolowski@intel.com>
> 
> As not all ICE_TX_FLAGS_* fit in current 16-bit limited
> tx_flags field, some VLAN-related flags would not properly apply.
> 
> Fix that by refactoring tx_flags variable into flags only and
> a separate variable that holds VLAN ID. As there is some space left,
> type variable can fit between those two. Pahole reports no size
> change to ice_tx_buf struct.
> 
> Fixes: aa1d3faf71a6 ("ice: Robustify cleaning/completing XDP Tx buffers")
> Signed-off-by: Jan Sokolowski <jan.sokolowski@intel.com>
> Reviewed-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> Tested-by: Michal Schmidt <mschmidt@redhat.com>
> Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH net] ice: Fix undersized tx_flags variable

[Jakub Kicinski]

On Mon,  8 May 2023 10:42:25 -0700 Tony Nguyen wrote:
> From: Jan Sokolowski <jan.sokolowski@intel.com>
> 
> As not all ICE_TX_FLAGS_* fit in current 16-bit limited
> tx_flags field, some VLAN-related flags would not properly apply.

nit: this is a bit of a understatement. The vlan info is gone
completely, right? Maybe say something along the lines of:

"VLAN ID was stored on upper 16 bits of tx_flags, the commit under
Fixes reduced the size of tx_flags to 16 bits discarding vlan
information completely."

> Fix that by refactoring tx_flags variable into flags only and
> a separate variable that holds VLAN ID.

This sentence just describes what the patch does, it's not necessary.

> As there is some space left,
> type variable can fit between those two. Pahole reports no size
> change to ice_tx_buf struct.

You need to also describe user-visible misbehavior.

> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h
> index fff0efe28373..76a34d435025 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h
> @@ -127,10 +127,9 @@ static inline int ice_skb_pad(void)
>  #define ICE_TX_FLAGS_IPV6	BIT(6)
>  #define ICE_TX_FLAGS_TUNNEL	BIT(7)
>  #define ICE_TX_FLAGS_HW_OUTER_SINGLE_VLAN	BIT(8)
> -#define ICE_TX_FLAGS_VLAN_M	0xffff0000
> -#define ICE_TX_FLAGS_VLAN_PR_M	0xe0000000
> -#define ICE_TX_FLAGS_VLAN_PR_S	29
> -#define ICE_TX_FLAGS_VLAN_S	16
> +
> +#define ICE_TX_VLAN_PR_M	0xe000
> +#define ICE_TX_VLAN_PR_S	13

You can use VLAN_PRIO_MASK and VLAN_PRIO_SHIFT if you're storing
it in a normal-ish 16b field.
-- 
pw-bot: cr