[PATCH iwl-next v2 0/5] drop unnecessary constant casts to u16

[PATCH iwl-next v2 0/5] drop unnecessary constant casts to u16

[Jacek Kowalski]

As requested by Simon Horman, here's the patch set to drop casts of
constants to u16 in comparisons and subtractions.

Changes are applied across all Intel wired drivers.

No behavioural changes intended.
Compile tested only.

v1 -> v2: drop casts in subtractions as well

Jacek Kowalski (5):
  e1000: drop unnecessary constant casts to u16
  e1000e: drop unnecessary constant casts to u16
  igb: drop unnecessary constant casts to u16
  igc: drop unnecessary constant casts to u16
  ixgbe: drop unnecessary constant casts to u16

 drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 +-
 drivers/net/ethernet/intel/e1000/e1000_hw.c      | 4 ++--
 drivers/net/ethernet/intel/e1000/e1000_main.c    | 2 +-
 drivers/net/ethernet/intel/e1000e/ethtool.c      | 2 +-
 drivers/net/ethernet/intel/e1000e/nvm.c          | 4 ++--
 drivers/net/ethernet/intel/igb/e1000_82575.c     | 4 ++--
 drivers/net/ethernet/intel/igb/e1000_i210.c      | 2 +-
 drivers/net/ethernet/intel/igb/e1000_nvm.c       | 4 ++--
 drivers/net/ethernet/intel/igc/igc_i225.c        | 2 +-
 drivers/net/ethernet/intel/igc/igc_nvm.c         | 4 ++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.c  | 2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c    | 2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c    | 2 +-
 13 files changed, 18 insertions(+), 18 deletions(-)

-- 
2.47.2

[PATCH iwl-next v2 1/5] e1000: drop unnecessary constant casts to u16

[Jacek Kowalski]

Remove unnecessary casts of constant values to u16.
Let the C type system do it's job.

Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
Suggested-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 +-
 drivers/net/ethernet/intel/e1000/e1000_hw.c      | 4 ++--
 drivers/net/ethernet/intel/e1000/e1000_main.c    | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
index d06d29c6c037..d152026a027b 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
@@ -806,7 +806,7 @@ static int e1000_eeprom_test(struct e1000_adapter *adapter, u64 *data)
 	}
 
 	/* If Checksum is not Correct return error else test passed */
-	if ((checksum != (u16)EEPROM_SUM) && !(*data))
+	if ((checksum != EEPROM_SUM) && !(*data))
 		*data = 2;
 
 	return *data;
diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.c b/drivers/net/ethernet/intel/e1000/e1000_hw.c
index f9328f2e669f..0e5de52b1067 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_hw.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_hw.c
@@ -3970,7 +3970,7 @@ s32 e1000_validate_eeprom_checksum(struct e1000_hw *hw)
 		return E1000_SUCCESS;
 
 #endif
-	if (checksum == (u16)EEPROM_SUM)
+	if (checksum == EEPROM_SUM)
 		return E1000_SUCCESS;
 	else {
 		e_dbg("EEPROM Checksum Invalid\n");
@@ -3997,7 +3997,7 @@ s32 e1000_update_eeprom_checksum(struct e1000_hw *hw)
 		}
 		checksum += eeprom_data;
 	}
-	checksum = (u16)EEPROM_SUM - checksum;
+	checksum = EEPROM_SUM - checksum;
 	if (e1000_write_eeprom(hw, EEPROM_CHECKSUM_REG, 1, &checksum) < 0) {
 		e_dbg("EEPROM Write Error\n");
 		return -E1000_ERR_EEPROM;
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index d8595e84326d..09acba2ed483 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -313,7 +313,7 @@ static void e1000_update_mng_vlan(struct e1000_adapter *adapter)
 		} else {
 			adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;
 		}
