| Message ID | 20230824091603.3188249-1-vinschen@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Anthony Nguyen |
| Headers | show |
| Series | [net] igb: clean up in all error paths when enabling SR-IOV | expand |
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>
Question to the Intel folks: On Aug 24 11: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: > [...] > 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. Along these lines, isn't the first and foremost bug that igb_enable_sriov() has been called for this NIC at all? In terms of patches, shouldn't the guard expression in igb_probe_vfs() /* Virtualization features not supported on i210 family. */ if ((hw->mac.type == e1000_i210) || (hw->mac.type == e1000_i211)) return; get changed to: /* Virtualization features not supported on i210 and 82580 family. */ if ((hw->mac.type == e1000_i210) || (hw->mac.type == e1000_i211) || (hw->mac.type == e1000_82580)) return; or, to make it independent of the actual HW: /* Virtualization features not supported? */ if (!pdev->is_physfn) return; Thanks, Corinna
On 8/24/2023 6:07 AM, Corinna Vinschen wrote: > Question to the Intel folks: > > On Aug 24 11: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: >> [...] >> 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. > > Along these lines, isn't the first and foremost bug that igb_enable_sriov() > has been called for this NIC at all? In terms of patches, shouldn't the > guard expression in igb_probe_vfs() > > /* Virtualization features not supported on i210 family. */ > if ((hw->mac.type == e1000_i210) || (hw->mac.type == e1000_i211)) > return; > > get changed to: > > /* Virtualization features not supported on i210 and 82580 family. */ > if ((hw->mac.type == e1000_i210) || (hw->mac.type == e1000_i211) || > (hw->mac.type == e1000_82580)) > return; Hi Corinna, Adding 82580 to this seems like a good change; did you want to submit a patch to do this? Thanks, Tony
On Aug 29 16:09, Tony Nguyen wrote: > On 8/24/2023 6:07 AM, Corinna Vinschen wrote: > > Question to the Intel folks: > > > > On Aug 24 11: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: > > > [...] > > > 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. > > > > Along these lines, isn't the first and foremost bug that igb_enable_sriov() > > has been called for this NIC at all? In terms of patches, shouldn't the > > guard expression in igb_probe_vfs() > > > > /* Virtualization features not supported on i210 family. */ > > if ((hw->mac.type == e1000_i210) || (hw->mac.type == e1000_i211)) > > return; > > > > get changed to: > > > > /* Virtualization features not supported on i210 and 82580 family. */ > > if ((hw->mac.type == e1000_i210) || (hw->mac.type == e1000_i211) || > > (hw->mac.type == e1000_82580)) > > return; > > Hi Corinna, > > Adding 82580 to this seems like a good change; did you want to submit a > patch to do this? Hi Tony, sure, I just sent the patch. Thanks, Corinna
> -----Original Message----- > From: Intel-wired-lan <intel-wired-lan-bounces@osuosl.org> On Behalf Of > Corinna Vinschen > Sent: Thursday, August 31, 2023 10:10 AM > To: Nguyen, Anthony L <anthony.l.nguyen@intel.com> > Cc: intel-wired-lan@lists.osuosl.org; Brandeburg, Jesse > <jesse.brandeburg@intel.com> > Subject: Re: [Intel-wired-lan] [PATCH net] igb: clean up in all error paths when > enabling SR-IOV > > On Aug 29 16:09, Tony Nguyen wrote: > > On 8/24/2023 6:07 AM, Corinna Vinschen wrote: > > > Question to the Intel folks: > > > > > > On Aug 24 11: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: > > > > [...] > > > > 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. > > > > > > Along these lines, isn't the first and foremost bug that > > > igb_enable_sriov() has been called for this NIC at all? In terms of > > > patches, shouldn't the guard expression in igb_probe_vfs() > > > > > > /* Virtualization features not supported on i210 family. */ > > > if ((hw->mac.type == e1000_i210) || (hw->mac.type == e1000_i211)) > > > return; > > > > > > get changed to: > > > > > > /* Virtualization features not supported on i210 and 82580 family. */ > > > if ((hw->mac.type == e1000_i210) || (hw->mac.type == e1000_i211) > || > > > (hw->mac.type == e1000_82580)) > > > return; > > Tested-by: Rafal Romanowski <rafal.romanowski@intel.com>
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;
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(-)