diff options
author | Alex Elder <elder@linaro.org> | 2021-08-19 16:12:28 -0500 |
---|---|---|
committer | David S. Miller <davem@davemloft.net> | 2021-08-20 14:43:39 +0100 |
commit | b8e36e13ea5e464414b3e6465045cf0689500448 (patch) | |
tree | 5722b786b1c9b288695fef767334cabe7be87784 /drivers/net/ipa/ipa_modem.c | |
parent | 6505782c93bee97f2ea25330314056e0dbda6a6d (diff) |
net: ipa: fix TX queue race
Jakub Kicinski pointed out a race condition in ipa_start_xmit() in a
recently-accepted series of patches:
https://lore.kernel.org/netdev/20210812195035.2816276-1-elder@linaro.org/
We are stopping the modem TX queue in that function if the power
state is not active. We restart the TX queue again once hardware
resume is complete.
TX path Power Management
------- ----------------
pm_runtime_get(); no power Start resume
Stop TX queue ...
pm_runtime_put() Resume complete
return NETDEV_TX_BUSY Start TX queue
pm_runtime_get()
Power present, transmit
pm_runtime_put() (auto-suspend)
The issue is that the power management (resume) activity and the
network transmit activity can occur concurrently, and there's a
chance the queue will be stopped *after* it has been started again.
TX path Power Management
------- ----------------
Resume underway
pm_runtime_get(); no power ...
Resume complete
Start TX queue
Stop TX queue <-- No more transmits after this
pm_runtime_put()
return NETDEV_TX_BUSY
We address this using a STARTED flag to indicate when the TX queue
has been started from the resume path, and a spinlock to make the
flag and queue updates happen atomically.
TX path Power Management
------- ----------------
Resume underway
pm_runtime_get(); no power Resume complete
start TX queue \
If STARTED flag is *not* set: > atomic
Stop TX queue set STARTED flag /
pm_runtime_put()
return NETDEV_TX_BUSY
A second flag is used to address a different race that involves
another path requesting power.
TX path Other path Power Management
------- ---------- ----------------
pm_runtime_get_sync() Resume
Start TX queue \ atomic
Set STARTED flag /
(do its thing)
pm_runtime_put()
(auto-suspend)
pm_runtime_get() Mark delayed resume
STARTED *is* set, so
do *not* stop TX queue <-- Queue should be stopped here
pm_runtime_put()
return NETDEV_TX_BUSY Suspend done, resume
Resume complete
pm_runtime_get()
Stop TX queue
(STARTED is *not* set) Start TX queue \ atomic
pm_runtime_put() Set STARTED flag /
return NETDEV_TX_BUSY
So a STOPPED flag is set in the transmit path when it has stopped
the TX queue, and this pair of operations is also protected by the
spinlock. The resume path only restarts the TX queue if the STOPPED
flag is set. This case isn't a major problem, but it avoids the
"non-trivial amount of useless work" done by the networking stack
when NETDEV_TX_BUSY is returned.
Fixes: 6b51f802d652b ("net: ipa: ensure hardware has power in ipa_start_xmit()")
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>
Diffstat (limited to 'drivers/net/ipa/ipa_modem.c')
-rw-r--r-- | drivers/net/ipa/ipa_modem.c | 7 |
1 files changed, 5 insertions, 2 deletions
diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c index c8724af935b8..16d87910305e 100644 --- a/drivers/net/ipa/ipa_modem.c +++ b/drivers/net/ipa/ipa_modem.c @@ -130,6 +130,7 @@ ipa_start_xmit(struct sk_buff *skb, struct net_device *netdev) if (ret < 1) { /* If a resume won't happen, just drop the packet */ if (ret < 0 && ret != -EINPROGRESS) { + ipa_power_modem_queue_active(ipa); pm_runtime_put_noidle(dev); goto err_drop_skb; } @@ -138,13 +139,15 @@ ipa_start_xmit(struct sk_buff *skb, struct net_device *netdev) * until we're resumed; ipa_modem_resume() arranges for the * TX queue to be started again. */ - netif_stop_queue(netdev); + ipa_power_modem_queue_stop(ipa); (void)pm_runtime_put(dev); return NETDEV_TX_BUSY; } + ipa_power_modem_queue_active(ipa); + ret = ipa_endpoint_skb_tx(endpoint, skb); (void)pm_runtime_put(dev); @@ -241,7 +244,7 @@ static void ipa_modem_wake_queue_work(struct work_struct *work) { struct ipa_priv *priv = container_of(work, struct ipa_priv, work); - netif_wake_queue(priv->ipa->modem_netdev); + ipa_power_modem_queue_wake(priv->ipa); } /** ipa_modem_resume() - resume callback for runtime_pm |