[net PATCH 0/2] Locking fixes for fbnic driver

[net PATCH 0/2] Locking fixes for fbnic driver

[Alexander Duyck]

Address a few locking issues that were reported on the fbnic driver.
Specifically in one case we were seeing locking leaks due to us not
releasing the locks in certain exception paths. In another case we were
using phylink_resume outside of a section in which we held the RTNL mutex
and as a result we were throwing an assert.

---

Alexander Duyck (2):
      fbnic: Fixup rtnl_lock and devl_lock handling related to mailbox code
      fbnic: Move phylink resume out of service_task and into open/close


 drivers/net/ethernet/meta/fbnic/fbnic_netdev.c |  4 ++++
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c    | 15 ++++++---------
 2 files changed, 10 insertions(+), 9 deletions(-)

--

[net PATCH 1/2] fbnic: Fixup rtnl_lock and devl_lock handling related to mailbox code

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The exception handling path for the __fbnic_pm_resume function had a bug in
that it was taking the devlink lock and then exiting to exception handling
instead of waiting until after it released the lock to do so. In order to
handle that I am swapping the placement of the unlock and the exception
handling jump to label so that we don't trigger a deadlock by holding the
lock longer than we need to.

In addition this change applies the same ordering to the rtnl_lock/unlock
calls in the same function as it should make the code easier to follow if
it adheres to a consistent pattern.

Fixes: 82534f446daa ("eth: fbnic: Add devlink dev flash support")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c |   13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
index b70e4cadb37b..a7784deea88f 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
@@ -443,11 +443,10 @@ static int __fbnic_pm_resume(struct device *dev)
 
 	/* Re-enable mailbox */
 	err = fbnic_fw_request_mbx(fbd);
+	devl_unlock(priv_to_devlink(fbd));
 	if (err)
 		goto err_free_irqs;
 
-	devl_unlock(priv_to_devlink(fbd));
-
 	/* Only send log history if log buffer is empty to prevent duplicate
 	 * log entries.
 	 */
@@ -464,20 +463,20 @@ static int __fbnic_pm_resume(struct device *dev)
 
 	rtnl_lock();
 
-	if (netif_running(netdev)) {
+	if (netif_running(netdev))
 		err = __fbnic_open(fbn);
-		if (err)
-			goto err_free_mbx;
-	}
 
 	rtnl_unlock();
+	if (err)
+		goto err_free_mbx;
 
 	return 0;
 err_free_mbx:
 	fbnic_fw_log_disable(fbd);
 
-	rtnl_unlock();
+	devl_lock(priv_to_devlink(fbd));
 	fbnic_fw_free_mbx(fbd);
+	devl_unlock(priv_to_devlink(fbd));
 err_free_irqs:
 	fbnic_free_irqs(fbd);
 err_invalidate_uc_addr:

[net PATCH 2/2] fbnic: Move phylink resume out of service_task and into open/close

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

