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

login
register
mail settings
Submitter Jonathan Austin
Date June 25, 2013, 11:30 a.m.
Message ID <51C97F40.5030101@arm.com>
Download mbox | patch
Permalink /patch/254120/
State New
Headers show

Comments

Jonathan Austin - June 25, 2013, 11:30 a.m.
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<-----------
Linus Walleij - June 25, 2013, 1:49 p.m.
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
Koul, Vinod - June 26, 2013, 4:48 a.m.
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
Russell King - ARM Linux - June 26, 2013, 8:19 a.m.
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
Steve Capper - June 26, 2013, 8:40 a.m.
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 Capper - June 26, 2013, 9:13 a.m.
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,
Koul, Vinod - June 26, 2013, 10:39 a.m.
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
Mark Brown - June 26, 2013, noon
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.
Russell King - ARM Linux - June 27, 2013, 9:46 a.m.
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.
Olof Johansson - July 3, 2013, 6:27 p.m.
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
Koul, Vinod - July 5, 2013, 6:18 a.m.
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
Russell King - ARM Linux - July 7, 2013, 12:32 p.m.
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.
Koul, Vinod - July 7, 2013, 1:34 p.m.
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
Koul, Vinod - July 8, 2013, 2:04 a.m.
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

Patch

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;