[Intel-wired-lan] [PATCH v2 1/2] e1000e: disregard NVM checksum on tgp when valid checksum mask is not set

[Intel-wired-lan] [PATCH v2 1/2] e1000e: disregard NVM checksum on tgp when valid checksum mask is not set

[Jacek Kowalski]

As described by Vitaly Lifshits:

> Starting from Tiger Lake, LAN NVM is locked for writes by SW, so the
> driver cannot perform checksum validation and correction. This means
> that all NVM images must leave the factory with correct checksum and
> checksum valid bit set. Since Tiger Lake devices were the first to have
> this lock, some systems in the field did not meet this requirement.
> Therefore, for these transitional devices we skip checksum update and
> verification, if the valid bit is not set.

Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
Reviewed-by: Simon Horman <horms@kernel.org>
Reviewed-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
Fixes: 4051f68318ca9 ("e1000e: Do not take care about recovery NVM checksum")
Cc: stable@vger.kernel.org
---
v1 -> v2: updated patch description
 drivers/net/ethernet/intel/e1000e/ich8lan.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c
index 364378133526..df4e7d781cb1 100644
--- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
+++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
@@ -4274,6 +4274,8 @@ static s32 e1000_validate_nvm_checksum_ich8lan(struct e1000_hw *hw)
 			ret_val = e1000e_update_nvm_checksum(hw);
 			if (ret_val)
 				return ret_val;
+		} else if (hw->mac.type == e1000_pch_tgp) {
+			return 0;
 		}
 	}
 
-- 
2.47.2

[Intel-wired-lan] [PATCH v2 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Jacek Kowalski]

As described by Vitaly Lifshits:

> Starting from Tiger Lake, LAN NVM is locked for writes by SW, so the
> driver cannot perform checksum validation and correction. This means
> that all NVM images must leave the factory with correct checksum and
> checksum valid bit set.

Unfortunately some systems have left the factory with an empty checksum.
NVM is not modifiable on this platform, hence ignore checksum 0xFFFF on
Tiger Lake systems to work around this.

Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
Fixes: 4051f68318ca9 ("e1000e: Do not take care about recovery NVM checksum")
Cc: stable@vger.kernel.org
---
v2: new check to fix yet another checksum issue
 drivers/net/ethernet/intel/e1000e/defines.h | 1 +
 drivers/net/ethernet/intel/e1000e/nvm.c     | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h
index 8294a7c4f122..01696eb8dace 100644
--- a/drivers/net/ethernet/intel/e1000e/defines.h
+++ b/drivers/net/ethernet/intel/e1000e/defines.h
@@ -637,6 +637,7 @@
 
 /* For checksumming, the sum of all words in the NVM should equal 0xBABA. */
 #define NVM_SUM                    0xBABA
+#define NVM_SUM_FACTORY_DEFAULT    0xFFFF
 
 /* PBA (printed board assembly) number words */
 #define NVM_PBA_OFFSET_0           8
diff --git a/drivers/net/ethernet/intel/e1000e/nvm.c b/drivers/net/ethernet/intel/e1000e/nvm.c
index e609f4df86f4..37cbf9236d84 100644
--- a/drivers/net/ethernet/intel/e1000e/nvm.c
+++ b/drivers/net/ethernet/intel/e1000e/nvm.c
@@ -558,6 +558,11 @@ s32 e1000e_validate_nvm_checksum_generic(struct e1000_hw *hw)
 		checksum += nvm_data;
 	}
 
