* [Intel-wired-lan] [PATCH v1 1/1] e1000e: Do not take care about recovery NVM checksum
@ 2021-07-15 7:03 Sasha Neftin
2021-07-15 7:15 ` Paul Menzel
2021-08-08 8:55 ` Fuxbrumer, Dvora
0 siblings, 2 replies; 6+ messages in thread
From: Sasha Neftin @ 2021-07-15 7:03 UTC (permalink / raw)
To: intel-wired-lan
According to the HW De, integrated GbE sets to read-only after
programming a unique MAC address. The driver should not take care of
NVM checksum updating starting from Tiger Lake.
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");
+
+ if (hw->mac.type < e1000_pch_cnp) {
+ 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;
+ }
}
return e1000e_validate_nvm_checksum_generic(hw);
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [PATCH v1 1/1] e1000e: Do not take care about recovery NVM checksum
2021-07-15 7:03 [Intel-wired-lan] [PATCH v1 1/1] e1000e: Do not take care about recovery NVM checksum Sasha Neftin
@ 2021-07-15 7:15 ` Paul Menzel
2021-07-15 7:54 ` Sasha Neftin
2021-08-08 8:55 ` Fuxbrumer, Dvora
1 sibling, 1 reply; 6+ messages in thread
From: Paul Menzel @ 2021-07-15 7:15 UTC (permalink / raw)
To: intel-wired-lan
Dear Sasha,
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*.
> programming a unique MAC address. The driver should not take care of
Excuse my ignorance, who is programming the MAC address?
> NVM checksum updating starting from Tiger Lake.
Who is updating the checksum? Please reference some datasheet name,
revision and section.
> 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.
> +
> + 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.
> + 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [PATCH v1 1/1] e1000e: Do not take care about recovery NVM checksum
2021-07-15 7:15 ` Paul Menzel
@ 2021-07-15 7:54 ` Sasha Neftin
2021-07-15 11:48 ` Paul Menzel
0 siblings, 1 reply; 6+ messages in thread
From: Sasha Neftin @ 2021-07-15 7:54 UTC (permalink / raw)
To: intel-wired-lan
On 7/15/2021 10:15, Paul Menzel wrote:
> Dear Sasha,
>
>
> 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
>
>> 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.
>
>> 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");
>
>> +
>> +??? 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.
>
>> +??????????? 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
Paul,
Thanks for your comments.
sasha
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [PATCH v1 1/1] e1000e: Do not take care about recovery NVM checksum
2021-07-15 7:54 ` Sasha Neftin
@ 2021-07-15 11:48 ` Paul Menzel
2021-07-15 12:26 ` Sasha Neftin
0 siblings, 1 reply; 6+ messages in thread
From: Paul Menzel @ 2021-07-15 11:48 UTC (permalink / raw)
To: intel-wired-lan
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.
>>> 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.)
>>> 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:
```
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [PATCH v1 1/1] e1000e: Do not take care about recovery NVM checksum
2021-07-15 11:48 ` Paul Menzel
@ 2021-07-15 12:26 ` Sasha Neftin
0 siblings, 0 replies; 6+ messages in thread
From: Sasha Neftin @ 2021-07-15 12:26 UTC (permalink / raw)
To: intel-wired-lan
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Intel-wired-lan] [PATCH v1 1/1] e1000e: Do not take care about recovery NVM checksum
2021-07-15 7:03 [Intel-wired-lan] [PATCH v1 1/1] e1000e: Do not take care about recovery NVM checksum Sasha Neftin
2021-07-15 7:15 ` Paul Menzel
@ 2021-08-08 8:55 ` Fuxbrumer, Dvora
1 sibling, 0 replies; 6+ messages in thread
From: Fuxbrumer, Dvora @ 2021-08-08 8:55 UTC (permalink / raw)
To: intel-wired-lan
On 7/15/2021 10:03, Sasha Neftin wrote:
> According to the HW De, integrated GbE sets to read-only after
> programming a unique MAC address. The driver should not take care of
> NVM checksum updating starting from Tiger Lake.
>
> 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(-)
>
Tested-by: Dvora Fuxbrumer <dvorax.fuxbrumer@linux.intel.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-08-08 8:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-15 7:03 [Intel-wired-lan] [PATCH v1 1/1] e1000e: Do not take care about recovery NVM checksum Sasha Neftin
2021-07-15 7:15 ` Paul Menzel
2021-07-15 7:54 ` Sasha Neftin
2021-07-15 11:48 ` Paul Menzel
2021-07-15 12:26 ` Sasha Neftin
2021-08-08 8:55 ` Fuxbrumer, Dvora
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.