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 <[email protected]> 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 > <[email protected]> wrote: > > > > >> This is an AI-generated review of your patch. The human sending this > > > > > > >> [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. > > >> > > >>> + break; > > >>> + ice_debug(hw, ICE_DBG_NVM, > > >>> + "NVM read EBUSY error, retry %d\n", > > >>> + retry_cnt + 1); > > >>> + last_cmd = false; > > >>> + ice_release_nvm(hw); > > >>> + msleep(ICE_SQ_SEND_DELAY_TIME_MS); > > >> > > > 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 <[email protected]> > > > > 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). > > > > > > > >> [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; > > >> > > > > > > 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. > > > > 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). > > > > > > > > > > > > > > > >>> + retry_cnt++; > > >>> + } else { > > >>> + bytes_read += read_size; > > >>> + offset += read_size; > > >>> + retry_cnt = 0; > > >>> + } > > >>> } while (!last_cmd); > > >>> > > >>> *length = bytes_read; > > > > > > Thanks, > > > Robert > >