-		if ((old_vid != (u16)E1000_MNG_VLAN_NONE) &&
+		if ((old_vid != E1000_MNG_VLAN_NONE) &&
 		    (vid != old_vid) &&
 		    !test_bit(old_vid, adapter->active_vlans))
 			e1000_vlan_rx_kill_vid(netdev, htons(ETH_P_8021Q),
-- 
2.47.2

Re: [PATCH iwl-next v2 1/5] e1000: drop unnecessary constant casts to u16

[Simon Horman]

On Tue, Jul 08, 2025 at 10:16:52AM +0200, Jacek Kowalski wrote:
> Remove unnecessary casts of constant values to u16.
> Let the C type system do it's job.
> 
> Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
> Suggested-by: Simon Horman <horms@kernel.org>
> ---
>  drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 +-
>  drivers/net/ethernet/intel/e1000/e1000_hw.c      | 4 ++--
>  drivers/net/ethernet/intel/e1000/e1000_main.c    | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
> index d06d29c6c037..d152026a027b 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
> @@ -806,7 +806,7 @@ static int e1000_eeprom_test(struct e1000_adapter *adapter, u64 *data)
>  	}
>  
>  	/* If Checksum is not Correct return error else test passed */
> -	if ((checksum != (u16)EEPROM_SUM) && !(*data))
> +	if ((checksum != EEPROM_SUM) && !(*data))
>  		*data = 2;

nit: If there is a v3 for some other reason, then I think
     you could also drop the inner parentheses here.

>  
>  	return *data;
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.c b/drivers/net/ethernet/intel/e1000/e1000_hw.c
> index f9328f2e669f..0e5de52b1067 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_hw.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_hw.c
> @@ -3970,7 +3970,7 @@ s32 e1000_validate_eeprom_checksum(struct e1000_hw *hw)
>  		return E1000_SUCCESS;
>  
>  #endif
> -	if (checksum == (u16)EEPROM_SUM)
> +	if (checksum == EEPROM_SUM)
>  		return E1000_SUCCESS;
>  	else {
>  		e_dbg("EEPROM Checksum Invalid\n");
> @@ -3997,7 +3997,7 @@ s32 e1000_update_eeprom_checksum(struct e1000_hw *hw)
>  		}
>  		checksum += eeprom_data;
>  	}
> -	checksum = (u16)EEPROM_SUM - checksum;
> +	checksum = EEPROM_SUM - checksum;
>  	if (e1000_write_eeprom(hw, EEPROM_CHECKSUM_REG, 1, &checksum) < 0) {
>  		e_dbg("EEPROM Write Error\n");
>  		return -E1000_ERR_EEPROM;
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
> index d8595e84326d..09acba2ed483 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> @@ -313,7 +313,7 @@ static void e1000_update_mng_vlan(struct e1000_adapter *adapter)
>  		} else {
>  			adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;
>  		}
> -		if ((old_vid != (u16)E1000_MNG_VLAN_NONE) &&
> +		if ((old_vid != E1000_MNG_VLAN_NONE) &&

Ditto.

But more importantly, both Clang 20.1.7 W=1 builds (or at any rate, builds
with -Wtautological-constant-out-of-range-compare), and Smatch complain
that the comparison above is now always true because E1000_MNG_VLAN_NONE is
-1, while old_vid is unsigned.

Perhaps E1000_MNG_VLAN_NONE should be updated to be UINT16_MAX?


>  		    (vid != old_vid) &&
>  		    !test_bit(old_vid, adapter->active_vlans))
>  			e1000_vlan_rx_kill_vid(netdev, htons(ETH_P_8021Q),
> -- 
> 2.47.2
> 

Re: [PATCH iwl-next v2 1/5] e1000: drop unnecessary constant casts to u16

[Jacek Kowalski]

>> -		if ((old_vid != (u16)E1000_MNG_VLAN_NONE) &&
>> +		if ((old_vid != E1000_MNG_VLAN_NONE) &&
>
> Ditto.
>
> But more importantly, both Clang 20.1.7 W=1 builds (or at any rate,
> builds with -Wtautological-constant-out-of-range-compare), and Smatch
> complain that the comparison above is now always true because
> E1000_MNG_VLAN_NONE is -1, while old_vid is unsigned.

You are right - I have missed that E1000_MNG_VLAN_NONE is negative.
Therefore (u16)E1000_MNG_VLAN_NONE has a side effect of causing a 
wraparound.

It's even more interesting that (inadvertently) I have not made a 
similar change in e1000e:

./drivers/net/ethernet/intel/e1000e/netdev.c:
if (adapter->mng_vlan_id != (u16)E1000_MNG_VLAN_NONE) {


> Perhaps E1000_MNG_VLAN_NONE should be updated to be UINT16_MAX?

There's no UINT16_MAX in kernel as far as I know. I'd rather leave it as 
it was or, if you insist on further refactoring, use either one of:

#define E1000_MNG_VLAN_NONE (u16)(~((u16) 0))
mimick ACPI: #define ACPI_UINT16_MAX                 (u16)(~((u16) 0))

#define E1000_MNG_VLAN_NONE ((u16)-1)
move the cast into the constant

#define E1000_MNG_VLAN_NONE 0xFFFF
use ready-made value

(parentheses left only due to the constant being "(-1)" and not "-1").

-- 
Best regards,
   Jacek Kowalski

Re: [PATCH iwl-next v2 1/5] e1000: drop unnecessary constant casts to u16

[David Laight]

On Tue, 8 Jul 2025 21:40:12 +0200
Jacek Kowalski <jacek@jacekk.info> wrote:

> >> -		if ((old_vid != (u16)E1000_MNG_VLAN_NONE) &&
> >> +		if ((old_vid != E1000_MNG_VLAN_NONE) &&  
> >
> > Ditto.
> >
> > But more importantly, both Clang 20.1.7 W=1 builds (or at any rate,
> > builds with -Wtautological-constant-out-of-range-compare), and Smatch
> > complain that the comparison above is now always true because
> > E1000_MNG_VLAN_NONE is -1, while old_vid is unsigned.

I'm guessing 'old_vid' is actually u16 (or the compiler knows the
value came from a u16).

In either case the compiler can 'know' that the condition is always
true - but a 'u16 old_vid' is promoted to 'signed int' prior to
the compare with -1, whereas if a 'u32 old_vid' is known to contain
a 16bit value it is the -1 that is converted to ~0u.

> 
> You are right - I have missed that E1000_MNG_VLAN_NONE is negative.
> Therefore (u16)E1000_MNG_VLAN_NONE has a side effect of causing a 
> wraparound.
> 
> It's even more interesting that (inadvertently) I have not made a 
> similar change in e1000e:
> 
> ./drivers/net/ethernet/intel/e1000e/netdev.c:
> if (adapter->mng_vlan_id != (u16)E1000_MNG_VLAN_NONE) {
> 
> 
> > Perhaps E1000_MNG_VLAN_NONE should be updated to be UINT16_MAX?  
> 
> There's no UINT16_MAX in kernel as far as I know. I'd rather leave it as 
> it was or, if you insist on further refactoring, use either one of:
> 
> #define E1000_MNG_VLAN_NONE (u16)(~((u16) 0))
> mimick ACPI: #define ACPI_UINT16_MAX                 (u16)(~((u16) 0))
> 
> #define E1000_MNG_VLAN_NONE ((u16)-1)
> move the cast into the constant
> 
> #define E1000_MNG_VLAN_NONE 0xFFFF
> use ready-made value

Possibly better is 0xFFFFu.
Then 'u16 old_val' is first promoted to 'signed int' and then implicitly
cast to 'unsigned int' prior to the comparison.

Remember C does all maths as [un]signed int (except for types bigger than int).
Local variables, function parameters and function results should really
be [un]signed int provided the domain of value doesn't exceed that of int.
Otherwise the compile is forced to generate explicit instructions to mask
the result of arithmetic operations to the smaller type.

	David 

> 
> (parentheses left only due to the constant being "(-1)" and not "-1").
> 

Re: [PATCH iwl-next v2 1/5] e1000: drop unnecessary constant casts to u16

[kernel test robot]

Hi Jacek,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tnguy-next-queue/dev-queue]

url:    https://github.com/intel-lab-lkp/linux/commits/Jacek-Kowalski/e1000-drop-unnecessary-constant-casts-to-u16/20250708-161919
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue.git dev-queue
patch link:    https://lore.kernel.org/r/e199da76-00d0-43d3-8f61-f433bc0352ad%40jacekk.info
patch subject: [PATCH iwl-next v2 1/5] e1000: drop unnecessary constant casts to u16
config: riscv-randconfig-002-20250709 (https://download.01.org/0day-ci/archive/20250709/202507091034.uiPhnpcc-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project 01c97b4953e87ae455bd4c41e3de3f0f0f29c61c)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250709/202507091034.uiPhnpcc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507091034.uiPhnpcc-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/ethernet/intel/e1000/e1000_main.c:316:16: warning: result of comparison of constant -1 with expression of type 'u16' (aka 'unsigned short') is always true [-Wtautological-constant-out-of-range-compare]
     316 |                 if ((old_vid != E1000_MNG_VLAN_NONE) &&
         |                      ~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~
   1 warning generated.


vim +316 drivers/net/ethernet/intel/e1000/e1000_main.c

   297	
   298	static void e1000_update_mng_vlan(struct e1000_adapter *adapter)
   299	{
   300		struct e1000_hw *hw = &adapter->hw;
   301		struct net_device *netdev = adapter->netdev;
   302		u16 vid = hw->mng_cookie.vlan_id;
   303		u16 old_vid = adapter->mng_vlan_id;
   304	
   305		if (!e1000_vlan_used(adapter))
   306			return;
   307	
   308		if (!test_bit(vid, adapter->active_vlans)) {
   309			if (hw->mng_cookie.status &
   310			    E1000_MNG_DHCP_COOKIE_STATUS_VLAN_SUPPORT) {
   311				e1000_vlan_rx_add_vid(netdev, htons(ETH_P_8021Q), vid);
   312				adapter->mng_vlan_id = vid;
   313			} else {
   314				adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;
   315			}
 > 316			if ((old_vid != E1000_MNG_VLAN_NONE) &&
   317			    (vid != old_vid) &&
   318			    !test_bit(old_vid, adapter->active_vlans))
   319				e1000_vlan_rx_kill_vid(netdev, htons(ETH_P_8021Q),
   320						       old_vid);
   321		} else {
   322			adapter->mng_vlan_id = vid;
   323		}
   324	}
   325	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH iwl-next v2 1/5] e1000: drop unnecessary constant casts to u16

[David Laight]

On Tue, 8 Jul 2025 10:16:52 +0200
Jacek Kowalski <jacek@jacekk.info> wrote:

> Remove unnecessary casts of constant values to u16.
> Let the C type system do it's job.
> 
> Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
> Suggested-by: Simon Horman <horms@kernel.org>

Reviewed-by: David Laight <david.laight.linux@gmain.com>

For all the patches, perhaps changing 'unnecessary' to 'pointless'.
All the cast values are immediately promoted to 'signed int' and
then possibly promoted to 'unsigned int' depending of the type of
the other arithmetic operands.

> ---
>  drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 +-
>  drivers/net/ethernet/intel/e1000/e1000_hw.c      | 4 ++--
>  drivers/net/ethernet/intel/e1000/e1000_main.c    | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
> index d06d29c6c037..d152026a027b 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c
> @@ -806,7 +806,7 @@ static int e1000_eeprom_test(struct e1000_adapter *adapter, u64 *data)
>  	}
>  
>  	/* If Checksum is not Correct return error else test passed */
> -	if ((checksum != (u16)EEPROM_SUM) && !(*data))
> +	if ((checksum != EEPROM_SUM) && !(*data))
>  		*data = 2;
>  
>  	return *data;
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_hw.c b/drivers/net/ethernet/intel/e1000/e1000_hw.c
> index f9328f2e669f..0e5de52b1067 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_hw.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_hw.c
> @@ -3970,7 +3970,7 @@ s32 e1000_validate_eeprom_checksum(struct e1000_hw *hw)
>  		return E1000_SUCCESS;
>  
>  #endif
> -	if (checksum == (u16)EEPROM_SUM)
> +	if (checksum == EEPROM_SUM)
>  		return E1000_SUCCESS;
>  	else {
>  		e_dbg("EEPROM Checksum Invalid\n");
> @@ -3997,7 +3997,7 @@ s32 e1000_update_eeprom_checksum(struct e1000_hw *hw)
>  		}
>  		checksum += eeprom_data;
>  	}
> -	checksum = (u16)EEPROM_SUM - checksum;
> +	checksum = EEPROM_SUM - checksum;
>  	if (e1000_write_eeprom(hw, EEPROM_CHECKSUM_REG, 1, &checksum) < 0) {
>  		e_dbg("EEPROM Write Error\n");
>  		return -E1000_ERR_EEPROM;
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
> index d8595e84326d..09acba2ed483 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> @@ -313,7 +313,7 @@ static void e1000_update_mng_vlan(struct e1000_adapter *adapter)
>  		} else {
>  			adapter->mng_vlan_id = E1000_MNG_VLAN_NONE;
>  		}
> -		if ((old_vid != (u16)E1000_MNG_VLAN_NONE) &&
> +		if ((old_vid != E1000_MNG_VLAN_NONE) &&
>  		    (vid != old_vid) &&
>  		    !test_bit(old_vid, adapter->active_vlans))
>  			e1000_vlan_rx_kill_vid(netdev, htons(ETH_P_8021Q),

[PATCH iwl-next v2 2/5] e1000e: drop unnecessary constant casts to u16

[Jacek Kowalski]

Remove unnecessary casts of constant values to u16.
Let the C type system do it's job.

Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
Suggested-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
 drivers/net/ethernet/intel/e1000e/nvm.c     | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
index c0bbb12eed2e..5d8c66253779 100644
--- a/drivers/net/ethernet/intel/e1000e/ethtool.c
+++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
@@ -959,7 +959,7 @@ static int e1000_eeprom_test(struct e1000_adapter *adapter, u64 *data)
 	}
 
 	/* If Checksum is not Correct return error else test passed */
-	if ((checksum != (u16)NVM_SUM) && !(*data))
+	if ((checksum != NVM_SUM) && !(*data))
 		*data = 2;
 
 	return *data;
diff --git a/drivers/net/ethernet/intel/e1000e/nvm.c b/drivers/net/ethernet/intel/e1000e/nvm.c
index 16369e6d245a..4bde1c9de1b9 100644
--- a/drivers/net/ethernet/intel/e1000e/nvm.c
+++ b/drivers/net/ethernet/intel/e1000e/nvm.c
@@ -564,7 +564,7 @@ s32 e1000e_validate_nvm_checksum_generic(struct e1000_hw *hw)
 		return 0;
 	}
 
-	if (checksum != (u16)NVM_SUM) {
+	if (checksum != NVM_SUM) {
 		e_dbg("NVM Checksum Invalid\n");
 		return -E1000_ERR_NVM;
 	}
@@ -594,7 +594,7 @@ s32 e1000e_update_nvm_checksum_generic(struct e1000_hw *hw)
 		}
 		checksum += nvm_data;
 	}
-	checksum = (u16)NVM_SUM - checksum;
+	checksum = NVM_SUM - checksum;
 	ret_val = e1000_write_nvm(hw, NVM_CHECKSUM_REG, 1, &checksum);
 	if (ret_val)
 		e_dbg("NVM Write Error while updating checksum.\n");
-- 
2.47.2

Re: [PATCH iwl-next v2 2/5] e1000e: drop unnecessary constant casts to u16

[Simon Horman]

On Tue, Jul 08, 2025 at 10:17:22AM +0200, Jacek Kowalski wrote:
> Remove unnecessary casts of constant values to u16.
> Let the C type system do it's job.
> 
> Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
> Suggested-by: Simon Horman <horms@kernel.org>

The nit below not withstanding this looks good to me.

Reviewed-by: Simon Horman <horms@kernel.org>

> ---
>  drivers/net/ethernet/intel/e1000e/ethtool.c | 2 +-
>  drivers/net/ethernet/intel/e1000e/nvm.c     | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/ethtool.c b/drivers/net/ethernet/intel/e1000e/ethtool.c
> index c0bbb12eed2e..5d8c66253779 100644
> --- a/drivers/net/ethernet/intel/e1000e/ethtool.c
> +++ b/drivers/net/ethernet/intel/e1000e/ethtool.c
> @@ -959,7 +959,7 @@ static int e1000_eeprom_test(struct e1000_adapter *adapter, u64 *data)
>  	}
>  
>  	/* If Checksum is not Correct return error else test passed */
> -	if ((checksum != (u16)NVM_SUM) && !(*data))
> +	if ((checksum != NVM_SUM) && !(*data))

Unnecessary inner parentheses here too.

...

[PATCH iwl-next v2 3/5] igb: drop unnecessary constant casts to u16

[Jacek Kowalski]

Remove unnecessary casts of constant values to u16.
Let the C type system do it's job.

Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
Suggested-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/intel/igb/e1000_82575.c | 4 ++--
 drivers/net/ethernet/intel/igb/e1000_i210.c  | 2 +-
 drivers/net/ethernet/intel/igb/e1000_nvm.c   | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c b/drivers/net/ethernet/intel/igb/e1000_82575.c
index 64dfc362d1dc..44a85ad749a4 100644
--- a/drivers/net/ethernet/intel/igb/e1000_82575.c
+++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
@@ -2372,7 +2372,7 @@ static s32 igb_validate_nvm_checksum_with_offset(struct e1000_hw *hw,
 		checksum += nvm_data;
 	}
 
-	if (checksum != (u16) NVM_SUM) {
+	if (checksum != NVM_SUM) {
 		hw_dbg("NVM Checksum Invalid\n");
 		ret_val = -E1000_ERR_NVM;
 		goto out;
@@ -2406,7 +2406,7 @@ static s32 igb_update_nvm_checksum_with_offset(struct e1000_hw *hw, u16 offset)
 		}
 		checksum += nvm_data;
 	}
-	checksum = (u16) NVM_SUM - checksum;
+	checksum = NVM_SUM - checksum;
 	ret_val = hw->nvm.ops.write(hw, (NVM_CHECKSUM_REG + offset), 1,
 				&checksum);
 	if (ret_val)
diff --git a/drivers/net/ethernet/intel/igb/e1000_i210.c b/drivers/net/ethernet/intel/igb/e1000_i210.c
index 503b239868e8..9db29b231d6a 100644
--- a/drivers/net/ethernet/intel/igb/e1000_i210.c
+++ b/drivers/net/ethernet/intel/igb/e1000_i210.c
@@ -602,7 +602,7 @@ static s32 igb_update_nvm_checksum_i210(struct e1000_hw *hw)
 			}
 			checksum += nvm_data;
 		}
-		checksum = (u16) NVM_SUM - checksum;
+		checksum = NVM_SUM - checksum;
 		ret_val = igb_write_nvm_srwr(hw, NVM_CHECKSUM_REG, 1,
 						&checksum);
 		if (ret_val) {
diff --git a/drivers/net/ethernet/intel/igb/e1000_nvm.c b/drivers/net/ethernet/intel/igb/e1000_nvm.c
index 2dcd64d6dec3..c8638502c2be 100644
--- a/drivers/net/ethernet/intel/igb/e1000_nvm.c
+++ b/drivers/net/ethernet/intel/igb/e1000_nvm.c
@@ -636,7 +636,7 @@ s32 igb_validate_nvm_checksum(struct e1000_hw *hw)
 		checksum += nvm_data;
 	}
 
-	if (checksum != (u16) NVM_SUM) {
+	if (checksum != NVM_SUM) {
 		hw_dbg("NVM Checksum Invalid\n");
 		ret_val = -E1000_ERR_NVM;
 		goto out;
@@ -668,7 +668,7 @@ s32 igb_update_nvm_checksum(struct e1000_hw *hw)
 		}
 		checksum += nvm_data;
 	}
-	checksum = (u16) NVM_SUM - checksum;
+	checksum = NVM_SUM - checksum;
 	ret_val = hw->nvm.ops.write(hw, NVM_CHECKSUM_REG, 1, &checksum);
 	if (ret_val)
 		hw_dbg("NVM Write Error while updating checksum.\n");
-- 
2.47.2

Re: [PATCH iwl-next v2 3/5] igb: drop unnecessary constant casts to u16

[Simon Horman]

On Tue, Jul 08, 2025 at 10:17:49AM +0200, Jacek Kowalski wrote:
> Remove unnecessary casts of constant values to u16.
> Let the C type system do it's job.
> 
> Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
> Suggested-by: Simon Horman <horms@kernel.org>

Reviewed-by: Simon Horman <horms@kernel.org>

[PATCH iwl-next v2 4/5] igc: drop unnecessary constant casts to u16

[Jacek Kowalski]

Remove unnecessary casts of constant values to u16.
Let the C type system do it's job.

Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
Suggested-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/intel/igc/igc_i225.c | 2 +-
 drivers/net/ethernet/intel/igc/igc_nvm.c  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_i225.c b/drivers/net/ethernet/intel/igc/igc_i225.c
index 0dd61719f1ed..5226d10cc95b 100644
--- a/drivers/net/ethernet/intel/igc/igc_i225.c
+++ b/drivers/net/ethernet/intel/igc/igc_i225.c
@@ -435,7 +435,7 @@ static s32 igc_update_nvm_checksum_i225(struct igc_hw *hw)
 		}
 		checksum += nvm_data;
 	}
