The hip04 remove path frees the TX/RX rings before unregistering the netdev. If the interface is still up, unregister_netdev() then runs .ndo_stop, whose TX reclaim and NAPI poll touch the already-freed DMA ring memory. The TX coalesce timer and the platform IRQ also outlive the netdev private data they dereference. Reorder hip04_remove() so the netdev is unregistered (which runs .ndo_stop synchronously, stopping NAPI and the TX queue) before the rings are freed. Free the devm-managed IRQ explicitly before free_netdev(), so hip04_mac_interrupt() (whose dev_id is the netdev) cannot fire against freed memory: devm would otherwise release it only after .remove returns. hip04_mac_stop() must quiesce both arming sites of the coalesce timer. The NAPI poll arms it, and napi_disable() returns once the poll calls napi_complete_done(), not when the poll function returns, so move that arm before napi_complete_done(). The early-exit paths in hip04_rx_poll() do not call napi_complete_done(); NAPI stays scheduled and will be polled again, so there is no need to arm the coalesce timer on that pass. The Tx path also arms it, and mac_stop() is reached directly from hip04_tx_timeout_task() as well as via .ndo_stop, so use netif_tx_disable() rather than netif_stop_queue() to wait for an in-flight hip04_mac_start_xmit() to finish. The timer is then drained with hrtimer_cancel(). A "closing" flag, checked at the single arming site, guards against a later arm. hip04_tx_timeout_task() restarts the device with mac_stop() + mac_open(). Serialize that restart against .ndo_open/.ndo_stop with a driver-private state_lock instead of rtnl_lock(): mac_open()/mac_stop() are split into internal __hip04_mac_open()/__hip04_mac_stop() helpers, and the .ndo_open/.ndo_stop wrappers as well as the timeout worker take the lock. This avoids extending RTNL usage; netdev_lock() is not sufficient here, as hip04 does not opt into netdev-ops locking. If timeout recovery is in progress when a close begins, .ndo_stop waits on the lock and performs the final MAC stop; if .ndo_stop acquires the lock first, the worker sees the device is no longer running and returns. Either way, recovery cannot re-enable the MAC during teardown. This issue was found by an in-house static analysis tool. Fixes: a41ea46a9a12 ("net: hisilicon: new hip04 ethernet driver") Cc: stable@vger.kernel.org Assisted-by: Codex:gpt-5.5 Signed-off-by: Fan Wu --- v3: address review from Przemek Kitszel. - Drop the rtnl_lock()/rtnl_unlock() and the #include added to hip04_tx_timeout_task() in v2. Serialize the tx-timeout restart against .ndo_open/.ndo_stop with a driver-private state_lock instead: mac_open()/mac_stop() are split into __hip04_mac_open()/__hip04_mac_stop() helpers, and the .ndo_open/.ndo_stop wrappers and the timeout worker all take the lock. This avoids new RTNL usage. netdev_lock() was considered but does not help: hip04 does not opt into netdev-ops locking, so its .ndo_open/.ndo_stop run under RTNL and the netdev instance lock does not protect them. - Rephrase the sentence about napi_complete_done() / early-exit paths that was hard to read. - s/Tx xmit/Tx/. v2: Address review comments from Simon Horman. Use netif_tx_disable() rather than netif_stop_queue() in hip04_mac_stop() so that an in-flight hip04_mac_start_xmit() finishes before the TX ring is reclaimed; mac_stop() is also called directly from the tx-timeout work, not only via .ndo_stop. Arm the coalesce timer in hip04_rx_poll() before napi_complete_done(), so that napi_disable() observes the final arm and the subsequent cancel cannot miss it. Free the devm-managed IRQ explicitly in hip04_remove() before free_netdev() so that hip04_mac_interrupt() cannot run against freed memory, placing it after unregister_netdev() so that the device is stopped first. Reorder hip04_remove() so that the netdev is unregistered (running .ndo_stop) before the PHY is disconnected and the TX/RX rings are freed. Check the return value of hip04_mac_open() in the tx-timeout restart path. v2: https://lore.kernel.org/netdev/20260710015730.630775-1-fanwu01@zju.edu.cn/ v1: https://lore.kernel.org/netdev/20260703050133.2445155-1-fanwu01@zju.edu.cn/ drivers/net/ethernet/hisilicon/hip04_eth.c | 107 ++++++++++++++++++++++++++---- 1 file changed, 96 insertions(+), 11 deletions(-) diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c index 18376bcc7..4bd192094 100644 --- a/drivers/net/ethernet/hisilicon/hip04_eth.c +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c @@ -15,6 +15,7 @@ #include #include #include +#include #define SC_PPE_RESET_DREQ 0x026C @@ -232,6 +233,8 @@ struct hip04_priv { int tx_coalesce_frames; int tx_coalesce_usecs; struct hrtimer tx_coalesce_timer; + bool closing; + struct mutex state_lock; /* Serializes MAC open/stop and timeout recovery. */ unsigned char *rx_buf[RX_DESC_NUM]; dma_addr_t rx_phys[RX_DESC_NUM]; @@ -497,6 +500,12 @@ static void hip04_start_tx_timer(struct hip04_priv *priv) { unsigned long ns = priv->tx_coalesce_usecs * NSEC_PER_USEC / 2; + /* Do not (re-)arm the Tx coalesce timer once teardown has begun. + * Both arming sites (Tx and NAPI Rx poll) go through here. + */ + if (smp_load_acquire(&priv->closing)) + return; + /* allow timer to fire after half the time at the earliest */ hrtimer_start_range_ns(&priv->tx_coalesce_timer, ns_to_ktime(ns), ns, HRTIMER_MODE_REL); @@ -649,12 +658,15 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget) priv->reg_inten |= RCV_INT; writel_relaxed(priv->reg_inten, priv->base + PPE_INTEN); } + /* Arm the coalesce timer BEFORE napi_complete_done(): napi_disable() + * in hip04_mac_stop() returns once SCHED is cleared here, not when + * the poll function returns, so arming afterwards can slip past the + * stop path's hrtimer_cancel(). + */ + if (tx_remaining) + hip04_start_tx_timer(priv); napi_complete_done(napi, rx); done: - /* start a new timer if necessary */ - if (rx < budget && tx_remaining) - hip04_start_tx_timer(priv); - return rx; } @@ -720,7 +732,7 @@ static void hip04_adjust_link(struct net_device *ndev) } } -static int hip04_mac_open(struct net_device *ndev) +static int __hip04_mac_open(struct net_device *ndev) { struct hip04_priv *priv = netdev_priv(ndev); int i; @@ -743,6 +755,13 @@ static int hip04_mac_open(struct net_device *ndev) hip04_set_recv_desc(priv, phys); } + /* RX mappings are established; clear the closing flag before + * re-enabling traffic. The store-release pairs with the load-acquire + * in hip04_start_tx_timer(); state_lock serializes this against + * mac_stop()'s store-release. + */ + smp_store_release(&priv->closing, false); + if (priv->phy) phy_start(priv->phy); @@ -754,13 +773,42 @@ static int hip04_mac_open(struct net_device *ndev) return 0; } -static int hip04_mac_stop(struct net_device *ndev) +static int hip04_mac_open(struct net_device *ndev) +{ + struct hip04_priv *priv = netdev_priv(ndev); + int ret; + + mutex_lock(&priv->state_lock); + ret = __hip04_mac_open(ndev); + mutex_unlock(&priv->state_lock); + return ret; +} + +static int __hip04_mac_stop(struct net_device *ndev) { struct hip04_priv *priv = netdev_priv(ndev); int i; + /* Stop new timer arms before draining: set the closing flag (checked + * at the single arming site), wait for the NAPI poll and any in-flight + * TX to finish, then cancel the timer. + * + * netif_tx_disable() (not netif_stop_queue()) is required because this + * function is also called directly from hip04_tx_timeout_task(), not + * only via .ndo_stop where the core has already deactivated TX; + * netif_tx_disable() waits for an in-flight hip04_mac_start_xmit(), + * which arms the timer, to finish. + * + * Because hip04_rx_poll() arms the timer before napi_complete_done(), + * napi_disable() returning means that arm has happened, so the + * hrtimer_cancel() below cannot miss it. The store-release pairs + * with the load in hip04_start_tx_timer(). + */ + smp_store_release(&priv->closing, true); + napi_disable(&priv->napi); - netif_stop_queue(ndev); + netif_tx_disable(ndev); + hrtimer_cancel(&priv->tx_coalesce_timer); hip04_mac_disable(ndev); hip04_tx_reclaim(ndev, true); hip04_reset_ppe(priv); @@ -779,6 +827,16 @@ static int hip04_mac_stop(struct net_device *ndev) return 0; } +static int hip04_mac_stop(struct net_device *ndev) +{ + struct hip04_priv *priv = netdev_priv(ndev); + + mutex_lock(&priv->state_lock); + __hip04_mac_stop(ndev); + mutex_unlock(&priv->state_lock); + return 0; +} + static void hip04_timeout(struct net_device *ndev, unsigned int txqueue) { struct hip04_priv *priv = netdev_priv(ndev); @@ -791,8 +849,23 @@ static void hip04_tx_timeout_task(struct work_struct *work) struct hip04_priv *priv; priv = container_of(work, struct hip04_priv, tx_timeout_task); - hip04_mac_stop(priv->ndev); - hip04_mac_open(priv->ndev); + + /* Serialize the restart with .ndo_open/.ndo_stop, which take the + * same lock. If a close has completed or is in progress the + * netif_running() check bails; if that check races the core's + * running-state clear, .ndo_stop still runs under state_lock + * afterwards and stops the device, so this worker cannot leave the + * MAC re-enabled against a teardown. + */ + mutex_lock(&priv->state_lock); + if (!netif_running(priv->ndev)) + goto out; + + __hip04_mac_stop(priv->ndev); + if (__hip04_mac_open(priv->ndev)) + netdev_err(priv->ndev, "restart after tx timeout failed\n"); +out: + mutex_unlock(&priv->state_lock); } static int hip04_get_coalesce(struct net_device *netdev, @@ -983,6 +1056,7 @@ static int hip04_mac_probe(struct platform_device *pdev) } INIT_WORK(&priv->tx_timeout_task, hip04_tx_timeout_task); + mutex_init(&priv->state_lock); ndev->netdev_ops = &hip04_netdev_ops; ndev->ethtool_ops = &hip04_ethtool_ops; @@ -1026,13 +1100,24 @@ static void hip04_remove(struct platform_device *pdev) struct hip04_priv *priv = netdev_priv(ndev); struct device *d = &pdev->dev; + unregister_netdev(ndev); + + /* The IRQ is devm-managed and would otherwise be freed only after + * this function returns. Free it now, after unregister_netdev() has + * run .ndo_stop to stop the device and mask its interrupt source, but + * before the manual free_netdev() below, so that hip04_mac_interrupt() + * (dev_id == ndev) cannot fire against freed memory. free_irq() also + * drains any in-flight handler. + */ + devm_free_irq(d, ndev->irq, ndev); + cancel_work_sync(&priv->tx_timeout_task); + hrtimer_cancel(&priv->tx_coalesce_timer); + if (priv->phy) phy_disconnect(priv->phy); hip04_free_ring(ndev, d); - unregister_netdev(ndev); of_node_put(priv->phy_node); - cancel_work_sync(&priv->tx_timeout_task); free_netdev(ndev); }