linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/5] net: ipa: wake up system on RX available
@ 2020-09-09  0:21 Alex Elder
  2020-09-09  0:21 ` [PATCH net-next 1/5] net: ipa: use atomic exchange for suspend reference Alex Elder
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Alex Elder @ 2020-09-09  0:21 UTC (permalink / raw)
  To: davem, kuba
  Cc: evgreen, subashab, cpratapa, bjorn.andersson, netdev, linux-kernel

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.

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, take
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 patch just checks the previous value of a reference
counter used for suspend, as precaution to catch bugs.  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 (5):
  net: ipa: use atomic exchange for suspend reference
  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           |  2 --
 drivers/net/ipa/ipa_clock.c     |  4 ---
 drivers/net/ipa/ipa_interrupt.c | 14 ++++++++
 drivers/net/ipa/ipa_main.c      | 62 ++++++++++++++++++---------------
 6 files changed, 51 insertions(+), 49 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH net-next 1/5] net: ipa: use atomic exchange for suspend reference
  2020-09-09  0:21 [PATCH net-next 0/5] net: ipa: wake up system on RX available Alex Elder
@ 2020-09-09  0:21 ` Alex Elder
  2020-09-09  3:27   ` David Miller
  2020-09-09  0:21 ` [PATCH net-next 2/5] net: ipa: manage endpoints separate from clock Alex Elder
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Alex Elder @ 2020-09-09  0:21 UTC (permalink / raw)
  To: davem, kuba
  Cc: evgreen, subashab, cpratapa, bjorn.andersson, netdev, linux-kernel

We take a single IPA clock reference to keep the clock running
until we get a system suspend operation.  When a system suspend
request arrives, we drop that reference, and if that's the last
reference (likely) we'll proceed with suspending endpoints and
disabling the IPA core clock and interconnects.

In most places we simply set the reference count to 0 or 1
atomically.  Instead--primarily to catch coding errors--use an
atomic exchange to update the reference count value, and report
an error in the event the previous value was unexpected.

In a few cases it's not hard to see that the error message should
never be reported.  Report them anyway, but add some excitement
to the message by ending it with an exclamation point.

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

diff --git a/drivers/net/ipa/ipa_main.c b/drivers/net/ipa/ipa_main.c
index 1fdfec41e4421..6b843fc989122 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 (!atomic_xchg(&ipa->suspend_ref, 1))
 		ipa_clock_get(ipa);
@@ -502,13 +503,15 @@ 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.
 	 */
-	atomic_set(&ipa->suspend_ref, 1);
+	if (atomic_xchg(&ipa->suspend_ref, 1))
+		dev_err(dev, "suspend clock reference already taken!\n");
 	ipa_clock_get(ipa);
 
 	ipa_hardware_config(ipa);
@@ -544,7 +547,8 @@ 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);
+	if (!atomic_xchg(&ipa->suspend_ref, 0))
+		dev_err(dev, "suspend clock reference already dropped!\n");
 
 	return ret;
 }
@@ -562,7 +566,8 @@ 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);
+	if (!atomic_xchg(&ipa->suspend_ref, 0))
+		dev_err(&ipa->pdev->dev, "no suspend clock reference\n");
 }
 
 static int ipa_firmware_load(struct device *dev)
@@ -913,7 +918,8 @@ static int ipa_suspend(struct device *dev)
 	struct ipa *ipa = dev_get_drvdata(dev);
 
 	ipa_clock_put(ipa);
-	atomic_set(&ipa->suspend_ref, 0);
+	if (!atomic_xchg(&ipa->suspend_ref, 0))
+		dev_err(dev, "suspend: missing suspend clock reference\n");
 
 	return 0;
 }
