* Re: [Intel-wired-lan] [PATCH iwl-next v1] ixgbe: reduce number of reads when getting OROM data
2025-07-31 12:50 [PATCH iwl-next v1] ixgbe: reduce number of reads when getting OROM data Jedrzej Jagielski
@ 2025-07-31 17:02 ` Jacob Keller
2025-08-01 11:21 ` Simon Horman
2025-08-02 11:17 ` [Intel-wired-lan] " Paul Menzel
2 siblings, 0 replies; 4+ messages in thread
From: Jacob Keller @ 2025-07-31 17:02 UTC (permalink / raw)
To: Jedrzej Jagielski, intel-wired-lan
Cc: anthony.l.nguyen, netdev, Aleksandr Loktionov
[-- Attachment #1.1: Type: text/plain, Size: 3715 bytes --]
On 7/31/2025 5:50 AM, 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@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> ---
Nice. I recall doing something similar in ice.
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> 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) {
> + 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;
> }
>
> - return -ENODATA;
> + err = -ENODATA;
> +cleanup:
> + kfree(orom_data);
> + return err;
> }
>
> /**
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH iwl-next v1] ixgbe: reduce number of reads when getting OROM data
2025-07-31 12:50 [PATCH iwl-next v1] ixgbe: reduce number of reads when getting OROM data Jedrzej Jagielski
2025-07-31 17:02 ` [Intel-wired-lan] " Jacob Keller
@ 2025-08-01 11:21 ` Simon Horman
2025-08-02 11:17 ` [Intel-wired-lan] " Paul Menzel
2 siblings, 0 replies; 4+ messages in thread
From: Simon Horman @ 2025-08-01 11:21 UTC (permalink / raw)
To: Jedrzej Jagielski
Cc: intel-wired-lan, anthony.l.nguyen, netdev, Aleksandr Loktionov
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@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
Thanks, nits below not withstanding, this looks good to me.
Reviewed-by: Simon Horman <horms@kernel.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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-next v1] ixgbe: reduce number of reads when getting OROM data
2025-07-31 12:50 [PATCH iwl-next v1] ixgbe: reduce number of reads when getting OROM data Jedrzej Jagielski
2025-07-31 17:02 ` [Intel-wired-lan] " Jacob Keller
2025-08-01 11:21 ` Simon Horman
@ 2025-08-02 11:17 ` Paul Menzel
2 siblings, 0 replies; 4+ messages in thread
From: Paul Menzel @ 2025-08-02 11:17 UTC (permalink / raw)
To: Jedrzej Jagielski
Cc: intel-wired-lan, anthony.l.nguyen, netdev, Aleksandr Loktionov
Dear Jedrzej,
Thank you for your patch.
Am 31.07.25 um 14:50 schrieb Jedrzej Jagielski:
> 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.
Awesome. For posterity, could you please add the measurements without
and with your patch to the commit message.
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> ---
> 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) {
> + 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;
> }
>
> - return -ENODATA;
> + err = -ENODATA;
> +cleanup:
> + kfree(orom_data);
> + return err;
> }
>
> /**
The diff looks good. With the commit message amended:
Reviewed-by: Paul Menzel <pmenzel@molgen.mpg.de>
Kind regards,
Paul
^ permalink raw reply [flat|nested] 4+ messages in thread