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>
diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index 8f25107..74eb9ec 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -48,11 +48,15 @@ struct ipa_interconnect {
* enum ipa_power_flag - IPA power flags
* @IPA_POWER_FLAG_RESUMED: Whether resume from suspend has been signaled
* @IPA_POWER_FLAG_SYSTEM: Hardware is system (not runtime) suspended
+ * @IPA_POWER_FLAG_STOPPED: Modem TX is disabled by ipa_start_xmit()
+ * @IPA_POWER_FLAG_STARTED: Modem TX was enabled by ipa_runtime_resume()
* @IPA_POWER_FLAG_COUNT: Number of defined power flags
*/
enum ipa_power_flag {
IPA_POWER_FLAG_RESUMED,
IPA_POWER_FLAG_SYSTEM,
+ IPA_POWER_FLAG_STOPPED,
+ IPA_POWER_FLAG_STARTED,
IPA_POWER_FLAG_COUNT, /* Last; not a flag */
};
@@ -60,6 +64,7 @@ enum ipa_power_flag {
* struct ipa_clock - IPA clocking information
* @dev: IPA device pointer
* @core: IPA core clock
+ * @spinlock: Protects modem TX queue enable/disable
* @flags: Boolean state flags
* @interconnect_count: Number of elements in interconnect[]
* @interconnect: Interconnect array
@@ -67,6 +72,7 @@ enum ipa_power_flag {
struct ipa_clock {
struct device *dev;
struct clk *core;
+ spinlock_t spinlock; /* used with STOPPED/STARTED power flags */
DECLARE_BITMAP(flags, IPA_POWER_FLAG_COUNT);
u32 interconnect_count;
struct ipa_interconnect *interconnect;
@@ -334,6 +340,70 @@ static void ipa_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
ipa_interrupt_suspend_clear_all(ipa->interrupt);
}
+/* The next few functions coordinate stopping and starting the modem
+ * network device transmit queue.
+ *
+ * Transmit can be running concurrent with power resume, and there's a
+ * chance the resume completes before the transmit path stops the queue,
+ * leaving the queue in a stopped state. The next two functions are used
+ * to avoid this: ipa_power_modem_queue_stop() is used by ipa_start_xmit()
+ * to conditionally stop the TX queue; and ipa_power_modem_queue_start()
+ * is used by ipa_runtime_resume() to conditionally restart it.
+ *
+ * Two flags and a spinlock are used. If the queue is stopped, the STOPPED
+ * power flag is set. And if the queue is started, the STARTED flag is set.
+ * The queue is only started on resume if the STOPPED flag is set. And the
+ * queue is only started in ipa_start_xmit() if the STARTED flag is *not*
+ * set. As a result, the queue remains operational if the two activites
+ * happen concurrently regardless of the order they complete. The spinlock
+ * ensures the flag and TX queue operations are done atomically.
+ *
+ * The first function stops the modem netdev transmit queue, but only if
+ * the STARTED flag is *not* set. That flag is cleared if it was set.
+ * If the queue is stopped, the STOPPED flag is set. This is called only
+ * from the power ->runtime_resume operation.
+ */
+void ipa_power_modem_queue_stop(struct ipa *ipa)
+{
+ struct ipa_clock *clock = ipa->clock;
+ unsigned long flags;
+
+ spin_lock_irqsave(&clock->spinlock, flags);
+
+ if (!__test_and_clear_bit(IPA_POWER_FLAG_STARTED, clock->flags)) {
+ netif_stop_queue(ipa->modem_netdev);
+ __set_bit(IPA_POWER_FLAG_STOPPED, clock->flags);
+ }
+
+ spin_unlock_irqrestore(&clock->spinlock, flags);
+}
+
+/* This function starts the modem netdev transmit queue, but only if the
+ * STOPPED flag is set. That flag is cleared if it was set. If the queue
+ * was restarted, the STARTED flag is set; this allows ipa_start_xmit()
+ * to skip stopping the queue in the event of a race.
+ */
+void ipa_power_modem_queue_wake(struct ipa *ipa)
+{
+ struct ipa_clock *clock = ipa->clock;
+ unsigned long flags;
+
+ spin_lock_irqsave(&clock->spinlock, flags);
+
+ if (__test_and_clear_bit(IPA_POWER_FLAG_STOPPED, clock->flags)) {
+ __set_bit(IPA_POWER_FLAG_STARTED, clock->flags);
+ netif_wake_queue(ipa->modem_netdev);
+ }
+
+ spin_unlock_irqrestore(&clock->spinlock, flags);
+}
+
+/* This function clears the STARTED flag once the TX queue is operating */
+void ipa_power_modem_queue_active(struct ipa *ipa)
+{
+ clear_bit(IPA_POWER_FLAG_STARTED, ipa->clock->flags);
+}
+
int ipa_power_setup(struct ipa *ipa)
{
int ret;
@@ -383,6 +453,7 @@ ipa_clock_init(struct device *dev, const struct ipa_clock_data *data)
}
clock->dev = dev;
clock->core = clk;
+ spin_lock_init(&clock->spinlock);
clock->interconnect_count = data->interconnect_count;
ret = ipa_interconnect_init(clock, dev, data->interconnect_data);