@@ -933,7 +939,8 @@ 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);
+	if (atomic_xchg(&ipa->suspend_ref, 1))
+		dev_err(dev, "resume: duplicate suspend clock reference\n");
 	ipa_clock_get(ipa);
 
 	return 0;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next 2/5] net: ipa: manage endpoints separate from clock
  2020-09-09  0:21 [PATCH net-next 0/5] net: ipa: wake up system on RX available Alex Elder
  2020-09-09  0:21 ` [PATCH net-next 1/5] net: ipa: use atomic exchange for suspend reference Alex Elder
@ 2020-09-09  0:21 ` Alex Elder
  2020-09-09  0:21 ` [PATCH net-next 3/5] net: ipa: use device_init_wakeup() Alex Elder
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Alex Elder @ 2020-09-09  0:21 UTC (permalink / raw)
  To: davem, kuba
  Cc: evgreen, subashab, cpratapa, bjorn.andersson, netdev, linux-kernel

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 | 4 ----
 drivers/net/ipa/ipa_main.c  | 4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ipa/ipa_clock.c b/drivers/net/ipa/ipa_clock.c
index 398f2e47043d8..f2d61c35ef941 100644
--- a/drivers/net/ipa/ipa_clock.c
+++ b/drivers/net/ipa/ipa_clock.c
@@ -229,8 +229,6 @@ void ipa_clock_get(struct ipa *ipa)
 		goto out_mutex_unlock;
 	}
 
-	ipa_endpoint_resume(ipa);
-
 	atomic_inc(&clock->count);
 
 out_mutex_unlock:
@@ -249,8 +247,6 @@ void ipa_clock_put(struct ipa *ipa)
 	if (!atomic_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 6b843fc989122..b8e4a2532fc1a 100644
--- a/drivers/net/ipa/ipa_main.c
+++ b/drivers/net/ipa/ipa_main.c
@@ -917,6 +917,8 @@ static int ipa_suspend(struct device *dev)
 {
 	struct ipa *ipa = dev_get_drvdata(dev);
 
+	ipa_endpoint_suspend(ipa);
+
 	ipa_clock_put(ipa);
 	if (!atomic_xchg(&ipa->suspend_ref, 0))
 		dev_err(dev, "suspend: missing suspend clock reference\n");
@@ -943,6 +945,8 @@ static int ipa_resume(struct device *dev)
 		dev_err(dev, "resume: duplicate suspend clock reference\n");
 	ipa_clock_get(ipa);
 
+	ipa_endpoint_resume(ipa);
+
 	return 0;
 }
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next 3/5] net: ipa: use device_init_wakeup()
  2020-09-09  0:21 [PATCH net-next 0/5] net: ipa: wake up system on RX available Alex Elder
  2020-09-09  0:21 ` [PATCH net-next 1/5] net: ipa: use atomic exchange for suspend reference Alex Elder
  2020-09-09  0:21 ` [PATCH net-next 2/5] net: ipa: manage endpoints separate from clock Alex Elder
@ 2020-09-09  0:21 ` Alex Elder
  2020-09-09  0:21 ` [PATCH net-next 4/5] net: ipa: enable wakeup on IPA interrupt Alex Elder
  2020-09-09  0:21 ` [PATCH net-next 5/5] net: ipa: do not enable GSI interrupt for wakeup Alex Elder
  4 siblings, 0 replies; 10+ messages in thread
From: Alex Elder @ 2020-09-09  0:21 UTC (permalink / raw)
  To: davem, kuba
  Cc: evgreen, subashab, cpratapa, bjorn.andersson, netdev, linux-kernel

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 407fee841a9a8..a1adc308e030f 100644
--- a/drivers/net/ipa/ipa.h
+++ b/drivers/net/ipa/ipa.h
@@ -104,8 +104,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 b8e4a2532fc1a..92ea6a811ae31 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 (!atomic_xchg(&ipa->suspend_ref, 1))
-		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);
@@ -714,7 +723,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;
@@ -763,19 +771,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;
@@ -783,7 +783,6 @@ static int ipa_probe(struct platform_device *pdev)
 	ipa->modem_rproc = rproc;
 	ipa->clock = clock;
 	atomic_set(&ipa->suspend_ref, 0);
