[Intel-wired-lan] [PATCH v1 1/1] e1000e: Do not take care about recovery NVM checksum

[Sasha Neftin <sasha.neftin@intel.com>]

On 7/15/2021 14:48, Paul Menzel wrote:
> Dear Sasha,
> 
> 
> Am 15.07.21 um 09:54 schrieb Sasha Neftin:
>> On 7/15/2021 10:15, Paul Menzel wrote:
> 
>>> Am 15.07.21 um 09:03 schrieb Sasha Neftin:
>>>
>>> Please describe the problem first (lockup) (maybe by summarizing the 
>>> bug report).
>>>
>>>> According to the HW De, integrated GbE sets to read-only after
>>>
>>> Please use *developers*.
>> I meant: hardware design
> 
> Hah. Thank you for the clarification. It?d be great, if you used that in 
> the v2.
no problem
> 
>>>> programming a unique MAC address. The driver should not take care of
>>>
>>> Excuse my ignorance, who is programming the MAC address? OS vendors 
>>> and PC vendors
>>>
>>>> NVM checksum updating starting from Tiger Lake.
>>>
>>> Who is updating the checksum? Please reference some datasheet name, 
>>> revision and section.
>> OS vendors and PC vendors
>> It is described in Intel RCR 1308265811 - I do not know if published 
>> externally. I've cc'd our front customer expert (Rex) - please, ask 
>> him if it published.
> 
> Even if not published, please still reference it. (Though public 
> datasheets by default would be nice.)
> 
definitely
>>>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=213667
>>>> Suggested-by: Dima Ruinskiy <dima.ruinskiy@intel.com>
>>>> Suggested-by: Vitaly Lifshits <vitaly.lifshits@intel.com>
>>>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>>>> ---
>>>> ? drivers/net/ethernet/intel/e1000e/ich8lan.c | 21 
>>>> +++++++++++++--------
>>>> ? 1 file changed, 13 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c 
>>>> b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>>> index 9bae4932a11d..e273e14a3419 100644
>>>> --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>>> +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c
>>>> @@ -4140,14 +4140,19 @@ static s32 
>>>> e1000_validate_nvm_checksum_ich8lan(struct e1000_hw *hw)
>>>> ????? if (ret_val)
>>>> ????????? return ret_val;
>>>> -??? if (!(data & valid_csum_mask)) {
>>>> -??????? data |= valid_csum_mask;
>>>> -??????? ret_val = e1000_write_nvm(hw, word, 1, &data);
>>>> -??????? if (ret_val)
>>>> -??????????? return ret_val;
>>>> -??????? ret_val = e1000e_update_nvm_checksum(hw);
>>>> -??????? if (ret_val)
>>>> -??????????? return ret_val;
>>>> +??? if (!(data & valid_csum_mask))
>>>> +??????? e_dbg("NVM Checksum Invalid\n");
>>>
>>> I?d spell it: NVM checksum invalid
>>>
>>> Shouldn?t this be at least a warning? It?d be good to elaborate for 
>>> users seeing this message. Something like: Your device might not 
>>> work. Please check your firmware or contact the developers.
>> to be consistent used same warning format as in nvm.c: ("NVM Checksum 
>> Invalid\n");
> 
> For consistency, is it possible to factor the NVM stuff out into `nvm.c`?
> 
> Also, the message should contain, that the manufacturer is at fault and 
> should be contacted.
> 
>>>> +
>>>> +??? if (hw->mac.type < e1000_pch_cnp) {
>>>> +??????? if (!(data & valid_csum_mask)) {
>>>
>>> As it?s the same check as above, I?d move this whole block into the 
>>> if condition above.
>> For old devices will performed checksum recovery.
>> NVM checksum validate will be processed for all.
> 
> I meant:
> 
Good idea, sure.
> ```
> if (!(data & valid_csum_mask)) {
>  ????e_dbg("NVM Checksum Invalid\n");
> 
>  ????if (hw->mac.type < e1000_pch_cnp) {
>  ??????? data |= valid_csum_mask;
>  ??????? [?]
>  ????}
> }
> return e1000e_validate_nvm_checksum_generic(hw);
> ```
> 
>>>> +??????????? data |= valid_csum_mask;
>>>> +??????????? ret_val = e1000_write_nvm(hw, word, 1, &data);
>>>> +??????????? if (ret_val)
>>>> +??????????????? return ret_val;
>>>> +??????????? ret_val = e1000e_update_nvm_checksum(hw);
>>>> +??????????? if (ret_val)
>>>> +??????????????? return ret_val;
>>>> +??????? }
>>>> ????? }
>>>> ????? return e1000e_validate_nvm_checksum_generic(hw);
> 
> 
> Kind regards,
> 
> Paul
I'll process v2.
Sasha