+	if (hw->mac.type == e1000_pch_tgp && checksum == (u16)NVM_SUM_FACTORY_DEFAULT) {
+		e_dbg("Factory-default NVM Checksum on TGP platform - ignoring\n");
+		return 0;
+	}
+
 	if (checksum != (u16)NVM_SUM) {
 		e_dbg("NVM Checksum Invalid\n");
 		return -E1000_ERR_NVM;
-- 
2.47.2

Re: [Intel-wired-lan] [PATCH v2 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Jacek Kowalski]

Vlad,

could you verify that the following patch works for you?

> diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h
> index 8294a7c4f122..01696eb8dace 100644
> --- a/drivers/net/ethernet/intel/e1000e/defines.h
> +++ b/drivers/net/ethernet/intel/e1000e/defines.h
> @@ -637,6 +637,7 @@
>  
>  /* For checksumming, the sum of all words in the NVM should equal 0xBABA. */
>  #define NVM_SUM                    0xBABA
> +#define NVM_SUM_FACTORY_DEFAULT    0xFFFF
>  
>  /* PBA (printed board assembly) number words */
>  #define NVM_PBA_OFFSET_0           8
> diff --git a/drivers/net/ethernet/intel/e1000e/nvm.c b/drivers/net/ethernet/intel/e1000e/nvm.c
> index e609f4df86f4..37cbf9236d84 100644
> --- a/drivers/net/ethernet/intel/e1000e/nvm.c
> +++ b/drivers/net/ethernet/intel/e1000e/nvm.c
> @@ -558,6 +558,11 @@ s32 e1000e_validate_nvm_checksum_generic(struct e1000_hw *hw)
>  		checksum += nvm_data;
>  	}
>  
> +	if (hw->mac.type == e1000_pch_tgp && checksum == (u16)NVM_SUM_FACTORY_DEFAULT) {
> +		e_dbg("Factory-default NVM Checksum on TGP platform - ignoring\n");
> +		return 0;
> +	}
> +
>  	if (checksum != (u16)NVM_SUM) {
>  		e_dbg("NVM Checksum Invalid\n");
>  		return -E1000_ERR_NVM;

-- 
Best regards,
  Jacek Kowalski

Re: [Intel-wired-lan] [PATCH v2 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Simon Horman]

On Mon, Jun 23, 2025 at 06:01:16PM +0200, Jacek Kowalski wrote:
> As described by Vitaly Lifshits:
> 
> > Starting from Tiger Lake, LAN NVM is locked for writes by SW, so the
> > driver cannot perform checksum validation and correction. This means
> > that all NVM images must leave the factory with correct checksum and
> > checksum valid bit set.
> 
> Unfortunately some systems have left the factory with an empty checksum.
> NVM is not modifiable on this platform, hence ignore checksum 0xFFFF on
> Tiger Lake systems to work around this.
> 
> Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
> Fixes: 4051f68318ca9 ("e1000e: Do not take care about recovery NVM checksum")
> Cc: stable@vger.kernel.org
> ---
> v2: new check to fix yet another checksum issue
>  drivers/net/ethernet/intel/e1000e/defines.h | 1 +
>  drivers/net/ethernet/intel/e1000e/nvm.c     | 5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h
> index 8294a7c4f122..01696eb8dace 100644
> --- a/drivers/net/ethernet/intel/e1000e/defines.h
> +++ b/drivers/net/ethernet/intel/e1000e/defines.h
> @@ -637,6 +637,7 @@
>  
>  /* For checksumming, the sum of all words in the NVM should equal 0xBABA. */
>  #define NVM_SUM                    0xBABA
> +#define NVM_SUM_FACTORY_DEFAULT    0xFFFF
>  
>  /* PBA (printed board assembly) number words */
>  #define NVM_PBA_OFFSET_0           8
> diff --git a/drivers/net/ethernet/intel/e1000e/nvm.c b/drivers/net/ethernet/intel/e1000e/nvm.c
> index e609f4df86f4..37cbf9236d84 100644
> --- a/drivers/net/ethernet/intel/e1000e/nvm.c
> +++ b/drivers/net/ethernet/intel/e1000e/nvm.c
> @@ -558,6 +558,11 @@ s32 e1000e_validate_nvm_checksum_generic(struct e1000_hw *hw)
>  		checksum += nvm_data;
>  	}
>  
> +	if (hw->mac.type == e1000_pch_tgp && checksum == (u16)NVM_SUM_FACTORY_DEFAULT) {

I see that a similar cast is applied to NVM_SUM. But why?
If it's not necessary then I would advocate dropping it.

> +		e_dbg("Factory-default NVM Checksum on TGP platform - ignoring\n");
> +		return 0;
> +	}
> +
>  	if (checksum != (u16)NVM_SUM) {
>  		e_dbg("NVM Checksum Invalid\n");
>  		return -E1000_ERR_NVM;
> -- 
> 2.47.2
> 
> 

Re: [Intel-wired-lan] [PATCH v2 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Jacek Kowalski]

>> +	if (hw->mac.type == e1000_pch_tgp && checksum == (u16)NVM_SUM_FACTORY_DEFAULT) {
> 
> I see that a similar cast is applied to NVM_SUM. But why?
> If it's not necessary then I would advocate dropping it.

It's like that since the beginning of git history, tracing back to e1000:

$ git show 1da177e4c3f4:drivers/net/e1000/e1000_hw.c | grep -A 1 EEPROM_SUM
     if(checksum == (uint16_t) EEPROM_SUM)
         return E1000_SUCCESS;
(...)


I'd really prefer to keep it as-is here for a moment, since similar 
constructs are not only here, and then clean them up separately.

Examples instances from drivers/net/ethernet/intel:

e1000/e1000_ethtool.c:  if ((checksum != (u16)EEPROM_SUM) && !(*data))
e1000/e1000_hw.c:       if (checksum == (u16)EEPROM_SUM)
e1000e/ethtool.c:       if ((checksum != (u16)NVM_SUM) && !(*data))
igb/e1000_82575.c:      if (checksum != (u16) NVM_SUM) {
igb/e1000_nvm.c:        if (checksum != (u16) NVM_SUM) {
igc/igc_nvm.c:  if (checksum != (u16)NVM_SUM) {

-- 
Best regards,
   Jacek Kowalski

Re: [Intel-wired-lan] [PATCH v2 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Simon Horman]

On Tue, Jun 24, 2025 at 02:51:09PM +0200, Jacek Kowalski wrote:
> > > +	if (hw->mac.type == e1000_pch_tgp && checksum == (u16)NVM_SUM_FACTORY_DEFAULT) {
> > 
> > I see that a similar cast is applied to NVM_SUM. But why?
> > If it's not necessary then I would advocate dropping it.
> 
> It's like that since the beginning of git history, tracing back to e1000:
> 
> $ git show 1da177e4c3f4:drivers/net/e1000/e1000_hw.c | grep -A 1 EEPROM_SUM
>     if(checksum == (uint16_t) EEPROM_SUM)
>         return E1000_SUCCESS;
> (...)
> 
> 
> I'd really prefer to keep it as-is here for a moment, since similar
> constructs are not only here, and then clean them up separately.
> 
> Examples instances from drivers/net/ethernet/intel:
> 
> e1000/e1000_ethtool.c:  if ((checksum != (u16)EEPROM_SUM) && !(*data))
> e1000/e1000_hw.c:       if (checksum == (u16)EEPROM_SUM)
> e1000e/ethtool.c:       if ((checksum != (u16)NVM_SUM) && !(*data))
> igb/e1000_82575.c:      if (checksum != (u16) NVM_SUM) {
> igb/e1000_nvm.c:        if (checksum != (u16) NVM_SUM) {
> igc/igc_nvm.c:  if (checksum != (u16)NVM_SUM) {

Ok. But can we look into cleaning this up as a follow-up?

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

Re: [Intel-wired-lan] [PATCH v2 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Jacek Kowalski]

>>>> +	if (hw->mac.type == e1000_pch_tgp && checksum == 
>>>> (u16)NVM_SUM_FACTORY_DEFAULT) {
>>> 
>>> I see that a similar cast is applied to NVM_SUM. But why? If 
>>> it's not necessary then I would advocate dropping it.
>> 
>> It's like that since the beginning of git history, tracing back to
>> e1000(...)
>> 
>> I'd really prefer to keep it as-is here for a moment, since 
>> similar constructs are not only here, and then clean them up 
>> separately.
> 
> Ok. But can we look into cleaning this up as a follow-up?

Sure, I'll prepare the patch and send it once this series is applied.

-- 
Best regards,
   Jacek Kowalski

Re: [Intel-wired-lan] [PATCH v2 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Vlad URSU]

On 23.06.2025 19:18, Jacek Kowalski wrote:
> Vlad,
> 
> could you verify that the following patch works for you?
> 
>> diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h
>> index 8294a7c4f122..01696eb8dace 100644
>> --- a/drivers/net/ethernet/intel/e1000e/defines.h
>> +++ b/drivers/net/ethernet/intel/e1000e/defines.h
>> @@ -637,6 +637,7 @@
>>   
>>   /* For checksumming, the sum of all words in the NVM should equal 0xBABA. */
>>   #define NVM_SUM                    0xBABA
>> +#define NVM_SUM_FACTORY_DEFAULT    0xFFFF
>>   
>>   /* PBA (printed board assembly) number words */
>>   #define NVM_PBA_OFFSET_0           8
>> diff --git a/drivers/net/ethernet/intel/e1000e/nvm.c b/drivers/net/ethernet/intel/e1000e/nvm.c
>> index e609f4df86f4..37cbf9236d84 100644
>> --- a/drivers/net/ethernet/intel/e1000e/nvm.c
>> +++ b/drivers/net/ethernet/intel/e1000e/nvm.c
>> @@ -558,6 +558,11 @@ s32 e1000e_validate_nvm_checksum_generic(struct e1000_hw *hw)
>>   		checksum += nvm_data;
>>   	}
>>   
>> +	if (hw->mac.type == e1000_pch_tgp && checksum == (u16)NVM_SUM_FACTORY_DEFAULT) {
>> +		e_dbg("Factory-default NVM Checksum on TGP platform - ignoring\n");
>> +		return 0;
>> +	}
>> +
>>   	if (checksum != (u16)NVM_SUM) {
>>   		e_dbg("NVM Checksum Invalid\n");
>>   		return -E1000_ERR_NVM;
> 

No, it doesn't.

You are comparing the wrong value with NVM_SUM_FACTORY_DEFAULT. You 
should check it against the checksum word 0x3F (NVM bytes 0x7E and 0x7F) 
which is used to ensure that the base NVM image
is a valid image, and which in my case is left unchanged by Dell in the 
firmware.

I believe the changes should look something like this:

---
  drivers/net/ethernet/intel/e1000e/defines.h |  3 +++
  drivers/net/ethernet/intel/e1000e/nvm.c     | 13 ++++++++++++-
  2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/defines.h 
b/drivers/net/ethernet/intel/e1000e/defines.h
index 8294a7c4f122..996a0f4d2b49 100644
--- a/drivers/net/ethernet/intel/e1000e/defines.h
+++ b/drivers/net/ethernet/intel/e1000e/defines.h
@@ -638,6 +638,9 @@
  /* For checksumming, the sum of all words in the NVM should equal 
0xBABA. */
  #define NVM_SUM                    0xBABA

+/* Factory default value for the NVM checksum word */
+#define NVM_CHECKSUM_WORD_FACTORY_DEFAULT       0xFFFF
+
  /* PBA (printed board assembly) number words */
  #define NVM_PBA_OFFSET_0           8
  #define NVM_PBA_OFFSET_1           9
diff --git a/drivers/net/ethernet/intel/e1000e/nvm.c 
b/drivers/net/ethernet/intel/e1000e/nvm.c
index e609f4df86f4..4620efac0208 100644
--- a/drivers/net/ethernet/intel/e1000e/nvm.c
+++ b/drivers/net/ethernet/intel/e1000e/nvm.c
@@ -547,7 +547,18 @@ s32 e1000e_validate_nvm_checksum_generic(struct 
e1000_hw *hw)
  {
  	s32 ret_val;
  	u16 checksum = 0;
-	u16 i, nvm_data;
+	u16 i, nvm_data, checksum_word;
+
+	ret_val = e1000_read_nvm(hw, NVM_CHECKSUM_REG, 1, &checksum_word);
+	if (ret_val) {
+		e_dbg("NVM Read Error\n");
+		return ret_val;
+	}
+
+	if (hw->mac.type == e1000_pch_tgp && checksum_word == 
(u16)NVM_CHECKSUM_WORD_FACTORY_DEFAULT) {
+		e_dbg("Factory-default NVM Checksum word on TGP platform - ignoring\n");
+		return 0;
+	}

  	for (i = 0; i < (NVM_CHECKSUM_REG + 1); i++) {
  		ret_val = e1000_read_nvm(hw, i, 1, &nvm_data);
-- 

Re: [Intel-wired-lan] [PATCH v2 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Jacek Kowalski]

> You are comparing the wrong value with NVM_SUM_FACTORY_DEFAULT. You 
> should check it against the checksum word 0x3F (NVM bytes 0x7E and
> 0x7F) which is used to ensure that the base NVM image is a valid
> image, and which in my case is left unchanged by Dell in the 
> firmware.

You are right that I'm comparing the wrong value. But it is only a 
matter of variable name:

-	if (hw->mac.type == e1000_pch_tgp && checksum ==
(u16)NVM_SUM_FACTORY_DEFAULT) {
+	if (hw->mac.type == e1000_pch_tgp && nvm_data ==
(u16)NVM_SUM_FACTORY_DEFAULT) {

Could you check my change with this modification?

-- 
Best regards,
   Jacek Kowalski

Re: [Intel-wired-lan] [PATCH v2 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Vlad URSU]

On 24.06.2025 21:34, Jacek Kowalski wrote:

> You are right that I'm comparing the wrong value. But it is only a 
> matter of variable name:

Ah, yes, you're right. I missed the fact that nvm_data will hold the 
checksum word at the end of the for loop.

> -    if (hw->mac.type == e1000_pch_tgp && checksum ==
> (u16)NVM_SUM_FACTORY_DEFAULT) {
> +    if (hw->mac.type == e1000_pch_tgp && nvm_data ==
> (u16)NVM_SUM_FACTORY_DEFAULT) {
> 
> Could you check my change with this modification?

It works with this change.