The fbnic driver was presenting with the following locking assert coming
out of a PM resume:
[   42.208116][  T164] RTNL: assertion failed at drivers/net/phy/phylink.c (2611)
[   42.208492][  T164] WARNING: CPU: 1 PID: 164 at drivers/net/phy/phylink.c:2611 phylink_resume+0x190/0x1e0
[   42.208872][  T164] Modules linked in:
[   42.209140][  T164] CPU: 1 UID: 0 PID: 164 Comm: bash Not tainted 6.17.0-rc2-virtme #134 PREEMPT(full)
[   42.209496][  T164] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.17.0-5.fc42 04/01/2014
[   42.209861][  T164] RIP: 0010:phylink_resume+0x190/0x1e0
[   42.210057][  T164] Code: 83 e5 01 0f 85 b0 fe ff ff c6 05 1c cd 3e 02 01 90 ba 33 0a 00 00 48 c7 c6 20 3a 1d a5 48 c7 c7 e0 3e 1d a5 e8 21 b8 90 fe 90 <0f> 0b 90 90 e9 86 fe ff ff e8 42 ea 1f ff e9 e2 fe ff ff 48 89 ef
[   42.210708][  T164] RSP: 0018:ffffc90000affbd8 EFLAGS: 00010296
[   42.210983][  T164] RAX: 0000000000000000 RBX: ffff8880078d8400 RCX: 0000000000000000
[   42.211235][  T164] RDX: 0000000000000000 RSI: 1ffffffff4f10938 RDI: 0000000000000001
[   42.211466][  T164] RBP: 0000000000000000 R08: ffffffffa2ae79ea R09: fffffbfff4b3eb84
[   42.211707][  T164] R10: 0000000000000003 R11: 0000000000000000 R12: ffff888007ad8000
[   42.211997][  T164] R13: 0000000000000002 R14: ffff888006a18800 R15: ffffffffa34c59e0
[   42.212234][  T164] FS:  00007f0dc8e39740(0000) GS:ffff88808f51f000(0000) knlGS:0000000000000000
[   42.212505][  T164] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   42.212704][  T164] CR2: 00007f0dc8e9fe10 CR3: 000000000b56d003 CR4: 0000000000772ef0
[   42.213227][  T164] PKRU: 55555554
[   42.213366][  T164] Call Trace:
[   42.213483][  T164]  <TASK>
[   42.213565][  T164]  __fbnic_pm_attach.isra.0+0x8e/0xa0
[   42.213725][  T164]  pci_reset_function+0x116/0x1d0
[   42.213895][  T164]  reset_store+0xa0/0x100
[   42.214025][  T164]  ? pci_dev_reset_attr_is_visible+0x50/0x50
[   42.214221][  T164]  ? sysfs_file_kobj+0xc1/0x1e0
[   42.214374][  T164]  ? sysfs_kf_write+0x65/0x160
[   42.214526][  T164]  kernfs_fop_write_iter+0x2f8/0x4c0
[   42.214677][  T164]  ? kernfs_vma_page_mkwrite+0x1f0/0x1f0
[   42.214836][  T164]  new_sync_write+0x308/0x6f0
[   42.214987][  T164]  ? __lock_acquire+0x34c/0x740
[   42.215135][  T164]  ? new_sync_read+0x6f0/0x6f0
[   42.215288][  T164]  ? lock_acquire.part.0+0xbc/0x260
[   42.215440][  T164]  ? ksys_write+0xff/0x200
[   42.215590][  T164]  ? perf_trace_sched_switch+0x6d0/0x6d0
[   42.215742][  T164]  vfs_write+0x65e/0xbb0
[   42.215876][  T164]  ksys_write+0xff/0x200
[   42.215994][  T164]  ? __ia32_sys_read+0xc0/0xc0
[   42.216141][  T164]  ? do_user_addr_fault+0x269/0x9f0
[   42.216292][  T164]  ? rcu_is_watching+0x15/0xd0
[   42.216442][  T164]  do_syscall_64+0xbb/0x360
[   42.216591][  T164]  entry_SYSCALL_64_after_hwframe+0x4b/0x53
[   42.216784][  T164] RIP: 0033:0x7f0dc8ea9986

A bit of digging showed that we were invoking the phylink_resume as a part
of the fbnic_up path when we were enabling the service task while not
holding the RTNL lock. We should be enabling this sooner as a part of the
ndo_open path and then just letting the service task come online later.
This will help to enforce the correct locking and brings the phylink
interface online at the same time as the network interface, instead of at a
later time.

I tested this on QEMU to verify this was working by putting the system to
sleep using "echo mem > /sys/power/state" to put the system to sleep in the
guest and then using the command "system_wakeup" in the QEMU monitor.