-	checksum = (u16)NVM_SUM - checksum;
+	checksum = NVM_SUM - checksum;
 	ret_val = igc_write_nvm_srwr(hw, NVM_CHECKSUM_REG, 1,
 				     &checksum);
 	if (ret_val) {
diff --git a/drivers/net/ethernet/intel/igc/igc_nvm.c b/drivers/net/ethernet/intel/igc/igc_nvm.c
index efd121c03967..a47b8d39238c 100644
--- a/drivers/net/ethernet/intel/igc/igc_nvm.c
+++ b/drivers/net/ethernet/intel/igc/igc_nvm.c
@@ -123,7 +123,7 @@ s32 igc_validate_nvm_checksum(struct igc_hw *hw)
 		checksum += nvm_data;
 	}
 
-	if (checksum != (u16)NVM_SUM) {
+	if (checksum != NVM_SUM) {
 		hw_dbg("NVM Checksum Invalid\n");
 		ret_val = -IGC_ERR_NVM;
 		goto out;
@@ -155,7 +155,7 @@ s32 igc_update_nvm_checksum(struct igc_hw *hw)
 		}
 		checksum += nvm_data;
 	}
-	checksum = (u16)NVM_SUM - checksum;
+	checksum = NVM_SUM - checksum;
 	ret_val = hw->nvm.ops.write(hw, NVM_CHECKSUM_REG, 1, &checksum);
 	if (ret_val)
 		hw_dbg("NVM Write Error while updating checksum.\n");
