[PATCH net] net: dm9000: Fix a recursive lockup in the TX timeout path

[PATCH net] net: dm9000: Fix a recursive lockup in the TX timeout path

[Ginger Li]

dm9000_timeout() takes db->lock with interrupts disabled and then calls
dm9000_init_dm9000(), which expects the caller to hold that lock and accesses
the chip registers directly.  For DM9000B devices dm9000_init_dm9000() also
calls dm9000_phy_write(), and that function takes db->lock again, so the TX
timeout path deadlocks on the same CPU.

dm9000_phy_write() already skips db->addr_lock when db->in_timeout is set,
because in that case the caller holds db->lock and sleeping is not allowed
either.  Skip db->lock as well then, so that the nested call from
dm9000_init_dm9000() works as intended.

Fixes: 6741f40 ("DM9000B: driver initialization upgrade")
Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
---
 drivers/net/ethernet/davicom/dm9000.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
--- 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,14 @@ 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 +359,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
-- 
2.43.0

Re: [PATCH net] net: dm9000: Fix a recursive lockup in the TX timeout path

[Markus Elfring]

…
> Fixes: 6741f40 ("DM9000B: driver initialization upgrade")

A longer hash would be preferred for such a tag.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v7.3-rc4#n157

Regards,
Markus

Re: [PATCH net] net: dm9000: Fix a recursive lockup in the TX timeout path

[Andrew Lunn]

On Wed, Sep 23, 2026 at 04:40:55PM +0800, Ginger Li wrote:
> dm9000_timeout() takes db->lock with interrupts disabled and then calls
> dm9000_init_dm9000(), which expects the caller to hold that lock and accesses
> the chip registers directly.  For DM9000B devices dm9000_init_dm9000() also
> calls dm9000_phy_write(), and that function takes db->lock again, so the TX
> timeout path deadlocks on the same CPU.
> 
> dm9000_phy_write() already skips db->addr_lock when db->in_timeout is set,
> because in that case the caller holds db->lock and sleeping is not allowed
> either.  Skip db->lock as well then, so that the nested call from
> dm9000_init_dm9000() works as intended.
> 
> Fixes: 6741f40 ("DM9000B: driver initialization upgrade")
> Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
> ---
>  drivers/net/ethernet/davicom/dm9000.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
> --- 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);
> +	}

This is ugly.

Is it possible to pull the locking out of dm9000_phy_write() to give a
version which tests the lock is taken using lockdep_assert_held(). Put
a wrapper around it which takes the lock for the normal case.

I would also take a look at dm9000_phy_read() and understand why its
locking is different.

Ideally you want to remove db->in_timeout, and make sure locked or
unlocked functions are called as needed. I assume you have the
hardware, and can trigger a timeout? So you can do a bigger refactor
like this?

     Andrew

Re: [PATCH net] net: dm9000: Fix a recursive lockup in the TX timeout path

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] In drivers/net/ethernet/davicom/dm9000.c, dm9000_phy_write() now
  decides whether to take and release both db->addr_lock and db->lock by…
- [Medium] in_timeout, in_suspend and wake_supported are adjacent 1-bit
  bitfields in the same unsigned int of struct board_info (lines…

--

net: dm9000: Fix a recursive lockup in the TX timeout path

This commit fixes a deadlock on DM9000B. There, dm9000_timeout() holds
db->lock while dm9000_init_dm9000() calls dm9000_phy_write(), which takes
db->lock again. dm9000_phy_write() already skipped db->addr_lock when
db->in_timeout was set, and it now skips db->lock in that case too.

> diff --git a/drivers/net/ethernet/davicom/dm9000.c b/drivers/net/ethernet/davicom/dm9000.c
> index b87eaf0c250ce..328043056ec48 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,14 @@ 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);

[Severity: High]
Can in_timeout change between these checks when dm9000_phy_write() is
called from somewhere other than dm9000_timeout()?

in_timeout is a device-wide flag. dm9000_timeout() sets and clears it, and
it runs from the dev_watchdog timer with only tx_global_lock held.

