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.
