amba-pl08x and 'get_signal' namespace collision/build error

PATCH Jonathan Austin 7 participants 15 messages Jun 25, 2013 – Jul 8, 2013 Arm (32-bit)

Share & tools

Jonathan Austin

author

2013-06-25 11:30 UTC #0 lore raw

Hi all,

There's a patch making its way to mainline via Russell's tree (8d96250700: ARM: mm: Transparent huge page support for LPAE systems) that breaks the build of the amba-pl08x driver (drivers/dma/amba-pl08x.c) because the 'get_signal' macro from include/linux/signal.h is now in the driver's scope and it clobbers a (previously) valid function call.

The error:
drivers/dma/amba-pl08x.c: In function ?pl08x_request_mux?:
drivers/dma/amba-pl08x.c:303:13: error: expected identifier before ?(? token

Here's the problematic include:
In file included from include/linux/sched.h:32:0,
--new include--> from /data/build/a32/linux/arch/arm/include/asm/tlbflush.h:204,

                 from /data/build/a32/linux/arch/arm/include/asm/pgtable.h:28,
                 from include/linux/mm.h:44,
                 from include/linux/scatterlist.h:6,
                 from include/linux/dmaengine.h:27,
                 from include/linux/amba/pl08x.h:21,
                 from drivers/dma/amba-pl08x.c:74:

include/linux/signal.h:298:2: error: #error get_signal defined here

Below is a 'fix' that *doesn't* require any renaming of either the get_signal function or macro, but there's nothing to say that driver shouldn't be able to use the pl08x_platform_data struct *and* do scheduling so this doesn't seem like the best fix really.

So can we do something better?

Al: the commit where you add the get_signal() helper suggests you want to make it a function not a macro in the future - what needs to be done before that can happen? I think that would be the best/easiest fix.

Jonny

----------8<-----------

arch/arm/include/asm/tlbflush.h +4 -1

index c78efbc..ce1185c 100644
@@ -201,7 +201,10 @@
 
 #ifndef __ASSEMBLY__
 
-#include <linux/sched.h>
+#include <linux/smp.h>
+#include <linux/mm_types.h>
+
+#include <asm/barrier.h>
 
 struct cpu_tlb_fns {
        void (*flush_user_range)(unsigned long, unsigned long, struct vm_area_struct *);

arch/arm/kernel/suspend.c +1 -0

index 38a5067..3100983 100644
@@ -1,4 +1,5 @@
 #include <linux/init.h>
+#include <linux/sched.h>
 
 #include <asm/idmap.h>
 #include <asm/pgalloc.h>

arch/arm/mm/idmap.c +1 -0

index 83cb3ac..304e8ce 100644
@@ -1,6 +1,7 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
+#include <linux/sched.h>
 
 #include <asm/cputype.h>
 #include <asm/idmap.h>

arch/arm/mm/mm.h +1 -0

index d5a4e9a..6eeaf3d 100644
@@ -1,6 +1,7 @@
 #ifdef CONFIG_MMU
 #include <linux/list.h>
 #include <linux/vmalloc.h>
+#include <linux/sched.h>
 
 /* the upper-most page table pointer */
 extern pmd_t *top_pmd;

Linus Walleij

2013-06-25 13:49 UTC #1 lore raw

Quoted 5 lines · Jonathan Austin

There's a patch making its way to mainline via Russell's tree (8d96250700: ARM: mm: Transparent huge page support for LPAE systems) that breaks the build of the amba-pl08x driver (drivers/dma/amba-pl08x.c) because the 'get_signal' macro from include/linux/signal.h is now in the driver's scope and it clobbers a (previously) valid function call.

This is already fixed by Mark Brown in the DMA tree.

Commit subject:
"dmaengine: PL08x: Avoid collisions with get_signal() macro"

Yours,
Linus Walleij

Russell King - ARM Linux

2013-06-25 18:45 UTC #2 lore raw

In reply to Jonathan Austin

Quoted 5 lines · Jonathan Austin

There's a patch making its way to mainline via Russell's tree (8d96250700: ARM: mm: Transparent huge page support for LPAE systems) that breaks the build of the amba-pl08x driver (drivers/dma/amba-pl08x.c) because the 'get_signal' macro from include/linux/signal.h is now in the driver's scope and it clobbers a (previously) valid function call.

Well, here's the change to asm/pgtable.h in that patch:

arch/arm/include/asm/pgtable.h +3 -0

index 9bcd262..eaedce7 100644
@@ -24,6 +24,9 @@
 #include <asm/memory.h>
 #include <asm/pgtable-hwdef.h>
 
+
+#include <asm/tlbflush.h>
+
 #ifdef CONFIG_ARM_LPAE
 #include <asm/pgtable-3level.h>
 #else

And the question is - if that's all that is going on in that file, why is asm/tlbflush.h being added to it? What in _that_ file uses anything from asm/tlbflush.h (nothing apparantly from what I can see)?

So, I'm tempted to kill this change off unless someone can justify why that addition happened - it looks completely inappropriate to me.

Vinod Koul

2013-06-26 04:48 UTC #3 lore raw

Quoted 24 lines · Russell King - ARM Linux

Quoted 5 lines · Jonathan Austin
There's a patch making its way to mainline via Russell's tree (8d96250700: ARM: mm: Transparent huge page support for LPAE systems) that breaks the build of the amba-pl08x driver (drivers/dma/amba-pl08x.c) because the 'get_signal' macro from include/linux/signal.h is now in the driver's scope and it clobbers a (previously) valid function call.
Well, here's the change to asm/pgtable.h in that patch:
arch/arm/include/asm/pgtable.h +3 -0
index 9bcd262..eaedce7 100644
@@ -24,6 +24,9 @@
 #include <asm/memory.h>
 #include <asm/pgtable-hwdef.h>
 
+
+#include <asm/tlbflush.h>
+
 #ifdef CONFIG_ARM_LPAE
 #include <asm/pgtable-3level.h>
 #else

Quoted 6 lines

And the question is - if that's all that is going on in that file, why is asm/tlbflush.h being added to it? What in _that_ file uses anything from asm/tlbflush.h (nothing apparantly from what I can see)?
So, I'm tempted to kill this change off unless someone can justify why that addition happened - it looks completely inappropriate to me.

Yup. This is fixed in slave-dma tree by a patch from mark by renaming it.

This should not show in the -next tree

--
~Vinod

Russell King - ARM Linux

2013-06-26 08:19 UTC #4 lore raw

Quoted 3 lines · Vinod Koul

Yup. This is fixed in slave-dma tree by a patch from mark by renaming it.
This should not show in the -next tree

Except, that change probably is probably responsible for this new error:

drivers/dma/amba-pl08x.c: In function 'pl08x_request_mux':
drivers/dma/amba-pl08x.c:304:13: error: expected identifier before '(' token

Vinod Koul

2013-06-26 10:39 UTC #5 lore raw

Quoted 9 lines · Russell King - ARM Linux

Quoted 3 lines · Vinod Koul
Yup. This is fixed in slave-dma tree by a patch from mark by renaming it.
This should not show in the -next tree
Except, that change probably is probably responsible for this new error:
drivers/dma/amba-pl08x.c: In function 'pl08x_request_mux':
drivers/dma/amba-pl08x.c:304:13: error: expected identifier before '(' token

This was pushed four days ago and havent seen any error report expect this.

Anyway I rebuild my -next and looks fine for me

  CC      drivers/dma/dmaengine.o
  CC      drivers/dma/virt-dma.o
  CC      drivers/dma/iovlock.o
  CC [M]  drivers/dma/dmatest.o
  CC      drivers/dma/amba-pl08x.o
  LD      drivers/dma/built-in.o

Can you share your config which failed for you?

Mark, I believe you are testing for this, have you seen above error?

--
~Vinod

Mark Brown

2013-06-26 12:00 UTC #6 lore raw

Quoted 18 lines · Vinod Koul

Quoted 4 lines · Russell King - ARM Linux
Except, that change probably is probably responsible for this new error:
drivers/dma/amba-pl08x.c: In function 'pl08x_request_mux':
drivers/dma/amba-pl08x.c:304:13: error: expected identifier before '(' token
This was pushed four days ago and havent seen any error report expect this.
Anyway I rebuild my -next and looks fine for me
  CC      drivers/dma/dmaengine.o
  CC      drivers/dma/virt-dma.o
  CC      drivers/dma/iovlock.o
  CC [M]  drivers/dma/dmatest.o
  CC      drivers/dma/amba-pl08x.o
  LD      drivers/dma/built-in.o

Quoted 3 lines

Can you share your config which failed for you?
Mark, I believe you are testing for this, have you seen above error?

No, that looks like the original error that was being fixed. The driver build tested OK for me at the time and currently in -next, though a tree which has the additional include but not the rename away from get_signal will obviously still have the issue.

BTW for build coverage it'd be handy if AMBA could be enabled without
having to be explicitly selected.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130626/1ef851cb/attachment.sig>

Russell King - ARM Linux

2013-06-27 09:46 UTC #7 lore raw

In reply to Vinod Koul

Quoted 21 lines · Vinod Koul

Quoted 9 lines · Russell King - ARM Linux
Quoted 3 lines · Vinod Koul
Yup. This is fixed in slave-dma tree by a patch from mark by renaming it.
This should not show in the -next tree
Except, that change probably is probably responsible for this new error:
drivers/dma/amba-pl08x.c: In function 'pl08x_request_mux':
drivers/dma/amba-pl08x.c:304:13: error: expected identifier before '(' token
This was pushed four days ago and havent seen any error report expect this.
Anyway I rebuild my -next and looks fine for me
  CC      drivers/dma/dmaengine.o
  CC      drivers/dma/virt-dma.o
  CC      drivers/dma/iovlock.o
  CC [M]  drivers/dma/dmatest.o
  CC      drivers/dma/amba-pl08x.o
  LD      drivers/dma/built-in.o

Quoted 3 lines

Can you share your config which failed for you?
Mark, I believe you are testing for this, have you seen above error?

Right, what's going on is that I have a commit in my tree which adds an include to asm/pgtable.h which introduces a #define for get_signal. This becomes visible to amba-pl08x.c.

However, my build test tree fails *every* time because what I'm building is Linus' tree, my tree plus arm-soc, and it doesn't include your tree. This makes my build testing for the next merge window impossible.

So, I need the fix to pl08x.c merged into my tree.

Olof Johansson

2013-07-03 18:27 UTC #8 lore raw

Hi,

Quoted 30 lines · Russell King - ARM Linux

Quoted 21 lines · Vinod Koul
Quoted 9 lines · Russell King - ARM Linux
Quoted 3 lines · Vinod Koul
Yup. This is fixed in slave-dma tree by a patch from mark by renaming it.
This should not show in the -next tree
Except, that change probably is probably responsible for this new error:
drivers/dma/amba-pl08x.c: In function 'pl08x_request_mux':
drivers/dma/amba-pl08x.c:304:13: error: expected identifier before '(' token
This was pushed four days ago and havent seen any error report expect this.
Anyway I rebuild my -next and looks fine for me
  CC      drivers/dma/dmaengine.o
  CC      drivers/dma/virt-dma.o
  CC      drivers/dma/iovlock.o
  CC [M]  drivers/dma/dmatest.o
  CC      drivers/dma/amba-pl08x.o
  LD      drivers/dma/built-in.o
Quoted 3 lines
Can you share your config which failed for you?
Mark, I believe you are testing for this, have you seen above error?
Right, what's going on is that I have a commit in my tree which adds an include to asm/pgtable.h which introduces a #define for get_signal. This becomes visible to amba-pl08x.c.

Quoted 5 lines

However, my build test tree fails *every* time because what I'm building is Linus' tree, my tree plus arm-soc, and it doesn't include your tree. This makes my build testing for the next merge window impossible.
So, I need the fix to pl08x.c merged into my tree.

This has now hit the mainline kernel, and several defconfigs (nhk815, lpc32xx and the spear ones) are broken there.

Vinod, when are you sending up your pull request with the fix? It'd be good to see it go in soon.

-Olof

Vinod Koul

2013-07-05 06:18 UTC #9 lore raw

Quoted 5 lines · Olof Johansson

This has now hit the mainline kernel, and several defconfigs (nhk815, lpc32xx and the spear ones) are broken there.
Vinod, when are you sending up your pull request with the fix? It'd be good to see it go in soon.

It should be on its way by the weekend

Thanks
~Vinod

Russell King - ARM Linux

2013-07-07 12:32 UTC #10 lore raw

Quoted 7 lines · Vinod Koul

Quoted 5 lines · Olof Johansson
This has now hit the mainline kernel, and several defconfigs (nhk815, lpc32xx and the spear ones) are broken there.
Vinod, when are you sending up your pull request with the fix? It'd be good to see it go in soon.
It should be on its way by the weekend

It still isn't in the tree.

Personally, I think that the fix should have come via my tree, because the breakage was caused my the changes in my tree - and for the last week or more I've not been able to build any of these ARM Ltd devel platforms because of this bug.

It also means that I'm not able to build test a load of changes I'm currently working on against a recent kernel (or indeed any v3.10 kernel.) In other words, the lack of this patch being merged is stopping me from doing my job properly. (Consider from your perspective: if I were to break something you relied upon for a couple of weeks, how would you feel about it?)

Please get this fix into mainline ASAP - even if you have to send it as a patch separately to Linus rather than putting it as part of your normal pull request.

Thanks.

Vinod Koul

2013-07-07 13:34 UTC #11 lore raw

Quoted 10 lines · Russell King - ARM Linux

Quoted 7 lines · Vinod Koul
Quoted 5 lines · Olof Johansson
This has now hit the mainline kernel, and several defconfigs (nhk815, lpc32xx and the spear ones) are broken there.
Vinod, when are you sending up your pull request with the fix? It'd be good to see it go in soon.
It should be on its way by the weekend
It still isn't in the tree.

I sent this morning, depends on how soon can linus merge this

Quoted 13 lines

Personally, I think that the fix should have come via my tree, because the breakage was caused my the changes in my tree - and for the last week or more I've not been able to build any of these ARM Ltd devel platforms because of this bug.
It also means that I'm not able to build test a load of changes I'm currently working on against a recent kernel (or indeed any v3.10 kernel.) In other words, the lack of this patch being merged is stopping me from doing my job properly. (Consider from your perspective: if I were to break something you relied upon for a couple of weeks, how would you feel about it?)

Quoted 3 lines

Please get this fix into mainline ASAP - even if you have to send it as a patch separately to Linus rather than putting it as part of your normal pull request.

Yes it could have gone thur your tree as well, or you could have merged my tree to resolve your breakage

--
Thanks
~Vinod

Vinod Koul

2013-07-08 02:04 UTC #12 lore raw

Quoted 6 lines · Vinod Koul

Quoted 5 lines · Russell King - ARM Linux
Quoted 4 lines · Vinod Koul
Quoted 3 lines · Olof Johansson
This has now hit the mainline kernel, and several defconfigs (nhk815, lpc32xx and the spear ones) are broken there.

Quoted 6 lines

Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
Vinod, when are you sending up your pull request with the fix? It'd be good to see it go in soon.
It should be on its way by the weekend
It still isn't in the tree.
I sent this morning, depends on how soon can linus merge this

Linus has merged this, so please merge the linus's tree and you should be happy :)

~Vinod

Steve Capper

2013-06-26 08:40 UTC #13 lore raw

In reply to Russell King - ARM Linux

Quoted 24 lines · Russell King - ARM Linux

Quoted 5 lines · Jonathan Austin
There's a patch making its way to mainline via Russell's tree (8d96250700: ARM: mm: Transparent huge page support for LPAE systems) that breaks the build of the amba-pl08x driver (drivers/dma/amba-pl08x.c) because the 'get_signal' macro from include/linux/signal.h is now in the driver's scope and it clobbers a (previously) valid function call.
Well, here's the change to asm/pgtable.h in that patch:
arch/arm/include/asm/pgtable.h +3 -0
index 9bcd262..eaedce7 100644
@@ -24,6 +24,9 @@
 #include <asm/memory.h>
 #include <asm/pgtable-hwdef.h>
 
+
+#include <asm/tlbflush.h>
+
 #ifdef CONFIG_ARM_LPAE
 #include <asm/pgtable-3level.h>
 #else

Quoted 7 lines

And the question is - if that's all that is going on in that file, why is asm/tlbflush.h being added to it? What in _that_ file uses anything from asm/tlbflush.h (nothing apparantly from what I can see)?
So, I'm tempted to kill this change off unless someone can justify why that addition happened - it looks completely inappropriate to me.

Hi Russell,
I needed tlbflush.h for the definition of flush_pmd_entry, called by set_pmd_at in pgtable-3level.h.

It was pulled into pgtable.h as it would also be needed for the equivalent set_pmd_at function for 2-levels of paging.

Best,

Signature · 1 line

Steve

Steve Capper

2013-06-26 09:13 UTC #14 lore raw

Quoted 37 lines · Steve Capper

Quoted 24 lines · Russell King - ARM Linux
Quoted 5 lines · Jonathan Austin
There's a patch making its way to mainline via Russell's tree (8d96250700: ARM: mm: Transparent huge page support for LPAE systems) that breaks the build of the amba-pl08x driver (drivers/dma/amba-pl08x.c) because the 'get_signal' macro from include/linux/signal.h is now in the driver's scope and it clobbers a (previously) valid function call.
Well, here's the change to asm/pgtable.h in that patch:
arch/arm/include/asm/pgtable.h +3 -0
index 9bcd262..eaedce7 100644
@@ -24,6 +24,9 @@
 #include <asm/memory.h>
 #include <asm/pgtable-hwdef.h>
 
+
+#include <asm/tlbflush.h>
+
 #ifdef CONFIG_ARM_LPAE
 #include <asm/pgtable-3level.h>
 #else
Quoted 7 lines
And the question is - if that's all that is going on in that file, why is asm/tlbflush.h being added to it? What in _that_ file uses anything from asm/tlbflush.h (nothing apparantly from what I can see)?
So, I'm tempted to kill this change off unless someone can justify why that addition happened - it looks completely inappropriate to me.
Hi Russell,
I needed tlbflush.h for the definition of flush_pmd_entry, called by set_pmd_at in pgtable-3level.h.

Quoted 3 lines

It was pulled into pgtable.h as it would also be needed for the equivalent set_pmd_at function for 2-levels of paging.

I was a little quick sending this sorry....

I have tried converting set_pmd_at into a macro to allow for delayed expansion. (As I've noticed that other functions in pgtable-3level.h do this to call flush_pmd_entry.)

Unfortunately, even with set_pmd_at defined as a macro, I get undefined references to flush_pmd_entry and clean_pmd_entry (called from pmdp_test_and_clear_young and pmdp_get_and_clear in asm-generic/pgtable.h).

I'm having a look to see if there's anything else I can do. Apologies for causing this problem.

Best,

Signature · 1 line

Steve