Re: [net PATCH] net: macb: Relocate mog_init_rings() callback from macb_mac_link_up() to macb_open()

[Kevin Hao <kexin.hao@windriver.com>]

[-- Attachment #1: Type: text/plain, Size: 8063 bytes --]

On Thu, Dec 04, 2025 at 10:25:04AM +0100, Paolo Abeni wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> On 11/28/25 11:36 AM, Xiaolei Wang wrote:
> > In the non-RT kernel, local_bh_disable() merely disables preemption,
> > whereas it maps to an actual spin lock in the RT kernel. Consequently,
> > when attempting to refill RX buffers via netdev_alloc_skb() in
> > macb_mac_link_up(), a deadlock scenario arises as follows:
> >   The dependency chain caused by macb_mac_link_up():
> >   &bp->lock --> (softirq_ctrl.lock) --> _xmit_ETHER#2
> 
> I'm sorry, but I can't see how this dependency chain is caused by
> mog_init_rings(), please extend the above info pin pointing the
> function/code effectively acquiring the lock and how it's reached.

Apologies for the confusion. It appears I made an error in the lock chain description.

The correct lock dependency chain is as follows:

   Chain caused by macb_mac_link_up():
   &bp->lock --> (softirq_ctrl.lock)

   Chain caused by macb_start_xmit():
   (softirq_ctrl.lock) --> _xmit_ETHER#2 --> &bp->lock

Below is the complete log output from lockdep:
   ======================================================
   WARNING: possible circular locking dependency detected
   6.18.0-08691-g2061f18ad76e #39 Not tainted
   ------------------------------------------------------
   kworker/0:0/8 is trying to acquire lock:
   ffff00080369bbe0 (&bp->lock){+.+.}-{3:3}, at: macb_start_xmit+0x808/0xb7c
   
   but task is already holding lock:
   ffff000803698e58 (&queue->tx_ptr_lock){+...}-{3:3}, at: macb_start_xmit+0x148/0xb7c
   
   which lock already depends on the new lock.
   
   
   the existing dependency chain (in reverse order) is:
   
   -> #3 (&queue->tx_ptr_lock){+...}-{3:3}:
          rt_spin_lock+0x50/0x1f0
          macb_start_xmit+0x148/0xb7c
          dev_hard_start_xmit+0x94/0x284
          sch_direct_xmit+0x8c/0x37c
          __dev_queue_xmit+0x708/0x1120
          neigh_resolve_output+0x148/0x28c
          ip6_finish_output2+0x2c0/0xb2c
          __ip6_finish_output+0x114/0x308
          ip6_output+0xc4/0x4a4
          mld_sendpack+0x220/0x68c
          mld_ifc_work+0x2a8/0x4f4
          process_one_work+0x20c/0x5f8
          worker_thread+0x1b0/0x35c
          kthread+0x144/0x200
          ret_from_fork+0x10/0x20
   
   -> #2 (_xmit_ETHER#2){+...}-{3:3}:
          rt_spin_lock+0x50/0x1f0
          sch_direct_xmit+0x11c/0x37c
          __dev_queue_xmit+0x708/0x1120
          neigh_resolve_output+0x148/0x28c
          ip6_finish_output2+0x2c0/0xb2c
          __ip6_finish_output+0x114/0x308
          ip6_output+0xc4/0x4a4
          mld_sendpack+0x220/0x68c
          mld_ifc_work+0x2a8/0x4f4
          process_one_work+0x20c/0x5f8
          worker_thread+0x1b0/0x35c
          kthread+0x144/0x200
          ret_from_fork+0x10/0x20
   
   -> #1 ((softirq_ctrl.lock)){+.+.}-{3:3}:
          lock_release+0x250/0x348
          __local_bh_enable_ip+0x7c/0x240
          __netdev_alloc_skb+0x1b4/0x1d8
          gem_rx_refill+0xdc/0x240
          gem_init_rings+0xb4/0x108
          macb_mac_link_up+0x9c/0x2b4
          phylink_resolve+0x170/0x614
          process_one_work+0x20c/0x5f8
          worker_thread+0x1b0/0x35c
          kthread+0x144/0x200
          ret_from_fork+0x10/0x20
   
   -> #0 (&bp->lock){+.+.}-{3:3}:
          __lock_acquire+0x15a8/0x2084
          lock_acquire+0x1cc/0x350
          rt_spin_lock+0x50/0x1f0
          macb_start_xmit+0x808/0xb7c
          dev_hard_start_xmit+0x94/0x284
          sch_direct_xmit+0x8c/0x37c
          __dev_queue_xmit+0x708/0x1120
          neigh_resolve_output+0x148/0x28c
          ip6_finish_output2+0x2c0/0xb2c
          __ip6_finish_output+0x114/0x308
          ip6_output+0xc4/0x4a4
          mld_sendpack+0x220/0x68c
          mld_ifc_work+0x2a8/0x4f4
          process_one_work+0x20c/0x5f8
          worker_thread+0x1b0/0x35c
          kthread+0x144/0x200
          ret_from_fork+0x10/0x20
   
   other info that might help us debug this:
   
   Chain exists of:
     &bp->lock --> _xmit_ETHER#2 --> &queue->tx_ptr_lock
   
    Possible unsafe locking scenario:
   
          CPU0                    CPU1
          ----                    ----
     lock(&queue->tx_ptr_lock);
                                  lock(_xmit_ETHER#2);
                                  lock(&queue->tx_ptr_lock);
     lock(&bp->lock);
   
    *** DEADLOCK ***
   
   16 locks held by kworker/0:0/8:
    #0: ffff000800905b38 ((wq_completion)mld){+.+.}-{0:0}, at: process_one_work+0x190/0x5f8
    #1: ffff800082943d80 ((work_completion)(&(&idev->mc_ifc_work)->work)){+.+.}-{0:0}, at: process_one_work+0x1b8/0x5f8
    #2: ffff0008036a1620 (&idev->mc_lock){+.+.}-{4:4}, at: mld_ifc_work+0x34/0x4f4
    #3: ffff800081bc0238 (rcu_read_lock){....}-{1:3}, at: mld_sendpack+0x0/0x68c
    #4: ffff800081bc0238 (rcu_read_lock){....}-{1:3}, at: ip6_output+0x44/0x4a4
    #5: ffff800081b7d158 (local_bh){.+.+}-{1:3}, at: __local_bh_disable_ip+0x2c/0x2ac
    #6: ffff00087f6e4588 ((softirq_ctrl.lock)){+.+.}-{3:3}, at: __local_bh_disable_ip+0x1d0/0x2ac
    #7: ffff800081bc0238 (rcu_read_lock){....}-{1:3}, at: rt_spin_lock+0xd4/0x1f0
    #8: ffff800081bc0238 (rcu_read_lock){....}-{1:3}, at: __local_bh_disable_ip+0x1b8/0x2ac
    #9: ffff800081bc0260 (rcu_read_lock_bh){....}-{1:3}, at: __dev_queue_xmit+0x64/0x1120
    #10: ffff000803711328 (dev->qdisc_tx_busylock ?: &qdisc_tx_busylock){+...}-{3:3}, at: __dev_queue_xmit+0x694/0x1120
    #11: ffff800081bc0238 (rcu_read_lock){....}-{1:3}, at: rt_spin_trylock+0x40/0x170
    #12: ffff0008001e3558 (_xmit_ETHER#2){+...}-{3:3}, at: sch_direct_xmit+0x11c/0x37c
    #13: ffff800081bc0238 (rcu_read_lock){....}-{1:3}, at: rt_spin_lock+0xd4/0x1f0
    #14: ffff000803698e58 (&queue->tx_ptr_lock){+...}-{3:3}, at: macb_start_xmit+0x148/0xb7c
    #15: ffff800081bc0238 (rcu_read_lock){....}-{1:3}, at: rt_spin_lock+0xd4/0x1f0
   
   stack backtrace:
   CPU: 0 UID: 0 PID: 8 Comm: kworker/0:0 Not tainted 6.18.0-08691-g2061f18ad76e #39 PREEMPT_RT 
   Hardware name: ZynqMP ZCU102 Rev1.0 (DT)
   Workqueue: mld mld_ifc_work
   Call trace:
    show_stack+0x18/0x24 (C)
    dump_stack_lvl+0xa0/0xf0
    dump_stack+0x18/0x24
    print_circular_bug+0x28c/0x370
    check_noncircular+0x198/0x1ac
    __lock_acquire+0x15a8/0x2084
    lock_acquire+0x1cc/0x350
    rt_spin_lock+0x50/0x1f0
    macb_start_xmit+0x808/0xb7c
    dev_hard_start_xmit+0x94/0x284
    sch_direct_xmit+0x8c/0x37c
    __dev_queue_xmit+0x708/0x1120
    neigh_resolve_output+0x148/0x28c
    ip6_finish_output2+0x2c0/0xb2c
    __ip6_finish_output+0x114/0x308
    ip6_output+0xc4/0x4a4
    mld_sendpack+0x220/0x68c
    mld_ifc_work+0x2a8/0x4f4
    process_one_work+0x20c/0x5f8
    worker_thread+0x1b0/0x35c
    kthread+0x144/0x200
    ret_from_fork+0x10/0x20

> 
> >   The dependency chain caused by macb_start_xmit():
> >   _xmit_ETHER#2 --> &bp->lock
> >
> > Notably, invoking the mog_init_rings() callback upon link establishment
> > is unnecessary. Instead, we can exclusively call mog_init_rings() within
> > the ndo_open() callback. This adjustment resolves the deadlock issue.
> > Given that mog_init_rings() is only applicable to
> > non-MACB_CAPS_MACB_IS_EMAC cases, we can simply move it to macb_open()
> > and simultaneously eliminate the MACB_CAPS_MACB_IS_EMAC check.
> >
> > Suggested-by: Kevin Hao <kexin.hao@windriver.com>
> > Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> 
> Please include a suitable fixes tag.

This issue has been present since commit 633e98a711ac ("net: macb: use resolved
link config in mac_link_up()") introduced the acquisition of bp->lock in
macb_mac_link_up(). Therefore, I will add:
  Fixes: 633e98a711ac ("net: macb: use resolved link config in mac_link_up()")

Thanks,
Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]