-- 
2.47.2

Re: [PATCH iwl-next v2 4/5] igc: drop unnecessary constant casts to u16

[Simon Horman]

On Tue, Jul 08, 2025 at 10:18:10AM +0200, Jacek Kowalski wrote:
> Remove unnecessary casts of constant values to u16.
> Let the C type system do it's job.
> 
> Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
> Suggested-by: Simon Horman <horms@kernel.org>

Reviewed-by: Simon Horman <horms@kernel.org>

...

[PATCH iwl-next v2 5/5] ixgbe: drop unnecessary constant casts to u16

[Jacek Kowalski]

Remove unnecessary casts of constant values to u16.
Let the C type system do it's job.

Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
Suggested-by: Simon Horman <horms@kernel.org>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_common.c | 2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c   | 2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
index 4ff19426ab74..cb28c26e12f2 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
@@ -1739,7 +1739,7 @@ int ixgbe_calc_eeprom_checksum_generic(struct ixgbe_hw *hw)
 		}
 	}
 
-	checksum = (u16)IXGBE_EEPROM_SUM - checksum;
+	checksum = IXGBE_EEPROM_SUM - checksum;
 
 	return (int)checksum;
 }
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
index c2353aed0120..07c4a42ea282 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
@@ -373,7 +373,7 @@ static int ixgbe_calc_eeprom_checksum_X540(struct ixgbe_hw *hw)
 		}
 	}
 