Fixes: 69684376eed5 ("eth: fbnic: Add link detection")
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_netdev.c |    4 ++++
 drivers/net/ethernet/meta/fbnic/fbnic_pci.c    |    2 --
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
index e67e99487a27..40581550da1a 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
@@ -52,6 +52,8 @@ int __fbnic_open(struct fbnic_net *fbn)
 	fbnic_bmc_rpc_init(fbd);
 	fbnic_rss_reinit(fbd, fbn);
 
+	phylink_resume(fbn->phylink);
+
 	return 0;
 time_stop:
 	fbnic_time_stop(fbn);
@@ -84,6 +86,8 @@ static int fbnic_stop(struct net_device *netdev)
 {
 	struct fbnic_net *fbn = netdev_priv(netdev);
 
+	phylink_suspend(fbn->phylink, fbnic_bmc_present(fbn->fbd));
+
 	fbnic_down(fbn);
 	fbnic_pcs_free_irq(fbn->fbd);
 
diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
index a7784deea88f..28e23e3ffca8 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
@@ -118,14 +118,12 @@ static void fbnic_service_task_start(struct fbnic_net *fbn)
 	struct fbnic_dev *fbd = fbn->fbd;
 
 	schedule_delayed_work(&fbd->service_task, HZ);
-	phylink_resume(fbn->phylink);
 }
 
 static void fbnic_service_task_stop(struct fbnic_net *fbn)
 {
 	struct fbnic_dev *fbd = fbn->fbd;
 
-	phylink_suspend(fbn->phylink, fbnic_bmc_present(fbd));
 	cancel_delayed_work(&fbd->service_task);
 }
 

Re: [net PATCH 1/2] fbnic: Fixup rtnl_lock and devl_lock handling related to mailbox code

[Przemek Kitszel]

On 8/26/25 00:56, Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@fb.com>
> 
> The exception handling path for the __fbnic_pm_resume function had a bug in
> that it was taking the devlink lock and then exiting to exception handling
> instead of waiting until after it released the lock to do so. In order to
> handle that I am swapping the placement of the unlock and the exception
> handling jump to label so that we don't trigger a deadlock by holding the
> lock longer than we need to.
> 
> In addition this change applies the same ordering to the rtnl_lock/unlock
> calls in the same function as it should make the code easier to follow if
> it adheres to a consistent pattern.

it does indeed, with the added benefit of calling fbnic_fw_log_disable()
w/o rtnl

> 
> Fixes: 82534f446daa ("eth: fbnic: Add devlink dev flash support")> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

> ---
>   drivers/net/ethernet/meta/fbnic/fbnic_pci.c |   13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> index b70e4cadb37b..a7784deea88f 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> @@ -443,11 +443,10 @@ static int __fbnic_pm_resume(struct device *dev)
>   
>   	/* Re-enable mailbox */
>   	err = fbnic_fw_request_mbx(fbd);
> +	devl_unlock(priv_to_devlink(fbd));
>   	if (err)
>   		goto err_free_irqs;
>   
> -	devl_unlock(priv_to_devlink(fbd));
> -
>   	/* Only send log history if log buffer is empty to prevent duplicate
>   	 * log entries.
>   	 */
> @@ -464,20 +463,20 @@ static int __fbnic_pm_resume(struct device *dev)
>   
>   	rtnl_lock();
>   
> -	if (netif_running(netdev)) {
> +	if (netif_running(netdev))
>   		err = __fbnic_open(fbn);
> -		if (err)
> -			goto err_free_mbx;
> -	}
>   
>   	rtnl_unlock();
> +	if (err)
> +		goto err_free_mbx;
>   
>   	return 0;
>   err_free_mbx:
>   	fbnic_fw_log_disable(fbd);
>   
> -	rtnl_unlock();
> +	devl_lock(priv_to_devlink(fbd));
>   	fbnic_fw_free_mbx(fbd);
> +	devl_unlock(priv_to_devlink(fbd));
>   err_free_irqs:
>   	fbnic_free_irqs(fbd);
>   err_invalidate_uc_addr:
> 
> 
> 

