* [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs @ 2014-09-09 7:12 Wang, Yalin 2014-09-23 10:17 ` Linus Walleij 2014-09-23 10:21 ` Linus Walleij 0 siblings, 2 replies; 14+ messages in thread From: Wang, Yalin @ 2014-09-09 7:12 UTC (permalink / raw) To: 'linus.walleij@linaro.org', 'gnurou@gmail.com', 'linux-gpio@vger.kernel.org', 'akpm@linux-foundation.org' this patch change use from irq_create_mapping to irq_alloc_descs_from, use irq_create_mapping to alloc virq one by one is not safe, it can't promise the allcated virqs are continuous, in stead, we use irq_alloc_descs_from() to alloc virqs in one time, so that the allocated virqs are in continuous bitmaps. Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com> --- drivers/gpio/gpiolib.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 15cc0bb..2b6c441 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -566,7 +566,6 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip, unsigned int type) { struct device_node *of_node; - unsigned int offset; unsigned irq_base = 0; if (!gpiochip || !irqchip) @@ -604,14 +603,13 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip, * any gpiochip calls. If the first_irq was zero, this is * necessary to allocate descriptors for all IRQs. */ - for (offset = 0; offset < gpiochip->ngpio; offset++) { - irq_base = irq_create_mapping(gpiochip->irqdomain, offset); - if (offset == 0) - /* - * Store the base into the gpiochip to be used when - * unmapping the irqs. - */ - gpiochip->irq_base = irq_base; + if (first_irq > 0) { + gpiochip->irq_base = first_irq; + } else { + gpiochip->irq_base = irq_alloc_descs_from(1, gpiochip->ngpio, + of_node_to_nid(of_node)); + irq_domain_associate_many(gpiochip->irqdomain, + gpiochip->irq_base, 0, gpiochip->ngpio); } acpi_gpiochip_request_interrupts(gpiochip); -- 2.1.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs 2014-09-09 7:12 [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs Wang, Yalin @ 2014-09-23 10:17 ` Linus Walleij 2014-09-23 10:21 ` Linus Walleij 1 sibling, 0 replies; 14+ messages in thread From: Linus Walleij @ 2014-09-23 10:17 UTC (permalink / raw) To: Wang, Yalin Cc: gnurou@gmail.com, linux-gpio@vger.kernel.org, akpm@linux-foundation.org On Tue, Sep 9, 2014 at 9:12 AM, Wang, Yalin <Yalin.Wang@sonymobile.com> wrote: > this patch change use from irq_create_mapping to irq_alloc_descs_from, > use irq_create_mapping to alloc virq one by one is not safe, > it can't promise the allcated virqs are continuous, > in stead, we use irq_alloc_descs_from() to alloc virqs in one time, > so that the allocated virqs are in continuous bitmaps. > > Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com> OK makes sense, patch applied. The patch was BASE64-mangled though. Fixed it up and applied. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs 2014-09-09 7:12 [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs Wang, Yalin 2014-09-23 10:17 ` Linus Walleij @ 2014-09-23 10:21 ` Linus Walleij 2014-09-23 12:03 ` Wang, Yalin 1 sibling, 1 reply; 14+ messages in thread From: Linus Walleij @ 2014-09-23 10:21 UTC (permalink / raw) To: Wang, Yalin Cc: gnurou@gmail.com, linux-gpio@vger.kernel.org, akpm@linux-foundation.org On Tue, Sep 9, 2014 at 9:12 AM, Wang, Yalin <Yalin.Wang@sonymobile.com> wrote: > this patch change use from irq_create_mapping to irq_alloc_descs_from, > use irq_create_mapping to alloc virq one by one is not safe, > it can't promise the allcated virqs are continuous, > in stead, we use irq_alloc_descs_from() to alloc virqs in one time, > so that the allocated virqs are in continuous bitmaps. > > Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com> (...) > - for (offset = 0; offset < gpiochip->ngpio; offset++) { > - irq_base = irq_create_mapping(gpiochip->irqdomain, offset); > - if (offset == 0) > - /* > - * Store the base into the gpiochip to be used when > - * unmapping the irqs. > - */ > - gpiochip->irq_base = irq_base; > + if (first_irq > 0) { > + gpiochip->irq_base = first_irq; Wait is this safe? Now you assume all descriptors are pre-allocated and associated in this case, atleast explain what is going on. > + } else { > + gpiochip->irq_base = irq_alloc_descs_from(1, gpiochip->ngpio, > + of_node_to_nid(of_node)); > + irq_domain_associate_many(gpiochip->irqdomain, > + gpiochip->irq_base, 0, gpiochip->ngpio); This part looks OK. I'm holding this patch back until the above is clarified. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs 2014-09-23 10:21 ` Linus Walleij @ 2014-09-23 12:03 ` Wang, Yalin 2014-09-23 12:59 ` Grygorii Strashko 0 siblings, 1 reply; 14+ messages in thread From: Wang, Yalin @ 2014-09-23 12:03 UTC (permalink / raw) To: Linus Walleij Cc: gnurou@gmail.com, linux-gpio@vger.kernel.org, akpm@linux-foundation.org hi this is because , here: gpiochip->irqdomain = irq_domain_add_simple(of_node, gpiochip->ngpio, first_irq, &gpiochip_domain_ops, gpiochip); irq_domain_add_simple() in this function, if (first_irq > 0) { if (IS_ENABLED(CONFIG_SPARSE_IRQ)) { /* attempt to allocated irq_descs */ int rc = irq_alloc_descs(first_irq, first_irq, size, of_node_to_nid(of_node)); if (rc < 0) pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n", first_irq); } irq_domain_associate_many(domain, first_irq, 0, size); } if first_irq > 0 , it will allocate it , and make sure the return virq is equal to first_irq . so we don't need allocate it again . ________________________________________ From: Linus Walleij [linus.walleij@linaro.org] Sent: Tuesday, September 23, 2014 6:21 PM To: Wang, Yalin Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org Subject: Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs On Tue, Sep 9, 2014 at 9:12 AM, Wang, Yalin <Yalin.Wang@sonymobile.com> wrote: > this patch change use from irq_create_mapping to irq_alloc_descs_from, > use irq_create_mapping to alloc virq one by one is not safe, > it can't promise the allcated virqs are continuous, > in stead, we use irq_alloc_descs_from() to alloc virqs in one time, > so that the allocated virqs are in continuous bitmaps. > > Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com> (...) > - for (offset = 0; offset < gpiochip->ngpio; offset++) { > - irq_base = irq_create_mapping(gpiochip->irqdomain, offset); > - if (offset == 0) > - /* > - * Store the base into the gpiochip to be used when > - * unmapping the irqs. > - */ > - gpiochip->irq_base = irq_base; > + if (first_irq > 0) { > + gpiochip->irq_base = first_irq; Wait is this safe? Now you assume all descriptors are pre-allocated and associated in this case, atleast explain what is going on. > + } else { > + gpiochip->irq_base = irq_alloc_descs_from(1, gpiochip->ngpio, > + of_node_to_nid(of_node)); > + irq_domain_associate_many(gpiochip->irqdomain, > + gpiochip->irq_base, 0, gpiochip->ngpio); This part looks OK. I'm holding this patch back until the above is clarified. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs 2014-09-23 12:03 ` Wang, Yalin @ 2014-09-23 12:59 ` Grygorii Strashko 2014-09-23 13:37 ` Wang, Yalin 2014-09-25 7:39 ` Linus Walleij 0 siblings, 2 replies; 14+ messages in thread From: Grygorii Strashko @ 2014-09-23 12:59 UTC (permalink / raw) To: Wang, Yalin, Linus Walleij Cc: gnurou@gmail.com, linux-gpio@vger.kernel.org, akpm@linux-foundation.org Hi Wang, On 09/23/2014 03:03 PM, Wang, Yalin wrote: > hi > > this is because , here: > > gpiochip->irqdomain = irq_domain_add_simple(of_node, > gpiochip->ngpio, first_irq, > &gpiochip_domain_ops, gpiochip); > > > irq_domain_add_simple() in this function, > if (first_irq > 0) { > if (IS_ENABLED(CONFIG_SPARSE_IRQ)) { > /* attempt to allocated irq_descs */ > int rc = irq_alloc_descs(first_irq, first_irq, size, > of_node_to_nid(of_node)); > if (rc < 0) > pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n", > first_irq); > } > irq_domain_associate_many(domain, first_irq, 0, size); > } > > if first_irq > 0 , it will allocate it , > and make sure the return virq is equal to first_irq . > so we don't need allocate it again . Could provide a little bit more information about issue you've observed, pls? As for me, you patch will completely disable Sparse IRQ feature :( Also, I'm sure that struct gpio_chip->irq_base field can be removed from gpiolib irqchip code - GPIO drivers shouldn't use it also, because otherwise they will be incompatible with Sparse IRQ feature. Now the "irq_base" is used only twice in gpiolib code and below diff should allow to drop it completely from gpiolib code. diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 15cc0bb..81762ed 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -524,7 +524,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) /* Remove all IRQ mappings and delete the domain */ if (gpiochip->irqdomain) { for (offset = 0; offset < gpiochip->ngpio; offset++) - irq_dispose_mapping(gpiochip->irq_base + offset); + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, offset)); irq_domain_remove(gpiochip->irqdomain); } not tested. > ________________________________________ > From: Linus Walleij [linus.walleij@linaro.org] > Sent: Tuesday, September 23, 2014 6:21 PM > To: Wang, Yalin > Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org > Subject: Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs > > On Tue, Sep 9, 2014 at 9:12 AM, Wang, Yalin <Yalin.Wang@sonymobile.com> wrote: > >> this patch change use from irq_create_mapping to irq_alloc_descs_from, >> use irq_create_mapping to alloc virq one by one is not safe, >> it can't promise the allcated virqs are continuous, >> in stead, we use irq_alloc_descs_from() to alloc virqs in one time, >> so that the allocated virqs are in continuous bitmaps. >> >> Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com> > > (...) > >> - for (offset = 0; offset < gpiochip->ngpio; offset++) { >> - irq_base = irq_create_mapping(gpiochip->irqdomain, offset); >> - if (offset == 0) >> - /* >> - * Store the base into the gpiochip to be used when >> - * unmapping the irqs. >> - */ >> - gpiochip->irq_base = irq_base; >> + if (first_irq > 0) { >> + gpiochip->irq_base = first_irq; > > Wait is this safe? Now you assume all descriptors are pre-allocated > and associated in this case, atleast explain what is going on. > >> + } else { >> + gpiochip->irq_base = irq_alloc_descs_from(1, gpiochip->ngpio, >> + of_node_to_nid(of_node)); >> + irq_domain_associate_many(gpiochip->irqdomain, >> + gpiochip->irq_base, 0, gpiochip->ngpio); > > This part looks OK. > > I'm holding this patch back until the above is clarified. > > Yours, > Linus Walleij-- > To unsubscribe from this list: send the line "unsubscribe linux-gpio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Best regards, -grygorii ^ permalink raw reply related [flat|nested] 14+ messages in thread
* RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs 2014-09-23 12:59 ` Grygorii Strashko @ 2014-09-23 13:37 ` Wang, Yalin 2014-09-23 13:39 ` Wang, Yalin 2014-09-25 7:39 ` Linus Walleij 1 sibling, 1 reply; 14+ messages in thread From: Wang, Yalin @ 2014-09-23 13:37 UTC (permalink / raw) To: Grygorii Strashko, Linus Walleij Cc: gnurou@gmail.com, linux-gpio@vger.kernel.org, akpm@linux-foundation.org Hi In fact, i don't encounter any problem about gpio code, i just find this issue when i see the source code, i feel it is not safe, so i make a patch for it. yes, you are right, "irq_base" is used only twice in gpiolib code, but it maybe used by some other drivers, if remove it, some drivers will can't get virq base. it should get it by find_irq_mapping(), but it is also ok. in fact , we can allow the virq are allocated one by one, but this need change gpiochip_irqchip_remove( ) function, it should not assume the virq are contentious, i think both method are ok , it is just decided by how you want design it :) To summarize, we should make gpiochip_irqchip_add() and gpiochip_irqchip_remove() both work correctly. ________________________________________ From: Grygorii Strashko [grygorii.strashko@ti.com] Sent: Tuesday, September 23, 2014 8:59 PM To: Wang, Yalin; Linus Walleij Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org Subject: Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs Hi Wang, On 09/23/2014 03:03 PM, Wang, Yalin wrote: > hi > > this is because , here: > > gpiochip->irqdomain = irq_domain_add_simple(of_node, > gpiochip->ngpio, first_irq, > &gpiochip_domain_ops, gpiochip); > > > irq_domain_add_simple() in this function, > if (first_irq > 0) { > if (IS_ENABLED(CONFIG_SPARSE_IRQ)) { > /* attempt to allocated irq_descs */ > int rc = irq_alloc_descs(first_irq, first_irq, size, > of_node_to_nid(of_node)); > if (rc < 0) > pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n", > first_irq); > } > irq_domain_associate_many(domain, first_irq, 0, size); > } > > if first_irq > 0 , it will allocate it , > and make sure the return virq is equal to first_irq . > so we don't need allocate it again . Could provide a little bit more information about issue you've observed, pls? As for me, you patch will completely disable Sparse IRQ feature :( Also, I'm sure that struct gpio_chip->irq_base field can be removed from gpiolib irqchip code - GPIO drivers shouldn't use it also, because otherwise they will be incompatible with Sparse IRQ feature. Now the "irq_base" is used only twice in gpiolib code and below diff should allow to drop it completely from gpiolib code. diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 15cc0bb..81762ed 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -524,7 +524,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) /* Remove all IRQ mappings and delete the domain */ if (gpiochip->irqdomain) { for (offset = 0; offset < gpiochip->ngpio; offset++) - irq_dispose_mapping(gpiochip->irq_base + offset); + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, offset)); irq_domain_remove(gpiochip->irqdomain); } not tested. > ________________________________________ > From: Linus Walleij [linus.walleij@linaro.org] > Sent: Tuesday, September 23, 2014 6:21 PM > To: Wang, Yalin > Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org > Subject: Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs > > On Tue, Sep 9, 2014 at 9:12 AM, Wang, Yalin <Yalin.Wang@sonymobile.com> wrote: > >> this patch change use from irq_create_mapping to irq_alloc_descs_from, >> use irq_create_mapping to alloc virq one by one is not safe, >> it can't promise the allcated virqs are continuous, >> in stead, we use irq_alloc_descs_from() to alloc virqs in one time, >> so that the allocated virqs are in continuous bitmaps. >> >> Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com> > > (...) > >> - for (offset = 0; offset < gpiochip->ngpio; offset++) { >> - irq_base = irq_create_mapping(gpiochip->irqdomain, offset); >> - if (offset == 0) >> - /* >> - * Store the base into the gpiochip to be used when >> - * unmapping the irqs. >> - */ >> - gpiochip->irq_base = irq_base; >> + if (first_irq > 0) { >> + gpiochip->irq_base = first_irq; > > Wait is this safe? Now you assume all descriptors are pre-allocated > and associated in this case, atleast explain what is going on. > >> + } else { >> + gpiochip->irq_base = irq_alloc_descs_from(1, gpiochip->ngpio, >> + of_node_to_nid(of_node)); >> + irq_domain_associate_many(gpiochip->irqdomain, >> + gpiochip->irq_base, 0, gpiochip->ngpio); > > This part looks OK. > > I'm holding this patch back until the above is clarified. > > Yours, > Linus Walleij-- > To unsubscribe from this list: send the line "unsubscribe linux-gpio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Best regards, -grygorii ^ permalink raw reply related [flat|nested] 14+ messages in thread
* RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs 2014-09-23 13:37 ` Wang, Yalin @ 2014-09-23 13:39 ` Wang, Yalin 2014-09-23 14:15 ` Wang, Yalin 0 siblings, 1 reply; 14+ messages in thread From: Wang, Yalin @ 2014-09-23 13:39 UTC (permalink / raw) To: Wang, Yalin, Grygorii Strashko, Linus Walleij Cc: gnurou@gmail.com, linux-gpio@vger.kernel.org, akpm@linux-foundation.org hi sorry, i don't notice Grygorii's patch , yes, this patch is more easy, it is ok to fix this problem. Great! ________________________________________ From: Wang, Yalin Sent: Tuesday, September 23, 2014 9:37 PM To: Grygorii Strashko; Linus Walleij Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org Subject: RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs Hi In fact, i don't encounter any problem about gpio code, i just find this issue when i see the source code, i feel it is not safe, so i make a patch for it. yes, you are right, "irq_base" is used only twice in gpiolib code, but it maybe used by some other drivers, if remove it, some drivers will can't get virq base. it should get it by find_irq_mapping(), but it is also ok. in fact , we can allow the virq are allocated one by one, but this need change gpiochip_irqchip_remove( ) function, it should not assume the virq are contentious, i think both method are ok , it is just decided by how you want design it :) To summarize, we should make gpiochip_irqchip_add() and gpiochip_irqchip_remove() both work correctly. ________________________________________ From: Grygorii Strashko [grygorii.strashko@ti.com] Sent: Tuesday, September 23, 2014 8:59 PM To: Wang, Yalin; Linus Walleij Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org Subject: Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs Hi Wang, On 09/23/2014 03:03 PM, Wang, Yalin wrote: > hi > > this is because , here: > > gpiochip->irqdomain = irq_domain_add_simple(of_node, > gpiochip->ngpio, first_irq, > &gpiochip_domain_ops, gpiochip); > > > irq_domain_add_simple() in this function, > if (first_irq > 0) { > if (IS_ENABLED(CONFIG_SPARSE_IRQ)) { > /* attempt to allocated irq_descs */ > int rc = irq_alloc_descs(first_irq, first_irq, size, > of_node_to_nid(of_node)); > if (rc < 0) > pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n", > first_irq); > } > irq_domain_associate_many(domain, first_irq, 0, size); > } > > if first_irq > 0 , it will allocate it , > and make sure the return virq is equal to first_irq . > so we don't need allocate it again . Could provide a little bit more information about issue you've observed, pls? As for me, you patch will completely disable Sparse IRQ feature :( Also, I'm sure that struct gpio_chip->irq_base field can be removed from gpiolib irqchip code - GPIO drivers shouldn't use it also, because otherwise they will be incompatible with Sparse IRQ feature. Now the "irq_base" is used only twice in gpiolib code and below diff should allow to drop it completely from gpiolib code. diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 15cc0bb..81762ed 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -524,7 +524,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) /* Remove all IRQ mappings and delete the domain */ if (gpiochip->irqdomain) { for (offset = 0; offset < gpiochip->ngpio; offset++) - irq_dispose_mapping(gpiochip->irq_base + offset); + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, offset)); irq_domain_remove(gpiochip->irqdomain); } not tested. > ________________________________________ > From: Linus Walleij [linus.walleij@linaro.org] > Sent: Tuesday, September 23, 2014 6:21 PM > To: Wang, Yalin > Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org > Subject: Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs > > On Tue, Sep 9, 2014 at 9:12 AM, Wang, Yalin <Yalin.Wang@sonymobile.com> wrote: > >> this patch change use from irq_create_mapping to irq_alloc_descs_from, >> use irq_create_mapping to alloc virq one by one is not safe, >> it can't promise the allcated virqs are continuous, >> in stead, we use irq_alloc_descs_from() to alloc virqs in one time, >> so that the allocated virqs are in continuous bitmaps. >> >> Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com> > > (...) > >> - for (offset = 0; offset < gpiochip->ngpio; offset++) { >> - irq_base = irq_create_mapping(gpiochip->irqdomain, offset); >> - if (offset == 0) >> - /* >> - * Store the base into the gpiochip to be used when >> - * unmapping the irqs. >> - */ >> - gpiochip->irq_base = irq_base; >> + if (first_irq > 0) { >> + gpiochip->irq_base = first_irq; > > Wait is this safe? Now you assume all descriptors are pre-allocated > and associated in this case, atleast explain what is going on. > >> + } else { >> + gpiochip->irq_base = irq_alloc_descs_from(1, gpiochip->ngpio, >> + of_node_to_nid(of_node)); >> + irq_domain_associate_many(gpiochip->irqdomain, >> + gpiochip->irq_base, 0, gpiochip->ngpio); > > This part looks OK. > > I'm holding this patch back until the above is clarified. > > Yours, > Linus Walleij-- > To unsubscribe from this list: send the line "unsubscribe linux-gpio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Best regards, -grygorii ^ permalink raw reply related [flat|nested] 14+ messages in thread
* RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs 2014-09-23 13:39 ` Wang, Yalin @ 2014-09-23 14:15 ` Wang, Yalin 2014-09-23 14:51 ` Wang, Yalin 2014-09-23 15:03 ` Grygorii Strashko 0 siblings, 2 replies; 14+ messages in thread From: Wang, Yalin @ 2014-09-23 14:15 UTC (permalink / raw) To: Grygorii Strashko, Linus Walleij Cc: gnurou@gmail.com, linux-gpio@vger.kernel.org, akpm@linux-foundation.org hi but it is not safe with a little problem, when remove , should not assume physical irq start from 0, i change like this, store gpiochip->irq_base = first_irq; --- diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 15cc0bb..c5fb2c1 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -517,14 +517,14 @@ static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) */ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) { - unsigned int offset; - + int i; acpi_gpiochip_free_interrupts(gpiochip); /* Remove all IRQ mappings and delete the domain */ if (gpiochip->irqdomain) { - for (offset = 0; offset < gpiochip->ngpio; offset++) - irq_dispose_mapping(gpiochip->irq_base + offset); + for (i = 0; i < gpiochip->ngpio; i++) + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, + gpiochip->irq_base + i)); irq_domain_remove(gpiochip->irqdomain); } @@ -596,6 +596,7 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip, gpiochip->irqchip = NULL; return -EINVAL; } + gpiochip->irq_base = first_irq; irqchip->irq_request_resources = gpiochip_irq_reqres; irqchip->irq_release_resources = gpiochip_irq_relres; @@ -604,14 +605,10 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip, * any gpiochip calls. If the first_irq was zero, this is * necessary to allocate descriptors for all IRQs. */ - for (offset = 0; offset < gpiochip->ngpio; offset++) { - irq_base = irq_create_mapping(gpiochip->irqdomain, offset); - if (offset == 0) - /* - * Store the base into the gpiochip to be used when - * unmapping the irqs. - */ - gpiochip->irq_base = irq_base; + if (first_irq == 0) { + for (offset = 0; offset < gpiochip->ngpio; offset++) + irq_base = irq_create_mapping(gpiochip->irqdomain, offset); + } acpi_gpiochip_request_interrupts(gpiochip); ________________________________________ From: Wang, Yalin Sent: Tuesday, September 23, 2014 9:39 PM To: Wang, Yalin; Grygorii Strashko; Linus Walleij Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org Subject: RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs hi sorry, i don't notice Grygorii's patch , yes, this patch is more easy, it is ok to fix this problem. Great! ________________________________________ From: Wang, Yalin Sent: Tuesday, September 23, 2014 9:37 PM To: Grygorii Strashko; Linus Walleij Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org Subject: RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs Hi In fact, i don't encounter any problem about gpio code, i just find this issue when i see the source code, i feel it is not safe, so i make a patch for it. yes, you are right, "irq_base" is used only twice in gpiolib code, but it maybe used by some other drivers, if remove it, some drivers will can't get virq base. it should get it by find_irq_mapping(), but it is also ok. in fact , we can allow the virq are allocated one by one, but this need change gpiochip_irqchip_remove( ) function, it should not assume the virq are contentious, i think both method are ok , it is just decided by how you want design it :) To summarize, we should make gpiochip_irqchip_add() and gpiochip_irqchip_remove() both work correctly. ________________________________________ From: Grygorii Strashko [grygorii.strashko@ti.com] Sent: Tuesday, September 23, 2014 8:59 PM To: Wang, Yalin; Linus Walleij Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org Subject: Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs Hi Wang, On 09/23/2014 03:03 PM, Wang, Yalin wrote: > hi > > this is because , here: > > gpiochip->irqdomain = irq_domain_add_simple(of_node, > gpiochip->ngpio, first_irq, > &gpiochip_domain_ops, gpiochip); > > > irq_domain_add_simple() in this function, > if (first_irq > 0) { > if (IS_ENABLED(CONFIG_SPARSE_IRQ)) { > /* attempt to allocated irq_descs */ > int rc = irq_alloc_descs(first_irq, first_irq, size, > of_node_to_nid(of_node)); > if (rc < 0) > pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n", > first_irq); > } > irq_domain_associate_many(domain, first_irq, 0, size); > } > > if first_irq > 0 , it will allocate it , > and make sure the return virq is equal to first_irq . > so we don't need allocate it again . Could provide a little bit more information about issue you've observed, pls? As for me, you patch will completely disable Sparse IRQ feature :( Also, I'm sure that struct gpio_chip->irq_base field can be removed from gpiolib irqchip code - GPIO drivers shouldn't use it also, because otherwise they will be incompatible with Sparse IRQ feature. Now the "irq_base" is used only twice in gpiolib code and below diff should allow to drop it completely from gpiolib code. diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 15cc0bb..81762ed 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -524,7 +524,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) /* Remove all IRQ mappings and delete the domain */ if (gpiochip->irqdomain) { for (offset = 0; offset < gpiochip->ngpio; offset++) - irq_dispose_mapping(gpiochip->irq_base + offset); + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, offset)); irq_domain_remove(gpiochip->irqdomain); } not tested. > ________________________________________ > From: Linus Walleij [linus.walleij@linaro.org] > Sent: Tuesday, September 23, 2014 6:21 PM > To: Wang, Yalin > Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org > Subject: Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs > > On Tue, Sep 9, 2014 at 9:12 AM, Wang, Yalin <Yalin.Wang@sonymobile.com> wrote: > >> this patch change use from irq_create_mapping to irq_alloc_descs_from, >> use irq_create_mapping to alloc virq one by one is not safe, >> it can't promise the allcated virqs are continuous, >> in stead, we use irq_alloc_descs_from() to alloc virqs in one time, >> so that the allocated virqs are in continuous bitmaps. >> >> Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com> > > (...) > >> - for (offset = 0; offset < gpiochip->ngpio; offset++) { >> - irq_base = irq_create_mapping(gpiochip->irqdomain, offset); >> - if (offset == 0) >> - /* >> - * Store the base into the gpiochip to be used when >> - * unmapping the irqs. >> - */ >> - gpiochip->irq_base = irq_base; >> + if (first_irq > 0) { >> + gpiochip->irq_base = first_irq; > > Wait is this safe? Now you assume all descriptors are pre-allocated > and associated in this case, atleast explain what is going on. > >> + } else { >> + gpiochip->irq_base = irq_alloc_descs_from(1, gpiochip->ngpio, >> + of_node_to_nid(of_node)); >> + irq_domain_associate_many(gpiochip->irqdomain, >> + gpiochip->irq_base, 0, gpiochip->ngpio); > > This part looks OK. > > I'm holding this patch back until the above is clarified. > > Yours, > Linus Walleij-- > To unsubscribe from this list: send the line "unsubscribe linux-gpio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Best regards, -grygorii ^ permalink raw reply related [flat|nested] 14+ messages in thread
* RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs 2014-09-23 14:15 ` Wang, Yalin @ 2014-09-23 14:51 ` Wang, Yalin 2014-09-23 15:03 ` Grygorii Strashko 1 sibling, 0 replies; 14+ messages in thread From: Wang, Yalin @ 2014-09-23 14:51 UTC (permalink / raw) To: Grygorii Strashko, Linus Walleij Cc: gnurou@gmail.com, linux-gpio@vger.kernel.org, akpm@linux-foundation.org hi i make a mistake, ignore my second patch, i think Grygorii's patch is ok to fix this issue, and gpiochip->irq_base can be removed if use Grygorii's patch . A better solution ! ________________________________________ From: Wang, Yalin Sent: Tuesday, September 23, 2014 10:15 PM To: Grygorii Strashko; Linus Walleij Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org Subject: RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs hi but it is not safe with a little problem, when remove , should not assume physical irq start from 0, i change like this, store gpiochip->irq_base = first_irq; --- diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 15cc0bb..c5fb2c1 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -517,14 +517,14 @@ static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) */ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) { - unsigned int offset; - + int i; acpi_gpiochip_free_interrupts(gpiochip); /* Remove all IRQ mappings and delete the domain */ if (gpiochip->irqdomain) { - for (offset = 0; offset < gpiochip->ngpio; offset++) - irq_dispose_mapping(gpiochip->irq_base + offset); + for (i = 0; i < gpiochip->ngpio; i++) + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, + gpiochip->irq_base + i)); irq_domain_remove(gpiochip->irqdomain); } @@ -596,6 +596,7 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip, gpiochip->irqchip = NULL; return -EINVAL; } + gpiochip->irq_base = first_irq; irqchip->irq_request_resources = gpiochip_irq_reqres; irqchip->irq_release_resources = gpiochip_irq_relres; @@ -604,14 +605,10 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip, * any gpiochip calls. If the first_irq was zero, this is * necessary to allocate descriptors for all IRQs. */ - for (offset = 0; offset < gpiochip->ngpio; offset++) { - irq_base = irq_create_mapping(gpiochip->irqdomain, offset); - if (offset == 0) - /* - * Store the base into the gpiochip to be used when - * unmapping the irqs. - */ - gpiochip->irq_base = irq_base; + if (first_irq == 0) { + for (offset = 0; offset < gpiochip->ngpio; offset++) + irq_base = irq_create_mapping(gpiochip->irqdomain, offset); + } acpi_gpiochip_request_interrupts(gpiochip); ________________________________________ From: Wang, Yalin Sent: Tuesday, September 23, 2014 9:39 PM To: Wang, Yalin; Grygorii Strashko; Linus Walleij Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org Subject: RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs hi sorry, i don't notice Grygorii's patch , yes, this patch is more easy, it is ok to fix this problem. Great! ________________________________________ From: Wang, Yalin Sent: Tuesday, September 23, 2014 9:37 PM To: Grygorii Strashko; Linus Walleij Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org Subject: RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs Hi In fact, i don't encounter any problem about gpio code, i just find this issue when i see the source code, i feel it is not safe, so i make a patch for it. yes, you are right, "irq_base" is used only twice in gpiolib code, but it maybe used by some other drivers, if remove it, some drivers will can't get virq base. it should get it by find_irq_mapping(), but it is also ok. in fact , we can allow the virq are allocated one by one, but this need change gpiochip_irqchip_remove( ) function, it should not assume the virq are contentious, i think both method are ok , it is just decided by how you want design it :) To summarize, we should make gpiochip_irqchip_add() and gpiochip_irqchip_remove() both work correctly. ________________________________________ From: Grygorii Strashko [grygorii.strashko@ti.com] Sent: Tuesday, September 23, 2014 8:59 PM To: Wang, Yalin; Linus Walleij Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org Subject: Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs Hi Wang, On 09/23/2014 03:03 PM, Wang, Yalin wrote: > hi > > this is because , here: > > gpiochip->irqdomain = irq_domain_add_simple(of_node, > gpiochip->ngpio, first_irq, > &gpiochip_domain_ops, gpiochip); > > > irq_domain_add_simple() in this function, > if (first_irq > 0) { > if (IS_ENABLED(CONFIG_SPARSE_IRQ)) { > /* attempt to allocated irq_descs */ > int rc = irq_alloc_descs(first_irq, first_irq, size, > of_node_to_nid(of_node)); > if (rc < 0) > pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n", > first_irq); > } > irq_domain_associate_many(domain, first_irq, 0, size); > } > > if first_irq > 0 , it will allocate it , > and make sure the return virq is equal to first_irq . > so we don't need allocate it again . Could provide a little bit more information about issue you've observed, pls? As for me, you patch will completely disable Sparse IRQ feature :( Also, I'm sure that struct gpio_chip->irq_base field can be removed from gpiolib irqchip code - GPIO drivers shouldn't use it also, because otherwise they will be incompatible with Sparse IRQ feature. Now the "irq_base" is used only twice in gpiolib code and below diff should allow to drop it completely from gpiolib code. diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 15cc0bb..81762ed 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -524,7 +524,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) /* Remove all IRQ mappings and delete the domain */ if (gpiochip->irqdomain) { for (offset = 0; offset < gpiochip->ngpio; offset++) - irq_dispose_mapping(gpiochip->irq_base + offset); + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, offset)); irq_domain_remove(gpiochip->irqdomain); } not tested. > ________________________________________ > From: Linus Walleij [linus.walleij@linaro.org] > Sent: Tuesday, September 23, 2014 6:21 PM > To: Wang, Yalin > Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org > Subject: Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs > > On Tue, Sep 9, 2014 at 9:12 AM, Wang, Yalin <Yalin.Wang@sonymobile.com> wrote: > >> this patch change use from irq_create_mapping to irq_alloc_descs_from, >> use irq_create_mapping to alloc virq one by one is not safe, >> it can't promise the allcated virqs are continuous, >> in stead, we use irq_alloc_descs_from() to alloc virqs in one time, >> so that the allocated virqs are in continuous bitmaps. >> >> Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com> > > (...) > >> - for (offset = 0; offset < gpiochip->ngpio; offset++) { >> - irq_base = irq_create_mapping(gpiochip->irqdomain, offset); >> - if (offset == 0) >> - /* >> - * Store the base into the gpiochip to be used when >> - * unmapping the irqs. >> - */ >> - gpiochip->irq_base = irq_base; >> + if (first_irq > 0) { >> + gpiochip->irq_base = first_irq; > > Wait is this safe? Now you assume all descriptors are pre-allocated > and associated in this case, atleast explain what is going on. > >> + } else { >> + gpiochip->irq_base = irq_alloc_descs_from(1, gpiochip->ngpio, >> + of_node_to_nid(of_node)); >> + irq_domain_associate_many(gpiochip->irqdomain, >> + gpiochip->irq_base, 0, gpiochip->ngpio); > > This part looks OK. > > I'm holding this patch back until the above is clarified. > > Yours, > Linus Walleij-- > To unsubscribe from this list: send the line "unsubscribe linux-gpio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Best regards, -grygorii ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs 2014-09-23 14:15 ` Wang, Yalin 2014-09-23 14:51 ` Wang, Yalin @ 2014-09-23 15:03 ` Grygorii Strashko 2014-09-24 1:59 ` Wang, Yalin 1 sibling, 1 reply; 14+ messages in thread From: Grygorii Strashko @ 2014-09-23 15:03 UTC (permalink / raw) To: Wang, Yalin, Linus Walleij Cc: gnurou@gmail.com, linux-gpio@vger.kernel.org, akpm@linux-foundation.org On 09/23/2014 05:15 PM, Wang, Yalin wrote: > hi > > but it is not safe with a little problem, > when remove , should not assume physical irq start from 0, > i change like this, > store gpiochip->irq_base = first_irq; > > --- > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 15cc0bb..c5fb2c1 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -517,14 +517,14 @@ static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) > */ > static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) > { > - unsigned int offset; > - > + int i; > acpi_gpiochip_free_interrupts(gpiochip); > > /* Remove all IRQ mappings and delete the domain */ > if (gpiochip->irqdomain) { > - for (offset = 0; offset < gpiochip->ngpio; offset++) > - irq_dispose_mapping(gpiochip->irq_base + offset); > + for (i = 0; i < gpiochip->ngpio; i++) > + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, > + gpiochip->irq_base + i)); No:) it should start from 0 irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, i)); irq_domain_add_simple() uses hwirq_base == 0 and current implementation of gpiolib irqchip helpers doesn't support setting of custom hwirq_base. > irq_domain_remove(gpiochip->irqdomain); > } > > @@ -596,6 +596,7 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip, > gpiochip->irqchip = NULL; > return -EINVAL; > } > + gpiochip->irq_base = first_irq; > irqchip->irq_request_resources = gpiochip_irq_reqres; > irqchip->irq_release_resources = gpiochip_irq_relres; > > @@ -604,14 +605,10 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip, > * any gpiochip calls. If the first_irq was zero, this is > * necessary to allocate descriptors for all IRQs. > */ > - for (offset = 0; offset < gpiochip->ngpio; offset++) { > - irq_base = irq_create_mapping(gpiochip->irqdomain, offset); > - if (offset == 0) > - /* > - * Store the base into the gpiochip to be used when > - * unmapping the irqs. > - */ > - gpiochip->irq_base = irq_base; > + if (first_irq == 0) { > + for (offset = 0; offset < gpiochip->ngpio; offset++) > + irq_base = irq_create_mapping(gpiochip->irqdomain, offset); > + > } > > acpi_gpiochip_request_interrupts(gpiochip); > > > ________________________________________ > From: Wang, Yalin > Sent: Tuesday, September 23, 2014 9:39 PM > To: Wang, Yalin; Grygorii Strashko; Linus Walleij > Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org > Subject: RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs > > hi > > sorry, > i don't notice Grygorii's patch , > yes, this patch is more easy, > it is ok to fix this problem. > > Great! > ________________________________________ > From: Wang, Yalin > Sent: Tuesday, September 23, 2014 9:37 PM > To: Grygorii Strashko; Linus Walleij > Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org > Subject: RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs > > Hi > > In fact, i don't encounter any problem about gpio code, > i just find this issue when i see the source code, > i feel it is not safe, so i make a patch for it. > > yes, you are right, > "irq_base" is used only twice in gpiolib code, > but it maybe used by some other drivers, > if remove it, some drivers will can't get virq base. > it should get it by find_irq_mapping(), but it is also ok. > > in fact , we can allow the virq are allocated one by one, > but this need change gpiochip_irqchip_remove( ) function, > it should not assume the virq are contentious, > i think both method are ok , > it is just decided by how you want design it :) > > To summarize, we should make gpiochip_irqchip_add() and > gpiochip_irqchip_remove() both work correctly. > > > ________________________________________ > From: Grygorii Strashko [grygorii.strashko@ti.com] > Sent: Tuesday, September 23, 2014 8:59 PM > To: Wang, Yalin; Linus Walleij > Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org > Subject: Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs > > Hi Wang, > > On 09/23/2014 03:03 PM, Wang, Yalin wrote: >> hi >> >> this is because , here: >> >> gpiochip->irqdomain = irq_domain_add_simple(of_node, >> gpiochip->ngpio, first_irq, >> &gpiochip_domain_ops, gpiochip); >> >> >> irq_domain_add_simple() in this function, >> if (first_irq > 0) { >> if (IS_ENABLED(CONFIG_SPARSE_IRQ)) { >> /* attempt to allocated irq_descs */ >> int rc = irq_alloc_descs(first_irq, first_irq, size, >> of_node_to_nid(of_node)); >> if (rc < 0) >> pr_info("Cannot allocate irq_descs @ IRQ%d, assuming pre-allocated\n", >> first_irq); >> } >> irq_domain_associate_many(domain, first_irq, 0, size); >> } >> >> if first_irq > 0 , it will allocate it , >> and make sure the return virq is equal to first_irq . >> so we don't need allocate it again . > > Could provide a little bit more information about issue you've observed, pls? > > As for me, you patch will completely disable Sparse IRQ feature :( > > Also, I'm sure that struct gpio_chip->irq_base field can > be removed from gpiolib irqchip code - GPIO drivers shouldn't use it also, > because otherwise they will be incompatible with Sparse IRQ feature. > > Now the "irq_base" is used only twice in gpiolib code and below diff should > allow to drop it completely from gpiolib code. > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 15cc0bb..81762ed 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -524,7 +524,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) > /* Remove all IRQ mappings and delete the domain */ > if (gpiochip->irqdomain) { > for (offset = 0; offset < gpiochip->ngpio; offset++) > - irq_dispose_mapping(gpiochip->irq_base + offset); > + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, offset)); > irq_domain_remove(gpiochip->irqdomain); > } > > not tested. > > > >> ________________________________________ >> From: Linus Walleij [linus.walleij@linaro.org] >> Sent: Tuesday, September 23, 2014 6:21 PM >> To: Wang, Yalin >> Cc: gnurou@gmail.com; linux-gpio@vger.kernel.org; akpm@linux-foundation.org >> Subject: Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs >> >> On Tue, Sep 9, 2014 at 9:12 AM, Wang, Yalin <Yalin.Wang@sonymobile.com> wrote: >> >>> this patch change use from irq_create_mapping to irq_alloc_descs_from, >>> use irq_create_mapping to alloc virq one by one is not safe, >>> it can't promise the allcated virqs are continuous, >>> in stead, we use irq_alloc_descs_from() to alloc virqs in one time, >>> so that the allocated virqs are in continuous bitmaps. >>> >>> Signed-off-by: Yalin Wang <yalin.wang@sonymobile.com> >> >> (...) >> >>> - for (offset = 0; offset < gpiochip->ngpio; offset++) { >>> - irq_base = irq_create_mapping(gpiochip->irqdomain, offset); >>> - if (offset == 0) >>> - /* >>> - * Store the base into the gpiochip to be used when >>> - * unmapping the irqs. >>> - */ >>> - gpiochip->irq_base = irq_base; >>> + if (first_irq > 0) { >>> + gpiochip->irq_base = first_irq; >> >> Wait is this safe? Now you assume all descriptors are pre-allocated >> and associated in this case, atleast explain what is going on. >> >>> + } else { >>> + gpiochip->irq_base = irq_alloc_descs_from(1, gpiochip->ngpio, >>> + of_node_to_nid(of_node)); >>> + irq_domain_associate_many(gpiochip->irqdomain, >>> + gpiochip->irq_base, 0, gpiochip->ngpio); >> >> This part looks OK. >> >> I'm holding this patch back until the above is clarified. >> >> Yours, >> Linus Walleij-- >> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > Best regards, > -grygorii-- > To unsubscribe from this list: send the line "unsubscribe linux-gpio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs 2014-09-23 15:03 ` Grygorii Strashko @ 2014-09-24 1:59 ` Wang, Yalin 0 siblings, 0 replies; 14+ messages in thread From: Wang, Yalin @ 2014-09-24 1:59 UTC (permalink / raw) To: 'Grygorii Strashko', Linus Walleij Cc: gnurou@gmail.com, linux-gpio@vger.kernel.org, akpm@linux-foundation.org Hi Grygorii, Yeah, you are right, I really make a mistake . I think both our patch are ok to fix this problem, But we use different solutions. My patch have a advantage than yours that It alloc virqs in one time and when free it, Doesn't need call irqa_find_mapping() one by one, performance Is better . For my patch, you said " As for me, you patch will completely disable Sparse IRQ feature :(" Why? Could you make me clear ? :) Then we can decide which solution to use to fix this issue. Thanks -----Original Message----- > hi > > but it is not safe with a little problem, when remove , should not > assume physical irq start from 0, i change like this, store > gpiochip->irq_base = first_irq; > > --- > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index > 15cc0bb..c5fb2c1 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -517,14 +517,14 @@ static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset) > */ > static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) > { > - unsigned int offset; > - > + int i; > acpi_gpiochip_free_interrupts(gpiochip); > > /* Remove all IRQ mappings and delete the domain */ > if (gpiochip->irqdomain) { > - for (offset = 0; offset < gpiochip->ngpio; offset++) > - irq_dispose_mapping(gpiochip->irq_base + offset); > + for (i = 0; i < gpiochip->ngpio; i++) > + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, > + gpiochip->irq_base + i)); No:) it should start from 0 irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, i)); irq_domain_add_simple() uses hwirq_base == 0 and current implementation of gpiolib irqchip helpers doesn't support setting of custom hwirq_base. > irq_domain_remove(gpiochip->irqdomain); > } > > @@ -596,6 +596,7 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip, > gpiochip->irqchip = NULL; > return -EINVAL; > } > + gpiochip->irq_base = first_irq; > irqchip->irq_request_resources = gpiochip_irq_reqres; > irqchip->irq_release_resources = gpiochip_irq_relres; > > @@ -604,14 +605,10 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip, > * any gpiochip calls. If the first_irq was zero, this is > * necessary to allocate descriptors for all IRQs. > */ > - for (offset = 0; offset < gpiochip->ngpio; offset++) { > - irq_base = irq_create_mapping(gpiochip->irqdomain, offset); > - if (offset == 0) > - /* > - * Store the base into the gpiochip to be used when > - * unmapping the irqs. > - */ > - gpiochip->irq_base = irq_base; > + if (first_irq == 0) { > + for (offset = 0; offset < gpiochip->ngpio; offset++) > + irq_base = irq_create_mapping(gpiochip->irqdomain, offset); > + > } > > acpi_gpiochip_request_interrupts(gpiochip); > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs 2014-09-23 12:59 ` Grygorii Strashko 2014-09-23 13:37 ` Wang, Yalin @ 2014-09-25 7:39 ` Linus Walleij 2014-09-25 7:47 ` Wang, Yalin 1 sibling, 1 reply; 14+ messages in thread From: Linus Walleij @ 2014-09-25 7:39 UTC (permalink / raw) To: Grygorii Strashko Cc: Wang, Yalin, gnurou@gmail.com, linux-gpio@vger.kernel.org, akpm@linux-foundation.org On Tue, Sep 23, 2014 at 2:59 PM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > Now the "irq_base" is used only twice in gpiolib code and below diff should > allow to drop it completely from gpiolib code. > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 15cc0bb..81762ed 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -524,7 +524,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip *gpiochip) > /* Remove all IRQ mappings and delete the domain */ > if (gpiochip->irqdomain) { > for (offset = 0; offset < gpiochip->ngpio; offset++) > - irq_dispose_mapping(gpiochip->irq_base + offset); > + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, offset)); > irq_domain_remove(gpiochip->irqdomain); > } > > not tested. I like the looks of this. Grygorii, can you send a proper, tested patch for this? Thansk! Yours, Linus Walleij ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs 2014-09-25 7:39 ` Linus Walleij @ 2014-09-25 7:47 ` Wang, Yalin 2014-09-25 13:18 ` Linus Walleij 0 siblings, 1 reply; 14+ messages in thread From: Wang, Yalin @ 2014-09-25 7:47 UTC (permalink / raw) To: 'Linus Walleij', Grygorii Strashko Cc: gnurou@gmail.com, linux-gpio@vger.kernel.org, akpm@linux-foundation.org > From: Linus Walleij [mailto:linus.walleij@linaro.org] > > On Tue, Sep 23, 2014 at 2:59 PM, Grygorii Strashko > <grygorii.strashko@ti.com> wrote: > > > Now the "irq_base" is used only twice in gpiolib code and below diff > > should allow to drop it completely from gpiolib code. > > > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index > > 15cc0bb..81762ed 100644 > > --- a/drivers/gpio/gpiolib.c > > +++ b/drivers/gpio/gpiolib.c > > @@ -524,7 +524,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip > *gpiochip) > > /* Remove all IRQ mappings and delete the domain */ > > if (gpiochip->irqdomain) { > > for (offset = 0; offset < gpiochip->ngpio; offset++) > > - irq_dispose_mapping(gpiochip->irq_base + offset); > > + > > + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, offset)); > > irq_domain_remove(gpiochip->irqdomain); > > } > > > > not tested. > > I like the looks of this. > > Grygorii, can you send a proper, tested patch for this? Thansk! Could we also remove gpiochip->irq_base if use this solution? It seems gpiochip->irq_base is useless . Thanks ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs 2014-09-25 7:47 ` Wang, Yalin @ 2014-09-25 13:18 ` Linus Walleij 0 siblings, 0 replies; 14+ messages in thread From: Linus Walleij @ 2014-09-25 13:18 UTC (permalink / raw) To: Wang, Yalin Cc: Grygorii Strashko, gnurou@gmail.com, linux-gpio@vger.kernel.org, akpm@linux-foundation.org On Thu, Sep 25, 2014 at 9:47 AM, Wang, Yalin <Yalin.Wang@sonymobile.com> wrote: >> From: Linus Walleij [mailto:linus.walleij@linaro.org] >> >> On Tue, Sep 23, 2014 at 2:59 PM, Grygorii Strashko >> <grygorii.strashko@ti.com> wrote: >> >> > Now the "irq_base" is used only twice in gpiolib code and below diff >> > should allow to drop it completely from gpiolib code. >> > >> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index >> > 15cc0bb..81762ed 100644 >> > --- a/drivers/gpio/gpiolib.c >> > +++ b/drivers/gpio/gpiolib.c >> > @@ -524,7 +524,7 @@ static void gpiochip_irqchip_remove(struct gpio_chip >> *gpiochip) >> > /* Remove all IRQ mappings and delete the domain */ >> > if (gpiochip->irqdomain) { >> > for (offset = 0; offset < gpiochip->ngpio; offset++) >> > - irq_dispose_mapping(gpiochip->irq_base + offset); >> > + >> > + irq_dispose_mapping(irq_find_mapping(gpiochip->irqdomain, offset)); >> > irq_domain_remove(gpiochip->irqdomain); >> > } >> > >> > not tested. >> >> I like the looks of this. >> >> Grygorii, can you send a proper, tested patch for this? Thansk! > > Could we also remove gpiochip->irq_base if use this solution? > It seems gpiochip->irq_base is useless . I think it is used in some drivers and especially for pin ranges visavis the pin control subsystem, so that could be a complex operation. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-09-25 13:18 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-09 7:12 [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs Wang, Yalin 2014-09-23 10:17 ` Linus Walleij 2014-09-23 10:21 ` Linus Walleij 2014-09-23 12:03 ` Wang, Yalin 2014-09-23 12:59 ` Grygorii Strashko 2014-09-23 13:37 ` Wang, Yalin 2014-09-23 13:39 ` Wang, Yalin 2014-09-23 14:15 ` Wang, Yalin 2014-09-23 14:51 ` Wang, Yalin 2014-09-23 15:03 ` Grygorii Strashko 2014-09-24 1:59 ` Wang, Yalin 2014-09-25 7:39 ` Linus Walleij 2014-09-25 7:47 ` Wang, Yalin 2014-09-25 13:18 ` Linus Walleij
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.