[PATCH iwl-net v2 0/5] idpf: fix issues in the reset handling path

[PATCH iwl-net v2 0/5] idpf: fix issues in the reset handling path

[Emil Tantilov]

Resolve multiple issues in the error path during reset handling of the
IDPF driver:
- Mailbox timeouts in the init task during a reset lead to the netdevs
  being unregistered.
- Reset times out and leaves the netdevs exposed to callbacks with the
  vport resources already freed.
- Simultaneous reset and soft reset calls will result in the loss of the
  vport state, leaving the netdev in DOWN state.
- Memory leak in idpf_vport_rel() where vport->rx_ptype_lkup was not
  freed during a reset.
- Memory leak in idpf_vc_core_deinit(), where kfree() was missing for
  hw->lan_regs.
- Crash on reset following a failed init on load, due to the service
  tasks still running in the background and attempting to handle the
  reset while resources are not initialized.

Changelog:
v2:
 - Patch 1 - No change.
 - Patch 2 - Changed the check for vport being NULL by accessing it via the
   adapter struct instead of idpf_netdev_priv. This avoids potential NULL
   pointer in the case where the init task failed on driver load.
   Updated comments to clarify vports can be NULL before the call to
   idpf_attach_and_open().
 - Patch 3 - No change.
 - Patch 4 - No change.
 - Patch 5 - New to the series, to fix a crash, following a failed driver
   load due to an error in the init task.

v1: https://lore.kernel.org/intel-wired-lan/20251110180823.18008-1-emil.s.tantilov@intel.com/

Emil Tantilov (5):
  idpf: keep the netdev when a reset fails
  idpf: detach and close netdevs while handling a reset
  idpf: fix memory leak in idpf_vport_rel()
  idpf: fix memory leak in idpf_vc_core_deinit()
  idpf: fix error handling in the init_task on load

 drivers/net/ethernet/intel/idpf/idpf_lib.c    | 156 +++++++++++-------
 .../net/ethernet/intel/idpf/idpf_virtchnl.c   |   4 +
 2 files changed, 96 insertions(+), 64 deletions(-)

-- 
2.37.3

[PATCH iwl-net v2 1/5] idpf: keep the netdev when a reset fails

[Emil Tantilov]

During a successful reset the driver would re-allocate vport resources
while keeping the netdevs intact. However, in case of an error in the
init task, the netdev of the failing vport will be unregistered,
effectively removing the network interface:

[  121.211076] idpf 0000:83:00.0: enabling device (0100 -> 0102)
[  121.221976] idpf 0000:83:00.0: Device HW Reset initiated
[  124.161229] idpf 0000:83:00.0 ens801f0: renamed from eth0
[  124.163364] idpf 0000:83:00.0 ens801f0d1: renamed from eth1
[  125.934656] idpf 0000:83:00.0 ens801f0d2: renamed from eth2
[  128.218429] idpf 0000:83:00.0 ens801f0d3: renamed from eth3

ip -br a
ens801f0         UP
ens801f0d1       UP
ens801f0d2       UP
ens801f0d3       UP
echo 1 > /sys/class/net/ens801f0/device/reset

[  145.885537] idpf 0000:83:00.0: resetting
[  145.990280] idpf 0000:83:00.0: reset done
[  146.284766] idpf 0000:83:00.0: HW reset detected
[  146.296610] idpf 0000:83:00.0: Device HW Reset initiated
[  211.556719] idpf 0000:83:00.0: Transaction timed-out (op:526 cookie:7700 vc_op:526 salt:77 timeout:60000ms)
[  272.996705] idpf 0000:83:00.0: Transaction timed-out (op:502 cookie:7800 vc_op:502 salt:78 timeout:60000ms)

ip -br a
ens801f0d1       DOWN
ens801f0d2       DOWN
ens801f0d3       DOWN

Re-shuffle the logic in the error path of the init task to make sure the
netdevs remain intact. This will allow the driver to attempt recovery via
subsequent resets, provided the FW is still functional.

The main change is to make sure that idpf_decfg_netdev() is not called
should the init task fail during a reset. The error handling is
consolidated under unwind_vports, as the removed labels had the same
cleanup logic split depending on the point of failure.

Fixes: ce1b75d0635c ("idpf: add ptypes and MAC filter support")
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_lib.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index bd38ecc7872c..2a53f3d504d2 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -1604,6 +1604,10 @@ void idpf_init_task(struct work_struct *work)
 		goto unwind_vports;
 	}
 
+	err = idpf_send_get_rx_ptype_msg(vport);
+	if (err)
+		goto unwind_vports;
+
 	index = vport->idx;
 	vport_config = adapter->vport_config[index];
 
@@ -1616,15 +1620,11 @@ void idpf_init_task(struct work_struct *work)
 	err = idpf_check_supported_desc_ids(vport);
 	if (err) {
 		dev_err(&pdev->dev, "failed to get required descriptor ids\n");
-		goto cfg_netdev_err;
+		goto unwind_vports;
 	}
 
 	if (idpf_cfg_netdev(vport))
-		goto cfg_netdev_err;
-
-	err = idpf_send_get_rx_ptype_msg(vport);
-	if (err)
-		goto handle_err;
+		goto unwind_vports;
 
 	/* Once state is put into DOWN, driver is ready for dev_open */
 	np = netdev_priv(vport->netdev);
@@ -1671,11 +1671,6 @@ void idpf_init_task(struct work_struct *work)
 
 	return;
 
