[iwl-net,v2] iavf: Fix iavf_shutdown to call iavf_remove instead iavf_close

Commit Message

Ranganatha Rao Nov. 29, 2023, 3:35 p.m. UTC

From: Slawomir Laba <[e-mail]>

Make the flow for pci shutdown be the same to the pci remove.

iavf_shutdown was implementing an incomplete version
of iavf_remove. It misses several calls to the kernel like
iavf_free_misc_irq, iavf_reset_interrupt_capability, iounmap
that might break the system on reboot or hibernation.

Implement the call of iavf_remove directly in iavf_shutdown to
close this gap.

Fixes below error messages (dmesg) during shutdown stress tests -
[685814.900917] ice 0000:88:00.0: MAC 02:d0:5f:82:43:5d does not exist for
 VF 0
[685814.900928] ice 0000:88:00.0: MAC 33:33:00:00:00:01 does not exist for
VF 0

Reproduction:

1. Create one VF interface:
echo 1 > /sys/class/net/<interface_name>/device/sriov_numvfs

2. Run live dmesg on the host:
dmesg -wH

3. On SUT, script below steps into vf_namespace_assignment.sh

<#!/bin/sh> // Remove <>. Git removes # line
if=<VF name> (edit this per VF name)
loop=0

while true; do

echo test round $loop
let loop++

ip netns add ns$loop
ip link set dev $if up
ip link set dev $if netns ns$loop
ip netns exec ns$loop ip link set dev $if up
ip netns exec ns$loop ip link set dev $if netns 1
ip netns delete ns$loop

done

4. Run the script for at least 1000 iterations on SUT:
./vf_namespace_assignment.sh

Expected result:
No errors in dmesg.

Fixes: 129cf89e5856 ("iavf: rename functions and structs to new name")
Signed-off-by: Slawomir Laba <[e-mail]>
Reviewed-by: Michal Swiatkowski <[e-mail]>
Reviewed-by: Ahmed Zaki <[e-mail]>
Reviewed-by: Jesse Brandeburg <[e-mail]>
Co-authored-by: Ranganatha Rao <[e-mail]>
Signed-off-by: Ranganatha Rao <[e-mail]>

---
v2: Add reproduction steps in commit log
---
 drivers/net/ethernet/intel/iavf/iavf_main.c | 72 ++++++---------------
 1 file changed, 21 insertions(+), 51 deletions(-)

Comments

Romanowski, Rafal Dec. 6, 2023, 7:39 p.m. UTC | #1

> -----Original Message-----
> From: Intel-wired-lan <[e-mail]> On Behalf Of
> Ranganatha Rao
> Sent: Wednesday, November 29, 2023 4:35 PM
> To: [e-mail]
> Cc: Laba, SlawomirX <[e-mail]>; [e-mail];
> Brandeburg, Jesse <[e-mail]>; Zaki, Ahmed
> <[e-mail]>; Rao, Ranganatha <[e-mail]>;
> Michal Swiatkowski <[e-mail]>
> Subject: [Intel-wired-lan] [PATCH iwl-net, v2] iavf: Fix iavf_shutdown to call
> iavf_remove instead iavf_close
> 
> From: Slawomir Laba <[e-mail]>
> 
> Make the flow for pci shutdown be the same to the pci remove.
> 
> iavf_shutdown was implementing an incomplete version of iavf_remove. It
> misses several calls to the kernel like iavf_free_misc_irq,
> iavf_reset_interrupt_capability, iounmap that might break the system on
> reboot or hibernation.
> 
> Implement the call of iavf_remove directly in iavf_shutdown to close this gap.
> 
> Fixes below error messages (dmesg) during shutdown stress tests -
> [685814.900917] ice 0000:88:00.0: MAC 02:d0:5f:82:43:5d does not exist
> for  VF 0 [685814.900928] ice 0000:88:00.0: MAC 33:33:00:00:00:01 does
> not exist for VF 0
> 
> Reproduction:
> 
> 1. Create one VF interface:
> echo 1 > /sys/class/net/<interface_name>/device/sriov_numvfs
> 
> 2. Run live dmesg on the host:
> dmesg -wH
> 
> 3. On SUT, script below steps into vf_namespace_assignment.sh
> 
> <#!/bin/sh> // Remove <>. Git removes # line if=<VF name> (edit this per VF
> name)
> loop=0
> 
> while true; do
> 
> echo test round $loop
> let loop++
> 
> ip netns add ns$loop
> ip link set dev $if up
> ip link set dev $if netns ns$loop
> ip netns exec ns$loop ip link set dev $if up ip netns exec ns$loop ip link set dev
> $if netns 1 ip netns delete ns$loop
> 
> done
> 
> 4. Run the script for at least 1000 iterations on SUT:
> ./vf_namespace_assignment.sh
> 
> Expected result:
> No errors in dmesg.
> 
> Fixes: 129cf89e5856 ("iavf: rename functions and structs to new name")
> Signed-off-by: Slawomir Laba <[e-mail]>
> Reviewed-by: Michal Swiatkowski <[e-mail]>
> Reviewed-by: Ahmed Zaki <[e-mail]>
> Reviewed-by: Jesse Brandeburg <[e-mail]>
> Co-authored-by: Ranganatha Rao <[e-mail]>
> Signed-off-by: Ranganatha Rao <[e-mail]>
> 
> ---
> v2: Add reproduction steps in commit log
> ---
>  drivers/net/ethernet/intel/iavf/iavf_main.c | 72 ++++++---------------
>  1 file changed, 21 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
> b/drivers/net/ethernet/intel/iavf/iavf_main.c
> index c862ebcd2e39..3c177dcd3b38 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c


