[PATCH net-next 01/15] igc: Call netif_queue_set_napi() with rtnl locked

[PATCH net-next 01/15] igc: Call netif_queue_set_napi() with rtnl locked

[Tony Nguyen]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

When runtime resuming igc we get:

  [  516.161666] RTNL: assertion failed at ./include/net/netdev_lock.h (72)

Happens because commit 310ae9eb2617 ("net: designate queue -> napi
linking as "ops protected"") added check for this. For this reason drop
the special case for runtime PM from __igc_resume(). This makes it take
rtnl lock unconditionally.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Avigail Dahan <avigailx.dahan@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 72bc5128d8b8..98024b46789f 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -7536,7 +7536,7 @@ static void igc_deliver_wake_packet(struct net_device *netdev)
 	netif_rx(skb);
 }
 
-static int __igc_resume(struct device *dev, bool rpm)
+static int __igc_resume(struct device *dev)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
 	struct net_device *netdev = pci_get_drvdata(pdev);
@@ -7581,11 +7581,9 @@ static int __igc_resume(struct device *dev, bool rpm)
 	wr32(IGC_WUS, ~0);
 
 	if (netif_running(netdev)) {
-		if (!rpm)
-			rtnl_lock();
+		rtnl_lock();
 		err = __igc_open(netdev, true);
-		if (!rpm)
-			rtnl_unlock();
+		rtnl_unlock();
 		if (!err)
 			netif_device_attach(netdev);
 	}
@@ -7595,12 +7593,12 @@ static int __igc_resume(struct device *dev, bool rpm)
 
 static int igc_resume(struct device *dev)
 {
-	return __igc_resume(dev, false);
+	return __igc_resume(dev);
 }
 
 static int igc_runtime_resume(struct device *dev)
 {
-	return __igc_resume(dev, true);
+	return __igc_resume(dev);
 }
 
 static int igc_suspend(struct device *dev)
-- 
2.47.1

Re: [PATCH net-next 01/15] igc: Call netif_queue_set_napi() with rtnl locked

[Bjorn Helgaas]

