[PATCH net-next 0/6] net: ipa: more work toward runtime PM

[PATCH net-next 0/6] net: ipa: more work toward runtime PM

[Alex Elder]

The first two patches in this series are basically bug fixes, but in
practice I don't think we've seen the problems they might cause.

The third patch moves clock and interconnect related error messages
around a bit, reporting better information and doing so in the
functions where they are enabled or disabled (rather than those
functions' callers).

The last three patches move power-related code into "ipa_clock.c",
as a step toward generalizing the purpose of that source file.

					-Alex

Alex Elder (6):
  net: ipa: don't suspend/resume modem if not up
  net: ipa: reorder netdev pointer assignments
  net: ipa: improve IPA clock error messages
  net: ipa: move IPA power operations to ipa_clock.c
  net: ipa: move ipa_suspend_handler()
  net: ipa: move IPA flags field

 drivers/net/ipa/ipa.h       |  12 ---
 drivers/net/ipa/ipa_clock.c | 147 +++++++++++++++++++++++++++++++-----
 drivers/net/ipa/ipa_clock.h |  15 ++++
 drivers/net/ipa/ipa_main.c  |  97 ++----------------------
 drivers/net/ipa/ipa_modem.c |  30 +++++---
 5 files changed, 172 insertions(+), 129 deletions(-)

-- 
2.27.0

[PATCH net-next 1/6] net: ipa: don't suspend/resume modem if not up

[Alex Elder]

The modem network device is set up by ipa_modem_start().  But its
TX queue is not actually started and endpoints enabled until it is
opened.

So avoid stopping the modem network device TX queue and disabling
endpoints on suspend or stop unless the netdev is marked UP.  And
skip attempting to resume unless it is UP.

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 4ea8287e9d237..663a610979e70 100644
--- a/drivers/net/ipa/ipa_modem.c
+++ b/drivers/net/ipa/ipa_modem.c
@@ -178,6 +178,9 @@ void ipa_modem_suspend(struct net_device *netdev)
 	struct ipa_priv *priv = netdev_priv(netdev);
 	struct ipa *ipa = priv->ipa;
 
+	if (!(netdev->flags & IFF_UP))
+		return;
+
 	netif_stop_queue(netdev);
 
 	ipa_endpoint_suspend_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]);
@@ -194,6 +197,9 @@ void ipa_modem_resume(struct net_device *netdev)
 	struct ipa_priv *priv = netdev_priv(netdev);
 	struct ipa *ipa = priv->ipa;
 
+	if (!(netdev->flags & IFF_UP))
+		return;
+
 	ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]);
 	ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]);
 
@@ -265,9 +271,11 @@ int ipa_modem_stop(struct ipa *ipa)
 	/* Prevent the modem from triggering a call to ipa_setup() */
 	ipa_smp2p_disable(ipa);
 
-	/* Stop the queue and disable the endpoints if it's open */
+	/* Clean up the netdev and endpoints if it was started */
 	if (netdev) {
-		(void)ipa_stop(netdev);
+		/* If it was opened, stop it first */
+		if (netdev->flags & IFF_UP)
+			(void)ipa_stop(netdev);
 		ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = NULL;
 		ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = NULL;
 		ipa->modem_netdev = NULL;
-- 
2.27.0

[PATCH net-next 2/6] net: ipa: reorder netdev pointer assignments

[Alex Elder]

Assign the ipa->modem_netdev and endpoint->netdev pointers *before*
registering the network device.  As soon as the device is
registered it can be opened, and by that time we'll want those
pointers valid.

Similarly, don't make those pointers NULL until *after* the modem
network device is unregistered in ipa_modem_stop().

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

diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c
index 663a610979e70..ad4019e8016ec 100644
--- a/drivers/net/ipa/ipa_modem.c
+++ b/drivers/net/ipa/ipa_modem.c
@@ -231,13 +231,15 @@ int ipa_modem_start(struct ipa *ipa)
 	SET_NETDEV_DEV(netdev, &ipa->pdev->dev);
 	priv = netdev_priv(netdev);
 	priv->ipa = ipa;
+	ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = netdev;
+	ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = netdev;
+	ipa->modem_netdev = netdev;
 
 	ret = register_netdev(netdev);
-	if (!ret) {
-		ipa->modem_netdev = netdev;
-		ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = netdev;
-		ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = netdev;
-	} else {
+	if (ret) {
+		ipa->modem_netdev = NULL;
+		ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = NULL;
+		ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = NULL;
 		free_netdev(netdev);
 	}
 
@@ -276,10 +278,10 @@ int ipa_modem_stop(struct ipa *ipa)
 		/* If it was opened, stop it first */
 		if (netdev->flags & IFF_UP)
 			(void)ipa_stop(netdev);
-		ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = NULL;
-		ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = NULL;
-		ipa->modem_netdev = NULL;
 		unregister_netdev(netdev);
+		ipa->modem_netdev = NULL;
+		ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = NULL;
+		ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = NULL;
 		free_netdev(netdev);
 	}
 
-- 
2.27.0

[PATCH net-next 3/6] net: ipa: improve IPA clock error messages

[Alex Elder]

Rearrange messages reported when errors occur in the IPA clock code,
so that the specific interconnect is identified when an error occurs
enabling or disabling it, or the core clock is indicated when an
error occurs enabling it.

Have ipa_interconnect_disable() return zero or the negative error
value returned by the first interconnect that produced an error
when disabled.  For now, the callers ignore the returned value.

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

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index 69ef6ea41e619..849b6ec671a4d 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -144,8 +144,12 @@ static int ipa_interconnect_enable(struct ipa *ipa)
 		ret = icc_set_bw(interconnect->path,
 				 interconnect->average_bandwidth,
 				 interconnect->peak_bandwidth);
-		if (ret)
+		if (ret) {
+			dev_err(&ipa->pdev->dev,
+				"error %d enabling %s interconnect\n",
+				ret, icc_get_name(interconnect->path));
 			goto out_unwind;
+		}
 		interconnect++;
 	}
 
@@ -159,10 +163,11 @@ static int ipa_interconnect_enable(struct ipa *ipa)
 }
 
 /* To disable an interconnect, we just its bandwidth to 0 */
-static void ipa_interconnect_disable(struct ipa *ipa)
+static int ipa_interconnect_disable(struct ipa *ipa)
 {
 	struct ipa_interconnect *interconnect;
 	struct ipa_clock *clock = ipa->clock;
+	struct device *dev = &ipa->pdev->dev;
 	int result = 0;
 	u32 count;
 	int ret;
@@ -172,13 +177,16 @@ static void ipa_interconnect_disable(struct ipa *ipa)
 	while (count--) {
 		interconnect--;
 		ret = icc_set_bw(interconnect->path, 0, 0);
-		if (ret && !result)
-			result = ret;
+		if (ret) {
+			dev_err(dev, "error %d disabling %s interconnect\n",
+				ret, icc_get_name(interconnect->path));
+			/* Try to disable all; record only the first error */
+			if (!result)
+				result = ret;
+		}
 	}
 
-	if (result)
-		dev_err(&ipa->pdev->dev,
-			"error %d disabling IPA interconnects\n", ret);
+	return result;
 }
 
 /* Turn on IPA clocks, including interconnects */
@@ -191,8 +199,10 @@ static int ipa_clock_enable(struct ipa *ipa)
 		return ret;
 
 	ret = clk_prepare_enable(ipa->clock->core);
-	if (ret)
-		ipa_interconnect_disable(ipa);
+	if (ret) {
+		dev_err(&ipa->pdev->dev, "error %d enabling core clock\n", ret);
+		(void)ipa_interconnect_disable(ipa);
+	}
 
 	return ret;
 }
