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

[Simon Horman <horms@kernel.org>]

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.