| Message ID | 20221230232230.2348757-4-elder@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ipa: simplify IPA interrupt handling | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 91 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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);
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);
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 --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);
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(-)