diff mbox series

[net] igb: clean up in all error paths when enabling SR-IOV

Message ID 20230824091603.3188249-1-vinschen@redhat.com (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series [net] igb: clean up in all error paths when enabling SR-IOV | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1328 this patch: 1328
netdev/cc_maintainers warning 2 maintainers not CCed: pabeni@redhat.com edumazet@google.com
netdev/build_clang success Errors and warnings before: 1351 this patch: 1351
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1351 this patch: 1351
netdev/checkpatch warning WARNING: 'occured' may be misspelled - perhaps 'occurred'?
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Corinna Vinschen Aug. 24, 2023, 9:16 a.m. UTC 
After commit 50f303496d92 ("igb: Enable SR-IOV after reinit"), removing
the igb module could hang or crash (depending on the machine) when the
module has been loaded with the max_vfs parameter set to some value != 0.

In case of one test machine with a dual port 82580, this hang occured:

[  232.480687] igb 0000:41:00.1: removed PHC on enp65s0f1
[  233.093257] igb 0000:41:00.1: IOV Disabled
[  233.329969] pcieport 0000:40:01.0: AER: Multiple Uncorrected (Non-Fatal) err0
[  233.340302] igb 0000:41:00.0: PCIe Bus Error: severity=Uncorrected (Non-Fata)
[  233.352248] igb 0000:41:00.0:   device [8086:1516] error status/mask=00100000
[  233.361088] igb 0000:41:00.0:    [20] UnsupReq               (First)
[  233.368183] igb 0000:41:00.0: AER:   TLP Header: 40000001 0000040f cdbfc00c c
[  233.376846] igb 0000:41:00.1: PCIe Bus Error: severity=Uncorrected (Non-Fata)
[  233.388779] igb 0000:41:00.1:   device [8086:1516] error status/mask=00100000
[  233.397629] igb 0000:41:00.1:    [20] UnsupReq               (First)
[  233.404736] igb 0000:41:00.1: AER:   TLP Header: 40000001 0000040f cdbfc00c c
[  233.538214] pci 0000:41:00.1: AER: can't recover (no error_detected callback)
[  233.538401] igb 0000:41:00.0: removed PHC on enp65s0f0
[  233.546197] pcieport 0000:40:01.0: AER: device recovery failed
[  234.157244] igb 0000:41:00.0: IOV Disabled
[  371.619705] INFO: task irq/35-aerdrv:257 blocked for more than 122 seconds.
[  371.627489]       Not tainted 6.4.0-dirty #2
[  371.632257] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this.
[  371.641000] task:irq/35-aerdrv   state:D stack:0     pid:257   ppid:2      f0
[  371.650330] Call Trace:
[  371.653061]  <TASK>
[  371.655407]  __schedule+0x20e/0x660
[  371.659313]  schedule+0x5a/0xd0
[  371.662824]  schedule_preempt_disabled+0x11/0x20
[  371.667983]  __mutex_lock.constprop.0+0x372/0x6c0
[  371.673237]  ? __pfx_aer_root_reset+0x10/0x10
[  371.678105]  report_error_detected+0x25/0x1c0
[  371.682974]  ? __pfx_report_normal_detected+0x10/0x10
[  371.688618]  pci_walk_bus+0x72/0x90
[  371.692519]  pcie_do_recovery+0xb2/0x330
[  371.696899]  aer_process_err_devices+0x117/0x170
[  371.702055]  aer_isr+0x1c0/0x1e0
[  371.705661]  ? __set_cpus_allowed_ptr+0x54/0xa0
[  371.710723]  ? __pfx_irq_thread_fn+0x10/0x10
[  371.715496]  irq_thread_fn+0x20/0x60
[  371.719491]  irq_thread+0xe6/0x1b0
[  371.723291]  ? __pfx_irq_thread_dtor+0x10/0x10
[  371.728255]  ? __pfx_irq_thread+0x10/0x10
[  371.732731]  kthread+0xe2/0x110
[  371.736243]  ? __pfx_kthread+0x10/0x10
[  371.740430]  ret_from_fork+0x2c/0x50
[  371.744428]  </TASK>

The reproducer was a simple script:

  #!/bin/sh
  for i in `seq 1 5`; do
    modprobe -rv igb
    modprobe -v igb max_vfs=1
    sleep 1
    modprobe -rv igb
  done

It turned out that this could only be reproduce on 82580 (quad and
dual-port), but not on 82576, i350 and i210.  Further debugging showed
that igb_enable_sriov()'s call to pci_enable_sriov() is failing, because
dev->is_physfn is 0 on 82580.

Prior to commit 50f303496d92 ("igb: Enable SR-IOV after reinit"),
igb_enable_sriov() jumped into the "err_out" cleanup branch.  After this
commit it only returned the error code.

So the cleanup didn't take place, and the incorrect VF setup in the
igb_adapter structure fooled the igb driver into assuming that VFs have
been set up where no VF actually existed.

Fix this problem by cleaning up again if pci_enable_sriov() fails.

Fixes: 50f303496d92 ("igb: Enable SR-IOV after reinit")
Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Akihiko Odaki Aug. 24, 2023, 11:02 a.m. UTC | #1 
On 2023/08/24 18:16, Corinna Vinschen wrote:
> After commit 50f303496d92 ("igb: Enable SR-IOV after reinit"), removing
> the igb module could hang or crash (depending on the machine) when the
> module has been loaded with the max_vfs parameter set to some value != 0.
> 
> In case of one test machine with a dual port 82580, this hang occured:
> 
> [  232.480687] igb 0000:41:00.1: removed PHC on enp65s0f1
> [  233.093257] igb 0000:41:00.1: IOV Disabled
> [  233.329969] pcieport 0000:40:01.0: AER: Multiple Uncorrected (Non-Fatal) err0
> [  233.340302] igb 0000:41:00.0: PCIe Bus Error: severity=Uncorrected (Non-Fata)
> [  233.352248] igb 0000:41:00.0:   device [8086:1516] error status/mask=00100000
> [  233.361088] igb 0000:41:00.0:    [20] UnsupReq               (First)
> [  233.368183] igb 0000:41:00.0: AER:   TLP Header: 40000001 0000040f cdbfc00c c
> [  233.376846] igb 0000:41:00.1: PCIe Bus Error: severity=Uncorrected (Non-Fata)
> [  233.388779] igb 0000:41:00.1:   device [8086:1516] error status/mask=00100000
> [  233.397629] igb 0000:41:00.1:    [20] UnsupReq               (First)
> [  233.404736] igb 0000:41:00.1: AER:   TLP Header: 40000001 0000040f cdbfc00c c
> [  233.538214] pci 0000:41:00.1: AER: can't recover (no error_detected callback)
> [  233.538401] igb 0000:41:00.0: removed PHC on enp65s0f0
> [  233.546197] pcieport 0000:40:01.0: AER: device recovery failed
> [  234.157244] igb 0000:41:00.0: IOV Disabled
> [  371.619705] INFO: task irq/35-aerdrv:257 blocked for more than 122 seconds.
> [  371.627489]       Not tainted 6.4.0-dirty #2
> [  371.632257] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this.
> [  371.641000] task:irq/35-aerdrv   state:D stack:0     pid:257   ppid:2      f0
> [  371.650330] Call Trace:
> [  371.653061]  <TASK>
> [  371.655407]  __schedule+0x20e/0x660
> [  371.659313]  schedule+0x5a/0xd0
> [  371.662824]  schedule_preempt_disabled+0x11/0x20
> [  371.667983]  __mutex_lock.constprop.0+0x372/0x6c0
> [  371.673237]  ? __pfx_aer_root_reset+0x10/0x10
> [  371.678105]  report_error_detected+0x25/0x1c0
> [  371.682974]  ? __pfx_report_normal_detected+0x10/0x10
> [  371.688618]  pci_walk_bus+0x72/0x90
> [  371.692519]  pcie_do_recovery+0xb2/0x330
> [  371.696899]  aer_process_err_devices+0x117/0x170
> [  371.702055]  aer_isr+0x1c0/0x1e0
> [  371.705661]  ? __set_cpus_allowed_ptr+0x54/0xa0
> [  371.710723]  ? __pfx_irq_thread_fn+0x10/0x10
> [  371.715496]  irq_thread_fn+0x20/0x60
> [  371.719491]  irq_thread+0xe6/0x1b0
> [  371.723291]  ? __pfx_irq_thread_dtor+0x10/0x10
> [  371.728255]  ? __pfx_irq_thread+0x10/0x10
> [  371.732731]  kthread+0xe2/0x110
> [  371.736243]  ? __pfx_kthread+0x10/0x10
> [  371.740430]  ret_from_fork+0x2c/0x50
> [  371.744428]  </TASK>
> 
> The reproducer was a simple script:
> 
>    #!/bin/sh
>    for i in `seq 1 5`; do
>      modprobe -rv igb
>      modprobe -v igb max_vfs=1
>      sleep 1
>      modprobe -rv igb
>    done
> 
> It turned out that this could only be reproduce on 82580 (quad and
> dual-port), but not on 82576, i350 and i210.  Further debugging showed
> that igb_enable_sriov()'s call to pci_enable_sriov() is failing, because
> dev->is_physfn is 0 on 82580.
> 
> Prior to commit 50f303496d92 ("igb: Enable SR-IOV after reinit"),
> igb_enable_sriov() jumped into the "err_out" cleanup branch.  After this
> commit it only returned the error code.
> 
> So the cleanup didn't take place, and the incorrect VF setup in the
> igb_adapter structure fooled the igb driver into assuming that VFs have
> been set up where no VF actually existed.
> 
> Fix this problem by cleaning up again if pci_enable_sriov() fails.
> 
> Fixes: 50f303496d92 ("igb: Enable SR-IOV after reinit")
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> ---
>   drivers/net/ethernet/intel/igb/igb_main.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 9a2561409b06..42ab9ca7f97e 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -3827,8 +3827,11 @@ static int igb_enable_sriov(struct pci_dev *pdev, int num_vfs, bool reinit)
>   	}
>   
>   	/* only call pci_enable_sriov() if no VFs are allocated already */
> -	if (!old_vfs)
> +	if (!old_vfs) {
>   		err = pci_enable_sriov(pdev, adapter->vfs_allocated_count);
> +		if (err)
> +			goto err_out;
> +	}
>   
>   	goto out;
>   

Reviewed-by: Akihiko Odaki <akihiko.odaki@daynix.com>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 9a2561409b06..42ab9ca7f97e 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -3827,8 +3827,11 @@  static int igb_enable_sriov(struct pci_dev *pdev, int num_vfs, bool reinit)
 	}
 
 	/* only call pci_enable_sriov() if no VFs are allocated already */
-	if (!old_vfs)
+	if (!old_vfs) {
 		err = pci_enable_sriov(pdev, adapter->vfs_allocated_count);
+		if (err)
+			goto err_out;
+	}
 
 	goto out;