Up until commit 6a45b0e2589f ("gpiolib: Introduce
gpiochip_irqchip_add_domain()") all irq_domains were allocated
by gpiolib itself and thus gpiolib also takes care of freeing it.
With gpiochip_irqchip_add_domain() a user of gpiolib can associate an
irq_domain with the gpio_chip. This irq_domain is not managed by
gpiolib and therefore must not be freed by gpiolib.
Fixes: 6a45b0e2589f ("gpiolib: Introduce gpiochip_irqchip_add_domain()")
Reported-by: Jiawen Wu <[email protected]>
Signed-off-by: Michael Walle <[email protected]>
---
drivers/gpio/gpiolib.c | 3 ++-
include/linux/gpio/driver.h | 8 ++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7c6fefcbebd1..b0e3a3de2bf2 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1740,7 +1740,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gc)
}
/* Remove all IRQ mappings and delete the domain */
- if (gc->irq.domain) {
+ if (!gc->irq.domain_is_ext && gc->irq.domain) {
unsigned int irq;
for (offset = 0; offset < gc->ngpio; offset++) {
@@ -1786,6 +1786,7 @@ int gpiochip_irqchip_add_domain(struct gpio_chip *gc,
gc->to_irq = gpiochip_to_irq;
gc->irq.domain = domain;
+ gc->irq.domain_is_ext = true;
/*
* Using barrier() here to prevent compiler from reordering
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 5c6db5533be6..3dbfed83823b 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -251,6 +251,14 @@ struct gpio_irq_chip {
*/
bool initialized;
+ /**
+ * @domain_is_ext:
+ *
+ * True it the irq_domain was allocated outside of gpiolib, in which
+ * case gpiolib won't free the irq_domain itself.
+ */
+ bool domain_is_ext;
+
/**
* @init_hw: optional routine to initialize hardware before
* an IRQ chip will be added. This is quite useful when
---
base-commit: 659140df086c67f5eb77bbffcab34a7e8accd2a8
change-id: 20230616-fixes-gpiolib-irq-domain-2b69ecae0fdd
On Fri, Jun 16, 2023 at 9:30 AM Michael Walle <[email protected]> wrote:
> Up until commit 6a45b0e2589f ("gpiolib: Introduce
> gpiochip_irqchip_add_domain()") all irq_domains were allocated
> by gpiolib itself and thus gpiolib also takes care of freeing it.
>
> With gpiochip_irqchip_add_domain() a user of gpiolib can associate an
> irq_domain with the gpio_chip. This irq_domain is not managed by
> gpiolib and therefore must not be freed by gpiolib.
>
> Fixes: 6a45b0e2589f ("gpiolib: Introduce gpiochip_irqchip_add_domain()")
> Reported-by: Jiawen Wu <[email protected]>
> Signed-off-by: Michael Walle <[email protected]>
Oh right.
I guess we can't devres it some way...
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Fri, Jun 16, 2023 at 09:30:06AM +0200, Michael Walle wrote:
> Up until commit 6a45b0e2589f ("gpiolib: Introduce
> gpiochip_irqchip_add_domain()") all irq_domains were allocated
> by gpiolib itself and thus gpiolib also takes care of freeing it.
>
> With gpiochip_irqchip_add_domain() a user of gpiolib can associate an
> irq_domain with the gpio_chip. This irq_domain is not managed by
> gpiolib and therefore must not be freed by gpiolib.
With or without below nit-pick
Reviewed-by: Andy Shevchenko <[email protected]>
...
> /* Remove all IRQ mappings and delete the domain */
> - if (gc->irq.domain) {
> + if (!gc->irq.domain_is_ext && gc->irq.domain) {
Looking at this, perhaps positive about allocation?
domain_is_allocated?
--
With Best Regards,
Andy Shevchenko
Am 2023-06-16 16:13, schrieb Andy Shevchenko:
> On Fri, Jun 16, 2023 at 09:30:06AM +0200, Michael Walle wrote:
>> Up until commit 6a45b0e2589f ("gpiolib: Introduce
>> gpiochip_irqchip_add_domain()") all irq_domains were allocated
>> by gpiolib itself and thus gpiolib also takes care of freeing it.
>>
>> With gpiochip_irqchip_add_domain() a user of gpiolib can associate an
>> irq_domain with the gpio_chip. This irq_domain is not managed by
>> gpiolib and therefore must not be freed by gpiolib.
>
> With or without below nit-pick
> Reviewed-by: Andy Shevchenko <[email protected]>
>
> ...
>
>> /* Remove all IRQ mappings and delete the domain */
>> - if (gc->irq.domain) {
>> + if (!gc->irq.domain_is_ext && gc->irq.domain) {
>
> Looking at this, perhaps positive about allocation?
>
> domain_is_allocated?
I mean all domains are allocated ;)
domain_is_allocated_externally? I'm bad with short and concise
names..
-michael
On Fri, Jun 16, 2023 at 9:02 PM Michael Walle <[email protected]> wrote:
> Am 2023-06-16 16:13, schrieb Andy Shevchenko:
> > On Fri, Jun 16, 2023 at 09:30:06AM +0200, Michael Walle wrote:
...
> >> /* Remove all IRQ mappings and delete the domain */
> >> - if (gc->irq.domain) {
> >> + if (!gc->irq.domain_is_ext && gc->irq.domain) {
> >
> > Looking at this, perhaps positive about allocation?
> >
> > domain_is_allocated?
>
> I mean all domains are allocated ;)
>
> domain_is_allocated_externally? I'm bad with short and concise
> names..
Naming is one of the hardest problems in software...
Your variant is long, but conscious. I dunno. Bart, Linus, do you have
any ideas?
--
With Best Regards,
Andy Shevchenko
On Sat, Jun 17, 2023 at 4:25 PM Andy Shevchenko
<[email protected]> wrote:
> On Fri, Jun 16, 2023 at 9:02 PM Michael Walle <[email protected]> wrote:
> > Am 2023-06-16 16:13, schrieb Andy Shevchenko:
> > > On Fri, Jun 16, 2023 at 09:30:06AM +0200, Michael Walle wrote:
>
> ...
>
> > >> /* Remove all IRQ mappings and delete the domain */
> > >> - if (gc->irq.domain) {
> > >> + if (!gc->irq.domain_is_ext && gc->irq.domain) {
> > >
> > > Looking at this, perhaps positive about allocation?
> > >
> > > domain_is_allocated?
> >
> > I mean all domains are allocated ;)
> >
> > domain_is_allocated_externally? I'm bad with short and concise
> > names..
>
> Naming is one of the hardest problems in software...
> Your variant is long, but conscious. I dunno. Bart, Linus, do you have
> any ideas?
No, this name is fine. It is meant for human readers and they
will immediately understand.
Yours,
Linus Walleij