-handle_err:
-	idpf_decfg_netdev(vport);
-cfg_netdev_err:
-	idpf_vport_rel(vport);
-	adapter->vports[index] = NULL;
 unwind_vports:
 	if (default_vport) {
 		for (index = 0; index < adapter->max_vports; index++) {
-- 
2.37.3

[PATCH iwl-net v2 2/5] idpf: detach and close netdevs while handling a reset

[Emil Tantilov]

Protect the reset path from callbacks by setting the netdevs to detached
state and close any netdevs in UP state until the reset handling has
completed. During a reset, the driver will de-allocate resources for the
vport, and there is no guarantee that those will recover, which is why the
existing vport_ctrl_lock does not provide sufficient protection.

idpf_detach_and_close() is called right before reset handling. If the
reset handling succeeds, the netdevs state is recovered via call to
idpf_attach_and_open(). If the reset handling fails the netdevs remain
down. The detach/down calls are protected with RTNL lock to avoid racing
with callbacks. On the recovery side the attach can be done without
holding the RTNL lock as there are no callbacks expected at that point,
due to detach/close always being done first in that flow.

The previous logic restoring the netdevs state based on the
IDPF_VPORT_UP_REQUESTED flag in the init task is not needed anymore, hence
the removal of idpf_set_vport_state(). The IDPF_VPORT_UP_REQUESTED is
still being used to restore the state of the netdevs following the reset,
but has no use outside of the reset handling flow.

idpf_init_hard_reset() is converted to void, since it was used as such and
there is no error handling being done based on its return value.

Before this change, invoking hard and soft resets simultaneously will
cause the driver to lose the vport state:
ip -br a
<inf>	UP
echo 1 > /sys/class/net/ens801f0/device/reset& \
ethtool -L ens801f0 combined 8
ip -br a
<inf>	DOWN
ip link set <inf> up
ip -br a
<inf>	DOWN

Also in case of a failure in the reset path, the netdev is left
exposed to external callbacks, while vport resources are not
initialized, leading to a crash on subsequent ifup/down:
[408471.398966] idpf 0000:83:00.0: HW reset detected
[408471.411744] idpf 0000:83:00.0: Device HW Reset initiated
[408472.277901] idpf 0000:83:00.0: The driver was unable to contact the device's firmware. Check that the FW is running. Driver state= 0x2
[408508.125551] BUG: kernel NULL pointer dereference, address: 0000000000000078
[408508.126112] #PF: supervisor read access in kernel mode
[408508.126687] #PF: error_code(0x0000) - not-present page
[408508.127256] PGD 2aae2f067 P4D 0
[408508.127824] Oops: Oops: 0000 [#1] SMP NOPTI
...
[408508.130871] RIP: 0010:idpf_stop+0x39/0x70 [idpf]
...
[408508.139193] Call Trace:
[408508.139637]  <TASK>
[408508.140077]  __dev_close_many+0xbb/0x260
[408508.140533]  __dev_change_flags+0x1cf/0x280
[408508.140987]  netif_change_flags+0x26/0x70
[408508.141434]  dev_change_flags+0x3d/0xb0
[408508.141878]  devinet_ioctl+0x460/0x890
[408508.142321]  inet_ioctl+0x18e/0x1d0
[408508.142762]  ? _copy_to_user+0x22/0x70
[408508.143207]  sock_do_ioctl+0x3d/0xe0
[408508.143652]  sock_ioctl+0x10e/0x330
[408508.144091]  ? find_held_lock+0x2b/0x80
[408508.144537]  __x64_sys_ioctl+0x96/0xe0
[408508.144979]  do_syscall_64+0x79/0x3d0
[408508.145415]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
[408508.145860] RIP: 0033:0x7f3e0bb4caff

Fixes: 0fe45467a104 ("idpf: add create vport and netdev configuration")
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_lib.c | 121 ++++++++++++---------
 1 file changed, 72 insertions(+), 49 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index 2a53f3d504d2..5c81f52db266 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -752,6 +752,65 @@ static int idpf_init_mac_addr(struct idpf_vport *vport,
 	return 0;
 }
 
+static void idpf_detach_and_close(struct idpf_adapter *adapter)
+{
+	int max_vports = adapter->max_vports;
+
+	for (int i = 0; i < max_vports; i++) {
+		struct net_device *netdev = adapter->netdevs[i];
+
+		/* If the interface is in detached state, that means the
+		 * previous reset was not handled successfully for this
+		 * vport.
+		 */
+		if (!netif_device_present(netdev))
+			continue;
+
+		/* Hold RTNL to protect racing with callbacks */
+		rtnl_lock();
+		netif_device_detach(netdev);
+		if (netif_running(netdev)) {
+			set_bit(IDPF_VPORT_UP_REQUESTED,
+				adapter->vport_config[i]->flags);
+			dev_close(netdev);
+		}
+		rtnl_unlock();
+	}
+}
+
+static void idpf_attach_and_open(struct idpf_adapter *adapter)
+{
+	int max_vports = adapter->max_vports;
+
+	for (int i = 0; i < max_vports; i++) {
+		struct idpf_vport *vport = adapter->vports[i];
+		struct idpf_vport_config *vport_config;
+		struct net_device *netdev;
+
+		/* In case of a critical error in the init task, the vport
+		 * will be freed. Only continue to restore the netdevs
+		 * if the vport is allocated.
+		 */
+		if (!vport)
+			continue;
+
+		/* No need for RTNL on attach as this function is called
+		 * following detach and dev_close(). We do take RTNL for
+		 * dev_open() below as it can race with external callbacks
+		 * following the call to netif_device_attach().
+		 */
+		netdev = adapter->netdevs[i];
+		netif_device_attach(netdev);
+		vport_config = adapter->vport_config[vport->idx];
+		if (test_and_clear_bit(IDPF_VPORT_UP_REQUESTED,
+				       vport_config->flags)) {
+			rtnl_lock();
+			dev_open(netdev, NULL);
+			rtnl_unlock();
+		}
+	}
+}
+
 /**
  * idpf_cfg_netdev - Allocate, configure and register a netdev
  * @vport: main vport structure
@@ -1064,10 +1123,11 @@ static void idpf_vport_dealloc(struct idpf_vport *vport)
 	idpf_idc_deinit_vport_aux_device(vport->vdev_info);
 
 	idpf_deinit_mac_addr(vport);
-	idpf_vport_stop(vport, true);
 
-	if (!test_bit(IDPF_HR_RESET_IN_PROG, adapter->flags))
+	if (!test_bit(IDPF_HR_RESET_IN_PROG, adapter->flags)) {
+		idpf_vport_stop(vport, true);
 		idpf_decfg_netdev(vport);
+	}
 	if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags)) {
 		idpf_del_all_mac_filters(vport);
 		idpf_del_all_flow_steer_filters(vport);
@@ -1569,7 +1629,6 @@ void idpf_init_task(struct work_struct *work)
 	struct idpf_vport_config *vport_config;
 	struct idpf_vport_max_q max_q;
 	struct idpf_adapter *adapter;
-	struct idpf_netdev_priv *np;
 	struct idpf_vport *vport;
 	u16 num_default_vports;
 	struct pci_dev *pdev;
@@ -1626,12 +1685,6 @@ void idpf_init_task(struct work_struct *work)
 	if (idpf_cfg_netdev(vport))
 		goto unwind_vports;
 
-	/* Once state is put into DOWN, driver is ready for dev_open */
-	np = netdev_priv(vport->netdev);
-	np->state = __IDPF_VPORT_DOWN;
-	if (test_and_clear_bit(IDPF_VPORT_UP_REQUESTED, vport_config->flags))
-		idpf_vport_open(vport, true);
-
 	/* Spawn and return 'idpf_init_task' work queue until all the
 	 * default vports are created
 	 */
@@ -1807,27 +1860,6 @@ static int idpf_check_reset_complete(struct idpf_hw *hw,
 	return -EBUSY;
 }
 
-/**
- * idpf_set_vport_state - Set the vport state to be after the reset
- * @adapter: Driver specific private structure
- */
-static void idpf_set_vport_state(struct idpf_adapter *adapter)
-{
-	u16 i;
-
-	for (i = 0; i < adapter->max_vports; i++) {
-		struct idpf_netdev_priv *np;
-
-		if (!adapter->netdevs[i])
-			continue;
-
-		np = netdev_priv(adapter->netdevs[i]);
-		if (np->state == __IDPF_VPORT_UP)
-			set_bit(IDPF_VPORT_UP_REQUESTED,
-				adapter->vport_config[i]->flags);
-	}
-}
-
 /**
  * idpf_init_hard_reset - Initiate a hardware reset
  * @adapter: Driver specific private structure
@@ -1836,28 +1868,17 @@ static void idpf_set_vport_state(struct idpf_adapter *adapter)
  * reallocate. Also reinitialize the mailbox. Return 0 on success,
  * negative on failure.
  */
-static int idpf_init_hard_reset(struct idpf_adapter *adapter)
+static void idpf_init_hard_reset(struct idpf_adapter *adapter)
 {
 	struct idpf_reg_ops *reg_ops = &adapter->dev_ops.reg_ops;
 	struct device *dev = &adapter->pdev->dev;
-	struct net_device *netdev;
 	int err;
-	u16 i;
 
+	idpf_detach_and_close(adapter);
 	mutex_lock(&adapter->vport_ctrl_lock);
 
 	dev_info(dev, "Device HW Reset initiated\n");
 
-	/* Avoid TX hangs on reset */
-	for (i = 0; i < adapter->max_vports; i++) {
-		netdev = adapter->netdevs[i];
-		if (!netdev)
-			continue;
-
-		netif_carrier_off(netdev);
-		netif_tx_disable(netdev);
-	}
-
 	/* Prepare for reset */
 	if (test_and_clear_bit(IDPF_HR_DRV_LOAD, adapter->flags)) {
 		reg_ops->trigger_reset(adapter, IDPF_HR_DRV_LOAD);
@@ -1866,7 +1887,6 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
 
 		idpf_idc_issue_reset_event(adapter->cdev_info);
 
-		idpf_set_vport_state(adapter);
 		idpf_vc_core_deinit(adapter);
 		if (!is_reset)
 			reg_ops->trigger_reset(adapter, IDPF_HR_FUNC_RESET);
@@ -1913,11 +1933,14 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
 unlock_mutex:
 	mutex_unlock(&adapter->vport_ctrl_lock);
 
-	/* Wait until all vports are created to init RDMA CORE AUX */
-	if (!err)
-		err = idpf_idc_init(adapter);
-
-	return err;
+	/* Attempt to restore netdevs and initialize RDMA CORE AUX device,
+	 * provided vc_core_init succeeded. It is still possible that
+	 * vports are not allocated at this point if the init task failed.
+	 */
+	if (!err) {
+		idpf_attach_and_open(adapter);
+		idpf_idc_init(adapter);
+	}
 }
 
 /**
-- 
2.37.3

[PATCH iwl-net v2 3/5] idpf: fix memory leak in idpf_vport_rel()

[Emil Tantilov]

Free vport->rx_ptype_lkup in idpf_vport_rel() to avoid leaking memory
during a reset. Reported by kmemleak:

unreferenced object 0xff450acac838a000 (size 4096):
  comm "kworker/u258:5", pid 7732, jiffies 4296830044
  hex dump (first 32 bytes):
    00 00 00 00 00 10 00 00 00 10 00 00 00 00 00 00  ................
    00 00 00 00 00 00 00 00 00 10 00 00 00 00 00 00  ................
  backtrace (crc 3da81902):
    __kmalloc_cache_noprof+0x469/0x7a0
    idpf_send_get_rx_ptype_msg+0x90/0x570 [idpf]
    idpf_init_task+0x1ec/0x8d0 [idpf]
    process_one_work+0x226/0x6d0
    worker_thread+0x19e/0x340
    kthread+0x10f/0x250
    ret_from_fork+0x251/0x2b0
    ret_from_fork_asm+0x1a/0x30

Fixes: 0fe45467a104 ("idpf: add create vport and netdev configuration")
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_lib.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index 5c81f52db266..5193968c9bb1 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -1105,6 +1105,8 @@ static void idpf_vport_rel(struct idpf_vport *vport)
 		kfree(adapter->vport_config[idx]->req_qs_chunks);
 		adapter->vport_config[idx]->req_qs_chunks = NULL;
 	}
+	kfree(vport->rx_ptype_lkup);
+	vport->rx_ptype_lkup = NULL;
 	kfree(vport);
 	adapter->num_alloc_vports--;
 }
-- 
2.37.3

[PATCH iwl-net v2 4/5] idpf: fix memory leak in idpf_vc_core_deinit()

[Emil Tantilov]

Make sure to free hw->lan_regs. Reported by kmemleak during reset:

unreferenced object 0xff1b913d02a936c0 (size 96):
  comm "kworker/u258:14", pid 2174, jiffies 4294958305
  hex dump (first 32 bytes):
    00 00 00 c0 a8 ba 2d ff 00 00 00 00 00 00 00 00  ......-.........
    00 00 40 08 00 00 00 00 00 00 25 b3 a8 ba 2d ff  ..@.......%...-.
  backtrace (crc 36063c4f):
    __kmalloc_noprof+0x48f/0x890
    idpf_vc_core_init+0x6ce/0x9b0 [idpf]
    idpf_vc_event_task+0x1fb/0x350 [idpf]
    process_one_work+0x226/0x6d0
    worker_thread+0x19e/0x340
    kthread+0x10f/0x250
    ret_from_fork+0x251/0x2b0
    ret_from_fork_asm+0x1a/0x30

Fixes: 6aa53e861c1a ("idpf: implement get LAN MMIO memory regions")
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Joshua Hay <joshua.a.hay@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_virtchnl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
index fc03d55bc9b9..ca302df9ff40 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl.c
@@ -3570,6 +3570,7 @@ int idpf_vc_core_init(struct idpf_adapter *adapter)
  */
 void idpf_vc_core_deinit(struct idpf_adapter *adapter)
 {
+	struct idpf_hw *hw = &adapter->hw;
 	bool remove_in_prog;
 
 	if (!test_bit(IDPF_VC_CORE_INIT, adapter->flags))
@@ -3593,6 +3594,9 @@ void idpf_vc_core_deinit(struct idpf_adapter *adapter)
 
 	idpf_vport_params_buf_rel(adapter);
 
+	kfree(hw->lan_regs);
+	hw->lan_regs = NULL;
+
 	kfree(adapter->vports);
 	adapter->vports = NULL;
 
-- 
2.37.3

[PATCH iwl-net v2 5/5] idpf: fix error handling in the init_task on load

[Emil Tantilov]

If the init_task fails during a driver load, we end up without vports and
netdevs, effectively failing the entire process. In that state a
subsequent reset will result in a crash as the service task attempts to
access uninitialized resources. Following trace is from an error in the
init_task where the CREATE_VPORT (op 501) is rejected by the FW:

[40922.763136] idpf 0000:83:00.0: Device HW Reset initiated
[40924.449797] idpf 0000:83:00.0: Transaction failed (op 501)
[40958.148190] idpf 0000:83:00.0: HW reset detected
[40958.161202] BUG: kernel NULL pointer dereference, address: 00000000000000a8
...
[40958.168094] Workqueue: idpf-0000:83:00.0-vc_event idpf_vc_event_task [idpf]
[40958.168865] RIP: 0010:idpf_vc_event_task+0x9b/0x350 [idpf]
...
[40958.177932] Call Trace:
[40958.178491]  <TASK>
[40958.179040]  process_one_work+0x226/0x6d0
[40958.179609]  worker_thread+0x19e/0x340
[40958.180158]  ? __pfx_worker_thread+0x10/0x10
[40958.180702]  kthread+0x10f/0x250
[40958.181238]  ? __pfx_kthread+0x10/0x10
[40958.181774]  ret_from_fork+0x251/0x2b0
[40958.182307]  ? __pfx_kthread+0x10/0x10
[40958.182834]  ret_from_fork_asm+0x1a/0x30
[40958.183370]  </TASK>

Fix the error handling in the init_task to make sure the service and
mailbox tasks are disabled if the error happens during load. These are
started in idpf_vc_core_init(), which spawns the init_task and has no way
of knowing if it failed. If the error happens on reset, following
successful driver load, the tasks can still run, as that will allow the
netdevs to attempt recovery through another reset. Stop the PTP callbacks
either way as those will be restarted by the call to idpf_vc_core_init()
during a successful reset.

Fixes: 0fe45467a104 ("idpf: add create vport and netdev configuration")
Reported-by: Vivek Kumar <iamvivekkumar@google.com>
Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
---
 drivers/net/ethernet/intel/idpf/idpf_lib.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
index 5193968c9bb1..89f3b46378c4 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
@@ -1716,10 +1716,9 @@ void idpf_init_task(struct work_struct *work)
 		set_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags);
 	}
 
-	/* As all the required vports are created, clear the reset flag
-	 * unconditionally here in case we were in reset and the link was down.
-	 */
+	/* Clear the reset and load bits as all vports are created */
 	clear_bit(IDPF_HR_RESET_IN_PROG, adapter->flags);
+	clear_bit(IDPF_HR_DRV_LOAD, adapter->flags);
 	/* Start the statistics task now */
 	queue_delayed_work(adapter->stats_wq, &adapter->stats_task,
 			   msecs_to_jiffies(10 * (pdev->devfn & 0x07)));
@@ -1733,6 +1732,15 @@ void idpf_init_task(struct work_struct *work)
 				idpf_vport_dealloc(adapter->vports[index]);
 		}
 	}
+	/* Cleanup after vc_core_init, which has no way of knowing the
+	 * init task failed on driver load.
+	 */
+	if (test_and_clear_bit(IDPF_HR_DRV_LOAD, adapter->flags)) {
+		cancel_delayed_work_sync(&adapter->serv_task);
+		cancel_delayed_work_sync(&adapter->mbx_task);
+	}
+	idpf_ptp_release(adapter);
+
 	clear_bit(IDPF_HR_RESET_IN_PROG, adapter->flags);
 }
 
