[PATCH v3 1/2] e1000e: disregard NVM checksum on tgp when valid checksum mask is not set

[PATCH v3 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
v2 -> v3: no changes
 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

[PATCH v3 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>
Tested-by: Vlad URSU <vlad@ursu.me>
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
v2 -> v3: fix variable bein compared, drop u16 cast
 drivers/net/ethernet/intel/e1000e/defines.h | 3 +++
 drivers/net/ethernet/intel/e1000e/nvm.c     | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h
index 8294a7c4f122..2dcf46080533 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 checksum value */
+#define NVM_CHECKSUM_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..56f2434bd00a 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 && nvm_data == NVM_CHECKSUM_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: [PATCH v3 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Simon Horman]

On Tue, Jun 24, 2025 at 09:14:40PM +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.

I think that you need to update the patch description.
As of v3 it's the last word of the checksum that is being checked,
not the entire checksum.

> 
> Signed-off-by: Jacek Kowalski <Jacek@jacekk.info>
> Tested-by: Vlad URSU <vlad@ursu.me>
> 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
> v2 -> v3: fix variable bein compared, drop u16 cast
>  drivers/net/ethernet/intel/e1000e/defines.h | 3 +++
>  drivers/net/ethernet/intel/e1000e/nvm.c     | 5 +++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/defines.h b/drivers/net/ethernet/intel/e1000e/defines.h
> index 8294a7c4f122..2dcf46080533 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 checksum value */
> +#define NVM_CHECKSUM_FACTORY_DEFAULT 0xFFFF

Perhaps it is too long, but I liked Vlad's suggestion of
naming this NVM_CHECKSUM_WORD_FACTORY_DEFAULT.

> +
>  /* 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..56f2434bd00a 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 && nvm_data == NVM_CHECKSUM_FACTORY_DEFAULT) {

Please wrap the line above so it is 80 columns wide or less.

	if (hw->mac.type == e1000_pch_tgp &&
	    nvm_data == NVM_CHECKSUM_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;

-- 
pw-bot: changes-requested

Re: [PATCH v3 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Jacek Kowalski]

>> 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.
> 
> I think that you need to update the patch description. As of v3 it's
> the last word of the checksum that is being checked, not the entire
> checksum.

As I understood it, "sum" is the resulting value while "checksum" is the
value appended so that the "sum" is equal to some constant.

But my understanding is utterly broken by this line:

>> if (checksum != (u16)NVM_SUM) {

Where variable checksum (shall it be "sum"?) that includes
"checksum" (or maybe checksum word?) from the *checksum* register 
address (NVM_CHECKSUM_REG) is compared to a constant called "NVM_SUM".


Is something like this fine by you:

> Unfortunately some systems have left the factory with an unmodified
> value of 0xFFFF at register address 0x3F (checksum word location).
> So on Tiger Lake platform we ignore the computed checksum when such
> condition is encountered.

?


>> +#define NVM_CHECKSUM_FACTORY_DEFAULT 0xFFFF
> 
> Perhaps it is too long, but I liked Vlad's suggestion of naming this
> NVM_CHECKSUM_WORD_FACTORY_DEFAULT.

I can update it as well once we agree on the wording.


>> +	if (hw->mac.type == e1000_pch_tgp && nvm_data == NVM_CHECKSUM_FACTORY_DEFAULT) {
> 
> Please wrap the line above so it is 80 columns wide or less.

Wilco.

-- 
Best regards,
   Jacek Kowalski

Re: [PATCH v3 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Simon Horman]

On Tue, Jun 24, 2025 at 11:05:05PM +0200, Jacek Kowalski wrote:
> > > 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.
> > 
> > I think that you need to update the patch description. As of v3 it's
> > the last word of the checksum that is being checked, not the entire
> > checksum.
> 
> As I understood it, "sum" is the resulting value while "checksum" is the
> value appended so that the "sum" is equal to some constant.
> 
> But my understanding is utterly broken by this line:
> 
> > > if (checksum != (u16)NVM_SUM) {
> 
> Where variable checksum (shall it be "sum"?) that includes
> "checksum" (or maybe checksum word?) from the *checksum* register address
> (NVM_CHECKSUM_REG) is compared to a constant called "NVM_SUM".

I agree with you in so far that there is room for interpretation on what
these terms mean. And I think your interpretation is internally consistent
(even if I might have interpreted things differently myself). But as you
say, the code seems to use these terms differently.

> Is something like this fine by you:
> 
> > Unfortunately some systems have left the factory with an unmodified
> > value of 0xFFFF at register address 0x3F (checksum word location).
> > So on Tiger Lake platform we ignore the computed checksum when such
> > condition is encountered.
> 
> ?

Yes, I think that matches the code change nicely.

> > > +#define NVM_CHECKSUM_FACTORY_DEFAULT 0xFFFF
> > 
> > Perhaps it is too long, but I liked Vlad's suggestion of naming this
> > NVM_CHECKSUM_WORD_FACTORY_DEFAULT.
> 
> I can update it as well once we agree on the wording.

Thanks.

> 
> 
> > > +	if (hw->mac.type == e1000_pch_tgp && nvm_data == NVM_CHECKSUM_FACTORY_DEFAULT) {
> > 
> > Please wrap the line above so it is 80 columns wide or less.
> 
> Wilco.

Likewise, thanks.

Re: [PATCH v3 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Jacek Kowalski]

>>>> +#define NVM_CHECKSUM_FACTORY_DEFAULT 0xFFFF
>>>
>>> Perhaps it is too long, but I liked Vlad's suggestion of naming this
>>> NVM_CHECKSUM_WORD_FACTORY_DEFAULT.

So the proposals are:

1. NVM_CHECKSUM_WORD_FACTORY_DEFAULT
2. NVM_CHECKSUM_FACTORY_DEFAULT
3. NVM_CHECKSUM_INVALID
4. NVM_CHECKSUM_MISSING
5. NVM_CHECKSUM_EMPTY
6. NVM_NO_CHECKSUM

Any other contenders?

-- 
Best regards,
  Jacek Kowalski

Re: [PATCH v3 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Vlad URSU]

On 25.06.2025 16:05, Jacek Kowalski wrote:
>>>>> +#define NVM_CHECKSUM_FACTORY_DEFAULT 0xFFFF
>>>>
>>>> Perhaps it is too long, but I liked Vlad's suggestion of naming this
>>>> NVM_CHECKSUM_WORD_FACTORY_DEFAULT.
> 
> So the proposals are:
> 
> 1. NVM_CHECKSUM_WORD_FACTORY_DEFAULT
> 2. NVM_CHECKSUM_FACTORY_DEFAULT
> 3. NVM_CHECKSUM_INVALID
> 4. NVM_CHECKSUM_MISSING
> 5. NVM_CHECKSUM_EMPTY
> 6. NVM_NO_CHECKSUM
> 
> Any other contenders?
> 

For reference, I called it "CHECKSUM_WORD" in my proposal because that's 
what it's refered to as in the intel documentation (section 10.3.2.2 - 
http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/ethernet-connection-i219-datasheet.pdf)

Re: [PATCH v3 2/2] e1000e: ignore factory-default checksum value on TGP platform

[Simon Horman]

On Wed, Jun 25, 2025 at 05:06:44PM +0300, Vlad URSU wrote:
> On 25.06.2025 16:05, Jacek Kowalski wrote:
> > > > > > +#define NVM_CHECKSUM_FACTORY_DEFAULT 0xFFFF
> > > > > 
> > > > > Perhaps it is too long, but I liked Vlad's suggestion of naming this
> > > > > NVM_CHECKSUM_WORD_FACTORY_DEFAULT.
> > 
> > So the proposals are:
> > 
> > 1. NVM_CHECKSUM_WORD_FACTORY_DEFAULT
> > 2. NVM_CHECKSUM_FACTORY_DEFAULT
> > 3. NVM_CHECKSUM_INVALID
> > 4. NVM_CHECKSUM_MISSING
> > 5. NVM_CHECKSUM_EMPTY
> > 6. NVM_NO_CHECKSUM
> > 
> > Any other contenders?
> > 
> 
> For reference, I called it "CHECKSUM_WORD" in my proposal because that's
> what it's refered to as in the intel documentation (section 10.3.2.2 - http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/ethernet-connection-i219-datasheet.pdf)
> 

FWIIW, I'd vote for 1.

Re: [PATCH v3 2/2] e1000e: ignore factory-default checksum value on TGP platform

[David Laight]

On Wed, 25 Jun 2025 15:05:01 +0200
Jacek Kowalski <jacek@jacekk.info> wrote:

> >>>> +#define NVM_CHECKSUM_FACTORY_DEFAULT 0xFFFF  
> >>>
> >>> Perhaps it is too long, but I liked Vlad's suggestion of naming this
> >>> NVM_CHECKSUM_WORD_FACTORY_DEFAULT.  
> 
> So the proposals are:
> 
> 1. NVM_CHECKSUM_WORD_FACTORY_DEFAULT
> 2. NVM_CHECKSUM_FACTORY_DEFAULT
> 3. NVM_CHECKSUM_INVALID
> 4. NVM_CHECKSUM_MISSING
> 5. NVM_CHECKSUM_EMPTY
> 6. NVM_NO_CHECKSUM
> 
> Any other contenders?
> 

0xffff

With a comment saying some manufacturers don't calculate the checksum.
Then you don't needs to search the definition to find out what is going on.

	David