Skip to contentSkip to conversation

gpio-ml-ioh: Support interrupt function

DISCUSSION Dan Carpenter 3 participants 7 messages Oct 17, 2011 – Oct 18, 2011

lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

Hi,

The linux-next patch 54be566317b6 "gpio-ml-ioh: Support interrupt function" generates a Sparse warning.

+ irq_base = irq_alloc_descs(-1, IOH_IRQ_BASE, num_ports[j],

+                                          GFP_KERNEL);
                                           ^^^^^^^^^^

The last argument should be a NUMA node, not a GFP_ flag. I'm not sure what the right fix is. There are currently 5 callers in my cscope for this function in linux-next.

2 pass GFP_KERNEL which is wrong.
2 pass 0 which maybe should be cpu_to_node(0)?
1 passes -1 which maybe could be NUMA_NO_NODE?

I'm probably embarrassing myself with my ignorance here, but I do know that GFP_KERNEL isn't right.

regards,
dan carpenter

#1 lore raw

Hi,

Quoted 7 lines · Dan Carpenter

The linux-next patch 54be566317b6 "gpio-ml-ioh: Support interrupt function" generates a Sparse warning.

(quoted diff) +2 -0
+               irq_base = irq_alloc_descs(-1, IOH_IRQ_BASE, num_ports[j],
+                                          GFP_KERNEL);
                                            ^^^^^^^^^^
Quoted 8 lines

The last argument should be a NUMA node, not a GFP_ flag. I'm not sure what the right fix is. There are currently 5 callers in my cscope for this function in linux-next.

2 pass GFP_KERNEL which is wrong.
2 pass 0 which maybe should be cpu_to_node(0)?
1 passes -1 which maybe could be NUMA_NO_NODE?

I can understand your saying.
Seeing accepted other drivers, '0' or '-1' is used. Focusing on GPIO driver, gpio-pca953x.c uses '-1'.

Though I tried to understand source code of irq_alloc_descs, I can't understand which value we should use.

Quoted 1 line

Grant,

Could you help me ?

Signature · 2 lines
tomoya
ROHM Co., Ltd.
#2 lore raw
Quoted 7 lines · Tomoya MORINAGA
Quoted 7 lines

The linux-next patch 54be566317b6 "gpio-ml-ioh: Support interrupt function" generates a Sparse warning.

(quoted diff) +2 -0
+               irq_base = irq_alloc_descs(-1, IOH_IRQ_BASE, num_ports[j],
+                                          GFP_KERNEL);
                                            ^^^^^^^^^^
Quoted 5 lines
Quoted 5 lines

The last argument should be a NUMA node, not a GFP_ flag. I'm not sure what the right fix is. There are currently 5 callers in my cscope for this function in linux-next.

2 pass GFP_KERNEL which is wrong.

Right, all irq_alloc_descs() allocations are already implicitly GFP_KERNEL.

Quoted 5 lines
Quoted 3 lines

2 pass 0 which maybe should be cpu_to_node(0)?
1 passes -1 which maybe could be NUMA_NO_NODE?

I can understand your saying.
Seeing accepted other drivers, '0' or '-1' is used.

There's actually no guarantee in the kernel that node 0 has memory, so unless that is assured in the context then passing 0 would be wrong.

Quoted 2 lines

Focusing on GPIO driver, gpio-pca953x.c uses '-1'.

The slab layer guarantees that passing -1 will allocate on the node that is local to the cpu that the code is running on. Unless there's a compelling reason to allocate on a different node, then this is what you want to use.

We try not to pass -1 directly, though, we try to use NUMA_NO_NODE wherever possible.

#3 lore raw
Quoted 30 lines · David Rientjes
Quoted 7 lines · Tomoya MORINAGA
Quoted 7 lines

The linux-next patch 54be566317b6 "gpio-ml-ioh: Support interrupt function" generates a Sparse warning.

(quoted diff) +2 -0
+               irq_base = irq_alloc_descs(-1, IOH_IRQ_BASE, num_ports[j],
+                                          GFP_KERNEL);
                                             ^^^^^^^^^^
