netdev - Re: [PATCH iwl-next v2] ixgbe: Convert ret val type from s32 to int

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5221bfaf-31bf-630f-9036-284c83c3eb76@intel.com>
Date: Fri, 5 Jan 2024 13:49:42 -0800
From: Tony Nguyen <anthony.l.nguyen@...el.com>
To: Jedrzej Jagielski <jedrzej.jagielski@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>
CC: <netdev@...r.kernel.org>, Jacob Keller <jacob.e.keller@...el.com>
Subject: Re: [PATCH iwl-next v2] ixgbe: Convert ret val type from s32 to int



On 1/5/2024 2:31 AM, Jedrzej Jagielski wrote:
> Currently big amount of the functions returning standard error codes
> are of type s32. Convert them to regular ints as typdefs here are
> not necessary to return standard error codes.
> 
> Suggested-by: Jacob Keller <jacob.e.keller@...el.com>
> Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
> ---
> v2: fix smatch warnings,

These changes aren't mentioned in the commit message, however, it should 
probably be in it's own commit to keep the changes separate and easier 
to see/review (since they pre-existed this patch).

Also, not seeing changes/fixes to the second warning [1]

New smatch warnings:
...
drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c:3130 
ixgbe_enter_lplu_t_x550em() warn: missing error code? 'status'

> alter the commit msg
> ---

...

> @@ -93,11 +93,11 @@ static s32 ixgbe_get_invariants_82598(struct ixgbe_hw *hw)
>    *  not known.  Perform the SFP init if necessary.
>    *
>    **/
> -static s32 ixgbe_init_phy_ops_82598(struct ixgbe_hw *hw)
> +static int ixgbe_init_phy_ops_82598(struct ixgbe_hw *hw)
>   {
>   	struct ixgbe_mac_info *mac = &hw->mac;
>   	struct ixgbe_phy_info *phy = &hw->phy;
> -	s32 ret_val;
> +	int ret_val;
>   	u16 list_offset, data_offset;

Should we RCT what we're touching? (many other instances in the patch as 
well)

...

> @@ -1866,9 +1866,9 @@ static s32 ixgbe_enable_rx_dma_82599(struct ixgbe_hw *hw, u32 regval)
>    *  Returns IXGBE_ERR_EEPROM_VERSION if the FW is not present or
>    *  if the FW version is not supported.
>    **/
> -static s32 ixgbe_verify_fw_version_82599(struct ixgbe_hw *hw)
> +static int ixgbe_verify_fw_version_82599(struct ixgbe_hw *hw)
>   {
> -	s32 status = IXGBE_ERR_EEPROM_VERSION;
> +	int status = IXGBE_ERR_EEPROM_VERSION;

Does IXGBE_ERR_EEPROM_VERSION exist anymore after your other patch? [2]

>   	u16 fw_offset, fw_ptp_cfg_offset;
>   	u16 offset;
>   	u16 fw_version = 0;

...

> @@ -2062,12 +2062,12 @@ static s32 ixgbe_reset_pipeline_82599(struct ixgbe_hw *hw)
>    *  Performs byte read operation to SFP module's EEPROM over I2C interface at
>    *  a specified device address.
>    **/
> -static s32 ixgbe_read_i2c_byte_82599(struct ixgbe_hw *hw, u8 byte_offset,
> +static int ixgbe_read_i2c_byte_82599(struct ixgbe_hw *hw, u8 byte_offset,
>   				     u8 dev_addr, u8 *data)
>   {
>   	u32 esdp;
> -	s32 status;
> -	s32 timeout = 200;
> +	int status;
> +	int timeout = 200;

unrelated change (at least a few others in this patch as well)

>   
>   	if (hw->phy.qsfp_shared_i2c_bus == true) {
>   		/* Acquire I2C bus ownership. */
> @@ -2116,12 +2116,12 @@ static s32 ixgbe_read_i2c_byte_82599(struct ixgbe_hw *hw, u8 byte_offset,
>    *  Performs byte write operation to SFP module's EEPROM over I2C interface at
>    *  a specified device address.
>    **/
> -static s32 ixgbe_write_i2c_byte_82599(struct ixgbe_hw *hw, u8 byte_offset,
> +static int ixgbe_write_i2c_byte_82599(struct ixgbe_hw *hw, u8 byte_offset,
>   				      u8 dev_addr, u8 data)
>   {
>   	u32 esdp;
> -	s32 status;
> -	s32 timeout = 200;
> +	int status;
> +	int timeout = 200;
>   
>   	if (hw->phy.qsfp_shared_i2c_bus == true) {
>   		/* Acquire I2C bus ownership. */

Thanks,
Tony

[1] 
https://lore.kernel.org/intel-wired-lan/08d8b75e-af80-438b-8006-9121b8444f49@moroto.mountain/
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit?id=5795f533f30a80aa0473652876296ebc9129e33a