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

[Jakub Kicinski <kuba@kernel.org>]

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