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

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

[Jonathan Austin]

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<-----------
diff --git a/arch/arm/include/asm/tlbflush.h b/arch/arm/include/asm/tlbflush.h
index c78efbc..ce1185c 100644
--- a/arch/arm/include/asm/tlbflush.h
+++ b/arch/arm/include/asm/tlbflush.h
@@ -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 *);
diff --git a/arch/arm/kernel/suspend.c b/arch/arm/kernel/suspend.c
index 38a5067..3100983 100644
--- a/arch/arm/kernel/suspend.c
+++ b/arch/arm/kernel/suspend.c
@@ -1,4 +1,5 @@
 #include <linux/init.h>
+#include <linux/sched.h>
 
 #include <asm/idmap.h>
 #include <asm/pgalloc.h>
diff --git a/arch/arm/mm/idmap.c b/arch/arm/mm/idmap.c
index 83cb3ac..304e8ce 100644
--- a/arch/arm/mm/idmap.c
+++ b/arch/arm/mm/idmap.c
@@ -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>
diff --git a/arch/arm/mm/mm.h b/arch/arm/mm/mm.h
index d5a4e9a..6eeaf3d 100644
--- a/arch/arm/mm/mm.h
+++ b/arch/arm/mm/mm.h
@@ -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;

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

[Linus Walleij]

On Tue, Jun 25, 2013 at 1:30 PM, Jonathan Austin
<jonathan.austin@arm.com> wrote:

> 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

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

[Russell King - ARM Linux]

On Tue, Jun 25, 2013 at 12:30:08PM +0100, Jonathan Austin wrote:
> 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:

diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
index 9bcd262..eaedce7 100644
--- a/arch/arm/include/asm/pgtable.h
+++ b/arch/arm/include/asm/pgtable.h
@@ -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.

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

[Vinod Koul]

On Tue, Jun 25, 2013 at 07:45:39PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 25, 2013 at 12:30:08PM +0100, Jonathan Austin wrote:
> > 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:
> 
> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> index 9bcd262..eaedce7 100644
> --- a/arch/arm/include/asm/pgtable.h
> +++ b/arch/arm/include/asm/pgtable.h
> @@ -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.
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

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

[Russell King - ARM Linux]

On Wed, Jun 26, 2013 at 10:18:42AM +0530, Vinod Koul wrote:
> 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

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

[Steve Capper]

On Tue, Jun 25, 2013 at 07:45:39PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 25, 2013 at 12:30:08PM +0100, Jonathan Austin wrote:
> > 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:
> 
> diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> index 9bcd262..eaedce7 100644
> --- a/arch/arm/include/asm/pgtable.h
> +++ b/arch/arm/include/asm/pgtable.h
> @@ -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.
> 

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,
-- 
Steve

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

[Steve Capper]

On Wed, Jun 26, 2013 at 09:40:25AM +0100, Steve Capper wrote:
> On Tue, Jun 25, 2013 at 07:45:39PM +0100, Russell King - ARM Linux wrote:
> > On Tue, Jun 25, 2013 at 12:30:08PM +0100, Jonathan Austin wrote:
> > > 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:
> > 
> > diff --git a/arch/arm/include/asm/pgtable.h b/arch/arm/include/asm/pgtable.h
> > index 9bcd262..eaedce7 100644
> > --- a/arch/arm/include/asm/pgtable.h
> > +++ b/arch/arm/include/asm/pgtable.h
> > @@ -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.
> > 
> 
> 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.
> 

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,
-- 
Steve

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

[Vinod Koul]

On Wed, Jun 26, 2013 at 09:19:09AM +0100, Russell King - ARM Linux wrote:
> On Wed, Jun 26, 2013 at 10:18:42AM +0530, Vinod Koul wrote:
> > 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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

On Wed, Jun 26, 2013 at 04:09:30PM +0530, Vinod Koul wrote:
> On Wed, Jun 26, 2013 at 09:19:09AM +0100, Russell King - ARM Linux wrote:

> > 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?

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Russell King - ARM Linux]

On Wed, Jun 26, 2013 at 04:09:30PM +0530, Vinod Koul wrote:
> On Wed, Jun 26, 2013 at 09:19:09AM +0100, Russell King - ARM Linux wrote:
> > On Wed, Jun 26, 2013 at 10:18:42AM +0530, Vinod Koul wrote:
> > > 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?

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.

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

[Olof Johansson]

Hi,


On Thu, Jun 27, 2013 at 2:46 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Wed, Jun 26, 2013 at 04:09:30PM +0530, Vinod Koul wrote:
>> On Wed, Jun 26, 2013 at 09:19:09AM +0100, Russell King - ARM Linux wrote:
>> > On Wed, Jun 26, 2013 at 10:18:42AM +0530, Vinod Koul wrote:
>> > > 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?
>
> 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.

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

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

[Vinod Koul]

On Wed, Jul 03, 2013 at 11:27:12AM -0700, Olof Johansson wrote:
> 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

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

[Russell King - ARM Linux]

On Fri, Jul 05, 2013 at 11:48:02AM +0530, Vinod Koul wrote:
> On Wed, Jul 03, 2013 at 11:27:12AM -0700, Olof Johansson wrote:
> > 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.

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

[Vinod Koul]

On Sun, Jul 07, 2013 at 01:32:40PM +0100, Russell King - ARM Linux wrote:
> On Fri, Jul 05, 2013 at 11:48:02AM +0530, Vinod Koul wrote:
> > On Wed, Jul 03, 2013 at 11:27:12AM -0700, Olof Johansson wrote:
> > > 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
> 
> 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.
Yes it could have gone thur your tree as well, or you could have merged my tree
to resolve your breakage

--
Thanks
~Vinod

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

[Vinod Koul]

On Sun, Jul 07, 2013 at 07:04:53PM +0530, Vinod Koul wrote:
> On Sun, Jul 07, 2013 at 01:32:40PM +0100, Russell King - ARM Linux wrote:
> > On Fri, Jul 05, 2013 at 11:48:02AM +0530, Vinod Koul wrote:
> > > On Wed, Jul 03, 2013 at 11:27:12AM -0700, Olof Johansson wrote:
> > > > 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
Linus has merged this, so please merge the linus's tree and you should be happy
:)

~Vinod