[PATCH net-next 0/6] net: ipa: last things before PM conversion

[PATCH net-next 0/6] net: ipa: last things before PM conversion

[Alex Elder]

This series contains a few remaining changes needed before fully
switching over to using runtime power management rather than the
previous "IPA clock" mechanism.

The first patch moves the calls to enable and disable the IPA
interrupt as a system wakeup interrupt into "ipa_clock.c" with the
rest of the power-related code.

The second adds a flag to make it possible to distinguish runtime
suspend from system suspend.

The third and fourth patches arrange for the ->start_xmit path to
resume hardware if necessary, to ensure it is powered.  If power is
not active, the TX queue is stopped, and arrangements are made for
the queue to be restarted once hardware power is active again.

The fifth patch keeps the TX queue active during suspend.  This
isn't necessary for system suspend but it's important for runtime
suspend.

And the last patch makes it so we don't hold the hardware active
while the modem network device is open.

					-Alex

Alex Elder (6):
  net: ipa: enable wakeup in ipa_power_setup()
  net: ipa: distinguish system from runtime suspend
  net: ipa: re-enable transmit in PM WQ context
  net: ipa: ensure hardware has power in ipa_start_xmit()
  net: ipa: don't stop TX on suspend
  net: ipa: don't hold clock reference while netdev open

 drivers/net/ipa/ipa_clock.c | 49 ++++++++++++++++++++-----
 drivers/net/ipa/ipa_clock.h |  4 ++-
 drivers/net/ipa/ipa_main.c  |  6 +---
 drivers/net/ipa/ipa_modem.c | 72 +++++++++++++++++++++++++++++++++----
 4 files changed, 111 insertions(+), 20 deletions(-)

-- 
2.27.0

[PATCH net-next 1/6] net: ipa: enable wakeup in ipa_power_setup()

[Alex Elder]

Move the call to enable the IPA interrupt as a wakeup interrupt into
ipa_power_setup(), disable it in ipa_power_teardown().

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_clock.c | 11 ++++++++++-
 drivers/net/ipa/ipa_clock.h |  4 +++-
 drivers/net/ipa/ipa_main.c  |  6 +-----
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index 6df66c574d594..cdbaba6618e9e 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -310,14 +310,23 @@ static void ipa_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
 	ipa_interrupt_suspend_clear_all(ipa->interrupt);
 }
 
-void ipa_power_setup(struct ipa *ipa)
+int ipa_power_setup(struct ipa *ipa)
 {
+	int ret;
+
 	ipa_interrupt_add(ipa->interrupt, IPA_IRQ_TX_SUSPEND,
 			  ipa_suspend_handler);
+
+	ret = device_init_wakeup(&ipa->pdev->dev, true);
+	if (ret)
+		ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
+
+	return ret;
 }
 
 void ipa_power_teardown(struct ipa *ipa)
 {
+	(void)device_init_wakeup(&ipa->pdev->dev, false);
 	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
 }
 
diff --git a/drivers/net/ipa/ipa_clock.h b/drivers/net/ipa/ipa_clock.h
index 5c118f2c42e7a..5c53241336a1a 100644
--- a/drivers/net/ipa/ipa_clock.h
+++ b/drivers/net/ipa/ipa_clock.h
@@ -25,8 +25,10 @@ u32 ipa_clock_rate(struct ipa *ipa);
 /**
  * ipa_power_setup() - Set up IPA power management
  * @ipa:	IPA pointer
+ *
+ * Return:	0 if successful, or a negative error code
  */
