Re: [Intel-wired-lan] [PATCH iwl-next v2 5/5] ixgbe: drop unnecessary constant casts to u16
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 > Suggested-by: Simon Horman Reviewed-by: Simon Horman
Re: [Intel-wired-lan] [PATCH iwl-next v2 5/5] ixgbe: drop unnecessary constant casts to u16
> 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: [Intel-wired-lan] [PATCH iwl-next v2 5/5] ixgbe: drop unnecessary constant casts to u16
> -Original Message- > From: Jacek Kowalski > Sent: Tuesday, July 8, 2025 11:35 AM > To: Loktionov, Aleksandr ; Nguyen, > Anthony L ; Kitszel, Przemyslaw > ; Andrew Lunn ; > David S. Miller ; Eric Dumazet > ; Jakub Kicinski ; Paolo Abeni > ; Simon Horman > Cc: [email protected]; [email protected] > 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 = 0x; > unsigned short checksum = NVM_SUM - test; > return checksum; > } > > vs.: > > #define NVM_SUM 0xBABA > int test(int num) { > volatile unsigned short test = 0x; > 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
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 = 0x;
unsigned short checksum = NVM_SUM - test;
return checksum;
}
vs.:
#define NVM_SUM 0xBABA
int test(int num) {
volatile unsigned short test = 0x;
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
> -Original Message- > From: Intel-wired-lan On Behalf > Of Jacek Kowalski > Sent: Tuesday, July 8, 2025 10:19 AM > To: Nguyen, Anthony L ; Kitszel, > Przemyslaw ; Andrew Lunn > ; David S. Miller ; Eric > Dumazet ; Jakub Kicinski ; Paolo > Abeni ; Simon Horman > Cc: [email protected]; [email protected] > 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 > Suggested-by: Simon Horman > --- > 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
