diff mbox series

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

Message ID 20221230232230.2348757-4-elder@linaro.org
State New
Headers
Series net: ipa: simplify IPA interrupt handling |

Commit Message

Alex Elder Dec. 30, 2022, 11:22 p.m. UTC 
  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(-)

Comments

Caleb Connolly Dec. 31, 2022, 5:56 p.m. UTC | #1 
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);
Alex Elder Jan. 1, 2023, 6:32 p.m. UTC | #2 
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);
Alex Elder Jan. 3, 2023, 8:25 p.m. UTC | #3 
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
diff mbox series

Patch

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);