[PATCH net-next 0/6] net: ipa: simplify IPA interrupt handling

[PATCH net-next 0/6] net: ipa: simplify IPA interrupt handling

[Alex Elder]

One of the IPA's two IRQs fires when data on a suspended channel is
available (to request that the channel--or system--be resumed to
recieve the pending data).  This interrupt also handles a few
conditions signaled by the embedded microcontroller.

For this "IPA interrupt", the current code requires a handler to be
dynamically registered for each interrupt condition.  Any condition
that has no registered handler is quietly ignored.  This design is
derived from the downstream IPA driver implementation.

There isn't any need for this complexity.  Even in the downstream
code, only four of the available 30 or so IPA interrupt conditions
are ever handled.  So these handlers can pretty easily just be
called directly in the main IRQ handler function.

This series simplifies the interrupt handling code by having the
small number of IPA interrupt handlers be called directly, rather
than having them be registered dynamically.

					-Alex

Alex Elder (6):
  net: ipa: introduce a common microcontroller interrupt handler
  net: ipa: introduce ipa_interrupt_enable()
  net: ipa: enable IPA interrupt handlers separate from registration
  net: ipa: register IPA interrupt handlers directly
  net: ipa: kill ipa_interrupt_add()
  net: ipa: don't maintain IPA interrupt handler array

 drivers/net/ipa/ipa_interrupt.c | 103 ++++++++++++++------------------
 drivers/net/ipa/ipa_interrupt.h |  47 +++++----------
 drivers/net/ipa/ipa_power.c     |  19 ++----
 drivers/net/ipa/ipa_power.h     |  12 ++++
 drivers/net/ipa/ipa_uc.c        |  21 +++++--
 drivers/net/ipa/ipa_uc.h        |   8 +++
 6 files changed, 98 insertions(+), 112 deletions(-)

-- 
2.34.1

[PATCH net-next 1/6] net: ipa: introduce a common microcontroller interrupt handler

[Alex Elder]

The prototype for an IPA interrupt handler supplies the IPA
interrupt ID, so it's possible to use a single function to handle
any type of microcontroller interrupt.

Introduce ipa_uc_interrupt_handler(), which calls the event or the
response handler depending on the IRQ ID provided.  Register the new
function as the handler for both microcontroller IPA interrupt types.

The called functions don't use their "irq_id" arguments, so remove
them.

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

diff --git a/drivers/net/ipa/ipa_uc.c b/drivers/net/ipa/ipa_uc.c
index f0ee472810153..0a890b44c09e1 100644
--- a/drivers/net/ipa/ipa_uc.c
+++ b/drivers/net/ipa/ipa_uc.c
@@ -124,7 +124,7 @@ static struct ipa_uc_mem_area *ipa_uc_shared(struct ipa *ipa)
 }
 
 /* Microcontroller event IPA interrupt handler */
-static void ipa_uc_event_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
+static void ipa_uc_event_handler(struct ipa *ipa)
 {
 	struct ipa_uc_mem_area *shared = ipa_uc_shared(ipa);
 	struct device *dev = &ipa->pdev->dev;
@@ -138,7 +138,7 @@ static void ipa_uc_event_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
 }
 
 /* Microcontroller response IPA interrupt handler */
-static void ipa_uc_response_hdlr(struct ipa *ipa, enum ipa_irq_id irq_id)
+static void ipa_uc_response_hdlr(struct ipa *ipa)
 {
 	struct ipa_uc_mem_area *shared = ipa_uc_shared(ipa);
 	struct device *dev = &ipa->pdev->dev;
@@ -170,13 +170,24 @@ static void ipa_uc_response_hdlr(struct ipa *ipa, enum ipa_irq_id irq_id)
 	}
 }
 
+static void ipa_uc_interrupt_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
+{
+	/* Silently ignore anything unrecognized */
+	if (irq_id == IPA_IRQ_UC_0)
+		ipa_uc_event_handler(ipa);
+	else if (irq_id == IPA_IRQ_UC_1)
+		ipa_uc_response_hdlr(ipa);
+}
+
 /* Configure the IPA microcontroller subsystem */
 void ipa_uc_config(struct ipa *ipa)
 {
+	struct ipa_interrupt *interrupt = ipa->interrupt;
+
 	ipa->uc_powered = false;
 	ipa->uc_loaded = false;
-	ipa_interrupt_add(ipa->interrupt, IPA_IRQ_UC_0, ipa_uc_event_handler);
-	ipa_interrupt_add(ipa->interrupt, IPA_IRQ_UC_1, ipa_uc_response_hdlr);
+	ipa_interrupt_add(interrupt, IPA_IRQ_UC_0, ipa_uc_interrupt_handler);
+	ipa_interrupt_add(interrupt, IPA_IRQ_UC_1, ipa_uc_interrupt_handler);
 }
 
 /* Inverse of ipa_uc_config() */
-- 
2.34.1

[PATCH net-next 2/6] net: ipa: introduce ipa_interrupt_enable()

[Alex Elder]

Create new function ipa_interrupt_enable() to encapsulate enabling
one of the IPA interrupt types.  Introduce ipa_interrupt_disable()
to reverse that operation.  Add a helper function to factor out the
common register update used by both.

Use these in ipa_interrupt_add() and ipa_interrupt_remove().

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

diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
index a49f66efacb87..7b7388c14806f 100644
--- a/drivers/net/ipa/ipa_interrupt.c
+++ b/drivers/net/ipa/ipa_interrupt.c
@@ -127,6 +127,29 @@ static irqreturn_t ipa_isr_thread(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static void ipa_interrupt_enabled_update(struct ipa *ipa)
+{
+	const struct ipa_reg *reg = ipa_reg(ipa, IPA_IRQ_EN);
+
+	iowrite32(ipa->interrupt->enabled, ipa->reg_virt + ipa_reg_offset(reg));
+}
+
+/* Enable an IPA interrupt type */
+static void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
+{
+	/* Update the IPA interrupt mask to enable it */
+	ipa->interrupt->enabled |= BIT(ipa_irq);
+	ipa_interrupt_enabled_update(ipa);
+}
+
+/* Disable an IPA interrupt type */
+static void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
+{
+	/* Update the IPA interrupt mask to disable it */
+	ipa->interrupt->enabled &= ~BIT(ipa_irq);
+	ipa_interrupt_enabled_update(ipa);
+}
+
 /* Common function used to enable/disable TX_SUSPEND for an endpoint */
 static void ipa_interrupt_suspend_control(struct ipa_interrupt *interrupt,
 					  u32 endpoint_id, bool enable)
@@ -205,36 +228,22 @@ void ipa_interrupt_simulate_suspend(struct ipa_interrupt *interrupt)
 void ipa_interrupt_add(struct ipa_interrupt *interrupt,
 		       enum ipa_irq_id ipa_irq, ipa_irq_handler_t handler)
 {
-	struct ipa *ipa = interrupt->ipa;
-	const struct ipa_reg *reg;
-
 	if (WARN_ON(ipa_irq >= IPA_IRQ_COUNT))
 		return;
 
 	interrupt->handler[ipa_irq] = handler;
 
-	/* Update the IPA interrupt mask to enable it */
-	interrupt->enabled |= BIT(ipa_irq);
-
-	reg = ipa_reg(ipa, IPA_IRQ_EN);
-	iowrite32(interrupt->enabled, ipa->reg_virt + ipa_reg_offset(reg));
+	ipa_interrupt_enable(interrupt->ipa, ipa_irq);
 }
 
 /* Remove the handler for an IPA interrupt type */
 void
 ipa_interrupt_remove(struct ipa_interrupt *interrupt, enum ipa_irq_id ipa_irq)
 {
-	struct ipa *ipa = interrupt->ipa;
-	const struct ipa_reg *reg;
-
 	if (WARN_ON(ipa_irq >= IPA_IRQ_COUNT))
 		return;
 
-	/* Update the IPA interrupt mask to disable it */
-	interrupt->enabled &= ~BIT(ipa_irq);
-
-	reg = ipa_reg(ipa, IPA_IRQ_EN);
-	iowrite32(interrupt->enabled, ipa->reg_virt + ipa_reg_offset(reg));
+	ipa_interrupt_disable(interrupt->ipa, ipa_irq);
 
 	interrupt->handler[ipa_irq] = NULL;
 }
-- 
2.34.1

[PATCH net-next 3/6] net: ipa: enable IPA interrupt handlers separate from registration

[Alex Elder]

Expose ipa_interrupt_enable() and have functions that register
IPA interrupt handlers enable them directly, rather than having the
registration process do that.  Do the same for disabling IPA
interrupt handlers.

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

diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
index 7b7388c14806f..87f4b94d02a3f 100644
--- a/drivers/net/ipa/ipa_interrupt.c
+++ b/drivers/net/ipa/ipa_interrupt.c
@@ -135,7 +135,7 @@ static void ipa_interrupt_enabled_update(struct ipa *ipa)
 }
 
 /* Enable an IPA interrupt type */
-static void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
+void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
 {
 	/* Update the IPA interrupt mask to enable it */
 	ipa->interrupt->enabled |= BIT(ipa_irq);
@@ -143,7 +143,7 @@ static void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
 }
 
 /* Disable an IPA interrupt type */
-static void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
+void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
 {
 	/* Update the IPA interrupt mask to disable it */
 	ipa->interrupt->enabled &= ~BIT(ipa_irq);
@@ -232,8 +232,6 @@ void ipa_interrupt_add(struct ipa_interrupt *interrupt,
 		return;
 
 	interrupt->handler[ipa_irq] = handler;
-
-	ipa_interrupt_enable(interrupt->ipa, ipa_irq);
 }
 
 /* Remove the handler for an IPA interrupt type */
@@ -243,8 +241,6 @@ ipa_interrupt_remove(struct ipa_interrupt *interrupt, enum ipa_irq_id ipa_irq)
 	if (WARN_ON(ipa_irq >= IPA_IRQ_COUNT))
 		return;
 
-	ipa_interrupt_disable(interrupt->ipa, ipa_irq);
-
 	interrupt->handler[ipa_irq] = NULL;
 }
 
diff --git a/drivers/net/ipa/ipa_interrupt.h b/drivers/net/ipa/ipa_interrupt.h
index f31fd9965fdc6..5f7d2e90ea337 100644
--- a/drivers/net/ipa/ipa_interrupt.h
+++ b/drivers/net/ipa/ipa_interrupt.h
@@ -85,6 +85,20 @@ void ipa_interrupt_suspend_clear_all(struct ipa_interrupt *interrupt);
  */
 void ipa_interrupt_simulate_suspend(struct ipa_interrupt *interrupt);
 
