* [Intel-wired-lan] [PATCH iwl-net v10 1/5] iavf: Wait for reset in callbacks which trigger it
2023-06-05 14:52 [Intel-wired-lan] [PATCH iwl-net v10 0/5] iavf: fix reset task deadlock Mateusz Palczewski
@ 2023-06-05 14:52 ` Mateusz Palczewski
2023-06-15 15:17 ` Romanowski, Rafal
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 2/5] Revert "iavf: Detach device during reset task" Mateusz Palczewski
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Mateusz Palczewski @ 2023-06-05 14:52 UTC (permalink / raw)
To: intel-wired-lan; +Cc: ivecera, Kamil Maziarz, Sylwester Dziedziuch
From: Marcin Szycik <marcin.szycik@linux.intel.com>
There was a fail when trying to add the interface to bonding
right after changing the MTU on the interface. It was caused
by bonding interface unable to open the interface due to
interface being in __RESETTING state because of MTU change.
Add new reset_waitqueue to indicate that reset has finished.
Add waiting for reset to finish in callbacks which trigger hw reset:
iavf_set_priv_flags(), iavf_change_mtu() and iavf_set_ringparam().
We use a 5000ms timeout period because on Hyper-V based systems,
this operation takes around 3000-4000ms. In normal circumstances,
it doesn't take more than 500ms to complete.
Add a function iavf_wait_for_reset() to reuse waiting for reset code and
use it also in iavf_set_channels(), which already waits for reset.
We don't use error handling in iavf_set_channels() as this could
cause the device to be in incorrect state if the reset was scheduled
but hit timeout or the waitng function was interrupted by a signal.
Fixes: 4e5e6b5d9d13 ("iavf: Fix return of set the new channel count")
Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Co-developed-by: Dawid Wesierski <dawidx.wesierski@intel.com>
Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
---
v1->v3: changes were done internally
v4: fixed SOB's
v5: changed the way we wake up the reset_waitqueue to make sure that reset is woken up
v6->v9: no changes
v10: Added fixes tag and fixed commit message
---
drivers/net/ethernet/intel/iavf/iavf.h | 2 +
.../net/ethernet/intel/iavf/iavf_ethtool.c | 31 ++++++-----
drivers/net/ethernet/intel/iavf/iavf_main.c | 51 ++++++++++++++++++-
.../net/ethernet/intel/iavf/iavf_virtchnl.c | 1 +
4 files changed, 68 insertions(+), 17 deletions(-)
diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index 9abaff1f2aff..c51b9ed4dc29 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -257,6 +257,7 @@ struct iavf_adapter {
struct work_struct adminq_task;
struct delayed_work client_task;
wait_queue_head_t down_waitqueue;
+ wait_queue_head_t reset_waitqueue;
wait_queue_head_t vc_waitqueue;
struct iavf_q_vector *q_vectors;
struct list_head vlan_filter_list;
@@ -591,5 +592,6 @@ void iavf_add_adv_rss_cfg(struct iavf_adapter *adapter);
void iavf_del_adv_rss_cfg(struct iavf_adapter *adapter);
struct iavf_mac_filter *iavf_add_filter(struct iavf_adapter *adapter,
const u8 *macaddr);
+int iavf_wait_for_reset(struct iavf_adapter *adapter);
int iavf_lock_timeout(struct mutex *lock, unsigned int msecs);
#endif /* _IAVF_H_ */
diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
index 92443f8e9fbd..b7141c2a941d 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
@@ -484,6 +484,7 @@ static int iavf_set_priv_flags(struct net_device *netdev, u32 flags)
{
struct iavf_adapter *adapter = netdev_priv(netdev);
u32 orig_flags, new_flags, changed_flags;
+ int ret = 0;
u32 i;
orig_flags = READ_ONCE(adapter->flags);
@@ -533,10 +534,13 @@ static int iavf_set_priv_flags(struct net_device *netdev, u32 flags)
if (netif_running(netdev)) {
adapter->flags |= IAVF_FLAG_RESET_NEEDED;
queue_work(adapter->wq, &adapter->reset_task);
+ ret = iavf_wait_for_reset(adapter);
+ if (ret)
+ netdev_warn(netdev, "Changing private flags timeout or interrupted waiting for reset");
}
}
- return 0;
+ return ret;
}
/**
@@ -627,6 +631,7 @@ static int iavf_set_ringparam(struct net_device *netdev,
{
struct iavf_adapter *adapter = netdev_priv(netdev);
u32 new_rx_count, new_tx_count;
+ int ret = 0;
if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
return -EINVAL;
@@ -673,9 +678,12 @@ static int iavf_set_ringparam(struct net_device *netdev,
if (netif_running(netdev)) {
adapter->flags |= IAVF_FLAG_RESET_NEEDED;
queue_work(adapter->wq, &adapter->reset_task);
+ ret = iavf_wait_for_reset(adapter);
+ if (ret)
+ netdev_warn(netdev, "Changing ring parameters timeout or interrupted waiting for reset");
}
- return 0;
+ return ret;
}
/**
@@ -1830,7 +1838,7 @@ static int iavf_set_channels(struct net_device *netdev,
{
struct iavf_adapter *adapter = netdev_priv(netdev);
u32 num_req = ch->combined_count;
- int i;
+ int ret = 0;
if ((adapter->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_ADQ) &&
adapter->num_tc) {
@@ -1854,20 +1862,11 @@ static int iavf_set_channels(struct net_device *netdev,
adapter->flags |= IAVF_FLAG_REINIT_ITR_NEEDED;
iavf_schedule_reset(adapter);
- /* wait for the reset is done */
- for (i = 0; i < IAVF_RESET_WAIT_COMPLETE_COUNT; i++) {
- msleep(IAVF_RESET_WAIT_MS);
- if (adapter->flags & IAVF_FLAG_RESET_PENDING)
- continue;
- break;
- }
- if (i == IAVF_RESET_WAIT_COMPLETE_COUNT) {
- adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
- adapter->num_req_queues = 0;
- return -EOPNOTSUPP;
- }
+ ret = iavf_wait_for_reset(adapter);
+ if (ret)
+ netdev_warn(netdev, "Changing channel count timeout or interrupted waiting for reset");
- return 0;
+ return ret;
}
/**
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 89892a0fd5b7..c815ef87e27d 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -166,6 +166,45 @@ static struct iavf_adapter *iavf_pdev_to_adapter(struct pci_dev *pdev)
return netdev_priv(pci_get_drvdata(pdev));
}
+/**
+ * iavf_is_reset_in_progress - Check if a reset is in progress
+ * @adapter: board private structure
+ */
+static bool iavf_is_reset_in_progress(struct iavf_adapter *adapter)
+{
+ if (adapter->state == __IAVF_RESETTING ||
+ adapter->flags & (IAVF_FLAG_RESET_PENDING |
+ IAVF_FLAG_RESET_NEEDED))
+ return true;
+
+ return false;
+}
+
+/**
+ * iavf_wait_for_reset - Wait for reset to finish.
+ * @adapter: board private structure
+ *
+ * Returns 0 if reset finished successfully, negative on timeout or interrupt.
+ */
+int iavf_wait_for_reset(struct iavf_adapter *adapter)
+{
+ int ret = wait_event_interruptible_timeout(adapter->reset_waitqueue,
+ !iavf_is_reset_in_progress(adapter),
+ msecs_to_jiffies(5000));
+
+ /* If ret < 0 then it means wait was interrupted.
+ * If ret == 0 then it means we got a timeout while waiting
+ * for reset to finish.
+ * If ret > 0 it means reset has finished.
+ */
+ if (ret > 0)
+ return 0;
+ else if (ret < 0)
+ return -EINTR;
+ else
+ return -EBUSY;
+}
+
/**
* iavf_allocate_dma_mem_d - OS specific memory alloc for shared code
* @hw: pointer to the HW structure
@@ -3163,6 +3202,7 @@ static void iavf_reset_task(struct work_struct *work)
adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
+ wake_up(&adapter->reset_waitqueue);
mutex_unlock(&adapter->client_lock);
mutex_unlock(&adapter->crit_lock);
@@ -4327,6 +4367,7 @@ static int iavf_close(struct net_device *netdev)
static int iavf_change_mtu(struct net_device *netdev, int new_mtu)
{
struct iavf_adapter *adapter = netdev_priv(netdev);
+ int ret = 0;
netdev_dbg(netdev, "changing MTU from %d to %d\n",
netdev->mtu, new_mtu);
@@ -4339,9 +4380,14 @@ static int iavf_change_mtu(struct net_device *netdev, int new_mtu)
if (netif_running(netdev)) {
adapter->flags |= IAVF_FLAG_RESET_NEEDED;
queue_work(adapter->wq, &adapter->reset_task);
+ ret = iavf_wait_for_reset(adapter);
+ if (ret < 0)
+ netdev_warn(netdev, "MTU change interrupted waiting for reset");
+ else if (ret)
+ netdev_warn(netdev, "MTU change timed out waiting for reset");
}
- return 0;
+ return ret;
}
#define NETIF_VLAN_OFFLOAD_FEATURES (NETIF_F_HW_VLAN_CTAG_RX | \
@@ -4942,6 +4988,9 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
/* Setup the wait queue for indicating transition to down status */
init_waitqueue_head(&adapter->down_waitqueue);
+ /* Setup the wait queue for indicating transition to running state */
+ init_waitqueue_head(&adapter->reset_waitqueue);
+
/* Setup the wait queue for indicating virtchannel events */
init_waitqueue_head(&adapter->vc_waitqueue);
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 7c0578b5457b..1bab896aaf40 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -2285,6 +2285,7 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
case VIRTCHNL_OP_ENABLE_QUEUES:
/* enable transmits */
iavf_irq_enable(adapter, true);
+ wake_up(&adapter->reset_waitqueue);
adapter->flags &= ~IAVF_FLAG_QUEUES_DISABLED;
break;
case VIRTCHNL_OP_DISABLE_QUEUES:
--
2.31.1
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-net v10 1/5] iavf: Wait for reset in callbacks which trigger it
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 1/5] iavf: Wait for reset in callbacks which trigger it Mateusz Palczewski
@ 2023-06-15 15:17 ` Romanowski, Rafal
0 siblings, 0 replies; 11+ messages in thread
From: Romanowski, Rafal @ 2023-06-15 15:17 UTC (permalink / raw)
To: Palczewski, Mateusz, intel-wired-lan@lists.osuosl.org
Cc: ivecera, Sylwester Dziedziuch, Maziarz, Kamil
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Mateusz Palczewski
> Sent: poniedziałek, 5 czerwca 2023 16:52
> To: intel-wired-lan@lists.osuosl.org
> Cc: ivecera <ivecera@redhat.com>; Maziarz, Kamil
> <kamil.maziarz@intel.com>; Sylwester Dziedziuch
> <sylwesterx.dziedziuch@intel.com>
> Subject: [Intel-wired-lan] [PATCH iwl-net v10 1/5] iavf: Wait for reset in
> callbacks which trigger it
>
> From: Marcin Szycik <marcin.szycik@linux.intel.com>
>
> There was a fail when trying to add the interface to bonding right after
> changing the MTU on the interface. It was caused by bonding interface
> unable to open the interface due to interface being in __RESETTING state
> because of MTU change.
>
> Add new reset_waitqueue to indicate that reset has finished.
>
> Add waiting for reset to finish in callbacks which trigger hw reset:
> iavf_set_priv_flags(), iavf_change_mtu() and iavf_set_ringparam().
> We use a 5000ms timeout period because on Hyper-V based systems, this
> operation takes around 3000-4000ms. In normal circumstances, it doesn't
> take more than 500ms to complete.
>
> Add a function iavf_wait_for_reset() to reuse waiting for reset code and use
> it also in iavf_set_channels(), which already waits for reset.
> We don't use error handling in iavf_set_channels() as this could cause the
> device to be in incorrect state if the reset was scheduled but hit timeout or
> the waitng function was interrupted by a signal.
>
> Fixes: 4e5e6b5d9d13 ("iavf: Fix return of set the new channel count")
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Co-developed-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> Signed-off-by: Sylwester Dziedziuch <sylwesterx.dziedziuch@intel.com>
> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> ---
> v1->v3: changes were done internally
> v4: fixed SOB's
> v5: changed the way we wake up the reset_waitqueue to make sure that
> reset is woken up
> v6->v9: no changes
> v10: Added fixes tag and fixed commit message
> ---
> drivers/net/ethernet/intel/iavf/iavf.h | 2 +
> .../net/ethernet/intel/iavf/iavf_ethtool.c | 31 ++++++-----
> drivers/net/ethernet/intel/iavf/iavf_main.c | 51 ++++++++++++++++++-
> .../net/ethernet/intel/iavf/iavf_virtchnl.c | 1 +
> 4 files changed, 68 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h
> b/drivers/net/ethernet/intel/iavf/iavf.h
> index 9abaff1f2aff..c51b9ed4dc29 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
> @@ -257,6 +257,7 @@ struct iavf_adapter {
> struct work_struct adminq_task;
> struct delayed_work client_task;
> wait_queue_head_t down_waitqueue;
> + wait_queue_head_t reset_waitqueue;
> wait_queue_head_t vc_waitqueue;
> struct iavf_q_vector *q_vectors;
> struct list_head vlan_filter_list;
> @@ -591,5 +592,6 @@ void iavf_add_adv_rss_cfg(struct iavf_adapter
> *adapter); void iavf_del_adv_rss_cfg(struct iavf_adapter *adapter); struct
> iavf_mac_filter *iavf_add_filter(struct iavf_adapter *adapter,
> const u8 *macaddr);
> +int iavf_wait_for_reset(struct iavf_adapter *adapter);
> int iavf_lock_timeout(struct mutex *lock, unsigned int msecs); #endif /*
> _IAVF_H_ */ diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> index 92443f8e9fbd..b7141c2a941d 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
> @@ -484,6 +484,7 @@ static int iavf_set_priv_flags(struct net_device
> *netdev, u32 flags) {
> struct iavf_adapter *adapter = netdev_priv(netdev);
> u32 orig_flags, new_flags, changed_flags;
> + int ret = 0;
> u32 i;
>
> orig_flags = READ_ONCE(adapter->flags); @@ -533,10 +534,13 @@
> static int iavf_set_priv_flags(struct net_device *netdev, u32 flags)
> if (netif_running(netdev)) {
> adapter->flags |= IAVF_FLAG_RESET_NEEDED;
> queue_work(adapter->wq, &adapter->reset_task);
> + ret = iavf_wait_for_reset(adapter);
> + if (ret)
> + netdev_warn(netdev, "Changing private flags
> timeout or interrupted
> +waiting for reset");
> }
> }
>
> - return 0;
> + return ret;
> }
>
> /**
> @@ -627,6 +631,7 @@ static int iavf_set_ringparam(struct net_device
> *netdev, {
> struct iavf_adapter *adapter = netdev_priv(netdev);
> u32 new_rx_count, new_tx_count;
> + int ret = 0;
>
> if ((ring->rx_mini_pending) || (ring->rx_jumbo_pending))
> return -EINVAL;
> @@ -673,9 +678,12 @@ static int iavf_set_ringparam(struct net_device
> *netdev,
> if (netif_running(netdev)) {
> adapter->flags |= IAVF_FLAG_RESET_NEEDED;
> queue_work(adapter->wq, &adapter->reset_task);
> + ret = iavf_wait_for_reset(adapter);
> + if (ret)
> + netdev_warn(netdev, "Changing ring parameters
> timeout or interrupted
> +waiting for reset");
> }
>
> - return 0;
> + return ret;
> }
>
> /**
> @@ -1830,7 +1838,7 @@ static int iavf_set_channels(struct net_device
> *netdev, {
> struct iavf_adapter *adapter = netdev_priv(netdev);
> u32 num_req = ch->combined_count;
> - int i;
> + int ret = 0;
>
> if ((adapter->vf_res->vf_cap_flags & VIRTCHNL_VF_OFFLOAD_ADQ)
> &&
> adapter->num_tc) {
> @@ -1854,20 +1862,11 @@ static int iavf_set_channels(struct net_device
> *netdev,
> adapter->flags |= IAVF_FLAG_REINIT_ITR_NEEDED;
> iavf_schedule_reset(adapter);
>
> - /* wait for the reset is done */
> - for (i = 0; i < IAVF_RESET_WAIT_COMPLETE_COUNT; i++) {
> - msleep(IAVF_RESET_WAIT_MS);
> - if (adapter->flags & IAVF_FLAG_RESET_PENDING)
> - continue;
> - break;
> - }
> - if (i == IAVF_RESET_WAIT_COMPLETE_COUNT) {
> - adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
> - adapter->num_req_queues = 0;
> - return -EOPNOTSUPP;
> - }
> + ret = iavf_wait_for_reset(adapter);
> + if (ret)
> + netdev_warn(netdev, "Changing channel count timeout or
> interrupted
> +waiting for reset");
>
> - return 0;
> + return ret;
> }
>
> /**
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 89892a0fd5b7..c815ef87e27d 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -166,6 +166,45 @@ static struct iavf_adapter
> *iavf_pdev_to_adapter(struct pci_dev *pdev)
> return netdev_priv(pci_get_drvdata(pdev));
> }
>
> +/**
> + * iavf_is_reset_in_progress - Check if a reset is in progress
> + * @adapter: board private structure
> + */
> +static bool iavf_is_reset_in_progress(struct iavf_adapter *adapter) {
> + if (adapter->state == __IAVF_RESETTING ||
> + adapter->flags & (IAVF_FLAG_RESET_PENDING |
> + IAVF_FLAG_RESET_NEEDED))
> + return true;
> +
> + return false;
> +}
> +
> +/**
> + * iavf_wait_for_reset - Wait for reset to finish.
> + * @adapter: board private structure
> + *
> + * Returns 0 if reset finished successfully, negative on timeout or interrupt.
> + */
> +int iavf_wait_for_reset(struct iavf_adapter *adapter) {
> + int ret = wait_event_interruptible_timeout(adapter-
> >reset_waitqueue,
> + !iavf_is_reset_in_progress(adapter),
> + msecs_to_jiffies(5000));
> +
> + /* If ret < 0 then it means wait was interrupted.
> + * If ret == 0 then it means we got a timeout while waiting
> + * for reset to finish.
> + * If ret > 0 it means reset has finished.
> + */
> + if (ret > 0)
> + return 0;
> + else if (ret < 0)
> + return -EINTR;
> + else
> + return -EBUSY;
> +}
> +
> /**
> * iavf_allocate_dma_mem_d - OS specific memory alloc for shared code
> * @hw: pointer to the HW structure
> @@ -3163,6 +3202,7 @@ static void iavf_reset_task(struct work_struct
> *work)
>
> adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
>
> + wake_up(&adapter->reset_waitqueue);
> mutex_unlock(&adapter->client_lock);
> mutex_unlock(&adapter->crit_lock);
>
> @@ -4327,6 +4367,7 @@ static int iavf_close(struct net_device *netdev)
> static int iavf_change_mtu(struct net_device *netdev, int new_mtu) {
> struct iavf_adapter *adapter = netdev_priv(netdev);
> + int ret = 0;
>
> netdev_dbg(netdev, "changing MTU from %d to %d\n",
> netdev->mtu, new_mtu);
> @@ -4339,9 +4380,14 @@ static int iavf_change_mtu(struct net_device
> *netdev, int new_mtu)
> if (netif_running(netdev)) {
> adapter->flags |= IAVF_FLAG_RESET_NEEDED;
> queue_work(adapter->wq, &adapter->reset_task);
> + ret = iavf_wait_for_reset(adapter);
> + if (ret < 0)
> + netdev_warn(netdev, "MTU change interrupted
> waiting for reset");
> + else if (ret)
> + netdev_warn(netdev, "MTU change timed out
> waiting for reset");
> }
>
> - return 0;
> + return ret;
> }
>
> #define NETIF_VLAN_OFFLOAD_FEATURES
> (NETIF_F_HW_VLAN_CTAG_RX | \
> @@ -4942,6 +4988,9 @@ static int iavf_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
> /* Setup the wait queue for indicating transition to down status */
> init_waitqueue_head(&adapter->down_waitqueue);
>
> + /* Setup the wait queue for indicating transition to running state */
> + init_waitqueue_head(&adapter->reset_waitqueue);
> +
> /* Setup the wait queue for indicating virtchannel events */
> init_waitqueue_head(&adapter->vc_waitqueue);
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index 7c0578b5457b..1bab896aaf40 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> @@ -2285,6 +2285,7 @@ void iavf_virtchnl_completion(struct iavf_adapter
> *adapter,
> case VIRTCHNL_OP_ENABLE_QUEUES:
> /* enable transmits */
> iavf_irq_enable(adapter, true);
> + wake_up(&adapter->reset_waitqueue);
> adapter->flags &= ~IAVF_FLAG_QUEUES_DISABLED;
> break;
> case VIRTCHNL_OP_DISABLE_QUEUES:
> --
> 2.31.1
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Intel-wired-lan] [PATCH iwl-net v10 2/5] Revert "iavf: Detach device during reset task"
2023-06-05 14:52 [Intel-wired-lan] [PATCH iwl-net v10 0/5] iavf: fix reset task deadlock Mateusz Palczewski
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 1/5] iavf: Wait for reset in callbacks which trigger it Mateusz Palczewski
@ 2023-06-05 14:52 ` Mateusz Palczewski
2023-06-15 15:28 ` Romanowski, Rafal
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 3/5] Revert "iavf: Do not restart Tx queues after reset task failure" Mateusz Palczewski
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Mateusz Palczewski @ 2023-06-05 14:52 UTC (permalink / raw)
To: intel-wired-lan; +Cc: ivecera
From: Marcin Szycik <marcin.szycik@linux.intel.com>
This reverts commit aa626da947e9cd30c4cf727493903e1adbb2c0a0.
Detaching device during reset was not fully fixing the rtnl locking issue,
as there could be a situation where callback was already in progress before
detaching netdev.
Furthermore, detaching netdevice causes TX timeouts if traffic is running.
To reproduce:
ip netns exec ns1 iperf3 -c $PEER_IP -t 600 --logfile /dev/null &
while :; do
for i in 200 7000 400 5000 300 3000 ; do
ip netns exec ns1 ip link set $VF1 mtu $i
sleep 2
done
sleep 10
done
Currently, callbacks such as iavf_change_mtu() wait for the reset.
If the reset fails to acquire the rtnl_lock, they schedule the netdev
update for later while continuing the reset flow. Operations like MTU
changes are performed under the rtnl_lock. Therefore, when the operation
finishes, another callback that uses rtnl_lock can start.
Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
---
v1->v7: no changes
v8: changed commit msg
v9->v10: no changes
---
drivers/net/ethernet/intel/iavf/iavf_main.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index c815ef87e27d..6945f462c56e 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -2991,11 +2991,6 @@ static void iavf_reset_task(struct work_struct *work)
int i = 0, err;
bool running;
- /* Detach interface to avoid subsequent NDO callbacks */
- rtnl_lock();
- netif_device_detach(netdev);
- rtnl_unlock();
-
/* When device is being removed it doesn't make sense to run the reset
* task, just return in such a case.
*/
@@ -3003,7 +2998,7 @@ static void iavf_reset_task(struct work_struct *work)
if (adapter->state != __IAVF_REMOVE)
queue_work(adapter->wq, &adapter->reset_task);
- goto reset_finish;
+ return;
}
while (!mutex_trylock(&adapter->client_lock))
@@ -3206,7 +3201,7 @@ static void iavf_reset_task(struct work_struct *work)
mutex_unlock(&adapter->client_lock);
mutex_unlock(&adapter->crit_lock);
- goto reset_finish;
+ return;
reset_err:
if (running) {
set_bit(__IAVF_VSI_DOWN, adapter->vsi.state);
@@ -3227,10 +3222,6 @@ static void iavf_reset_task(struct work_struct *work)
}
dev_err(&adapter->pdev->dev, "failed to allocate resources during reinit\n");
-reset_finish:
- rtnl_lock();
- netif_device_attach(netdev);
- rtnl_unlock();
}
/**
--
2.31.1
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-net v10 2/5] Revert "iavf: Detach device during reset task"
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 2/5] Revert "iavf: Detach device during reset task" Mateusz Palczewski
@ 2023-06-15 15:28 ` Romanowski, Rafal
0 siblings, 0 replies; 11+ messages in thread
From: Romanowski, Rafal @ 2023-06-15 15:28 UTC (permalink / raw)
To: Palczewski, Mateusz, intel-wired-lan@lists.osuosl.org; +Cc: ivecera
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Mateusz Palczewski
> Sent: poniedziałek, 5 czerwca 2023 16:52
> To: intel-wired-lan@lists.osuosl.org
> Cc: ivecera <ivecera@redhat.com>
> Subject: [Intel-wired-lan] [PATCH iwl-net v10 2/5] Revert "iavf: Detach device
> during reset task"
>
> From: Marcin Szycik <marcin.szycik@linux.intel.com>
>
> This reverts commit aa626da947e9cd30c4cf727493903e1adbb2c0a0.
>
> Detaching device during reset was not fully fixing the rtnl locking issue, as
> there could be a situation where callback was already in progress before
> detaching netdev.
>
> Furthermore, detaching netdevice causes TX timeouts if traffic is running.
> To reproduce:
>
> ip netns exec ns1 iperf3 -c $PEER_IP -t 600 --logfile /dev/null & while :; do
> for i in 200 7000 400 5000 300 3000 ; do
> ip netns exec ns1 ip link set $VF1 mtu $i
> sleep 2
> done
> sleep 10
> done
>
> Currently, callbacks such as iavf_change_mtu() wait for the reset.
> If the reset fails to acquire the rtnl_lock, they schedule the netdev update
> for later while continuing the reset flow. Operations like MTU changes are
> performed under the rtnl_lock. Therefore, when the operation finishes,
> another callback that uses rtnl_lock can start.
>
> Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> ---
> v1->v7: no changes
> v8: changed commit msg
> v9->v10: no changes
> ---
> drivers/net/ethernet/intel/iavf/iavf_main.c | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index c815ef87e27d..6945f462c56e 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -2991,11 +2991,6 @@ static void iavf_reset_task(struct work_struct
> *work)
> int i = 0, err;
> bool running;
>
> - /* Detach interface to avoid subsequent NDO callbacks */
> - rtnl_lock();
> - netif_device_detach(netdev);
> - rtnl_unlock();
> -
> /* When device is being removed it doesn't make sense to run the
> reset
> * task, just return in such a case.
> */
> @@ -3003,7 +2998,7 @@ static void iavf_reset_task(struct work_struct
> *work)
> if (adapter->state != __IAVF_REMOVE)
> queue_work(adapter->wq, &adapter->reset_task);
>
> - goto reset_finish;
> + return;
> }
>
> while (!mutex_trylock(&adapter->client_lock))
> @@ -3206,7 +3201,7 @@ static void iavf_reset_task(struct work_struct
> *work)
> mutex_unlock(&adapter->client_lock);
> mutex_unlock(&adapter->crit_lock);
>
> - goto reset_finish;
> + return;
> reset_err:
> if (running) {
> set_bit(__IAVF_VSI_DOWN, adapter->vsi.state); @@ -
> 3227,10 +3222,6 @@ static void iavf_reset_task(struct work_struct *work)
> }
>
> dev_err(&adapter->pdev->dev, "failed to allocate resources during
> reinit\n");
> -reset_finish:
> - rtnl_lock();
> - netif_device_attach(netdev);
> - rtnl_unlock();
> }
>
> /**
> --
> 2.31.1
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Intel-wired-lan] [PATCH iwl-net v10 3/5] Revert "iavf: Do not restart Tx queues after reset task failure"
2023-06-05 14:52 [Intel-wired-lan] [PATCH iwl-net v10 0/5] iavf: fix reset task deadlock Mateusz Palczewski
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 1/5] iavf: Wait for reset in callbacks which trigger it Mateusz Palczewski
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 2/5] Revert "iavf: Detach device during reset task" Mateusz Palczewski
@ 2023-06-05 14:52 ` Mateusz Palczewski
2023-06-15 15:28 ` Romanowski, Rafal
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 4/5] iavf: fix a deadlock caused by rtnl and driver's lock circular dependencies Mateusz Palczewski
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 5/5] iavf: fix reset task race with iavf_remove() Mateusz Palczewski
4 siblings, 1 reply; 11+ messages in thread
From: Mateusz Palczewski @ 2023-06-05 14:52 UTC (permalink / raw)
To: intel-wired-lan; +Cc: ivecera
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. We leave the removal of "hacky" IFF_UP flag update.
Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
---
v1->v8: no changes
v9: we leave the removal of "hacky" IFF_UP update that was implemented in this patch
v10: 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 6945f462c56e..50521ba8fbb2 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -2962,6 +2962,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);
@@ -3056,11 +3057,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. */
}
@@ -3211,16 +3207,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");
}
--
2.31.1
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-net v10 3/5] Revert "iavf: Do not restart Tx queues after reset task failure"
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 3/5] Revert "iavf: Do not restart Tx queues after reset task failure" Mateusz Palczewski
@ 2023-06-15 15:28 ` Romanowski, Rafal
0 siblings, 0 replies; 11+ messages in thread
From: Romanowski, Rafal @ 2023-06-15 15:28 UTC (permalink / raw)
To: Palczewski, Mateusz, intel-wired-lan@lists.osuosl.org; +Cc: ivecera
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Mateusz Palczewski
> Sent: poniedziałek, 5 czerwca 2023 16:52
> To: intel-wired-lan@lists.osuosl.org
> Cc: ivecera <ivecera@redhat.com>
> Subject: [Intel-wired-lan] [PATCH iwl-net v10 3/5] Revert "iavf: Do not restart
> Tx queues after reset task failure"
>
> 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.
> We leave the removal of "hacky" IFF_UP flag update.
>
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> ---
> v1->v8: no changes
> v9: we leave the removal of "hacky" IFF_UP update that was implemented in
> this patch
> v10: 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 6945f462c56e..50521ba8fbb2 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -2962,6 +2962,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);
> @@ -3056,11 +3057,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. */
> }
>
> @@ -3211,16 +3207,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"); }
>
> --
> 2.31.1
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Intel-wired-lan] [PATCH iwl-net v10 4/5] iavf: fix a deadlock caused by rtnl and driver's lock circular dependencies
2023-06-05 14:52 [Intel-wired-lan] [PATCH iwl-net v10 0/5] iavf: fix reset task deadlock Mateusz Palczewski
` (2 preceding siblings ...)
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 3/5] Revert "iavf: Do not restart Tx queues after reset task failure" Mateusz Palczewski
@ 2023-06-05 14:52 ` Mateusz Palczewski
2023-06-15 15:27 ` Romanowski, Rafal
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 5/5] iavf: fix reset task race with iavf_remove() Mateusz Palczewski
4 siblings, 1 reply; 11+ messages in thread
From: Mateusz Palczewski @ 2023-06-05 14:52 UTC (permalink / raw)
To: intel-wired-lan; +Cc: ivecera
From: Ahmed Zaki <ahmed.zaki@intel.com>
A driver's lock (crit_lock) is used to serialize all the driver's tasks.
Lockdep, however, shows a circular dependency between rtnl and
crit_lock. This happens when an ndo that already holds the rtnl requests
the driver to reset, since the reset task (in some paths) tries to grab
rtnl to either change real number of queues of update netdev features.
[566.241851] ======================================================
[566.241893] WARNING: possible circular locking dependency detected
[566.241936] 6.2.14-100.fc36.x86_64+debug #1 Tainted: G OE
[566.241984] ------------------------------------------------------
[566.242025] repro.sh/2604 is trying to acquire lock:
[566.242061] ffff9280fc5ceee8 (&adapter->crit_lock){+.+.}-{3:3}, at: iavf_close+0x3c/0x240 [iavf]
[566.242167]
but task is already holding lock:
[566.242209] ffffffff9976d350 (rtnl_mutex){+.+.}-{3:3}, at: iavf_remove+0x6b5/0x730 [iavf]
[566.242300]
which lock already depends on the new lock.
[566.242353]
the existing dependency chain (in reverse order) is:
[566.242401]
-> #1 (rtnl_mutex){+.+.}-{3:3}:
[566.242451] __mutex_lock+0xc1/0xbb0
[566.242489] iavf_init_interrupt_scheme+0x179/0x440 [iavf]
[566.242560] iavf_watchdog_task+0x80b/0x1400 [iavf]
[566.242627] process_one_work+0x2b3/0x560
[566.242663] worker_thread+0x4f/0x3a0
[566.242696] kthread+0xf2/0x120
[566.242730] ret_from_fork+0x29/0x50
[566.242763]
-> #0 (&adapter->crit_lock){+.+.}-{3:3}:
[566.242815] __lock_acquire+0x15ff/0x22b0
[566.242869] lock_acquire+0xd2/0x2c0
[566.242901] __mutex_lock+0xc1/0xbb0
[566.242934] iavf_close+0x3c/0x240 [iavf]
[566.242997] __dev_close_many+0xac/0x120
[566.243036] dev_close_many+0x8b/0x140
[566.243071] unregister_netdevice_many_notify+0x165/0x7c0
[566.243116] unregister_netdevice_queue+0xd3/0x110
[566.243157] iavf_remove+0x6c1/0x730 [iavf]
[566.243217] pci_device_remove+0x33/0xa0
[566.243257] device_release_driver_internal+0x1bc/0x240
[566.243299] pci_stop_bus_device+0x6c/0x90
[566.243338] pci_stop_and_remove_bus_device+0xe/0x20
[566.243380] pci_iov_remove_virtfn+0xd1/0x130
[566.243417] sriov_disable+0x34/0xe0
[566.243448] ice_free_vfs+0x2da/0x330 [ice]
[566.244383] ice_sriov_configure+0x88/0xad0 [ice]
[566.245353] sriov_numvfs_store+0xde/0x1d0
[566.246156] kernfs_fop_write_iter+0x15e/0x210
[566.246921] vfs_write+0x288/0x530
[566.247671] ksys_write+0x74/0xf0
[566.248408] do_syscall_64+0x58/0x80
[566.249145] entry_SYSCALL_64_after_hwframe+0x72/0xdc
[566.249886]
other info that might help us debug this:
[566.252014] Possible unsafe locking scenario:
[566.253432] CPU0 CPU1
[566.254118] ---- ----
[566.254800] lock(rtnl_mutex);
[566.255514] lock(&adapter->crit_lock);
[566.256233] lock(rtnl_mutex);
[566.256897] lock(&adapter->crit_lock);
[566.257388]
*** DEADLOCK ***
The deadlock can be triggered by a script that is continuously resetting
the VF adapter while doing other operations requiring RTNL, e.g:
while :; do
ip link set $VF up
ethtool --set-channels $VF combined 2
ip link set $VF down
ip link set $VF up
ethtool --set-channels $VF combined 4
ip link set $VF down
done
Any operation that triggers a reset can substitute "ethtool --set-channles"
As a fix, add a new task "finish_config" that do all the work which
needs rtnl lock. With the exception of iavf_remove(), all work that
require rtnl should be called from this task.
As for iavf_remove(), at the point where we need to call
unregister_netdevice() (and grab rtnl_lock), we make sure the finish_config
task is not running (cancel_work_sync()) to safely grab rtnl. Subsequent
finish_config work cannot restart after that since the task is guarded
by the __IAVF_IN_REMOVE_TASK bit in iavf_schedule_finish_config().
Fixes: 5ac49f3c2702 ("iavf: use mutexes for locking of critical sections")
Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
---
v1->v3: changes were done internally
v4: added space before open parenthesis '(', fixed code indent
v5: changed the way the scheduled function updates the netdev from { trylock -> reschedule }
design to one that just takes and wait for rtnl_lock lock. Introduced 30ms delay in scheduling
to account for scheduling the resets in quick succession.
v6: added a guard to iavf_delayed_set_interrupt_capability function to prevent updating an unregistered netdev.
Removed the delay from the scheduling and moved the rtnl to obtain the number of queues after acquiring the lock
v7: made it compatible with net-queue, no changes in logic
v8->v9: no changes
v10: Replaced ‘iavf: Don't lock rtnl_lock twice in reset’ with ‘iavf: fix a deadlock caused by rtnl and driver's lock circular dependencies‘
---
drivers/net/ethernet/intel/iavf/iavf.h | 2 +
drivers/net/ethernet/intel/iavf/iavf_main.c | 114 +++++++++++++-----
.../net/ethernet/intel/iavf/iavf_virtchnl.c | 1 +
3 files changed, 85 insertions(+), 32 deletions(-)
diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index c51b9ed4dc29..d8f9833cd288 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -255,6 +255,7 @@ struct iavf_adapter {
struct workqueue_struct *wq;
struct work_struct reset_task;
struct work_struct adminq_task;
+ struct work_struct finish_config;
struct delayed_work client_task;
wait_queue_head_t down_waitqueue;
wait_queue_head_t reset_waitqueue;
@@ -521,6 +522,7 @@ int iavf_process_config(struct iavf_adapter *adapter);
int iavf_parse_vf_resource_msg(struct iavf_adapter *adapter);
void iavf_schedule_reset(struct iavf_adapter *adapter);
void iavf_schedule_request_stats(struct iavf_adapter *adapter);
+void iavf_schedule_finish_config(struct iavf_adapter *adapter);
void iavf_reset(struct iavf_adapter *adapter);
void iavf_set_ethtool_ops(struct net_device *netdev);
void iavf_update_stats(struct iavf_adapter *adapter);
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 50521ba8fbb2..5cab0938aa87 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -1705,10 +1705,10 @@ static int iavf_set_interrupt_capability(struct iavf_adapter *adapter)
adapter->msix_entries[vector].entry = vector;
err = iavf_acquire_msix_vectors(adapter, v_budget);
+ if (!err)
+ iavf_schedule_finish_config(adapter);
out:
- netif_set_real_num_rx_queues(adapter->netdev, pairs);
- netif_set_real_num_tx_queues(adapter->netdev, pairs);
return err;
}
@@ -1927,9 +1927,7 @@ int iavf_init_interrupt_scheme(struct iavf_adapter *adapter)
goto err_alloc_queues;
}
- rtnl_lock();
err = iavf_set_interrupt_capability(adapter);
- rtnl_unlock();
if (err) {
dev_err(&adapter->pdev->dev,
"Unable to setup interrupt capabilities\n");
@@ -2015,6 +2013,78 @@ static int iavf_reinit_interrupt_scheme(struct iavf_adapter *adapter, bool runni
return err;
}
+/**
+ * iavf_finish_config - do all netdev work that needs RTNL
+ * @work: our work_struct
+ *
+ * Do work that needs both RTNL and crit_lock.
+ **/
+static void iavf_finish_config(struct work_struct *work)
+{
+ struct iavf_adapter *adapter;
+ int pairs, err;
+
+ adapter = container_of(work, struct iavf_adapter, finish_config);
+
+ /* Always take RTNL first to prevent circular lock dependency */
+ rtnl_lock();
+ mutex_lock(&adapter->crit_lock);
+
+ if ((adapter->flags & IAVF_FLAG_SETUP_NETDEV_FEATURES) &&
+ adapter->netdev_registered &&
+ !test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section)) {
+ netdev_update_features(adapter->netdev);
+ adapter->flags &= ~IAVF_FLAG_SETUP_NETDEV_FEATURES;
+ }
+
+ switch (adapter->state) {
+ case __IAVF_DOWN:
+ if (!adapter->netdev_registered) {
+ err = register_netdevice(adapter->netdev);
+ if (err) {
+ dev_err(&adapter->pdev->dev,
+ "Unable to register netdev (%d).\n", err);
+
+ /* go back and try again.*/
+ iavf_free_rss(adapter);
+ iavf_free_misc_irq(adapter);
+ iavf_reset_interrupt_capability(adapter);
+ iavf_change_state(adapter,
+ __IAVF_INIT_CONFIG_ADAPTER);
+ goto out;
+ }
+ adapter->netdev_registered = true;
+ }
+
+ /* Set the real number of queues when reset occurs while
+ * state == __IAVF_DOWN
+ */
+ fallthrough;
+ case __IAVF_RUNNING:
+ pairs = adapter->num_active_queues;
+ netif_set_real_num_rx_queues(adapter->netdev, pairs);
+ netif_set_real_num_tx_queues(adapter->netdev, pairs);
+ break;
+
+ default:
+ break;
+ }
+
+out:
+ mutex_unlock(&adapter->crit_lock);
+ rtnl_unlock();
+}
+
+/**
+ * iavf_schedule_finish_config - Set the flags and schedule a reset event
+ * @adapter: board private structure
+ **/
+void iavf_schedule_finish_config(struct iavf_adapter *adapter)
+{
+ if (!test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))
+ queue_work(adapter->wq, &adapter->finish_config);
+}
+
/**
* iavf_process_aq_command - process aq_required flags
* and sends aq command
@@ -2652,22 +2722,8 @@ static void iavf_init_config_adapter(struct iavf_adapter *adapter)
netif_carrier_off(netdev);
adapter->link_up = false;
-
- /* set the semaphore to prevent any callbacks after device registration
- * up to time when state of driver will be set to __IAVF_DOWN
- */
- rtnl_lock();
- if (!adapter->netdev_registered) {
- err = register_netdevice(netdev);
- if (err) {
- rtnl_unlock();
- goto err_register;
- }
- }
-
- adapter->netdev_registered = true;
-
netif_tx_stop_all_queues(netdev);
+
if (CLIENT_ALLOWED(adapter)) {
err = iavf_lan_add_device(adapter);
if (err)
@@ -2680,7 +2736,6 @@ static void iavf_init_config_adapter(struct iavf_adapter *adapter)
iavf_change_state(adapter, __IAVF_DOWN);
set_bit(__IAVF_VSI_DOWN, adapter->vsi.state);
- rtnl_unlock();
iavf_misc_irq_enable(adapter);
wake_up(&adapter->down_waitqueue);
@@ -2700,10 +2755,11 @@ static void iavf_init_config_adapter(struct iavf_adapter *adapter)
/* request initial VLAN offload settings */
iavf_set_vlan_offload_features(adapter, 0, netdev->features);
+ iavf_schedule_finish_config(adapter);
return;
+
err_mem:
iavf_free_rss(adapter);
-err_register:
iavf_free_misc_irq(adapter);
err_sw_init:
iavf_reset_interrupt_capability(adapter);
@@ -2730,15 +2786,6 @@ static void iavf_watchdog_task(struct work_struct *work)
goto restart_watchdog;
}
- if ((adapter->flags & IAVF_FLAG_SETUP_NETDEV_FEATURES) &&
- adapter->netdev_registered &&
- !test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section) &&
- rtnl_trylock()) {
- netdev_update_features(adapter->netdev);
- rtnl_unlock();
- adapter->flags &= ~IAVF_FLAG_SETUP_NETDEV_FEATURES;
- }
-
if (adapter->flags & IAVF_FLAG_PF_COMMS_FAILED)
iavf_change_state(adapter, __IAVF_COMM_FAILED);
@@ -4957,6 +5004,7 @@ static int iavf_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
INIT_WORK(&adapter->reset_task, iavf_reset_task);
INIT_WORK(&adapter->adminq_task, iavf_adminq_task);
+ INIT_WORK(&adapter->finish_config, iavf_finish_config);
INIT_DELAYED_WORK(&adapter->watchdog_task, iavf_watchdog_task);
INIT_DELAYED_WORK(&adapter->client_task, iavf_client_task);
queue_delayed_work(adapter->wq, &adapter->watchdog_task,
@@ -5099,13 +5147,15 @@ static void iavf_remove(struct pci_dev *pdev)
usleep_range(500, 1000);
}
cancel_delayed_work_sync(&adapter->watchdog_task);
+ cancel_work_sync(&adapter->finish_config);
+ rtnl_lock();
if (adapter->netdev_registered) {
- rtnl_lock();
unregister_netdevice(netdev);
adapter->netdev_registered = false;
- rtnl_unlock();
}
+ rtnl_unlock();
+
if (CLIENT_ALLOWED(adapter)) {
err = iavf_lan_del_device(adapter);
if (err)
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 1bab896aaf40..073ac29ed84c 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -2237,6 +2237,7 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
iavf_process_config(adapter);
adapter->flags |= IAVF_FLAG_SETUP_NETDEV_FEATURES;
+ iavf_schedule_finish_config(adapter);
iavf_set_queue_vlan_tag_loc(adapter);
--
2.31.1
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-net v10 4/5] iavf: fix a deadlock caused by rtnl and driver's lock circular dependencies
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 4/5] iavf: fix a deadlock caused by rtnl and driver's lock circular dependencies Mateusz Palczewski
@ 2023-06-15 15:27 ` Romanowski, Rafal
0 siblings, 0 replies; 11+ messages in thread
From: Romanowski, Rafal @ 2023-06-15 15:27 UTC (permalink / raw)
To: Palczewski, Mateusz, intel-wired-lan@lists.osuosl.org; +Cc: ivecera
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Mateusz Palczewski
> Sent: poniedziałek, 5 czerwca 2023 16:52
> To: intel-wired-lan@lists.osuosl.org
> Cc: ivecera <ivecera@redhat.com>
> Subject: [Intel-wired-lan] [PATCH iwl-net v10 4/5] iavf: fix a deadlock caused
> by rtnl and driver's lock circular dependencies
>
> From: Ahmed Zaki <ahmed.zaki@intel.com>
>
> A driver's lock (crit_lock) is used to serialize all the driver's tasks.
> Lockdep, however, shows a circular dependency between rtnl and crit_lock.
> This happens when an ndo that already holds the rtnl requests the driver to
> reset, since the reset task (in some paths) tries to grab rtnl to either change
> real number of queues of update netdev features.
>
> [566.241851]
> ======================================================
> [566.241893] WARNING: possible circular locking dependency detected
> [566.241936] 6.2.14-100.fc36.x86_64+debug #1 Tainted: G OE
> [566.241984] ------------------------------------------------------
> [566.242025] repro.sh/2604 is trying to acquire lock:
> [566.242061] ffff9280fc5ceee8 (&adapter->crit_lock){+.+.}-{3:3}, at:
> iavf_close+0x3c/0x240 [iavf]
> [566.242167]
> but task is already holding lock:
> [566.242209] ffffffff9976d350 (rtnl_mutex){+.+.}-{3:3}, at:
> iavf_remove+0x6b5/0x730 [iavf]
> [566.242300]
> which lock already depends on the new lock.
>
> [566.242353]
> the existing dependency chain (in reverse order) is:
> [566.242401]
> -> #1 (rtnl_mutex){+.+.}-{3:3}:
> [566.242451] __mutex_lock+0xc1/0xbb0
> [566.242489] iavf_init_interrupt_scheme+0x179/0x440 [iavf]
> [566.242560] iavf_watchdog_task+0x80b/0x1400 [iavf]
> [566.242627] process_one_work+0x2b3/0x560
> [566.242663] worker_thread+0x4f/0x3a0
> [566.242696] kthread+0xf2/0x120
> [566.242730] ret_from_fork+0x29/0x50
> [566.242763]
> -> #0 (&adapter->crit_lock){+.+.}-{3:3}:
> [566.242815] __lock_acquire+0x15ff/0x22b0
> [566.242869] lock_acquire+0xd2/0x2c0
> [566.242901] __mutex_lock+0xc1/0xbb0
> [566.242934] iavf_close+0x3c/0x240 [iavf]
> [566.242997] __dev_close_many+0xac/0x120
> [566.243036] dev_close_many+0x8b/0x140
> [566.243071] unregister_netdevice_many_notify+0x165/0x7c0
> [566.243116] unregister_netdevice_queue+0xd3/0x110
> [566.243157] iavf_remove+0x6c1/0x730 [iavf]
> [566.243217] pci_device_remove+0x33/0xa0
> [566.243257] device_release_driver_internal+0x1bc/0x240
> [566.243299] pci_stop_bus_device+0x6c/0x90
> [566.243338] pci_stop_and_remove_bus_device+0xe/0x20
> [566.243380] pci_iov_remove_virtfn+0xd1/0x130
> [566.243417] sriov_disable+0x34/0xe0
> [566.243448] ice_free_vfs+0x2da/0x330 [ice]
> [566.244383] ice_sriov_configure+0x88/0xad0 [ice]
> [566.245353] sriov_numvfs_store+0xde/0x1d0
> [566.246156] kernfs_fop_write_iter+0x15e/0x210
> [566.246921] vfs_write+0x288/0x530
> [566.247671] ksys_write+0x74/0xf0
> [566.248408] do_syscall_64+0x58/0x80
> [566.249145] entry_SYSCALL_64_after_hwframe+0x72/0xdc
> [566.249886]
> other info that might help us debug this:
>
> [566.252014] Possible unsafe locking scenario:
>
> [566.253432] CPU0 CPU1
> [566.254118] ---- ----
> [566.254800] lock(rtnl_mutex);
> [566.255514] lock(&adapter->crit_lock);
> [566.256233] lock(rtnl_mutex);
> [566.256897] lock(&adapter->crit_lock);
> [566.257388]
> *** DEADLOCK ***
>
> The deadlock can be triggered by a script that is continuously resetting the VF
> adapter while doing other operations requiring RTNL, e.g:
>
> while :; do
> ip link set $VF up
> ethtool --set-channels $VF combined 2
> ip link set $VF down
> ip link set $VF up
> ethtool --set-channels $VF combined 4
> ip link set $VF down
> done
>
> Any operation that triggers a reset can substitute "ethtool --set-channles"
>
> As a fix, add a new task "finish_config" that do all the work which needs rtnl
> lock. With the exception of iavf_remove(), all work that require rtnl should
> be called from this task.
>
> As for iavf_remove(), at the point where we need to call
> unregister_netdevice() (and grab rtnl_lock), we make sure the finish_config
> task is not running (cancel_work_sync()) to safely grab rtnl. Subsequent
> finish_config work cannot restart after that since the task is guarded by the
> __IAVF_IN_REMOVE_TASK bit in iavf_schedule_finish_config().
>
> Fixes: 5ac49f3c2702 ("iavf: use mutexes for locking of critical sections")
> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> Signed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> ---
> v1->v3: changes were done internally
> v4: added space before open parenthesis '(', fixed code indent
> v5: changed the way the scheduled function updates the netdev from {
> trylock -> reschedule }
> design to one that just takes and wait for rtnl_lock lock. Introduced 30ms
> delay in scheduling
> to account for scheduling the resets in quick succession.
> v6: added a guard to iavf_delayed_set_interrupt_capability function to
> prevent updating an unregistered netdev.
> Removed the delay from the scheduling and moved the rtnl to obtain the
> number of queues after acquiring the lock
> v7: made it compatible with net-queue, no changes in logic
> v8->v9: no changes
> v10: Replaced ‘iavf: Don't lock rtnl_lock twice in reset’ with ‘iavf: fix a
> deadlock caused by rtnl and driver's lock circular dependencies‘
> ---
> drivers/net/ethernet/intel/iavf/iavf.h | 2 +
> drivers/net/ethernet/intel/iavf/iavf_main.c | 114 +++++++++++++-----
> .../net/ethernet/intel/iavf/iavf_virtchnl.c | 1 +
> 3 files changed, 85 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h
> b/drivers/net/ethernet/intel/iavf/iavf.h
> index c51b9ed4dc29..d8f9833cd288 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
> @@ -255,6 +255,7 @@ struct iavf_adapter {
> struct workqueue_struct *wq;
> struct work_struct reset_task;
> struct work_struct adminq_task;
> + struct work_struct finish_config;
> struct delayed_work client_task;
> wait_queue_head_t down_waitqueue;
> wait_queue_head_t reset_waitqueue;
> @@ -521,6 +522,7 @@ int iavf_process_config(struct iavf_adapter
> *adapter); int iavf_parse_vf_resource_msg(struct iavf_adapter *adapter);
> void iavf_schedule_reset(struct iavf_adapter *adapter); void
> iavf_schedule_request_stats(struct iavf_adapter *adapter);
> +void iavf_schedule_finish_config(struct iavf_adapter *adapter);
> void iavf_reset(struct iavf_adapter *adapter); void
> iavf_set_ethtool_ops(struct net_device *netdev); void
> iavf_update_stats(struct iavf_adapter *adapter); diff --git
> a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index 50521ba8fbb2..5cab0938aa87 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
> @@ -1705,10 +1705,10 @@ static int iavf_set_interrupt_capability(struct
> iavf_adapter *adapter)
> adapter->msix_entries[vector].entry = vector;
>
> err = iavf_acquire_msix_vectors(adapter, v_budget);
> + if (!err)
> + iavf_schedule_finish_config(adapter);
>
> out:
> - netif_set_real_num_rx_queues(adapter->netdev, pairs);
> - netif_set_real_num_tx_queues(adapter->netdev, pairs);
> return err;
> }
>
> @@ -1927,9 +1927,7 @@ int iavf_init_interrupt_scheme(struct iavf_adapter
> *adapter)
> goto err_alloc_queues;
> }
>
> - rtnl_lock();
> err = iavf_set_interrupt_capability(adapter);
> - rtnl_unlock();
> if (err) {
> dev_err(&adapter->pdev->dev,
> "Unable to setup interrupt capabilities\n"); @@ -
> 2015,6 +2013,78 @@ static int iavf_reinit_interrupt_scheme(struct
> iavf_adapter *adapter, bool runni
> return err;
> }
>
> +/**
> + * iavf_finish_config - do all netdev work that needs RTNL
> + * @work: our work_struct
> + *
> + * Do work that needs both RTNL and crit_lock.
> + **/
> +static void iavf_finish_config(struct work_struct *work) {
> + struct iavf_adapter *adapter;
> + int pairs, err;
> +
> + adapter = container_of(work, struct iavf_adapter, finish_config);
> +
> + /* Always take RTNL first to prevent circular lock dependency */
> + rtnl_lock();
> + mutex_lock(&adapter->crit_lock);
> +
> + if ((adapter->flags & IAVF_FLAG_SETUP_NETDEV_FEATURES) &&
> + adapter->netdev_registered &&
> + !test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section)) {
> + netdev_update_features(adapter->netdev);
> + adapter->flags &= ~IAVF_FLAG_SETUP_NETDEV_FEATURES;
> + }
> +
> + switch (adapter->state) {
> + case __IAVF_DOWN:
> + if (!adapter->netdev_registered) {
> + err = register_netdevice(adapter->netdev);
> + if (err) {
> + dev_err(&adapter->pdev->dev,
> + "Unable to register netdev (%d).\n",
> err);
> +
> + /* go back and try again.*/
> + iavf_free_rss(adapter);
> + iavf_free_misc_irq(adapter);
> + iavf_reset_interrupt_capability(adapter);
> + iavf_change_state(adapter,
> +
> __IAVF_INIT_CONFIG_ADAPTER);
> + goto out;
> + }
> + adapter->netdev_registered = true;
> + }
> +
> + /* Set the real number of queues when reset occurs while
> + * state == __IAVF_DOWN
> + */
> + fallthrough;
> + case __IAVF_RUNNING:
> + pairs = adapter->num_active_queues;
> + netif_set_real_num_rx_queues(adapter->netdev, pairs);
> + netif_set_real_num_tx_queues(adapter->netdev, pairs);
> + break;
> +
> + default:
> + break;
> + }
> +
> +out:
> + mutex_unlock(&adapter->crit_lock);
> + rtnl_unlock();
> +}
> +
> +/**
> + * iavf_schedule_finish_config - Set the flags and schedule a reset
> +event
> + * @adapter: board private structure
> + **/
> +void iavf_schedule_finish_config(struct iavf_adapter *adapter) {
> + if (!test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))
> + queue_work(adapter->wq, &adapter->finish_config); }
> +
> /**
> * iavf_process_aq_command - process aq_required flags
> * and sends aq command
> @@ -2652,22 +2722,8 @@ static void iavf_init_config_adapter(struct
> iavf_adapter *adapter)
>
> netif_carrier_off(netdev);
> adapter->link_up = false;
> -
> - /* set the semaphore to prevent any callbacks after device
> registration
> - * up to time when state of driver will be set to __IAVF_DOWN
> - */
> - rtnl_lock();
> - if (!adapter->netdev_registered) {
> - err = register_netdevice(netdev);
> - if (err) {
> - rtnl_unlock();
> - goto err_register;
> - }
> - }
> -
> - adapter->netdev_registered = true;
> -
> netif_tx_stop_all_queues(netdev);
> +
> if (CLIENT_ALLOWED(adapter)) {
> err = iavf_lan_add_device(adapter);
> if (err)
> @@ -2680,7 +2736,6 @@ static void iavf_init_config_adapter(struct
> iavf_adapter *adapter)
>
> iavf_change_state(adapter, __IAVF_DOWN);
> set_bit(__IAVF_VSI_DOWN, adapter->vsi.state);
> - rtnl_unlock();
>
> iavf_misc_irq_enable(adapter);
> wake_up(&adapter->down_waitqueue);
> @@ -2700,10 +2755,11 @@ static void iavf_init_config_adapter(struct
> iavf_adapter *adapter)
> /* request initial VLAN offload settings */
> iavf_set_vlan_offload_features(adapter, 0, netdev-
> >features);
>
> + iavf_schedule_finish_config(adapter);
> return;
> +
> err_mem:
> iavf_free_rss(adapter);
> -err_register:
> iavf_free_misc_irq(adapter);
> err_sw_init:
> iavf_reset_interrupt_capability(adapter);
> @@ -2730,15 +2786,6 @@ static void iavf_watchdog_task(struct work_struct
> *work)
> goto restart_watchdog;
> }
>
> - if ((adapter->flags & IAVF_FLAG_SETUP_NETDEV_FEATURES) &&
> - adapter->netdev_registered &&
> - !test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section) &&
> - rtnl_trylock()) {
> - netdev_update_features(adapter->netdev);
> - rtnl_unlock();
> - adapter->flags &= ~IAVF_FLAG_SETUP_NETDEV_FEATURES;
> - }
> -
> if (adapter->flags & IAVF_FLAG_PF_COMMS_FAILED)
> iavf_change_state(adapter, __IAVF_COMM_FAILED);
>
> @@ -4957,6 +5004,7 @@ static int iavf_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
>
> INIT_WORK(&adapter->reset_task, iavf_reset_task);
> INIT_WORK(&adapter->adminq_task, iavf_adminq_task);
> + INIT_WORK(&adapter->finish_config, iavf_finish_config);
> INIT_DELAYED_WORK(&adapter->watchdog_task,
> iavf_watchdog_task);
> INIT_DELAYED_WORK(&adapter->client_task, iavf_client_task);
> queue_delayed_work(adapter->wq, &adapter->watchdog_task,
> @@ -5099,13 +5147,15 @@ static void iavf_remove(struct pci_dev *pdev)
> usleep_range(500, 1000);
> }
> cancel_delayed_work_sync(&adapter->watchdog_task);
> + cancel_work_sync(&adapter->finish_config);
>
> + rtnl_lock();
> if (adapter->netdev_registered) {
> - rtnl_lock();
> unregister_netdevice(netdev);
> adapter->netdev_registered = false;
> - rtnl_unlock();
> }
> + rtnl_unlock();
> +
> if (CLIENT_ALLOWED(adapter)) {
> err = iavf_lan_del_device(adapter);
> if (err)
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> index 1bab896aaf40..073ac29ed84c 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
> @@ -2237,6 +2237,7 @@ void iavf_virtchnl_completion(struct iavf_adapter
> *adapter,
>
> iavf_process_config(adapter);
> adapter->flags |= IAVF_FLAG_SETUP_NETDEV_FEATURES;
> + iavf_schedule_finish_config(adapter);
>
> iavf_set_queue_vlan_tag_loc(adapter);
>
> --
> 2.31.1
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Intel-wired-lan] [PATCH iwl-net v10 5/5] iavf: fix reset task race with iavf_remove()
2023-06-05 14:52 [Intel-wired-lan] [PATCH iwl-net v10 0/5] iavf: fix reset task deadlock Mateusz Palczewski
` (3 preceding siblings ...)
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 4/5] iavf: fix a deadlock caused by rtnl and driver's lock circular dependencies Mateusz Palczewski
@ 2023-06-05 14:52 ` Mateusz Palczewski
2023-06-15 15:21 ` Romanowski, Rafal
4 siblings, 1 reply; 11+ messages in thread
From: Mateusz Palczewski @ 2023-06-05 14:52 UTC (permalink / raw)
To: intel-wired-lan; +Cc: ivecera
From: Ahmed Zaki <ahmed.zaki@intel.com>
The reset task is currently scheduled from the watchdog or adminq tasks.
First, all direct calls to schedule the reset task are replaced with the
iavf_schedule_reset(), which is modified to accept the flag showing the
type of reset.
To prevent the reset task from starting once iavf_remove() starts, we need
to check the __IAVF_IN_REMOVE_TASK bit before we schedule it. This is now
easily added to iavf_schedule_reset().
Finally, remove the check for IAVF_FLAG_RESET_NEEDED in the watchdog task.
It is redundant since all callers who set the flag immediately schedules
the reset task.
Fixes: 3ccd54ef44eb ("iavf: Fix init state closure on remove")
Fixes: 14756b2ae265 ("iavf: Fix __IAVF_RESETTING state usage")
Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
Singed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
---
v10: Added patch ‘iavf: fix reset task race with iavf_remove()’
---
drivers/net/ethernet/intel/iavf/iavf.h | 2 +-
.../net/ethernet/intel/iavf/iavf_ethtool.c | 8 ++---
drivers/net/ethernet/intel/iavf/iavf_main.c | 32 +++++++------------
.../net/ethernet/intel/iavf/iavf_virtchnl.c | 3 +-
4 files changed, 16 insertions(+), 29 deletions(-)
diff --git a/drivers/net/ethernet/intel/iavf/iavf.h b/drivers/net/ethernet/intel/iavf/iavf.h
index d8f9833cd288..c66c1a69bf67 100644
--- a/drivers/net/ethernet/intel/iavf/iavf.h
+++ b/drivers/net/ethernet/intel/iavf/iavf.h
@@ -520,7 +520,7 @@ int iavf_up(struct iavf_adapter *adapter);
void iavf_down(struct iavf_adapter *adapter);
int iavf_process_config(struct iavf_adapter *adapter);
int iavf_parse_vf_resource_msg(struct iavf_adapter *adapter);
-void iavf_schedule_reset(struct iavf_adapter *adapter);
+void iavf_schedule_reset(struct iavf_adapter *adapter, u64 flags);
void iavf_schedule_request_stats(struct iavf_adapter *adapter);
void iavf_schedule_finish_config(struct iavf_adapter *adapter);
void iavf_reset(struct iavf_adapter *adapter);
diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
index b7141c2a941d..2f47cfa7f06e 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
@@ -532,8 +532,7 @@ static int iavf_set_priv_flags(struct net_device *netdev, u32 flags)
/* issue a reset to force legacy-rx change to take effect */
if (changed_flags & IAVF_FLAG_LEGACY_RX) {
if (netif_running(netdev)) {
- adapter->flags |= IAVF_FLAG_RESET_NEEDED;
- queue_work(adapter->wq, &adapter->reset_task);
+ iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);
ret = iavf_wait_for_reset(adapter);
if (ret)
netdev_warn(netdev, "Changing private flags timeout or interrupted waiting for reset");
@@ -676,8 +675,7 @@ static int iavf_set_ringparam(struct net_device *netdev,
}
if (netif_running(netdev)) {
- adapter->flags |= IAVF_FLAG_RESET_NEEDED;
- queue_work(adapter->wq, &adapter->reset_task);
+ iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);
ret = iavf_wait_for_reset(adapter);
if (ret)
netdev_warn(netdev, "Changing ring parameters timeout or interrupted waiting for reset");
@@ -1860,7 +1858,7 @@ static int iavf_set_channels(struct net_device *netdev,
adapter->num_req_queues = num_req;
adapter->flags |= IAVF_FLAG_REINIT_ITR_NEEDED;
- iavf_schedule_reset(adapter);
+ iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);
ret = iavf_wait_for_reset(adapter);
if (ret)
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 5cab0938aa87..00af281d2f4b 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -309,12 +309,14 @@ int iavf_lock_timeout(struct mutex *lock, unsigned int msecs)
/**
* iavf_schedule_reset - Set the flags and schedule a reset event
* @adapter: board private structure
+ * @flags: IAVF_FLAG_RESET_PENDING or IAVF_FLAG_RESET_NEEDED
**/
-void iavf_schedule_reset(struct iavf_adapter *adapter)
+void iavf_schedule_reset(struct iavf_adapter *adapter, u64 flags)
{
- if (!(adapter->flags &
- (IAVF_FLAG_RESET_PENDING | IAVF_FLAG_RESET_NEEDED))) {
- adapter->flags |= IAVF_FLAG_RESET_NEEDED;
+ if (!test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section) &&
+ !(adapter->flags &
+ (IAVF_FLAG_RESET_PENDING | IAVF_FLAG_RESET_NEEDED))) {
+ adapter->flags |= flags;
queue_work(adapter->wq, &adapter->reset_task);
}
}
@@ -342,7 +344,7 @@ static void iavf_tx_timeout(struct net_device *netdev, unsigned int txqueue)
struct iavf_adapter *adapter = netdev_priv(netdev);
adapter->tx_timeout_count++;
- iavf_schedule_reset(adapter);
+ iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);
}
/**
@@ -2492,7 +2494,7 @@ int iavf_parse_vf_resource_msg(struct iavf_adapter *adapter)
adapter->vsi_res->num_queue_pairs);
adapter->flags |= IAVF_FLAG_REINIT_MSIX_NEEDED;
adapter->num_req_queues = adapter->vsi_res->num_queue_pairs;
- iavf_schedule_reset(adapter);
+ iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);
return -EAGAIN;
}
@@ -2789,14 +2791,6 @@ static void iavf_watchdog_task(struct work_struct *work)
if (adapter->flags & IAVF_FLAG_PF_COMMS_FAILED)
iavf_change_state(adapter, __IAVF_COMM_FAILED);
- if (adapter->flags & IAVF_FLAG_RESET_NEEDED) {
- adapter->aq_required = 0;
- adapter->current_op = VIRTCHNL_OP_UNKNOWN;
- mutex_unlock(&adapter->crit_lock);
- queue_work(adapter->wq, &adapter->reset_task);
- return;
- }
-
switch (adapter->state) {
case __IAVF_STARTUP:
iavf_startup(adapter);
@@ -2924,11 +2918,10 @@ static void iavf_watchdog_task(struct work_struct *work)
/* check for hw reset */
reg_val = rd32(hw, IAVF_VF_ARQLEN1) & IAVF_VF_ARQLEN1_ARQENABLE_MASK;
if (!reg_val) {
- adapter->flags |= IAVF_FLAG_RESET_PENDING;
adapter->aq_required = 0;
adapter->current_op = VIRTCHNL_OP_UNKNOWN;
dev_err(&adapter->pdev->dev, "Hardware reset detected\n");
- queue_work(adapter->wq, &adapter->reset_task);
+ iavf_schedule_reset(adapter, IAVF_FLAG_RESET_PENDING);
mutex_unlock(&adapter->crit_lock);
queue_delayed_work(adapter->wq,
&adapter->watchdog_task, HZ * 2);
@@ -3303,9 +3296,7 @@ static void iavf_adminq_task(struct work_struct *work)
} while (pending);
mutex_unlock(&adapter->crit_lock);
- if ((adapter->flags &
- (IAVF_FLAG_RESET_PENDING | IAVF_FLAG_RESET_NEEDED)) ||
- adapter->state == __IAVF_RESETTING)
+ if (iavf_is_reset_in_progress(adapter))
goto freedom;
/* check for error indications */
@@ -4402,8 +4393,7 @@ static int iavf_change_mtu(struct net_device *netdev, int new_mtu)
}
if (netif_running(netdev)) {
- adapter->flags |= IAVF_FLAG_RESET_NEEDED;
- queue_work(adapter->wq, &adapter->reset_task);
+ iavf_schedule_reset(adapter, IAVF_FLAG_RESET_NEEDED);
ret = iavf_wait_for_reset(adapter);
if (ret < 0)
netdev_warn(netdev, "MTU change interrupted waiting for reset");
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
index 073ac29ed84c..be3c007ce90a 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c
@@ -1961,9 +1961,8 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
case VIRTCHNL_EVENT_RESET_IMPENDING:
dev_info(&adapter->pdev->dev, "Reset indication received from the PF\n");
if (!(adapter->flags & IAVF_FLAG_RESET_PENDING)) {
- adapter->flags |= IAVF_FLAG_RESET_PENDING;
dev_info(&adapter->pdev->dev, "Scheduling reset task\n");
- queue_work(adapter->wq, &adapter->reset_task);
+ iavf_schedule_reset(adapter, IAVF_FLAG_RESET_PENDING);
}
break;
default:
--
2.31.1
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [Intel-wired-lan] [PATCH iwl-net v10 5/5] iavf: fix reset task race with iavf_remove()
2023-06-05 14:52 ` [Intel-wired-lan] [PATCH iwl-net v10 5/5] iavf: fix reset task race with iavf_remove() Mateusz Palczewski
@ 2023-06-15 15:21 ` Romanowski, Rafal
0 siblings, 0 replies; 11+ messages in thread
From: Romanowski, Rafal @ 2023-06-15 15:21 UTC (permalink / raw)
To: Palczewski, Mateusz, intel-wired-lan@lists.osuosl.org; +Cc: ivecera
> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Mateusz Palczewski
> Sent: poniedziałek, 5 czerwca 2023 16:52
> To: intel-wired-lan@lists.osuosl.org
> Cc: ivecera <ivecera@redhat.com>
> Subject: [Intel-wired-lan] [PATCH iwl-net v10 5/5] iavf: fix reset task race with
> iavf_remove()
>
> From: Ahmed Zaki <ahmed.zaki@intel.com>
>
> The reset task is currently scheduled from the watchdog or adminq tasks.
> First, all direct calls to schedule the reset task are replaced with the
> iavf_schedule_reset(), which is modified to accept the flag showing the type
> of reset.
>
> To prevent the reset task from starting once iavf_remove() starts, we need
> to check the __IAVF_IN_REMOVE_TASK bit before we schedule it. This is
> now easily added to iavf_schedule_reset().
>
> Finally, remove the check for IAVF_FLAG_RESET_NEEDED in the watchdog
> task.
> It is redundant since all callers who set the flag immediately schedules the
> reset task.
>
> Fixes: 3ccd54ef44eb ("iavf: Fix init state closure on remove")
> Fixes: 14756b2ae265 ("iavf: Fix __IAVF_RESETTING state usage")
> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com>
> Singed-off-by: Mateusz Palczewski <mateusz.palczewski@intel.com>
> ---
> v10: Added patch ‘iavf: fix reset task race with iavf_remove()’
> ---
> drivers/net/ethernet/intel/iavf/iavf.h | 2 +-
> .../net/ethernet/intel/iavf/iavf_ethtool.c | 8 ++---
> drivers/net/ethernet/intel/iavf/iavf_main.c | 32 +++++++------------
> .../net/ethernet/intel/iavf/iavf_virtchnl.c | 3 +-
> 4 files changed, 16 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf.h
> b/drivers/net/ethernet/intel/iavf/iavf.h
> index d8f9833cd288..c66c1a69bf67 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf.h
Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
^ permalink raw reply [flat|nested] 11+ messages in thread