@@ -201,7 +211,7 @@ static int ipa_clock_enable(struct ipa *ipa)
 static void ipa_clock_disable(struct ipa *ipa)
 {
 	clk_disable_unprepare(ipa->clock->core);
-	ipa_interconnect_disable(ipa);
+	(void)ipa_interconnect_disable(ipa);
 }
 
 /* Get an IPA clock reference, but only if the reference count is
@@ -238,13 +248,8 @@ void ipa_clock_get(struct ipa *ipa)
 		goto out_mutex_unlock;
 
 	ret = ipa_clock_enable(ipa);
-	if (ret) {
-		dev_err(&ipa->pdev->dev, "error %d enabling IPA clock\n", ret);
-		goto out_mutex_unlock;
-	}
-
-	refcount_set(&clock->count, 1);
-
+	if (!ret)
+		refcount_set(&clock->count, 1);
 out_mutex_unlock:
 	mutex_unlock(&clock->mutex);
 }
-- 
2.27.0

[PATCH net-next 4/6] net: ipa: move IPA power operations to ipa_clock.c

[Alex Elder]

Move ipa_suspend() and ipa_resume(), as well as the definition of
the ipa_pm_ops structure into "ipa_clock.c".  Make ipa_pm_ops public
and declare it as extern in "ipa_clock.h".

This is part of centralizing IPA power management functionality into
"ipa_clock.c" (the file will eventually get a name change).

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

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index 849b6ec671a4d..475ea6318cdb2 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -9,9 +9,12 @@
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/interconnect.h>
+#include <linux/pm.h>
+#include <linux/bitops.h>
 
 #include "ipa.h"
 #include "ipa_clock.h"
+#include "ipa_endpoint.h"
 #include "ipa_modem.h"
 #include "ipa_data.h"
 
@@ -334,3 +337,62 @@ void ipa_clock_exit(struct ipa_clock *clock)
 	kfree(clock);
 	clk_put(clk);
 }
+
+/**
+ * ipa_suspend() - Power management system suspend callback
+ * @dev:	IPA device structure
+ *
+ * Return:	Always returns zero
+ *
+ * Called by the PM framework when a system suspend operation is invoked.
+ * Suspends endpoints and releases the clock reference held to keep
+ * the IPA clock running until this point.
+ */
+static int ipa_suspend(struct device *dev)
+{
+	struct ipa *ipa = dev_get_drvdata(dev);
+
+	/* Endpoints aren't usable until setup is complete */
+	if (ipa->setup_complete) {
+		__clear_bit(IPA_FLAG_RESUMED, ipa->flags);
+		ipa_endpoint_suspend(ipa);
+		gsi_suspend(&ipa->gsi);
+	}
+
+	ipa_clock_put(ipa);
+
+	return 0;
+}
+
+/**
+ * ipa_resume() - Power management system resume callback
+ * @dev:	IPA device structure
+ *
+ * Return:	Always returns 0
+ *
+ * Called by the PM framework when a system resume operation is invoked.
+ * Takes an IPA clock reference to keep the clock running until suspend,
+ * and resumes endpoints.
+ */
+static int ipa_resume(struct device *dev)
+{
+	struct ipa *ipa = dev_get_drvdata(dev);
+
+	/* This clock reference will keep the IPA out of suspend
+	 * until we get a power management suspend request.
+	 */
+	ipa_clock_get(ipa);
+
+	/* Endpoints aren't usable until setup is complete */
+	if (ipa->setup_complete) {
+		gsi_resume(&ipa->gsi);
+		ipa_endpoint_resume(ipa);
+	}
+
+	return 0;
+}
+
+const struct dev_pm_ops ipa_pm_ops = {
+	.suspend	= ipa_suspend,
+	.resume		= ipa_resume,
+};
diff --git a/drivers/net/ipa/ipa_clock.h b/drivers/net/ipa/ipa_clock.h
index 1fe634760e59d..2f0310d5709ca 100644
--- a/drivers/net/ipa/ipa_clock.h
+++ b/drivers/net/ipa/ipa_clock.h
@@ -11,6 +11,9 @@ struct device;
 struct ipa;
 struct ipa_clock_data;
 
+/* IPA device power management function block */
+extern const struct dev_pm_ops ipa_pm_ops;
+
 /**
  * ipa_clock_rate() - Return the current IPA core clock rate
  * @ipa:	IPA structure
diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index ae51109dea01b..28350b7c50c56 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -874,65 +874,6 @@ static void ipa_shutdown(struct platform_device *pdev)
 		dev_err(&pdev->dev, "shutdown: remove returned %d\n", ret);
 }
 
-/**
- * ipa_suspend() - Power management system suspend callback
- * @dev:	IPA device structure
- *
- * Return:	Always returns zero
- *
- * Called by the PM framework when a system suspend operation is invoked.
- * Suspends endpoints and releases the clock reference held to keep
- * the IPA clock running until this point.
- */
-static int ipa_suspend(struct device *dev)
-{
-	struct ipa *ipa = dev_get_drvdata(dev);
-
-	/* Endpoints aren't usable until setup is complete */
-	if (ipa->setup_complete) {
-		__clear_bit(IPA_FLAG_RESUMED, ipa->flags);
-		ipa_endpoint_suspend(ipa);
-		gsi_suspend(&ipa->gsi);
-	}
-
-	ipa_clock_put(ipa);
-
-	return 0;
-}
-
-/**
- * ipa_resume() - Power management system resume callback
- * @dev:	IPA device structure
- *
- * Return:	Always returns 0
- *
- * Called by the PM framework when a system resume operation is invoked.
- * Takes an IPA clock reference to keep the clock running until suspend,
- * and resumes endpoints.
- */
-static int ipa_resume(struct device *dev)
-{
-	struct ipa *ipa = dev_get_drvdata(dev);
-
-	/* This clock reference will keep the IPA out of suspend
-	 * until we get a power management suspend request.
-	 */
-	ipa_clock_get(ipa);
-
-	/* Endpoints aren't usable until setup is complete */
-	if (ipa->setup_complete) {
-		gsi_resume(&ipa->gsi);
-		ipa_endpoint_resume(ipa);
-	}
-
-	return 0;
-}
-
-static const struct dev_pm_ops ipa_pm_ops = {
-	.suspend	= ipa_suspend,
-	.resume		= ipa_resume,
-};
-
 static const struct attribute_group *ipa_attribute_groups[] = {
 	&ipa_attribute_group,
 	&ipa_feature_attribute_group,
-- 
2.27.0

[PATCH net-next 5/6] net: ipa: move ipa_suspend_handler()

[Alex Elder]

Move ipa_suspend_handler() into "ipa_clock.c" from "ipa_main.c", to
group with the reset of the suspend/resume code.  This IPA interrupt
is triggered if an IPA RX endpoint is suspended but has a packet to
be delivered.

Introduce ipa_power_setup() and ipa_power_teardown() to add and
remove the handler for the IPA SUSPEND interrupt at the same place
as before, while allowing the handler to remain private.

The "power" naming convention will be adopted elsewhere in this
file as well (soon).

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_clock.c | 34 +++++++++++++++++++++++++++++++++
 drivers/net/ipa/ipa_clock.h | 12 ++++++++++++
 drivers/net/ipa/ipa_main.c  | 38 +++++++------------------------------
 3 files changed, 53 insertions(+), 31 deletions(-)

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index 475ea6318cdb2..9e77d4854fe03 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -279,6 +279,40 @@ u32 ipa_clock_rate(struct ipa *ipa)
 	return ipa->clock ? (u32)clk_get_rate(ipa->clock->core) : 0;
 }
 
