RE: [PATCH] gpiolib:change to use irq_alloc_descs_from to alloc virqs

["Wang, Yalin" <Yalin.Wang@xxxxxxxxxxxxxx>]

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@xxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx
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@xxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx
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@xxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx
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@xxxxxx]
Sent: Tuesday, September 23, 2014 8:59 PM
To: Wang, Yalin; Linus Walleij
Cc: gnurou@xxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx
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@xxxxxxxxxx]
> Sent: Tuesday, September 23, 2014 6:21 PM
> To: Wang, Yalin
> Cc: gnurou@xxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>
> (...)
>
>> -       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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html