Hey Jacob, Tony, Przemek Thanks again for the review. I have submitted the v2 patch with the mentioned changes. ref: https://lists.osuosl.org/pipermail/intel-wired-lan/Week-of-Mon-20260713/056079.html
Regards, Robert On Wed, Jul 15, 2026 at 8:22 PM Jacob Keller <[email protected]> wrote: > > On 7/15/2026 10:04 AM, Tony Nguyen wrote: > > > > > > 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 > > I concur as well. In theory we should probably try to fully refactor the > send_sq commands to propagate the error out from the lowest level > without sq_last_status at all.. but thats a bigger ask and I don't think > Robert needs to be asked to do that. > > The approach described here seems like a reasonable compromise fornow. > > Thanks, > Jake
