netdev - Re: [PATCH iwl-next v1] ixgbe: reduce number of reads when getting OROM data

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] [day] [month] [year] [list]

Message-ID: <20250801112125.GO8494@horms.kernel.org>
Date: Fri, 1 Aug 2025 12:21:25 +0100
From: Simon Horman <horms@...nel.org>
To: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, anthony.l.nguyen@...el.com,
	netdev@...r.kernel.org,
	Aleksandr Loktionov <aleksandr.loktionov@...el.com>
Subject: Re: [PATCH iwl-next v1] ixgbe: reduce number of reads when getting
 OROM data

On Thu, Jul 31, 2025 at 02:50:25PM +0200, Jedrzej Jagielski wrote:
> Currently, during locating the CIVD section, the ixgbe driver loops
> over the OROM area and at each iteration reads only OROM-datastruct-size
> amount of data. This results in many small reads and is inefficient.
> 
> Optimize this by reading the entire OROM bank into memory once before
> entering the loop. This significantly reduces the probing time.
> 
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@...el.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>

Thanks, nits below not withstanding, this looks good to me.

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

> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 58 +++++++++++++------
>  1 file changed, 39 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> index 87b03c1992a8..048b2aae155a 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c
> @@ -3006,50 +3006,70 @@ static int ixgbe_get_nvm_srev(struct ixgbe_hw *hw,
>   * Searches through the Option ROM flash contents to locate the CIVD data for
>   * the image.
>   *
> - * Return: the exit code of the operation.
> + * Return: -ENOMEM when cannot allocate memory, -EDOM for checksum violation,
> + *	   -ENODATA when cannot find proper data, -EIO for faulty read or
> + *	   0 on success.
> + *
> + *	   On success @civd stores collected data.
>   */
>  static int
>  ixgbe_get_orom_civd_data(struct ixgbe_hw *hw, enum ixgbe_bank_select bank,
>  			 struct ixgbe_orom_civd_info *civd)
>  {
> -	struct ixgbe_orom_civd_info tmp;
> +	u32 orom_size = hw->flash.banks.orom_size;
> +	u8 *orom_data;
>  	u32 offset;
>  	int err;
>  
> +	orom_data = kzalloc(orom_size, GFP_KERNEL);
> +	if (!orom_data)
> +		return -ENOMEM;
> +
> +	err = ixgbe_read_flash_module(hw, bank,
> +				      IXGBE_E610_SR_1ST_OROM_BANK_PTR, 0,
> +				      orom_data, orom_size);
> +	if (err) {
> +		err = -EIO;
> +		goto cleanup;
> +	}
> +
>  	/* The CIVD section is located in the Option ROM aligned to 512 bytes.
>  	 * The first 4 bytes must contain the ASCII characters "$CIV".
>  	 * A simple modulo 256 sum of all of the bytes of the structure must
>  	 * equal 0.
>  	 */
> -	for (offset = 0; (offset + SZ_512) <= hw->flash.banks.orom_size;
> -	     offset += SZ_512) {
> +	for (offset = 0; (offset + SZ_512) <= orom_size; offset += SZ_512) {

nit: while we are here the inner parentheses could be removed

> +		struct ixgbe_orom_civd_info *tmp;
>  		u8 sum = 0;
>  		u32 i;
>  
> -		err = ixgbe_read_flash_module(hw, bank,
> -					      IXGBE_E610_SR_1ST_OROM_BANK_PTR,
> -					      offset,
> -					      (u8 *)&tmp, sizeof(tmp));
> -		if (err)
> -			return err;
> +		BUILD_BUG_ON(sizeof(*tmp) > SZ_512);
> +
> +		tmp = (struct ixgbe_orom_civd_info *)&orom_data[offset];
>  
>  		/* Skip forward until we find a matching signature */
> -		if (memcmp(IXGBE_OROM_CIV_SIGNATURE, tmp.signature,
> -			   sizeof(tmp.signature)))
> +		if (memcmp(IXGBE_OROM_CIV_SIGNATURE, tmp->signature,
> +			   sizeof(tmp->signature)))
>  			continue;
>  
>  		/* Verify that the simple checksum is zero */
> -		for (i = 0; i < sizeof(tmp); i++)
> -			sum += ((u8 *)&tmp)[i];
> +		for (i = 0; i < sizeof(*tmp); i++)
> +			sum += ((u8 *)tmp)[i];
>  
> -		if (sum)
> -			return -EDOM;
> +		if (sum) {
> +			err = -EDOM;
> +			goto cleanup;
> +		}
>  
> -		*civd = tmp;
> -		return 0;
> +		*civd = *tmp;
> +		err = 0;
> +		goto cleanup;

nit: maybe it's just me, but break feels more natural here.

>  	}
>  
> -	return -ENODATA;
> +	err = -ENODATA;
> +cleanup:
> +	kfree(orom_data);
> +	return err;
>  }
>  
>  /**
> -- 
> 2.31.1
> 
>