[PATCH net] net: davicom: dm9000: dm9000_phy_write(): fix deadlock during netdev watchdog handling
The dm9000 takes the db->lock spin lock in dm9000_timeout() and calls into dm9000_init_dm9000(). For the DM9000B the PHY is reset with dm9000_phy_write(). That function again takes the db->lock spin lock, which results in a deadlock. For reference the backtrace:
| [<c0425050>] (rt_spin_lock_slowlock_locked) from [<c0425100>] (rt_spin_lock_slowlock+0x60/0xc4) | [<c0425100>] (rt_spin_lock_slowlock) from [<c02e1174>] (dm9000_phy_write+0x2c/0x1a4) | [<c02e1174>] (dm9000_phy_write) from [<c02e16b0>] (dm9000_init_dm9000+0x288/0x2a4) | [<c02e16b0>] (dm9000_init_dm9000) from [<c02e1724>] (dm9000_timeout+0x58/0xd4) | [<c02e1724>] (dm9000_timeout) from [<c036f298>] (dev_watchdog+0x258/0x2a8) | [<c036f298>] (dev_watchdog) from [<c0068168>] (call_timer_fn+0x20/0x88) | [<c0068168>] (call_timer_fn) from [<c00687c8>] (expire_timers+0xf0/0x194) | [<c00687c8>] (expire_timers) from [<c0068920>] (run_timer_softirq+0xb4/0x25c) | [<c0068920>] (run_timer_softirq) from [<c0021a30>] (do_current_softirqs+0x16c/0x228) | [<c0021a30>] (do_current_softirqs) from [<c0021b14>] (run_ksoftirqd+0x28/0x4c) | [<c0021b14>] (run_ksoftirqd) from [<c0040488>] (smpboot_thread_fn+0x278/0x290) | [<c0040488>] (smpboot_thread_fn) from [<c003c28c>] (kthread+0x124/0x164) | [<c003c28c>] (kthread) from [<c00090f0>] (ret_from_fork+0x14/0x24)
To workaround similar problem (take mutex inside spin lock ) , a "in_timeout" variable was added in 582379839bbd ("dm9000: avoid sleeping in dm9000_timeout callback"). Use this variable and not take the spin lock inside dm9000_phy_write() if in_timeout is true.
Cc: stable@vger.kernel.org Signed-off-by: Marc Kleine-Budde mkl@pengutronix.de --- During the netdev watchdog handling the dm9000 driver takes the same spin lock twice. Avoid this by extending an existing workaround. --- drivers/net/ethernet/davicom/dm9000.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c index 05a89ab6766c..3a056a54aaf9 100644 --- a/drivers/net/ethernet/davicom/dm9000.c +++ b/drivers/net/ethernet/davicom/dm9000.c @@ -325,10 +325,10 @@ dm9000_phy_write(struct net_device *dev, unsigned long reg_save;
dm9000_dbg(db, 5, "phy_write[%02x] = %04x\n", reg, value); - if (!db->in_timeout) + if (!db->in_timeout) { mutex_lock(&db->addr_lock); - - spin_lock_irqsave(&db->lock, flags); + spin_lock_irqsave(&db->lock, flags); + }
/* Save previous register address */ reg_save = readb(db->io_addr); @@ -344,11 +344,13 @@ dm9000_phy_write(struct net_device *dev, iow(db, DM9000_EPCR, EPCR_EPOS | EPCR_ERPRW);
writeb(reg_save, db->io_addr); - spin_unlock_irqrestore(&db->lock, flags); + if (!db->in_timeout) + spin_unlock_irqrestore(&db->lock, flags);
dm9000_msleep(db, 1); /* Wait write complete */
- spin_lock_irqsave(&db->lock, flags); + if (!db->in_timeout) + spin_lock_irqsave(&db->lock, flags); reg_save = readb(db->io_addr);
iow(db, DM9000_EPCR, 0x0); /* Clear phyxcer write command */ @@ -356,9 +358,10 @@ dm9000_phy_write(struct net_device *dev, /* restore the previous address */ writeb(reg_save, db->io_addr);
- spin_unlock_irqrestore(&db->lock, flags); - if (!db->in_timeout) + if (!db->in_timeout) { + spin_unlock_irqrestore(&db->lock, flags); mutex_unlock(&db->addr_lock); + } }
/* dm9000_set_io
--- base-commit: 776fe19953b0e0af00399e50fb3b205101d4b3c1 change-id: 20231010-dm9000-fix-deadlock-ffb1e5295ee1
Best regards,
On 10.10.2023 09:35:19, Marc Kleine-Budde wrote:
The dm9000 takes the db->lock spin lock in dm9000_timeout() and calls into dm9000_init_dm9000(). For the DM9000B the PHY is reset with dm9000_phy_write(). That function again takes the db->lock spin lock, which results in a deadlock. For reference the backtrace:
| [<c0425050>] (rt_spin_lock_slowlock_locked) from [<c0425100>] (rt_spin_lock_slowlock+0x60/0xc4) | [<c0425100>] (rt_spin_lock_slowlock) from [<c02e1174>] (dm9000_phy_write+0x2c/0x1a4) | [<c02e1174>] (dm9000_phy_write) from [<c02e16b0>] (dm9000_init_dm9000+0x288/0x2a4) | [<c02e16b0>] (dm9000_init_dm9000) from [<c02e1724>] (dm9000_timeout+0x58/0xd4) | [<c02e1724>] (dm9000_timeout) from [<c036f298>] (dev_watchdog+0x258/0x2a8) | [<c036f298>] (dev_watchdog) from [<c0068168>] (call_timer_fn+0x20/0x88) | [<c0068168>] (call_timer_fn) from [<c00687c8>] (expire_timers+0xf0/0x194) | [<c00687c8>] (expire_timers) from [<c0068920>] (run_timer_softirq+0xb4/0x25c) | [<c0068920>] (run_timer_softirq) from [<c0021a30>] (do_current_softirqs+0x16c/0x228) | [<c0021a30>] (do_current_softirqs) from [<c0021b14>] (run_ksoftirqd+0x28/0x4c) | [<c0021b14>] (run_ksoftirqd) from [<c0040488>] (smpboot_thread_fn+0x278/0x290) | [<c0040488>] (smpboot_thread_fn) from [<c003c28c>] (kthread+0x124/0x164) | [<c003c28c>] (kthread) from [<c00090f0>] (ret_from_fork+0x14/0x24)
To workaround similar problem (take mutex inside spin lock ) , a "in_timeout" variable was added in 582379839bbd ("dm9000: avoid sleeping in dm9000_timeout callback"). Use this variable and not take the spin lock inside dm9000_phy_write() if in_timeout is true.
Cc: stable@vger.kernel.org Signed-off-by: Marc Kleine-Budde mkl@pengutronix.de
Fixes: a1365275e745 ("[PATCH] DM9000 network driver")
regards, Marc
Marc Kleine-Budde mkl@pengutronix.de :
The dm9000 takes the db->lock spin lock in dm9000_timeout() and calls into dm9000_init_dm9000(). For the DM9000B the PHY is reset with dm9000_phy_write(). That function again takes the db->lock spin lock, which results in a deadlock. For reference the backtrace:
[...]
To workaround similar problem (take mutex inside spin lock ) , a "in_timeout" variable was added in 582379839bbd ("dm9000: avoid sleeping in dm9000_timeout callback"). Use this variable and not take the spin lock inside dm9000_phy_write() if in_timeout is true.
Cc: stable@vger.kernel.org Signed-off-by: Marc Kleine-Budde mkl@pengutronix.de
During the netdev watchdog handling the dm9000 driver takes the same spin lock twice. Avoid this by extending an existing workaround.
I can review it but I can't really endorse it. :o)
Extending ugly workaround in pre-2000 style device drivers... I'd rather see the thing fixed if there is some real use for it.
On 11.10.2023 00:21:31, Francois Romieu wrote:
Marc Kleine-Budde mkl@pengutronix.de :
The dm9000 takes the db->lock spin lock in dm9000_timeout() and calls into dm9000_init_dm9000(). For the DM9000B the PHY is reset with dm9000_phy_write(). That function again takes the db->lock spin lock, which results in a deadlock. For reference the backtrace:
[...]
To workaround similar problem (take mutex inside spin lock ) , a "in_timeout" variable was added in 582379839bbd ("dm9000: avoid sleeping in dm9000_timeout callback"). Use this variable and not take the spin lock inside dm9000_phy_write() if in_timeout is true.
Cc: stable@vger.kernel.org Signed-off-by: Marc Kleine-Budde mkl@pengutronix.de
During the netdev watchdog handling the dm9000 driver takes the same spin lock twice. Avoid this by extending an existing workaround.
I can review it but I can't really endorse it. :o)
Extending ugly workaround in pre-2000 style device drivers... I'd rather see the thing fixed if there is some real use for it.
There definitely are still users of this drivers on modern kernels out there.
I too don't like the feeling of wrapping more and more duct tape around existing drivers. How about moving the functionality to dm9000_phy_write_locked() and leave the locking in dm9000_phy_write(). I will prepare a patch.
regards, Marc
On Wed, 2023-10-11 at 08:43 +0200, Marc Kleine-Budde wrote:
On 11.10.2023 00:21:31, Francois Romieu wrote:
Marc Kleine-Budde mkl@pengutronix.de :
The dm9000 takes the db->lock spin lock in dm9000_timeout() and calls into dm9000_init_dm9000(). For the DM9000B the PHY is reset with dm9000_phy_write(). That function again takes the db->lock spin lock, which results in a deadlock. For reference the backtrace:
[...]
To workaround similar problem (take mutex inside spin lock ) , a "in_timeout" variable was added in 582379839bbd ("dm9000: avoid sleeping in dm9000_timeout callback"). Use this variable and not take the spin lock inside dm9000_phy_write() if in_timeout is true.
Cc: stable@vger.kernel.org Signed-off-by: Marc Kleine-Budde mkl@pengutronix.de
During the netdev watchdog handling the dm9000 driver takes the same spin lock twice. Avoid this by extending an existing workaround.
I can review it but I can't really endorse it. :o)
Extending ugly workaround in pre-2000 style device drivers... I'd rather see the thing fixed if there is some real use for it.
There definitely are still users of this drivers on modern kernels out there.
I too don't like the feeling of wrapping more and more duct tape around existing drivers. How about moving the functionality to dm9000_phy_write_locked() and leave the locking in dm9000_phy_write(). I will prepare a patch.
If you have the H/W handy to try some more invasive change, I'm wondering if you could schedule a work from dm9000_timeout() and there acquire all the needed locks.
Cheers,
Paolo
linux-stable-mirror@lists.linaro.org
-
Francois Romieu -
Marc Kleine-Budde -
Paolo Abeni