Messages in this thread | | | Date | Thu, 24 Feb 2022 10:52:48 +0800 | Subject | Re: [PATCH 3/3] drivers/net/ftgmac100: fix DHCP potential failure with systemd | From | Heyi Guo <> |
| |
Hi Florian,
在 2022/2/24 上午1:55, Florian Fainelli 写道: > > > On 2/23/2022 3:39 AM, Heyi Guo wrote: >> Hi Florian, >> >> 在 2022/2/23 下午1:00, Florian Fainelli 写道: >>> >>> >>> On 2/22/2022 7:14 PM, Heyi Guo wrote: >>>> DHCP failures were observed with systemd 247.6. The issue could be >>>> reproduced by rebooting Aspeed 2600 and then running ifconfig ethX >>>> down/up. >>>> >>>> It is caused by below procedures in the driver: >>>> >>>> 1. ftgmac100_open() enables net interface and call phy_start() >>>> 2. When PHY is link up, it calls netif_carrier_on() and then >>>> adjust_link callback >>>> 3. ftgmac100_adjust_link() will schedule the reset task >>>> 4. ftgmac100_reset_task() will then reset the MAC in another schedule >>>> >>>> After step 2, systemd will be notified to send DHCP discover packet, >>>> while the packet might be corrupted by MAC reset operation in step 4. >>>> >>>> Call ftgmac100_reset() directly instead of scheduling task to fix the >>>> issue. >>>> >>>> Signed-off-by: Heyi Guo <guoheyi@linux.alibaba.com> >>>> --- >>>> Cc: Andrew Lunn <andrew@lunn.ch> >>>> Cc: "David S. Miller" <davem@davemloft.net> >>>> Cc: Jakub Kicinski <kuba@kernel.org> >>>> Cc: Joel Stanley <joel@jms.id.au> >>>> Cc: Guangbin Huang <huangguangbin2@huawei.com> >>>> Cc: Hao Chen <chenhao288@hisilicon.com> >>>> Cc: Arnd Bergmann <arnd@arndb.de> >>>> Cc: Dylan Hung <dylan_hung@aspeedtech.com> >>>> Cc: netdev@vger.kernel.org >>>> >>>> >>>> --- >>>> drivers/net/ethernet/faraday/ftgmac100.c | 13 +++++++++++-- >>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c >>>> b/drivers/net/ethernet/faraday/ftgmac100.c >>>> index c1deb6e5d26c5..d5356db7539a4 100644 >>>> --- a/drivers/net/ethernet/faraday/ftgmac100.c >>>> +++ b/drivers/net/ethernet/faraday/ftgmac100.c >>>> @@ -1402,8 +1402,17 @@ static void ftgmac100_adjust_link(struct >>>> net_device *netdev) >>>> /* Disable all interrupts */ >>>> iowrite32(0, priv->base + FTGMAC100_OFFSET_IER); >>>> - /* Reset the adapter asynchronously */ >>>> - schedule_work(&priv->reset_task); >>>> + /* Release phy lock to allow ftgmac100_reset to aquire it, >>>> keeping lock >>> >>> typo: acquire >>> >> Thanks for the catch :) >>>> + * order consistent to prevent dead lock. >>>> + */ >>>> + if (netdev->phydev) >>>> + mutex_unlock(&netdev->phydev->lock); >>>> + >>>> + ftgmac100_reset(priv); >>>> + >>>> + if (netdev->phydev) >>>> + mutex_lock(&netdev->phydev->lock); >>> >>> Do you really need to perform a full MAC reset whenever the link >>> goes up or down? Instead cannot you just extract the maccr >>> configuration which adjusts the speed and be done with it? >> >> This is the original behavior and not changed in this patch set, and >> I'm not familiar with the hardware design of ftgmac100, so I'd like >> to limit the changes to the code which really causes practical issues. > > This unlocking and re-locking seems superfluous when you could > introduce a version of ftgmac100_reset() which does not acquire the > PHY device mutex, and have that version called from > ftgmac100_adjust_link(). For every other call site, you would acquire > it. Something like this for instance: > > > diff --git a/drivers/net/ethernet/faraday/ftgmac100.c > b/drivers/net/ethernet/faraday/ftgmac100.c > index 691605c15265..98179c3fd9ee 100644 > --- a/drivers/net/ethernet/faraday/ftgmac100.c > +++ b/drivers/net/ethernet/faraday/ftgmac100.c > @@ -1038,7 +1038,7 @@ static void ftgmac100_adjust_link(struct > net_device *netdev) > iowrite32(0, priv->base + FTGMAC100_OFFSET_IER); > > /* Reset the adapter asynchronously */ > - schedule_work(&priv->reset_task); > + ftgmac100_reset(priv, false); > } > > static int ftgmac100_mii_probe(struct net_device *netdev) > @@ -1410,10 +1410,8 @@ static int ftgmac100_init_all(struct ftgmac100 > *priv, bool ignore_alloc_err) > return err; > } > > -static void ftgmac100_reset_task(struct work_struct *work) > +static void ftgmac100_reset_task(struct ftgmac100_priv *priv, bool > lock_phy) > { > - struct ftgmac100 *priv = container_of(work, struct ftgmac100, > - reset_task); > struct net_device *netdev = priv->netdev; > int err; > > @@ -1421,7 +1419,7 @@ static void ftgmac100_reset_task(struct > work_struct *work) > > /* Lock the world */ > rtnl_lock(); > - if (netdev->phydev) > + if (netdev->phydev && lock_phy) > mutex_lock(&netdev->phydev->lock); > if (priv->mii_bus) > mutex_lock(&priv->mii_bus->mdio_lock); > @@ -1454,11 +1452,19 @@ static void ftgmac100_reset_task(struct > work_struct *work) > bail: > if (priv->mii_bus) > mutex_unlock(&priv->mii_bus->mdio_lock); > - if (netdev->phydev) > + if (netdev->phydev && lock_phy) > mutex_unlock(&netdev->phydev->lock); > rtnl_unlock(); > }
This is also what we supposed to do at first, however it will introduce a potential dead lock for different locks acquiring order, and CONFIG_PROVE_LOCKING will complain about it:
[ 16.852199] ====================================================== [ 16.859102] WARNING: possible circular locking dependency detected [ 16.866012] 5.10.36-60b3c9d-dirty-15f4fba #1 Not tainted [ 16.871976] ------------------------------------------------------ [ 16.871991] kworker/1:1/23 is trying to acquire lock: [ 16.872000] 80fa0920 (rtnl_mutex){+.+.}-{3:3}, at: rtnl_lock+0x24/0x28 [ 16.872047] [ 16.872047] but task is already holding lock: [ 16.872051] 821d44c0 (&dev->lock){+.+.}-{3:3}, at: phy_state_machine+0x50/0x290 [ 16.872076] [ 16.872076] which lock already depends on the new lock. [ 16.872076] [ 16.872080] [ 16.872080] the existing dependency chain (in reverse order) is: [ 16.872083] [ 16.872083] -> #1 (&dev->lock){+.+.}-{3:3}: [ 16.872106] lock_acquire+0x6c/0x74 [ 16.872117] __mutex_lock+0xb4/0xa48 [ 16.872132] mutex_lock_nested+0x2c/0x34 [ 16.872141] phy_start+0x30/0xc4 [ 16.872155] ftgmac100_open+0x1a0/0x254 [ 16.872168] __dev_open+0x110/0x1d0 [ 16.872180] __dev_change_flags+0x1d0/0x258 [ 16.872192] dev_change_flags+0x28/0x58 [ 16.872204] do_setlink+0x258/0xc60 [ 16.872212] rtnl_setlink+0x110/0x18c [ 16.872219] rtnetlink_rcv_msg+0x1d0/0x53c [ 16.872226] netlink_rcv_skb+0xd0/0x128 [ 16.872233] rtnetlink_rcv+0x20/0x24 [ 16.872244] netlink_unicast+0x1a8/0x26c [ 16.872254] netlink_sendmsg+0x220/0x464 [ 16.872265] __sys_sendto+0xe4/0x134 [ 16.872276] sys_sendto+0x24/0x2c [ 16.872288] ret_fast_syscall+0x0/0x28 [ 16.872297] 0x7ed9e928 [ 16.872301] [ 16.872301] -> #0 (rtnl_mutex){+.+.}-{3:3}: [ 16.872325] __lock_acquire+0x17e8/0x3268 [ 16.872331] lock_acquire.part.0+0xcc/0x394 [ 16.872341] lock_acquire+0x6c/0x74 [ 16.872354] __mutex_lock+0xb4/0xa48 [ 16.872365] mutex_lock_nested+0x2c/0x34 [ 16.872377] rtnl_lock+0x24/0x28 [ 16.872389] ftgmac100_adjust_link+0xc0/0x144 [ 16.872401] phy_link_change+0x38/0x64 [ 16.872411] phy_check_link_status+0xa8/0xfc [ 16.872422] phy_state_machine+0x80/0x290 [ 16.872435] process_one_work+0x294/0x7d8 [ 16.872447] worker_thread+0x6c/0x548 [ 16.872456] kthread+0x170/0x178 [ 16.872462] ret_from_fork+0x14/0x20 [ 16.872467] 0x0 [ 16.872471] [ 16.872471] other info that might help us debug this: [ 16.872471] [ 16.872475] Possible unsafe locking scenario: [ 16.872475] [ 16.872478] CPU0 CPU1 [ 16.872482] ---- ---- [ 16.872485] lock(&dev->lock); [ 16.872495] lock(rtnl_mutex); [ 16.872505] lock(&dev->lock); [ 16.872513] lock(rtnl_mutex); [ 16.872522] [ 16.872522] *** DEADLOCK *** [ 16.872522] [ 16.872528] 3 locks held by kworker/1:1/23: [ 16.872532] #0: 818472a8 ((wq_completion)events_power_efficient){+.+.}-{0:0}, at: process_one_work+0x1e8/0x7d8 [ 16.872558] #1: 819fbef8 ((work_completion)(&(&dev->state_queue)->work)){+.+.}-{0:0}, at: process_one_work+0x1e8/0x7d8 [ 16.872582] #2: 821d44c0 (&dev->lock){+.+.}-{3:3}, at: phy_state_machine+0x50/0x290
Thanks,
Heyi
> > +static void ftgmac100_reset_task(struct work_struct *work) > +{ > + struct ftgmac100 *priv = container_of(work, struct ftgmac100, > + reset_task); > + > + ftgmac100_reset(priv, true); > +} > + > static int ftgmac100_open(struct net_device *netdev) > { > struct ftgmac100 *priv = netdev_priv(netdev)
| |