-	ipa->wakeup_source = wakeup_source;
 	ipa->version = data->version;
 
 	ret = ipa_reg_init(ipa);
@@ -862,8 +861,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:
@@ -877,11 +874,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)
@@ -898,7 +892,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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next 4/5] net: ipa: enable wakeup on IPA interrupt
  2020-09-09  0:21 [PATCH net-next 0/5] net: ipa: wake up system on RX available Alex Elder
                   ` (2 preceding siblings ...)
  2020-09-09  0:21 ` [PATCH net-next 3/5] net: ipa: use device_init_wakeup() Alex Elder
@ 2020-09-09  0:21 ` Alex Elder
  2020-09-09  0:21 ` [PATCH net-next 5/5] net: ipa: do not enable GSI interrupt for wakeup Alex Elder
  4 siblings, 0 replies; 10+ messages in thread
From: Alex Elder @ 2020-09-09  0:21 UTC (permalink / raw)
  To: davem, kuba
  Cc: evgreen, subashab, cpratapa, bjorn.andersson, netdev, linux-kernel

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next 5/5] net: ipa: do not enable GSI interrupt for wakeup
  2020-09-09  0:21 [PATCH net-next 0/5] net: ipa: wake up system on RX available Alex Elder
                   ` (3 preceding siblings ...)
  2020-09-09  0:21 ` [PATCH net-next 4/5] net: ipa: enable wakeup on IPA interrupt Alex Elder
@ 2020-09-09  0:21 ` Alex Elder
  4 siblings, 0 replies; 10+ messages in thread
From: Alex Elder @ 2020-09-09  0:21 UTC (permalink / raw)
  To: davem, kuba
  Cc: evgreen, subashab, cpratapa, bjorn.andersson, netdev, linux-kernel

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 1/5] net: ipa: use atomic exchange for suspend reference
  2020-09-09  0:21 ` [PATCH net-next 1/5] net: ipa: use atomic exchange for suspend reference Alex Elder
@ 2020-09-09  3:27   ` David Miller
  2020-09-09 13:43     ` Alex Elder
  0 siblings, 1 reply; 10+ messages in thread
From: David Miller @ 2020-09-09  3:27 UTC (permalink / raw)
  To: elder
  Cc: kuba, evgreen, subashab, cpratapa, bjorn.andersson, netdev, linux-kernel

From: Alex Elder <elder@linaro.org>
Date: Tue,  8 Sep 2020 19:21:23 -0500

> We take a single IPA clock reference to keep the clock running
> until we get a system suspend operation.  When a system suspend
> request arrives, we drop that reference, and if that's the last
> reference (likely) we'll proceed with suspending endpoints and
> disabling the IPA core clock and interconnects.
> 
> In most places we simply set the reference count to 0 or 1
> atomically.  Instead--primarily to catch coding errors--use an
> atomic exchange to update the reference count value, and report
> an error in the event the previous value was unexpected.
> 
> In a few cases it's not hard to see that the error message should
> never be reported.  Report them anyway, but add some excitement
> to the message by ending it with an exclamation point.
> 
> Signed-off-by: Alex Elder <elder@linaro.org>

Please use refcount_t if you're wanting to validate things like
this.

