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

[Sasha Neftin]

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 at intel.com>
>>>> Suggested-by: Vitaly Lifshits <vitaly.lifshits at intel.com>
>>>> Signed-off-by: Sasha Neftin <sasha.neftin at 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