-void ipa_power_setup(struct ipa *ipa);
+int ipa_power_setup(struct ipa *ipa);
 
 /**
  * ipa_power_teardown() - Inverse of ipa_power_setup()
diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index f332210ce5354..2f8ef831fa213 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -101,9 +101,7 @@ int ipa_setup(struct ipa *ipa)
 	if (ret)
 		return ret;
 
-	ipa_power_setup(ipa);
-
-	ret = device_init_wakeup(dev, true);
+	ret = ipa_power_setup(ipa);
 	if (ret)
 		goto err_gsi_teardown;
 
@@ -154,7 +152,6 @@ int ipa_setup(struct ipa *ipa)
 err_endpoint_teardown:
 	ipa_endpoint_teardown(ipa);
 	ipa_power_teardown(ipa);
-	(void)device_init_wakeup(dev, false);
 err_gsi_teardown:
 	gsi_teardown(&ipa->gsi);
 
@@ -181,7 +178,6 @@ static void ipa_teardown(struct ipa *ipa)
 	ipa_endpoint_disable_one(command_endpoint);
 	ipa_endpoint_teardown(ipa);
 	ipa_power_teardown(ipa);
-	(void)device_init_wakeup(&ipa->pdev->dev, false);
 	gsi_teardown(&ipa->gsi);
 }
 
-- 
2.27.0

[PATCH net-next 2/6] net: ipa: distinguish system from runtime suspend

[Alex Elder]

Add a new flag that is set when the hardware is suspended due to a
system suspend operation, distingishing it from runtime suspend.
Use it in the SUSPEND IPA interrupt handler to determine whether to
trigger a system resume because of the event.  Define new suspend
and resume power management callback functions to set and clear the
new flag, respectively.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_clock.c | 38 ++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index cdbaba6618e9e..8f25107c1f1e7 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -47,10 +47,12 @@ 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_COUNT:	Number of defined power flags
  */
 enum ipa_power_flag {
 	IPA_POWER_FLAG_RESUMED,
+	IPA_POWER_FLAG_SYSTEM,
 	IPA_POWER_FLAG_COUNT,		/* Last; not a flag */
 };
 
@@ -281,6 +283,27 @@ int ipa_clock_put(struct ipa *ipa)
 	return pm_runtime_put(&ipa->pdev->dev);
 }
 