Tested-by: Rafal Romanowski <[e-mail]>

Patch

diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c
index c862ebcd2e39..3c177dcd3b38 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -276,27 +276,6 @@  void iavf_free_virt_mem(struct iavf_hw *hw, struct iavf_virt_mem *mem)
 	kfree(mem->va);
 }
 
-/**
- * iavf_lock_timeout - try to lock mutex but give up after timeout
- * @lock: mutex that should be locked
- * @msecs: timeout in msecs
- *
- * Returns 0 on success, negative on failure
- **/
-static int iavf_lock_timeout(struct mutex *lock, unsigned int msecs)
-{
-	unsigned int wait, delay = 10;
-
-	for (wait = 0; wait < msecs; wait += delay) {
-		if (mutex_trylock(lock))
-			return 0;
-
-		msleep(delay);
-	}
-
-	return -1;
-}
-
 /**
  * iavf_schedule_reset - Set the flags and schedule a reset event
  * @adapter: board private structure
@@ -4825,34 +4804,6 @@  int iavf_process_config(struct iavf_adapter *adapter)
 	return 0;
 }
 
-/**
- * iavf_shutdown - Shutdown the device in preparation for a reboot
- * @pdev: pci device structure
- **/
-static void iavf_shutdown(struct pci_dev *pdev)
-{
-	struct iavf_adapter *adapter = iavf_pdev_to_adapter(pdev);
-	struct net_device *netdev = adapter->netdev;
-
-	netif_device_detach(netdev);
-
-	if (netif_running(netdev))
-		iavf_close(netdev);
-
-	if (iavf_lock_timeout(&adapter->crit_lock, 5000))
-		dev_warn(&adapter->pdev->dev, "%s: failed to acquire crit_lock\n", __func__);
-	/* Prevent the watchdog from running. */
-	iavf_change_state(adapter, __IAVF_REMOVE);
-	adapter->aq_required = 0;
-	mutex_unlock(&adapter->crit_lock);
-
-#ifdef CONFIG_PM
-	pci_save_state(pdev);
-
-#endif
-	pci_disable_device(pdev);
-}
-
 /**
  * iavf_probe - Device Initialization Routine
  * @pdev: PCI device information struct
@@ -5063,16 +5014,21 @@  static int __maybe_unused iavf_resume(struct device *dev_d)
  **/
 static void iavf_remove(struct pci_dev *pdev)
 {
-	struct iavf_adapter *adapter = iavf_pdev_to_adapter(pdev);
 	struct iavf_fdir_fltr *fdir, *fdirtmp;
 	struct iavf_vlan_filter *vlf, *vlftmp;
 	struct iavf_cloud_filter *cf, *cftmp;
 	struct iavf_adv_rss *rss, *rsstmp;
 	struct iavf_mac_filter *f, *ftmp;
+	struct iavf_adapter *adapter;
 	struct net_device *netdev;
 	struct iavf_hw *hw;
 
-	netdev = adapter->netdev;
+	/* Don't proceed with remove if netdev is already freed */
+	netdev = pci_get_drvdata(pdev);
+	if (!netdev)
+		return;
+
+	adapter = iavf_pdev_to_adapter(pdev);
 	hw = &adapter->hw;
 
 	if (test_and_set_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))
@@ -5184,11 +5140,25 @@  static void iavf_remove(struct pci_dev *pdev)
 
 	destroy_workqueue(adapter->wq);
 
+	pci_set_drvdata(pdev, NULL);
+
 	free_netdev(netdev);
 
 	pci_disable_device(pdev);
 }
 
+/**
+ * iavf_shutdown - Shutdown the device in preparation for a reboot
+ * @pdev: pci device structure
+ **/
+static void iavf_shutdown(struct pci_dev *pdev)
+{
+	iavf_remove(pdev);
+
+	if (system_state == SYSTEM_POWER_OFF)
+		pci_set_power_state(pdev, PCI_D3hot);
+}
+
 static SIMPLE_DEV_PM_OPS(iavf_pm_ops, iavf_suspend, iavf_resume);
 
 static struct pci_driver iavf_driver = {