@@ -1882,7 +1890,7 @@ static void idpf_init_hard_reset(struct idpf_adapter *adapter)
 	dev_info(dev, "Device HW Reset initiated\n");
 
 	/* Prepare for reset */
-	if (test_and_clear_bit(IDPF_HR_DRV_LOAD, adapter->flags)) {
+	if (test_bit(IDPF_HR_DRV_LOAD, adapter->flags)) {
 		reg_ops->trigger_reset(adapter, IDPF_HR_DRV_LOAD);
 	} else if (test_and_clear_bit(IDPF_HR_FUNC_RESET, adapter->flags)) {
 		bool is_reset = idpf_is_reset_detected(adapter);
-- 
2.37.3

Re: [PATCH iwl-net v2 2/5] idpf: detach and close netdevs while handling a reset

[Simon Horman]

On Thu, Nov 20, 2025 at 04:12:15PM -0800, Emil Tantilov wrote:

...

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> index 2a53f3d504d2..5c81f52db266 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> @@ -752,6 +752,65 @@ static int idpf_init_mac_addr(struct idpf_vport *vport,
>  	return 0;
>  }
>  
> +static void idpf_detach_and_close(struct idpf_adapter *adapter)
> +{
> +	int max_vports = adapter->max_vports;
> +
> +	for (int i = 0; i < max_vports; i++) {
> +		struct net_device *netdev = adapter->netdevs[i];
> +
> +		/* If the interface is in detached state, that means the
> +		 * previous reset was not handled successfully for this
> +		 * vport.
> +		 */
> +		if (!netif_device_present(netdev))
> +			continue;
> +
> +		/* Hold RTNL to protect racing with callbacks */
> +		rtnl_lock();
> +		netif_device_detach(netdev);
> +		if (netif_running(netdev)) {
> +			set_bit(IDPF_VPORT_UP_REQUESTED,
> +				adapter->vport_config[i]->flags);
> +			dev_close(netdev);
> +		}
> +		rtnl_unlock();
> +	}
> +}
> +
> +static void idpf_attach_and_open(struct idpf_adapter *adapter)
> +{
> +	int max_vports = adapter->max_vports;
> +
> +	for (int i = 0; i < max_vports; i++) {
> +		struct idpf_vport *vport = adapter->vports[i];
> +		struct idpf_vport_config *vport_config;
> +		struct net_device *netdev;
> +
> +		/* In case of a critical error in the init task, the vport
> +		 * will be freed. Only continue to restore the netdevs
> +		 * if the vport is allocated.
> +		 */
> +		if (!vport)
> +			continue;
> +
> +		/* No need for RTNL on attach as this function is called
> +		 * following detach and dev_close(). We do take RTNL for
> +		 * dev_open() below as it can race with external callbacks
> +		 * following the call to netif_device_attach().
> +		 */
> +		netdev = adapter->netdevs[i];
> +		netif_device_attach(netdev);
> +		vport_config = adapter->vport_config[vport->idx];
> +		if (test_and_clear_bit(IDPF_VPORT_UP_REQUESTED,
> +				       vport_config->flags)) {
> +			rtnl_lock();
> +			dev_open(netdev, NULL);
> +			rtnl_unlock();
> +		}
> +	}
> +}
> +
>  /**
>   * idpf_cfg_netdev - Allocate, configure and register a netdev
>   * @vport: main vport structure

...

> @@ -1807,27 +1860,6 @@ static int idpf_check_reset_complete(struct idpf_hw *hw,
>  	return -EBUSY;
>  }
>  
> -/**
> - * idpf_set_vport_state - Set the vport state to be after the reset
> - * @adapter: Driver specific private structure
> - */
> -static void idpf_set_vport_state(struct idpf_adapter *adapter)
> -{
> -	u16 i;
> -
> -	for (i = 0; i < adapter->max_vports; i++) {
> -		struct idpf_netdev_priv *np;
> -
> -		if (!adapter->netdevs[i])
> -			continue;
> -
> -		np = netdev_priv(adapter->netdevs[i]);
> -		if (np->state == __IDPF_VPORT_UP)
> -			set_bit(IDPF_VPORT_UP_REQUESTED,
> -				adapter->vport_config[i]->flags);
> -	}
> -}
> -
>  /**
>   * idpf_init_hard_reset - Initiate a hardware reset
>   * @adapter: Driver specific private structure

> @@ -1836,28 +1868,17 @@ static void idpf_set_vport_state(struct idpf_adapter *adapter)
>   * reallocate. Also reinitialize the mailbox. Return 0 on success,
>   * negative on failure.
>   */
> -static int idpf_init_hard_reset(struct idpf_adapter *adapter)
> +static void idpf_init_hard_reset(struct idpf_adapter *adapter)
>  {
>  	struct idpf_reg_ops *reg_ops = &adapter->dev_ops.reg_ops;
>  	struct device *dev = &adapter->pdev->dev;
> -	struct net_device *netdev;
>  	int err;
> -	u16 i;
>  
> +	idpf_detach_and_close(adapter);
>  	mutex_lock(&adapter->vport_ctrl_lock);
>  
>  	dev_info(dev, "Device HW Reset initiated\n");
>  
> -	/* Avoid TX hangs on reset */
> -	for (i = 0; i < adapter->max_vports; i++) {
> -		netdev = adapter->netdevs[i];
> -		if (!netdev)
> -			continue;

Hi Emil,

In this code that is removed there is a check for !netdev.
And also there is a similar check in idpf_set_vport_state().
But there is no such check in idpf_detach_and_close().
Is this intentional?

Likewise, is such a check needed in idpf_attach_and_open()?

My understanding is that netdevs[i] is populated asynchronously
via idpf_init_task. But I'm unsure if that is known to have
completed or not.

Flagged by Claude Code with https://github.com/masoncl/review-prompts/


> -
> -		netif_carrier_off(netdev);
> -		netif_tx_disable(netdev);
> -	}
> -
>  	/* Prepare for reset */
>  	if (test_and_clear_bit(IDPF_HR_DRV_LOAD, adapter->flags)) {
>  		reg_ops->trigger_reset(adapter, IDPF_HR_DRV_LOAD);
> @@ -1866,7 +1887,6 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
>  
>  		idpf_idc_issue_reset_event(adapter->cdev_info);
>  
> -		idpf_set_vport_state(adapter);
>  		idpf_vc_core_deinit(adapter);
>  		if (!is_reset)
>  			reg_ops->trigger_reset(adapter, IDPF_HR_FUNC_RESET);
> @@ -1913,11 +1933,14 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
>  unlock_mutex:
>  	mutex_unlock(&adapter->vport_ctrl_lock);
>  
> -	/* Wait until all vports are created to init RDMA CORE AUX */
> -	if (!err)
> -		err = idpf_idc_init(adapter);
> -
> -	return err;
> +	/* Attempt to restore netdevs and initialize RDMA CORE AUX device,
> +	 * provided vc_core_init succeeded. It is still possible that
> +	 * vports are not allocated at this point if the init task failed.
> +	 */
> +	if (!err) {
> +		idpf_attach_and_open(adapter);
> +		idpf_idc_init(adapter);
> +	}
>  }
>  
>  /**
> -- 
> 2.37.3
> 
> 

Re: [PATCH iwl-net v2 2/5] idpf: detach and close netdevs while handling a reset

[Tantilov, Emil S]

On 11/25/2025 5:42 AM, Simon Horman wrote:
> On Thu, Nov 20, 2025 at 04:12:15PM -0800, Emil Tantilov wrote:
> 
> ...
> 
>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
>> index 2a53f3d504d2..5c81f52db266 100644
>> --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
>> +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
>> @@ -752,6 +752,65 @@ static int idpf_init_mac_addr(struct idpf_vport *vport,
>>   	return 0;
>>   }
>>   
>> +static void idpf_detach_and_close(struct idpf_adapter *adapter)
>> +{
>> +	int max_vports = adapter->max_vports;
>> +
>> +	for (int i = 0; i < max_vports; i++) {
>> +		struct net_device *netdev = adapter->netdevs[i];
>> +
>> +		/* If the interface is in detached state, that means the
>> +		 * previous reset was not handled successfully for this
>> +		 * vport.
>> +		 */
>> +		if (!netif_device_present(netdev))
>> +			continue;
>> +
>> +		/* Hold RTNL to protect racing with callbacks */
>> +		rtnl_lock();
>> +		netif_device_detach(netdev);
>> +		if (netif_running(netdev)) {
>> +			set_bit(IDPF_VPORT_UP_REQUESTED,
>> +				adapter->vport_config[i]->flags);
>> +			dev_close(netdev);
>> +		}
>> +		rtnl_unlock();
>> +	}
>> +}
>> +
>> +static void idpf_attach_and_open(struct idpf_adapter *adapter)
>> +{
>> +	int max_vports = adapter->max_vports;
>> +
>> +	for (int i = 0; i < max_vports; i++) {
>> +		struct idpf_vport *vport = adapter->vports[i];
>> +		struct idpf_vport_config *vport_config;
>> +		struct net_device *netdev;
>> +
>> +		/* In case of a critical error in the init task, the vport
>> +		 * will be freed. Only continue to restore the netdevs
>> +		 * if the vport is allocated.
>> +		 */
>> +		if (!vport)
>> +			continue;
>> +
>> +		/* No need for RTNL on attach as this function is called
>> +		 * following detach and dev_close(). We do take RTNL for
>> +		 * dev_open() below as it can race with external callbacks
>> +		 * following the call to netif_device_attach().
>> +		 */
>> +		netdev = adapter->netdevs[i];
>> +		netif_device_attach(netdev);
>> +		vport_config = adapter->vport_config[vport->idx];
>> +		if (test_and_clear_bit(IDPF_VPORT_UP_REQUESTED,
>> +				       vport_config->flags)) {
>> +			rtnl_lock();
>> +			dev_open(netdev, NULL);
>> +			rtnl_unlock();
>> +		}
>> +	}
>> +}
>> +
>>   /**
>>    * idpf_cfg_netdev - Allocate, configure and register a netdev
>>    * @vport: main vport structure
> 
> ...
> 
>> @@ -1807,27 +1860,6 @@ static int idpf_check_reset_complete(struct idpf_hw *hw,
>>   	return -EBUSY;
>>   }
>>   
>> -/**
>> - * idpf_set_vport_state - Set the vport state to be after the reset
>> - * @adapter: Driver specific private structure
>> - */
>> -static void idpf_set_vport_state(struct idpf_adapter *adapter)
>> -{
>> -	u16 i;
>> -
>> -	for (i = 0; i < adapter->max_vports; i++) {
>> -		struct idpf_netdev_priv *np;
>> -
>> -		if (!adapter->netdevs[i])
>> -			continue;
>> -
>> -		np = netdev_priv(adapter->netdevs[i]);
>> -		if (np->state == __IDPF_VPORT_UP)
>> -			set_bit(IDPF_VPORT_UP_REQUESTED,
>> -				adapter->vport_config[i]->flags);
>> -	}
>> -}
>> -
>>   /**
>>    * idpf_init_hard_reset - Initiate a hardware reset
>>    * @adapter: Driver specific private structure
> 
>> @@ -1836,28 +1868,17 @@ static void idpf_set_vport_state(struct idpf_adapter *adapter)
>>    * reallocate. Also reinitialize the mailbox. Return 0 on success,
>>    * negative on failure.
>>    */
>> -static int idpf_init_hard_reset(struct idpf_adapter *adapter)
>> +static void idpf_init_hard_reset(struct idpf_adapter *adapter)
>>   {
>>   	struct idpf_reg_ops *reg_ops = &adapter->dev_ops.reg_ops;
>>   	struct device *dev = &adapter->pdev->dev;
>> -	struct net_device *netdev;
>>   	int err;
>> -	u16 i;
>>   
>> +	idpf_detach_and_close(adapter);
>>   	mutex_lock(&adapter->vport_ctrl_lock);
>>   
>>   	dev_info(dev, "Device HW Reset initiated\n");
>>   
>> -	/* Avoid TX hangs on reset */
>> -	for (i = 0; i < adapter->max_vports; i++) {
>> -		netdev = adapter->netdevs[i];
>> -		if (!netdev)
>> -			continue;
> 
> Hi Emil,
> 
> In this code that is removed there is a check for !netdev.
> And also there is a similar check in idpf_set_vport_state().
> But there is no such check in idpf_detach_and_close().
> Is this intentional?

This logic is a bit confusing because the reset path is executed on both
driver load and a reset (since the initialization is identical it makes
sense to re-use the code). This is what roughly happens on load and
reset:

driver load -> reset -> configure vports -> create netdevs
reset -> de-allocate vports -> re-allocate vports

The first patch:
https://lore.kernel.org/intel-wired-lan/20251121001218.4565-2-emil.s.tantilov@intel.com/

makes sure that we never lose the netdev on a reset, following a
successful driver load. Previously this could happen in the error path.
In other words during a reset there is no need to check for a netdev as
this is guaranteed, but we must make sure that vports are present as
those can be freed.

The 5th patch:
https://lore.kernel.org/intel-wired-lan/20251121001218.4565-6-emil.s.tantilov@intel.com/

fixes another instance where we could fail in the reset error path by
ensuring the service task, which handles resets is cancelled as at
that point we have neither vports, nor netdevs, hence nothing to
"serve". Hope this makes sense, but the gist of it is that with this
series applied the reset can be protected by just making sure that
the vports are allocated. If for whatever reason netdevs happen to
be NULL, following this series it would be a bug introduced somewhere
else in the code that will have to be addressed.

Thanks,
Emil

> 
> Likewise, is such a check needed in idpf_attach_and_open()?
> 
> My understanding is that netdevs[i] is populated asynchronously
> via idpf_init_task. But I'm unsure if that is known to have
> completed or not.
> 
> Flagged by Claude Code with https://github.com/masoncl/review-prompts/
> 
> 
>> -
>> -		netif_carrier_off(netdev);
>> -		netif_tx_disable(netdev);
>> -	}
>> -
>>   	/* Prepare for reset */
>>   	if (test_and_clear_bit(IDPF_HR_DRV_LOAD, adapter->flags)) {
>>   		reg_ops->trigger_reset(adapter, IDPF_HR_DRV_LOAD);
>> @@ -1866,7 +1887,6 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
>>   
>>   		idpf_idc_issue_reset_event(adapter->cdev_info);
>>   
>> -		idpf_set_vport_state(adapter);
>>   		idpf_vc_core_deinit(adapter);
>>   		if (!is_reset)
>>   			reg_ops->trigger_reset(adapter, IDPF_HR_FUNC_RESET);
>> @@ -1913,11 +1933,14 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
>>   unlock_mutex:
>>   	mutex_unlock(&adapter->vport_ctrl_lock);
>>   
>> -	/* Wait until all vports are created to init RDMA CORE AUX */
>> -	if (!err)
>> -		err = idpf_idc_init(adapter);
>> -
>> -	return err;
>> +	/* Attempt to restore netdevs and initialize RDMA CORE AUX device,
>> +	 * provided vc_core_init succeeded. It is still possible that
>> +	 * vports are not allocated at this point if the init task failed.
>> +	 */
>> +	if (!err) {
>> +		idpf_attach_and_open(adapter);
>> +		idpf_idc_init(adapter);
>> +	}
>>   }
>>   
>>   /**
>> -- 
>> 2.37.3
>>
>>
> 

Re: [PATCH iwl-net v2 2/5] idpf: detach and close netdevs while handling a reset

[Simon Horman]

On Tue, Nov 25, 2025 at 06:58:37AM -0800, Tantilov, Emil S wrote:
> 
> 
> On 11/25/2025 5:42 AM, Simon Horman wrote:
> > On Thu, Nov 20, 2025 at 04:12:15PM -0800, Emil Tantilov wrote:
> > 
> > ...
> > 
> > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > > index 2a53f3d504d2..5c81f52db266 100644
> > > --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > > +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > > @@ -752,6 +752,65 @@ static int idpf_init_mac_addr(struct idpf_vport *vport,
> > >   	return 0;
> > >   }
> > > +static void idpf_detach_and_close(struct idpf_adapter *adapter)
> > > +{
> > > +	int max_vports = adapter->max_vports;
> > > +
> > > +	for (int i = 0; i < max_vports; i++) {
> > > +		struct net_device *netdev = adapter->netdevs[i];
> > > +
> > > +		/* If the interface is in detached state, that means the
> > > +		 * previous reset was not handled successfully for this
> > > +		 * vport.
> > > +		 */
> > > +		if (!netif_device_present(netdev))
> > > +			continue;
> > > +
> > > +		/* Hold RTNL to protect racing with callbacks */
> > > +		rtnl_lock();
> > > +		netif_device_detach(netdev);
> > > +		if (netif_running(netdev)) {
> > > +			set_bit(IDPF_VPORT_UP_REQUESTED,
> > > +				adapter->vport_config[i]->flags);
> > > +			dev_close(netdev);
> > > +		}
> > > +		rtnl_unlock();
> > > +	}
> > > +}
> > > +
> > > +static void idpf_attach_and_open(struct idpf_adapter *adapter)
> > > +{
> > > +	int max_vports = adapter->max_vports;
> > > +
> > > +	for (int i = 0; i < max_vports; i++) {
> > > +		struct idpf_vport *vport = adapter->vports[i];
> > > +		struct idpf_vport_config *vport_config;
> > > +		struct net_device *netdev;
> > > +
> > > +		/* In case of a critical error in the init task, the vport
> > > +		 * will be freed. Only continue to restore the netdevs
> > > +		 * if the vport is allocated.
> > > +		 */
> > > +		if (!vport)
> > > +			continue;
> > > +
> > > +		/* No need for RTNL on attach as this function is called
> > > +		 * following detach and dev_close(). We do take RTNL for
> > > +		 * dev_open() below as it can race with external callbacks
> > > +		 * following the call to netif_device_attach().
> > > +		 */
> > > +		netdev = adapter->netdevs[i];
> > > +		netif_device_attach(netdev);
> > > +		vport_config = adapter->vport_config[vport->idx];
> > > +		if (test_and_clear_bit(IDPF_VPORT_UP_REQUESTED,
> > > +				       vport_config->flags)) {
> > > +			rtnl_lock();
> > > +			dev_open(netdev, NULL);
> > > +			rtnl_unlock();
> > > +		}
> > > +	}
> > > +}
> > > +
> > >   /**
> > >    * idpf_cfg_netdev - Allocate, configure and register a netdev
> > >    * @vport: main vport structure
> > 
> > ...
> > 
> > > @@ -1807,27 +1860,6 @@ static int idpf_check_reset_complete(struct idpf_hw *hw,
> > >   	return -EBUSY;
> > >   }
> > > -/**
> > > - * idpf_set_vport_state - Set the vport state to be after the reset
> > > - * @adapter: Driver specific private structure
> > > - */
> > > -static void idpf_set_vport_state(struct idpf_adapter *adapter)
> > > -{
> > > -	u16 i;
> > > -
> > > -	for (i = 0; i < adapter->max_vports; i++) {
> > > -		struct idpf_netdev_priv *np;
> > > -
> > > -		if (!adapter->netdevs[i])
> > > -			continue;
> > > -
> > > -		np = netdev_priv(adapter->netdevs[i]);
> > > -		if (np->state == __IDPF_VPORT_UP)
> > > -			set_bit(IDPF_VPORT_UP_REQUESTED,
> > > -				adapter->vport_config[i]->flags);
> > > -	}
> > > -}
> > > -
> > >   /**
> > >    * idpf_init_hard_reset - Initiate a hardware reset
> > >    * @adapter: Driver specific private structure
> > 
> > > @@ -1836,28 +1868,17 @@ static void idpf_set_vport_state(struct idpf_adapter *adapter)
> > >    * reallocate. Also reinitialize the mailbox. Return 0 on success,
> > >    * negative on failure.
> > >    */
> > > -static int idpf_init_hard_reset(struct idpf_adapter *adapter)
> > > +static void idpf_init_hard_reset(struct idpf_adapter *adapter)
> > >   {
> > >   	struct idpf_reg_ops *reg_ops = &adapter->dev_ops.reg_ops;
> > >   	struct device *dev = &adapter->pdev->dev;
> > > -	struct net_device *netdev;
> > >   	int err;
> > > -	u16 i;
> > > +	idpf_detach_and_close(adapter);
> > >   	mutex_lock(&adapter->vport_ctrl_lock);
> > >   	dev_info(dev, "Device HW Reset initiated\n");
> > > -	/* Avoid TX hangs on reset */
> > > -	for (i = 0; i < adapter->max_vports; i++) {
> > > -		netdev = adapter->netdevs[i];
> > > -		if (!netdev)
> > > -			continue;
> > 
> > Hi Emil,
> > 
> > In this code that is removed there is a check for !netdev.
> > And also there is a similar check in idpf_set_vport_state().
> > But there is no such check in idpf_detach_and_close().
> > Is this intentional?
> 
> This logic is a bit confusing because the reset path is executed on both
> driver load and a reset (since the initialization is identical it makes
> sense to re-use the code). This is what roughly happens on load and
> reset:
> 
> driver load -> reset -> configure vports -> create netdevs
> reset -> de-allocate vports -> re-allocate vports
> 
> The first patch:
> https://lore.kernel.org/intel-wired-lan/20251121001218.4565-2-emil.s.tantilov@intel.com/
> 
> makes sure that we never lose the netdev on a reset, following a
> successful driver load. Previously this could happen in the error path.
> In other words during a reset there is no need to check for a netdev as
> this is guaranteed, but we must make sure that vports are present as
> those can be freed.
> 
> The 5th patch:
> https://lore.kernel.org/intel-wired-lan/20251121001218.4565-6-emil.s.tantilov@intel.com/
> 
> fixes another instance where we could fail in the reset error path by
> ensuring the service task, which handles resets is cancelled as at
> that point we have neither vports, nor netdevs, hence nothing to
> "serve". Hope this makes sense, but the gist of it is that with this
> series applied the reset can be protected by just making sure that
> the vports are allocated. If for whatever reason netdevs happen to
> be NULL, following this series it would be a bug introduced somewhere
> else in the code that will have to be addressed.

I did spend a bit of time trying to figure out the flow,
but not entirely successfully. Thanks for setting me straight.

...

Re: [PATCH iwl-net v2 2/5] idpf: detach and close netdevs while handling a reset

[Chittim, Madhu]

On 11/20/2025 4:12 PM, Tantilov, Emil S wrote:
> Protect the reset path from callbacks by setting the netdevs to detached
> state and close any netdevs in UP state until the reset handling has
> completed. During a reset, the driver will de-allocate resources for the
> vport, and there is no guarantee that those will recover, which is why the
> existing vport_ctrl_lock does not provide sufficient protection.
> 
> idpf_detach_and_close() is called right before reset handling. If the
> reset handling succeeds, the netdevs state is recovered via call to
> idpf_attach_and_open(). If the reset handling fails the netdevs remain
> down. The detach/down calls are protected with RTNL lock to avoid racing
> with callbacks. On the recovery side the attach can be done without
> holding the RTNL lock as there are no callbacks expected at that point,
> due to detach/close always being done first in that flow.
> 
> The previous logic restoring the netdevs state based on the
> IDPF_VPORT_UP_REQUESTED flag in the init task is not needed anymore, hence
> the removal of idpf_set_vport_state(). The IDPF_VPORT_UP_REQUESTED is
> still being used to restore the state of the netdevs following the reset,
> but has no use outside of the reset handling flow.
> 
> idpf_init_hard_reset() is converted to void, since it was used as such and
> there is no error handling being done based on its return value.
> 
> Before this change, invoking hard and soft resets simultaneously will
> cause the driver to lose the vport state:
> ip -br a
> <inf>	UP
> echo 1 > /sys/class/net/ens801f0/device/reset& \
> ethtool -L ens801f0 combined 8
> ip -br a
> <inf>	DOWN
> ip link set <inf> up
> ip -br a
> <inf>	DOWN
> 
> Also in case of a failure in the reset path, the netdev is left
> exposed to external callbacks, while vport resources are not
> initialized, leading to a crash on subsequent ifup/down:
> [408471.398966] idpf 0000:83:00.0: HW reset detected
> [408471.411744] idpf 0000:83:00.0: Device HW Reset initiated
> [408472.277901] idpf 0000:83:00.0: The driver was unable to contact the device's firmware. Check that the FW is running. Driver state= 0x2
> [408508.125551] BUG: kernel NULL pointer dereference, address: 0000000000000078
> [408508.126112] #PF: supervisor read access in kernel mode
> [408508.126687] #PF: error_code(0x0000) - not-present page
> [408508.127256] PGD 2aae2f067 P4D 0
> [408508.127824] Oops: Oops: 0000 [#1] SMP NOPTI
> ...
> [408508.130871] RIP: 0010:idpf_stop+0x39/0x70 [idpf]
> ...
> [408508.139193] Call Trace:
> [408508.139637]  <TASK>
> [408508.140077]  __dev_close_many+0xbb/0x260
> [408508.140533]  __dev_change_flags+0x1cf/0x280
> [408508.140987]  netif_change_flags+0x26/0x70
> [408508.141434]  dev_change_flags+0x3d/0xb0
> [408508.141878]  devinet_ioctl+0x460/0x890
> [408508.142321]  inet_ioctl+0x18e/0x1d0
> [408508.142762]  ? _copy_to_user+0x22/0x70
> [408508.143207]  sock_do_ioctl+0x3d/0xe0
> [408508.143652]  sock_ioctl+0x10e/0x330
> [408508.144091]  ? find_held_lock+0x2b/0x80
> [408508.144537]  __x64_sys_ioctl+0x96/0xe0
> [408508.144979]  do_syscall_64+0x79/0x3d0
> [408508.145415]  entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [408508.145860] RIP: 0033:0x7f3e0bb4caff
> 
> Fixes: 0fe45467a104 ("idpf: add create vport and netdev configuration")
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>

Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>

> ---
>   drivers/net/ethernet/intel/idpf/idpf_lib.c | 121 ++++++++++++---------
>   1 file changed, 72 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> index 2a53f3d504d2..5c81f52db266 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> @@ -752,6 +752,65 @@ static int idpf_init_mac_addr(struct idpf_vport *vport,
>   	return 0;
>   }
>   
> +static void idpf_detach_and_close(struct idpf_adapter *adapter)
> +{
> +	int max_vports = adapter->max_vports;
> +
> +	for (int i = 0; i < max_vports; i++) {
> +		struct net_device *netdev = adapter->netdevs[i];
> +
> +		/* If the interface is in detached state, that means the
> +		 * previous reset was not handled successfully for this
> +		 * vport.
> +		 */
> +		if (!netif_device_present(netdev))
> +			continue;
> +
> +		/* Hold RTNL to protect racing with callbacks */
> +		rtnl_lock();
> +		netif_device_detach(netdev);
> +		if (netif_running(netdev)) {
> +			set_bit(IDPF_VPORT_UP_REQUESTED,
> +				adapter->vport_config[i]->flags);
> +			dev_close(netdev);
> +		}
> +		rtnl_unlock();
> +	}
> +}
> +
> +static void idpf_attach_and_open(struct idpf_adapter *adapter)
> +{
> +	int max_vports = adapter->max_vports;
> +
> +	for (int i = 0; i < max_vports; i++) {
> +		struct idpf_vport *vport = adapter->vports[i];
> +		struct idpf_vport_config *vport_config;
> +		struct net_device *netdev;
> +
> +		/* In case of a critical error in the init task, the vport
> +		 * will be freed. Only continue to restore the netdevs
> +		 * if the vport is allocated.
> +		 */
> +		if (!vport)
> +			continue;
> +
> +		/* No need for RTNL on attach as this function is called
> +		 * following detach and dev_close(). We do take RTNL for
> +		 * dev_open() below as it can race with external callbacks
> +		 * following the call to netif_device_attach().
> +		 */
> +		netdev = adapter->netdevs[i];
> +		netif_device_attach(netdev);
> +		vport_config = adapter->vport_config[vport->idx];
> +		if (test_and_clear_bit(IDPF_VPORT_UP_REQUESTED,
> +				       vport_config->flags)) {
> +			rtnl_lock();
> +			dev_open(netdev, NULL);
> +			rtnl_unlock();
> +		}
> +	}
> +}
> +
>   /**
>    * idpf_cfg_netdev - Allocate, configure and register a netdev
>    * @vport: main vport structure
> @@ -1064,10 +1123,11 @@ static void idpf_vport_dealloc(struct idpf_vport *vport)
>   	idpf_idc_deinit_vport_aux_device(vport->vdev_info);
>   
>   	idpf_deinit_mac_addr(vport);
> -	idpf_vport_stop(vport, true);
>   
> -	if (!test_bit(IDPF_HR_RESET_IN_PROG, adapter->flags))
> +	if (!test_bit(IDPF_HR_RESET_IN_PROG, adapter->flags)) {
> +		idpf_vport_stop(vport, true);
>   		idpf_decfg_netdev(vport);
> +	}
>   	if (test_bit(IDPF_REMOVE_IN_PROG, adapter->flags)) {
>   		idpf_del_all_mac_filters(vport);
>   		idpf_del_all_flow_steer_filters(vport);
> @@ -1569,7 +1629,6 @@ void idpf_init_task(struct work_struct *work)
>   	struct idpf_vport_config *vport_config;
>   	struct idpf_vport_max_q max_q;
>   	struct idpf_adapter *adapter;
> -	struct idpf_netdev_priv *np;
>   	struct idpf_vport *vport;
>   	u16 num_default_vports;
>   	struct pci_dev *pdev;
> @@ -1626,12 +1685,6 @@ void idpf_init_task(struct work_struct *work)
>   	if (idpf_cfg_netdev(vport))
>   		goto unwind_vports;
>   
> -	/* Once state is put into DOWN, driver is ready for dev_open */
> -	np = netdev_priv(vport->netdev);
> -	np->state = __IDPF_VPORT_DOWN;
> -	if (test_and_clear_bit(IDPF_VPORT_UP_REQUESTED, vport_config->flags))
> -		idpf_vport_open(vport, true);
> -
>   	/* Spawn and return 'idpf_init_task' work queue until all the
>   	 * default vports are created
>   	 */
> @@ -1807,27 +1860,6 @@ static int idpf_check_reset_complete(struct idpf_hw *hw,
>   	return -EBUSY;
>   }
>   
> -/**
> - * idpf_set_vport_state - Set the vport state to be after the reset
> - * @adapter: Driver specific private structure
> - */
> -static void idpf_set_vport_state(struct idpf_adapter *adapter)
> -{
> -	u16 i;
> -
> -	for (i = 0; i < adapter->max_vports; i++) {
> -		struct idpf_netdev_priv *np;
> -
> -		if (!adapter->netdevs[i])
> -			continue;
> -
> -		np = netdev_priv(adapter->netdevs[i]);
> -		if (np->state == __IDPF_VPORT_UP)
> -			set_bit(IDPF_VPORT_UP_REQUESTED,
> -				adapter->vport_config[i]->flags);
> -	}
> -}
> -
>   /**
>    * idpf_init_hard_reset - Initiate a hardware reset
>    * @adapter: Driver specific private structure
> @@ -1836,28 +1868,17 @@ static void idpf_set_vport_state(struct idpf_adapter *adapter)
>    * reallocate. Also reinitialize the mailbox. Return 0 on success,
>    * negative on failure.
>    */
> -static int idpf_init_hard_reset(struct idpf_adapter *adapter)
> +static void idpf_init_hard_reset(struct idpf_adapter *adapter)
>   {
>   	struct idpf_reg_ops *reg_ops = &adapter->dev_ops.reg_ops;
>   	struct device *dev = &adapter->pdev->dev;
> -	struct net_device *netdev;
>   	int err;
> -	u16 i;
>   
> +	idpf_detach_and_close(adapter);
>   	mutex_lock(&adapter->vport_ctrl_lock);
>   
>   	dev_info(dev, "Device HW Reset initiated\n");
>   
> -	/* Avoid TX hangs on reset */
> -	for (i = 0; i < adapter->max_vports; i++) {
> -		netdev = adapter->netdevs[i];
> -		if (!netdev)
> -			continue;
> -
> -		netif_carrier_off(netdev);
> -		netif_tx_disable(netdev);
> -	}
> -
>   	/* Prepare for reset */
>   	if (test_and_clear_bit(IDPF_HR_DRV_LOAD, adapter->flags)) {
>   		reg_ops->trigger_reset(adapter, IDPF_HR_DRV_LOAD);
> @@ -1866,7 +1887,6 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
>   
>   		idpf_idc_issue_reset_event(adapter->cdev_info);
>   
> -		idpf_set_vport_state(adapter);
>   		idpf_vc_core_deinit(adapter);
>   		if (!is_reset)
>   			reg_ops->trigger_reset(adapter, IDPF_HR_FUNC_RESET);
> @@ -1913,11 +1933,14 @@ static int idpf_init_hard_reset(struct idpf_adapter *adapter)
>   unlock_mutex:
>   	mutex_unlock(&adapter->vport_ctrl_lock);
>   
> -	/* Wait until all vports are created to init RDMA CORE AUX */
> -	if (!err)
> -		err = idpf_idc_init(adapter);
> -
> -	return err;
> +	/* Attempt to restore netdevs and initialize RDMA CORE AUX device,
> +	 * provided vc_core_init succeeded. It is still possible that
> +	 * vports are not allocated at this point if the init task failed.
> +	 */
> +	if (!err) {
> +		idpf_attach_and_open(adapter);
> +		idpf_idc_init(adapter);
> +	}
>   }
>   
>   /**

Re: [PATCH iwl-net v2 5/5] idpf: fix error handling in the init_task on load

[Chittim, Madhu]

On 11/20/2025 4:12 PM, Tantilov, Emil S wrote:
> If the init_task fails during a driver load, we end up without vports and
> netdevs, effectively failing the entire process. In that state a
> subsequent reset will result in a crash as the service task attempts to
> access uninitialized resources. Following trace is from an error in the
> init_task where the CREATE_VPORT (op 501) is rejected by the FW:
> 
> [40922.763136] idpf 0000:83:00.0: Device HW Reset initiated
> [40924.449797] idpf 0000:83:00.0: Transaction failed (op 501)
> [40958.148190] idpf 0000:83:00.0: HW reset detected
> [40958.161202] BUG: kernel NULL pointer dereference, address: 00000000000000a8
> ...
> [40958.168094] Workqueue: idpf-0000:83:00.0-vc_event idpf_vc_event_task [idpf]
> [40958.168865] RIP: 0010:idpf_vc_event_task+0x9b/0x350 [idpf]
> ...
> [40958.177932] Call Trace:
> [40958.178491]  <TASK>
> [40958.179040]  process_one_work+0x226/0x6d0
> [40958.179609]  worker_thread+0x19e/0x340
> [40958.180158]  ? __pfx_worker_thread+0x10/0x10
> [40958.180702]  kthread+0x10f/0x250
> [40958.181238]  ? __pfx_kthread+0x10/0x10
> [40958.181774]  ret_from_fork+0x251/0x2b0
> [40958.182307]  ? __pfx_kthread+0x10/0x10
> [40958.182834]  ret_from_fork_asm+0x1a/0x30
> [40958.183370]  </TASK>
> 
> Fix the error handling in the init_task to make sure the service and
> mailbox tasks are disabled if the error happens during load. These are
> started in idpf_vc_core_init(), which spawns the init_task and has no way
> of knowing if it failed. If the error happens on reset, following
> successful driver load, the tasks can still run, as that will allow the
> netdevs to attempt recovery through another reset. Stop the PTP callbacks
> either way as those will be restarted by the call to idpf_vc_core_init()
> during a successful reset.
> 
> Fixes: 0fe45467a104 ("idpf: add create vport and netdev configuration")
> Reported-by: Vivek Kumar <iamvivekkumar@google.com>
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>

Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>

> ---
>   drivers/net/ethernet/intel/idpf/idpf_lib.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> index 5193968c9bb1..89f3b46378c4 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> @@ -1716,10 +1716,9 @@ void idpf_init_task(struct work_struct *work)
>   		set_bit(IDPF_VPORT_REG_NETDEV, vport_config->flags);
>   	}
>   
> -	/* As all the required vports are created, clear the reset flag
> -	 * unconditionally here in case we were in reset and the link was down.
> -	 */
> +	/* Clear the reset and load bits as all vports are created */
>   	clear_bit(IDPF_HR_RESET_IN_PROG, adapter->flags);
> +	clear_bit(IDPF_HR_DRV_LOAD, adapter->flags);
>   	/* Start the statistics task now */
>   	queue_delayed_work(adapter->stats_wq, &adapter->stats_task,
>   			   msecs_to_jiffies(10 * (pdev->devfn & 0x07)));
> @@ -1733,6 +1732,15 @@ void idpf_init_task(struct work_struct *work)
>   				idpf_vport_dealloc(adapter->vports[index]);
>   		}
>   	}
> +	/* Cleanup after vc_core_init, which has no way of knowing the
> +	 * init task failed on driver load.
> +	 */
> +	if (test_and_clear_bit(IDPF_HR_DRV_LOAD, adapter->flags)) {
> +		cancel_delayed_work_sync(&adapter->serv_task);
> +		cancel_delayed_work_sync(&adapter->mbx_task);
> +	}
> +	idpf_ptp_release(adapter);
> +
>   	clear_bit(IDPF_HR_RESET_IN_PROG, adapter->flags);
>   }
>   
> @@ -1882,7 +1890,7 @@ static void idpf_init_hard_reset(struct idpf_adapter *adapter)
>   	dev_info(dev, "Device HW Reset initiated\n");
>   
>   	/* Prepare for reset */
> -	if (test_and_clear_bit(IDPF_HR_DRV_LOAD, adapter->flags)) {
> +	if (test_bit(IDPF_HR_DRV_LOAD, adapter->flags)) {
>   		reg_ops->trigger_reset(adapter, IDPF_HR_DRV_LOAD);
>   	} else if (test_and_clear_bit(IDPF_HR_FUNC_RESET, adapter->flags)) {
>   		bool is_reset = idpf_is_reset_detected(adapter);

RE: [Intel-wired-lan] [PATCH iwl-net v2 1/5] idpf: keep the netdev when a reset fails

[Salin, Samuel]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Emil Tantilov
> Sent: Thursday, November 20, 2025 4:12 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; decot@google.com; willemb@google.com; Hay, Joshua
> A <joshua.a.hay@intel.com>; Chittim, Madhu <madhu.chittim@intel.com>;
> Lobakin, Aleksander <aleksander.lobakin@intel.com>; Zaremba, Larysa
> <larysa.zaremba@intel.com>; iamvivekkumar@google.com
> Subject: [Intel-wired-lan] [PATCH iwl-net v2 1/5] idpf: keep the netdev when a
> reset fails
> 
> During a successful reset the driver would re-allocate vport resources while
> keeping the netdevs intact. However, in case of an error in the init task, the
> netdev of the failing vport will be unregistered, effectively removing the
> network interface:
> 
> [  121.211076] idpf 0000:83:00.0: enabling device (0100 -> 0102) [
> 121.221976] idpf 0000:83:00.0: Device HW Reset initiated [  124.161229]
> idpf 0000:83:00.0 ens801f0: renamed from eth0 [  124.163364] idpf
> 0000:83:00.0 ens801f0d1: renamed from eth1 [  125.934656] idpf
> 0000:83:00.0 ens801f0d2: renamed from eth2 [  128.218429] idpf
> 0000:83:00.0 ens801f0d3: renamed from eth3
> 
> ip -br a
> ens801f0         UP
> ens801f0d1       UP
> ens801f0d2       UP
> ens801f0d3       UP
> echo 1 > /sys/class/net/ens801f0/device/reset
> 
> [  145.885537] idpf 0000:83:00.0: resetting [  145.990280] idpf
> 0000:83:00.0: reset done [  146.284766] idpf 0000:83:00.0: HW reset
> detected [  146.296610] idpf 0000:83:00.0: Device HW Reset initiated [
> 211.556719] idpf 0000:83:00.0: Transaction timed-out (op:526 cookie:7700
> vc_op:526 salt:77 timeout:60000ms) [  272.996705] idpf 0000:83:00.0:
> Transaction timed-out (op:502 cookie:7800 vc_op:502 salt:78
> timeout:60000ms)
> 
> ip -br a
> ens801f0d1       DOWN
> ens801f0d2       DOWN
> ens801f0d3       DOWN
> 
> Re-shuffle the logic in the error path of the init task to make sure the netdevs
> remain intact. This will allow the driver to attempt recovery via subsequent
> resets, provided the FW is still functional.
> 
> The main change is to make sure that idpf_decfg_netdev() is not called should
> the init task fail during a reset. The error handling is consolidated under
> unwind_vports, as the removed labels had the same cleanup logic split
> depending on the point of failure.
> 
> Fixes: ce1b75d0635c ("idpf: add ptypes and MAC filter support")
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> ---
> 2.37.3
Tested-by: Samuel Salin <Samuel.salin@intel.com>

RE: [Intel-wired-lan] [PATCH iwl-net v2 2/5] idpf: detach and close netdevs while handling a reset

[Salin, Samuel]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Simon Horman
> Sent: Wednesday, November 26, 2025 9:11 AM
> To: Tantilov, Emil S <emil.s.tantilov@intel.com>
> Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Loktionov,
> Aleksandr <aleksandr.loktionov@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; decot@google.com; willemb@google.com; Hay, Joshua
> A <joshua.a.hay@intel.com>; Chittim, Madhu <madhu.chittim@intel.com>;
> Lobakin, Aleksander <aleksander.lobakin@intel.com>; Zaremba, Larysa
> <larysa.zaremba@intel.com>; iamvivekkumar@google.com
> Subject: Re: [Intel-wired-lan] [PATCH iwl-net v2 2/5] idpf: detach and close
> netdevs while handling a reset
> 
> On Tue, Nov 25, 2025 at 06:58:37AM -0800, Tantilov, Emil S wrote:
> >
> >
> > On 11/25/2025 5:42 AM, Simon Horman wrote:
> > > On Thu, Nov 20, 2025 at 04:12:15PM -0800, Emil Tantilov wrote:
> > >
> > > ...
> > >
> > > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > > > b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > > > index 2a53f3d504d2..5c81f52db266 100644
> > > > --- a/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > > > +++ b/drivers/net/ethernet/intel/idpf/idpf_lib.c
> > > > @@ -752,6 +752,65 @@ static int idpf_init_mac_addr(struct idpf_vport
> *vport,
> > > >   	return 0;
> > > >   }
> > > > +static void idpf_detach_and_close(struct idpf_adapter *adapter) {
> > > > +	int max_vports = adapter->max_vports;
> > > > +
> > > > +	for (int i = 0; i < max_vports; i++) {
> > > > +		struct net_device *netdev = adapter->netdevs[i];
> > > > +
> > > > +		/* If the interface is in detached state, that means the
> > > > +		 * previous reset was not handled successfully for this
> > > > +		 * vport.
> > > > +		 */
> > > > +		if (!netif_device_present(netdev))
> > > > +			continue;
> > > > +
> > > > +		/* Hold RTNL to protect racing with callbacks */
> > > > +		rtnl_lock();
> > > > +		netif_device_detach(netdev);
> > > > +		if (netif_running(netdev)) {
> > > > +			set_bit(IDPF_VPORT_UP_REQUESTED,
> > > > +				adapter->vport_config[i]->flags);
> > > > +			dev_close(netdev);
> > > > +		}
> > > > +		rtnl_unlock();
> > > > +	}
> > > > +}
> > > > +
> > > > +static void idpf_attach_and_open(struct idpf_adapter *adapter) {
> > > > +	int max_vports = adapter->max_vports;
> > > > +
> > > > +	for (int i = 0; i < max_vports; i++) {
> > > > +		struct idpf_vport *vport = adapter->vports[i];
> > > > +		struct idpf_vport_config *vport_config;
> > > > +		struct net_device *netdev;
> > > > +
> > > > +		/* In case of a critical error in the init task, the vport
> > > > +		 * will be freed. Only continue to restore the netdevs
> > > > +		 * if the vport is allocated.
> > > > +		 */
> > > > +		if (!vport)
> > > > +			continue;
> > > > +
> > > > +		/* No need for RTNL on attach as this function is called
> > > > +		 * following detach and dev_close(). We do take RTNL for
> > > > +		 * dev_open() below as it can race with external callbacks
> > > > +		 * following the call to netif_device_attach().
> > > > +		 */
> > > > +		netdev = adapter->netdevs[i];
> > > > +		netif_device_attach(netdev);
> > > > +		vport_config = adapter->vport_config[vport->idx];
> > > > +		if (test_and_clear_bit(IDPF_VPORT_UP_REQUESTED,
> > > > +				       vport_config->flags)) {
> > > > +			rtnl_lock();
> > > > +			dev_open(netdev, NULL);
> > > > +			rtnl_unlock();
> > > > +		}
> > > > +	}
> > > > +}
> > > > +
> > > >   /**
> > > >    * idpf_cfg_netdev - Allocate, configure and register a netdev
> > > >    * @vport: main vport structure
> > >
> > > ...
> > >
> > > > @@ -1807,27 +1860,6 @@ static int idpf_check_reset_complete(struct
> idpf_hw *hw,
> > > >   	return -EBUSY;
> > > >   }
> > > > -/**
> > > > - * idpf_set_vport_state - Set the vport state to be after the
> > > > reset
> > > > - * @adapter: Driver specific private structure
> > > > - */
> > > > -static void idpf_set_vport_state(struct idpf_adapter *adapter) -{
> > > > -	u16 i;
> > > > -
> > > > -	for (i = 0; i < adapter->max_vports; i++) {
> > > > -		struct idpf_netdev_priv *np;
> > > > -
> > > > -		if (!adapter->netdevs[i])
> > > > -			continue;
> > > > -
> > > > -		np = netdev_priv(adapter->netdevs[i]);
> > > > -		if (np->state == __IDPF_VPORT_UP)
> > > > -			set_bit(IDPF_VPORT_UP_REQUESTED,
> > > > -				adapter->vport_config[i]->flags);
> > > > -	}
> > > > -}
> > > > -
> > > >   /**
> > > >    * idpf_init_hard_reset - Initiate a hardware reset
> > > >    * @adapter: Driver specific private structure
> > >
> > > > @@ -1836,28 +1868,17 @@ static void idpf_set_vport_state(struct
> idpf_adapter *adapter)
> > > >    * reallocate. Also reinitialize the mailbox. Return 0 on success,
> > > >    * negative on failure.
> > > >    */
> > > > -static int idpf_init_hard_reset(struct idpf_adapter *adapter)
> > > > +static void idpf_init_hard_reset(struct idpf_adapter *adapter)
> > > >   {
> > > >   	struct idpf_reg_ops *reg_ops = &adapter->dev_ops.reg_ops;
> > > >   	struct device *dev = &adapter->pdev->dev;
> > > > -	struct net_device *netdev;
> > > >   	int err;
> > > > -	u16 i;
> > > > +	idpf_detach_and_close(adapter);
> > > >   	mutex_lock(&adapter->vport_ctrl_lock);
> > > >   	dev_info(dev, "Device HW Reset initiated\n");
> > > > -	/* Avoid TX hangs on reset */
> > > > -	for (i = 0; i < adapter->max_vports; i++) {
> > > > -		netdev = adapter->netdevs[i];
> > > > -		if (!netdev)
> > > > -			continue;
> > >
> > > Hi Emil,
> > >
> > > In this code that is removed there is a check for !netdev.
> > > And also there is a similar check in idpf_set_vport_state().
> > > But there is no such check in idpf_detach_and_close().
> > > Is this intentional?
> >
> > This logic is a bit confusing because the reset path is executed on
> > both driver load and a reset (since the initialization is identical it
> > makes sense to re-use the code). This is what roughly happens on load
> > and
> > reset:
> >
> > driver load -> reset -> configure vports -> create netdevs reset ->
> > de-allocate vports -> re-allocate vports
> >
> > The first patch:
> > https://lore.kernel.org/intel-wired-lan/20251121001218.4565-2-emil.s.t
> > antilov@intel.com/
> >
> > makes sure that we never lose the netdev on a reset, following a
> > successful driver load. Previously this could happen in the error path.
> > In other words during a reset there is no need to check for a netdev
> > as this is guaranteed, but we must make sure that vports are present
> > as those can be freed.
> >
> > The 5th patch:
> > https://lore.kernel.org/intel-wired-lan/20251121001218.4565-6-emil.s.t
> > antilov@intel.com/
> >
> > fixes another instance where we could fail in the reset error path by
> > ensuring the service task, which handles resets is cancelled as at
> > that point we have neither vports, nor netdevs, hence nothing to
> > "serve". Hope this makes sense, but the gist of it is that with this
> > series applied the reset can be protected by just making sure that the
> > vports are allocated. If for whatever reason netdevs happen to be
> > NULL, following this series it would be a bug introduced somewhere
> > else in the code that will have to be addressed.
> 
> I did spend a bit of time trying to figure out the flow, but not entirely
> successfully. Thanks for setting me straight.
> 
> ...

Tested-by: Samuel Salin <Samuel.salin@intel.com>

RE: [Intel-wired-lan] [PATCH iwl-net v2 3/5] idpf: fix memory leak in idpf_vport_rel()

[Salin, Samuel]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Emil Tantilov
> Sent: Thursday, November 20, 2025 4:12 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; decot@google.com; willemb@google.com; Hay, Joshua
> A <joshua.a.hay@intel.com>; Chittim, Madhu <madhu.chittim@intel.com>;
> Lobakin, Aleksander <aleksander.lobakin@intel.com>; Zaremba, Larysa
> <larysa.zaremba@intel.com>; iamvivekkumar@google.com
> Subject: [Intel-wired-lan] [PATCH iwl-net v2 3/5] idpf: fix memory leak in
> idpf_vport_rel()
> 
> Free vport->rx_ptype_lkup in idpf_vport_rel() to avoid leaking memory during
> a reset. Reported by kmemleak:
> 
> unreferenced object 0xff450acac838a000 (size 4096):
>   comm "kworker/u258:5", pid 7732, jiffies 4296830044
>   hex dump (first 32 bytes):
>     00 00 00 00 00 10 00 00 00 10 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 10 00 00 00 00 00 00  ................
>   backtrace (crc 3da81902):
>     __kmalloc_cache_noprof+0x469/0x7a0
>     idpf_send_get_rx_ptype_msg+0x90/0x570 [idpf]
>     idpf_init_task+0x1ec/0x8d0 [idpf]
>     process_one_work+0x226/0x6d0
>     worker_thread+0x19e/0x340
>     kthread+0x10f/0x250
>     ret_from_fork+0x251/0x2b0
>     ret_from_fork_asm+0x1a/0x30
> 
> Fixes: 0fe45467a104 ("idpf: add create vport and netdev configuration")
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
> ---
> 2.37.3
Tested-by: Samuel Salin <Samuel.salin@intel.com>

RE: [Intel-wired-lan] [PATCH iwl-net v2 4/5] idpf: fix memory leak in idpf_vc_core_deinit()

[Salin, Samuel]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Emil Tantilov
> Sent: Thursday, November 20, 2025 4:12 PM
> To: intel-wired-lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; decot@google.com; willemb@google.com; Hay, Joshua
> A <joshua.a.hay@intel.com>; Chittim, Madhu <madhu.chittim@intel.com>;
> Lobakin, Aleksander <aleksander.lobakin@intel.com>; Zaremba, Larysa
> <larysa.zaremba@intel.com>; iamvivekkumar@google.com
> Subject: [Intel-wired-lan] [PATCH iwl-net v2 4/5] idpf: fix memory leak in
> idpf_vc_core_deinit()
> 
> Make sure to free hw->lan_regs. Reported by kmemleak during reset:
> 
> unreferenced object 0xff1b913d02a936c0 (size 96):
>   comm "kworker/u258:14", pid 2174, jiffies 4294958305
>   hex dump (first 32 bytes):
>     00 00 00 c0 a8 ba 2d ff 00 00 00 00 00 00 00 00  ......-.........
>     00 00 40 08 00 00 00 00 00 00 25 b3 a8 ba 2d ff  ..@.......%...-.
>   backtrace (crc 36063c4f):
>     __kmalloc_noprof+0x48f/0x890
>     idpf_vc_core_init+0x6ce/0x9b0 [idpf]
>     idpf_vc_event_task+0x1fb/0x350 [idpf]
>     process_one_work+0x226/0x6d0
>     worker_thread+0x19e/0x340
>     kthread+0x10f/0x250
>     ret_from_fork+0x251/0x2b0
>     ret_from_fork_asm+0x1a/0x30
> 
> Fixes: 6aa53e861c1a ("idpf: implement get LAN MMIO memory regions")
> Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
> Reviewed-by: Joshua Hay <joshua.a.hay@intel.com>
> ---
> 2.37.3

Tested-by: Samuel Salin <Samuel.salin@intel.com>

RE: [Intel-wired-lan] [PATCH iwl-net v2 5/5] idpf: fix error handling in the init_task on load

[Salin, Samuel]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of
> Chittim, Madhu
> Sent: Monday, December 1, 2025 1:44 PM
> To: Tantilov, Emil S <emil.s.tantilov@intel.com>; intel-wired-
> lan@lists.osuosl.org
> Cc: netdev@vger.kernel.org; Loktionov, Aleksandr
> <aleksandr.loktionov@intel.com>; Kitszel, Przemyslaw
> <przemyslaw.kitszel@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.com>; andrew+netdev@lunn.ch;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; decot@google.com; willemb@google.com; Hay, Joshua
> A <joshua.a.hay@intel.com>; Lobakin, Aleksander
> <aleksander.lobakin@intel.com>; Zaremba, Larysa
> <larysa.zaremba@intel.com>; iamvivekkumar@google.com
> Subject: Re: [Intel-wired-lan] [PATCH iwl-net v2 5/5] idpf: fix error handling in
> the init_task on load
> 
> 
> 
> On 11/20/2025 4:12 PM, Tantilov, Emil S wrote:
> > If the init_task fails during a driver load, we end up without vports
> > and netdevs, effectively failing the entire process. In that state a
> > subsequent reset will result in a crash as the service task attempts
> > to access uninitialized resources. Following trace is from an error in
> > the init_task where the CREATE_VPORT (op 501) is rejected by the FW:
> >
> > [40922.763136] idpf 0000:83:00.0: Device HW Reset initiated
> > [40924.449797] idpf 0000:83:00.0: Transaction failed (op 501)
> > [40958.148190] idpf 0000:83:00.0: HW reset detected [40958.161202]
> > BUG: kernel NULL pointer dereference, address: 00000000000000a8 ...
> > [40958.168094] Workqueue: idpf-0000:83:00.0-vc_event
> > idpf_vc_event_task [idpf] [40958.168865] RIP:
> > 0010:idpf_vc_event_task+0x9b/0x350 [idpf] ...
> > [40958.177932] Call Trace:
> > [40958.178491]  <TASK>
> > [40958.179040]  process_one_work+0x226/0x6d0 [40958.179609]
> > worker_thread+0x19e/0x340 [40958.180158]  ?
> > __pfx_worker_thread+0x10/0x10 [40958.180702]  kthread+0x10f/0x250
> > [40958.181238]  ? __pfx_kthread+0x10/0x10 [40958.181774]
> > ret_from_fork+0x251/0x2b0 [40958.182307]  ? __pfx_kthread+0x10/0x10
> > [40958.182834]  ret_from_fork_asm+0x1a/0x30 [40958.183370]  </TASK>
> >
> > Fix the error handling in the init_task to make sure the service and
> > mailbox tasks are disabled if the error happens during load. These are
> > started in idpf_vc_core_init(), which spawns the init_task and has no
> > way of knowing if it failed. If the error happens on reset, following
> > successful driver load, the tasks can still run, as that will allow
> > the netdevs to attempt recovery through another reset. Stop the PTP
> > callbacks either way as those will be restarted by the call to
> > idpf_vc_core_init() during a successful reset.
> >
> > Fixes: 0fe45467a104 ("idpf: add create vport and netdev
> > configuration")
> > Reported-by: Vivek Kumar <iamvivekkumar@google.com>
> > Signed-off-by: Emil Tantilov <emil.s.tantilov@intel.com>
> 
> Reviewed-by: Madhu Chittim <madhu.chittim@intel.com>
> 
> > ---

Tested-by: Samuel Salin <Samuel.salin@intel.com>