-	checksum = (u16)IXGBE_EEPROM_SUM - checksum;
+	checksum = IXGBE_EEPROM_SUM - checksum;
 
 	return (int)checksum;
 }
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
index bfa647086c70..0cc80ce8fcdc 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
@@ -1060,7 +1060,7 @@ static int ixgbe_calc_checksum_X550(struct ixgbe_hw *hw, u16 *buffer,
 			return status;
 	}
 
-	checksum = (u16)IXGBE_EEPROM_SUM - checksum;
+	checksum = IXGBE_EEPROM_SUM - checksum;
 
 	return (int)checksum;
 }
-- 
2.47.2

RE: [Intel-wired-lan] [PATCH iwl-next v2 5/5] ixgbe: drop unnecessary constant casts to u16

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf
> Of Jacek Kowalski
> Sent: Tuesday, July 8, 2025 10:19 AM
> To: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Kitszel,
> Przemyslaw <przemyslaw.kitszel@intel.com>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; Simon Horman <horms@kernel.org>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org
> Subject: [Intel-wired-lan] [PATCH iwl-next v2 5/5] ixgbe: drop
> unnecessary constant casts to u16
> 
> Remove unnecessary casts of constant values to u16.
> Let the C type system do it's job.
> 
> Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
> Suggested-by: Simon Horman <horms@kernel.org>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_common.c | 2 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c   | 2 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c   | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> index 4ff19426ab74..cb28c26e12f2 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> @@ -1739,7 +1739,7 @@ int ixgbe_calc_eeprom_checksum_generic(struct
> ixgbe_hw *hw)
>  		}
>  	}
> 
> -	checksum = (u16)IXGBE_EEPROM_SUM - checksum;
> +	checksum = IXGBE_EEPROM_SUM - checksum;
> 
Can't lead to different results, especially when:
checksum > IXGBE_EEPROM_SUM → the result becomes negative in int, and narrowing to u16 causes unexpected wraparound?

