[PATCH net-next v2 0/7] net: ipa: wake up system on RX available

[PATCH net-next v2 0/7] net: ipa: wake up system on RX available

[Alex Elder]

This series arranges for the IPA driver to wake up a suspended
system if the IPA hardware has a packet to deliver to the AP.
Version 2 replaces the first patch from version 1 with three
patches, in response to David Miller's feedback.

Specifically:
  - The first patch now replaces an atomic_t field with a
    refcount_t.  The affected field is not the one David
    commented on, but this fix is consistent with what he
    asked for.
  - The second patch replaces the atomic_t field David *did*
    comment on with a single bit in a new bitmap field;
    ultimately what's needed there is a Boolean flag anyway.
  - The third patch is renamed, but basically does the same
    thing the first patch did in version 1.  It now operates
    on a bit in a bitmap rather than on an atomic variable.

Currently, the GSI interrupt is set up to be a waking interrupt.
But the GSI interrupt won't actually fire for a stopped channel (or
a channel that underlies a suspended endpoint).  The fix involves
having the IPA rather than GSI interrupt wake up the AP.

The IPA hardware clock is managed by both the modem and the AP.
Even if the AP is in a fully-suspended state, the modem can clock
the IPA hardware, and can send a packet through IPA that is destined
for an endpoint on the AP.

When the IPA hardware finds a packet's destination is stopped or
suspended, it sends an *IPA interrupt* to the destination "execution
environment" (EE--in this case, the AP).  The desired behavior is
for the EE (even if suspended) to be able to handle the incoming
packet.

To do this, we arrange for the IPA interrupt to be a wakeup
interrupt.  And if the system is suspended when that interrupt
fires, we trigger a system resume operation.  While resuming the
system, the IPA driver starts all its channels (or for SDM845,
takes its endpoints out of suspend mode).

Whenever an RX channel is started, if it has a packet ready to be
consumed, the GSI interrupt will fire.  At this point the inbound
packet that caused this wakeup activity will be received.

The first three patches in the series were described above.  The
next three arrange for the IPA interrupt wake up the system.
Finally, with this design, we no longer want the GSI interrupt to
wake a suspended system, so that is removed by the last patch.

					-Alex


Alex Elder (7):
  net: ipa: use refcount_t for IPA clock reference count
  net: ipa: replace ipa->suspend_ref with a flag bit
  net: ipa: verify reference flag values
  net: ipa: manage endpoints separate from clock
  net: ipa: use device_init_wakeup()
  net: ipa: enable wakeup on IPA interrupt
  net: ipa: do not enable GSI interrupt for wakeup

 drivers/net/ipa/gsi.c           | 17 ++------
 drivers/net/ipa/gsi.h           |  1 -
 drivers/net/ipa/ipa.h           | 16 +++++--
 drivers/net/ipa/ipa_clock.c     | 28 +++++-------
 drivers/net/ipa/ipa_interrupt.c | 14 ++++++
 drivers/net/ipa/ipa_main.c      | 76 +++++++++++++++++++--------------
 6 files changed, 84 insertions(+), 68 deletions(-)

-- 
2.20.1

[PATCH net-next v2 1/7] net: ipa: use refcount_t for IPA clock reference count

[Alex Elder]

Take advantage of the checking provided by refcount_t, rather than
using a plain atomic to represent the IPA clock reference count.

Note that we need to *set* the value to 1 in ipa_clock_get() rather
than incrementing it from 0 (because doing that is considered an
error for a refcount_t).

Signed-off-by: Alex Elder <elder@linaro.org>
---
v2: This patch is new in version 2 of the series.

 drivers/net/ipa/ipa_clock.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index 398f2e47043d8..b703866f2e20b 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -4,7 +4,7 @@
  * Copyright (C) 2018-2020 Linaro Ltd.
  */
 
-#include <linux/atomic.h>
+#include <linux/refcount.h>
 #include <linux/mutex.h>
 #include <linux/clk.h>
 #include <linux/device.h>