Re: [net PATCH 2/2] fbnic: Move phylink resume out of service_task and into open/close

[Przemek Kitszel]

On 8/26/25 00:56, Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@fb.com>
> 
> The fbnic driver was presenting with the following locking assert coming
> out of a PM resume:
> [   42.208116][  T164] RTNL: assertion failed at drivers/net/phy/phylink.c (2611)
> [   42.208492][  T164] WARNING: CPU: 1 PID: 164 at drivers/net/phy/phylink.c:2611 phylink_resume+0x190/0x1e0
> [   42.208872][  T164] Modules linked in:
> [   42.209140][  T164] CPU: 1 UID: 0 PID: 164 Comm: bash Not tainted 6.17.0-rc2-virtme #134 PREEMPT(full)
> [   42.209496][  T164] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.17.0-5.fc42 04/01/2014
> [   42.209861][  T164] RIP: 0010:phylink_resume+0x190/0x1e0
> [   42.210057][  T164] Code: 83 e5 01 0f 85 b0 fe ff ff c6 05 1c cd 3e 02 01 90 ba 33 0a 00 00 48 c7 c6 20 3a 1d a5 48 c7 c7 e0 3e 1d a5 e8 21 b8 90 fe 90 <0f> 0b 90 90 e9 86 fe ff ff e8 42 ea 1f ff e9 e2 fe ff ff 48 89 ef
> [   42.210708][  T164] RSP: 0018:ffffc90000affbd8 EFLAGS: 00010296
> [   42.210983][  T164] RAX: 0000000000000000 RBX: ffff8880078d8400 RCX: 0000000000000000
> [   42.211235][  T164] RDX: 0000000000000000 RSI: 1ffffffff4f10938 RDI: 0000000000000001
> [   42.211466][  T164] RBP: 0000000000000000 R08: ffffffffa2ae79ea R09: fffffbfff4b3eb84
> [   42.211707][  T164] R10: 0000000000000003 R11: 0000000000000000 R12: ffff888007ad8000
> [   42.211997][  T164] R13: 0000000000000002 R14: ffff888006a18800 R15: ffffffffa34c59e0
> [   42.212234][  T164] FS:  00007f0dc8e39740(0000) GS:ffff88808f51f000(0000) knlGS:0000000000000000
> [   42.212505][  T164] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   42.212704][  T164] CR2: 00007f0dc8e9fe10 CR3: 000000000b56d003 CR4: 0000000000772ef0
> [   42.213227][  T164] PKRU: 55555554
> [   42.213366][  T164] Call Trace:
> [   42.213483][  T164]  <TASK>
> [   42.213565][  T164]  __fbnic_pm_attach.isra.0+0x8e/0xa0
> [   42.213725][  T164]  pci_reset_function+0x116/0x1d0
> [   42.213895][  T164]  reset_store+0xa0/0x100
> [   42.214025][  T164]  ? pci_dev_reset_attr_is_visible+0x50/0x50
> [   42.214221][  T164]  ? sysfs_file_kobj+0xc1/0x1e0
> [   42.214374][  T164]  ? sysfs_kf_write+0x65/0x160
> [   42.214526][  T164]  kernfs_fop_write_iter+0x2f8/0x4c0
> [   42.214677][  T164]  ? kernfs_vma_page_mkwrite+0x1f0/0x1f0
> [   42.214836][  T164]  new_sync_write+0x308/0x6f0
> [   42.214987][  T164]  ? __lock_acquire+0x34c/0x740
> [   42.215135][  T164]  ? new_sync_read+0x6f0/0x6f0
> [   42.215288][  T164]  ? lock_acquire.part.0+0xbc/0x260
> [   42.215440][  T164]  ? ksys_write+0xff/0x200
> [   42.215590][  T164]  ? perf_trace_sched_switch+0x6d0/0x6d0
> [   42.215742][  T164]  vfs_write+0x65e/0xbb0
> [   42.215876][  T164]  ksys_write+0xff/0x200
> [   42.215994][  T164]  ? __ia32_sys_read+0xc0/0xc0
> [   42.216141][  T164]  ? do_user_addr_fault+0x269/0x9f0
> [   42.216292][  T164]  ? rcu_is_watching+0x15/0xd0
> [   42.216442][  T164]  do_syscall_64+0xbb/0x360
> [   42.216591][  T164]  entry_SYSCALL_64_after_hwframe+0x4b/0x53
> [   42.216784][  T164] RIP: 0033:0x7f0dc8ea9986
> 
> A bit of digging showed that we were invoking the phylink_resume as a part
> of the fbnic_up path when we were enabling the service task while not
> holding the RTNL lock. We should be enabling this sooner as a part of the
> ndo_open path and then just letting the service task come online later.
> This will help to enforce the correct locking and brings the phylink
> interface online at the same time as the network interface, instead of at a
> later time.
> 
> I tested this on QEMU to verify this was working by putting the system to
> sleep using "echo mem > /sys/power/state" to put the system to sleep in the
> guest and then using the command "system_wakeup" in the QEMU monitor.
> 
> Fixes: 69684376eed5 ("eth: fbnic: Add link detection")
> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>

