* [Intel-wired-lan] [PATCH] i40e: avoid NVM acquire deadlock during NVM update
@ 2017-09-01 20:42 Jacob Keller
2017-09-01 21:35 ` Bowers, AndrewX
2017-09-03 5:34 ` Stefan Assmann
0 siblings, 2 replies; 4+ messages in thread
From: Jacob Keller @ 2017-09-01 20:42 UTC (permalink / raw)
To: intel-wired-lan
From: Anjali Singhai Jain <anjali.singhai@intel.com>
X722 devices use the AdminQ to access the NVM, and this requires taking
the AdminQ lock. Because of this, we lock the AdminQ during
i40e_read_nvm(), which is also called in places where the lock is
already held, such as the firmware update path which wants to lock once
and then unlock when finished after performing several tasks.
Although this should have only affected X722 devices, commit
96a39aed25e6 ("i40e: Acquire NVM lock before reads on all devices",
2016-12-02) added locking for all NVM reads, regardless of device
family.
This resulted in us accidentally causing NVM acquire timeouts on all
devices, causing failed firmware updates which left the eeprom in
a corrupt state.
Create unsafe non-locked variants of i40e_read_nvm_word and
i40e_read_nvm_buffer, __i40e_read_nvm_word and __i40e_read_nvm_buffer
respectively. These variants will not take the NVM lock and are expected
to only be called in places where the NVM lock is already held if
needed.
Since the only caller of i40e_read_nvm_buffer() was in such a path,
remove it entirely in favor of the unsafe version. If necessary we can
always add it back in the future.
Additionally, we now need to hold the NVM lock in i40e_validate_checksum
because the call to i40e_calc_nvm_checksum now assumes that the NVM lock
is held. We can further move the call to read I40E_SR_SW_CHECKSUM_WORD
up a bit so that we do not need to acquire the NVM lock twice.
This should resolve firmware updates and also fix potential raise that
could have caused the driver to report an invalid NVM checksum upon
driver load.
Reported-by: Stefan Assmann <sassmann@kpanic.de>
Fixes: 96a39aed25e6 ("i40e: Acquire NVM lock before reads on all devices", 2016-12-02)
Signed-off-by: Anjali Singhai Jain <anjali.singhai@intel.com>
Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
This is a more complete version of the patch suggested by Stefan, found
at https://patchwork.ozlabs.org/patch/808699/ and it should be able to
avoid needing the 2nd patch in his series, which reverted a workqueue
change. The code is slightly refactored from what he suggested, and also
includes the i40e_nvm_read_buffer() function, as well as fixing up the
issues in i40e_validate_checksum().
Stefan, can you let me know if this works for you? I didn't have any
issues, but I'd like to confirm this fix is acceptable to you.
drivers/net/ethernet/intel/i40e/i40e_nvm.c | 102 ++++++++++++++---------
drivers/net/ethernet/intel/i40e/i40e_prototype.h | 2 -
2 files changed, 62 insertions(+), 42 deletions(-)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_nvm.c b/drivers/net/ethernet/intel/i40e/i40e_nvm.c
index 96afef98a08f..d92aedc278aa 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_nvm.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_nvm.c
@@ -283,7 +283,7 @@ static i40e_status i40e_read_nvm_aq(struct i40e_hw *hw, u8 module_pointer,
* @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF)
* @data: word read from the Shadow RAM
*
- * Reads one 16 bit word from the Shadow RAM using the GLNVM_SRCTL register.
+ * Reads one 16 bit word from the Shadow RAM using the AdminQ
**/
static i40e_status i40e_read_nvm_word_aq(struct i40e_hw *hw, u16 offset,
u16 *data)
@@ -297,27 +297,49 @@ static i40e_status i40e_read_nvm_word_aq(struct i40e_hw *hw, u16 offset,
}
/**
- * i40e_read_nvm_word - Reads Shadow RAM
+ * __i40e_read_nvm_word - Reads nvm word, assumes called does the locking
* @hw: pointer to the HW structure
* @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF)
* @data: word read from the Shadow RAM
*
- * Reads one 16 bit word from the Shadow RAM using the GLNVM_SRCTL register.
+ * Reads one 16 bit word from the Shadow RAM.
+ *
+ * Do not use this function except in cases where the nvm lock is already
+ * taken via i40e_acquire_nvm().
+ **/
+static i40e_status __i40e_read_nvm_word(struct i40e_hw *hw,
+ u16 offset, u16 *data)
+{
+ i40e_status ret_code = 0;
+
+ if (hw->flags & I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE)
+ ret_code = i40e_read_nvm_word_aq(hw, offset, data);
+ else
+ ret_code = i40e_read_nvm_word_srctl(hw, offset, data);
+ return ret_code;
+}
+
+/**
+ * i40e_read_nvm_word - Reads nvm word and acquire lock if necessary
+ * @hw: pointer to the HW structure
+ * @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF)
+ * @data: word read from the Shadow RAM
+ *
+ * Reads one 16 bit word from the Shadow RAM.
**/
i40e_status i40e_read_nvm_word(struct i40e_hw *hw, u16 offset,
- u16 *data)
+ u16 *data)
{
- enum i40e_status_code ret_code = 0;
+ i40e_status ret_code = 0;
ret_code = i40e_acquire_nvm(hw, I40E_RESOURCE_READ);
- if (!ret_code) {
- if (hw->flags & I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE) {
- ret_code = i40e_read_nvm_word_aq(hw, offset, data);
- } else {
- ret_code = i40e_read_nvm_word_srctl(hw, offset, data);
- }
- i40e_release_nvm(hw);
- }
+ if (ret_code)
+ return ret_code;
+
+ ret_code = __i40e_read_nvm_word(hw, offset, data);
+
+ i40e_release_nvm(hw);
+
return ret_code;
}
@@ -410,31 +432,25 @@ static i40e_status i40e_read_nvm_buffer_aq(struct i40e_hw *hw, u16 offset,
}
/**
- * i40e_read_nvm_buffer - Reads Shadow RAM buffer
+ * __i40e_read_nvm_buffer - Reads nvm buffer, caller must acquire lock
* @hw: pointer to the HW structure
* @offset: offset of the Shadow RAM word to read (0x000000 - 0x001FFF).
* @words: (in) number of words to read; (out) number of words actually read
* @data: words read from the Shadow RAM
*
* Reads 16 bit words (data buffer) from the SR using the i40e_read_nvm_srrd()
- * method. The buffer read is preceded by the NVM ownership take
- * and followed by the release.
+ * method.
**/
-i40e_status i40e_read_nvm_buffer(struct i40e_hw *hw, u16 offset,
- u16 *words, u16 *data)
+static i40e_status __i40e_read_nvm_buffer(struct i40e_hw *hw,
+ u16 offset, u16 *words,
+ u16 *data)
{
- enum i40e_status_code ret_code = 0;
+ i40e_status ret_code = 0;
- if (hw->flags & I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE) {
- ret_code = i40e_acquire_nvm(hw, I40E_RESOURCE_READ);
- if (!ret_code) {
- ret_code = i40e_read_nvm_buffer_aq(hw, offset, words,
- data);
- i40e_release_nvm(hw);
- }
- } else {
+ if (hw->flags & I40E_HW_FLAG_AQ_SRCTL_ACCESS_ENABLE)
+ ret_code = i40e_read_nvm_buffer_aq(hw, offset, words, data);
+ else
ret_code = i40e_read_nvm_buffer_srctl(hw, offset, words, data);
- }
return ret_code;
}
@@ -516,15 +532,15 @@ static i40e_status i40e_calc_nvm_checksum(struct i40e_hw *hw,
data = (u16 *)vmem.va;
/* read pointer to VPD area */
- ret_code = i40e_read_nvm_word(hw, I40E_SR_VPD_PTR, &vpd_module);
+ ret_code = __i40e_read_nvm_word(hw, I40E_SR_VPD_PTR, &vpd_module);
if (ret_code) {
ret_code = I40E_ERR_NVM_CHECKSUM;
goto i40e_calc_nvm_checksum_exit;
}
/* read pointer to PCIe Alt Auto-load module */
- ret_code = i40e_read_nvm_word(hw, I40E_SR_PCIE_ALT_AUTO_LOAD_PTR,
- &pcie_alt_module);
+ ret_code = __i40e_read_nvm_word(hw, I40E_SR_PCIE_ALT_AUTO_LOAD_PTR,
+ &pcie_alt_module);
if (ret_code) {
ret_code = I40E_ERR_NVM_CHECKSUM;
goto i40e_calc_nvm_checksum_exit;
@@ -538,7 +554,7 @@ static i40e_status i40e_calc_nvm_checksum(struct i40e_hw *hw,
if ((i % I40E_SR_SECTOR_SIZE_IN_WORDS) == 0) {
u16 words = I40E_SR_SECTOR_SIZE_IN_WORDS;
- ret_code = i40e_read_nvm_buffer(hw, i, &words, data);
+ ret_code = __i40e_read_nvm_buffer(hw, i, &words, data);
if (ret_code) {
ret_code = I40E_ERR_NVM_CHECKSUM;
goto i40e_calc_nvm_checksum_exit;
@@ -610,14 +626,19 @@ i40e_status i40e_validate_nvm_checksum(struct i40e_hw *hw,
u16 checksum_sr = 0;
u16 checksum_local = 0;
- ret_code = i40e_calc_nvm_checksum(hw, &checksum_local);
- if (ret_code)
- goto i40e_validate_nvm_checksum_exit;
-
- /* Do not use i40e_read_nvm_word() because we do not want to take
- * the synchronization semaphores twice here.
+ /* We must acquire the NVM lock in order to correctly synchronize the
+ * NVM accesses across multiple PFs. Without doing so it is possible
+ * for one of the PFs to read invalid data potentially indicating that
+ * the checksum is invalid.
*/
- i40e_read_nvm_word(hw, I40E_SR_SW_CHECKSUM_WORD, &checksum_sr);
+ ret_code = i40e_acquire_nvm(hw, I40E_RESOURCE_READ);
+ if (ret_code)
+ return ret_code;
+ ret_code = i40e_calc_nvm_checksum(hw, &checksum_local);
+ __i40e_read_nvm_word(hw, I40E_SR_SW_CHECKSUM_WORD, &checksum_sr);
+ i40e_release_nvm(hw);
+ if (ret_code)
+ return ret_code;
/* Verify read checksum from EEPROM is the same as
* calculated checksum
@@ -629,7 +650,6 @@ i40e_status i40e_validate_nvm_checksum(struct i40e_hw *hw,
if (checksum)
*checksum = checksum_local;
-i40e_validate_nvm_checksum_exit:
return ret_code;
}
@@ -1026,6 +1046,7 @@ static i40e_status i40e_nvmupd_state_writing(struct i40e_hw *hw,
break;
case I40E_NVMUPD_CSUM_CON:
+ /* Assumes the caller has acquired the nvm */
status = i40e_update_nvm_checksum(hw);
if (status) {
*perrno = hw->aq.asq_last_status ?
@@ -1040,6 +1061,7 @@ static i40e_status i40e_nvmupd_state_writing(struct i40e_hw *hw,
break;
case I40E_NVMUPD_CSUM_LCB:
+ /* Assumes the caller has acquired the nvm */
status = i40e_update_nvm_checksum(hw);
if (status) {
*perrno = hw->aq.asq_last_status ?
diff --git a/drivers/net/ethernet/intel/i40e/i40e_prototype.h b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
index 6254ad5167eb..01502561035c 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_prototype.h
+++ b/drivers/net/ethernet/intel/i40e/i40e_prototype.h
@@ -311,8 +311,6 @@ i40e_status i40e_acquire_nvm(struct i40e_hw *hw,
void i40e_release_nvm(struct i40e_hw *hw);
i40e_status i40e_read_nvm_word(struct i40e_hw *hw, u16 offset,
u16 *data);
-i40e_status i40e_read_nvm_buffer(struct i40e_hw *hw, u16 offset,
- u16 *words, u16 *data);
i40e_status i40e_update_nvm_checksum(struct i40e_hw *hw);
i40e_status i40e_validate_nvm_checksum(struct i40e_hw *hw,
u16 *checksum);
--
2.14.1.436.g33e61a4f0239
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [Intel-wired-lan] [PATCH] i40e: avoid NVM acquire deadlock during NVM update
2017-09-01 20:42 [Intel-wired-lan] [PATCH] i40e: avoid NVM acquire deadlock during NVM update Jacob Keller
@ 2017-09-01 21:35 ` Bowers, AndrewX
2017-09-03 5:34 ` Stefan Assmann
1 sibling, 0 replies; 4+ messages in thread
From: Bowers, AndrewX @ 2017-09-01 21:35 UTC (permalink / raw)
To: intel-wired-lan
> -----Original Message-----
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces at osuosl.org] On
> Behalf Of Jacob Keller
> Sent: Friday, September 1, 2017 1:43 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Cc: Stefan Assmann <sassmann@kpanic.de>
> Subject: [Intel-wired-lan] [PATCH] i40e: avoid NVM acquire deadlock during
> NVM update
>
> From: Anjali Singhai Jain <anjali.singhai@intel.com>
>
> X722 devices use the AdminQ to access the NVM, and this requires taking the
> AdminQ lock. Because of this, we lock the AdminQ during i40e_read_nvm(),
> which is also called in places where the lock is already held, such as the
> firmware update path which wants to lock once and then unlock when
> finished after performing several tasks.
>
> Although this should have only affected X722 devices, commit
> 96a39aed25e6 ("i40e: Acquire NVM lock before reads on all devices",
> 2016-12-02) added locking for all NVM reads, regardless of device family.
>
> This resulted in us accidentally causing NVM acquire timeouts on all devices,
> causing failed firmware updates which left the eeprom in a corrupt state.
>
> Create unsafe non-locked variants of i40e_read_nvm_word and
> i40e_read_nvm_buffer, __i40e_read_nvm_word and
> __i40e_read_nvm_buffer respectively. These variants will not take the NVM
> lock and are expected to only be called in places where the NVM lock is
> already held if needed.
>
> Since the only caller of i40e_read_nvm_buffer() was in such a path, remove
> it entirely in favor of the unsafe version. If necessary we can always add it
> back in the future.
>
> Additionally, we now need to hold the NVM lock in i40e_validate_checksum
> because the call to i40e_calc_nvm_checksum now assumes that the NVM
> lock is held. We can further move the call to read
> I40E_SR_SW_CHECKSUM_WORD up a bit so that we do not need to acquire
> the NVM lock twice.
>
> This should resolve firmware updates and also fix potential raise that could
> have caused the driver to report an invalid NVM checksum upon driver load.
>
> Reported-by: Stefan Assmann <sassmann@kpanic.de>
> Fixes: 96a39aed25e6 ("i40e: Acquire NVM lock before reads on all devices",
> 2016-12-02)
> Signed-off-by: Anjali Singhai Jain <anjali.singhai@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>
> This is a more complete version of the patch suggested by Stefan, found at
> https://patchwork.ozlabs.org/patch/808699/ and it should be able to avoid
> needing the 2nd patch in his series, which reverted a workqueue change.
> The code is slightly refactored from what he suggested, and also includes the
> i40e_nvm_read_buffer() function, as well as fixing up the issues in
> i40e_validate_checksum().
>
> Stefan, can you let me know if this works for you? I didn't have any issues,
> but I'd like to confirm this fix is acceptable to you.
>
> drivers/net/ethernet/intel/i40e/i40e_nvm.c | 102 ++++++++++++++------
> ---
> drivers/net/ethernet/intel/i40e/i40e_prototype.h | 2 -
> 2 files changed, 62 insertions(+), 42 deletions(-)
Tested-by: Andrew Bowers <andrewx.bowers@intel.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Intel-wired-lan] [PATCH] i40e: avoid NVM acquire deadlock during NVM update
2017-09-01 20:42 [Intel-wired-lan] [PATCH] i40e: avoid NVM acquire deadlock during NVM update Jacob Keller
2017-09-01 21:35 ` Bowers, AndrewX
@ 2017-09-03 5:34 ` Stefan Assmann
2017-09-05 22:00 ` Keller, Jacob E
1 sibling, 1 reply; 4+ messages in thread
From: Stefan Assmann @ 2017-09-03 5:34 UTC (permalink / raw)
To: intel-wired-lan
On 01.09.2017 22:42, Jacob Keller wrote:
> From: Anjali Singhai Jain <anjali.singhai@intel.com>
>
> X722 devices use the AdminQ to access the NVM, and this requires taking
> the AdminQ lock. Because of this, we lock the AdminQ during
> i40e_read_nvm(), which is also called in places where the lock is
> already held, such as the firmware update path which wants to lock once
> and then unlock when finished after performing several tasks.
>
> Although this should have only affected X722 devices, commit
> 96a39aed25e6 ("i40e: Acquire NVM lock before reads on all devices",
> 2016-12-02) added locking for all NVM reads, regardless of device
> family.
>
> This resulted in us accidentally causing NVM acquire timeouts on all
> devices, causing failed firmware updates which left the eeprom in
> a corrupt state.
>
> Create unsafe non-locked variants of i40e_read_nvm_word and
> i40e_read_nvm_buffer, __i40e_read_nvm_word and __i40e_read_nvm_buffer
> respectively. These variants will not take the NVM lock and are expected
> to only be called in places where the NVM lock is already held if
> needed.
>
> Since the only caller of i40e_read_nvm_buffer() was in such a path,
> remove it entirely in favor of the unsafe version. If necessary we can
> always add it back in the future.
>
> Additionally, we now need to hold the NVM lock in i40e_validate_checksum
> because the call to i40e_calc_nvm_checksum now assumes that the NVM lock
> is held. We can further move the call to read I40E_SR_SW_CHECKSUM_WORD
> up a bit so that we do not need to acquire the NVM lock twice.
>
> This should resolve firmware updates and also fix potential raise that
> could have caused the driver to report an invalid NVM checksum upon
> driver load.
>
> Reported-by: Stefan Assmann <sassmann@kpanic.de>
> Fixes: 96a39aed25e6 ("i40e: Acquire NVM lock before reads on all devices", 2016-12-02)
> Signed-off-by: Anjali Singhai Jain <anjali.singhai@intel.com>
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>
> This is a more complete version of the patch suggested by Stefan, found
> at https://patchwork.ozlabs.org/patch/808699/ and it should be able to
> avoid needing the 2nd patch in his series, which reverted a workqueue
> change. The code is slightly refactored from what he suggested, and also
> includes the i40e_nvm_read_buffer() function, as well as fixing up the
> issues in i40e_validate_checksum().
>
> Stefan, can you let me know if this works for you? I didn't have any
> issues, but I'd like to confirm this fix is acceptable to you.
Hi Jacob,
I've reflashed all the NICs I had available, everything went smoothly.
Let's get your patch in asap.
Thanks!
Stefan
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Intel-wired-lan] [PATCH] i40e: avoid NVM acquire deadlock during NVM update
2017-09-03 5:34 ` Stefan Assmann
@ 2017-09-05 22:00 ` Keller, Jacob E
0 siblings, 0 replies; 4+ messages in thread
From: Keller, Jacob E @ 2017-09-05 22:00 UTC (permalink / raw)
To: intel-wired-lan
> -----Original Message-----
> From: Stefan Assmann [mailto:sassmann at kpanic.de]
> Sent: Saturday, September 02, 2017 10:35 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>; Intel Wired LAN <intel-wired-
> lan at lists.osuosl.org>
> Cc: Singhai, Anjali <anjali.singhai@intel.com>
> Subject: Re: [PATCH] i40e: avoid NVM acquire deadlock during NVM update
>
> On 01.09.2017 22:42, Jacob Keller wrote:
> > From: Anjali Singhai Jain <anjali.singhai@intel.com>
> >
> > X722 devices use the AdminQ to access the NVM, and this requires taking
> > the AdminQ lock. Because of this, we lock the AdminQ during
> > i40e_read_nvm(), which is also called in places where the lock is
> > already held, such as the firmware update path which wants to lock once
> > and then unlock when finished after performing several tasks.
> >
> > Although this should have only affected X722 devices, commit
> > 96a39aed25e6 ("i40e: Acquire NVM lock before reads on all devices",
> > 2016-12-02) added locking for all NVM reads, regardless of device
> > family.
> >
> > This resulted in us accidentally causing NVM acquire timeouts on all
> > devices, causing failed firmware updates which left the eeprom in
> > a corrupt state.
> >
> > Create unsafe non-locked variants of i40e_read_nvm_word and
> > i40e_read_nvm_buffer, __i40e_read_nvm_word and __i40e_read_nvm_buffer
> > respectively. These variants will not take the NVM lock and are expected
> > to only be called in places where the NVM lock is already held if
> > needed.
> >
> > Since the only caller of i40e_read_nvm_buffer() was in such a path,
> > remove it entirely in favor of the unsafe version. If necessary we can
> > always add it back in the future.
> >
> > Additionally, we now need to hold the NVM lock in i40e_validate_checksum
> > because the call to i40e_calc_nvm_checksum now assumes that the NVM lock
> > is held. We can further move the call to read I40E_SR_SW_CHECKSUM_WORD
> > up a bit so that we do not need to acquire the NVM lock twice.
> >
> > This should resolve firmware updates and also fix potential raise that
> > could have caused the driver to report an invalid NVM checksum upon
> > driver load.
> >
> > Reported-by: Stefan Assmann <sassmann@kpanic.de>
> > Fixes: 96a39aed25e6 ("i40e: Acquire NVM lock before reads on all devices", 2016-
> 12-02)
> > Signed-off-by: Anjali Singhai Jain <anjali.singhai@intel.com>
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > ---
> >
> > This is a more complete version of the patch suggested by Stefan, found
> > at https://patchwork.ozlabs.org/patch/808699/ and it should be able to
> > avoid needing the 2nd patch in his series, which reverted a workqueue
> > change. The code is slightly refactored from what he suggested, and also
> > includes the i40e_nvm_read_buffer() function, as well as fixing up the
> > issues in i40e_validate_checksum().
> >
> > Stefan, can you let me know if this works for you? I didn't have any
> > issues, but I'd like to confirm this fix is acceptable to you.
>
> Hi Jacob,
>
> I've reflashed all the NICs I had available, everything went smoothly.
> Let's get your patch in asap.
>
> Thanks!
>
> Stefan
Excellent news!
Thanks,
Jake
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-09-05 22:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-01 20:42 [Intel-wired-lan] [PATCH] i40e: avoid NVM acquire deadlock during NVM update Jacob Keller
2017-09-01 21:35 ` Bowers, AndrewX
2017-09-03 5:34 ` Stefan Assmann
2017-09-05 22:00 ` Keller, Jacob E
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox