| Message ID | 20210812195035.2816276-4-elder@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | net: ipa: last things before PM conversion | expand |
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
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);
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 <elder@linaro.org> --- drivers/net/ipa/ipa_modem.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) -- 2.27.0