| Message ID | 20210719163154.986679-1-anthony.l.nguyen@intel.com |
|---|---|
| Headers | show |
| Series | 40GbE Intel Wired LAN Driver Updates 2021-07-19 | expand |
On Mon, 19 Jul 2021 09:31:54 -0700, Tony Nguyen wrote: > To avoid races between iavf_init_task(), iavf_reset_task(), > iavf_watchdog_task(), iavf_adminq_task() as well as the shutdown and > remove functions more locking is required. > The current protection by __IAVF_IN_CRITICAL_TASK is needed in > additional places. > > - The reset task performs state transitions, therefore needs locking. > - The adminq task acts on replies from the PF in > iavf_virtchnl_completion() which may alter the states. > - The init task is not only run during probe but also if a VF gets stuck > to reinitialize it. > - The shutdown function performs a state transition. > - The remove function performs a state transition and also free's > resources. > > iavf_lock_timeout() is introduced to avoid waiting infinitely > and cause a deadlock. Rather unlock and print a warning. I have a vague recollection of complaining about something like this previously. Why not use a normal lock? Please at the very least include an explanation in the commit message. If you use bit locks you should use the _lock and _unlock flavours of the bitops.
On 20.07.21 13:31, Jakub Kicinski wrote: > On Mon, 19 Jul 2021 09:31:54 -0700, Tony Nguyen wrote: >> To avoid races between iavf_init_task(), iavf_reset_task(), >> iavf_watchdog_task(), iavf_adminq_task() as well as the shutdown and >> remove functions more locking is required. >> The current protection by __IAVF_IN_CRITICAL_TASK is needed in >> additional places. >> >> - The reset task performs state transitions, therefore needs locking. >> - The adminq task acts on replies from the PF in >> iavf_virtchnl_completion() which may alter the states. >> - The init task is not only run during probe but also if a VF gets stuck >> to reinitialize it. >> - The shutdown function performs a state transition. >> - The remove function performs a state transition and also free's >> resources. >> >> iavf_lock_timeout() is introduced to avoid waiting infinitely >> and cause a deadlock. Rather unlock and print a warning. > > I have a vague recollection of complaining about something like this > previously. Why not use a normal lock? Please at the very least include > an explanation in the commit message. > > If you use bit locks you should use the _lock and _unlock flavours of > the bitops. > Hi Jakub, yes you remember correctly, back then we agreed to fix this afterwards. It's not been forgotten, working on the conversion is the next step. Thanks! Stefan
Hello: This series was applied to netdev/net-next.git (refs/heads/master): On Mon, 19 Jul 2021 09:31:51 -0700 you wrote: > This series contains updates to iavf and i40e drivers. > > Stefan Assmann adds locking to a path that does not acquire a spinlock > where needed for i40e. He also adjusts locking of critical sections to > help avoid races and removes overriding of the adapter state during > pending reset for iavf driver. > > [...] Here is the summary with links: - [net-next,1/3] i40e: improve locking of mac_filter_hash https://git.kernel.org/netdev/net-next/c/8b4b06919fd6 - [net-next,2/3] iavf: do not override the adapter state in the watchdog task https://git.kernel.org/netdev/net-next/c/22c8fd71d3a5 - [net-next,3/3] iavf: fix locking of critical sections https://git.kernel.org/netdev/net-next/c/226d528512cf You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html