On 7/15/2026 3:22 AM, Robert Malz wrote:
...
- if (status)
+ if (status) {
+ /* ice_release_nvm() issues an AQ command that would
+ * overwrite sq_last_status, which some callers
+ * inspect after a failed read. Preserve the read's
+ * AQ error across the release.
+ */
+ aq_err = hw->adminq.sq_last_status;
+
+ ice_release_nvm(hw);
+ hw->adminq.sq_last_status = aq_err;
Does restoring hw->adminq.sq_last_status here without holding the Admin
Queue
lock risk overwriting the status of a concurrent AdminQ command?
If another thread acquires the lock and sends a command immediately after
ice_release_nvm(hw), this lockless write could corrupt its error status.
[TN] I don't believe the AQ lock will work as desired we can't guarantee
that we will have the lock directly following the release in order to
restore the AQ error. Similar to the NVM lock issue, I think this is a
small window but wanted to bring this here in case others had
thoughts/comments on this.
[RM] Agree, this can cause issues. We can't drop sq_last_status as
ice_discover_flash_size depends on it.
Proposed fix: drop the save/restore sq_last_status and propagate the
read AQ error through a real return path instead of the shared global.
I could add optional enum libie_aq_err *read_aq_err out param to
ice_read_flat_nvm(), capture sq_last_status before the
ice_release_nvm, and have ice_discover_flash_size() test that instead.
The log-only callers (devlink/ethtool) don't care about it.
Let me know what you think about it.
This sounds good to me.
Thanks,
Tony
