[PATCH net-next 0/6] net: ipa: don't abort system suspend

[PATCH net-next 0/6] net: ipa: don't abort system suspend

[Alex Elder]

Currently the IPA code aborts an in-progress system suspend if an
IPA interrupt arrives before the suspend completes.  There is no
need to do that though, because the IPA driver handles a forced
suspend correctly, quiescing any hardware activity before finally 
turning off clocks and interconnects.

This series drops the call to pm_wakeup_dev_event() if an IPA
SUSPEND interrupt arrives during system suspend.  Doing this
makes the two remaining IPA power flags unnecessary, and allows
some additional code to be cleaned up--and best of all, removed.
The result is much simpler (and I'm really glad not to be using
these flags any more).

The first patch implements the main change.  The second and
third remove the flags that were used to determine whether to
call pm_wakeup_dev_event().  The next two remove a function that
becomes a trivial wrapper, and the last one just avoids writing
a register unnecessarily.

Note that the first two patches will have checkpatch warnings,
because checkpatch disagrees with my compiler on what to do when
a block contains only a semicolon.  I went with what the compiler
recommends.

clang says: warning: suggest braces around empty body
checkpatch: WARNING: braces {} are not necessary for single statement blocks

					-Alex

Alex Elder (6):
  net: ipa: don't bother aborting system resume
  net: ipa: kill IPA_POWER_FLAG_SYSTEM
  net: ipa: kill the IPA_POWER_FLAG_RESUMED flag
  net: ipa: move ipa_interrupt_suspend_clear_all() up
  net: ipa: kill ipa_power_suspend_handler()
  net: ipa: don't bother zeroing an already zero register

 drivers/net/ipa/ipa_interrupt.c | 50 ++++++++++++++++-----------------
 drivers/net/ipa/ipa_interrupt.h |  8 ------
 drivers/net/ipa/ipa_power.c     | 33 ----------------------
 drivers/net/ipa/ipa_power.h     | 11 --------
 4 files changed, 25 insertions(+), 77 deletions(-)

-- 
2.40.1

[PATCH net-next 1/6] net: ipa: don't bother aborting system resume

[Alex Elder]

The IPA interrupt can fire if there is data to be delivered to a GSI
channel that is suspended.  This condition occurs in three scenarios.

First, runtime power management automatically suspends the IPA
hardware after half a second of inactivity.  This has nothing
to do with system suspend, so a SYSTEM IPA power flag is used to
avoid calling pm_wakeup_dev_event() when runtime suspended.

Second, if the system is suspended, the receipt of an IPA interrupt
should trigger a system resume.  Configuring the IPA interrupt for
wakeup accomplishes this.

Finally, if system suspend is underway and the IPA interrupt fires,
we currently call pm_wakeup_dev_event() to abort the system suspend.

The IPA driver correctly handles quiescing the hardware before
suspending it, so there's really no need to abort a suspend in
progress in the third case.  We can simply quiesce and suspend
things, and be done.

Incoming data can still wake the system after it's suspended.
The IPA interrupt has wakeup mode enabled, so if it fires *after*
we've suspended, it will trigger a wakeup (if not disabled via
sysfs).

Stop calling pm_wakeup_dev_event() to abort a system suspend in
progress in ipa_power_suspend_handler().

Signed-off-by: Alex Elder <elder@linaro.org>
---
Note: checkpatch warns: braces {} are not necessary...

 drivers/net/ipa/ipa_power.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
index 128a816f65237..694bc71e0a170 100644
--- a/drivers/net/ipa/ipa_power.c
+++ b/drivers/net/ipa/ipa_power.c
@@ -220,8 +220,9 @@ void ipa_power_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
 	 * system suspend, trigger a system resume.
 	 */
 	if (!__test_and_set_bit(IPA_POWER_FLAG_RESUMED, ipa->power->flags))
-		if (test_bit(IPA_POWER_FLAG_SYSTEM, ipa->power->flags))
-			pm_wakeup_dev_event(&ipa->pdev->dev, 0, true);
+		if (test_bit(IPA_POWER_FLAG_SYSTEM, ipa->power->flags)) {
+			;
+		}
 
 	/* Acknowledge/clear the suspend interrupt on all endpoints */
 	ipa_interrupt_suspend_clear_all(ipa->interrupt);
-- 
2.40.1

[PATCH net-next 2/6] net: ipa: kill IPA_POWER_FLAG_SYSTEM

[Alex Elder]