+/**
+ * ipa_suspend_handler() - Handle the suspend IPA interrupt
+ * @ipa:	IPA pointer
+ * @irq_id:	IPA interrupt type (unused)
+ *
+ * If an RX endpoint is suspended, and the IPA has a packet destined for
+ * that endpoint, the IPA generates a SUSPEND interrupt to inform the AP
+ * that it should resume the endpoint.  If we get one of these interrupts
+ * we just wake up the system.
+ */
+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.
+	 */
+	if (!test_and_set_bit(IPA_FLAG_RESUMED, ipa->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);
+}
+
+void ipa_power_setup(struct ipa *ipa)
+{
+	ipa_interrupt_add(ipa->interrupt, IPA_IRQ_TX_SUSPEND,
+			  ipa_suspend_handler);
+}
+
+void ipa_power_teardown(struct ipa *ipa)
+{
+	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
+}
+
 /* Initialize IPA clocking */
 struct ipa_clock *
 ipa_clock_init(struct device *dev, const struct ipa_clock_data *data)
diff --git a/drivers/net/ipa/ipa_clock.h b/drivers/net/ipa/ipa_clock.h
index 2f0310d5709ca..2a0f7ff3c9e64 100644
--- a/drivers/net/ipa/ipa_clock.h
+++ b/drivers/net/ipa/ipa_clock.h
@@ -22,6 +22,18 @@ extern const struct dev_pm_ops ipa_pm_ops;
  */
 u32 ipa_clock_rate(struct ipa *ipa);
 
+/**
+ * ipa_power_setup() - Set up IPA power management
+ * @ipa:	IPA pointer
+ */
+void ipa_power_setup(struct ipa *ipa);
+
+/**
+ * ipa_power_teardown() - Inverse of ipa_power_setup()
+ * @ipa:	IPA pointer
+ */
+void ipa_power_teardown(struct ipa *ipa);
+
 /**
  * ipa_clock_init() - Initialize IPA clocking
  * @dev:	IPA device
diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index 28350b7c50c56..25bbb456e0078 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -79,29 +79,6 @@
 /* Divider for 19.2 MHz crystal oscillator clock to get common timer clock */
 #define IPA_XO_CLOCK_DIVIDER	192	/* 1 is subtracted where used */
 
-/**
- * ipa_suspend_handler() - Handle the suspend IPA interrupt
- * @ipa:	IPA pointer
- * @irq_id:	IPA interrupt type (unused)
- *
- * If an RX endpoint is in suspend state, and the IPA has a packet
- * destined for that endpoint, the IPA generates a SUSPEND interrupt
- * to inform the AP that it should resume the endpoint.  If we get
- * one of these interrupts we just resume everything.
- */
-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.
-	 */
-	if (!test_and_set_bit(IPA_FLAG_RESUMED, ipa->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);
-}
-
 /**
  * ipa_setup() - Set up IPA hardware
  * @ipa:	IPA pointer
@@ -124,12 +101,11 @@ int ipa_setup(struct ipa *ipa)
 	if (ret)
 		return ret;
 
-	ipa_interrupt_add(ipa->interrupt, IPA_IRQ_TX_SUSPEND,
-			  ipa_suspend_handler);
+	ipa_power_setup(ipa);
 
 	ret = device_init_wakeup(dev, true);
 	if (ret)
-		goto err_interrupt_remove;
+		goto err_gsi_teardown;
 
 	ipa_endpoint_setup(ipa);
 
@@ -177,9 +153,9 @@ int ipa_setup(struct ipa *ipa)
 	ipa_endpoint_disable_one(command_endpoint);
 err_endpoint_teardown:
 	ipa_endpoint_teardown(ipa);
+	ipa_power_teardown(ipa);
 	(void)device_init_wakeup(dev, false);
-err_interrupt_remove:
-	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
+err_gsi_teardown:
 	gsi_teardown(&ipa->gsi);
 
 	return ret;
@@ -204,8 +180,8 @@ static void ipa_teardown(struct ipa *ipa)
 	command_endpoint = ipa->name_map[IPA_ENDPOINT_AP_COMMAND_TX];
 	ipa_endpoint_disable_one(command_endpoint);
 	ipa_endpoint_teardown(ipa);
+	ipa_power_teardown(ipa);
 	(void)device_init_wakeup(&ipa->pdev->dev, false);
-	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
 	gsi_teardown(&ipa->gsi);
 }
 
@@ -474,7 +450,7 @@ static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
 
 	ret = ipa_endpoint_config(ipa);
 	if (ret)
-		goto err_interrupt_deconfig;
+		goto err_uc_deconfig;
 
 	ipa_table_config(ipa);		/* No deconfig required */
 