+/**
+ * ipa_interrupt_enable() - Enable an IPA interrupt type
+ * @ipa:	IPA pointer
+ * @ipa_irq:	IPA interrupt ID
+ */
+void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq);
+
+/**
+ * ipa_interrupt_disable() - Disable an IPA interrupt type
+ * @ipa:	IPA pointer
+ * @ipa_irq:	IPA interrupt ID
+ */
+void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id ipa_irq);
+
 /**
  * ipa_interrupt_config() - Configure the IPA interrupt framework
  * @ipa:	IPA pointer
diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
index 8420f93128a26..9148d606d5fc2 100644
--- a/drivers/net/ipa/ipa_power.c
+++ b/drivers/net/ipa/ipa_power.c
@@ -337,10 +337,13 @@ int ipa_power_setup(struct ipa *ipa)
 
 	ipa_interrupt_add(ipa->interrupt, IPA_IRQ_TX_SUSPEND,
 			  ipa_suspend_handler);
+	ipa_interrupt_enable(ipa, IPA_IRQ_TX_SUSPEND);
 
 	ret = device_init_wakeup(&ipa->pdev->dev, true);
-	if (ret)
+	if (ret) {
+		ipa_interrupt_disable(ipa, IPA_IRQ_TX_SUSPEND);
 		ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
+	}
 
 	return ret;
 }
@@ -348,6 +351,7 @@ int ipa_power_setup(struct ipa *ipa)
 void ipa_power_teardown(struct ipa *ipa)
 {
 	(void)device_init_wakeup(&ipa->pdev->dev, false);
+	ipa_interrupt_disable(ipa, IPA_IRQ_TX_SUSPEND);
 	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
 }
 
diff --git a/drivers/net/ipa/ipa_uc.c b/drivers/net/ipa/ipa_uc.c
index 0a890b44c09e1..af541758d047f 100644
--- a/drivers/net/ipa/ipa_uc.c
+++ b/drivers/net/ipa/ipa_uc.c
@@ -187,7 +187,9 @@ void ipa_uc_config(struct ipa *ipa)
 	ipa->uc_powered = false;
 	ipa->uc_loaded = false;
 	ipa_interrupt_add(interrupt, IPA_IRQ_UC_0, ipa_uc_interrupt_handler);
+	ipa_interrupt_enable(ipa, IPA_IRQ_UC_0);
 	ipa_interrupt_add(interrupt, IPA_IRQ_UC_1, ipa_uc_interrupt_handler);
+	ipa_interrupt_enable(ipa, IPA_IRQ_UC_1);
 }
 
 /* Inverse of ipa_uc_config() */
@@ -195,7 +197,9 @@ void ipa_uc_deconfig(struct ipa *ipa)
 {
 	struct device *dev = &ipa->pdev->dev;
 
+	ipa_interrupt_disable(ipa, IPA_IRQ_UC_1);
 	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_1);
+	ipa_interrupt_disable(ipa, IPA_IRQ_UC_0);
 	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_0);
 	if (ipa->uc_loaded)
 		ipa_power_retention(ipa, false);
-- 
2.34.1

[PATCH net-next 4/6] net: ipa: register IPA interrupt handlers directly

[Alex Elder]

Declare the microcontroller IPA interrupt handler publicly, and
assign it directly in ipa_interrupt_config().  Make the SUSPEND IPA
interrupt handler public, and rename it ipa_power_suspend_handler().
Assign it directly in ipa_interrupt_config() as well.

This makes it unnecessary to do this in ipa_interrupt_add().  Make
similar changes for removing IPA interrupt handlers.

The next two patches will finish the cleanup, removing the
add/remove functions and the handler array entirely.

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

diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
index 87f4b94d02a3f..f32ac40a79372 100644
--- a/drivers/net/ipa/ipa_interrupt.c
+++ b/drivers/net/ipa/ipa_interrupt.c
@@ -26,6 +26,8 @@
 #include "ipa.h"
 #include "ipa_reg.h"
 #include "ipa_endpoint.h"
+#include "ipa_power.h"
+#include "ipa_uc.h"
 #include "ipa_interrupt.h"
 
 /**
@@ -228,20 +230,14 @@ void ipa_interrupt_simulate_suspend(struct ipa_interrupt *interrupt)
 void ipa_interrupt_add(struct ipa_interrupt *interrupt,
 		       enum ipa_irq_id ipa_irq, ipa_irq_handler_t handler)
 {
-	if (WARN_ON(ipa_irq >= IPA_IRQ_COUNT))
-		return;
-
-	interrupt->handler[ipa_irq] = handler;
+	WARN_ON(ipa_irq >= IPA_IRQ_COUNT);
 }
 
 /* Remove the handler for an IPA interrupt type */
 void
 ipa_interrupt_remove(struct ipa_interrupt *interrupt, enum ipa_irq_id ipa_irq)
 {
-	if (WARN_ON(ipa_irq >= IPA_IRQ_COUNT))
-		return;
-
-	interrupt->handler[ipa_irq] = NULL;
+	WARN_ON(ipa_irq >= IPA_IRQ_COUNT);
 }
 
 /* Configure the IPA interrupt framework */
@@ -284,6 +280,10 @@ struct ipa_interrupt *ipa_interrupt_config(struct ipa *ipa)
 		goto err_free_irq;
 	}
 
+	interrupt->handler[IPA_IRQ_UC_0] = ipa_uc_interrupt_handler;
+	interrupt->handler[IPA_IRQ_UC_1] = ipa_uc_interrupt_handler;
+	interrupt->handler[IPA_IRQ_TX_SUSPEND] = ipa_power_suspend_handler;
+
 	return interrupt;
 
 err_free_irq:
diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
index 9148d606d5fc2..4198f8e97e40b 100644
--- a/drivers/net/ipa/ipa_power.c
+++ b/drivers/net/ipa/ipa_power.c
@@ -202,17 +202,7 @@ u32 ipa_core_clock_rate(struct ipa *ipa)
 	return ipa->power ? (u32)clk_get_rate(ipa->power->core) : 0;
 }
 
-/**
- * ipa_suspend_handler() - Handle the suspend IPA interrupt
- * @ipa:	IPA pointer
- * @irq_id:	IPA interrupt type (unused)
- *
- * If an RX endpoint is suspended, and the IPA has a packet destined for
- * that endpoint, the IPA generates a SUSPEND interrupt to inform the AP
- * that it should resume the endpoint.  If we get one of these interrupts
- * we just wake up the system.
- */
-static void ipa_suspend_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
+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
@@ -336,7 +326,7 @@ int ipa_power_setup(struct ipa *ipa)
 	int ret;
 
 	ipa_interrupt_add(ipa->interrupt, IPA_IRQ_TX_SUSPEND,
-			  ipa_suspend_handler);
+			  ipa_power_suspend_handler);
 	ipa_interrupt_enable(ipa, IPA_IRQ_TX_SUSPEND);
 
 	ret = device_init_wakeup(&ipa->pdev->dev, true);
