| Message ID | 20230419115006.200409-5-kamil.maziarz@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iavf: fix reset task deadlock | expand |
On 4/19/2023 4:50 AM, Kamil Maziarz wrote: > From: Marcin Szycik <marcin.szycik@linux.intel.com> > > This reverts commit 08f1c147b7265245d67321585c68a27e990e0c4b. > > Netdev is no longer being detached during reset, so this fix can be > reverted. > > Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com> > Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com> With the other fixes, this is good. Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > --- > v2: no changes > --- > v3: no changes > --- > drivers/net/ethernet/intel/iavf/iavf_main.c | 16 +--------------- > 1 file changed, 1 insertion(+), 15 deletions(-) > > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c > index 8dd488158961..8f13685ed2fe 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c > @@ -2988,6 +2988,7 @@ static void iavf_disable_vf(struct iavf_adapter *adapter) > iavf_free_queues(adapter); > memset(adapter->vf_res, 0, IAVF_VIRTCHNL_VF_RESOURCE_SIZE); > iavf_shutdown_adminq(&adapter->hw); > + adapter->netdev->flags &= ~IFF_UP; > adapter->flags &= ~IAVF_FLAG_RESET_PENDING; > iavf_change_state(adapter, __IAVF_DOWN); > wake_up(&adapter->down_waitqueue); > @@ -3082,11 +3083,6 @@ static void iavf_reset_task(struct work_struct *work) > iavf_disable_vf(adapter); > mutex_unlock(&adapter->client_lock); > mutex_unlock(&adapter->crit_lock); > - if (netif_running(netdev)) { > - rtnl_lock(); > - dev_close(netdev); > - rtnl_unlock(); > - } > return; /* Do not attempt to reinit. It's dead, Jim. */ > } > > @@ -3237,16 +3233,6 @@ static void iavf_reset_task(struct work_struct *work) > > mutex_unlock(&adapter->client_lock); > mutex_unlock(&adapter->crit_lock); > - > - if (netif_running(netdev)) { > - /* Close device to ensure that Tx queues will not be started > - * during netif_device_attach() at the end of the reset task. > - */ > - rtnl_lock(); > - dev_close(netdev); > - rtnl_unlock(); > - } > - > dev_err(&adapter->pdev->dev, "failed to allocate resources during reinit\n"); > } >
On 4/19/2023 11:42 AM, Jacob Keller wrote: > > > On 4/19/2023 4:50 AM, Kamil Maziarz wrote: >> From: Marcin Szycik <marcin.szycik@linux.intel.com> >> >> This reverts commit 08f1c147b7265245d67321585c68a27e990e0c4b. >> >> Netdev is no longer being detached during reset, so this fix can be >> reverted. >> >> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com> >> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com> > > With the other fixes, this is good. > > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > >> --- >> v2: no changes >> --- >> v3: no changes >> --- >> drivers/net/ethernet/intel/iavf/iavf_main.c | 16 +--------------- >> 1 file changed, 1 insertion(+), 15 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c >> index 8dd488158961..8f13685ed2fe 100644 >> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c >> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c >> @@ -2988,6 +2988,7 @@ static void iavf_disable_vf(struct iavf_adapter *adapter) >> iavf_free_queues(adapter); >> memset(adapter->vf_res, 0, IAVF_VIRTCHNL_VF_RESOURCE_SIZE); >> iavf_shutdown_adminq(&adapter->hw); >> + adapter->netdev->flags &= ~IFF_UP; Wait. We must not modify netdev->flags like this. Please do not restore this part of the commit. Perhaps a new commit message with a partial revert explanation. Modification of the netdev flags like this is incorrect. See the original commit message which said: Also replace the hacky manipulation with IFF_UP flag by device close that clears properly both IFF_UP and __LINK_STATE_START flags. In these case iavf_close() does not do anything because the adapter state is already __IAVF_DOWN. NAK this change as-is. It may be no longer required to perform the other parts of the patch, but I do not accept re-adding the incorrect IFF_UP modification. Thanks, Jake
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c index 8dd488158961..8f13685ed2fe 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_main.c +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c @@ -2988,6 +2988,7 @@ static void iavf_disable_vf(struct iavf_adapter *adapter) iavf_free_queues(adapter); memset(adapter->vf_res, 0, IAVF_VIRTCHNL_VF_RESOURCE_SIZE); iavf_shutdown_adminq(&adapter->hw); + adapter->netdev->flags &= ~IFF_UP; adapter->flags &= ~IAVF_FLAG_RESET_PENDING; iavf_change_state(adapter, __IAVF_DOWN); wake_up(&adapter->down_waitqueue); @@ -3082,11 +3083,6 @@ static void iavf_reset_task(struct work_struct *work) iavf_disable_vf(adapter); mutex_unlock(&adapter->client_lock); mutex_unlock(&adapter->crit_lock); - if (netif_running(netdev)) { - rtnl_lock(); - dev_close(netdev); - rtnl_unlock(); - } return; /* Do not attempt to reinit. It's dead, Jim. */ } @@ -3237,16 +3233,6 @@ static void iavf_reset_task(struct work_struct *work) mutex_unlock(&adapter->client_lock); mutex_unlock(&adapter->crit_lock); - - if (netif_running(netdev)) { - /* Close device to ensure that Tx queues will not be started - * during netif_device_attach() at the end of the reset task. - */ - rtnl_lock(); - dev_close(netdev); - rtnl_unlock(); - } - dev_err(&adapter->pdev->dev, "failed to allocate resources during reinit\n"); }