Quoted 5 lines
Quoted 5 lines

The last argument should be a NUMA node, not a GFP_ flag. I'm not sure what the right fix is. There are currently 5 callers in my cscope for this function in linux-next.

2 pass GFP_KERNEL which is wrong.

Right, all irq_alloc_descs() allocations are already implicitly GFP_KERNEL.

Quoted 5 lines
Quoted 3 lines

2 pass 0 which maybe should be cpu_to_node(0)?
1 passes -1 which maybe could be NUMA_NO_NODE?

I can understand your saying.
Seeing accepted other drivers, '0' or '-1' is used.

There's actually no guarantee in the kernel that node 0 has memory, so unless that is assured in the context then passing 0 would be wrong.

Quoted 2 lines

Focusing on GPIO driver, gpio-pca953x.c uses '-1'.

Quoted 8 lines

The slab layer guarantees that passing -1 will allocate on the node that is local to the cpu that the code is running on. Unless there's a compelling reason to allocate on a different node, then this is what you want to use.

We try not to pass -1 directly, though, we try to use NUMA_NO_NODE wherever possible.

Hi, David
Thank you for your information.
I will try to test with '-1'.

After testing with '-1',
I will post the patch for this.

Thanks,

Signature · 2 lines
tomoya
ROHM Co., Ltd.
#4 lore raw
Quoted 7 lines · Tomoya MORINAGA

Hi, David
Thank you for your information.
I will try to test with '-1'.

After testing with '-1',
I will post the patch for this.

Please try with NUMA_NO_NODE, that's what the slab allocators use and will be the ones consuming this. It's available in include/linux/numa.h.

#5 lore raw
Quoted 6 lines · David Rientjes
Quoted 4 lines · Tomoya MORINAGA

Hi, David
Thank you for your information.
I will try to test with '-1'.

Quoted 6 lines
Quoted 3 lines

After testing with '-1',
I will post the patch for this.

Please try with NUMA_NO_NODE, that's what the slab allocators use and will be the ones consuming this. It's available in include/linux/numa.h.

OK, I'll use NUMA_NO_NODE instead '-1'.

thanks,

Signature · 2 lines
tomoya
ROHM Co., Ltd.
#6 lore raw
In reply to David Rientjes
Quoted 30 lines · David Rientjes
Quoted 7 lines · Tomoya MORINAGA
Quoted 7 lines

The linux-next patch 54be566317b6 "gpio-ml-ioh: Support interrupt function" generates a Sparse warning.

(quoted diff) +2 -0
+               irq_base = irq_alloc_descs(-1, IOH_IRQ_BASE, num_ports[j],
+                                          GFP_KERNEL);
                                             ^^^^^^^^^^
Quoted 5 lines
Quoted 5 lines

The last argument should be a NUMA node, not a GFP_ flag. I'm not sure what the right fix is. There are currently 5 callers in my cscope for this function in linux-next.

2 pass GFP_KERNEL which is wrong.

Right, all irq_alloc_descs() allocations are already implicitly GFP_KERNEL.

Quoted 5 lines
Quoted 3 lines

2 pass 0 which maybe should be cpu_to_node(0)?
1 passes -1 which maybe could be NUMA_NO_NODE?

I can understand your saying.
Seeing accepted other drivers, '0' or '-1' is used.

There's actually no guarantee in the kernel that node 0 has memory, so unless that is assured in the context then passing 0 would be wrong.

Quoted 2 lines

Focusing on GPIO driver, gpio-pca953x.c uses '-1'.

Quoted 8 lines

The slab layer guarantees that passing -1 will allocate on the node that is local to the cpu that the code is running on. Unless there's a compelling reason to allocate on a different node, then this is what you want to use.

We try not to pass -1 directly, though, we try to use NUMA_NO_NODE wherever possible.
--

Using NUMA_NO_NODE, I confirmed our gpio(gpio-pch) works well. I will submit the patch both gpio-pch and gpio-ml-ioh soon.

thanks,

Signature · 2 lines
tomoya
ROHM Co., Ltd.