Create a new work structure in the modem private data, and use it to
re-enable the modem network device transmit queue when resuming.
This is needed by the next patch, which stops the TX queue if IPA
power isn't active when a transmit request arrives. Packets will
start arriving the instant the TX queue is enabled, but resuming
isn't complete until ipa_modem_resume() returns. This way we're
sure to be resumed before transmits are allowed again.
Cancel it before calling ipa_stop() in ipa_modem_stop() to ensure
the transmit queue restart completes before it gets stopped there.
Signed-off-by: Alex Elder <[email protected]>
---
drivers/net/ipa/ipa_modem.c | 30 ++++++++++++++++++++++++++++--
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c
index 06e44afd2cf66..0a3b034614b61 100644
--- a/drivers/net/ipa/ipa_modem.c
+++ b/drivers/net/ipa/ipa_modem.c
@@ -9,6 +9,7 @@
#include <linux/netdevice.h>
#include <linux/skbuff.h>
#include <linux/if_rmnet.h>
+#include <linux/pm_runtime.h>
#include <linux/remoteproc/qcom_rproc.h>
#include "ipa.h"
@@ -33,9 +34,14 @@ enum ipa_modem_state {
IPA_MODEM_STATE_STOPPING,
};
-/** struct ipa_priv - IPA network device private data */
+/**
+ * struct ipa_priv - IPA network device private data
+ * @ipa: IPA pointer
+ * @work: Work structure used to wake the modem netdev TX queue
+ */
struct ipa_priv {
struct ipa *ipa;
+ struct work_struct work;
};
/** ipa_open() - Opens the modem network interface */
@@ -189,6 +195,21 @@ void ipa_modem_suspend(struct net_device *netdev)
ipa_endpoint_suspend_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]);
}
+/**
+ * ipa_modem_wake_queue_work() - enable modem netdev queue
+ * @work: Work structure
+ *
+ * Re-enable transmit on the modem network device. This is called
+ * in (power management) work queue context, scheduled when resuming
+ * the modem.
+ */
+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_modem_resume() - resume callback for runtime_pm
* @dev: pointer to device
*
@@ -205,7 +226,8 @@ void ipa_modem_resume(struct net_device *netdev)
ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]);
ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]);
- netif_wake_queue(netdev);
+ /* Arrange for the TX queue to be restarted */
+ (void)queue_pm_work(&priv->work);
}
int ipa_modem_start(struct ipa *ipa)
@@ -233,6 +255,7 @@ int ipa_modem_start(struct ipa *ipa)
SET_NETDEV_DEV(netdev, &ipa->pdev->dev);
priv = netdev_priv(netdev);
priv->ipa = ipa;
+ INIT_WORK(&priv->work, ipa_modem_wake_queue_work);
ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = netdev;
ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = netdev;
ipa->modem_netdev = netdev;
@@ -277,6 +300,9 @@ int ipa_modem_stop(struct ipa *ipa)
/* Clean up the netdev and endpoints if it was started */
if (netdev) {
+ struct ipa_priv *priv = netdev_priv(netdev);
+
+ cancel_work_sync(&priv->work);
/* If it was opened, stop it first */
if (netdev->flags & IFF_UP)
(void)ipa_stop(netdev);
--
2.27.0
On Thu, 12 Aug 2021 14:50:32 -0500 Alex Elder wrote:
> +/**
> + * ipa_modem_wake_queue_work() - enable modem netdev queue
> + * @work: Work structure
> + *
> + * Re-enable transmit on the modem network device. This is called
> + * in (power management) work queue context, scheduled when resuming
> + * the modem.
> + */
> +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_modem_resume() - resume callback for runtime_pm
> * @dev: pointer to device
> *
> @@ -205,7 +226,8 @@ void ipa_modem_resume(struct net_device *netdev)
> ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]);
> ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]);
>
> - netif_wake_queue(netdev);
> + /* Arrange for the TX queue to be restarted */
> + (void)queue_pm_work(&priv->work);
> }
Why move the wake call to a work queue, tho? It's okay to call it
from any context.
On 8/13/21 7:44 PM, Jakub Kicinski wrote:
> On Thu, 12 Aug 2021 14:50:32 -0500 Alex Elder wrote:
>> +/**
>> + * ipa_modem_wake_queue_work() - enable modem netdev queue
>> + * @work: Work structure
>> + *
>> + * Re-enable transmit on the modem network device. This is called
>> + * in (power management) work queue context, scheduled when resuming
>> + * the modem.
>> + */
>> +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_modem_resume() - resume callback for runtime_pm
>> * @dev: pointer to device
>> *
>> @@ -205,7 +226,8 @@ void ipa_modem_resume(struct net_device *netdev)
>> ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]);
>> ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]);
>>
>> - netif_wake_queue(netdev);
>> + /* Arrange for the TX queue to be restarted */
>> + (void)queue_pm_work(&priv->work);
>> }
>
> Why move the wake call to a work queue, tho? It's okay to call it
> from any context.
The issue isn't about the context in which is run (well, not
really, not in the sense you're talking about).
The issue has to do with the PM ->runtime_resume function
running concurrent with the network ->start_xmit function.
We need the hardware powered in ipa_start_xmit(). So we
call pm_runtime_get(), which will not block and which will
indicate in its return value whether power: is active
(return is 1); will be active once the resume underway
completes (return is -EINPROGRESS); will be active once
suspend underway and a delayed resume completes (return
is 0); or will be active once the newly-scheduled resume
completes (return is 0, scheduled on PM work queue).
We don't expect any other error, but if we get one we
drop the packet.
If the return value is 1, power is active and we transmit
the packet. If the return value indicates power is not
active, but will be, we stop the TX queue. No other packets
should be passed to ->start_xmit until TX is started again.
We wish to restart the TX queue when the ipa_runtime_resume()
completes. Here is the call path:
ipa_runtime_resume() This is the ->runtime_resume PM op
ipa_endpoint_resume()
ipa_modem_resume()
netif_wake_queue() Without this patch
The instant netif_wake_queue() is called, we start getting
calls to ipa_start_xmit(), which again attempts to transmit
the SKB that caused the queue to be stopped. And there is a
good chance that when that is called, the ipa_runtime_resume()
PM callback is still executing, and not complete. In that case,
we'll *again* get an -EINPROGRESS back from pm_runtime_get() in
ipa_start_xmit(), and we stop the TX queue again. Basically,
we're stuck.
All we need is for the TX queue to be started *after* the
PM ->runtime_resume callback completes and marks the the
PM runtime status ACTIVE. Scheduling this on the PM
workqueue ensures this will happen then, if we happen
to be running ipa_runtime_resume() via that workqueue.
If not, there's a bit of a race but it should resolve
(but I think here lies the specific race you mentioned
in the other message).
I'm open to other suggestions, but my hope was to at least
explain why I did it this way. I'll think about it over
the weekend and will send a new version of the series when
I come up with a solution.
Thank you very much for the review.
-Alex