| Message ID | 20240418204729.1952353-2-elder@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: ipa: eight simple cleanups | expand |
On 18/04/2024 21:47, Alex Elder wrote: > Keep track of which endpoints have the SUSPEND IPA interrupt enabled > in a variable-length bitmap. This will be used in the next patch to > allow the SUSPEND interrupt type to be disabled except when needed. > > Signed-off-by: Alex Elder <elder@linaro.org> > --- > drivers/net/ipa/ipa_interrupt.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c > index c44ec05f71e6f..0e8d4e43275ea 100644 > --- a/drivers/net/ipa/ipa_interrupt.c > +++ b/drivers/net/ipa/ipa_interrupt.c > @@ -37,11 +37,13 @@ > * @ipa: IPA pointer > * @irq: Linux IRQ number used for IPA interrupts > * @enabled: Mask indicating which interrupts are enabled > + * @suspend_enabled: Bitmap of endpoints with the SUSPEND interrupt enabled > */ > struct ipa_interrupt { > struct ipa *ipa; > u32 irq; > u32 enabled; > + unsigned long *suspend_enabled; > }; > > /* Clear the suspend interrupt for all endpoints that signaled it */ > @@ -211,6 +213,7 @@ static void ipa_interrupt_suspend_control(struct ipa_interrupt *interrupt, > val |= mask; > else > val &= ~mask; > + __change_bit(endpoint_id, interrupt->suspend_enabled); > > iowrite32(val, ipa->reg_virt + offset); > } > @@ -246,7 +249,16 @@ int ipa_interrupt_config(struct ipa *ipa) > > interrupt->ipa = ipa; > > - /* Disable all IPA interrupt types */ > + /* Initially all IPA interrupt types are disabled */ > + interrupt->enabled = 0; > + interrupt->suspend_enabled = bitmap_zalloc(ipa->endpoint_count, > + GFP_KERNEL); why not use devm_bitmap_zalloc() instead and skip managing the cleanup ? > + if (!interrupt->suspend_enabled) { > + ret = -ENOMEM; > + goto err_kfree; > + } > + > + /* Disable IPA interrupt types */ > reg = ipa_reg(ipa, IPA_IRQ_EN); > iowrite32(0, ipa->reg_virt + reg_offset(reg)); > > @@ -254,7 +266,7 @@ int ipa_interrupt_config(struct ipa *ipa) > "ipa", interrupt); > if (ret) { > dev_err(dev, "error %d requesting \"ipa\" IRQ\n", ret); > - goto err_kfree; > + goto err_free_bitmap; > } > > ret = dev_pm_set_wake_irq(dev, irq); > @@ -270,6 +282,8 @@ int ipa_interrupt_config(struct ipa *ipa) > > err_free_irq: > free_irq(interrupt->irq, interrupt); > +err_free_bitmap: > + bitmap_free(interrupt->suspend_enabled); > err_kfree: > kfree(interrupt); You could also use devm_kzalloc() and do away with the kfree()s you have here on the probe path. > > @@ -286,6 +300,7 @@ void ipa_interrupt_deconfig(struct ipa *ipa) > > dev_pm_clear_wake_irq(dev); > free_irq(interrupt->irq, interrupt); > + bitmap_free(interrupt->suspend_enabled); > } > > /* Initialize the IPA interrupt structure */ Just suggestions though. Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On 4/18/24 6:52 PM, Bryan O'Donoghue wrote: > On 18/04/2024 21:47, Alex Elder wrote: >> Keep track of which endpoints have the SUSPEND IPA interrupt enabled >> in a variable-length bitmap. This will be used in the next patch to >> allow the SUSPEND interrupt type to be disabled except when needed. >> >> Signed-off-by: Alex Elder <elder@linaro.org> >> --- >> drivers/net/ipa/ipa_interrupt.c | 19 +++++++++++++++++-- >> 1 file changed, 17 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ipa/ipa_interrupt.c >> b/drivers/net/ipa/ipa_interrupt.c >> index c44ec05f71e6f..0e8d4e43275ea 100644 >> --- a/drivers/net/ipa/ipa_interrupt.c >> +++ b/drivers/net/ipa/ipa_interrupt.c >> @@ -37,11 +37,13 @@ >> * @ipa: IPA pointer >> * @irq: Linux IRQ number used for IPA interrupts >> * @enabled: Mask indicating which interrupts are enabled >> + * @suspend_enabled: Bitmap of endpoints with the SUSPEND >> interrupt enabled >> */ >> struct ipa_interrupt { >> struct ipa *ipa; >> u32 irq; >> u32 enabled; >> + unsigned long *suspend_enabled; >> }; >> /* Clear the suspend interrupt for all endpoints that signaled it */ >> @@ -211,6 +213,7 @@ static void ipa_interrupt_suspend_control(struct >> ipa_interrupt *interrupt, >> val |= mask; >> else >> val &= ~mask; >> + __change_bit(endpoint_id, interrupt->suspend_enabled); >> iowrite32(val, ipa->reg_virt + offset); >> } >> @@ -246,7 +249,16 @@ int ipa_interrupt_config(struct ipa *ipa) >> interrupt->ipa = ipa; >> - /* Disable all IPA interrupt types */ >> + /* Initially all IPA interrupt types are disabled */ >> + interrupt->enabled = 0; >> + interrupt->suspend_enabled = bitmap_zalloc(ipa->endpoint_count, >> + GFP_KERNEL); > > why not use devm_bitmap_zalloc() instead and skip managing the cleanup ? I don't use the devm_*() variants in the IPA driver. I know I can, but if I'm make the switch I want to do it everywhere. Not now. Thanks for the review. -Alex >> + if (!interrupt->suspend_enabled) { >> + ret = -ENOMEM; >> + goto err_kfree; >> + } >> + >> + /* Disable IPA interrupt types */ >> reg = ipa_reg(ipa, IPA_IRQ_EN); >> iowrite32(0, ipa->reg_virt + reg_offset(reg)); >> @@ -254,7 +266,7 @@ int ipa_interrupt_config(struct ipa *ipa) >> "ipa", interrupt); >> if (ret) { >> dev_err(dev, "error %d requesting \"ipa\" IRQ\n", ret); >> - goto err_kfree; >> + goto err_free_bitmap; >> } >> ret = dev_pm_set_wake_irq(dev, irq); >> @@ -270,6 +282,8 @@ int ipa_interrupt_config(struct ipa *ipa) >> err_free_irq: >> free_irq(interrupt->irq, interrupt); >> +err_free_bitmap: >> + bitmap_free(interrupt->suspend_enabled); >> err_kfree: >> kfree(interrupt); > > You could also use devm_kzalloc() and do away with the kfree()s you have > here on the probe path. > >> @@ -286,6 +300,7 @@ void ipa_interrupt_deconfig(struct ipa *ipa) >> dev_pm_clear_wake_irq(dev); >> free_irq(interrupt->irq, interrupt); >> + bitmap_free(interrupt->suspend_enabled); >> } >> /* Initialize the IPA interrupt structure */ > > Just suggestions though. > > Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
diff --git a/drivers/net/ipa/ipa_interrupt.c b/drivers/net/ipa/ipa_interrupt.c index c44ec05f71e6f..0e8d4e43275ea 100644 --- a/drivers/net/ipa/ipa_interrupt.c +++ b/drivers/net/ipa/ipa_interrupt.c @@ -37,11 +37,13 @@ * @ipa: IPA pointer * @irq: Linux IRQ number used for IPA interrupts * @enabled: Mask indicating which interrupts are enabled + * @suspend_enabled: Bitmap of endpoints with the SUSPEND interrupt enabled */ struct ipa_interrupt { struct ipa *ipa; u32 irq; u32 enabled; + unsigned long *suspend_enabled; }; /* Clear the suspend interrupt for all endpoints that signaled it */ @@ -211,6 +213,7 @@ static void ipa_interrupt_suspend_control(struct ipa_interrupt *interrupt, val |= mask; else val &= ~mask; + __change_bit(endpoint_id, interrupt->suspend_enabled); iowrite32(val, ipa->reg_virt + offset); } @@ -246,7 +249,16 @@ int ipa_interrupt_config(struct ipa *ipa) interrupt->ipa = ipa; - /* Disable all IPA interrupt types */ + /* Initially all IPA interrupt types are disabled */ + interrupt->enabled = 0; + interrupt->suspend_enabled = bitmap_zalloc(ipa->endpoint_count, + GFP_KERNEL); + if (!interrupt->suspend_enabled) { + ret = -ENOMEM; + goto err_kfree; + } + + /* Disable IPA interrupt types */ reg = ipa_reg(ipa, IPA_IRQ_EN); iowrite32(0, ipa->reg_virt + reg_offset(reg)); @@ -254,7 +266,7 @@ int ipa_interrupt_config(struct ipa *ipa) "ipa", interrupt); if (ret) { dev_err(dev, "error %d requesting \"ipa\" IRQ\n", ret); - goto err_kfree; + goto err_free_bitmap; } ret = dev_pm_set_wake_irq(dev, irq); @@ -270,6 +282,8 @@ int ipa_interrupt_config(struct ipa *ipa) err_free_irq: free_irq(interrupt->irq, interrupt); +err_free_bitmap: + bitmap_free(interrupt->suspend_enabled); err_kfree: kfree(interrupt); @@ -286,6 +300,7 @@ void ipa_interrupt_deconfig(struct ipa *ipa) dev_pm_clear_wake_irq(dev); free_irq(interrupt->irq, interrupt); + bitmap_free(interrupt->suspend_enabled); } /* Initialize the IPA interrupt structure */
Keep track of which endpoints have the SUSPEND IPA interrupt enabled in a variable-length bitmap. This will be used in the next patch to allow the SUSPEND interrupt type to be disabled except when needed. Signed-off-by: Alex Elder <elder@linaro.org> --- drivers/net/ipa/ipa_interrupt.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)