A resume that fails in bcmgenet_init_dma() returns with the interface still IFF_UP and the datapath gone. The next ifdown runs the teardown a second time and napi_disable() waits for a NAPI_STATE_SCHED bit that nothing will clear: task:ip state:D usleep_range_state+0xec/0x158 napi_disable_locked+0x254/0x318 napi_disable+0x3c/0x68 bcmgenet_netif_stop+0x1c8/0x318 bcmgenet_close+0x24/0xd0 __dev_close_many+0x18c/0x310 RTNL is held across the wait, so everything else blocks behind it. The same resume gates the clock and detaches the device. Close releases the clock a second time and ifup fails with -ENODEV. The hardware filters stay armed, so the next WoL suspend wakes on any frame. That suspend sets CMD_RX_EN for magic packet detection, so the WoL resume reuses rings that are gone. A failed resume should leave a state the ordinary paths can handle. Clear rx_cbs and tx_cbs as they are freed so they answer whether a datapath exists, and leave the hardware as bcmgenet_resume_noirq() set it up so close and suspend finish the teardown once. The WoL resume rebuilds instead, so stop the PHY first, phy_start() only takes PHY_READY or PHY_HALTED. Flagged in a sashiko review and reproduced on a CM4 by forcing the resume allocation to fail. Fixes: b6e978e50444 ("net: bcmgenet: add suspend/resume callbacks") Fixes: 254f3239dd07 ("net: bcmgenet: revise suspend/resume") # for the WoL parts Signed-off-by: Nicolai Buchwitz --- drivers/net/ethernet/broadcom/genet/bcmgenet.c | 46 +++++++++++++++++++++----- 1 file changed, 37 insertions(+), 9 deletions(-) diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index e8908916558b1..34acd3c47cc55 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -3107,7 +3107,9 @@ static void bcmgenet_fini_dma(struct bcmgenet_priv *priv) bcmgenet_free_rx_buffers(priv); bcmgenet_destroy_rx_page_pools(priv); kfree(priv->rx_cbs); + priv->rx_cbs = NULL; kfree(priv->tx_cbs); + priv->tx_cbs = NULL; } /* init_edma: Initialize DMA control register */ @@ -3165,6 +3167,7 @@ static int bcmgenet_init_dma(struct bcmgenet_priv *priv, bool flush_rx) priv->tx_cbs = kzalloc_objs(struct enet_cb, priv->num_tx_bds); if (!priv->tx_cbs) { kfree(priv->rx_cbs); + priv->rx_cbs = NULL; return -ENOMEM; } @@ -3184,7 +3187,9 @@ static int bcmgenet_init_dma(struct bcmgenet_priv *priv, bool flush_rx) bcmgenet_free_rx_buffers(priv); bcmgenet_destroy_rx_page_pools(priv); kfree(priv->rx_cbs); + priv->rx_cbs = NULL; kfree(priv->tx_cbs); + priv->tx_cbs = NULL; return ret; } @@ -3493,7 +3498,9 @@ static int bcmgenet_close(struct net_device *dev) netif_dbg(priv, ifdown, dev, "bcmgenet_close\n"); - bcmgenet_netif_stop(dev, false); + /* A failed resume has torn the datapath down already */ + if (priv->tx_cbs) + bcmgenet_netif_stop(dev, false); /* Really kill the PHY state machine and disconnect from it */ phy_disconnect(dev->phydev); @@ -3501,6 +3508,9 @@ static int bcmgenet_close(struct net_device *dev) free_irq(priv->irq0, priv); free_irq(priv->irq1, priv); + /* A failed resume left the device detached, __dev_open() refuses that */ + netif_device_attach(dev); + if (priv->internal_phy) ret = bcmgenet_power_down(priv, GENET_POWER_PASSIVE); @@ -4258,7 +4268,11 @@ static int bcmgenet_resume(struct device *d) if (device_may_wakeup(d) && priv->wolopts) { reg = bcmgenet_umac_readl(priv, UMAC_CMD); - if (reg & CMD_RX_EN) { + + /* The WoL power down sets CMD_RX_EN itself, so the bit + * alone does not mean the rings survived + */ + if (priv->tx_cbs && (reg & CMD_RX_EN)) { /* Successfully exited WoL, just resume data flows */ list_for_each_entry(rule, &priv->rxnfc_list, list) if (rule->state == BCMGENET_RXNFC_STATE_ENABLED) @@ -4288,6 +4302,9 @@ static int bcmgenet_resume(struct device *d) bcmgenet_rdma_disable(priv); bcmgenet_intr_disable(priv); bcmgenet_fini_dma(priv); + if (phy_is_started(dev->phydev) || + dev->phydev->state == PHY_ERROR) + phy_stop(dev->phydev); enable_irq(priv->irq1); } @@ -4310,8 +4327,9 @@ static int bcmgenet_resume(struct device *d) /* Reinitialize TDMA and RDMA and SW housekeeping */ ret = bcmgenet_init_dma(priv, false); if (ret) { + /* Leave the block clocked, close and suspend tear it down */ netdev_err(dev, "failed to initialize DMA\n"); - goto out_clk_disable; + return ret; } if (!device_may_wakeup(d)) @@ -4322,12 +4340,6 @@ static int bcmgenet_resume(struct device *d) netif_device_attach(dev); return 0; - -out_clk_disable: - if (priv->internal_phy) - bcmgenet_power_down(priv, GENET_POWER_PASSIVE); - clk_disable_unprepare(priv->clk); - return ret; } static int bcmgenet_suspend(struct device *d) @@ -4340,6 +4352,22 @@ static int bcmgenet_suspend(struct device *d) if (!netif_running(dev)) return 0; + /* A failed resume has torn the datapath down already. Resume still + * enables irq1 on the WoL path, so it has to be disabled here. + */ + if (!priv->tx_cbs) { + if (device_may_wakeup(d) && priv->wolopts) { + /* No rings to receive into, so no filter wake */ + bcmgenet_hfb_reg_writel(priv, 0, HFB_CTRL); + bcmgenet_intrl2_0_writel(priv, 0xFFFFFFFF, + INTRL2_CPU_MASK_SET); + bcmgenet_intrl2_0_writel(priv, 0xFFFFFFFF, + INTRL2_CPU_CLEAR); + disable_irq(priv->irq1); + } + return 0; + } + netif_device_detach(dev); if (device_may_wakeup(d) && priv->wolopts) { -- 2.53.0 bcmgenet_set_coalesce() writes tx_max_coalesced_frames straight into DMA_MBUF_DONE_THRESH and keeps no copy of it, while bcmgenet_init_tx_ring() programs a fixed 1. An ifdown and ifup or a resume then puts the Tx coalescing back to one frame per interrupt. Rx keeps its value per ring and reapplies it on init. Do the same for Tx. Fixes: 2f9130709d2c ("net: bcmgenet: Implement TX coalescing control knobs") Signed-off-by: Nicolai Buchwitz --- drivers/net/ethernet/broadcom/genet/bcmgenet.c | 10 ++++++++-- drivers/net/ethernet/broadcom/genet/bcmgenet.h | 1 + 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c index 34acd3c47cc55..8db82d924d218 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c @@ -931,10 +931,13 @@ static int bcmgenet_set_coalesce(struct net_device *dev, /* Program all TX queues with the same values, as there is no * ethtool knob to do coalescing on a per-queue basis */ - for (i = 0; i <= priv->hw_params->tx_queues; i++) + for (i = 0; i <= priv->hw_params->tx_queues; i++) { + priv->tx_rings[i].tx_max_coalesced_frames = + ec->tx_max_coalesced_frames; bcmgenet_tdma_ring_writel(priv, i, ec->tx_max_coalesced_frames, DMA_MBUF_DONE_THRESH); + } for (i = 0; i <= priv->hw_params->rx_queues; i++) bcmgenet_set_ring_rx_coalesce(&priv->rx_rings[i], ec); @@ -2748,7 +2751,8 @@ static void bcmgenet_init_tx_ring(struct bcmgenet_priv *priv, bcmgenet_tdma_ring_writel(priv, index, 0, TDMA_PROD_INDEX); bcmgenet_tdma_ring_writel(priv, index, 0, TDMA_CONS_INDEX); - bcmgenet_tdma_ring_writel(priv, index, 1, DMA_MBUF_DONE_THRESH); + bcmgenet_tdma_ring_writel(priv, index, ring->tx_max_coalesced_frames, + DMA_MBUF_DONE_THRESH); /* Disable rate control for now */ bcmgenet_tdma_ring_writel(priv, index, flow_period_val, TDMA_FLOW_PERIOD); @@ -4166,6 +4170,8 @@ static int bcmgenet_probe(struct platform_device *pdev) priv->rx_rings[i].index = i; priv->rx_rings[i].rx_max_coalesced_frames = 1; } + for (i = 0; i <= priv->hw_params->tx_queues; i++) + priv->tx_rings[i].tx_max_coalesced_frames = 1; /* Initialize u64 stats seq counter for 32bit machines */ for (i = 0; i <= GENET_MAX_MQ_CNT; i++) { diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h index 86f2aed20dbe8..e48ce39d3e7c8 100644 --- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h +++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h @@ -553,6 +553,7 @@ struct bcmgenet_tx_ring { unsigned int prod_index; /* Tx ring producer index SW copy */ unsigned int cb_ptr; /* Tx ring initial CB ptr */ unsigned int end_ptr; /* Tx ring end CB ptr */ + unsigned int tx_max_coalesced_frames; struct bcmgenet_priv *priv; }; -- 2.53.0