dm9000_phy_write() is also the mii.mdio_write callback. It is reached under
rtnl through these paths:

  dm9000_nway_reset()->mii_nway_restart()
  dm9000_set_link_ksettings()->mii_ethtool_set_link_ksettings()
  dm9000_ioctl()->generic_mii_ioctl() (SIOCSMIIREG)

None of these paths are serialized against the watchdog. So
in_timeout == 1 only means that some CPU is inside dm9000_timeout(). It
does not mean that this caller holds db->lock.

The flag is now read four times without a lock, and each read decides
whether a lock is taken or released. Before this patch the relock after
dm9000_msleep() was unconditional. A caller would spin until
dm9000_timeout() finished, so the final check always ran under db->lock,
where in_timeout can only be 0.

Consider this sequence on DM9000B:

CPU0 (ethtool -r)                  CPU1 (dev_watchdog)
dm9000_phy_write()
  in_timeout == 0
  mutex_lock(&db->addr_lock)
  spin_lock_irqsave(&db->lock)
  iow(db, DM9000_EPCR, ...)
  spin_unlock_irqrestore(&db->lock)
  dm9000_msleep(db, 1)
                                   dm9000_timeout()
                                     spin_lock_irqsave(&db->lock)
                                     db->in_timeout = 1
                                     dm9000_init_dm9000()
                                       dm9000_phy_write() x2
                                         mdelay(1) each
  in_timeout == 1, relock skipped
  readb(db->io_addr), iow() ...    iow() ...
  in_timeout == 1, unlocks skipped

CPU0's index/data accesses then interleave with CPU1's iow() pairs, and
db->addr_lock is never released. Would later phy_read, EEPROM and
ethtool/MII callers then hang, often with rtnl held?

If CPU1 clears the flag before CPU0 reaches the final check, CPU0 instead
calls spin_unlock_irqrestore() on a db->lock it doesn't hold.

The opposite case also seems possible. If CPU0 enters while
in_timeout == 1, it takes neither lock and flags is never initialized. If
the flag then drops to 0 before the first unlock check, CPU0 runs
spin_unlock_irqrestore(&db->lock, flags) with garbage flags. Later it runs
mutex_unlock() on an addr_lock it never took.

Would it be more robust to stop inferring lock ownership from in_timeout?
For example, the timeout path in dm9000_init_dm9000() could call an
unlocked __dm9000_phy_write() helper (or pass a locked argument), and
normal MDIO callers would always take both locks.

> +
>  	reg_save = readb(db->io_addr);
>  
>  	iow(db, DM9000_EPCR, 0x0);	/* Clear phyxcer write command */
> @@ -356,9 +359,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);
> +	}
>  }

[Severity: Medium]
With this change, in_timeout effectively means "the caller already holds
db->lock". Could a stale in_timeout value leave every later
dm9000_phy_write() running without db->lock or addr_lock?

in_timeout shares a word with in_suspend and wake_supported in struct
board_info:

	unsigned int	in_timeout:1;
	unsigned int	in_suspend:1;
	unsigned int	wake_supported:1;

dm9000_timeout() writes in_timeout under db->lock. dm9000_drv_suspend() and
dm9000_drv_resume() write in_suspend without any lock.

In dm9000_drv_suspend() the store happens before netif_device_detach(), so
the watchdog can still be running dm9000_timeout() on another CPU:

	db->in_suspend = 1;

	if (!netif_running(ndev))
		return 0;

	netif_device_detach(ndev);

dm9000_drv_resume() likewise clears in_suspend after netif_device_attach()
has already re-enabled the watchdog.

Suppose the suspend read-modify-write loads the word while in_timeout is 1,
and stores it back after dm9000_timeout() has cleared the flag. in_timeout
then stays at 1 until the next TX timeout.

The lost update itself predates this patch, but before this patch it only
caused addr_lock to be skipped. Now these later dm9000_phy_write() callers
would skip both locks:

  resume, via dm9000_init_dm9000() on DM9000B
  ethtool and MII ioctls
  dm9000_shutdown()

Their index/data register accesses would then race with the xmit and IRQ
paths.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923084055.65091-1-ginger.jzllee%40gmail.com