diff --git a/drivers/net/ipa/ipa_power.h b/drivers/net/ipa/ipa_power.h
index 896f052e51a1c..3a4c59ea1222b 100644
--- a/drivers/net/ipa/ipa_power.h
+++ b/drivers/net/ipa/ipa_power.h
@@ -10,6 +10,7 @@ struct device;
 
 struct ipa;
 struct ipa_power_data;
+enum ipa_irq_id;
 
 /* IPA device power management function block */
 extern const struct dev_pm_ops ipa_pm_ops;
@@ -47,6 +48,17 @@ void ipa_power_modem_queue_active(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
diff --git a/drivers/net/ipa/ipa_uc.c b/drivers/net/ipa/ipa_uc.c
index af541758d047f..6b7d289cfaffa 100644
--- a/drivers/net/ipa/ipa_uc.c
+++ b/drivers/net/ipa/ipa_uc.c
@@ -170,7 +170,7 @@ static void ipa_uc_response_hdlr(struct ipa *ipa)
 	}
 }
 
-static void ipa_uc_interrupt_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
+void ipa_uc_interrupt_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
 {
 	/* Silently ignore anything unrecognized */
 	if (irq_id == IPA_IRQ_UC_0)
diff --git a/drivers/net/ipa/ipa_uc.h b/drivers/net/ipa/ipa_uc.h
index 8514096e6f36f..85aa0df818c23 100644
--- a/drivers/net/ipa/ipa_uc.h
+++ b/drivers/net/ipa/ipa_uc.h
@@ -7,6 +7,14 @@
 #define _IPA_UC_H_
 
 struct ipa;
+enum ipa_irq_id;
+
+/**
+ * ipa_uc_interrupt_handler() - Handler for microcontroller IPA interrupts
+ * @ipa:	IPA pointer
+ * @irq_id:	IPA interrupt ID
+ */
+void ipa_uc_interrupt_handler(struct ipa *ipa, enum ipa_irq_id irq_id);
 
 /**
  * ipa_uc_config() - Configure the IPA microcontroller subsystem
-- 
2.34.1

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

[Alex Elder]

The dynamic assignment of IPA interrupt handlers isn't needed; we
only handle three IPA interrupt types, and their handler functions
are now assigned directly.  We can get rid of ipa_interrupt_add()
and ipa_interrupt_remove() now, because they serve no purpose.

Signed-off-by: Alex Elder <elder@linaro.org>
---
 drivers/net/ipa/ipa_interrupt.c | 16 ++--------------
 drivers/net/ipa/ipa_interrupt.h | 33 ---------------------------------
 drivers/net/ipa/ipa_power.c     |  7 +------
 drivers/net/ipa/ipa_uc.c        |  6 ------
 4 files changed, 3 insertions(+), 59 deletions(-)

diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
index f32ac40a79372..f0a68b0a242c1 100644
--- a/drivers/net/ipa/ipa_interrupt.c
+++ b/drivers/net/ipa/ipa_interrupt.c
@@ -30,6 +30,8 @@
 #include "ipa_uc.h"
 #include "ipa_interrupt.h"
 
+typedef void (*ipa_irq_handler_t)(struct ipa *ipa, enum ipa_irq_id irq_id);
+
 /**
  * struct ipa_interrupt - IPA interrupt information
  * @ipa:		IPA pointer
@@ -226,20 +228,6 @@ void ipa_interrupt_simulate_suspend(struct ipa_interrupt *interrupt)
 	ipa_interrupt_process(interrupt, IPA_IRQ_TX_SUSPEND);
 }
 
-/* Add a handler for an IPA interrupt */
-void ipa_interrupt_add(struct ipa_interrupt *interrupt,
-		       enum ipa_irq_id ipa_irq, ipa_irq_handler_t handler)
-{
-	WARN_ON(ipa_irq >= IPA_IRQ_COUNT);
-}
-
-/* Remove the handler for an IPA interrupt type */
-void
-ipa_interrupt_remove(struct ipa_interrupt *interrupt, enum ipa_irq_id ipa_irq)
-{
-	WARN_ON(ipa_irq >= IPA_IRQ_COUNT);
-}
-
 /* Configure the IPA interrupt framework */
 struct ipa_interrupt *ipa_interrupt_config(struct ipa *ipa)
 {
diff --git a/drivers/net/ipa/ipa_interrupt.h b/drivers/net/ipa/ipa_interrupt.h
index 5f7d2e90ea337..c1df054be8fab 100644
--- a/drivers/net/ipa/ipa_interrupt.h
+++ b/drivers/net/ipa/ipa_interrupt.h
@@ -12,39 +12,6 @@
 struct ipa;
 struct ipa_interrupt;
 
-/**
- * typedef ipa_irq_handler_t - IPA interrupt handler function type
- * @ipa:	IPA pointer
- * @irq_id:	interrupt type
- *
- * Callback function registered by ipa_interrupt_add() to handle a specific
- * IPA interrupt type
- */
-typedef void (*ipa_irq_handler_t)(struct ipa *ipa, enum ipa_irq_id irq_id);
-
-/**
- * ipa_interrupt_add() - Register a handler for an IPA interrupt type
- * @interrupt:	IPA interrupt structure
- * @irq_id:	IPA interrupt type
- * @handler:	Handler function for the interrupt
- *
- * Add a handler for an IPA interrupt and enable it.  IPA interrupt
- * handlers are run in threaded interrupt context, so are allowed to
- * block.
- */
-void ipa_interrupt_add(struct ipa_interrupt *interrupt, enum ipa_irq_id irq_id,
-		       ipa_irq_handler_t handler);
-
-/**
- * ipa_interrupt_remove() - Remove the handler for an IPA interrupt type
- * @interrupt:	IPA interrupt structure
- * @irq_id:	IPA interrupt type
- *
- * Remove an IPA interrupt handler and disable it.
- */
-void ipa_interrupt_remove(struct ipa_interrupt *interrupt,
-			  enum ipa_irq_id irq_id);
-
 /**
  * ipa_interrupt_suspend_enable - Enable TX_SUSPEND for an endpoint
  * @interrupt:		IPA interrupt structure
diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
index 4198f8e97e40b..a282512ebd2d8 100644
--- a/drivers/net/ipa/ipa_power.c
+++ b/drivers/net/ipa/ipa_power.c
@@ -325,15 +325,11 @@ int ipa_power_setup(struct ipa *ipa)
 {
 	int ret;
 
-	ipa_interrupt_add(ipa->interrupt, IPA_IRQ_TX_SUSPEND,
-			  ipa_power_suspend_handler);
 	ipa_interrupt_enable(ipa, IPA_IRQ_TX_SUSPEND);
 
 	ret = device_init_wakeup(&ipa->pdev->dev, true);
-	if (ret) {
+	if (ret)
 		ipa_interrupt_disable(ipa, IPA_IRQ_TX_SUSPEND);
-		ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
-	}
 
 	return ret;
 }
@@ -342,7 +338,6 @@ void ipa_power_teardown(struct ipa *ipa)
 {
 	(void)device_init_wakeup(&ipa->pdev->dev, false);
 	ipa_interrupt_disable(ipa, IPA_IRQ_TX_SUSPEND);
-	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
 }
 
 /* Initialize IPA power management */
diff --git a/drivers/net/ipa/ipa_uc.c b/drivers/net/ipa/ipa_uc.c
index 6b7d289cfaffa..cb8a76a75f21d 100644
--- a/drivers/net/ipa/ipa_uc.c
+++ b/drivers/net/ipa/ipa_uc.c
@@ -182,13 +182,9 @@ void ipa_uc_interrupt_handler(struct ipa *ipa, enum ipa_irq_id irq_id)
 /* Configure the IPA microcontroller subsystem */
 void ipa_uc_config(struct ipa *ipa)
 {
-	struct ipa_interrupt *interrupt = ipa->interrupt;
-
 	ipa->uc_powered = false;
 	ipa->uc_loaded = false;
-	ipa_interrupt_add(interrupt, IPA_IRQ_UC_0, ipa_uc_interrupt_handler);
 	ipa_interrupt_enable(ipa, IPA_IRQ_UC_0);
-	ipa_interrupt_add(interrupt, IPA_IRQ_UC_1, ipa_uc_interrupt_handler);
 	ipa_interrupt_enable(ipa, IPA_IRQ_UC_1);
 }
 
@@ -198,9 +194,7 @@ void ipa_uc_deconfig(struct ipa *ipa)
 	struct device *dev = &ipa->pdev->dev;
 
 	ipa_interrupt_disable(ipa, IPA_IRQ_UC_1);
-	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_1);
 	ipa_interrupt_disable(ipa, IPA_IRQ_UC_0);
-	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_0);
 	if (ipa->uc_loaded)
 		ipa_power_retention(ipa, false);
 