+static int ipa_suspend(struct device *dev)
+{
+	struct ipa *ipa = dev_get_drvdata(dev);
+
+	__set_bit(IPA_POWER_FLAG_SYSTEM, ipa->clock->flags);
+
+	return pm_runtime_force_suspend(dev);
+}
+
+static int ipa_resume(struct device *dev)
+{
+	struct ipa *ipa = dev_get_drvdata(dev);
+	int ret;
+
+	ret = pm_runtime_force_resume(dev);
+
+	__clear_bit(IPA_POWER_FLAG_SYSTEM, ipa->clock->flags);
+
+	return ret;
+}
+
 /* Return the current IPA core clock rate */
 u32 ipa_clock_rate(struct ipa *ipa)
 {
@@ -299,12 +322,13 @@ u32 ipa_clock_rate(struct ipa *ipa)
  */
 static void ipa_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
 {
-	/* Just report the event, and let system resume handle the rest.
-	 * More than one endpoint could signal this; if so, ignore
-	 * all but the first.
+	/* To handle an IPA interrupt we will have resumed the hardware
+	 * just to handle the interrupt, so we're done.  If we are in a
+	 * system suspend, trigger a system resume.
 	 */
-	if (!test_and_set_bit(IPA_POWER_FLAG_RESUMED, ipa->clock->flags))
-		pm_wakeup_dev_event(&ipa->pdev->dev, 0, true);
+	if (!__test_and_set_bit(IPA_POWER_FLAG_RESUMED, ipa->clock->flags))
+		if (test_bit(IPA_POWER_FLAG_SYSTEM, ipa->clock->flags))
+			pm_wakeup_dev_event(&ipa->pdev->dev, 0, true);
 
 	/* Acknowledge/clear the suspend interrupt on all endpoints */
 	ipa_interrupt_suspend_clear_all(ipa->interrupt);
@@ -390,8 +414,8 @@ void ipa_clock_exit(struct ipa_clock *clock)
 }
 
 const struct dev_pm_ops ipa_pm_ops = {
-	.suspend		= pm_runtime_force_suspend,
-	.resume			= pm_runtime_force_resume,
+	.suspend		= ipa_suspend,
+	.resume			= ipa_resume,
 	.runtime_suspend	= ipa_runtime_suspend,
 	.runtime_resume		= ipa_runtime_resume,
 	.runtime_idle		= ipa_runtime_idle,
-- 
2.27.0

[PATCH net-next 3/6] net: ipa: re-enable transmit in PM WQ context

[Alex Elder]

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(-)

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

[PATCH net-next 4/6] net: ipa: ensure hardware has power in ipa_start_xmit()

[Alex Elder]

We need to ensure the hardware is powered when we transmit a packet.
But if it's not, we can't block to wait for it.  So asynchronously
request power in ipa_start_xmit(), and only proceed if the return
value indicates the power state is active.

If the hardware is not active, a runtime resume request will have
been initiated.  In that case, stop the network stack from further
transmit attempts until the resume completes.  Return NETDEV_TX_BUSY,
to retry sending the packet once the queue is restarted.

If the power request returns an error (other than -EINPROGRESS,
which just means a resume requested elsewhere isn't complete), just
drop the packet.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_modem.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c
index 0a3b034614b61..aa1b483d9f7db 100644
--- a/drivers/net/ipa/ipa_modem.c
+++ b/drivers/net/ipa/ipa_modem.c
@@ -106,6 +106,7 @@ static int ipa_start_xmit(struct sk_buff *skb, struct net_device *netdev)
 	struct ipa_endpoint *endpoint;
 	struct ipa *ipa = priv->ipa;
 	u32 skb_len = skb->len;
+	struct device *dev;
 	int ret;
 
 	if (!skb_len)
@@ -115,7 +116,31 @@ static int ipa_start_xmit(struct sk_buff *skb, struct net_device *netdev)
 	if (endpoint->data->qmap && skb->protocol != htons(ETH_P_MAP))
 		goto err_drop_skb;
 
+	/* The hardware must be powered for us to transmit */
+	dev = &ipa->pdev->dev;
+	ret = pm_runtime_get(dev);
+	if (ret < 1) {
+		/* If a resume won't happen, just drop the packet */
+		if (ret < 0 && ret != -EINPROGRESS) {
+			pm_runtime_put_noidle(dev);
+			goto err_drop_skb;
+		}
+
+		/* No power (yet).  Stop the network stack from transmitting
+		 * until we're resumed; ipa_modem_resume() arranges for the
+		 * TX queue to be started again.
+		 */
+		netif_stop_queue(netdev);
+
+		(void)pm_runtime_put(dev);
+
+		return NETDEV_TX_BUSY;
+	}
+
 	ret = ipa_endpoint_skb_tx(endpoint, skb);
+
+	(void)pm_runtime_put(dev);
+
 	if (ret) {
 		if (ret != -E2BIG)
 			return NETDEV_TX_BUSY;
@@ -201,7 +226,10 @@ void ipa_modem_suspend(struct net_device *netdev)
  *
  * Re-enable transmit on the modem network device.  This is called
  * in (power management) work queue context, scheduled when resuming
- * the modem.
+ * the modem.  We can't enable the queue directly in ipa_modem_resume()
+ * because transmits restart the instant the queue is awakened; but the
+ * device power state won't be ACTIVE until *after* ipa_modem_resume()
+ * returns.
  */
 static void ipa_modem_wake_queue_work(struct work_struct *work)
 {
-- 
2.27.0

[PATCH net-next 5/6] net: ipa: don't stop TX on suspend

[Alex Elder]

Currently we stop the modem netdev transmit queue when suspending
the hardware.  For system suspend this ensured we'd never attempt
to transmit while attempting to suspend the modem endpoints.

For runtime suspend, the IPA hardware might get suspended while the
system is operating.  In that case we want an attempt to transmit a
packet to cause the hardware to resume if necessary.  But if we
disable the queue this cannot happen.

So stop disabling the queue on suspend.  In case we end up disabling
it in ipa_start_xmit() (see the previous commit), we still arrange
to start the TX queue on resume.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_modem.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c
index aa1b483d9f7db..b176910d72868 100644
--- a/drivers/net/ipa/ipa_modem.c
+++ b/drivers/net/ipa/ipa_modem.c
@@ -214,8 +214,6 @@ void ipa_modem_suspend(struct net_device *netdev)
 	if (!(netdev->flags & IFF_UP))
 		return;
 
-	netif_stop_queue(netdev);
-
 	ipa_endpoint_suspend_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]);
 	ipa_endpoint_suspend_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]);
 }
-- 
2.27.0

[PATCH net-next 6/6] net: ipa: don't hold clock reference while netdev open

[Alex Elder]

Currently a clock reference is taken whenever the ->ndo_open
callback for the modem netdev is called.  That reference is dropped
when the device is closed, in ipa_stop().

We no longer need this, because ipa_start_xmit() now handles the
situation where the hardware power state is not active.

Drop the clock reference in ipa_open() when we're done, and take a
new reference in ipa_stop() before we begin closing the interface.

Finally (and unrelated, but trivial), change the return type of
ipa_start_xmit() to be netdev_tx_t instead of int.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_modem.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c
index b176910d72868..c8724af935b85 100644
--- a/drivers/net/ipa/ipa_modem.c
+++ b/drivers/net/ipa/ipa_modem.c
@@ -65,6 +65,8 @@ static int ipa_open(struct net_device *netdev)
 
 	netif_start_queue(netdev);
 
+	(void)ipa_clock_put(ipa);
+
 	return 0;
 
 err_disable_tx:
@@ -80,12 +82,17 @@ static int ipa_stop(struct net_device *netdev)
 {
 	struct ipa_priv *priv = netdev_priv(netdev);
 	struct ipa *ipa = priv->ipa;
+	int ret;
+
+	ret = ipa_clock_get(ipa);
+	if (WARN_ON(ret < 0))
+		goto out_clock_put;
 
 	netif_stop_queue(netdev);
 
 	ipa_endpoint_disable_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]);
 	ipa_endpoint_disable_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]);
-
+out_clock_put:
 	(void)ipa_clock_put(ipa);
 
 	return 0;
@@ -99,7 +106,8 @@ static int ipa_stop(struct net_device *netdev)
  * NETDEV_TX_OK: Success
  * NETDEV_TX_BUSY: Error while transmitting the skb. Try again later
  */
-static int ipa_start_xmit(struct sk_buff *skb, struct net_device *netdev)
+static netdev_tx_t
+ipa_start_xmit(struct sk_buff *skb, struct net_device *netdev)
 {
 	struct net_device_stats *stats = &netdev->stats;
 	struct ipa_priv *priv = netdev_priv(netdev);
-- 
2.27.0

Re: [PATCH net-next 3/6] net: ipa: re-enable transmit in PM WQ context

[Jakub Kicinski]

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.

Re: [PATCH net-next 4/6] net: ipa: ensure hardware has power in ipa_start_xmit()

[Jakub Kicinski]

On Thu, 12 Aug 2021 14:50:33 -0500 Alex Elder wrote:
> +	/* The hardware must be powered for us to transmit */
> +	dev = &ipa->pdev->dev;
> +	ret = pm_runtime_get(dev);
> +	if (ret < 1) {
> +		/* If a resume won't happen, just drop the packet */
> +		if (ret < 0 && ret != -EINPROGRESS) {
> +			pm_runtime_put_noidle(dev);
> +			goto err_drop_skb;
> +		}

This is racy, what if the pm work gets scheduled on another CPU and
calls wake right here (i.e. before you call netif_stop_queue())?
The queue may never get woken up?

> +		/* No power (yet).  Stop the network stack from transmitting
> +		 * until we're resumed; ipa_modem_resume() arranges for the
> +		 * TX queue to be started again.
> +		 */
> +		netif_stop_queue(netdev);
> +
> +		(void)pm_runtime_put(dev);
> +
> +		return NETDEV_TX_BUSY;

Re: [PATCH net-next 4/6] net: ipa: ensure hardware has power in ipa_start_xmit()

[Alex Elder]

On 8/13/21 7:46 PM, Jakub Kicinski wrote:
> On Thu, 12 Aug 2021 14:50:33 -0500 Alex Elder wrote:
>> +	/* The hardware must be powered for us to transmit */
>> +	dev = &ipa->pdev->dev;
>> +	ret = pm_runtime_get(dev);
>> +	if (ret < 1) {
>> +		/* If a resume won't happen, just drop the packet */
>> +		if (ret < 0 && ret != -EINPROGRESS) {
>> +			pm_runtime_put_noidle(dev);
>> +			goto err_drop_skb;
>> +		}
> 
> This is racy, what if the pm work gets scheduled on another CPU and
> calls wake right here (i.e. before you call netif_stop_queue())?
> The queue may never get woken up?

I haven't been seeing this happen but I think you may be right.

I did think about this race, but I think I was relying on the
PM work queue to somehow avoid the problem.  I need to think
about this again after a good night's sleep.  I might need
to add an atomic flag or something.

					-Alex

>> +		/* No power (yet).  Stop the network stack from transmitting
>> +		 * until we're resumed; ipa_modem_resume() arranges for the
>> +		 * TX queue to be started again.
>> +		 */
>> +		netif_stop_queue(netdev);
>> +
>> +		(void)pm_runtime_put(dev);
>> +
>> +		return NETDEV_TX_BUSY;

Re: [PATCH net-next 3/6] net: ipa: re-enable transmit in PM WQ context

[Alex Elder]

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

Re: [PATCH net-next 0/6] net: ipa: last things before PM conversion

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Thu, 12 Aug 2021 14:50:29 -0500 you wrote:
> This series contains a few remaining changes needed before fully
> switching over to using runtime power management rather than the
> previous "IPA clock" mechanism.
> 
> The first patch moves the calls to enable and disable the IPA
> interrupt as a system wakeup interrupt into "ipa_clock.c" with the
> rest of the power-related code.
> 
> [...]

Here is the summary with links:
  - [net-next,1/6] net: ipa: enable wakeup in ipa_power_setup()
    https://git.kernel.org/netdev/net-next/c/d430fe4bac02
  - [net-next,2/6] net: ipa: distinguish system from runtime suspend
    https://git.kernel.org/netdev/net-next/c/b9c532c11cab
  - [net-next,3/6] net: ipa: re-enable transmit in PM WQ context
    https://git.kernel.org/netdev/net-next/c/a96e73fa1269
  - [net-next,4/6] net: ipa: ensure hardware has power in ipa_start_xmit()
    https://git.kernel.org/netdev/net-next/c/6b51f802d652
  - [net-next,5/6] net: ipa: don't stop TX on suspend
    https://git.kernel.org/netdev/net-next/c/8dcf8bb30f17
  - [net-next,6/6] net: ipa: don't hold clock reference while netdev open
    https://git.kernel.org/netdev/net-next/c/8dc181f2cd62

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next 4/6] net: ipa: ensure hardware has power in ipa_start_xmit()

[Jakub Kicinski]

On Fri, 13 Aug 2021 21:25:23 -0500 Alex Elder wrote:
> > This is racy, what if the pm work gets scheduled on another CPU and
> > calls wake right here (i.e. before you call netif_stop_queue())?
> > The queue may never get woken up?  
> 
> I haven't been seeing this happen but I think you may be right.
> 
> I did think about this race, but I think I was relying on the
> PM work queue to somehow avoid the problem.  I need to think
> about this again after a good night's sleep.  I might need
> to add an atomic flag or something.

Maybe add a spin lock?  Seems like the whole wake up path will be
expensive enough for a spin lock to be in the noise. You can always 
add complexity later.

Re: [PATCH net-next 4/6] net: ipa: ensure hardware has power in ipa_start_xmit()

[Alex Elder]

On 8/16/21 9:15 AM, Jakub Kicinski wrote:
> On Fri, 13 Aug 2021 21:25:23 -0500 Alex Elder wrote:
>>> This is racy, what if the pm work gets scheduled on another CPU and
>>> calls wake right here (i.e. before you call netif_stop_queue())?
>>> The queue may never get woken up?
>>
>> I haven't been seeing this happen but I think you may be right.
>>
>> I did think about this race, but I think I was relying on the
>> PM work queue to somehow avoid the problem.  I need to think
>> about this again after a good night's sleep.  I might need
>> to add an atomic flag or something.
> 
> Maybe add a spin lock?  Seems like the whole wake up path will be
> expensive enough for a spin lock to be in the noise. You can always
> add complexity later.

Exactly what I just decided after trying to work out a
clever way without using a spinlock...  I'll be sending
out a fix today.  Thanks.

					-Alex

Re: [PATCH net-next 4/6] net: ipa: ensure hardware has power in ipa_start_xmit()

[Alex Elder]

On 8/16/21 9:20 AM, Alex Elder wrote:
> On 8/16/21 9:15 AM, Jakub Kicinski wrote:
>> On Fri, 13 Aug 2021 21:25:23 -0500 Alex Elder wrote:
>>>> This is racy, what if the pm work gets scheduled on another CPU and
>>>> calls wake right here (i.e. before you call netif_stop_queue())?
>>>> The queue may never get woken up?
>>>
>>> I haven't been seeing this happen but I think you may be right.
>>>
>>> I did think about this race, but I think I was relying on the
>>> PM work queue to somehow avoid the problem.  I need to think
>>> about this again after a good night's sleep.  I might need
>>> to add an atomic flag or something.
>>
>> Maybe add a spin lock?  Seems like the whole wake up path will be
>> expensive enough for a spin lock to be in the noise. You can always
>> add complexity later.
> 
> Exactly what I just decided after trying to work out a
> clever way without using a spinlock...  I'll be sending
> out a fix today.  Thanks.

I'm finding this isn't an easy problem to solve (or even think
about).  While I ponder the best course of action I'm going
to send out another series (i.e., *before* I send a fix for
this issue) because I'd like to get everything I have out
for review this week.  I *will* address this potential race
one way or another, possibly later today.

					-Alex

> 
>                      -Alex
> 

Re: [PATCH net-next 4/6] net: ipa: ensure hardware has power in ipa_start_xmit()

[Jakub Kicinski]

On Mon, 16 Aug 2021 12:56:40 -0500 Alex Elder wrote:
> I'm finding this isn't an easy problem to solve (or even think
> about).  While I ponder the best course of action I'm going
> to send out another series (i.e., *before* I send a fix for
> this issue) because I'd like to get everything I have out
> for review this week.  I *will* address this potential race
> one way or another, possibly later today.

Alright, thanks for the heads up.