| Message ID | 20220628013342.13581-3-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add MTU change with stmmac interface running | expand |
On Tue, 28 Jun 2022 03:33:39 +0200 Christian Marangi wrote: > + stmmac_disable_all_queues(priv); > + > + for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) > + hrtimer_cancel(&priv->tx_queue[chan].txtimer); IIRC this hrtimer is to check for completions. Canceling it before netif_tx_disable() looks odd, presumably until the queues are stopped the timer can get scheduled again, no? > netif_tx_disable(dev); > > if (device_may_wakeup(priv->device)) > @@ -3764,11 +3769,6 @@ static int stmmac_release(struct net_device *dev) > phylink_stop(priv->phylink); > phylink_disconnect_phy(priv->phylink); > > - stmmac_disable_all_queues(priv); > - > - for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) > - hrtimer_cancel(&priv->tx_queue[chan].txtimer);
On Tue, Jun 28, 2022 at 08:54:35PM -0700, Jakub Kicinski wrote: > On Tue, 28 Jun 2022 03:33:39 +0200 Christian Marangi wrote: > > + stmmac_disable_all_queues(priv); > > + > > + for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) > > + hrtimer_cancel(&priv->tx_queue[chan].txtimer); > > IIRC this hrtimer is to check for completions. Canceling it before > netif_tx_disable() looks odd, presumably until the queues are stopped > the timer can get scheduled again, no? > My concern is that at the time hrtimer_cancel is called there may be some packet that still has to be processed and this cause kernel panic as stmmac_release free the descriptor (and tx_clean try to free garbage pointer) Bu honestly I put the hrtimer_cancel up to be extra safe, the main problem here was disabling napi polling after tx_disable that I think was wrong from the start. > > netif_tx_disable(dev); > > > > if (device_may_wakeup(priv->device)) > > @@ -3764,11 +3769,6 @@ static int stmmac_release(struct net_device *dev) > > phylink_stop(priv->phylink); > > phylink_disconnect_phy(priv->phylink); > > > > - stmmac_disable_all_queues(priv); > > - > > - for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) > > - hrtimer_cancel(&priv->tx_queue[chan].txtimer);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index f861246de2e5..f4ba27c1c7e0 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3756,6 +3756,11 @@ static int stmmac_release(struct net_device *dev) struct stmmac_priv *priv = netdev_priv(dev); u32 chan; + stmmac_disable_all_queues(priv); + + for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) + hrtimer_cancel(&priv->tx_queue[chan].txtimer); + netif_tx_disable(dev); if (device_may_wakeup(priv->device)) @@ -3764,11 +3769,6 @@ static int stmmac_release(struct net_device *dev) phylink_stop(priv->phylink); phylink_disconnect_phy(priv->phylink); - stmmac_disable_all_queues(priv); - - for (chan = 0; chan < priv->plat->tx_queues_to_use; chan++) - hrtimer_cancel(&priv->tx_queue[chan].txtimer); - /* Free the IRQ lines */ stmmac_free_irq(dev, REQ_IRQ_ERR_ALL, 0);
Disable all queues before tx_disable in stmmac_release to prevent a corner case where packet may be still queued at the same time tx_disable is called resulting in kernel panic if some packet still has to be processed. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)