-- 
2.34.1

[PATCH net-next 6/6] net: ipa: don't maintain IPA interrupt handler array

[Alex Elder]

We can call the two IPA interrupt handler functions directly;
there's no need to maintain the array of handler function pointers
any more.

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

diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
index f0a68b0a242c1..5f047b29e6ef0 100644
--- a/drivers/net/ipa/ipa_interrupt.c
+++ b/drivers/net/ipa/ipa_interrupt.c
@@ -30,54 +30,52 @@
 #include "ipa_uc.h"
 #include "ipa_interrupt.h"
 
-typedef void (*ipa_irq_handler_t)(struct ipa *ipa, enum ipa_irq_id irq_id);
-
 /**
  * struct ipa_interrupt - IPA interrupt information
  * @ipa:		IPA pointer
  * @irq:		Linux IRQ number used for IPA interrupts
  * @enabled:		Mask indicating which interrupts are enabled
- * @handler:		Array of handlers indexed by IPA interrupt ID
  */
 struct ipa_interrupt {
 	struct ipa *ipa;
 	u32 irq;
 	u32 enabled;
-	ipa_irq_handler_t handler[IPA_IRQ_COUNT];
 };
 
-/* Returns true if the interrupt type is associated with the microcontroller */
-static bool ipa_interrupt_uc(struct ipa_interrupt *interrupt, u32 irq_id)
-{
-	return irq_id == IPA_IRQ_UC_0 || irq_id == IPA_IRQ_UC_1;
-}
-
 /* Process a particular interrupt type that has been received */
 static void ipa_interrupt_process(struct ipa_interrupt *interrupt, u32 irq_id)
 {
-	bool uc_irq = ipa_interrupt_uc(interrupt, irq_id);
 	struct ipa *ipa = interrupt->ipa;
 	const struct ipa_reg *reg;
 	u32 mask = BIT(irq_id);
 	u32 offset;
 
-	/* For microcontroller interrupts, clear the interrupt right away,
-	 * "to avoid clearing unhandled interrupts."
-	 */
 	reg = ipa_reg(ipa, IPA_IRQ_CLR);
 	offset = ipa_reg_offset(reg);
-	if (uc_irq)
+
+	switch (irq_id) {
+	case IPA_IRQ_UC_0:
+	case IPA_IRQ_UC_1:
+		/* For microcontroller interrupts, clear the interrupt right
+		 * away, "to avoid clearing unhandled interrupts."
+		 */
 		iowrite32(mask, ipa->reg_virt + offset);
+		ipa_uc_interrupt_handler(ipa, irq_id);
+		break;
 
-	if (irq_id < IPA_IRQ_COUNT && interrupt->handler[irq_id])
-		interrupt->handler[irq_id](interrupt->ipa, irq_id);
+	case IPA_IRQ_TX_SUSPEND:
+		/* Clearing the SUSPEND_TX interrupt also clears the
+		 * register that tells us which suspended endpoint(s)
+		 * caused the interrupt, so defer clearing until after
+		 * the handler has been called.
+		 */
+		ipa_power_suspend_handler(ipa, irq_id);
+		fallthrough;
 
-	/* Clearing the SUSPEND_TX interrupt also clears the register
-	 * that tells us which suspended endpoint(s) caused the interrupt,
-	 * so defer clearing until after the handler has been called.
-	 */
-	if (!uc_irq)
+	default:	/* Silently ignore (and clear) any other condition */
 		iowrite32(mask, ipa->reg_virt + offset);
+		break;
+	}
 }
 
 /* IPA IRQ handler is threaded */