changes look good,
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

next time please minify dmesg output:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#backtraces-in-commit-messages

> ---
>   drivers/net/ethernet/meta/fbnic/fbnic_netdev.c |    4 ++++
>   drivers/net/ethernet/meta/fbnic/fbnic_pci.c    |    2 --
>   2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> index e67e99487a27..40581550da1a 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_netdev.c
> @@ -52,6 +52,8 @@ int __fbnic_open(struct fbnic_net *fbn)
>   	fbnic_bmc_rpc_init(fbd);
>   	fbnic_rss_reinit(fbd, fbn);
>   
> +	phylink_resume(fbn->phylink);
> +
>   	return 0;
>   time_stop:
>   	fbnic_time_stop(fbn);
> @@ -84,6 +86,8 @@ static int fbnic_stop(struct net_device *netdev)
>   {
>   	struct fbnic_net *fbn = netdev_priv(netdev);
>   
> +	phylink_suspend(fbn->phylink, fbnic_bmc_present(fbn->fbd));
> +
>   	fbnic_down(fbn);
>   	fbnic_pcs_free_irq(fbn->fbd);
>   
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> index a7784deea88f..28e23e3ffca8 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
> @@ -118,14 +118,12 @@ static void fbnic_service_task_start(struct fbnic_net *fbn)
>   	struct fbnic_dev *fbd = fbn->fbd;
>   
>   	schedule_delayed_work(&fbd->service_task, HZ);
> -	phylink_resume(fbn->phylink);
>   }
>   
>   static void fbnic_service_task_stop(struct fbnic_net *fbn)
>   {
>   	struct fbnic_dev *fbd = fbn->fbd;
>   
> -	phylink_suspend(fbn->phylink, fbnic_bmc_present(fbd));
>   	cancel_delayed_work(&fbd->service_task);
>   }
>   
> 
> 
> 

Re: [net PATCH 0/2] Locking fixes for fbnic driver

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 25 Aug 2025 15:56:00 -0700 you wrote:
> Address a few locking issues that were reported on the fbnic driver.
> Specifically in one case we were seeing locking leaks due to us not
> releasing the locks in certain exception paths. In another case we were
> using phylink_resume outside of a section in which we held the RTNL mutex
> and as a result we were throwing an assert.
> 
> 
> [...]

Here is the summary with links:
  - [net,1/2] fbnic: Fixup rtnl_lock and devl_lock handling related to mailbox code
    https://git.kernel.org/netdev/net/c/2ddaa562b465
  - [net,2/2] fbnic: Move phylink resume out of service_task and into open/close
    https://git.kernel.org/netdev/net/c/6ede14a2c636

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html