On Mon, Mar 30, 2026 at 04:02:30PM -0700, Tony Nguyen wrote:
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> When runtime resuming igc we get:
> 
>   [  516.161666] RTNL: assertion failed at ./include/net/netdev_lock.h (72)
> 
> Happens because commit 310ae9eb2617 ("net: designate queue -> napi
> linking as "ops protected"") added check for this. For this reason drop
> the special case for runtime PM from __igc_resume(). This makes it take
> rtnl lock unconditionally.

Taking the rtnl lock unconditionally certainly makes the code nicer,
but the commit log only mentions the "avoid the warning" benefit, not
the actual reason this is safe to do.

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> Tested-by: Avigail Dahan <avigailx.dahan@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 72bc5128d8b8..98024b46789f 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -7536,7 +7536,7 @@ static void igc_deliver_wake_packet(struct net_device *netdev)
>  	netif_rx(skb);
>  }
>  
> -static int __igc_resume(struct device *dev, bool rpm)
> +static int __igc_resume(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct net_device *netdev = pci_get_drvdata(pdev);
> @@ -7581,11 +7581,9 @@ static int __igc_resume(struct device *dev, bool rpm)
>  	wr32(IGC_WUS, ~0);
>  
>  	if (netif_running(netdev)) {
> -		if (!rpm)
> -			rtnl_lock();
> +		rtnl_lock();
>  		err = __igc_open(netdev, true);
> -		if (!rpm)
> -			rtnl_unlock();
> +		rtnl_unlock();
>  		if (!err)
>  			netif_device_attach(netdev);
>  	}
> @@ -7595,12 +7593,12 @@ static int __igc_resume(struct device *dev, bool rpm)
>  
>  static int igc_resume(struct device *dev)
>  {
> -	return __igc_resume(dev, false);
> +	return __igc_resume(dev);
>  }
>  
>  static int igc_runtime_resume(struct device *dev)
>  {
> -	return __igc_resume(dev, true);
> +	return __igc_resume(dev);
>  }
>  
>  static int igc_suspend(struct device *dev)
> -- 
> 2.47.1
> 

Re: [PATCH net-next 01/15] igc: Call netif_queue_set_napi() with rtnl locked

[Paolo Abeni]

On 3/31/26 7:37 PM, Bjorn Helgaas wrote:
> On Mon, Mar 30, 2026 at 04:02:30PM -0700, Tony Nguyen wrote:
>> From: Mika Westerberg <mika.westerberg@linux.intel.com>
>>
>> When runtime resuming igc we get:
>>
>>   [  516.161666] RTNL: assertion failed at ./include/net/netdev_lock.h (72)
>>
>> Happens because commit 310ae9eb2617 ("net: designate queue -> napi
>> linking as "ops protected"") added check for this. For this reason drop
>> the special case for runtime PM from __igc_resume(). This makes it take
>> rtnl lock unconditionally.
> 
> Taking the rtnl lock unconditionally certainly makes the code nicer,
> but the commit log only mentions the "avoid the warning" benefit, not
> the actual reason this is safe to do.

Sashiko says it's not safe:

---
Can this regression cause a self-deadlock when a runtime resume is
triggered from paths that already hold the rtnl lock?
If the network interface is logically up but the link is disconnected,
igc_runtime_idle() allows the device to enter runtime suspend. When a
user queries the device using ethtool, the networking core acquires
rtnl_lock() and then calls pm_runtime_get_sync() to ensure the hardware
is awake.
This synchronously executes the driver's runtime resume callback, which
calls __igc_resume(). Because netif_running(netdev) is true, the
modified __igc_resume() unconditionally attempts to acquire rtnl_lock().
Since the executing thread already holds this non-recursive mutex, it
appears the system would self-deadlock, hanging the network stack.
---

/P

Re: [PATCH net-next 01/15] igc: Call netif_queue_set_napi() with rtnl locked

[Mika Westerberg]

Hi,

On Thu, Apr 02, 2026 at 12:29:06PM +0200, Paolo Abeni wrote:
> On 3/31/26 7:37 PM, Bjorn Helgaas wrote:
> > On Mon, Mar 30, 2026 at 04:02:30PM -0700, Tony Nguyen wrote:
> >> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> >>
> >> When runtime resuming igc we get:
> >>
> >>   [  516.161666] RTNL: assertion failed at ./include/net/netdev_lock.h (72)
> >>
> >> Happens because commit 310ae9eb2617 ("net: designate queue -> napi
> >> linking as "ops protected"") added check for this. For this reason drop
> >> the special case for runtime PM from __igc_resume(). This makes it take
> >> rtnl lock unconditionally.
> > 
> > Taking the rtnl lock unconditionally certainly makes the code nicer,
> > but the commit log only mentions the "avoid the warning" benefit, not
> > the actual reason this is safe to do.
> 
> Sashiko says it's not safe:
> 
> ---
> Can this regression cause a self-deadlock when a runtime resume is
> triggered from paths that already hold the rtnl lock?
> If the network interface is logically up but the link is disconnected,
> igc_runtime_idle() allows the device to enter runtime suspend. When a
> user queries the device using ethtool, the networking core acquires
> rtnl_lock() and then calls pm_runtime_get_sync() to ensure the hardware
> is awake.
> This synchronously executes the driver's runtime resume callback, which
> calls __igc_resume(). Because netif_running(netdev) is true, the
> modified __igc_resume() unconditionally attempts to acquire rtnl_lock().
> Since the executing thread already holds this non-recursive mutex, it
> appears the system would self-deadlock, hanging the network stack.
> ---

It's a good analysis. I just tried this flow:

1. Boot the system up, nothing connected to igc NIC.
2. Plug in cable to igc.
3. Configure the interface.
4. Enable runtime PM for igc.
5. Unplug the cable.
6. Verify igc is runtime suspended.
7. Run ethtool <interface>

This leads to deadlock as below.

igc maintainers, please drop this patch. I apologize I did not realize this
flow when I did it.

[ 1231.655924] INFO: task ethtool:3139 blocked for more than 122 seconds.
[ 1231.662515]       Tainted: G     U              7.0.0-rc6+ #1748
[ 1231.668551] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[ 1231.676410] task:ethtool         state:D stack:0     pid:3139  tgid:3139  ppid:292    task_flags:0x480000 flags:0x00080800
[ 1231.687508] Call Trace:
[ 1231.689997]  <TASK>
[ 1231.692132]  __schedule+0x58a/0x1820
[ 1231.695747]  ? sysvec_apic_timer_interrupt+0x4c/0xa0
[ 1231.700742]  ? asm_sysvec_apic_timer_interrupt+0x1b/0x20
[ 1231.706090]  schedule+0x64/0xe0
[ 1231.709262]  schedule_preempt_disabled+0x15/0x30
[ 1231.713907]  __mutex_lock+0x377/0xa60
[ 1231.717606]  __mutex_lock_slowpath+0x13/0x20
[ 1231.721905]  mutex_lock+0x2c/0x40
[ 1231.725259]  rtnl_lock+0x15/0x20
[ 1231.728541]  __igc_resume+0x19a/0x2b0 [igc]
[ 1231.732798]  igc_runtime_resume+0xe/0x20 [igc]
[ 1231.737288]  pci_pm_runtime_resume+0xce/0x100
[ 1231.741678]  ? __pfx_pci_pm_runtime_resume+0x10/0x10
[ 1231.746681]  __rpm_callback+0xab/0x310
[ 1231.750458]  ? ktime_get_mono_fast_ns+0x3a/0x100
[ 1231.755107]  ? __pfx_pci_pm_runtime_resume+0x10/0x10
[ 1231.760096]  rpm_resume+0x4bb/0x670
[ 1231.763618]  __pm_runtime_resume+0x5c/0x80
[ 1231.767749]  dev_ethtool+0x19d/0xc90
[ 1231.771352]  dev_ioctl+0x23c/0x550
[ 1231.774791]  sock_do_ioctl+0x11f/0x1b0
[ 1231.778569]  sock_ioctl+0x27f/0x390
[ 1231.782091]  ? handle_mm_fault+0x11a5/0x1250
[ 1231.786388]  __se_sys_ioctl+0x75/0xd0
[ 1231.790077]  __x64_sys_ioctl+0x1d/0x30
[ 1231.793851]  x64_sys_call+0x14ed/0x2d30
[ 1231.797719]  do_syscall_64+0xfb/0x680
[ 1231.801404]  ? arch_exit_to_user_mode_prepare+0xd/0xb0
[ 1231.806559]  ? irqentry_exit+0x3b/0x510
[ 1231.810413]  entry_SYSCALL_64_after_hwframe+0x76/0x7e

[PATCH net-next 02/15] igc: Let the PCI core deal with the PM resume flow

[Tony Nguyen]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

Currently igc driver calls pci_set_power_state() and pci_restore_state()
and the like to bring the device back from low power states. However,
PCI core handles all this on behalf of the driver. Furthermore with PTM
enabled the PCI core re-enables it on resume but the driver calls
pci_restore_state() which ends up disabling it again.

For this reason let the PCI core handle the common PM resume flow.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Avigail Dahan <avigailx.dahan@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 98024b46789f..0e785af0a3a3 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -7544,9 +7544,6 @@ static int __igc_resume(struct device *dev)
 	struct igc_hw *hw = &adapter->hw;
 	u32 err, val;
 
-	pci_set_power_state(pdev, PCI_D0);
-	pci_restore_state(pdev);
-
 	if (!pci_device_is_present(pdev))
 		return -ENODEV;
 	err = pci_enable_device_mem(pdev);
@@ -7556,9 +7553,6 @@ static int __igc_resume(struct device *dev)
 	}
 	pci_set_master(pdev);
 
-	pci_enable_wake(pdev, PCI_D3hot, 0);
-	pci_enable_wake(pdev, PCI_D3cold, 0);
-
 	if (igc_is_device_id_i226(hw))
 		pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
 
-- 
2.47.1

Re: [PATCH net-next 02/15] igc: Let the PCI core deal with the PM resume flow

[Bjorn Helgaas]

On Mon, Mar 30, 2026 at 04:02:31PM -0700, Tony Nguyen wrote:
> From: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> Currently igc driver calls pci_set_power_state() and pci_restore_state()
> and the like to bring the device back from low power states. However,
> PCI core handles all this on behalf of the driver. Furthermore with PTM
> enabled the PCI core re-enables it on resume but the driver calls
> pci_restore_state() which ends up disabling it again.
> 
> For this reason let the PCI core handle the common PM resume flow.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
> Tested-by: Avigail Dahan <avigailx.dahan@intel.com>
> Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>

I love it, thanks a lot for doing this!

> ---
>  drivers/net/ethernet/intel/igc/igc_main.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 98024b46789f..0e785af0a3a3 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -7544,9 +7544,6 @@ static int __igc_resume(struct device *dev)
>  	struct igc_hw *hw = &adapter->hw;
>  	u32 err, val;
>  
> -	pci_set_power_state(pdev, PCI_D0);
> -	pci_restore_state(pdev);
> -
>  	if (!pci_device_is_present(pdev))
>  		return -ENODEV;
>  	err = pci_enable_device_mem(pdev);

__igc_shutdown() calls pci_disable_device() in the suspend paths,
which disables bus mastering.  There are only a relative handful of
drivers that do this, mostly for NICs, so I'm not sure if it's really
necessary or if it's been cargo-culted.

Here in the resume path we call pci_enable_device_mem(),
pci_set_master(), and conditionally call pci_disable_link_state().  I
suspect all those are unnecessary because pci_pm_resume() should have
called pci_restore_state() before we get here.

> @@ -7556,9 +7553,6 @@ static int __igc_resume(struct device *dev)
>  	}
>  	pci_set_master(pdev);
>  
> -	pci_enable_wake(pdev, PCI_D3hot, 0);
> -	pci_enable_wake(pdev, PCI_D3cold, 0);
> -
>  	if (igc_is_device_id_i226(hw))
>  		pci_disable_link_state(pdev, PCIE_LINK_STATE_L1_2);
>  
> -- 
> 2.47.1
> 

[PATCH net-next 03/15] igc: Don't reset the hardware on suspend path

[Tony Nguyen]

From: Mika Westerberg <mika.westerberg@linux.intel.com>

Commit c01163dbd1b8 ("PCI/PM: Always disable PTM for all devices during
suspend") made the PCI core to suspend (disable) PTM before driver
suspend hooks are called. In case of igc what happens is that on suspend
path PCI core calls pci_suspend_ptm() then igc suspend hook that calls
igc_down() that ends up calling igc_ptp_reset() (which according to the
comment is actually needed for re-enabling the device). Anyways that
function also poll IGC_PTM_STAT that will end up timing out because PTM
is already disabled:

  [  160.716119] igc 0000:03:00.0 enp3s0: Timeout reading IGC_PTM_STAT register

There should be no reason resetting the hardware on suspend path so fix
this by avoiding the reset.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Tested-by: Avigail Dahan <avigailx.dahan@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc.h         |  2 +-
 drivers/net/ethernet/intel/igc/igc_ethtool.c |  6 +++---
 drivers/net/ethernet/intel/igc/igc_main.c    | 13 +++++++------
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 17236813965d..caa5b89b2b24 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -349,7 +349,7 @@ struct igc_adapter {
 };
 
 void igc_up(struct igc_adapter *adapter);
-void igc_down(struct igc_adapter *adapter);
+void igc_down(struct igc_adapter *adapter, bool reset);
 int igc_open(struct net_device *netdev);
 int igc_close(struct net_device *netdev);
 int igc_setup_tx_resources(struct igc_ring *ring);
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 0122009bedd0..4d1bcc19255f 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -638,7 +638,7 @@ igc_ethtool_set_ringparam(struct net_device *netdev,
 		goto clear_reset;
 	}
 
-	igc_down(adapter);
+	igc_down(adapter, true);
 
 	/* We can't just free everything and then setup again,
 	 * because the ISRs in MSI-X mode get passed pointers
@@ -737,7 +737,7 @@ static int igc_ethtool_set_pauseparam(struct net_device *netdev,
 	if (adapter->fc_autoneg == AUTONEG_ENABLE) {
 		hw->fc.requested_mode = igc_fc_default;
 		if (netif_running(adapter->netdev)) {
-			igc_down(adapter);
+			igc_down(adapter, true);
 			igc_up(adapter);
 		} else {
 			igc_reset(adapter);
@@ -2077,7 +2077,7 @@ igc_ethtool_set_link_ksettings(struct net_device *netdev,
 
 	/* reset the link */
 	if (netif_running(adapter->netdev)) {
-		igc_down(adapter);
+		igc_down(adapter, true);
 		igc_up(adapter);
 	} else {
 		igc_reset(adapter);
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 0e785af0a3a3..80a90ce0ad0e 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -5312,8 +5312,9 @@ void igc_update_stats(struct igc_adapter *adapter)
 /**
  * igc_down - Close the interface
  * @adapter: board private structure
+ * @reset: issue reset
  */
-void igc_down(struct igc_adapter *adapter)
+void igc_down(struct igc_adapter *adapter, bool reset)
 {
 	struct net_device *netdev = adapter->netdev;
 	struct igc_hw *hw = &adapter->hw;
@@ -5369,7 +5370,7 @@ void igc_down(struct igc_adapter *adapter)
 	adapter->link_speed = 0;
 	adapter->link_duplex = 0;
 
-	if (!pci_channel_offline(adapter->pdev))
+	if (reset && !pci_channel_offline(adapter->pdev))
 		igc_reset(adapter);
 
 	/* clear VLAN promisc flag so VFTA will be updated if necessary */
@@ -5387,7 +5388,7 @@ void igc_reinit_locked(struct igc_adapter *adapter)
 {
 	while (test_and_set_bit(__IGC_RESETTING, &adapter->state))
 		usleep_range(1000, 2000);
-	igc_down(adapter);
+	igc_down(adapter, true);
 	igc_up(adapter);
 	clear_bit(__IGC_RESETTING, &adapter->state);
 }
@@ -5441,7 +5442,7 @@ static int igc_change_mtu(struct net_device *netdev, int new_mtu)
 	adapter->max_frame_size = max_frame;
 
 	if (netif_running(netdev))
-		igc_down(adapter);
+		igc_down(adapter, true);
 
 	netdev_dbg(netdev, "changing MTU from %d to %d\n", netdev->mtu, new_mtu);
 	WRITE_ONCE(netdev->mtu, new_mtu);
@@ -6305,7 +6306,7 @@ static int __igc_close(struct net_device *netdev, bool suspending)
 	if (!suspending)
 		pm_runtime_get_sync(&pdev->dev);
 
-	igc_down(adapter);
+	igc_down(adapter, !suspending);
 
 	igc_release_hw_control(adapter);
 
@@ -7646,7 +7647,7 @@ static pci_ers_result_t igc_io_error_detected(struct pci_dev *pdev,
 	}
 
 	if (netif_running(netdev))
-		igc_down(adapter);
+		igc_down(adapter, true);
 	pci_disable_device(pdev);
 	rtnl_unlock();
 
-- 
2.47.1