| Message ID | 20220127150807.26448-1-tbogendoerfer@suse.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: e1000e: Recover at least in-memory copy of NVM checksum | expand |
On 1/27/2022 17:08, Thomas Bogendoerfer wrote: > Commit 4051f68318ca ("e1000e: Do not take care about recovery NVM > checksum") causes a regression for systems with a broken NVM checksum > and hardware which is not able to update the NVM. Before the change the > in-memory copy was correct even the NVM update doesn't work, which is > good enough for the driver to work again. > > See > > https://bugzilla.opensuse.org/show_bug.cgi?id=1191663 > > for more details. > > This patch reverts the change and moves the check for hardware without > NVM update capability right before the real flash update. > > Fixes: 4051f68318ca ("e1000e: Do not take care about recovery NVM checksum") > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> > --- > drivers/net/ethernet/intel/e1000e/ich8lan.c | 21 ++++++++++----------- > 1 file changed, 10 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c > index 5e4fc9b4e2ad..613a60f24ba6 100644 > --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c > +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c > @@ -3808,6 +3808,9 @@ static s32 e1000_update_nvm_checksum_spt(struct e1000_hw *hw) > if (nvm->type != e1000_nvm_flash_sw) > goto out; > > + if (hw->mac.type >= e1000_pch_cnp) > + goto out; > + > nvm->ops.acquire(hw); > > /* We're writing to the opposite bank so if we're on bank 1, > @@ -4136,17 +4139,13 @@ static s32 e1000_validate_nvm_checksum_ich8lan(struct e1000_hw *hw) > return ret_val; > > if (!(data & valid_csum_mask)) { > - e_dbg("NVM Checksum Invalid\n"); > - > - if (hw->mac.type < e1000_pch_cnp) { I meant: change hw->mac.type < e1000_pch_cnp to hw->mac.type < e1000_pch_tgp > - 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; > - } Hello Thomas, For security reasons starting from the TGL platform SPI controller will be locked for SW access. I've double-checked with our HW architect, not from SPT, from TGP. So, first, we can change the mac type e1000_pch_cnp to e1000_pch_tgp (as fix for initial patch) Do we want (second) to allow HW initialization with the "wrong" NVM checksum? It could cause unexpected (HW) behavior in the future. Even if you will "recover" check in shadow RAM - there is no guarantee that NVM is good. > + 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); Thanks, Sasha
On Mon, Jan 31, 2022 at 12:51:07PM +0200, Neftin, Sasha wrote: > Hello Thomas, > For security reasons starting from the TGL platform SPI controller will be > locked for SW access. I've double-checked with our HW architect, not from > SPT, from TGP. So, first, we can change the mac type e1000_pch_cnp to > e1000_pch_tgp (as fix for initial patch) ok, that would fix the mentioned bug. Are you sending a patch for that ? > Do we want (second) to allow HW initialization with the "wrong" NVM > checksum? It could cause unexpected (HW) behavior in the future. Even if you > will "recover" check in shadow RAM - there is no guarantee that NVM is good. sure. Out of curiosity why is the NVM fixup there in the first place ? Thomas.
On 1/31/2022 18:41, Thomas Bogendoerfer wrote: > On Mon, Jan 31, 2022 at 12:51:07PM +0200, Neftin, Sasha wrote: >> Hello Thomas, >> For security reasons starting from the TGL platform SPI controller will be >> locked for SW access. I've double-checked with our HW architect, not from >> SPT, from TGP. So, first, we can change the mac type e1000_pch_cnp to >> e1000_pch_tgp (as fix for initial patch) > > ok, that would fix the mentioned bug. Are you sending a patch for that ? Sure. I will send patch for this and inform you > >> Do we want (second) to allow HW initialization with the "wrong" NVM >> checksum? It could cause unexpected (HW) behavior in the future. Even if you >> will "recover" check in shadow RAM - there is no guarantee that NVM is good. > > sure. Out of curiosity why is the NVM fixup there in the first place ? It is legacy implementation (many years ago). I believe the 'main idea' was to allow SW to fix checksum when somehow it was computed wrongly. (probably recover checksum calculation bug in NVM release process) > > Thomas. > Sasha
diff --git a/drivers/net/ethernet/intel/e1000e/ich8lan.c b/drivers/net/ethernet/intel/e1000e/ich8lan.c index 5e4fc9b4e2ad..613a60f24ba6 100644 --- a/drivers/net/ethernet/intel/e1000e/ich8lan.c +++ b/drivers/net/ethernet/intel/e1000e/ich8lan.c @@ -3808,6 +3808,9 @@ static s32 e1000_update_nvm_checksum_spt(struct e1000_hw *hw) if (nvm->type != e1000_nvm_flash_sw) goto out; + if (hw->mac.type >= e1000_pch_cnp) + goto out; + nvm->ops.acquire(hw); /* We're writing to the opposite bank so if we're on bank 1, @@ -4136,17 +4139,13 @@ static s32 e1000_validate_nvm_checksum_ich8lan(struct e1000_hw *hw) return ret_val; if (!(data & valid_csum_mask)) { - e_dbg("NVM Checksum Invalid\n"); - - if (hw->mac.type < e1000_pch_cnp) { - 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; - } + 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);
Commit 4051f68318ca ("e1000e: Do not take care about recovery NVM checksum") causes a regression for systems with a broken NVM checksum and hardware which is not able to update the NVM. Before the change the in-memory copy was correct even the NVM update doesn't work, which is good enough for the driver to work again. See https://bugzilla.opensuse.org/show_bug.cgi?id=1191663 for more details. This patch reverts the change and moves the check for hardware without NVM update capability right before the real flash update. Fixes: 4051f68318ca ("e1000e: Do not take care about recovery NVM checksum") Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> --- drivers/net/ethernet/intel/e1000e/ich8lan.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)