With this patch you are changing the semantics of the code - from explicit 16bit arithmetic to full int implicit promotion which can be error-prone or compiler-dependent /* for different targets */.

>  	return (int)checksum;
>  }
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
> index c2353aed0120..07c4a42ea282 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c
> @@ -373,7 +373,7 @@ static int ixgbe_calc_eeprom_checksum_X540(struct
> ixgbe_hw *hw)
>  		}
>  	}
> 
> -	checksum = (u16)IXGBE_EEPROM_SUM - checksum;
> +	checksum = IXGBE_EEPROM_SUM - checksum;
> 
>  	return (int)checksum;
>  }
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
> index bfa647086c70..0cc80ce8fcdc 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c
> @@ -1060,7 +1060,7 @@ static int ixgbe_calc_checksum_X550(struct
> ixgbe_hw *hw, u16 *buffer,
>  			return status;
>  	}
> 
> -	checksum = (u16)IXGBE_EEPROM_SUM - checksum;
> +	checksum = IXGBE_EEPROM_SUM - checksum;
> 
>  	return (int)checksum;
>  }
> --
> 2.47.2

Re: [Intel-wired-lan] [PATCH iwl-next v2 5/5] ixgbe: drop unnecessary constant casts to u16

[Jacek Kowalski]

>> -	checksum = (u16)IXGBE_EEPROM_SUM - checksum;
>> +	checksum = IXGBE_EEPROM_SUM - checksum;
>>
> Can't lead to different results, especially when:
> checksum > IXGBE_EEPROM_SUM → the result becomes negative in int, and narrowing to u16 causes unexpected wraparound?
> 
> With this patch you are changing the semantics of the code - from explicit 16bit arithmetic to full int implicit promotion which can be error-prone or compiler-dependent /* for different targets */.


