| Message ID | 20210715070342.2948195-1-sasha.neftin@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1,1/1] e1000e: Do not take care about recovery NVM checksum | expand |
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
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
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
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
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>
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);
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(-)