@@ -51,7 +51,7 @@
  * @config_path:	Configuration space interconnect
  */
 struct ipa_clock {
-	atomic_t count;
+	refcount_t count;
 	struct mutex mutex; /* protects clock enable/disable */
 	struct clk *core;
 	struct icc_path *memory_path;
@@ -195,7 +195,7 @@ static void ipa_clock_disable(struct ipa *ipa)
  */
 bool ipa_clock_get_additional(struct ipa *ipa)
 {
-	return !!atomic_inc_not_zero(&ipa->clock->count);
+	return refcount_inc_not_zero(&ipa->clock->count);
 }
 
 /* Get an IPA clock reference.  If the reference count is non-zero, it is
@@ -231,7 +231,7 @@ void ipa_clock_get(struct ipa *ipa)
 
 	ipa_endpoint_resume(ipa);
 
-	atomic_inc(&clock->count);
+	refcount_set(&clock->count, 1);
 
 out_mutex_unlock:
 	mutex_unlock(&clock->mutex);
@@ -246,7 +246,7 @@ void ipa_clock_put(struct ipa *ipa)
 	struct ipa_clock *clock = ipa->clock;
 
 	/* If this is not the last reference there's nothing more to do */
-	if (!atomic_dec_and_mutex_lock(&clock->count, &clock->mutex))
+	if (!refcount_dec_and_mutex_lock(&clock->count, &clock->mutex))
 		return;
 
 	ipa_endpoint_suspend(ipa);
@@ -294,7 +294,7 @@ struct ipa_clock *ipa_clock_init(struct device *dev)
 		goto err_kfree;
 
 	mutex_init(&clock->mutex);
-	atomic_set(&clock->count, 0);
+	refcount_set(&clock->count, 0);
 
 	return clock;
 
@@ -311,7 +311,7 @@ void ipa_clock_exit(struct ipa_clock *clock)
 {
 	struct clk *clk = clock->core;
 
-	WARN_ON(atomic_read(&clock->count) != 0);
+	WARN_ON(refcount_read(&clock->count) != 0);
 	mutex_destroy(&clock->mutex);
 	ipa_interconnect_exit(clock);
 	kfree(clock);
-- 
2.20.1

[PATCH net-next v2 2/7] net: ipa: replace ipa->suspend_ref with a flag bit

[Alex Elder]

For suspend/resume, we currently take an extra clock reference to
prevent the IPA clock from being shut down until a power management
suspend request arrives.  An atomic field in the IPA structure is
used to indicate whether this reference has been taken.

Instead, introduce a new flags bitmap in the IPA structure, and use
a single bit in that bitmap rather than the atomic to indicate
whether we have taken the special IPA clock reference.

Signed-off-by: Alex Elder <elder@linaro.org>
---
v2: New patch to use a bitmap bit rather than an atomic_t.

 drivers/net/ipa/ipa.h      | 14 ++++++++++++--
 drivers/net/ipa/ipa_main.c | 14 +++++++-------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ipa/ipa.h b/drivers/net/ipa/ipa.h
index 407fee841a9a8..e02fe979b645b 100644
--- a/drivers/net/ipa/ipa.h
+++ b/drivers/net/ipa/ipa.h
@@ -27,15 +27,25 @@ struct ipa_clock;
 struct ipa_smp2p;
 struct ipa_interrupt;
 
+/**
+ * enum ipa_flag - IPA state flags
+ * @IPA_FLAG_CLOCK_HELD:	Whether IPA clock is held to prevent suspend
+ * @IPA_FLAG_COUNT:		Number of defined IPA flags
+ */
+enum ipa_flag {
+	IPA_FLAG_CLOCK_HELD,
+	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
  * @modem_rproc:	Remoteproc handle for modem subsystem
  * @smp2p:		SMP2P information
  * @clock:		IPA clocking information
- * @suspend_ref:	Whether clock reference preventing suspend taken
  * @table_addr:		DMA address of filter/route table content
  * @table_virt:		Virtual address of filter/route table content
  * @interrupt:		IPA Interrupt information
@@ -70,6 +80,7 @@ struct ipa_interrupt;
  */
 struct ipa {
 	struct gsi gsi;
+	DECLARE_BITMAP(flags, IPA_FLAG_COUNT);
 	enum ipa_version version;
 	struct platform_device *pdev;
 	struct rproc *modem_rproc;
@@ -77,7 +88,6 @@ struct ipa {
 	void *notifier;
 	struct ipa_smp2p *smp2p;
 	struct ipa_clock *clock;
-	atomic_t suspend_ref;
 
 	dma_addr_t table_addr;
 	__le64 *table_virt;
diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index 1fdfec41e4421..409375b96eb8f 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -84,7 +84,7 @@ static void ipa_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
 	 * endpoints will be resumed as a result.  This reference will
 	 * be dropped when we get a power management suspend request.
 	 */
-	if (!atomic_xchg(&ipa->suspend_ref, 1))
+	if (!test_and_set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
 		ipa_clock_get(ipa);
 
 	/* Acknowledge/clear the suspend interrupt on all endpoints */
@@ -508,7 +508,7 @@ static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
 	 * is held after initialization completes, and won't get dropped
 	 * unless/until a system suspend request arrives.
 	 */
-	atomic_set(&ipa->suspend_ref, 1);
+	__set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
 	ipa_clock_get(ipa);
 
 	ipa_hardware_config(ipa);
@@ -544,7 +544,7 @@ static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
 err_hardware_deconfig:
 	ipa_hardware_deconfig(ipa);
 	ipa_clock_put(ipa);
-	atomic_set(&ipa->suspend_ref, 0);
+	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
 
 	return ret;
 }
@@ -562,7 +562,7 @@ static void ipa_deconfig(struct ipa *ipa)
 	ipa_endpoint_deconfig(ipa);
 	ipa_hardware_deconfig(ipa);
 	ipa_clock_put(ipa);
-	atomic_set(&ipa->suspend_ref, 0);
+	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
 }
 
 static int ipa_firmware_load(struct device *dev)
@@ -777,7 +777,7 @@ static int ipa_probe(struct platform_device *pdev)
 	dev_set_drvdata(dev, ipa);
 	ipa->modem_rproc = rproc;
 	ipa->clock = clock;
-	atomic_set(&ipa->suspend_ref, 0);
+	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
 	ipa->wakeup_source = wakeup_source;
 	ipa->version = data->version;
 
@@ -913,7 +913,7 @@ static int ipa_suspend(struct device *dev)
 	struct ipa *ipa = dev_get_drvdata(dev);
 
 	ipa_clock_put(ipa);
-	atomic_set(&ipa->suspend_ref, 0);
+	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
 
 	return 0;
 }
@@ -933,7 +933,7 @@ static int ipa_resume(struct device *dev)
 	/* This clock reference will keep the IPA out of suspend
 	 * until we get a power management suspend request.
 	 */
-	atomic_set(&ipa->suspend_ref, 1);
+	__set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
 	ipa_clock_get(ipa);
 
 	return 0;
-- 
2.20.1

[PATCH net-next v2 3/7] net: ipa: verify reference flag values

[Alex Elder]

We take a single IPA clock reference to keep the clock running until
we get a system suspend operation, and maintain a flag indicating
whether that reference has been taken.  When a suspend request
arrives, we drop that reference and clear the flag.

In most places we simply set or clear the extra-reference flag.
Instead--primarily to catch coding errors--test the previous value
of the flag and report an error in the event the previous value is
unexpected.  And if the clock reference is already taken, don't take
another.

In a couple of cases it's pretty clear atomic access is not
necessary and an error should never be reported.  Report these
anyway, conveying our surprise with an added exclamation point.

Signed-off-by: Alex Elder <elder@linaro.org>
---
v2: Updated to operate on a bitmap bit rather than an atomic_t.

 drivers/net/ipa/ipa_main.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index 409375b96eb8f..cfdf60ded86ca 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -83,6 +83,7 @@ static void ipa_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
 	/* Take a a single clock reference to prevent suspend.  All
 	 * endpoints will be resumed as a result.  This reference will
 	 * be dropped when we get a power management suspend request.
+	 * The first call activates the clock; ignore any others.
 	 */
 	if (!test_and_set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
 		ipa_clock_get(ipa);
@@ -502,14 +503,17 @@ static void ipa_resource_deconfig(struct ipa *ipa)
  */
 static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
 {
+	struct device *dev = &ipa->pdev->dev;
 	int ret;
 
 	/* Get a clock reference to allow initialization.  This reference
 	 * is held after initialization completes, and won't get dropped
 	 * unless/until a system suspend request arrives.
 	 */
-	__set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
-	ipa_clock_get(ipa);
+	if (!__test_and_set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
+		ipa_clock_get(ipa);
+	else
+		dev_err(dev, "suspend clock reference already taken!\n");
 
 	ipa_hardware_config(ipa);
 
@@ -544,7 +548,8 @@ static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
 err_hardware_deconfig:
 	ipa_hardware_deconfig(ipa);
 	ipa_clock_put(ipa);
-	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
+	if (!__test_and_clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
+		dev_err(dev, "suspend clock reference already dropped!\n");
 
 	return ret;
 }
@@ -562,7 +567,8 @@ static void ipa_deconfig(struct ipa *ipa)
 	ipa_endpoint_deconfig(ipa);
 	ipa_hardware_deconfig(ipa);
 	ipa_clock_put(ipa);
-	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
+	if (!test_and_clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
+		dev_err(&ipa->pdev->dev, "no suspend clock reference\n");
 }
 
 static int ipa_firmware_load(struct device *dev)
@@ -913,7 +919,8 @@ static int ipa_suspend(struct device *dev)
 	struct ipa *ipa = dev_get_drvdata(dev);
 
 	ipa_clock_put(ipa);
-	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
+	if (!test_and_clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
+		dev_err(dev, "suspend: missing suspend clock reference\n");
 
 	return 0;
 }
@@ -933,8 +940,10 @@ static int ipa_resume(struct device *dev)
 	/* This clock reference will keep the IPA out of suspend
 	 * until we get a power management suspend request.
 	 */
-	__set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
-	ipa_clock_get(ipa);
+	if (!test_and_set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
+		ipa_clock_get(ipa);
+	else
+		dev_err(dev, "resume: duplicate suspend clock reference\n");
 
 	return 0;
 }
-- 
2.20.1

[PATCH net-next v2 4/7] net: ipa: manage endpoints separate from clock

[Alex Elder]

Currently, when (before) the last IPA clock reference is dropped,
all endpoints are suspended.  And whenever the first IPA clock
reference is taken, all endpoints are resumed (or started).

In most cases there's no need to start endpoints when the clock
starts.  So move the calls to ipa_endpoint_suspend() and
ipa_endpoint_resume() out of ipa_clock_put() and ipa_clock_get(),
respectiely.  Instead, only suspend endpoints when handling a system
suspend, and only resume endpoints when handling a system resume.

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

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index b703866f2e20b..a2c0fde058199 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -200,9 +200,8 @@ bool ipa_clock_get_additional(struct ipa *ipa)
 
 /* Get an IPA clock reference.  If the reference count is non-zero, it is
  * incremented and return is immediate.  Otherwise it is checked again
- * under protection of the mutex, and if appropriate the clock (and
- * interconnects) are enabled suspended endpoints (if any) are resumed
- * before returning.
+ * under protection of the mutex, and if appropriate the IPA clock
+ * is enabled.
  *
  * Incrementing the reference count is intentionally deferred until
  * after the clock is running and endpoints are resumed.
@@ -229,17 +228,14 @@ void ipa_clock_get(struct ipa *ipa)
 		goto out_mutex_unlock;
 	}
 
-	ipa_endpoint_resume(ipa);
-
 	refcount_set(&clock->count, 1);
 
 out_mutex_unlock:
 	mutex_unlock(&clock->mutex);
 }
 
-/* Attempt to remove an IPA clock reference.  If this represents the last
- * reference, suspend endpoints and disable the clock (and interconnects)
- * under protection of a mutex.
+/* Attempt to remove an IPA clock reference.  If this represents the
+ * last reference, disable the IPA clock under protection of the mutex.
  */
 void ipa_clock_put(struct ipa *ipa)
 {
@@ -249,8 +245,6 @@ void ipa_clock_put(struct ipa *ipa)
 	if (!refcount_dec_and_mutex_lock(&clock->count, &clock->mutex))
 		return;
 
-	ipa_endpoint_suspend(ipa);
-
 	ipa_clock_disable(ipa);
 
 	mutex_unlock(&clock->mutex);
diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index cfdf60ded86ca..3b68b53c99015 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -913,11 +913,15 @@ static int ipa_remove(struct platform_device *pdev)
  * 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);
 
+	ipa_endpoint_suspend(ipa);
+
 	ipa_clock_put(ipa);
 	if (!test_and_clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
 		dev_err(dev, "suspend: missing suspend clock reference\n");
@@ -932,6 +936,8 @@ static int ipa_suspend(struct device *dev)
  * 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)
 {
@@ -945,6 +951,8 @@ static int ipa_resume(struct device *dev)
 	else
 		dev_err(dev, "resume: duplicate suspend clock reference\n");
 
+	ipa_endpoint_resume(ipa);
+
 	return 0;
 }
 
-- 
2.20.1

[PATCH net-next v2 5/7] net: ipa: use device_init_wakeup()

[Alex Elder]

The call to wakeup_source_register() in ipa_probe() does not do what
it was intended to do.  Call device_init_wakeup() in ipa_setup()
instead, to set the IPA device as wakeup-capable and to initially
enable wakeup capability.

When we receive a SUSPEND interrupt, call pm_wakeup_dev_event()
with a zero processing time, to simply call for a resume without
any other processing.  The ipa_resume() call will take care of
waking things up again, and will handle receiving the packet.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa.h      |  2 --
 drivers/net/ipa/ipa_main.c | 43 ++++++++++++++++----------------------
 2 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ipa/ipa.h b/drivers/net/ipa/ipa.h
index e02fe979b645b..c688155ccf375 100644
--- a/drivers/net/ipa/ipa.h
+++ b/drivers/net/ipa/ipa.h
@@ -114,8 +114,6 @@ struct ipa {
 	void *zero_virt;
 	size_t zero_size;
 
-	struct wakeup_source *wakeup_source;
-
 	/* Bit masks indicating endpoint state */
 	u32 available;		/* supported by hardware */
 	u32 filter_map;
diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index 3b68b53c99015..5e714d9d2e5cb 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -75,18 +75,19 @@
  * @ipa:	IPA pointer
  * @irq_id:	IPA interrupt type (unused)
  *
- * When in suspended state, the IPA can trigger a resume by sending a SUSPEND
- * IPA interrupt.
+ * 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)
 {
-	/* Take a a single clock reference to prevent suspend.  All
-	 * endpoints will be resumed as a result.  This reference will
-	 * be dropped when we get a power management suspend request.
-	 * The first call activates the clock; ignore any others.
+	/* 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_CLOCK_HELD, ipa->flags))
-		ipa_clock_get(ipa);
+		pm_wakeup_dev_event(&ipa->pdev->dev, 0, true);
 
 	/* Acknowledge/clear the suspend interrupt on all endpoints */
 	ipa_interrupt_suspend_clear_all(ipa->interrupt);
@@ -107,6 +108,7 @@ int ipa_setup(struct ipa *ipa)
 {
 	struct ipa_endpoint *exception_endpoint;
 	struct ipa_endpoint *command_endpoint;
+	struct device *dev = &ipa->pdev->dev;
 	int ret;
 
 	/* Setup for IPA v3.5.1 has some slight differences */
@@ -124,6 +126,10 @@ int ipa_setup(struct ipa *ipa)
 
 	ipa_uc_setup(ipa);
 
+	ret = device_init_wakeup(dev, true);
+	if (ret)
+		goto err_uc_teardown;
+
 	ipa_endpoint_setup(ipa);
 
 	/* We need to use the AP command TX endpoint to perform other
@@ -159,7 +165,7 @@ int ipa_setup(struct ipa *ipa)
 
 	ipa->setup_complete = true;
 
-	dev_info(&ipa->pdev->dev, "IPA driver setup completed successfully\n");
+	dev_info(dev, "IPA driver setup completed successfully\n");
 
 	return 0;
 
@@ -174,6 +180,8 @@ int ipa_setup(struct ipa *ipa)
 	ipa_endpoint_disable_one(command_endpoint);
 err_endpoint_teardown:
 	ipa_endpoint_teardown(ipa);
+	(void)device_init_wakeup(dev, false);
+err_uc_teardown:
 	ipa_uc_teardown(ipa);
 	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
 	ipa_interrupt_teardown(ipa->interrupt);
@@ -201,6 +209,7 @@ 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);
+	(void)device_init_wakeup(&ipa->pdev->dev, false);
 	ipa_uc_teardown(ipa);
 	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
 	ipa_interrupt_teardown(ipa->interrupt);
@@ -715,7 +724,6 @@ static void ipa_validate_build(void)
  */
 static int ipa_probe(struct platform_device *pdev)
 {
-	struct wakeup_source *wakeup_source;
 	struct device *dev = &pdev->dev;
 	const struct ipa_data *data;
 	struct ipa_clock *clock;
@@ -764,19 +772,11 @@ static int ipa_probe(struct platform_device *pdev)
 		goto err_clock_exit;
 	}
 
-	/* Create a wakeup source. */
-	wakeup_source = wakeup_source_register(dev, "ipa");
-	if (!wakeup_source) {
-		/* The most likely reason for failure is memory exhaustion */
-		ret = -ENOMEM;
-		goto err_clock_exit;
-	}
-
 	/* Allocate and initialize the IPA structure */
 	ipa = kzalloc(sizeof(*ipa), GFP_KERNEL);
 	if (!ipa) {
 		ret = -ENOMEM;
-		goto err_wakeup_source_unregister;
+		goto err_clock_exit;
 	}
 
 	ipa->pdev = pdev;
@@ -784,7 +784,6 @@ static int ipa_probe(struct platform_device *pdev)
 	ipa->modem_rproc = rproc;
 	ipa->clock = clock;
 	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
-	ipa->wakeup_source = wakeup_source;
 	ipa->version = data->version;
 
 	ret = ipa_reg_init(ipa);
@@ -863,8 +862,6 @@ static int ipa_probe(struct platform_device *pdev)
 	ipa_reg_exit(ipa);
 err_kfree_ipa:
 	kfree(ipa);
-err_wakeup_source_unregister:
-	wakeup_source_unregister(wakeup_source);
 err_clock_exit:
 	ipa_clock_exit(clock);
 err_rproc_put:
@@ -878,11 +875,8 @@ static int ipa_remove(struct platform_device *pdev)
 	struct ipa *ipa = dev_get_drvdata(&pdev->dev);
 	struct rproc *rproc = ipa->modem_rproc;
 	struct ipa_clock *clock = ipa->clock;
-	struct wakeup_source *wakeup_source;
 	int ret;
 
-	wakeup_source = ipa->wakeup_source;
-
 	if (ipa->setup_complete) {
 		ret = ipa_modem_stop(ipa);
 		if (ret)
@@ -899,7 +893,6 @@ static int ipa_remove(struct platform_device *pdev)
 	ipa_mem_exit(ipa);
 	ipa_reg_exit(ipa);
 	kfree(ipa);
-	wakeup_source_unregister(wakeup_source);
 	ipa_clock_exit(clock);
 	rproc_put(rproc);
 
-- 
2.20.1

[PATCH net-next v2 6/7] net: ipa: enable wakeup on IPA interrupt

[Alex Elder]

Now that we handle wakeup interrupts properly, arrange for the IPA
interrupt to be treated as a wakeup interrupt.

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

diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
index 90353987c45fc..cc1ea28f7bc2e 100644
--- a/drivers/net/ipa/ipa_interrupt.c
+++ b/drivers/net/ipa/ipa_interrupt.c
@@ -237,8 +237,16 @@ struct ipa_interrupt *ipa_interrupt_setup(struct ipa *ipa)
 		goto err_kfree;
 	}
 
+	ret = enable_irq_wake(irq);
+	if (ret) {
+		dev_err(dev, "error %d enabling wakeup for \"ipa\" IRQ\n", ret);
+		goto err_free_irq;
+	}
+
 	return interrupt;
 
+err_free_irq:
+	free_irq(interrupt->irq, interrupt);
 err_kfree:
 	kfree(interrupt);
 
@@ -248,6 +256,12 @@ struct ipa_interrupt *ipa_interrupt_setup(struct ipa *ipa)
 /* Tear down the IPA interrupt framework */
 void ipa_interrupt_teardown(struct ipa_interrupt *interrupt)
 {
+	struct device *dev = &interrupt->ipa->pdev->dev;
+	int ret;
+
+	ret = disable_irq_wake(interrupt->irq);
+	if (ret)
+		dev_err(dev, "error %d disabling \"ipa\" IRQ wakeup\n", ret);
 	free_irq(interrupt->irq, interrupt);
 	kfree(interrupt);
 }
-- 
2.20.1

[PATCH net-next v2 7/7] net: ipa: do not enable GSI interrupt for wakeup

[Alex Elder]

We now trigger a system resume when we receive an IPA SUSPEND
interrupt.  We should *not* wake up on GSI interrupts.

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

diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
index 0e63d35320aaf..cb75f7d540571 100644
--- a/drivers/net/ipa/gsi.c
+++ b/drivers/net/ipa/gsi.c
@@ -1987,31 +1987,26 @@ int gsi_init(struct gsi *gsi, struct platform_device *pdev, bool prefetch,
 	}
 	gsi->irq = irq;
 
-	ret = enable_irq_wake(gsi->irq);
-	if (ret)
-		dev_warn(dev, "error %d enabling gsi wake irq\n", ret);
-	gsi->irq_wake_enabled = !ret;
-
 	/* Get GSI memory range and map it */
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gsi");
 	if (!res) {
 		dev_err(dev, "DT error getting \"gsi\" memory property\n");
 		ret = -ENODEV;
-		goto err_disable_irq_wake;
+		goto err_free_irq;
 	}
 
 	size = resource_size(res);
 	if (res->start > U32_MAX || size > U32_MAX - res->start) {
 		dev_err(dev, "DT memory resource \"gsi\" out of range\n");
 		ret = -EINVAL;
-		goto err_disable_irq_wake;
+		goto err_free_irq;
 	}
 
 	gsi->virt = ioremap(res->start, size);
 	if (!gsi->virt) {
 		dev_err(dev, "unable to remap \"gsi\" memory\n");
 		ret = -ENOMEM;
-		goto err_disable_irq_wake;
+		goto err_free_irq;
 	}
 
 	ret = gsi_channel_init(gsi, prefetch, count, data, modem_alloc);
@@ -2025,9 +2020,7 @@ int gsi_init(struct gsi *gsi, struct platform_device *pdev, bool prefetch,
 
 err_iounmap:
 	iounmap(gsi->virt);
-err_disable_irq_wake:
-	if (gsi->irq_wake_enabled)
-		(void)disable_irq_wake(gsi->irq);
+err_free_irq:
 	free_irq(gsi->irq, gsi);
 
 	return ret;
@@ -2038,8 +2031,6 @@ void gsi_exit(struct gsi *gsi)
 {
 	mutex_destroy(&gsi->mutex);
 	gsi_channel_exit(gsi);
-	if (gsi->irq_wake_enabled)
-		(void)disable_irq_wake(gsi->irq);
 	free_irq(gsi->irq, gsi);
 	iounmap(gsi->virt);
 }
diff --git a/drivers/net/ipa/gsi.h b/drivers/net/ipa/gsi.h
index 061312773df09..3f9f29d531c43 100644
--- a/drivers/net/ipa/gsi.h
+++ b/drivers/net/ipa/gsi.h
@@ -150,7 +150,6 @@ struct gsi {
 	struct net_device dummy_dev;	/* needed for NAPI */
 	void __iomem *virt;
 	u32 irq;
-	bool irq_wake_enabled;
 	u32 channel_count;
 	u32 evt_ring_count;
 	struct gsi_channel channel[GSI_CHANNEL_COUNT_MAX];
-- 
2.20.1

Re: [PATCH net-next v2 3/7] net: ipa: verify reference flag values

[Bjorn Andersson]

On Fri 11 Sep 19:45 CDT 2020, Alex Elder wrote:

> We take a single IPA clock reference to keep the clock running until
> we get a system suspend operation, and maintain a flag indicating
> whether that reference has been taken.  When a suspend request
> arrives, we drop that reference and clear the flag.
> 
> In most places we simply set or clear the extra-reference flag.
> Instead--primarily to catch coding errors--test the previous value
> of the flag and report an error in the event the previous value is
> unexpected.  And if the clock reference is already taken, don't take
> another.
> 
> In a couple of cases it's pretty clear atomic access is not
> necessary and an error should never be reported.  Report these
> anyway, conveying our surprise with an added exclamation point.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
> v2: Updated to operate on a bitmap bit rather than an atomic_t.
> 
>  drivers/net/ipa/ipa_main.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index 409375b96eb8f..cfdf60ded86ca 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c
> @@ -83,6 +83,7 @@ static void ipa_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
>  	/* Take a a single clock reference to prevent suspend.  All
>  	 * endpoints will be resumed as a result.  This reference will
>  	 * be dropped when we get a power management suspend request.
> +	 * The first call activates the clock; ignore any others.
>  	 */
>  	if (!test_and_set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
>  		ipa_clock_get(ipa);
> @@ -502,14 +503,17 @@ static void ipa_resource_deconfig(struct ipa *ipa)
>   */
>  static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
>  {
> +	struct device *dev = &ipa->pdev->dev;
>  	int ret;
>  
>  	/* Get a clock reference to allow initialization.  This reference
>  	 * is held after initialization completes, and won't get dropped
>  	 * unless/until a system suspend request arrives.
>  	 */
> -	__set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
> -	ipa_clock_get(ipa);
> +	if (!__test_and_set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
> +		ipa_clock_get(ipa);
> +	else
> +		dev_err(dev, "suspend clock reference already taken!\n");
>  
>  	ipa_hardware_config(ipa);
>  
> @@ -544,7 +548,8 @@ static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
>  err_hardware_deconfig:
>  	ipa_hardware_deconfig(ipa);
>  	ipa_clock_put(ipa);
> -	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
> +	if (!__test_and_clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
> +		dev_err(dev, "suspend clock reference already dropped!\n");
>  
>  	return ret;
>  }
> @@ -562,7 +567,8 @@ static void ipa_deconfig(struct ipa *ipa)
>  	ipa_endpoint_deconfig(ipa);
>  	ipa_hardware_deconfig(ipa);
>  	ipa_clock_put(ipa);
> -	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
> +	if (!test_and_clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))

Doesn't this imply that we ran with the clocks disabled, which
presumably would have nasty side effects?

This seems like something that is worthy of more than just a simple
printout - which no one will actually read.  If you instead use a
WARN_ON() to highlight this at least some of the test environments out
there will pick it up and report it...

Regards,
Bjorn

> +		dev_err(&ipa->pdev->dev, "no suspend clock reference\n");
>  }
>  
>  static int ipa_firmware_load(struct device *dev)
> @@ -913,7 +919,8 @@ static int ipa_suspend(struct device *dev)
>  	struct ipa *ipa = dev_get_drvdata(dev);
>  
>  	ipa_clock_put(ipa);
> -	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
> +	if (!test_and_clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
> +		dev_err(dev, "suspend: missing suspend clock reference\n");
>  
>  	return 0;
>  }
> @@ -933,8 +940,10 @@ static int ipa_resume(struct device *dev)
>  	/* This clock reference will keep the IPA out of suspend
>  	 * until we get a power management suspend request.
>  	 */
> -	__set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
> -	ipa_clock_get(ipa);
> +	if (!test_and_set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
> +		ipa_clock_get(ipa);
> +	else
> +		dev_err(dev, "resume: duplicate suspend clock reference\n");
>  
>  	return 0;
>  }
> -- 
> 2.20.1
> 

Re: [PATCH net-next v2 5/7] net: ipa: use device_init_wakeup()

[Bjorn Andersson]

On Fri 11 Sep 19:45 CDT 2020, Alex Elder wrote:

> The call to wakeup_source_register() in ipa_probe() does not do what
> it was intended to do.  Call device_init_wakeup() in ipa_setup()
> instead, to set the IPA device as wakeup-capable and to initially
> enable wakeup capability.
> 
> When we receive a SUSPEND interrupt, call pm_wakeup_dev_event()
> with a zero processing time, to simply call for a resume without
> any other processing.  The ipa_resume() call will take care of
> waking things up again, and will handle receiving the packet.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ipa/ipa.h      |  2 --
>  drivers/net/ipa/ipa_main.c | 43 ++++++++++++++++----------------------
>  2 files changed, 18 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa.h b/drivers/net/ipa/ipa.h
> index e02fe979b645b..c688155ccf375 100644
> --- a/drivers/net/ipa/ipa.h
> +++ b/drivers/net/ipa/ipa.h
> @@ -114,8 +114,6 @@ struct ipa {
>  	void *zero_virt;
>  	size_t zero_size;
>  
> -	struct wakeup_source *wakeup_source;
> -
>  	/* Bit masks indicating endpoint state */
>  	u32 available;		/* supported by hardware */
>  	u32 filter_map;
> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index 3b68b53c99015..5e714d9d2e5cb 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c
> @@ -75,18 +75,19 @@
>   * @ipa:	IPA pointer
>   * @irq_id:	IPA interrupt type (unused)
>   *
> - * When in suspended state, the IPA can trigger a resume by sending a SUSPEND
> - * IPA interrupt.
> + * 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)
>  {
> -	/* Take a a single clock reference to prevent suspend.  All
> -	 * endpoints will be resumed as a result.  This reference will
> -	 * be dropped when we get a power management suspend request.
> -	 * The first call activates the clock; ignore any others.
> +	/* 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_CLOCK_HELD, ipa->flags))
> -		ipa_clock_get(ipa);
> +		pm_wakeup_dev_event(&ipa->pdev->dev, 0, true);
>  
>  	/* Acknowledge/clear the suspend interrupt on all endpoints */
>  	ipa_interrupt_suspend_clear_all(ipa->interrupt);
> @@ -107,6 +108,7 @@ int ipa_setup(struct ipa *ipa)
>  {
>  	struct ipa_endpoint *exception_endpoint;
>  	struct ipa_endpoint *command_endpoint;
> +	struct device *dev = &ipa->pdev->dev;
>  	int ret;
>  
>  	/* Setup for IPA v3.5.1 has some slight differences */
> @@ -124,6 +126,10 @@ int ipa_setup(struct ipa *ipa)
>  
>  	ipa_uc_setup(ipa);
>  
> +	ret = device_init_wakeup(dev, true);
> +	if (ret)
> +		goto err_uc_teardown;
> +
>  	ipa_endpoint_setup(ipa);
>  
>  	/* We need to use the AP command TX endpoint to perform other
> @@ -159,7 +165,7 @@ int ipa_setup(struct ipa *ipa)
>  
>  	ipa->setup_complete = true;
>  
> -	dev_info(&ipa->pdev->dev, "IPA driver setup completed successfully\n");
> +	dev_info(dev, "IPA driver setup completed successfully\n");
>  
>  	return 0;
>  
> @@ -174,6 +180,8 @@ int ipa_setup(struct ipa *ipa)
>  	ipa_endpoint_disable_one(command_endpoint);
>  err_endpoint_teardown:
>  	ipa_endpoint_teardown(ipa);
> +	(void)device_init_wakeup(dev, false);
> +err_uc_teardown:
>  	ipa_uc_teardown(ipa);
>  	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
>  	ipa_interrupt_teardown(ipa->interrupt);
> @@ -201,6 +209,7 @@ 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);
> +	(void)device_init_wakeup(&ipa->pdev->dev, false);
>  	ipa_uc_teardown(ipa);
>  	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
>  	ipa_interrupt_teardown(ipa->interrupt);
> @@ -715,7 +724,6 @@ static void ipa_validate_build(void)
>   */
>  static int ipa_probe(struct platform_device *pdev)
>  {
> -	struct wakeup_source *wakeup_source;
>  	struct device *dev = &pdev->dev;
>  	const struct ipa_data *data;
>  	struct ipa_clock *clock;
> @@ -764,19 +772,11 @@ static int ipa_probe(struct platform_device *pdev)
>  		goto err_clock_exit;
>  	}
>  
> -	/* Create a wakeup source. */
> -	wakeup_source = wakeup_source_register(dev, "ipa");
> -	if (!wakeup_source) {
> -		/* The most likely reason for failure is memory exhaustion */
> -		ret = -ENOMEM;
> -		goto err_clock_exit;
> -	}
> -
>  	/* Allocate and initialize the IPA structure */
>  	ipa = kzalloc(sizeof(*ipa), GFP_KERNEL);
>  	if (!ipa) {
>  		ret = -ENOMEM;
> -		goto err_wakeup_source_unregister;
> +		goto err_clock_exit;
>  	}
>  
>  	ipa->pdev = pdev;
> @@ -784,7 +784,6 @@ static int ipa_probe(struct platform_device *pdev)
>  	ipa->modem_rproc = rproc;
>  	ipa->clock = clock;
>  	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
> -	ipa->wakeup_source = wakeup_source;
>  	ipa->version = data->version;
>  
>  	ret = ipa_reg_init(ipa);
> @@ -863,8 +862,6 @@ static int ipa_probe(struct platform_device *pdev)
>  	ipa_reg_exit(ipa);
>  err_kfree_ipa:
>  	kfree(ipa);
> -err_wakeup_source_unregister:
> -	wakeup_source_unregister(wakeup_source);
>  err_clock_exit:
>  	ipa_clock_exit(clock);
>  err_rproc_put:
> @@ -878,11 +875,8 @@ static int ipa_remove(struct platform_device *pdev)
>  	struct ipa *ipa = dev_get_drvdata(&pdev->dev);
>  	struct rproc *rproc = ipa->modem_rproc;
>  	struct ipa_clock *clock = ipa->clock;
> -	struct wakeup_source *wakeup_source;
>  	int ret;
>  
> -	wakeup_source = ipa->wakeup_source;
> -
>  	if (ipa->setup_complete) {
>  		ret = ipa_modem_stop(ipa);
>  		if (ret)
> @@ -899,7 +893,6 @@ static int ipa_remove(struct platform_device *pdev)
>  	ipa_mem_exit(ipa);
>  	ipa_reg_exit(ipa);
>  	kfree(ipa);
> -	wakeup_source_unregister(wakeup_source);
>  	ipa_clock_exit(clock);
>  	rproc_put(rproc);
>  
> -- 
> 2.20.1
> 

Re: [PATCH net-next v2 4/7] net: ipa: manage endpoints separate from clock

[Bjorn Andersson]

On Fri 11 Sep 19:45 CDT 2020, Alex Elder wrote:

> Currently, when (before) the last IPA clock reference is dropped,
> all endpoints are suspended.  And whenever the first IPA clock
> reference is taken, all endpoints are resumed (or started).
> 
> In most cases there's no need to start endpoints when the clock
> starts.  So move the calls to ipa_endpoint_suspend() and
> ipa_endpoint_resume() out of ipa_clock_put() and ipa_clock_get(),
> respectiely.  Instead, only suspend endpoints when handling a system
> suspend, and only resume endpoints when handling a system resume.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ipa/ipa_clock.c | 14 ++++----------
>  drivers/net/ipa/ipa_main.c  |  8 ++++++++
>  2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
> index b703866f2e20b..a2c0fde058199 100644
> --- a/drivers/net/ipa/ipa_clock.c
> +++ b/drivers/net/ipa/ipa_clock.c
> @@ -200,9 +200,8 @@ bool ipa_clock_get_additional(struct ipa *ipa)
>  
>  /* Get an IPA clock reference.  If the reference count is non-zero, it is
>   * incremented and return is immediate.  Otherwise it is checked again
> - * under protection of the mutex, and if appropriate the clock (and
> - * interconnects) are enabled suspended endpoints (if any) are resumed
> - * before returning.
> + * under protection of the mutex, and if appropriate the IPA clock
> + * is enabled.
>   *
>   * Incrementing the reference count is intentionally deferred until
>   * after the clock is running and endpoints are resumed.
> @@ -229,17 +228,14 @@ void ipa_clock_get(struct ipa *ipa)
>  		goto out_mutex_unlock;
>  	}
>  
> -	ipa_endpoint_resume(ipa);
> -
>  	refcount_set(&clock->count, 1);
>  
>  out_mutex_unlock:
>  	mutex_unlock(&clock->mutex);
>  }
>  
> -/* Attempt to remove an IPA clock reference.  If this represents the last
> - * reference, suspend endpoints and disable the clock (and interconnects)
> - * under protection of a mutex.
> +/* Attempt to remove an IPA clock reference.  If this represents the
> + * last reference, disable the IPA clock under protection of the mutex.
>   */
>  void ipa_clock_put(struct ipa *ipa)
>  {
> @@ -249,8 +245,6 @@ void ipa_clock_put(struct ipa *ipa)
>  	if (!refcount_dec_and_mutex_lock(&clock->count, &clock->mutex))
>  		return;
>  
> -	ipa_endpoint_suspend(ipa);
> -
>  	ipa_clock_disable(ipa);
>  
>  	mutex_unlock(&clock->mutex);
> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
> index cfdf60ded86ca..3b68b53c99015 100644
> --- a/drivers/net/ipa/ipa_main.c
> +++ b/drivers/net/ipa/ipa_main.c
> @@ -913,11 +913,15 @@ static int ipa_remove(struct platform_device *pdev)
>   * 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);
>  
> +	ipa_endpoint_suspend(ipa);
> +
>  	ipa_clock_put(ipa);
>  	if (!test_and_clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
>  		dev_err(dev, "suspend: missing suspend clock reference\n");
> @@ -932,6 +936,8 @@ static int ipa_suspend(struct device *dev)
>   * 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)
>  {
> @@ -945,6 +951,8 @@ static int ipa_resume(struct device *dev)
>  	else
>  		dev_err(dev, "resume: duplicate suspend clock reference\n");
>  
> +	ipa_endpoint_resume(ipa);
> +
>  	return 0;
>  }
>  
> -- 
> 2.20.1
> 

Re: [PATCH net-next v2 6/7] net: ipa: enable wakeup on IPA interrupt

[Bjorn Andersson]

On Fri 11 Sep 19:45 CDT 2020, Alex Elder wrote:

> Now that we handle wakeup interrupts properly, arrange for the IPA
> interrupt to be treated as a wakeup interrupt.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ipa/ipa_interrupt.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
> index 90353987c45fc..cc1ea28f7bc2e 100644
> --- a/drivers/net/ipa/ipa_interrupt.c
> +++ b/drivers/net/ipa/ipa_interrupt.c
> @@ -237,8 +237,16 @@ struct ipa_interrupt *ipa_interrupt_setup(struct ipa *ipa)
>  		goto err_kfree;
>  	}
>  
> +	ret = enable_irq_wake(irq);
> +	if (ret) {
> +		dev_err(dev, "error %d enabling wakeup for \"ipa\" IRQ\n", ret);
> +		goto err_free_irq;
> +	}
> +
>  	return interrupt;
>  
> +err_free_irq:
> +	free_irq(interrupt->irq, interrupt);
>  err_kfree:
>  	kfree(interrupt);
>  
> @@ -248,6 +256,12 @@ struct ipa_interrupt *ipa_interrupt_setup(struct ipa *ipa)
>  /* Tear down the IPA interrupt framework */
>  void ipa_interrupt_teardown(struct ipa_interrupt *interrupt)
>  {
> +	struct device *dev = &interrupt->ipa->pdev->dev;
> +	int ret;
> +
> +	ret = disable_irq_wake(interrupt->irq);
> +	if (ret)
> +		dev_err(dev, "error %d disabling \"ipa\" IRQ wakeup\n", ret);
>  	free_irq(interrupt->irq, interrupt);
>  	kfree(interrupt);
>  }
> -- 
> 2.20.1
> 

Re: [PATCH net-next v2 7/7] net: ipa: do not enable GSI interrupt for wakeup

[Bjorn Andersson]

On Fri 11 Sep 19:45 CDT 2020, Alex Elder wrote:

> We now trigger a system resume when we receive an IPA SUSPEND
> interrupt.  We should *not* wake up on GSI interrupts.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>  drivers/net/ipa/gsi.c | 17 ++++-------------
>  drivers/net/ipa/gsi.h |  1 -
>  2 files changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c
> index 0e63d35320aaf..cb75f7d540571 100644
> --- a/drivers/net/ipa/gsi.c
> +++ b/drivers/net/ipa/gsi.c
> @@ -1987,31 +1987,26 @@ int gsi_init(struct gsi *gsi, struct platform_device *pdev, bool prefetch,
>  	}
>  	gsi->irq = irq;
>  
> -	ret = enable_irq_wake(gsi->irq);
> -	if (ret)
> -		dev_warn(dev, "error %d enabling gsi wake irq\n", ret);
> -	gsi->irq_wake_enabled = !ret;
> -
>  	/* Get GSI memory range and map it */
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "gsi");
>  	if (!res) {
>  		dev_err(dev, "DT error getting \"gsi\" memory property\n");
>  		ret = -ENODEV;
> -		goto err_disable_irq_wake;
> +		goto err_free_irq;
>  	}
>  
>  	size = resource_size(res);
>  	if (res->start > U32_MAX || size > U32_MAX - res->start) {
>  		dev_err(dev, "DT memory resource \"gsi\" out of range\n");
>  		ret = -EINVAL;
> -		goto err_disable_irq_wake;
> +		goto err_free_irq;
>  	}
>  
>  	gsi->virt = ioremap(res->start, size);
>  	if (!gsi->virt) {
>  		dev_err(dev, "unable to remap \"gsi\" memory\n");
>  		ret = -ENOMEM;
> -		goto err_disable_irq_wake;
> +		goto err_free_irq;
>  	}
>  
>  	ret = gsi_channel_init(gsi, prefetch, count, data, modem_alloc);
> @@ -2025,9 +2020,7 @@ int gsi_init(struct gsi *gsi, struct platform_device *pdev, bool prefetch,
>  
>  err_iounmap:
>  	iounmap(gsi->virt);
> -err_disable_irq_wake:
> -	if (gsi->irq_wake_enabled)
> -		(void)disable_irq_wake(gsi->irq);
> +err_free_irq:
>  	free_irq(gsi->irq, gsi);
>  
>  	return ret;
> @@ -2038,8 +2031,6 @@ void gsi_exit(struct gsi *gsi)
>  {
>  	mutex_destroy(&gsi->mutex);
>  	gsi_channel_exit(gsi);
> -	if (gsi->irq_wake_enabled)
> -		(void)disable_irq_wake(gsi->irq);
>  	free_irq(gsi->irq, gsi);
>  	iounmap(gsi->virt);
>  }
> diff --git a/drivers/net/ipa/gsi.h b/drivers/net/ipa/gsi.h
> index 061312773df09..3f9f29d531c43 100644
> --- a/drivers/net/ipa/gsi.h
> +++ b/drivers/net/ipa/gsi.h
> @@ -150,7 +150,6 @@ struct gsi {
>  	struct net_device dummy_dev;	/* needed for NAPI */
>  	void __iomem *virt;
>  	u32 irq;
> -	bool irq_wake_enabled;
>  	u32 channel_count;
>  	u32 evt_ring_count;
>  	struct gsi_channel channel[GSI_CHANNEL_COUNT_MAX];
> -- 
> 2.20.1
> 

Re: [PATCH net-next v2 3/7] net: ipa: verify reference flag values

[Alex Elder]

On 9/12/20 9:25 PM, Bjorn Andersson wrote:
> On Fri 11 Sep 19:45 CDT 2020, Alex Elder wrote:
> 
>> We take a single IPA clock reference to keep the clock running until
>> we get a system suspend operation, and maintain a flag indicating
>> whether that reference has been taken.  When a suspend request
>> arrives, we drop that reference and clear the flag.
>>
>> In most places we simply set or clear the extra-reference flag.
>> Instead--primarily to catch coding errors--test the previous value
>> of the flag and report an error in the event the previous value is
>> unexpected.  And if the clock reference is already taken, don't take
>> another.
>>
>> In a couple of cases it's pretty clear atomic access is not
>> necessary and an error should never be reported.  Report these
>> anyway, conveying our surprise with an added exclamation point.
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>> v2: Updated to operate on a bitmap bit rather than an atomic_t.
>>
>>  drivers/net/ipa/ipa_main.c | 23 ++++++++++++++++-------
>>  1 file changed, 16 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
>> index 409375b96eb8f..cfdf60ded86ca 100644
>> --- a/drivers/net/ipa/ipa_main.c
>> +++ b/drivers/net/ipa/ipa_main.c
>> @@ -83,6 +83,7 @@ static void ipa_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
>>  	/* Take a a single clock reference to prevent suspend.  All
>>  	 * endpoints will be resumed as a result.  This reference will
>>  	 * be dropped when we get a power management suspend request.
>> +	 * The first call activates the clock; ignore any others.
>>  	 */
>>  	if (!test_and_set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
>>  		ipa_clock_get(ipa);
>> @@ -502,14 +503,17 @@ static void ipa_resource_deconfig(struct ipa *ipa)
>>   */
>>  static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
>>  {
>> +	struct device *dev = &ipa->pdev->dev;
>>  	int ret;
>>  
>>  	/* Get a clock reference to allow initialization.  This reference
>>  	 * is held after initialization completes, and won't get dropped
>>  	 * unless/until a system suspend request arrives.
>>  	 */
>> -	__set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
>> -	ipa_clock_get(ipa);
>> +	if (!__test_and_set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
>> +		ipa_clock_get(ipa);
>> +	else
>> +		dev_err(dev, "suspend clock reference already taken!\n");
>>  
>>  	ipa_hardware_config(ipa);
>>  
>> @@ -544,7 +548,8 @@ static int ipa_config(struct ipa *ipa, const struct ipa_data *data)
>>  err_hardware_deconfig:
>>  	ipa_hardware_deconfig(ipa);
>>  	ipa_clock_put(ipa);
>> -	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
>> +	if (!__test_and_clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
>> +		dev_err(dev, "suspend clock reference already dropped!\n");
>>  
>>  	return ret;
>>  }
>> @@ -562,7 +567,8 @@ static void ipa_deconfig(struct ipa *ipa)
>>  	ipa_endpoint_deconfig(ipa);
>>  	ipa_hardware_deconfig(ipa);
>>  	ipa_clock_put(ipa);
>> -	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
>> +	if (!test_and_clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
> 
> Doesn't this imply that we ran with the clocks disabled, which
> presumably would have nasty side effects?

Yes.  This is one of those that I mentioned "can't happen"
but I added the check anyway.

We call ipa_config() as the last step of ipa_probe().  The inverse
of ipa_config() is ipa_deconfig(), and that is called in two cases:
- If the AP is loading firmware, it does so *after* ipa_config()
  has been called and returned success.  If firmware loading fails,
  ipa_deconfig() is called in the error path to clean up.  If we
  never reached ipa_config() in the probe function, we will never
  call ipa_deconfig() in the error path.
- If ipa_config() fails when called in ipa_probe(), it will clean
  up all changed state and return an error value.  I *assume* that
  if the ->probe function returns an error, the ->remove function
  will never be called.  So again, we will never call ipa_deconfig()
  unless ipa_config() has been called.

That's the reasoning anyway.  That being said, you make a very
good point, in that the whole purpose of checking this at all
is to catch coding errors, and a WARN() call would provide much
better information than just an error message would.

So I will plan to update this in a new version of this patch
(and series).  I'll wait until tonight or tomorrow to see if
there is any other feedback before preparing that.

Thanks a lot.

					-Alex

> This seems like something that is worthy of more than just a simple
> printout - which no one will actually read.  If you instead use a
> WARN_ON() to highlight this at least some of the test environments out
> there will pick it up and report it...
> 
> Regards,
> Bjorn
> 
>> +		dev_err(&ipa->pdev->dev, "no suspend clock reference\n");
>>  }
>>  
>>  static int ipa_firmware_load(struct device *dev)
>> @@ -913,7 +919,8 @@ static int ipa_suspend(struct device *dev)
>>  	struct ipa *ipa = dev_get_drvdata(dev);
>>  
>>  	ipa_clock_put(ipa);
>> -	__clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
>> +	if (!test_and_clear_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
>> +		dev_err(dev, "suspend: missing suspend clock reference\n");
>>  
>>  	return 0;
>>  }
>> @@ -933,8 +940,10 @@ static int ipa_resume(struct device *dev)
>>  	/* This clock reference will keep the IPA out of suspend
>>  	 * until we get a power management suspend request.
>>  	 */
>> -	__set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags);
>> -	ipa_clock_get(ipa);
>> +	if (!test_and_set_bit(IPA_FLAG_CLOCK_HELD, ipa->flags))
>> +		ipa_clock_get(ipa);
>> +	else
>> +		dev_err(dev, "resume: duplicate suspend clock reference\n");
>>  
>>  	return 0;
>>  }
>> -- 
>> 2.20.1
>>