@@ -268,10 +266,6 @@ struct ipa_interrupt *ipa_interrupt_config(struct ipa *ipa)
 		goto err_free_irq;
 	}
 
-	interrupt->handler[IPA_IRQ_UC_0] = ipa_uc_interrupt_handler;
-	interrupt->handler[IPA_IRQ_UC_1] = ipa_uc_interrupt_handler;
-	interrupt->handler[IPA_IRQ_TX_SUSPEND] = ipa_power_suspend_handler;
-
 	return interrupt;
 
 err_free_irq:
-- 
2.34.1

Re: [PATCH net-next 0/6] net: ipa: simplify IPA interrupt handling

[Jakub Kicinski]

On Fri, 30 Dec 2022 17:22:24 -0600 Alex Elder wrote:
> [PATCH net-next 0/6] net: ipa: simplify IPA interrupt handling

We kept net-next closed for an extra week due to end-of-the-year
festivities, back in business next week, sorry.

Re: [PATCH net-next 0/6] net: ipa: simplify IPA interrupt handling

[Alex Elder]

On 12/30/22 9:52 PM, Jakub Kicinski wrote:
> On Fri, 30 Dec 2022 17:22:24 -0600 Alex Elder wrote:
>> [PATCH net-next 0/6] net: ipa: simplify IPA interrupt handling
> 
> We kept net-next closed for an extra week due to end-of-the-year
> festivities, back in business next week, sorry.

No, *I* am sorry.  I forgot to even check this time, and I normally do.
   http://vger.kernel.org/~davem/net-next.html

It's perfectly fine, I'll wait.  I won't re-send next week unless it
becomes obvious I should.

Thanks, and happy new year!

					-Alex

Re: [PATCH net-next 3/6] net: ipa: enable IPA interrupt handlers separate from registration

[Caleb Connolly]

On 30/12/2022 23:22, Alex Elder wrote:
> Expose ipa_interrupt_enable() and have functions that register
> IPA interrupt handlers enable them directly, rather than having the
> registration process do that.  Do the same for disabling IPA
> interrupt handlers.

Hi,
> 
> Signed-off-by: Alex Elder <elder@linaro.org>
> ---
>   drivers/net/ipa/ipa_interrupt.c |  8 ++------
>   drivers/net/ipa/ipa_interrupt.h | 14 ++++++++++++++
>   drivers/net/ipa/ipa_power.c     |  6 +++++-
>   drivers/net/ipa/ipa_uc.c        |  4 ++++
>   4 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c
> index 7b7388c14806f..87f4b94d02a3f 100644
> --- a/drivers/net/ipa/ipa_interrupt.c
> +++ b/drivers/net/ipa/ipa_interrupt.c
> @@ -135,7 +135,7 @@ static void ipa_interrupt_enabled_update(struct ipa *ipa)
>   }
>   
>   /* Enable an IPA interrupt type */
> -static void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
> +void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
>   {
>   	/* Update the IPA interrupt mask to enable it */
>   	ipa->interrupt->enabled |= BIT(ipa_irq);
> @@ -143,7 +143,7 @@ static void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
>   }
>   
>   /* Disable an IPA interrupt type */
> -static void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
> +void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
>   {
>   	/* Update the IPA interrupt mask to disable it */
>   	ipa->interrupt->enabled &= ~BIT(ipa_irq);
> @@ -232,8 +232,6 @@ void ipa_interrupt_add(struct ipa_interrupt *interrupt,
>   		return;
>   
>   	interrupt->handler[ipa_irq] = handler;
> -
> -	ipa_interrupt_enable(interrupt->ipa, ipa_irq);
>   }
>   
>   /* Remove the handler for an IPA interrupt type */
> @@ -243,8 +241,6 @@ ipa_interrupt_remove(struct ipa_interrupt *interrupt, enum ipa_irq_id ipa_irq)
>   	if (WARN_ON(ipa_irq >= IPA_IRQ_COUNT))
>   		return;
>   
> -	ipa_interrupt_disable(interrupt->ipa, ipa_irq);
> -
>   	interrupt->handler[ipa_irq] = NULL;
>   }
>   
> diff --git a/drivers/net/ipa/ipa_interrupt.h b/drivers/net/ipa/ipa_interrupt.h
> index f31fd9965fdc6..5f7d2e90ea337 100644
> --- a/drivers/net/ipa/ipa_interrupt.h
> +++ b/drivers/net/ipa/ipa_interrupt.h
> @@ -85,6 +85,20 @@ void ipa_interrupt_suspend_clear_all(struct ipa_interrupt *interrupt);
>    */
>   void ipa_interrupt_simulate_suspend(struct ipa_interrupt *interrupt);
>   
> +/**
> + * ipa_interrupt_enable() - Enable an IPA interrupt type
> + * @ipa:	IPA pointer
> + * @ipa_irq:	IPA interrupt ID
> + */
> +void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq);

I think you forgot a forward declaration for enum ipa_irq_id

