From: Robert Malz via Intel-wired-lan <hidden> Date: 2026-06-17 12:08:06
When the admin queue command to read NVM returns EBUSY, the driver
currently treats it as a fatal error and aborts the entire read
operation. This can cause spurious NVM read failures during periods of
high firmware activity.
Add retry logic to ice_read_flat_nvm() that handles EBUSY responses
from the admin queue. When an EBUSY error is encountered, release the
NVM resource lock, wait for ICE_SQ_SEND_DELAY_TIME_MS, re-acquire it,
and retry the failed read. The retry is attempted up to
ICE_SQ_SEND_MAX_EXECUTE times before giving up.
Code was extracted from OOT ice driver 1.15.4 release. Additional
change was made to reset last_cmd in case of retry to make sure that
all commands are retried properly.
Fixes: e94509906d6b ("ice: create function to read a section of the NVM and Shadow RAM")
Signed-off-by: Robert Malz <redacted>
---
Changes in v2:
- change ICE_AQ_RC_EBUSY -> LIBIE_AQ_RC_EBUSY
Changes in v3:
- resending to comply with the netdev 24-hour rule. No code changes since v2.
drivers/net/ethernet/intel/ice/ice_nvm.c | 25 +++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
From: Simon Horman <horms@kernel.org> Date: 2026-06-18 15:20:40
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
ice: retry reading NVM if admin queue returns EBUSY
When the admin queue command to read NVM returns EBUSY, the driver
currently treats it as a fatal error and aborts the entire read operation.
This patch adds retry logic to handle this condition.
[Severity: Low]
Does this check allow an extra retry execution? Since retry_cnt starts at 0,
using a strictly greater-than check allows the loop to retry 4 times
(5 total executions) instead of the 3 executions intended by
ICE_SQ_SEND_MAX_EXECUTE.
[Severity: High]
Does dropping the NVM lock here violate the caller's assumption of mutual
exclusion?
Callers of ice_read_flat_nvm() typically acquire the NVM lock beforehand to
ensure an atomic read of multiple sectors without interleaved writes. By
explicitly dropping the lock during the msleep() window, another thread, VF,
or PF could acquire the NVM lock and modify the NVM before the read resumes,
potentially resulting in reading torn or inconsistent NVM data.
+ status = ice_acquire_nvm(hw, ICE_RES_READ);
+ if (status)
+ break;
[Severity: High]
Can this lead to an unbalanced NVM resource lock release and potential data
corruption?
If ice_acquire_nvm() fails here, the loop breaks and returns an error with the
lock currently unheld. However, callers like ice_read_flash_module() are
unaware of this internal lock state change and will unconditionally call
ice_release_nvm() upon failure:
drivers/net/ethernet/intel/ice/ice_nvm.c:ice_read_flash_module() {
status = ice_acquire_nvm(hw, ICE_RES_READ);
if (status)
return status;
status = ice_read_flat_nvm(hw, start + offset, &length, data, false);
ice_release_nvm(hw);
...
Because firmware tracks resource locks per-PF rather than per-OS-thread, this
errant second release could forcefully strip the lock from another thread on
the same PF that successfully acquired it, exposing the NVM to concurrent
modification.
From: Robert Malz via Intel-wired-lan <hidden> Date: 2026-06-24 11:15:55
Hey Simon,
Thanks for the review. I'll try to address these comments one by one.
On Thu, Jun 18, 2026 at 5:20 PM Simon Horman [off-list ref] wrote:
This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
ice: retry reading NVM if admin queue returns EBUSY
When the admin queue command to read NVM returns EBUSY, the driver
currently treats it as a fatal error and aborts the entire read operation.
This patch adds retry logic to handle this condition.
[Severity: Low]
Does this check allow an extra retry execution? Since retry_cnt starts at 0,
using a strictly greater-than check allows the loop to retry 4 times
(5 total executions) instead of the 3 executions intended by
ICE_SQ_SEND_MAX_EXECUTE.
The review is correct that this allows one extra execution. However, I
intentionally kept this verbatim from the OOT driver (1.15.4) to
retain the significant test exposure it has there. The absolute worst
case is one harmless extra retry before giving up. If you strongly
prefer strict adherence to ICE_SQ_SEND_MAX_EXECUTE, I can update the
condition in a v4.
[Severity: High]
Does dropping the NVM lock here violate the caller's assumption of mutual
exclusion?
Callers of ice_read_flat_nvm() typically acquire the NVM lock beforehand to
ensure an atomic read of multiple sectors without interleaved writes. By
explicitly dropping the lock during the msleep() window, another thread, VF,
or PF could acquire the NVM lock and modify the NVM before the read resumes,
potentially resulting in reading torn or inconsistent NVM data.
quoted
+ status = ice_acquire_nvm(hw, ICE_RES_READ);
+ if (status)
+ break;
I don't think this is an issue. The NVM resource (req_res 0x0008) is
an ownership lease, not a content snapshot. The FW grants it with a
hold timeout (see the timeout field handling in ice_aq_req_res). It
never guaranteed a point-in-time view of the flash.
Very similar behavior already exists in ice_devlink_nvm_snapshot(),
which reads the NVM in chunks via ice_read_flat_nvm() and deliberately
acquires/releases the lock between chunks (with a comment explaining
the FW reclaim).
Additionally, the lock is only dropped on the failure path, where
neither bytes_read nor offset have been advanced. The driver simply
re-reads the failed chunk after re-acquiring. So no data that is
returned to the caller is committed across the drop window.
Overall the lock drop does not weaken any guarantee callers actually
have. If we wanted to introduce a true snapshot guarantee, that would
probably require different approach and separate patch.
[Severity: High]
Can this lead to an unbalanced NVM resource lock release and potential data
corruption?
If ice_acquire_nvm() fails here, the loop breaks and returns an error with the
lock currently unheld. However, callers like ice_read_flash_module() are
unaware of this internal lock state change and will unconditionally call
ice_release_nvm() upon failure:
drivers/net/ethernet/intel/ice/ice_nvm.c:ice_read_flash_module() {
status = ice_acquire_nvm(hw, ICE_RES_READ);
if (status)
return status;
status = ice_read_flat_nvm(hw, start + offset, &length, data, false);
ice_release_nvm(hw);
...
Because firmware tracks resource locks per-PF rather than per-OS-thread, this
errant second release could forcefully strip the lock from another thread on
the same PF that successfully acquired it, exposing the NVM to concurrent
modification.
Agreed, this might be a real bug, and the one of the three I think is
worth investigating.
If ice_acquire_nvm() fails after the drop, ice_read_flat_nvm() returns
with the lock unheld while callers unconditionally call
ice_release_nvm(), so a stray release is issued.
On probability, though, the window is very small. Reaching it requires
sustained EBUSY across the retry budget plus a failed re-acquire
(which itself polls up to ICE_NVM_TIMEOUT), and concurrently another
requester taking the lock. Most reads happen during init (ice_probe,
and reset/rebuild via ice_init_nvm), and NVM writes only happen on an
already initialized driver. The devlink/ethtool nvm_read paths are
also exposed, but hitting this race would require precise timing
against a concurrent NVM owner on the device.
I'd prefer to keep the scope of this patch limited to the EBUSY retry
path and not take on the unbalanced-release fix here. A proper fix
should change the lock-ownership contract of ice_read_flat_nvm() (on
error, the lock must be released by ice_read_flat_nvm(), callers
release only on success) and update all callers. Code change sould be
simple for all callers but ice_discover_flash_size(), it intentionally
holds one lease across a read loop and would need to re-acquire after
each expected boundary failure.
Given how small the original window is, I'd rather not trade tested
OOT behavior for the risk of a complex unbalanced NVM lock fix. I
actually have a patch mostly ready that fixes the lock-ownership
contract, but I really don't like it. It changes the design of
ice_read_flat_nvm(), making it less intuitive for callers. More
importantly, I just don't have the resources or test coverage right
now to properly verify such change.
However, I can modify the failure path for ice_acquire_nvm inside
ice_read_flat_nvm. Instead of bailing out immediately, we can just
retry it within the existing retry budget. In this case, the
probability of leaving ice_read_flat_nvm without holding the lock is
reduced even further without needing a refactor.
Please let me know what you think about my thought process on this.
This is an AI-generated review of your patch. The human sending this
quoted
[Severity: Low]
Does this check allow an extra retry execution? Since retry_cnt starts at 0,
using a strictly greater-than check allows the loop to retry 4 times
(5 total executions) instead of the 3 executions intended by
ICE_SQ_SEND_MAX_EXECUTE.
The review is correct that this allows one extra execution. However, I
intentionally kept this verbatim from the OOT driver (1.15.4) to
This OOT change was made entirely by Grzegorz, for E825C. Would be great
to acknowledge his authorship (+ SoB ofc, below).
Signed-off-by: Grzegorz Nitka <redacted>
What was the platform that you have encountered the bug on?
retain the significant test exposure it has there. The absolute worst
case is one harmless extra retry before giving up. If you strongly
prefer strict adherence to ICE_SQ_SEND_MAX_EXECUTE, I can update the
condition in a v4.
Please update (to have one attempt less).
quoted
[Severity: High]
Does dropping the NVM lock here violate the caller's assumption of mutual
exclusion?
Callers of ice_read_flat_nvm() typically acquire the NVM lock beforehand to
ensure an atomic read of multiple sectors without interleaved writes. By
explicitly dropping the lock during the msleep() window, another thread, VF,
or PF could acquire the NVM lock and modify the NVM before the read resumes,
potentially resulting in reading torn or inconsistent NVM data.
quoted
+ status = ice_acquire_nvm(hw, ICE_RES_READ);
+ if (status)
+ break;
I don't think this is an issue. The NVM resource (req_res 0x0008) is
an ownership lease, not a content snapshot. The FW grants it with a
hold timeout (see the timeout field handling in ice_aq_req_res). It
never guaranteed a point-in-time view of the flash.
Very similar behavior already exists in ice_devlink_nvm_snapshot(),
which reads the NVM in chunks via ice_read_flat_nvm() and deliberately
acquires/releases the lock between chunks (with a comment explaining
the FW reclaim).
Additionally, the lock is only dropped on the failure path, where
neither bytes_read nor offset have been advanced. The driver simply
re-reads the failed chunk after re-acquiring. So no data that is
returned to the caller is committed across the drop window.
Overall the lock drop does not weaken any guarantee callers actually
have. If we wanted to introduce a true snapshot guarantee, that would
probably require different approach and separate patch.
quoted
[Severity: High]
Can this lead to an unbalanced NVM resource lock release and potential data
corruption?
If ice_acquire_nvm() fails here, the loop breaks and returns an error with the
lock currently unheld. However, callers like ice_read_flash_module() are
unaware of this internal lock state change and will unconditionally call
ice_release_nvm() upon failure:
drivers/net/ethernet/intel/ice/ice_nvm.c:ice_read_flash_module() {
status = ice_acquire_nvm(hw, ICE_RES_READ);
if (status)
return status;
status = ice_read_flat_nvm(hw, start + offset, &length, data, false);
ice_release_nvm(hw);
...
Because firmware tracks resource locks per-PF rather than per-OS-thread, this
errant second release could forcefully strip the lock from another thread on
the same PF that successfully acquired it, exposing the NVM to concurrent
modification.
Agreed, this might be a real bug, and the one of the three I think is
worth investigating.
If ice_acquire_nvm() fails after the drop, ice_read_flat_nvm() returns
with the lock unheld while callers unconditionally call
ice_release_nvm(), so a stray release is issued.
On probability, though, the window is very small. Reaching it requires
sustained EBUSY across the retry budget plus a failed re-acquire
(which itself polls up to ICE_NVM_TIMEOUT), and concurrently another
requester taking the lock. Most reads happen during init (ice_probe,
and reset/rebuild via ice_init_nvm), and NVM writes only happen on an
already initialized driver. The devlink/ethtool nvm_read paths are
also exposed, but hitting this race would require precise timing
against a concurrent NVM owner on the device.
I'd prefer to keep the scope of this patch limited to the EBUSY retry
path and not take on the unbalanced-release fix here. A proper fix
should change the lock-ownership contract of ice_read_flat_nvm() (on
error, the lock must be released by ice_read_flat_nvm(), callers
release only on success) and update all callers. Code change sould be
simple for all callers but ice_discover_flash_size(), it intentionally
holds one lease across a read loop and would need to re-acquire after
each expected boundary failure.
Given how small the original window is, I'd rather not trade tested
OOT behavior for the risk of a complex unbalanced NVM lock fix. I
actually have a patch mostly ready that fixes the lock-ownership
contract, but I really don't like it. It changes the design of
ice_read_flat_nvm(), making it less intuitive for callers. More
importantly, I just don't have the resources or test coverage right
now to properly verify such change.
However, I can modify the failure path for ice_acquire_nvm inside
ice_read_flat_nvm. Instead of bailing out immediately, we can just
retry it within the existing retry budget. In this case, the
probability of leaving ice_read_flat_nvm without holding the lock is
reduced even further without needing a refactor.
Please let me know what you think about my thought process on this.
I think that both AI-reported issues against the lock are valid
concerns.
I think that sleep was the actual fix, and re-locking were merely
a necessity due to their expiration (as you said).
A proper fix would be to just increase lock-timeout to accommodate all
attempts (and still do the retries&sleep, but without unlocking).
From: Robert Malz via Intel-wired-lan <hidden> Date: 2026-06-25 10:14:56
Hey Przemek,
Thanks a lot for the feedback.
I was sure that we use ICE_NVM_TIMEOUT (180s) as a timeout every time
(ice_acquire_nvm) but your proposal made me rethink it a little.
First of all, the datasheet for E810 specifies the timeout as: "As an
input, the software might specify timeout longer than the default
taken for this resource, and up to one minute."
180s is greater than one minute so I took a look into AQC logs:
[ 110.698471] ice 0000:05:00.0: CQ CMD: opcode 0x0008, flags 0x2000,
datalen 0x0000, retval 0x0000
[ 110.698474] ice 0000:05:00.0: cookie (h,l) 0x00000000 0x00000000
[ 110.698477] ice 0000:05:00.0: param (0,1) 0x00010001 0x0002BF20
[ 110.698480] ice 0000:05:00.0: addr (h,l) 0x00000000 0x00000000
[ 110.698645] ice 0000:05:00.0: ATQ: desc and buffer writeback:
[ 110.698648] ice 0000:05:00.0: CQ CMD: opcode 0x0008, flags 0x2003,
datalen 0x0000, retval 0x0000
[ 110.698651] ice 0000:05:00.0: cookie (h,l) 0x00000000 0x00000000
[ 110.698654] ice 0000:05:00.0: param (0,1) 0x00010001 0x00000BB8
[ 110.698657] ice 0000:05:00.0: addr (h,l) 0x00000000 0x00000000
Based on the above, the driver requested a 0x0002BF20 timeout (180 000
ms) but the FW returned only 0x00000BB8 (3s).
I'm assuming this is expected behavior since the maximum timeout for
NVM read should be 60,000 ms.
If changing the timeout requested by the driver to 60s for read ops is
handled correctly by the FW and the FW respects that lock, the retry
patch submitted in this email thread might not be required at all.
Let me quickly prepare a new patch and test it. I'll update this
thread once I have a results.
Regards,
Robert
On Thu, Jun 25, 2026 at 9:51 AM Przemek Kitszel
[off-list ref] wrote:
quoted
quoted
This is an AI-generated review of your patch. The human sending this
quoted
quoted
[Severity: Low]
Does this check allow an extra retry execution? Since retry_cnt starts at 0,
using a strictly greater-than check allows the loop to retry 4 times
(5 total executions) instead of the 3 executions intended by
ICE_SQ_SEND_MAX_EXECUTE.
The review is correct that this allows one extra execution. However, I
intentionally kept this verbatim from the OOT driver (1.15.4) to
This OOT change was made entirely by Grzegorz, for E825C. Would be great
to acknowledge his authorship (+ SoB ofc, below).
Signed-off-by: Grzegorz Nitka <redacted>
What was the platform that you have encountered the bug on?
quoted
retain the significant test exposure it has there. The absolute worst
case is one harmless extra retry before giving up. If you strongly
prefer strict adherence to ICE_SQ_SEND_MAX_EXECUTE, I can update the
condition in a v4.
Please update (to have one attempt less).
quoted
quoted
[Severity: High]
Does dropping the NVM lock here violate the caller's assumption of mutual
exclusion?
Callers of ice_read_flat_nvm() typically acquire the NVM lock beforehand to
ensure an atomic read of multiple sectors without interleaved writes. By
explicitly dropping the lock during the msleep() window, another thread, VF,
or PF could acquire the NVM lock and modify the NVM before the read resumes,
potentially resulting in reading torn or inconsistent NVM data.
quoted
+ status = ice_acquire_nvm(hw, ICE_RES_READ);
+ if (status)
+ break;
I don't think this is an issue. The NVM resource (req_res 0x0008) is
an ownership lease, not a content snapshot. The FW grants it with a
hold timeout (see the timeout field handling in ice_aq_req_res). It
never guaranteed a point-in-time view of the flash.
Very similar behavior already exists in ice_devlink_nvm_snapshot(),
which reads the NVM in chunks via ice_read_flat_nvm() and deliberately
acquires/releases the lock between chunks (with a comment explaining
the FW reclaim).
Additionally, the lock is only dropped on the failure path, where
neither bytes_read nor offset have been advanced. The driver simply
re-reads the failed chunk after re-acquiring. So no data that is
returned to the caller is committed across the drop window.
Overall the lock drop does not weaken any guarantee callers actually
have. If we wanted to introduce a true snapshot guarantee, that would
probably require different approach and separate patch.
quoted
[Severity: High]
Can this lead to an unbalanced NVM resource lock release and potential data
corruption?
If ice_acquire_nvm() fails here, the loop breaks and returns an error with the
lock currently unheld. However, callers like ice_read_flash_module() are
unaware of this internal lock state change and will unconditionally call
ice_release_nvm() upon failure:
drivers/net/ethernet/intel/ice/ice_nvm.c:ice_read_flash_module() {
status = ice_acquire_nvm(hw, ICE_RES_READ);
if (status)
return status;
status = ice_read_flat_nvm(hw, start + offset, &length, data, false);
ice_release_nvm(hw);
...
Because firmware tracks resource locks per-PF rather than per-OS-thread, this
errant second release could forcefully strip the lock from another thread on
the same PF that successfully acquired it, exposing the NVM to concurrent
modification.
Agreed, this might be a real bug, and the one of the three I think is
worth investigating.
If ice_acquire_nvm() fails after the drop, ice_read_flat_nvm() returns
with the lock unheld while callers unconditionally call
ice_release_nvm(), so a stray release is issued.
On probability, though, the window is very small. Reaching it requires
sustained EBUSY across the retry budget plus a failed re-acquire
(which itself polls up to ICE_NVM_TIMEOUT), and concurrently another
requester taking the lock. Most reads happen during init (ice_probe,
and reset/rebuild via ice_init_nvm), and NVM writes only happen on an
already initialized driver. The devlink/ethtool nvm_read paths are
also exposed, but hitting this race would require precise timing
against a concurrent NVM owner on the device.
I'd prefer to keep the scope of this patch limited to the EBUSY retry
path and not take on the unbalanced-release fix here. A proper fix
should change the lock-ownership contract of ice_read_flat_nvm() (on
error, the lock must be released by ice_read_flat_nvm(), callers
release only on success) and update all callers. Code change sould be
simple for all callers but ice_discover_flash_size(), it intentionally
holds one lease across a read loop and would need to re-acquire after
each expected boundary failure.
Given how small the original window is, I'd rather not trade tested
OOT behavior for the risk of a complex unbalanced NVM lock fix. I
actually have a patch mostly ready that fixes the lock-ownership
contract, but I really don't like it. It changes the design of
ice_read_flat_nvm(), making it less intuitive for callers. More
importantly, I just don't have the resources or test coverage right
now to properly verify such change.
However, I can modify the failure path for ice_acquire_nvm inside
ice_read_flat_nvm. Instead of bailing out immediately, we can just
retry it within the existing retry budget. In this case, the
probability of leaving ice_read_flat_nvm without holding the lock is
reduced even further without needing a refactor.
Please let me know what you think about my thought process on this.
I think that both AI-reported issues against the lock are valid
concerns.
I think that sleep was the actual fix, and re-locking were merely
a necessity due to their expiration (as you said).
A proper fix would be to just increase lock-timeout to accommodate all
attempts (and still do the retries&sleep, but without unlocking).
From: Robert Malz via Intel-wired-lan <hidden> Date: 2026-06-26 08:15:50
Hey Przemek,
I ran some tests and unfortunately, the following sentence from the
datasheet is true:
"For specific resources, such as Change Lock (0x0003) and Global Config Lock
(0x0004), this field is used by software to override the default timeout for the
operation, and also to specify the timeout used for this operation."
This means we can only change a default timeout for 0x0003 and 0x0004
but not for 0x0001 (NVM resource).
Whatever timeout I provide FW defaults to 0xB88
Input:
[ 2209.656758] ice 0000:31:00.0: CQ CMD: opcode 0x0008, flags 0x2000,
datalen 0x0000, retval 0x0000
[ 2209.656760] ice 0000:31:00.0: cookie (h,l) 0x00000000 0x00000000
[ 2209.656761] ice 0000:31:00.0: param (0,1) 0x00010001 0x00000BB9
Output:
[ 2209.656927] ice 0000:31:00.0: CQ CMD: opcode 0x0008, flags 0x2003,
datalen 0x0000, retval 0x0000
[ 2209.656929] ice 0000:31:00.0: cookie (h,l) 0x00000000 0x00000000
[ 2209.656931] ice 0000:31:00.0: param (0,1) 0x00010001 0x00000BB8
Correct me If I'm wrong, but the only way to properly handle it is to
ensure the resource is locked and released between every
ice_acquire_nvm call.
I'll start working on this.
Regards,
Robert
On Thu, Jun 25, 2026 at 12:14 PM Robert Malz [off-list ref] wrote:
Hey Przemek,
Thanks a lot for the feedback.
I was sure that we use ICE_NVM_TIMEOUT (180s) as a timeout every time
(ice_acquire_nvm) but your proposal made me rethink it a little.
First of all, the datasheet for E810 specifies the timeout as: "As an
input, the software might specify timeout longer than the default
taken for this resource, and up to one minute."
180s is greater than one minute so I took a look into AQC logs:
[ 110.698471] ice 0000:05:00.0: CQ CMD: opcode 0x0008, flags 0x2000,
datalen 0x0000, retval 0x0000
[ 110.698474] ice 0000:05:00.0: cookie (h,l) 0x00000000 0x00000000
[ 110.698477] ice 0000:05:00.0: param (0,1) 0x00010001 0x0002BF20
[ 110.698480] ice 0000:05:00.0: addr (h,l) 0x00000000 0x00000000
[ 110.698645] ice 0000:05:00.0: ATQ: desc and buffer writeback:
[ 110.698648] ice 0000:05:00.0: CQ CMD: opcode 0x0008, flags 0x2003,
datalen 0x0000, retval 0x0000
[ 110.698651] ice 0000:05:00.0: cookie (h,l) 0x00000000 0x00000000
[ 110.698654] ice 0000:05:00.0: param (0,1) 0x00010001 0x00000BB8
[ 110.698657] ice 0000:05:00.0: addr (h,l) 0x00000000 0x00000000
Based on the above, the driver requested a 0x0002BF20 timeout (180 000
ms) but the FW returned only 0x00000BB8 (3s).
I'm assuming this is expected behavior since the maximum timeout for
NVM read should be 60,000 ms.
If changing the timeout requested by the driver to 60s for read ops is
handled correctly by the FW and the FW respects that lock, the retry
patch submitted in this email thread might not be required at all.
Let me quickly prepare a new patch and test it. I'll update this
thread once I have a results.
Regards,
Robert
On Thu, Jun 25, 2026 at 9:51 AM Przemek Kitszel
[off-list ref] wrote:
quoted
quoted
quoted
This is an AI-generated review of your patch. The human sending this
quoted
quoted
[Severity: Low]
Does this check allow an extra retry execution? Since retry_cnt starts at 0,
using a strictly greater-than check allows the loop to retry 4 times
(5 total executions) instead of the 3 executions intended by
ICE_SQ_SEND_MAX_EXECUTE.
The review is correct that this allows one extra execution. However, I
intentionally kept this verbatim from the OOT driver (1.15.4) to
This OOT change was made entirely by Grzegorz, for E825C. Would be great
to acknowledge his authorship (+ SoB ofc, below).
Signed-off-by: Grzegorz Nitka <redacted>
What was the platform that you have encountered the bug on?
quoted
retain the significant test exposure it has there. The absolute worst
case is one harmless extra retry before giving up. If you strongly
prefer strict adherence to ICE_SQ_SEND_MAX_EXECUTE, I can update the
condition in a v4.
Please update (to have one attempt less).
quoted
quoted
[Severity: High]
Does dropping the NVM lock here violate the caller's assumption of mutual
exclusion?
Callers of ice_read_flat_nvm() typically acquire the NVM lock beforehand to
ensure an atomic read of multiple sectors without interleaved writes. By
explicitly dropping the lock during the msleep() window, another thread, VF,
or PF could acquire the NVM lock and modify the NVM before the read resumes,
potentially resulting in reading torn or inconsistent NVM data.
quoted
+ status = ice_acquire_nvm(hw, ICE_RES_READ);
+ if (status)
+ break;
I don't think this is an issue. The NVM resource (req_res 0x0008) is
an ownership lease, not a content snapshot. The FW grants it with a
hold timeout (see the timeout field handling in ice_aq_req_res). It
never guaranteed a point-in-time view of the flash.
Very similar behavior already exists in ice_devlink_nvm_snapshot(),
which reads the NVM in chunks via ice_read_flat_nvm() and deliberately
acquires/releases the lock between chunks (with a comment explaining
the FW reclaim).
Additionally, the lock is only dropped on the failure path, where
neither bytes_read nor offset have been advanced. The driver simply
re-reads the failed chunk after re-acquiring. So no data that is
returned to the caller is committed across the drop window.
Overall the lock drop does not weaken any guarantee callers actually
have. If we wanted to introduce a true snapshot guarantee, that would
probably require different approach and separate patch.
quoted
[Severity: High]
Can this lead to an unbalanced NVM resource lock release and potential data
corruption?
If ice_acquire_nvm() fails here, the loop breaks and returns an error with the
lock currently unheld. However, callers like ice_read_flash_module() are
unaware of this internal lock state change and will unconditionally call
ice_release_nvm() upon failure:
drivers/net/ethernet/intel/ice/ice_nvm.c:ice_read_flash_module() {
status = ice_acquire_nvm(hw, ICE_RES_READ);
if (status)
return status;
status = ice_read_flat_nvm(hw, start + offset, &length, data, false);
ice_release_nvm(hw);
...
Because firmware tracks resource locks per-PF rather than per-OS-thread, this
errant second release could forcefully strip the lock from another thread on
the same PF that successfully acquired it, exposing the NVM to concurrent
modification.
Agreed, this might be a real bug, and the one of the three I think is
worth investigating.
If ice_acquire_nvm() fails after the drop, ice_read_flat_nvm() returns
with the lock unheld while callers unconditionally call
ice_release_nvm(), so a stray release is issued.
On probability, though, the window is very small. Reaching it requires
sustained EBUSY across the retry budget plus a failed re-acquire
(which itself polls up to ICE_NVM_TIMEOUT), and concurrently another
requester taking the lock. Most reads happen during init (ice_probe,
and reset/rebuild via ice_init_nvm), and NVM writes only happen on an
already initialized driver. The devlink/ethtool nvm_read paths are
also exposed, but hitting this race would require precise timing
against a concurrent NVM owner on the device.
I'd prefer to keep the scope of this patch limited to the EBUSY retry
path and not take on the unbalanced-release fix here. A proper fix
should change the lock-ownership contract of ice_read_flat_nvm() (on
error, the lock must be released by ice_read_flat_nvm(), callers
release only on success) and update all callers. Code change sould be
simple for all callers but ice_discover_flash_size(), it intentionally
holds one lease across a read loop and would need to re-acquire after
each expected boundary failure.
Given how small the original window is, I'd rather not trade tested
OOT behavior for the risk of a complex unbalanced NVM lock fix. I
actually have a patch mostly ready that fixes the lock-ownership
contract, but I really don't like it. It changes the design of
ice_read_flat_nvm(), making it less intuitive for callers. More
importantly, I just don't have the resources or test coverage right
now to properly verify such change.
However, I can modify the failure path for ice_acquire_nvm inside
ice_read_flat_nvm. Instead of bailing out immediately, we can just
retry it within the existing retry budget. In this case, the
probability of leaving ice_read_flat_nvm without holding the lock is
reduced even further without needing a refactor.
Please let me know what you think about my thought process on this.
I think that both AI-reported issues against the lock are valid
concerns.
I think that sleep was the actual fix, and re-locking were merely
a necessity due to their expiration (as you said).
A proper fix would be to just increase lock-timeout to accommodate all
attempts (and still do the retries&sleep, but without unlocking).
Hey Przemek,
I ran some tests and unfortunately, the following sentence from the
datasheet is true:
"For specific resources, such as Change Lock (0x0003) and Global Config Lock
(0x0004), this field is used by software to override the default timeout for the
operation, and also to specify the timeout used for this operation."
This means we can only change a default timeout for 0x0003 and 0x0004
but not for 0x0001 (NVM resource).
Whatever timeout I provide FW defaults to 0xB88
Input:
[ 2209.656758] ice 0000:31:00.0: CQ CMD: opcode 0x0008, flags 0x2000,
datalen 0x0000, retval 0x0000
[ 2209.656760] ice 0000:31:00.0: cookie (h,l) 0x00000000 0x00000000
[ 2209.656761] ice 0000:31:00.0: param (0,1) 0x00010001 0x00000BB9
Output:
[ 2209.656927] ice 0000:31:00.0: CQ CMD: opcode 0x0008, flags 0x2003,
datalen 0x0000, retval 0x0000
[ 2209.656929] ice 0000:31:00.0: cookie (h,l) 0x00000000 0x00000000
[ 2209.656931] ice 0000:31:00.0: param (0,1) 0x00010001 0x00000BB8
Correct me If I'm wrong, but the only way to properly handle it is to
ensure the resource is locked and released between every
ice_acquire_nvm call.
I'll start working on this.
thank you for checking out!
I agree that simple retries with improved (refactored) locking will be
good solution.
Failure to lock should count as an unsuccessful attempt, with possible
retry after a sleep.
From: Robert Malz via Intel-wired-lan <hidden> Date: 2026-07-03 10:46:41
Hey Przemek,
I have submitted a new patch to solve the current issue:
https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20260629/055891.html
I ran numerous tests on it and also verified it against the setup
which initially triggered the issue.
This system had a problem where ice_discover_flash_size read time was
greater that 3 seconds and as a result failed with EBUSY status (FW at
asome point releases the lock).
With new patch issue was not reproduced even when the read time was
much higher than normally expected:
[ 75.999353] ice 0000:05:00.0: CQ CMD: opcode 0x0008, flags 0x2000,
datalen 0x0000, retval 0x0000 <- First lock acquisition
[ 82.152231] ice 0000:05:00.0: Predicted flash size is 10485760
bytes <- ice_discover_flash_size finished reading (7 seconds).
In this case, the driver properly read the NVM contents without
needing any retries.
I also analyzed the overhead of adding an acquire/release lock between
each 0x0701 call and based on my observations it did not negatively
impact the driver in any way.
Execution time for ice_discover_flash_size in normal scenario
increased from around 0,02s to 0,05s which should still be acceptable.
Regards,
Robert
On Fri, Jun 26, 2026 at 1:46 PM Przemek Kitszel
[off-list ref] wrote:
On 6/26/26 10:15, Robert Malz wrote:
quoted
Hey Przemek,
I ran some tests and unfortunately, the following sentence from the
datasheet is true:
"For specific resources, such as Change Lock (0x0003) and Global Config Lock
(0x0004), this field is used by software to override the default timeout for the
operation, and also to specify the timeout used for this operation."
This means we can only change a default timeout for 0x0003 and 0x0004
but not for 0x0001 (NVM resource).
Whatever timeout I provide FW defaults to 0xB88
Input:
[ 2209.656758] ice 0000:31:00.0: CQ CMD: opcode 0x0008, flags 0x2000,
datalen 0x0000, retval 0x0000
[ 2209.656760] ice 0000:31:00.0: cookie (h,l) 0x00000000 0x00000000
[ 2209.656761] ice 0000:31:00.0: param (0,1) 0x00010001 0x00000BB9
Output:
[ 2209.656927] ice 0000:31:00.0: CQ CMD: opcode 0x0008, flags 0x2003,
datalen 0x0000, retval 0x0000
[ 2209.656929] ice 0000:31:00.0: cookie (h,l) 0x00000000 0x00000000
[ 2209.656931] ice 0000:31:00.0: param (0,1) 0x00010001 0x00000BB8
Correct me If I'm wrong, but the only way to properly handle it is to
ensure the resource is locked and released between every
ice_acquire_nvm call.
I'll start working on this.
thank you for checking out!
I agree that simple retries with improved (refactored) locking will be
good solution.
Failure to lock should count as an unsuccessful attempt, with possible
retry after a sleep.