[iwl-net] ice: fix reads from NVM Shadow RAM on E830 and E825-C devices

The ice driver reads data from the Shadow RAM portion of the NVM during
initialization, including data used to identify the NVM image and device,
such as the ETRACK ID used to populate devlink dev info fw.bundle.

Currently it is using a fixed offset defined by ICE_CSS_HEADER_LENGTH to
compute the appropriate offset. This worked fine for E810 and E822 devices
which both have CSS header length of 330 words.

Other devices, including both E825-C and E830 devices have different sizes
for their CSS header. The use of a hard coded value results in the driver
reading from the wrong block in the NVM when attempting to access the
Shadow RAM copy. This results in the driver reporting the fw.bundle as 0x0
in both the devlink dev info and ethtool -i output.

The first E830 support was introduced by commit ba20ecb1d1bb ("ice: Hook up
4 E830 devices by adding their IDs") and the first E825-C support was
introducted by commit f64e18944233 ("ice: introduce new E825C devices
family")

The NVM actually contains the CSS header length embedded in it. Remove the
hard coded value and replace it with logic to read the length from the NVM
directly. This is more resilient against all existing and future hardware,
vs looking up the expected values from a table. It ensures the driver will
read from the appropriate place when determining the ETRACK ID value used
for populating the fw.bundle_id and for reporting in ethtool -i.

The CSS header length for both the active and inactive flash bank is stored
in the ice_bank_info structure to avoid unnecessary duplicate work when
accessing multiple words of the Shadow RAM. Both banks are read in the
unlikely event that the header length is different for the NVM in the
inactive bank, rather than being different only by the overall device
family.

Fixes: ba20ecb1d1bb ("ice: Hook up 4 E830 devices by adding their IDs")
Co-developed-by: Paul Greenwalt <paul.greenwalt@intel.com>
Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_nvm.c  | 88 ++++++++++++++++++++++++++++++-
 drivers/net/ethernet/intel/ice/ice_type.h | 14 +++--
 2 files changed, 93 insertions(+), 9 deletions(-)


---
base-commit: e4a87abf588536d1cdfb128595e6e680af5cf3ed
change-id: 20240516-iwl-net-2024-05-16-fix-css-hdr-len-9d2319a4f7ed

Best regards,

Dear Jacob, dear Paul,


Thank you for the patch.

Am 20.05.24 um 23:39 schrieb Jacob Keller:
> The ice driver reads data from the Shadow RAM portion of the NVM during
> initialization, including data used to identify the NVM image and device,
> such as the ETRACK ID used to populate devlink dev info fw.bundle.
> 
> Currently it is using a fixed offset defined by ICE_CSS_HEADER_LENGTH to
> compute the appropriate offset. This worked fine for E810 and E822 devices
> which both have CSS header length of 330 words.
> 
> Other devices, including both E825-C and E830 devices have different sizes
> for their CSS header. The use of a hard coded value results in the driver
> reading from the wrong block in the NVM when attempting to access the
> Shadow RAM copy. This results in the driver reporting the fw.bundle as 0x0
> in both the devlink dev info and ethtool -i output.
> 
> The first E830 support was introduced by commit ba20ecb1d1bb ("ice: Hook up
> 4 E830 devices by adding their IDs") and the first E825-C support was
> introducted by commit f64e18944233 ("ice: introduce new E825C devices

introduced

> family")
> 
> The NVM actually contains the CSS header length embedded in it. Remove the
> hard coded value and replace it with logic to read the length from the NVM
> directly. This is more resilient against all existing and future hardware,
> vs looking up the expected values from a table. It ensures the driver will
> read from the appropriate place when determining the ETRACK ID value used
> for populating the fw.bundle_id and for reporting in ethtool -i.
> 
> The CSS header length for both the active and inactive flash bank is stored
> in the ice_bank_info structure to avoid unnecessary duplicate work when
> accessing multiple words of the Shadow RAM. Both banks are read in the
> unlikely event that the header length is different for the NVM in the
> inactive bank, rather than being different only by the overall device
> family.
> 
> Fixes: ba20ecb1d1bb ("ice: Hook up 4 E830 devices by adding their IDs")
> Co-developed-by: Paul Greenwalt <paul.greenwalt@intel.com>
> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_nvm.c  | 88 ++++++++++++++++++++++++++++++-
>   drivers/net/ethernet/intel/ice/ice_type.h | 14 +++--
>   2 files changed, 93 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
> index 84eab92dc03c..5968011e8c7e 100644
> --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
> @@ -374,11 +374,25 @@ ice_read_nvm_module(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u1
>    *
>    * Read the specified word from the copy of the Shadow RAM found in the
>    * specified NVM module.
> + *
> + * Note that the Shadow RAM copy is always located after the CSS header, and
> + * is aligned to 64-byte (32-word) offsets.
>    */
>   static int
>   ice_read_nvm_sr_copy(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u16 *data)
>   {
> -	return ice_read_nvm_module(hw, bank, ICE_NVM_SR_COPY_WORD_OFFSET + offset, data);
> +	u32 sr_copy;
> +
> +	switch (bank) {
> +	case ICE_ACTIVE_FLASH_BANK:
> +		sr_copy = roundup(hw->flash.banks.active_css_hdr_len, 32);
> +		break;
> +	case ICE_INACTIVE_FLASH_BANK:
> +		sr_copy = roundup(hw->flash.banks.inactive_css_hdr_len, 32);
> +		break;
> +	}
> +
> +	return ice_read_nvm_module(hw, bank, sr_copy + offset, data);
>   }
>   
>   /**
> @@ -1009,6 +1023,72 @@ static int ice_determine_active_flash_banks(struct ice_hw *hw)
>   	return 0;
>   }
>   
> +/**
> + * ice_get_nvm_css_hdr_len - Read the CSS header length from the NVM CSS header
> + * @hw: pointer to the HW struct
> + * @bank: whether to read from the active or inactive flash bank
> + * @hdr_len: storage for header length in words
> + *
> + * Read the CSS header length from the NVM CSS header and add the Authentication
> + * header size, and then convert to words.
> + *
> + * Return: zero on success, or a negative error code on failure.
> + */
> +static int
> +ice_get_nvm_css_hdr_len(struct ice_hw *hw, enum ice_bank_select bank,
> +			u32 *hdr_len)
> +{
> +	u16 hdr_len_l, hdr_len_h;
> +	u32 hdr_len_dword;
> +	int status;
> +
> +	status = ice_read_nvm_module(hw, bank, ICE_NVM_CSS_HDR_LEN_L,
> +				     &hdr_len_l);
> +	if (status)
> +		return status;
> +
> +	status = ice_read_nvm_module(hw, bank, ICE_NVM_CSS_HDR_LEN_H,
> +				     &hdr_len_h);
> +	if (status)
> +		return status;
> +
> +	/* CSS header length is in DWORD, so convert to words and add
> +	 * authentication header size
> +	 */
> +	hdr_len_dword = hdr_len_h << 16 | hdr_len_l;
> +	*hdr_len = (hdr_len_dword * 2) + ICE_NVM_AUTH_HEADER_LEN;
> +
> +	return 0;
> +}
> +
> +/**
> + * ice_determine_css_hdr_len - Discover CSS header length for the device
> + * @hw: pointer to the HW struct
> + *
> + * Determine the size of the CSS header at the start of the NVM module. This
> + * is useful for locating the Shadow RAM copy in the NVM, as the Shadow RAM is
> + * always located just after the CSS header.
> + *
> + * Return: zero on success, or a negative error code on failure.
> + */
> +static int ice_determine_css_hdr_len(struct ice_hw *hw)
> +{
> +	struct ice_bank_info *banks = &hw->flash.banks;
> +	int status;
> +
> +	status = ice_get_nvm_css_hdr_len(hw, ICE_ACTIVE_FLASH_BANK,
> +					 &banks->active_css_hdr_len);
> +	if (status)
> +		return status;
> +
> +	status = ice_get_nvm_css_hdr_len(hw, ICE_INACTIVE_FLASH_BANK,
> +					 &banks->inactive_css_hdr_len);
> +	if (status)
> +		return status;
> +
> +	return 0;
> +}
> +
>   /**
>    * ice_init_nvm - initializes NVM setting
>    * @hw: pointer to the HW struct
> @@ -1055,6 +1135,12 @@ int ice_init_nvm(struct ice_hw *hw)
>   		return status;
>   	}
>   
> +	status = ice_determine_css_hdr_len(hw);
> +	if (status) {
> +		ice_debug(hw, ICE_DBG_NVM, "Failed to determine Shadow RAM copy offsets.\n");

As this is a new failure path, should the user be warned about this, if 
it cannot be determined, and NVM possibly be broken?


Kind regards,

Paul


> +		return status;
> +	}
> +
>   	status = ice_get_nvm_ver_info(hw, ICE_ACTIVE_FLASH_BANK, &flash->nvm);
>   	if (status) {
>   		ice_debug(hw, ICE_DBG_INIT, "Failed to read NVM info.\n");
> diff --git a/drivers/net/ethernet/intel/ice/ice_type.h b/drivers/net/ethernet/intel/ice/ice_type.h
> index f0796a93f428..eef397e5baa0 100644
> --- a/drivers/net/ethernet/intel/ice/ice_type.h
> +++ b/drivers/net/ethernet/intel/ice/ice_type.h
> @@ -482,6 +482,8 @@ struct ice_bank_info {
>   	u32 orom_size;				/* Size of OROM bank */
>   	u32 netlist_ptr;			/* Pointer to 1st Netlist bank */
>   	u32 netlist_size;			/* Size of Netlist bank */
> +	u32 active_css_hdr_len;			/* Active CSS header length */
> +	u32 inactive_css_hdr_len;		/* Inactive CSS header length */
>   	enum ice_flash_bank nvm_bank;		/* Active NVM bank */
>   	enum ice_flash_bank orom_bank;		/* Active OROM bank */
>   	enum ice_flash_bank netlist_bank;	/* Active Netlist bank */
> @@ -1087,17 +1089,13 @@ struct ice_aq_get_set_rss_lut_params {
>   #define ICE_SR_SECTOR_SIZE_IN_WORDS	0x800
>   
>   /* CSS Header words */
> +#define ICE_NVM_CSS_HDR_LEN_L			0x02
> +#define ICE_NVM_CSS_HDR_LEN_H			0x03
>   #define ICE_NVM_CSS_SREV_L			0x14
>   #define ICE_NVM_CSS_SREV_H			0x15
>   
> -/* Length of CSS header section in words */
> -#define ICE_CSS_HEADER_LENGTH			330
> -
> -/* Offset of Shadow RAM copy in the NVM bank area. */
> -#define ICE_NVM_SR_COPY_WORD_OFFSET		roundup(ICE_CSS_HEADER_LENGTH, 32)
> -
> -/* Size in bytes of Option ROM trailer */
> -#define ICE_NVM_OROM_TRAILER_LENGTH		(2 * ICE_CSS_HEADER_LENGTH)
> +/* Length of Authentication header section in words */
> +#define ICE_NVM_AUTH_HEADER_LEN			0x08
>   
>   /* The Link Topology Netlist section is stored as a series of words. It is
>    * stored in the NVM as a TLV, with the first two words containing the type
> 
> ---
> base-commit: e4a87abf588536d1cdfb128595e6e680af5cf3ed
> change-id: 20240516-iwl-net-2024-05-16-fix-css-hdr-len-9d2319a4f7ed
> 
> Best regards,

On 5/20/2024 10:55 PM, Paul Menzel wrote:
> Dear Jacob, dear Paul,
> 
> 
> Thank you for the patch.
> 
> Am 20.05.24 um 23:39 schrieb Jacob Keller:
>> The ice driver reads data from the Shadow RAM portion of the NVM during
>> initialization, including data used to identify the NVM image and device,
>> such as the ETRACK ID used to populate devlink dev info fw.bundle.
>>
>> Currently it is using a fixed offset defined by ICE_CSS_HEADER_LENGTH to
>> compute the appropriate offset. This worked fine for E810 and E822 devices
>> which both have CSS header length of 330 words.
>>
>> Other devices, including both E825-C and E830 devices have different sizes
>> for their CSS header. The use of a hard coded value results in the driver
>> reading from the wrong block in the NVM when attempting to access the
>> Shadow RAM copy. This results in the driver reporting the fw.bundle as 0x0
>> in both the devlink dev info and ethtool -i output.
>>
>> The first E830 support was introduced by commit ba20ecb1d1bb ("ice: Hook up
>> 4 E830 devices by adding their IDs") and the first E825-C support was
>> introducted by commit f64e18944233 ("ice: introduce new E825C devices
> 
> introduced
> 
>> family")
>>
>> The NVM actually contains the CSS header length embedded in it. Remove the
>> hard coded value and replace it with logic to read the length from the NVM
>> directly. This is more resilient against all existing and future hardware,
>> vs looking up the expected values from a table. It ensures the driver will
>> read from the appropriate place when determining the ETRACK ID value used
>> for populating the fw.bundle_id and for reporting in ethtool -i.
>>
>> The CSS header length for both the active and inactive flash bank is stored
>> in the ice_bank_info structure to avoid unnecessary duplicate work when
>> accessing multiple words of the Shadow RAM. Both banks are read in the
>> unlikely event that the header length is different for the NVM in the
>> inactive bank, rather than being different only by the overall device
>> family.
>>
>> Fixes: ba20ecb1d1bb ("ice: Hook up 4 E830 devices by adding their IDs")
>> Co-developed-by: Paul Greenwalt <paul.greenwalt@intel.com>
>> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> ---
>>   drivers/net/ethernet/intel/ice/ice_nvm.c  | 88 ++++++++++++++++++++++++++++++-
>>   drivers/net/ethernet/intel/ice/ice_type.h | 14 +++--
>>   2 files changed, 93 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
>> index 84eab92dc03c..5968011e8c7e 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
>> @@ -374,11 +374,25 @@ ice_read_nvm_module(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u1
>>    *
>>    * Read the specified word from the copy of the Shadow RAM found in the
>>    * specified NVM module.
>> + *
>> + * Note that the Shadow RAM copy is always located after the CSS header, and
>> + * is aligned to 64-byte (32-word) offsets.
>>    */
>>   static int
>>   ice_read_nvm_sr_copy(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u16 *data)
>>   {
>> -	return ice_read_nvm_module(hw, bank, ICE_NVM_SR_COPY_WORD_OFFSET + offset, data);
>> +	u32 sr_copy;
>> +
>> +	switch (bank) {
>> +	case ICE_ACTIVE_FLASH_BANK:
>> +		sr_copy = roundup(hw->flash.banks.active_css_hdr_len, 32);
>> +		break;
>> +	case ICE_INACTIVE_FLASH_BANK:
>> +		sr_copy = roundup(hw->flash.banks.inactive_css_hdr_len, 32);
>> +		break;
>> +	}
>> +
>> +	return ice_read_nvm_module(hw, bank, sr_copy + offset, data);
>>   }
>>   
>>   /**
>> @@ -1009,6 +1023,72 @@ static int ice_determine_active_flash_banks(struct ice_hw *hw)
>>   	return 0;
>>   }
>>   
>> +/**
>> + * ice_get_nvm_css_hdr_len - Read the CSS header length from the NVM CSS header
>> + * @hw: pointer to the HW struct
>> + * @bank: whether to read from the active or inactive flash bank
>> + * @hdr_len: storage for header length in words
>> + *
>> + * Read the CSS header length from the NVM CSS header and add the Authentication
>> + * header size, and then convert to words.
>> + *
>> + * Return: zero on success, or a negative error code on failure.
>> + */
>> +static int
>> +ice_get_nvm_css_hdr_len(struct ice_hw *hw, enum ice_bank_select bank,
>> +			u32 *hdr_len)
>> +{
>> +	u16 hdr_len_l, hdr_len_h;
>> +	u32 hdr_len_dword;
>> +	int status;
>> +
>> +	status = ice_read_nvm_module(hw, bank, ICE_NVM_CSS_HDR_LEN_L,
>> +				     &hdr_len_l);
>> +	if (status)
>> +		return status;
>> +
>> +	status = ice_read_nvm_module(hw, bank, ICE_NVM_CSS_HDR_LEN_H,
>> +				     &hdr_len_h);
>> +	if (status)
>> +		return status;
>> +
>> +	/* CSS header length is in DWORD, so convert to words and add
>> +	 * authentication header size
>> +	 */
>> +	hdr_len_dword = hdr_len_h << 16 | hdr_len_l;
>> +	*hdr_len = (hdr_len_dword * 2) + ICE_NVM_AUTH_HEADER_LEN;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ice_determine_css_hdr_len - Discover CSS header length for the device
>> + * @hw: pointer to the HW struct
>> + *
>> + * Determine the size of the CSS header at the start of the NVM module. This
>> + * is useful for locating the Shadow RAM copy in the NVM, as the Shadow RAM is
>> + * always located just after the CSS header.
>> + *
>> + * Return: zero on success, or a negative error code on failure.
>> + */
>> +static int ice_determine_css_hdr_len(struct ice_hw *hw)
>> +{
>> +	struct ice_bank_info *banks = &hw->flash.banks;
>> +	int status;
>> +
>> +	status = ice_get_nvm_css_hdr_len(hw, ICE_ACTIVE_FLASH_BANK,
>> +					 &banks->active_css_hdr_len);
>> +	if (status)
>> +		return status;
>> +
>> +	status = ice_get_nvm_css_hdr_len(hw, ICE_INACTIVE_FLASH_BANK,
>> +					 &banks->inactive_css_hdr_len);
>> +	if (status)
>> +		return status;
>> +
>> +	return 0;
>> +}
>> +
>>   /**
>>    * ice_init_nvm - initializes NVM setting
>>    * @hw: pointer to the HW struct
>> @@ -1055,6 +1135,12 @@ int ice_init_nvm(struct ice_hw *hw)
>>   		return status;
>>   	}
>>   
>> +	status = ice_determine_css_hdr_len(hw);
>> +	if (status) {
>> +		ice_debug(hw, ICE_DBG_NVM, "Failed to determine Shadow RAM copy offsets.\n");
> 
> As this is a new failure path, should the user be warned about this, if 
> it cannot be determined, and NVM possibly be broken?
> 
> 
> Kind regards,
> 
> Paul


Possibly. I'm also trying to avoid spamming the log with failure
messages which are more useful for developers who can enable them vs end
users who may not understand.

Dear Jacob,


Am 21.05.24 um 21:27 schrieb Jacob Keller:

> On 5/20/2024 10:55 PM, Paul Menzel wrote:

>> Am 20.05.24 um 23:39 schrieb Jacob Keller:
>>> The ice driver reads data from the Shadow RAM portion of the NVM during
>>> initialization, including data used to identify the NVM image and device,
>>> such as the ETRACK ID used to populate devlink dev info fw.bundle.
>>>
>>> Currently it is using a fixed offset defined by ICE_CSS_HEADER_LENGTH to
>>> compute the appropriate offset. This worked fine for E810 and E822 devices
>>> which both have CSS header length of 330 words.
>>>
>>> Other devices, including both E825-C and E830 devices have different sizes
>>> for their CSS header. The use of a hard coded value results in the driver
>>> reading from the wrong block in the NVM when attempting to access the
>>> Shadow RAM copy. This results in the driver reporting the fw.bundle as 0x0
>>> in both the devlink dev info and ethtool -i output.
>>>
>>> The first E830 support was introduced by commit ba20ecb1d1bb ("ice: Hook up
>>> 4 E830 devices by adding their IDs") and the first E825-C support was
>>> introducted by commit f64e18944233 ("ice: introduce new E825C devices
>>
>> introduced
>>
>>> family")
>>>
>>> The NVM actually contains the CSS header length embedded in it. Remove the
>>> hard coded value and replace it with logic to read the length from the NVM
>>> directly. This is more resilient against all existing and future hardware,
>>> vs looking up the expected values from a table. It ensures the driver will
>>> read from the appropriate place when determining the ETRACK ID value used
>>> for populating the fw.bundle_id and for reporting in ethtool -i.
>>>
>>> The CSS header length for both the active and inactive flash bank is stored
>>> in the ice_bank_info structure to avoid unnecessary duplicate work when
>>> accessing multiple words of the Shadow RAM. Both banks are read in the
>>> unlikely event that the header length is different for the NVM in the
>>> inactive bank, rather than being different only by the overall device
>>> family.
>>>
>>> Fixes: ba20ecb1d1bb ("ice: Hook up 4 E830 devices by adding their IDs")
>>> Co-developed-by: Paul Greenwalt <paul.greenwalt@intel.com>
>>> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
>>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>> ---
>>>    drivers/net/ethernet/intel/ice/ice_nvm.c  | 88 ++++++++++++++++++++++++++++++-
>>>    drivers/net/ethernet/intel/ice/ice_type.h | 14 +++--
>>>    2 files changed, 93 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
>>> index 84eab92dc03c..5968011e8c7e 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
>>> @@ -374,11 +374,25 @@ ice_read_nvm_module(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u1
>>>     *
>>>     * Read the specified word from the copy of the Shadow RAM found in the
>>>     * specified NVM module.
>>> + *
>>> + * Note that the Shadow RAM copy is always located after the CSS header, and
>>> + * is aligned to 64-byte (32-word) offsets.
>>>     */
>>>    static int
>>>    ice_read_nvm_sr_copy(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u16 *data)
>>>    {
>>> -	return ice_read_nvm_module(hw, bank, ICE_NVM_SR_COPY_WORD_OFFSET + offset, data);
>>> +	u32 sr_copy;
>>> +
>>> +	switch (bank) {
>>> +	case ICE_ACTIVE_FLASH_BANK:
>>> +		sr_copy = roundup(hw->flash.banks.active_css_hdr_len, 32);
>>> +		break;
>>> +	case ICE_INACTIVE_FLASH_BANK:
>>> +		sr_copy = roundup(hw->flash.banks.inactive_css_hdr_len, 32);
>>> +		break;
>>> +	}
>>> +
>>> +	return ice_read_nvm_module(hw, bank, sr_copy + offset, data);
>>>    }
>>>    
>>>    /**
>>> @@ -1009,6 +1023,72 @@ static int ice_determine_active_flash_banks(struct ice_hw *hw)
>>>    	return 0;
>>>    }
>>>    
>>> +/**
>>> + * ice_get_nvm_css_hdr_len - Read the CSS header length from the NVM CSS header
>>> + * @hw: pointer to the HW struct
>>> + * @bank: whether to read from the active or inactive flash bank
>>> + * @hdr_len: storage for header length in words
>>> + *
>>> + * Read the CSS header length from the NVM CSS header and add the Authentication
>>> + * header size, and then convert to words.
>>> + *
>>> + * Return: zero on success, or a negative error code on failure.
>>> + */
>>> +static int
>>> +ice_get_nvm_css_hdr_len(struct ice_hw *hw, enum ice_bank_select bank,
>>> +			u32 *hdr_len)
>>> +{
>>> +	u16 hdr_len_l, hdr_len_h;
>>> +	u32 hdr_len_dword;
>>> +	int status;
>>> +
>>> +	status = ice_read_nvm_module(hw, bank, ICE_NVM_CSS_HDR_LEN_L,
>>> +				     &hdr_len_l);
>>> +	if (status)
>>> +		return status;
>>> +
>>> +	status = ice_read_nvm_module(hw, bank, ICE_NVM_CSS_HDR_LEN_H,
>>> +				     &hdr_len_h);
>>> +	if (status)
>>> +		return status;
>>> +
>>> +	/* CSS header length is in DWORD, so convert to words and add
>>> +	 * authentication header size
>>> +	 */
>>> +	hdr_len_dword = hdr_len_h << 16 | hdr_len_l;
>>> +	*hdr_len = (hdr_len_dword * 2) + ICE_NVM_AUTH_HEADER_LEN;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +/**
>>> + * ice_determine_css_hdr_len - Discover CSS header length for the device
>>> + * @hw: pointer to the HW struct
>>> + *
>>> + * Determine the size of the CSS header at the start of the NVM module. This
>>> + * is useful for locating the Shadow RAM copy in the NVM, as the Shadow RAM is
>>> + * always located just after the CSS header.
>>> + *
>>> + * Return: zero on success, or a negative error code on failure.
>>> + */
>>> +static int ice_determine_css_hdr_len(struct ice_hw *hw)
>>> +{
>>> +	struct ice_bank_info *banks = &hw->flash.banks;
>>> +	int status;
>>> +
>>> +	status = ice_get_nvm_css_hdr_len(hw, ICE_ACTIVE_FLASH_BANK,
>>> +					 &banks->active_css_hdr_len);
>>> +	if (status)
>>> +		return status;
>>> +
>>> +	status = ice_get_nvm_css_hdr_len(hw, ICE_INACTIVE_FLASH_BANK,
>>> +					 &banks->inactive_css_hdr_len);
>>> +	if (status)
>>> +		return status;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>    /**
>>>     * ice_init_nvm - initializes NVM setting
>>>     * @hw: pointer to the HW struct
>>> @@ -1055,6 +1135,12 @@ int ice_init_nvm(struct ice_hw *hw)
>>>    		return status;
>>>    	}
>>>    
>>> +	status = ice_determine_css_hdr_len(hw);
>>> +	if (status) {
>>> +		ice_debug(hw, ICE_DBG_NVM, "Failed to determine Shadow RAM copy offsets.\n");
>>
>> As this is a new failure path, should the user be warned about this, if
>> it cannot be determined, and NVM possibly be broken?

> Possibly. I'm also trying to avoid spamming the log with failure
> messages which are more useful for developers who can enable them vs end
> users who may not understand.

I agree that logging too much is also confusing. Excuse my ignorance, 
but what happens if NVM is broken and the offset cannot be determined. 
Will the user get any error message and know what to do (replace the 
device or call support)? Or another view point, is the bug report with 
the Linux log messages included going to have the information, so the 
support folks or developers can pinpoint the problem without replying to 
the user to enable debug messages?


Kind regards,

Paul

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of Jacob Keller
> Sent: Tuesday, May 21, 2024 3:10 AM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>; Greenwalt, Paul <paul.greenwalt@intel.com>
> Subject: [Intel-wired-lan] [PATCH iwl-net] ice: fix reads from NVM Shadow RAM on E830 and E825-C devices
>
> The ice driver reads data from the Shadow RAM portion of the NVM during initialization, including data used to identify the NVM image and device, such as the ETRACK ID used to populate devlink dev info fw.bundle.
>
> Currently it is using a fixed offset defined by ICE_CSS_HEADER_LENGTH to compute the appropriate offset. This worked fine for E810 and E822 devices which both have CSS header length of 330 words.
>
> Other devices, including both E825-C and E830 devices have different sizes for their CSS header. The use of a hard coded value results in the driver reading from the wrong block in the NVM when attempting to access the Shadow RAM copy. This results in the driver reporting the fw.bundle as 0x0 in both the devlink dev info and ethtool -i output.
>
> The first E830 support was introduced by commit ba20ecb1d1bb ("ice: Hook up
> 4 E830 devices by adding their IDs") and the first E825-C support was introducted by commit f64e18944233 ("ice: introduce new E825C devices
 > family")
>
> The NVM actually contains the CSS header length embedded in it. Remove the hard coded value and replace it with logic to read the length from the NVM directly. This is more resilient against all existing and future hardware, vs looking up the expected values from a table. > It ensures the driver will read from the appropriate place when determining the ETRACK ID value used for populating the fw.bundle_id and for reporting in ethtool -i.
>
> The CSS header length for both the active and inactive flash bank is stored in the ice_bank_info structure to avoid unnecessary duplicate work when accessing multiple words of the Shadow RAM. Both banks are read in the unlikely event that the header length is different for the NVM in the inactive bank, rather than being different only by the overall device family.
>
> Fixes: ba20ecb1d1bb ("ice: Hook up 4 E830 devices by adding their IDs")
> Co-developed-by: Paul Greenwalt <paul.greenwalt@intel.com>
> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> ---
>  drivers/net/ethernet/intel/ice/ice_nvm.c  | 88 ++++++++++++++++++++++++++++++-  drivers/net/ethernet/intel/ice/ice_type.h | 14 +++--
>  2 files changed, 93 insertions(+), 9 deletions(-)
>

Tested-by: Pucha Himasekhar Reddy <himasekharx.reddy.pucha@intel.com> (A Contingent worker at Intel)

On 5/21/2024 9:11 PM, Paul Menzel wrote:
> Dear Jacob,
> 
> 
> Am 21.05.24 um 21:27 schrieb Jacob Keller:
> 
>> On 5/20/2024 10:55 PM, Paul Menzel wrote:
> 
>>> Am 20.05.24 um 23:39 schrieb Jacob Keller:
>>>> The ice driver reads data from the Shadow RAM portion of the NVM during
>>>> initialization, including data used to identify the NVM image and device,
>>>> such as the ETRACK ID used to populate devlink dev info fw.bundle.
>>>>
>>>> Currently it is using a fixed offset defined by ICE_CSS_HEADER_LENGTH to
>>>> compute the appropriate offset. This worked fine for E810 and E822 devices
>>>> which both have CSS header length of 330 words.
>>>>
>>>> Other devices, including both E825-C and E830 devices have different sizes
>>>> for their CSS header. The use of a hard coded value results in the driver
>>>> reading from the wrong block in the NVM when attempting to access the
>>>> Shadow RAM copy. This results in the driver reporting the fw.bundle as 0x0
>>>> in both the devlink dev info and ethtool -i output.
>>>>
>>>> The first E830 support was introduced by commit ba20ecb1d1bb ("ice: Hook up
>>>> 4 E830 devices by adding their IDs") and the first E825-C support was
>>>> introducted by commit f64e18944233 ("ice: introduce new E825C devices
>>>
>>> introduced
>>>
>>>> family")
>>>>
>>>> The NVM actually contains the CSS header length embedded in it. Remove the
>>>> hard coded value and replace it with logic to read the length from the NVM
>>>> directly. This is more resilient against all existing and future hardware,
>>>> vs looking up the expected values from a table. It ensures the driver will
>>>> read from the appropriate place when determining the ETRACK ID value used
>>>> for populating the fw.bundle_id and for reporting in ethtool -i.
>>>>
>>>> The CSS header length for both the active and inactive flash bank is stored
>>>> in the ice_bank_info structure to avoid unnecessary duplicate work when
>>>> accessing multiple words of the Shadow RAM. Both banks are read in the
>>>> unlikely event that the header length is different for the NVM in the
>>>> inactive bank, rather than being different only by the overall device
>>>> family.
>>>>
>>>> Fixes: ba20ecb1d1bb ("ice: Hook up 4 E830 devices by adding their IDs")
>>>> Co-developed-by: Paul Greenwalt <paul.greenwalt@intel.com>
>>>> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.com>
>>>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>>>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>>>> ---
>>>>    drivers/net/ethernet/intel/ice/ice_nvm.c  | 88 ++++++++++++++++++++++++++++++-
>>>>    drivers/net/ethernet/intel/ice/ice_type.h | 14 +++--
>>>>    2 files changed, 93 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ice/ice_nvm.c b/drivers/net/ethernet/intel/ice/ice_nvm.c
>>>> index 84eab92dc03c..5968011e8c7e 100644
>>>> --- a/drivers/net/ethernet/intel/ice/ice_nvm.c
>>>> +++ b/drivers/net/ethernet/intel/ice/ice_nvm.c
>>>> @@ -374,11 +374,25 @@ ice_read_nvm_module(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u1
>>>>     *
>>>>     * Read the specified word from the copy of the Shadow RAM found in the
>>>>     * specified NVM module.
>>>> + *
>>>> + * Note that the Shadow RAM copy is always located after the CSS header, and
>>>> + * is aligned to 64-byte (32-word) offsets.
>>>>     */
>>>>    static int
>>>>    ice_read_nvm_sr_copy(struct ice_hw *hw, enum ice_bank_select bank, u32 offset, u16 *data)
>>>>    {
>>>> -	return ice_read_nvm_module(hw, bank, ICE_NVM_SR_COPY_WORD_OFFSET + offset, data);
>>>> +	u32 sr_copy;
>>>> +
>>>> +	switch (bank) {
>>>> +	case ICE_ACTIVE_FLASH_BANK:
>>>> +		sr_copy = roundup(hw->flash.banks.active_css_hdr_len, 32);
>>>> +		break;
>>>> +	case ICE_INACTIVE_FLASH_BANK:
>>>> +		sr_copy = roundup(hw->flash.banks.inactive_css_hdr_len, 32);
>>>> +		break;
>>>> +	}
>>>> +
>>>> +	return ice_read_nvm_module(hw, bank, sr_copy + offset, data);
>>>>    }
>>>>    
>>>>    /**
>>>> @@ -1009,6 +1023,72 @@ static int ice_determine_active_flash_banks(struct ice_hw *hw)
>>>>    	return 0;
>>>>    }
>>>>    
>>>> +/**
>>>> + * ice_get_nvm_css_hdr_len - Read the CSS header length from the NVM CSS header
>>>> + * @hw: pointer to the HW struct
>>>> + * @bank: whether to read from the active or inactive flash bank
>>>> + * @hdr_len: storage for header length in words
>>>> + *
>>>> + * Read the CSS header length from the NVM CSS header and add the Authentication
>>>> + * header size, and then convert to words.
>>>> + *
>>>> + * Return: zero on success, or a negative error code on failure.
>>>> + */
>>>> +static int
>>>> +ice_get_nvm_css_hdr_len(struct ice_hw *hw, enum ice_bank_select bank,
>>>> +			u32 *hdr_len)
>>>> +{
>>>> +	u16 hdr_len_l, hdr_len_h;
>>>> +	u32 hdr_len_dword;
>>>> +	int status;
>>>> +
>>>> +	status = ice_read_nvm_module(hw, bank, ICE_NVM_CSS_HDR_LEN_L,
>>>> +				     &hdr_len_l);
>>>> +	if (status)
>>>> +		return status;
>>>> +
>>>> +	status = ice_read_nvm_module(hw, bank, ICE_NVM_CSS_HDR_LEN_H,
>>>> +				     &hdr_len_h);
>>>> +	if (status)
>>>> +		return status;
>>>> +
>>>> +	/* CSS header length is in DWORD, so convert to words and add
>>>> +	 * authentication header size
>>>> +	 */
>>>> +	hdr_len_dword = hdr_len_h << 16 | hdr_len_l;
>>>> +	*hdr_len = (hdr_len_dword * 2) + ICE_NVM_AUTH_HEADER_LEN;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +/**
>>>> + * ice_determine_css_hdr_len - Discover CSS header length for the device
>>>> + * @hw: pointer to the HW struct
>>>> + *
>>>> + * Determine the size of the CSS header at the start of the NVM module. This
>>>> + * is useful for locating the Shadow RAM copy in the NVM, as the Shadow RAM is
>>>> + * always located just after the CSS header.
>>>> + *
>>>> + * Return: zero on success, or a negative error code on failure.
>>>> + */
>>>> +static int ice_determine_css_hdr_len(struct ice_hw *hw)
>>>> +{
>>>> +	struct ice_bank_info *banks = &hw->flash.banks;
>>>> +	int status;
>>>> +
>>>> +	status = ice_get_nvm_css_hdr_len(hw, ICE_ACTIVE_FLASH_BANK,
>>>> +					 &banks->active_css_hdr_len);
>>>> +	if (status)
>>>> +		return status;
>>>> +
>>>> +	status = ice_get_nvm_css_hdr_len(hw, ICE_INACTIVE_FLASH_BANK,
>>>> +					 &banks->inactive_css_hdr_len);
>>>> +	if (status)
>>>> +		return status;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>>    /**
>>>>     * ice_init_nvm - initializes NVM setting
>>>>     * @hw: pointer to the HW struct
>>>> @@ -1055,6 +1135,12 @@ int ice_init_nvm(struct ice_hw *hw)
>>>>    		return status;
>>>>    	}
>>>>    
>>>> +	status = ice_determine_css_hdr_len(hw);
>>>> +	if (status) {
>>>> +		ice_debug(hw, ICE_DBG_NVM, "Failed to determine Shadow RAM copy offsets.\n");
>>>
>>> As this is a new failure path, should the user be warned about this, if
>>> it cannot be determined, and NVM possibly be broken?
> 
>> Possibly. I'm also trying to avoid spamming the log with failure
>> messages which are more useful for developers who can enable them vs end
>> users who may not understand.
> 
> I agree that logging too much is also confusing. Excuse my ignorance, 
> but what happens if NVM is broken and the offset cannot be determined. 
> Will the user get any error message and know what to do (replace the 
> device or call support)? Or another view point, is the bug report with 
> the Linux log messages included going to have the information, so the 
> support folks or developers can pinpoint the problem without replying to 
> the user to enable debug messages?
> 

I'm not sure. In this case it probably does make sense to warn, since
this is unexpected and is unlikely to result in spamming the log
multiple times.

> 
> Kind regards,
> 
> Paul