Kind Regards,
Caleb
> +
> +/**
> + * ipa_interrupt_disable() - Disable an IPA interrupt type
> + * @ipa:	IPA pointer
> + * @ipa_irq:	IPA interrupt ID
> + */
> +void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id ipa_irq);
> +
>   /**
>    * ipa_interrupt_config() - Configure the IPA interrupt framework
>    * @ipa:	IPA pointer
> diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
> index 8420f93128a26..9148d606d5fc2 100644
> --- a/drivers/net/ipa/ipa_power.c
> +++ b/drivers/net/ipa/ipa_power.c
> @@ -337,10 +337,13 @@ int ipa_power_setup(struct ipa *ipa)
>   
>   	ipa_interrupt_add(ipa->interrupt, IPA_IRQ_TX_SUSPEND,
>   			  ipa_suspend_handler);
> +	ipa_interrupt_enable(ipa, IPA_IRQ_TX_SUSPEND);
>   
>   	ret = device_init_wakeup(&ipa->pdev->dev, true);
> -	if (ret)
> +	if (ret) {
> +		ipa_interrupt_disable(ipa, IPA_IRQ_TX_SUSPEND);
>   		ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
> +	}
>   
>   	return ret;
>   }
> @@ -348,6 +351,7 @@ int ipa_power_setup(struct ipa *ipa)
>   void ipa_power_teardown(struct ipa *ipa)
>   {
>   	(void)device_init_wakeup(&ipa->pdev->dev, false);
> +	ipa_interrupt_disable(ipa, IPA_IRQ_TX_SUSPEND);
>   	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
>   }
>   
> diff --git a/drivers/net/ipa/ipa_uc.c b/drivers/net/ipa/ipa_uc.c
> index 0a890b44c09e1..af541758d047f 100644
> --- a/drivers/net/ipa/ipa_uc.c
> +++ b/drivers/net/ipa/ipa_uc.c
> @@ -187,7 +187,9 @@ void ipa_uc_config(struct ipa *ipa)
>   	ipa->uc_powered = false;
>   	ipa->uc_loaded = false;
>   	ipa_interrupt_add(interrupt, IPA_IRQ_UC_0, ipa_uc_interrupt_handler);
> +	ipa_interrupt_enable(ipa, IPA_IRQ_UC_0);
>   	ipa_interrupt_add(interrupt, IPA_IRQ_UC_1, ipa_uc_interrupt_handler);
> +	ipa_interrupt_enable(ipa, IPA_IRQ_UC_1);
>   }
>   
>   /* Inverse of ipa_uc_config() */
> @@ -195,7 +197,9 @@ void ipa_uc_deconfig(struct ipa *ipa)
>   {
>   	struct device *dev = &ipa->pdev->dev;
>   
> +	ipa_interrupt_disable(ipa, IPA_IRQ_UC_1);
>   	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_1);
> +	ipa_interrupt_disable(ipa, IPA_IRQ_UC_0);
>   	ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_0);
>   	if (ipa->uc_loaded)
>   		ipa_power_retention(ipa, false);

Re: [PATCH net-next 3/6] net: ipa: enable IPA interrupt handlers separate from registration

[Alex Elder]

On 12/31/22 11:56 AM, Caleb Connolly wrote:
> 
> 
> On 30/12/2022 23:22, Alex Elder wrote:
>> Expose ipa_interrupt_enable() and have functions that register
>> IPA interrupt handlers enable them directly, rather than having the
>> registration process do that.  Do the same for disabling IPA
>> interrupt handlers.
> 
> Hi,
>>
>> Signed-off-by: Alex Elder <elder@linaro.org>
>> ---
>>   drivers/net/ipa/ipa_interrupt.c |  8 ++------
>>   drivers/net/ipa/ipa_interrupt.h | 14 ++++++++++++++
>>   drivers/net/ipa/ipa_power.c     |  6 +++++-
>>   drivers/net/ipa/ipa_uc.c        |  4 ++++
>>   4 files changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ipa/ipa_interrupt.c 
>> b/drivers/net/ipa/ipa_interrupt.c
>> index 7b7388c14806f..87f4b94d02a3f 100644
>> --- a/drivers/net/ipa/ipa_interrupt.c
>> +++ b/drivers/net/ipa/ipa_interrupt.c
>> @@ -135,7 +135,7 @@ static void ipa_interrupt_enabled_update(struct 
>> ipa *ipa)
>>   }
>>   /* Enable an IPA interrupt type */
>> -static void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id 
>> ipa_irq)
>> +void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
>>   {
>>       /* Update the IPA interrupt mask to enable it */
>>       ipa->interrupt->enabled |= BIT(ipa_irq);
>> @@ -143,7 +143,7 @@ static void ipa_interrupt_enable(struct ipa *ipa, 
>> enum ipa_irq_id ipa_irq)
>>   }
>>   /* Disable an IPA interrupt type */
>> -static void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id 
>> ipa_irq)
>> +void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id ipa_irq)
>>   {
>>       /* Update the IPA interrupt mask to disable it */
>>       ipa->interrupt->enabled &= ~BIT(ipa_irq);
>> @@ -232,8 +232,6 @@ void ipa_interrupt_add(struct ipa_interrupt 
>> *interrupt,
>>           return;
>>       interrupt->handler[ipa_irq] = handler;
>> -
>> -    ipa_interrupt_enable(interrupt->ipa, ipa_irq);
>>   }
>>   /* Remove the handler for an IPA interrupt type */
>> @@ -243,8 +241,6 @@ ipa_interrupt_remove(struct ipa_interrupt 
>> *interrupt, enum ipa_irq_id ipa_irq)
>>       if (WARN_ON(ipa_irq >= IPA_IRQ_COUNT))
>>           return;
>> -    ipa_interrupt_disable(interrupt->ipa, ipa_irq);
>> -
>>       interrupt->handler[ipa_irq] = NULL;
>>   }
>> diff --git a/drivers/net/ipa/ipa_interrupt.h 
>> b/drivers/net/ipa/ipa_interrupt.h
>> index f31fd9965fdc6..5f7d2e90ea337 100644
>> --- a/drivers/net/ipa/ipa_interrupt.h
>> +++ b/drivers/net/ipa/ipa_interrupt.h
>> @@ -85,6 +85,20 @@ void ipa_interrupt_suspend_clear_all(struct 
>> ipa_interrupt *interrupt);
>>    */
>>   void ipa_interrupt_simulate_suspend(struct ipa_interrupt *interrupt);
>> +/**
>> + * ipa_interrupt_enable() - Enable an IPA interrupt type
>> + * @ipa:    IPA pointer
>> + * @ipa_irq:    IPA interrupt ID
>> + */
>> +void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq);
> 
> I think you forgot a forward declaration for enum ipa_irq_id