As far as I understand the C language does this by design - in the terms of C specification:

> If an int can represent all values of the original type (...), the value is converted to an int; otherwise, it is converted to an unsigned int. These are called the integer promotions. (see note)
> 
> (note) The integer promotions are applied only: as part of the usual arithmetic conversions, (...)


And subtraction semantics are:

> If both operands have arithmetic type, the usual arithmetic conversions are performed on them.



So there is no *16 bit arithmetic* - it is always done on integers (usually 32 bits).

Or have I missed something?


Additionally I've checked AMD64, ARM and MIPS assembly output from GCC and clang on https://godbolt.org/z/GPsMxrWfe - both of the following snippets compile to exactly the same assembly:

#define NVM_SUM 0xBABA
int test(int num) {
    volatile unsigned short test = 0xFFFF;
    unsigned short checksum = NVM_SUM - test;
    return checksum;
}

vs.:

#define NVM_SUM 0xBABA
int test(int num) {
    volatile unsigned short test = 0xFFFF;
    unsigned short checksum = ((unsigned short)NVM_SUM) - test;
    return checksum;
}

-- 
Best regards,
  Jacek Kowalski

RE: [Intel-wired-lan] [PATCH iwl-next v2 5/5] ixgbe: drop unnecessary constant casts to u16

[Loktionov, Aleksandr]

