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 Reviewed-by: Aleksandr Loktionov --- 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 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 UP echo 1 > /sys/class/net/ens801f0/device/reset& \ ethtool -L ens801f0 combined 8 ip -br a DOWN ip link set up ip -br a 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] [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 Reviewed-by: Aleksandr Loktionov --- drivers/net/ethernet/intel/idpf/idpf_lib.c | 120 ++++++++++++--------- 1 file changed, 71 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..8c7c3e6bccc7 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 net_device *netdev = adapter->netdevs[i]; + struct idpf_vport_config *vport_config; + struct idpf_vport *vport; + + /* In case of a critical error in the init task, the vport + * will be freed. Only continue to restore the netdevs + * if vport is not NULL. + */ + vport = idpf_netdev_to_vport(netdev); + 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(). + */ + 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,13 @@ 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; + /* Restore netdevs state and initialize RDMA CORE AUX device, provided + * vport resources are allocated successfully. + */ + if (!err) { + idpf_attach_and_open(adapter); + idpf_idc_init(adapter); + } } /** -- 2.37.3 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 Reviewed-by: Aleksandr Loktionov Reviewed-by: Madhu Chittim --- 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 8c7c3e6bccc7..8c3041f00cda 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 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 Reviewed-by: Aleksandr Loktionov Reviewed-by: Joshua Hay --- 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