@@ -491,7 +467,7 @@ static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
 
 err_endpoint_deconfig:
 	ipa_endpoint_deconfig(ipa);
-err_interrupt_deconfig:
+err_uc_deconfig:
 	ipa_uc_deconfig(ipa);
 	ipa_interrupt_deconfig(ipa->interrupt);
 	ipa->interrupt = NULL;
-- 
2.27.0

[PATCH net-next 6/6] net: ipa: move IPA flags field

[Alex Elder]

The ipa->flags field is only ever used in "ipa_clock.c", related to
suspend/resume activity.

Move the definition of the ipa_flag enumerated type to "ipa_clock.c".
And move the flags field from the ipa structure and to the ipa_clock
structure.  Rename the type and its values to include "power" or
"POWER" in the name.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa.h       | 12 ------------
 drivers/net/ipa/ipa_clock.c | 16 ++++++++++++++--
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ipa/ipa.h b/drivers/net/ipa/ipa.h
index 71ba996096bb9..34152fe02963d 100644
--- a/drivers/net/ipa/ipa.h
+++ b/drivers/net/ipa/ipa.h
@@ -27,20 +27,9 @@ struct ipa_clock;
 struct ipa_smp2p;
 struct ipa_interrupt;
 
-/**
- * enum ipa_flag - IPA state flags
- * @IPA_FLAG_RESUMED:	Whether resume from suspend has been signaled
- * @IPA_FLAG_COUNT:	Number of defined IPA flags
- */
-enum ipa_flag {
-	IPA_FLAG_RESUMED,
-	IPA_FLAG_COUNT,		/* Last; not a flag */
-};
-
 /**
  * struct ipa - IPA information
  * @gsi:		Embedded GSI structure
- * @flags:		Boolean state flags
  * @version:		IPA hardware version
  * @pdev:		Platform device
  * @completion:		Used to signal pipeline clear transfer complete
@@ -83,7 +72,6 @@ enum ipa_flag {
  */
 struct ipa {
 	struct gsi gsi;
-	DECLARE_BITMAP(flags, IPA_FLAG_COUNT);
 	enum ipa_version version;
 	struct platform_device *pdev;
 	struct completion completion;
diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index 9e77d4854fe03..a67b6136e3c01 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -45,11 +45,22 @@ struct ipa_interconnect {
 	u32 peak_bandwidth;
 };
 
+/**
+ * enum ipa_power_flag - IPA power flags
+ * @IPA_POWER_FLAG_RESUMED:	Whether resume from suspend has been signaled
+ * @IPA_POWER_FLAG_COUNT:	Number of defined power flags
+ */
+enum ipa_power_flag {
+	IPA_POWER_FLAG_RESUMED,
+	IPA_POWER_FLAG_COUNT,		/* Last; not a flag */
+};
+
 /**
  * struct ipa_clock - IPA clocking information
  * @count:		Clocking reference count
  * @mutex:		Protects clock enable/disable
  * @core:		IPA core clock
+ * @flags:		Boolean state flags
  * @interconnect_count:	Number of elements in interconnect[]
  * @interconnect:	Interconnect array
  */
@@ -57,6 +68,7 @@ struct ipa_clock {
 	refcount_t count;
 	struct mutex mutex; /* protects clock enable/disable */
 	struct clk *core;
+	DECLARE_BITMAP(flags, IPA_POWER_FLAG_COUNT);
 	u32 interconnect_count;
 	struct ipa_interconnect *interconnect;
 };
@@ -295,7 +307,7 @@ static void ipa_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
 	 * More than one endpoint could signal this; if so, ignore
 	 * all but the first.
 	 */
-	if (!test_and_set_bit(IPA_FLAG_RESUMED, ipa->flags))
+	if (!test_and_set_bit(IPA_POWER_FLAG_RESUMED, ipa->clock->flags))
 		pm_wakeup_dev_event(&ipa->pdev->dev, 0, true);
 
 	/* Acknowledge/clear the suspend interrupt on all endpoints */
@@ -388,7 +400,7 @@ static int ipa_suspend(struct device *dev)
 
 	/* Endpoints aren't usable until setup is complete */
 	if (ipa->setup_complete) {
-		__clear_bit(IPA_FLAG_RESUMED, ipa->flags);
+		__clear_bit(IPA_POWER_FLAG_RESUMED, ipa->clock->flags);
 		ipa_endpoint_suspend(ipa);
 		gsi_suspend(&ipa->gsi);
 	}
-- 
2.27.0

Re: [PATCH net-next 1/6] net: ipa: don't suspend/resume modem if not up

[Jakub Kicinski]

On Wed,  4 Aug 2021 10:36:21 -0500 Alex Elder wrote:
> The modem network device is set up by ipa_modem_start().  But its
> TX queue is not actually started and endpoints enabled until it is
> opened.
> 
> So avoid stopping the modem network device TX queue and disabling
> endpoints on suspend or stop unless the netdev is marked UP.  And
> skip attempting to resume unless it is UP.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>

You said in the cover letter that in practice this fix doesn't matter.
It seems trivial to test so perhaps it doesn't and we should leave the
code be? Looking at dev->flags without holding rtnl_lock() seems
suspicious, drivers commonly put the relevant portion of suspend/resume
routines under rtnl_lock()/rtnl_unlock() (although to be completely
frank IDK if it's actually possible for concurrent suspend +
open/close to happen).

Are there any callers of ipa_modem_stop() which don't hold rtnl_lock()? 

> diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c
> index 4ea8287e9d237..663a610979e70 100644
> --- a/drivers/net/ipa/ipa_modem.c
> +++ b/drivers/net/ipa/ipa_modem.c
> @@ -178,6 +178,9 @@ void ipa_modem_suspend(struct net_device *netdev)
>  	struct ipa_priv *priv = netdev_priv(netdev);
>  	struct ipa *ipa = priv->ipa;
>  
> +	if (!(netdev->flags & IFF_UP))
> +		return;
> +
>  	netif_stop_queue(netdev);
>  
>  	ipa_endpoint_suspend_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]);
> @@ -194,6 +197,9 @@ void ipa_modem_resume(struct net_device *netdev)
>  	struct ipa_priv *priv = netdev_priv(netdev);
>  	struct ipa *ipa = priv->ipa;
>  
> +	if (!(netdev->flags & IFF_UP))
> +		return;
> +
>  	ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]);
>  	ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]);
>  
> @@ -265,9 +271,11 @@ int ipa_modem_stop(struct ipa *ipa)
>  	/* Prevent the modem from triggering a call to ipa_setup() */
>  	ipa_smp2p_disable(ipa);
>  
> -	/* Stop the queue and disable the endpoints if it's open */
> +	/* Clean up the netdev and endpoints if it was started */
>  	if (netdev) {
> -		(void)ipa_stop(netdev);
> +		/* If it was opened, stop it first */
> +		if (netdev->flags & IFF_UP)
> +			(void)ipa_stop(netdev);
>  		ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = NULL;
>  		ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = NULL;
>  		ipa->modem_netdev = NULL;

Re: [PATCH net-next 2/6] net: ipa: reorder netdev pointer assignments

[Jakub Kicinski]

On Wed,  4 Aug 2021 10:36:22 -0500 Alex Elder wrote:
> Assign the ipa->modem_netdev and endpoint->netdev pointers *before*
> registering the network device.  As soon as the device is
> registered it can be opened, and by that time we'll want those
> pointers valid.
> 
> Similarly, don't make those pointers NULL until *after* the modem
> network device is unregistered in ipa_modem_stop().
> 
> Signed-off-by: Alex Elder <elder@linaro.org>

This one seems like a pretty legit race, net would be better if you
don't mind.

Re: [PATCH net-next 2/6] net: ipa: reorder netdev pointer assignments

[Jakub Kicinski]

On Thu, 5 Aug 2021 18:27:12 -0700 Jakub Kicinski wrote:
> On Wed,  4 Aug 2021 10:36:22 -0500 Alex Elder wrote:
> > Assign the ipa->modem_netdev and endpoint->netdev pointers *before*
> > registering the network device.  As soon as the device is
> > registered it can be opened, and by that time we'll want those
> > pointers valid.
> > 
> > Similarly, don't make those pointers NULL until *after* the modem
> > network device is unregistered in ipa_modem_stop().
> > 
> > Signed-off-by: Alex Elder <elder@linaro.org>  
> 
> This one seems like a pretty legit race, net would be better if you
> don't mind.

Ah, this set was already applied, don't mind me :)

Re: [PATCH net-next 1/6] net: ipa: don't suspend/resume modem if not up

[Alex Elder]

On 8/5/21 8:26 PM, Jakub Kicinski wrote:
> On Wed,  4 Aug 2021 10:36:21 -0500 Alex Elder wrote:
>> The modem network device is set up by ipa_modem_start().  But its
>> TX queue is not actually started and endpoints enabled until it is
>> opened.
>>
>> So avoid stopping the modem network device TX queue and disabling
>> endpoints on suspend or stop unless the netdev is marked UP.  And
>> skip attempting to resume unless it is UP.
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
> 
> You said in the cover letter that in practice this fix doesn't matter.

I don't think we've seen this problem with system suspend, but
with runtime suspend we could get a forced suspend request at
any time (and frequently), so if there is a problem, it will be
much more likely to occur.

For suspend, I don't think it's actually a "problem".  Disabling
the TX queue if it wasn't open is harmless--it just sets the
DRV_XOFF bit in the TX queue state field.  And we have a
separate "enabled endpoints" mask that prevents stopping or
suspending the endpoint if it wasn't opened.

But for resume, waking the queue schedules it.  I'm not sure
what exactly ensues in that case, but it's not correct if the
network device hasn't been opened.  For endpoints, again, they
won't be resumed if they weren't enabled, so that part's OK.

> It seems trivial to test so perhaps it doesn't and we should leave the
> code be? Looking at dev->flags without holding rtnl_lock() seems
> suspicious, drivers commonly put the relevant portion of suspend/resume
> routines under rtnl_lock()/rtnl_unlock() (although to be completely

I don't use rtnl_lock()/rtnl_unlock() *anywhere* in the driver.
It has no netlink interface (yet), and therefore I didn't even
think about using rtnl_lock().  Do I need it?

> frank IDK if it's actually possible for concurrent suspend +
> open/close to happen).

I think it isn't possible, but I'm less than 100% sure.  I've
been thinking a lot about exactly this sort of question lately...

> Are there any callers of ipa_modem_stop() which don't hold rtnl_lock()?

None of them take that lock.  It is called in the driver ->remove
callback, and is called during cleanup if the modem crashes.

I think this fix is good, but as I said in the cover letter I'm
not aware of ever having hit it to date.

Thank you very much for your review and comments.

					-Alex

>> diff --git a/drivers/net/ipa/ipa_modem.c b/drivers/net/ipa/ipa_modem.c
>> index 4ea8287e9d237..663a610979e70 100644
>> --- a/drivers/net/ipa/ipa_modem.c
>> +++ b/drivers/net/ipa/ipa_modem.c
>> @@ -178,6 +178,9 @@ void ipa_modem_suspend(struct net_device *netdev)
>>   	struct ipa_priv *priv = netdev_priv(netdev);
>>   	struct ipa *ipa = priv->ipa;
>>   
>> +	if (!(netdev->flags & IFF_UP))
>> +		return;
>> +
>>   	netif_stop_queue(netdev);
>>   
>>   	ipa_endpoint_suspend_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]);
>> @@ -194,6 +197,9 @@ void ipa_modem_resume(struct net_device *netdev)
>>   	struct ipa_priv *priv = netdev_priv(netdev);
>>   	struct ipa *ipa = priv->ipa;
>>   
>> +	if (!(netdev->flags & IFF_UP))
>> +		return;
>> +
>>   	ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]);
>>   	ipa_endpoint_resume_one(ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]);
>>   
>> @@ -265,9 +271,11 @@ int ipa_modem_stop(struct ipa *ipa)
>>   	/* Prevent the modem from triggering a call to ipa_setup() */
>>   	ipa_smp2p_disable(ipa);
>>   
>> -	/* Stop the queue and disable the endpoints if it's open */
>> +	/* Clean up the netdev and endpoints if it was started */
>>   	if (netdev) {
>> -		(void)ipa_stop(netdev);
>> +		/* If it was opened, stop it first */
>> +		if (netdev->flags & IFF_UP)
>> +			(void)ipa_stop(netdev);
>>   		ipa->name_map[IPA_ENDPOINT_AP_MODEM_RX]->netdev = NULL;
>>   		ipa->name_map[IPA_ENDPOINT_AP_MODEM_TX]->netdev = NULL;
>>   		ipa->modem_netdev = NULL;
> 

Re: [PATCH net-next 2/6] net: ipa: reorder netdev pointer assignments

[Alex Elder]

On 8/5/21 8:27 PM, Jakub Kicinski wrote:
> On Wed,  4 Aug 2021 10:36:22 -0500 Alex Elder wrote:
>> Assign the ipa->modem_netdev and endpoint->netdev pointers *before*
>> registering the network device.  As soon as the device is
>> registered it can be opened, and by that time we'll want those
>> pointers valid.
>>
>> Similarly, don't make those pointers NULL until *after* the modem
>> network device is unregistered in ipa_modem_stop().
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
> 
> This one seems like a pretty legit race, net would be better if you
> don't mind.

I don't mind at all.  But now that it's accepted, I'm not sure how
to go about getting it back-ported.  Mainly I don't want to interfere
with any interaction between net/master and net-next/master...  Maybe
you're in a better position to do that.  And if so:

Fixes: 57f63faf0562 ("net: ipa: only set endpoint netdev pointer when in 
use")

I'll happily do it if you can tell me the best way how.  Thanks.

					-Alex

Re: [PATCH net-next 2/6] net: ipa: reorder netdev pointer assignments

[Alex Elder]

On 8/5/21 8:41 PM, Jakub Kicinski wrote:
> On Thu, 5 Aug 2021 18:27:12 -0700 Jakub Kicinski wrote:
>> On Wed,  4 Aug 2021 10:36:22 -0500 Alex Elder wrote:
>>> Assign the ipa->modem_netdev and endpoint->netdev pointers *before*
>>> registering the network device.  As soon as the device is
>>> registered it can be opened, and by that time we'll want those
>>> pointers valid.
>>>
>>> Similarly, don't make those pointers NULL until *after* the modem
>>> network device is unregistered in ipa_modem_stop().
>>>
>>> Signed-off-by: Alex Elder <elder@linaro.org>
>>
>> This one seems like a pretty legit race, net would be better if you
>> don't mind.
> 
> Ah, this set was already applied, don't mind me :)

I really appreciate your review and feedback.  Applied or not,
it's valuable to me.

					-Alex

Re: [PATCH net-next 1/6] net: ipa: don't suspend/resume modem if not up

[Jakub Kicinski]

On Fri, 6 Aug 2021 06:39:46 -0500 Alex Elder wrote:
> On 8/5/21 8:26 PM, Jakub Kicinski wrote:
> > On Wed,  4 Aug 2021 10:36:21 -0500 Alex Elder wrote:  
> >> The modem network device is set up by ipa_modem_start().  But its
> >> TX queue is not actually started and endpoints enabled until it is
> >> opened.
> >>
> >> So avoid stopping the modem network device TX queue and disabling
> >> endpoints on suspend or stop unless the netdev is marked UP.  And
> >> skip attempting to resume unless it is UP.
> >>
> >> Signed-off-by: Alex Elder <elder@linaro.org>  
> > 
> > You said in the cover letter that in practice this fix doesn't matter.  
> 
> I don't think we've seen this problem with system suspend, but
> with runtime suspend we could get a forced suspend request at
> any time (and frequently), so if there is a problem, it will be
> much more likely to occur.
> 
> For suspend, I don't think it's actually a "problem".  Disabling
> the TX queue if it wasn't open is harmless--it just sets the
> DRV_XOFF bit in the TX queue state field.  And we have a
> separate "enabled endpoints" mask that prevents stopping or
> suspending the endpoint if it wasn't opened.
> 
> But for resume, waking the queue schedules it.  I'm not sure
> what exactly ensues in that case, but it's not correct if the
> network device hasn't been opened.  For endpoints, again, they
> won't be resumed if they weren't enabled, so that part's OK.
> 
> > It seems trivial to test so perhaps it doesn't and we should leave the
> > code be? Looking at dev->flags without holding rtnl_lock() seems
> > suspicious, drivers commonly put the relevant portion of suspend/resume
> > routines under rtnl_lock()/rtnl_unlock() (although to be completely  
> 
> I don't use rtnl_lock()/rtnl_unlock() *anywhere* in the driver.
> It has no netlink interface (yet), and therefore I didn't even
> think about using rtnl_lock().  Do I need it?

Runtime PM interactions with rtnl_lock get really tricky, if there are
callers which will wake the device up while holding rtnl then taking
rtnl in .resume will cause an obvious deadlock, right?

I'm starting to feel like driver's RPM-related code has to be under it's
own lock, and interrogating higher layer's (e.g. network stack's) state
from RPM code should be avoided...

Long story short I don't think we have a good handle on this, 
I certainly don't so maybe let's leave your code be, for now.

> > frank IDK if it's actually possible for concurrent suspend +
> > open/close to happen).  
> 
> I think it isn't possible, but I'm less than 100% sure.  I've
> been thinking a lot about exactly this sort of question lately...
> 
> > Are there any callers of ipa_modem_stop() which don't hold rtnl_lock()?  
> 
> None of them take that lock.  It is called in the driver ->remove
> callback, and is called during cleanup if the modem crashes.
> 
> I think this fix is good, but as I said in the cover letter I'm
> not aware of ever having hit it to date.
> 
> Thank you very much for your review and comments.