The SYSTEM IPA power flag is set, cleared, and tested.  But nothing
happens based on its value when tested, so it serves no purpose.
Get rid of this flag.

Signed-off-by: Alex Elder <elder@linaro.org>
---
Note: checkpatch warns: braces {} are not necessary...

 drivers/net/ipa/ipa_power.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
index 694bc71e0a170..be9e859e853fb 100644
--- a/drivers/net/ipa/ipa_power.c
+++ b/drivers/net/ipa/ipa_power.c
@@ -37,12 +37,10 @@
 /**
  * enum ipa_power_flag - IPA power flags
  * @IPA_POWER_FLAG_RESUMED:	Whether resume from suspend has been signaled
- * @IPA_POWER_FLAG_SYSTEM:	Hardware is system (not runtime) suspended
  * @IPA_POWER_FLAG_COUNT:	Number of defined power flags
  */
 enum ipa_power_flag {
 	IPA_POWER_FLAG_RESUMED,
-	IPA_POWER_FLAG_SYSTEM,
 	IPA_POWER_FLAG_COUNT,		/* Last; not a flag */
 };
 
@@ -173,8 +171,6 @@ static int ipa_suspend(struct device *dev)
 {
 	struct ipa *ipa = dev_get_drvdata(dev);
 
-	__set_bit(IPA_POWER_FLAG_SYSTEM, ipa->power->flags);
-
 	/* Increment the disable depth to ensure that the IRQ won't
 	 * be re-enabled until the matching _enable call in
 	 * ipa_resume(). We do this to ensure that the interrupt
@@ -196,8 +192,6 @@ static int ipa_resume(struct device *dev)
 
 	ret = pm_runtime_force_resume(dev);
 
-	__clear_bit(IPA_POWER_FLAG_SYSTEM, ipa->power->flags);
-
 	/* Now that PM runtime is enabled again it's safe
 	 * to turn the IRQ back on and process any data
 	 * that was received during suspend.
@@ -219,10 +213,9 @@ void ipa_power_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
 	 * just to handle the interrupt, so we're done.  If we are in a
 	 * system suspend, trigger a system resume.
 	 */
-	if (!__test_and_set_bit(IPA_POWER_FLAG_RESUMED, ipa->power->flags))
-		if (test_bit(IPA_POWER_FLAG_SYSTEM, ipa->power->flags)) {
-			;
-		}
+	if (!__test_and_set_bit(IPA_POWER_FLAG_RESUMED, ipa->power->flags)) {
+		;
+	}
 
 	/* Acknowledge/clear the suspend interrupt on all endpoints */
 	ipa_interrupt_suspend_clear_all(ipa->interrupt);
-- 
2.40.1

[PATCH net-next 3/6] net: ipa: kill the IPA_POWER_FLAG_RESUMED flag

[Alex Elder]

The IPA_POWER_FLAG_RESUMED was originally used to avoid calling
pm_wakeup_dev_event() more than once when handling a SUSPEND
interrupt.  This call is no longer made, so there' no need for the
flag, so get rid of it.

That leaves no more IPA power flags usefully defined, so just get
rid of the bitmap in the IPA power structure and the definition of
the ipa_power_flag enumerated type.

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

diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
index be9e859e853fb..eee251d67f81a 100644
--- a/drivers/net/ipa/ipa_power.c
+++ b/drivers/net/ipa/ipa_power.c
@@ -34,22 +34,11 @@
 
 #define IPA_AUTOSUSPEND_DELAY	500	/* milliseconds */
 
-/**
- * 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_power - IPA power management information
  * @dev:		IPA device pointer
  * @core:		IPA core clock
  * @qmp:		QMP handle for AOSS communication
- * @flags:		Boolean state flags
  * @interconnect_count:	Number of elements in interconnect[]
  * @interconnect:	Interconnect array
  */
@@ -57,7 +46,6 @@ struct ipa_power {
 	struct device *dev;
 	struct clk *core;
 	struct qmp *qmp;
-	DECLARE_BITMAP(flags, IPA_POWER_FLAG_COUNT);
 	u32 interconnect_count;
 	struct icc_bulk_data interconnect[] __counted_by(interconnect_count);
 };
@@ -139,7 +127,6 @@ static int ipa_runtime_suspend(struct device *dev)
 
 	/* Endpoints aren't usable until setup is complete */
 	if (ipa->setup_complete) {
-		__clear_bit(IPA_POWER_FLAG_RESUMED, ipa->power->flags);
 		ipa_endpoint_suspend(ipa);
 		gsi_suspend(&ipa->gsi);
 	}
@@ -209,14 +196,6 @@ u32 ipa_core_clock_rate(struct ipa *ipa)
 
 void ipa_power_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
 {
-	/* To handle an IPA interrupt we will have resumed the hardware
-	 * just to handle the interrupt, so we're done.  If we are in a
-	 * system suspend, trigger a system resume.
-	 */
-	if (!__test_and_set_bit(IPA_POWER_FLAG_RESUMED, ipa->power->flags)) {
-		;
-	}
-
 	/* Acknowledge/clear the suspend interrupt on all endpoints */
 	ipa_interrupt_suspend_clear_all(ipa->interrupt);
 }
-- 
2.40.1

[PATCH net-next 4/6] net: ipa: move ipa_interrupt_suspend_clear_all() up

[Alex Elder]

The next patch makes ipa_interrupt_suspend_clear_all() static,
calling it only within "ipa_interrupt.c".  Move its definition
higher in the file so no declaration is needed.

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

diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
index a78c692f2d3c5..e5e01655e8c28 100644
--- a/drivers/net/ipa/ipa_interrupt.c
+++ b/drivers/net/ipa/ipa_interrupt.c
@@ -43,6 +43,30 @@ struct ipa_interrupt {
 	u32 enabled;
 };
 
+/* Clear the suspend interrupt for all endpoints that signaled it */
+void ipa_interrupt_suspend_clear_all(struct ipa_interrupt *interrupt)
+{
+	struct ipa *ipa = interrupt->ipa;
+	u32 unit_count;
+	u32 unit;
+
+	unit_count = DIV_ROUND_UP(ipa->endpoint_count, 32);
+	for (unit = 0; unit < unit_count; unit++) {
+		const struct reg *reg;
+		u32 val;
+
+		reg = ipa_reg(ipa, IRQ_SUSPEND_INFO);
+		val = ioread32(ipa->reg_virt + reg_n_offset(reg, unit));
+
+		/* SUSPEND interrupt status isn't cleared on IPA version 3.0 */
+		if (ipa->version == IPA_VERSION_3_0)
+			continue;
+
+		reg = ipa_reg(ipa, IRQ_SUSPEND_CLR);
+		iowrite32(val, ipa->reg_virt + reg_n_offset(reg, unit));
+	}
+}
+
 /* Process a particular interrupt type that has been received */
 static void ipa_interrupt_process(struct ipa_interrupt *interrupt, u32 irq_id)
 {
@@ -205,30 +229,6 @@ ipa_interrupt_suspend_disable(struct ipa_interrupt *interrupt, u32 endpoint_id)
 	ipa_interrupt_suspend_control(interrupt, endpoint_id, false);
 }
 
-/* Clear the suspend interrupt for all endpoints that signaled it */
-void ipa_interrupt_suspend_clear_all(struct ipa_interrupt *interrupt)
-{
-	struct ipa *ipa = interrupt->ipa;
-	u32 unit_count;
-	u32 unit;
-
-	unit_count = DIV_ROUND_UP(ipa->endpoint_count, 32);
-	for (unit = 0; unit < unit_count; unit++) {
-		const struct reg *reg;
-		u32 val;
-
-		reg = ipa_reg(ipa, IRQ_SUSPEND_INFO);
-		val = ioread32(ipa->reg_virt + reg_n_offset(reg, unit));
-
-		/* SUSPEND interrupt status isn't cleared on IPA version 3.0 */
-		if (ipa->version == IPA_VERSION_3_0)
-			continue;
-
-		reg = ipa_reg(ipa, IRQ_SUSPEND_CLR);
-		iowrite32(val, ipa->reg_virt + reg_n_offset(reg, unit));
-	}
-}
-
 /* Simulate arrival of an IPA TX_SUSPEND interrupt */
 void ipa_interrupt_simulate_suspend(struct ipa_interrupt *interrupt)
 {
-- 
2.40.1

[PATCH net-next 5/6] net: ipa: kill ipa_power_suspend_handler()

[Alex Elder]

Now that ipa_power_suspend_handler() is a trivial wrapper around
ipa_interrupt_suspend_clear_all(), we can open-code it in the one
place it's used, and get rid of the function.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_interrupt.c |  4 ++--
 drivers/net/ipa/ipa_interrupt.h |  8 --------
 drivers/net/ipa/ipa_power.c     |  6 ------
 drivers/net/ipa/ipa_power.h     | 11 -----------
 4 files changed, 2 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
index e5e01655e8c28..501962cc4e90f 100644
--- a/drivers/net/ipa/ipa_interrupt.c
+++ b/drivers/net/ipa/ipa_interrupt.c
@@ -44,7 +44,7 @@ struct ipa_interrupt {
 };
 
 /* Clear the suspend interrupt for all endpoints that signaled it */
-void ipa_interrupt_suspend_clear_all(struct ipa_interrupt *interrupt)
+static void ipa_interrupt_suspend_clear_all(struct ipa_interrupt *interrupt)
 {
 	struct ipa *ipa = interrupt->ipa;
 	u32 unit_count;
@@ -94,7 +94,7 @@ static void ipa_interrupt_process(struct ipa_interrupt *interrupt, u32 irq_id)
 		 * caused the interrupt, so defer clearing until after
 		 * the handler has been called.
 		 */
-		ipa_power_suspend_handler(ipa, irq_id);
+		ipa_interrupt_suspend_clear_all(interrupt);
 		fallthrough;
 
 	default:	/* Silently ignore (and clear) any other condition */
diff --git a/drivers/net/ipa/ipa_interrupt.h b/drivers/net/ipa/ipa_interrupt.h
index 12e3e798ccb38..53e1b71685c75 100644
--- a/drivers/net/ipa/ipa_interrupt.h
+++ b/drivers/net/ipa/ipa_interrupt.h
@@ -34,14 +34,6 @@ void ipa_interrupt_suspend_enable(struct ipa_interrupt *interrupt,
 void ipa_interrupt_suspend_disable(struct ipa_interrupt *interrupt,
 				   u32 endpoint_id);
 
-/**
- * ipa_interrupt_suspend_clear_all - clear all suspend interrupts
- * @interrupt:	IPA interrupt structure
- *
- * Clear the TX_SUSPEND interrupt for all endpoints that signaled it.
- */
-void ipa_interrupt_suspend_clear_all(struct ipa_interrupt *interrupt);
-
 /**
  * ipa_interrupt_simulate_suspend() - Simulate TX_SUSPEND IPA interrupt
  * @interrupt:	IPA interrupt structure
diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
index eee251d67f81a..0f635b8356bfb 100644
--- a/drivers/net/ipa/ipa_power.c
+++ b/drivers/net/ipa/ipa_power.c
@@ -194,12 +194,6 @@ u32 ipa_core_clock_rate(struct ipa *ipa)
 	return ipa->power ? (u32)clk_get_rate(ipa->power->core) : 0;
 }
 
-void ipa_power_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
-{
-	/* Acknowledge/clear the suspend interrupt on all endpoints */
-	ipa_interrupt_suspend_clear_all(ipa->interrupt);
-}
-
 static int ipa_power_retention_init(struct ipa_power *power)
 {
 	struct qmp *qmp = qmp_get(power->dev);
diff --git a/drivers/net/ipa/ipa_power.h b/drivers/net/ipa/ipa_power.h
index 718aacf5e2b23..227cc04bea806 100644
--- a/drivers/net/ipa/ipa_power.h
+++ b/drivers/net/ipa/ipa_power.h
@@ -30,17 +30,6 @@ u32 ipa_core_clock_rate(struct ipa *ipa);
  */
 void ipa_power_retention(struct ipa *ipa, bool enable);
 
-/**
- * ipa_power_suspend_handler() - Handler for SUSPEND IPA interrupts
- * @ipa:	IPA pointer
- * @irq_id:	IPA interrupt ID (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.
- */
-void ipa_power_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id);
-
 /**
  * ipa_power_setup() - Set up IPA power management
  * @ipa:	IPA pointer
-- 
2.40.1

[PATCH net-next 6/6] net: ipa: don't bother zeroing an already zero register

[Alex Elder]

In ipa_interrupt_suspend_clear_all(), if the SUSPEND_INFO register
read contains no set bits, there's no interrupt condition to clear.
Skip the write to the clear register in that case.

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

diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
index 501962cc4e90f..4d80bf77a5323 100644
--- a/drivers/net/ipa/ipa_interrupt.c
+++ b/drivers/net/ipa/ipa_interrupt.c
@@ -59,7 +59,7 @@ static void ipa_interrupt_suspend_clear_all(struct ipa_interrupt *interrupt)
 		val = ioread32(ipa->reg_virt + reg_n_offset(reg, unit));
 
 		/* SUSPEND interrupt status isn't cleared on IPA version 3.0 */
-		if (ipa->version == IPA_VERSION_3_0)
+		if (!val || ipa->version == IPA_VERSION_3_0)
 			continue;
 
 		reg = ipa_reg(ipa, IRQ_SUSPEND_CLR);
-- 
2.40.1

Re: [PATCH net-next 1/6] net: ipa: don't bother aborting system resume

[Paolo Abeni]

On Fri, 2024-02-23 at 07:39 -0600, Alex Elder wrote:
> The IPA interrupt can fire if there is data to be delivered to a GSI
> channel that is suspended.  This condition occurs in three scenarios.
> 
> First, runtime power management automatically suspends the IPA
> hardware after half a second of inactivity.  This has nothing
> to do with system suspend, so a SYSTEM IPA power flag is used to
> avoid calling pm_wakeup_dev_event() when runtime suspended.
> 
> Second, if the system is suspended, the receipt of an IPA interrupt
> should trigger a system resume.  Configuring the IPA interrupt for
> wakeup accomplishes this.
> 
> Finally, if system suspend is underway and the IPA interrupt fires,
> we currently call pm_wakeup_dev_event() to abort the system suspend.
> 
> The IPA driver correctly handles quiescing the hardware before
> suspending it, so there's really no need to abort a suspend in
> progress in the third case.  We can simply quiesce and suspend
> things, and be done.
> 
> Incoming data can still wake the system after it's suspended.
> The IPA interrupt has wakeup mode enabled, so if it fires *after*
> we've suspended, it will trigger a wakeup (if not disabled via
> sysfs).
> 
> Stop calling pm_wakeup_dev_event() to abort a system suspend in
> progress in ipa_power_suspend_handler().
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
> Note: checkpatch warns: braces {} are not necessary...
> 
>  drivers/net/ipa/ipa_power.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
> index 128a816f65237..694bc71e0a170 100644
> --- a/drivers/net/ipa/ipa_power.c
> +++ b/drivers/net/ipa/ipa_power.c
> @@ -220,8 +220,9 @@ void ipa_power_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
>  	 * system suspend, trigger a system resume.
>  	 */
>  	if (!__test_and_set_bit(IPA_POWER_FLAG_RESUMED, ipa->power->flags))
> -		if (test_bit(IPA_POWER_FLAG_SYSTEM, ipa->power->flags))
> -			pm_wakeup_dev_event(&ipa->pdev->dev, 0, true);
> +		if (test_bit(IPA_POWER_FLAG_SYSTEM, ipa->power->flags)) {
> +			;
> +		}

FTR, I would have dropped the whole 'if' statement above and the
related comment in this patch, saving a few checkpatch warnings. Not a
big deal since the the chunk is removed a few patches later.

Cheers,

Paolo

Re: [PATCH net-next 0/6] net: ipa: don't abort system suspend

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri, 23 Feb 2024 07:39:24 -0600 you wrote:
> Currently the IPA code aborts an in-progress system suspend if an
> IPA interrupt arrives before the suspend completes.  There is no
> need to do that though, because the IPA driver handles a forced
> suspend correctly, quiescing any hardware activity before finally
> turning off clocks and interconnects.
> 
> This series drops the call to pm_wakeup_dev_event() if an IPA
> SUSPEND interrupt arrives during system suspend.  Doing this
> makes the two remaining IPA power flags unnecessary, and allows
> some additional code to be cleaned up--and best of all, removed.
> The result is much simpler (and I'm really glad not to be using
> these flags any more).
> 
> [...]

Here is the summary with links:
  - [net-next,1/6] net: ipa: don't bother aborting system resume
    https://git.kernel.org/netdev/net-next/c/4b2274d3811a
  - [net-next,2/6] net: ipa: kill IPA_POWER_FLAG_SYSTEM
    https://git.kernel.org/netdev/net-next/c/54f19ff7676f
  - [net-next,3/6] net: ipa: kill the IPA_POWER_FLAG_RESUMED flag
    https://git.kernel.org/netdev/net-next/c/dae5d6e8f0ec
  - [net-next,4/6] net: ipa: move ipa_interrupt_suspend_clear_all() up
    https://git.kernel.org/netdev/net-next/c/ef63ca78da2e
  - [net-next,5/6] net: ipa: kill ipa_power_suspend_handler()
    https://git.kernel.org/netdev/net-next/c/423df2e09d3b
  - [net-next,6/6] net: ipa: don't bother zeroing an already zero register
    https://git.kernel.org/netdev/net-next/c/f9345952e74a

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