Thank you.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 1/5] net: ipa: use atomic exchange for suspend reference
  2020-09-09  3:27   ` David Miller
@ 2020-09-09 13:43     ` Alex Elder
  2020-09-09 21:14       ` David Miller
  0 siblings, 1 reply; 10+ messages in thread
From: Alex Elder @ 2020-09-09 13:43 UTC (permalink / raw)
  To: David Miller
  Cc: kuba, evgreen, subashab, cpratapa, bjorn.andersson, netdev, linux-kernel

On 9/8/20 10:27 PM, David Miller wrote:
> From: Alex Elder <elder@linaro.org>
> Date: Tue,  8 Sep 2020 19:21:23 -0500
> 
>> We take a single IPA clock reference to keep the clock running
>> until we get a system suspend operation.  When a system suspend
>> request arrives, we drop that reference, and if that's the last
>> reference (likely) we'll proceed with suspending endpoints and
>> disabling the IPA core clock and interconnects.
>>
>> In most places we simply set the reference count to 0 or 1
>> atomically.  Instead--primarily to catch coding errors--use an
>> atomic exchange to update the reference count value, and report
>> an error in the event the previous value was unexpected.
>>
>> In a few cases it's not hard to see that the error message should
>> never be reported.  Report them anyway, but add some excitement
>> to the message by ending it with an exclamation point.
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
> 
> Please use refcount_t if you're wanting to validate things like
> this.

There is exactly one reference here; the "reference" is
essentially a Boolean flag.  So the value is always either
0 or 1.

I can use refcount_dec_if_one() for the 1->0 transition,
but I'm not sure how I can do the 0->1 transition with
refcount_t.  I admit I might be missing something.

Would you like me to add refcount_inc_if_zero()?

Otherwise would you prefer a different naming convention
to use for this Boolean "reference count"?

Thanks.

					-Alex

> 
> Thank you.
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 1/5] net: ipa: use atomic exchange for suspend reference
  2020-09-09 13:43     ` Alex Elder
@ 2020-09-09 21:14       ` David Miller
  2020-09-09 21:23         ` Alex Elder
  0 siblings, 1 reply; 10+ messages in thread
From: David Miller @ 2020-09-09 21:14 UTC (permalink / raw)
  To: elder
  Cc: kuba, evgreen, subashab, cpratapa, bjorn.andersson, netdev, linux-kernel

From: Alex Elder <elder@linaro.org>
Date: Wed, 9 Sep 2020 08:43:44 -0500

> There is exactly one reference here; the "reference" is
> essentially a Boolean flag.  So the value is always either
> 0 or 1.

Aha, then why not use a bitmask and test_and_set_bit() et al.?

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next 1/5] net: ipa: use atomic exchange for suspend reference
  2020-09-09 21:14       ` David Miller
@ 2020-09-09 21:23         ` Alex Elder
  0 siblings, 0 replies; 10+ messages in thread
From: Alex Elder @ 2020-09-09 21:23 UTC (permalink / raw)
  To: David Miller
  Cc: kuba, evgreen, subashab, cpratapa, bjorn.andersson, netdev, linux-kernel

On 9/9/20 4:14 PM, David Miller wrote:
> From: Alex Elder <elder@linaro.org>
> Date: Wed, 9 Sep 2020 08:43:44 -0500
> 
>> There is exactly one reference here; the "reference" is
>> essentially a Boolean flag.  So the value is always either
>> 0 or 1.
> 
> Aha, then why not use a bitmask and test_and_set_bit() et al.?

OK I'll go take a look at that option.  It's overkill for
bits used but it makes it more obvious it's a single bit,
so it's probably a better idea.

I'll try to turn this around again by tomorrow.  Thank you.

					-Alex

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-09-09 21:23 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-09  0:21 [PATCH net-next 0/5] net: ipa: wake up system on RX available Alex Elder
2020-09-09  0:21 ` [PATCH net-next 1/5] net: ipa: use atomic exchange for suspend reference Alex Elder
2020-09-09  3:27   ` David Miller
2020-09-09 13:43     ` Alex Elder
2020-09-09 21:14       ` David Miller
2020-09-09 21:23         ` Alex Elder
2020-09-09  0:21 ` [PATCH net-next 2/5] net: ipa: manage endpoints separate from clock Alex Elder
2020-09-09  0:21 ` [PATCH net-next 3/5] net: ipa: use device_init_wakeup() Alex Elder
2020-09-09  0:21 ` [PATCH net-next 4/5] net: ipa: enable wakeup on IPA interrupt Alex Elder
2020-09-09  0:21 ` [PATCH net-next 5/5] net: ipa: do not enable GSI interrupt for wakeup Alex Elder

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).