Thanks, I'll verify this and will send v2 with a fix once
net-next is open for business again.

					-Alex

> 
> Kind Regards,
> Caleb
>> +
>> +/**
>> + * ipa_interrupt_disable() - Disable an IPA interrupt type
>> + * @ipa:    IPA pointer
>> + * @ipa_irq:    IPA interrupt ID
>> + */
>> +void ipa_interrupt_disable(struct ipa *ipa, enum ipa_irq_id ipa_irq);
>> +
>>   /**
>>    * ipa_interrupt_config() - Configure the IPA interrupt framework
>>    * @ipa:    IPA pointer
>> diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c
>> index 8420f93128a26..9148d606d5fc2 100644
>> --- a/drivers/net/ipa/ipa_power.c
>> +++ b/drivers/net/ipa/ipa_power.c
>> @@ -337,10 +337,13 @@ int ipa_power_setup(struct ipa *ipa)
>>       ipa_interrupt_add(ipa->interrupt, IPA_IRQ_TX_SUSPEND,
>>                 ipa_suspend_handler);
>> +    ipa_interrupt_enable(ipa, IPA_IRQ_TX_SUSPEND);
>>       ret = device_init_wakeup(&ipa->pdev->dev, true);
>> -    if (ret)
>> +    if (ret) {
>> +        ipa_interrupt_disable(ipa, IPA_IRQ_TX_SUSPEND);
>>           ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
>> +    }
>>       return ret;
>>   }
>> @@ -348,6 +351,7 @@ int ipa_power_setup(struct ipa *ipa)
>>   void ipa_power_teardown(struct ipa *ipa)
>>   {
>>       (void)device_init_wakeup(&ipa->pdev->dev, false);
>> +    ipa_interrupt_disable(ipa, IPA_IRQ_TX_SUSPEND);
>>       ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_TX_SUSPEND);
>>   }
>> diff --git a/drivers/net/ipa/ipa_uc.c b/drivers/net/ipa/ipa_uc.c
>> index 0a890b44c09e1..af541758d047f 100644
>> --- a/drivers/net/ipa/ipa_uc.c
>> +++ b/drivers/net/ipa/ipa_uc.c
>> @@ -187,7 +187,9 @@ void ipa_uc_config(struct ipa *ipa)
>>       ipa->uc_powered = false;
>>       ipa->uc_loaded = false;
>>       ipa_interrupt_add(interrupt, IPA_IRQ_UC_0, 
>> ipa_uc_interrupt_handler);
>> +    ipa_interrupt_enable(ipa, IPA_IRQ_UC_0);
>>       ipa_interrupt_add(interrupt, IPA_IRQ_UC_1, 
>> ipa_uc_interrupt_handler);
>> +    ipa_interrupt_enable(ipa, IPA_IRQ_UC_1);
>>   }
>>   /* Inverse of ipa_uc_config() */
>> @@ -195,7 +197,9 @@ void ipa_uc_deconfig(struct ipa *ipa)
>>   {
>>       struct device *dev = &ipa->pdev->dev;
>> +    ipa_interrupt_disable(ipa, IPA_IRQ_UC_1);
>>       ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_1);
>> +    ipa_interrupt_disable(ipa, IPA_IRQ_UC_0);
>>       ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_0);
>>       if (ipa->uc_loaded)
>>           ipa_power_retention(ipa, false);

Re: [PATCH net-next 3/6] net: ipa: enable IPA interrupt handlers separate from registration

[Alex Elder]

On 12/31/22 11:56 AM, Caleb Connolly wrote:
>>
>> diff --git a/drivers/net/ipa/ipa_interrupt.h 
>> b/drivers/net/ipa/ipa_interrupt.h
>> index f31fd9965fdc6..5f7d2e90ea337 100644
>> --- a/drivers/net/ipa/ipa_interrupt.h
>> +++ b/drivers/net/ipa/ipa_interrupt.h
>> @@ -85,6 +85,20 @@ void ipa_interrupt_suspend_clear_all(struct 
>> ipa_interrupt *interrupt);
>>    */
>>   void ipa_interrupt_simulate_suspend(struct ipa_interrupt *interrupt);
>> +/**
>> + * ipa_interrupt_enable() - Enable an IPA interrupt type
>> + * @ipa:    IPA pointer
>> + * @ipa_irq:    IPA interrupt ID
>> + */
>> +void ipa_interrupt_enable(struct ipa *ipa, enum ipa_irq_id ipa_irq);
> 
> I think you forgot a forward declaration for enum ipa_irq_id
> 
> Kind Regards,
> Caleb

OK I checked this.

You are correct that ipa_irq_id should be declared as an
enum at the top of "ipa_interrupt.h".  In addition to the
new function declarations, there were some existing
references to the enumerated type.  I believe this became
a (not-reported) problem starting with this commit:

   322053105f095 net: ipa: move definition of enum ipa_irq_id

It being missing did not result in any build warnings,
however.  Here's why.

The ipa_irq_id enumerated type is defined in "ipa_reg.h".

Note that "ipa_reg.h" is included by "ipa_endpoint.h".  So if
"ipa_endpoint.h" is included before "ipa_interrupt.h", the
ipa_irq_id type will have been defined before it's referenced
in "ipa_interrupt.h".

These files include "ipa_interrupt.h":

ipa.h:
It is included after "ipa_endpoint.h", so the enumerated type
is defined before it's needed in "ipa_interrupt.h".

ipa_main.c:
Here too, "ipa_endpoint.h" (as well as "ipa_reg.h") is included
before "ipa_interrupt.h", so the enumerated type is defined
before it's used there.

ipa_interrupt.c
In this case "ipa_reg.h" is included, then "ipa_endpoint.h",
before "ipa_interrupt.h".  So again, the enumerated type is
defined before it's referenced in "ipa_interrupt.h".

Nevertheless, your point is a good one and I'm going to
implement your suggestion when I post version 2.

Thank you!

					-Alex