> -----Original Message-----
> From: Jacek Kowalski <jacek@jacekk.info>
> Sent: Tuesday, July 8, 2025 11:35 AM
> To: Loktionov, Aleksandr <aleksandr.loktionov@intel.com>; Nguyen,
> Anthony L <anthony.l.nguyen@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Andrew Lunn <andrew+netdev@lunn.ch>;
> David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Simon Horman <horms@kernel.org>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org
> Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2 5/5] ixgbe: drop
> unnecessary constant casts to u16
> 
> >> -	checksum = (u16)IXGBE_EEPROM_SUM - checksum;
> >> +	checksum = IXGBE_EEPROM_SUM - checksum;
> >>
> > Can't lead to different results, especially when:
> > checksum > IXGBE_EEPROM_SUM → the result becomes negative in int,
> and narrowing to u16 causes unexpected wraparound?
> >
> > With this patch you are changing the semantics of the code - from
> explicit 16bit arithmetic to full int implicit promotion which can be
> error-prone or compiler-dependent /* for different targets */.
> 
> 
> As far as I understand the C language does this by design - in the
> terms of C specification:
> 
> > If an int can represent all values of the original type (...), the
> value is converted to an int; otherwise, it is converted to an
> unsigned int. These are called the integer promotions. (see note)
> >
> > (note) The integer promotions are applied only: as part of the usual
> arithmetic conversions, (...)
> 
> 
> And subtraction semantics are:
> 
> > If both operands have arithmetic type, the usual arithmetic
> conversions are performed on them.
> 
> 
> 
> So there is no *16 bit arithmetic* - it is always done on integers
> (usually 32 bits).
> 
> Or have I missed something?
> 
> 
> Additionally I've checked AMD64, ARM and MIPS assembly output from GCC
> and clang on https://godbolt.org/z/GPsMxrWfe - both of the following
> snippets compile to exactly the same assembly:
> 
> #define NVM_SUM 0xBABA
> int test(int num) {
>     volatile unsigned short test = 0xFFFF;
>     unsigned short checksum = NVM_SUM - test;
>     return checksum;
> }
> 
> vs.:
> 
> #define NVM_SUM 0xBABA
> int test(int num) {
>     volatile unsigned short test = 0xFFFF;
>     unsigned short checksum = ((unsigned short)NVM_SUM) - test;
>     return checksum;
> }
> 
> --
> Best regards,
>   Jacek Kowalski

Thank you for the tests, I've copy pasted into Godbolt and' verified a doesn't of targets, the code is identical got gcc.
So, the change looks scary for the first glance, but GCC actually handles it the same way.

Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>

Re: [Intel-wired-lan] [PATCH iwl-next v2 5/5] ixgbe: drop unnecessary constant casts to u16

[Jacek Kowalski]

> So, the change looks scary for the first glance, but GCC actually
> handles it the same way.

Basically if there are differences, it would be a compiler bug
due to violation of C language specification.

-- 
Best regards,
  Jacek Kowalski

Re: [PATCH iwl-next v2 5/5] ixgbe: drop unnecessary constant casts to u16

[Simon Horman]

On Tue, Jul 08, 2025 at 10:18:35AM +0200, Jacek Kowalski wrote:
> Remove unnecessary casts of constant values to u16.
> Let the C type system do it's job.
> 
> Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
> Suggested-by: Simon Horman <horms@kernel.org>

Reviewed-by: Simon Horman <horms@kernel.org>