[PATCH] arm: Fix linux-next compile error in arch/arm/common/gic.c

[PATCH] arm: Fix linux-next compile error in arch/arm/common/gic.c

[Joerg Roedel]

With CONFIG_SMP=n the following compile error occurs:

  CC      arch/arm/common/gic.o
arch/arm/common/gic.c: In function 'gic_init_bases':
arch/arm/common/gic.c:679:4: error: implicit declaration of function 'cpu_logical_map' [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors

This patch fixes the problem.

Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/arm/common/gic.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index b2dc2dd..b33f6b0 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -676,7 +676,10 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 		}
 
 		for_each_possible_cpu(cpu) {
-			unsigned long offset = percpu_offset * cpu_logical_map(cpu);
+			unsigned long offset = percpu_offset;
+#ifdef CONFIG_SMP
+			offset *= cpu_logical_map(cpu);
+#endif
 			*per_cpu_ptr(gic->dist_base.percpu_base, cpu) = dist_base + offset;
 			*per_cpu_ptr(gic->cpu_base.percpu_base, cpu) = cpu_base + offset;
 		}
-- 
1.7.5.4

Re: [PATCH] arm: Fix linux-next compile error in arch/arm/common/gic.c

[Yang Bai]

On Tue, Jan 3, 2012 at 12:33 AM, Joerg Roedel <joerg.roedel@amd.com> wrote:
> With CONFIG_SMP=n the following compile error occurs:
>
>  CC      arch/arm/common/gic.o
> arch/arm/common/gic.c: In function 'gic_init_bases':
> arch/arm/common/gic.c:679:4: error: implicit declaration of function 'cpu_logical_map' [-Werror=implicit-function-declaration]
> cc1: some warnings being treated as errors
>
> This patch fixes the problem.
>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Rob Herring <rob.herring@calxeda.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  arch/arm/common/gic.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index b2dc2dd..b33f6b0 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -676,7 +676,10 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>                }
>
>                for_each_possible_cpu(cpu) {
> -                       unsigned long offset = percpu_offset * cpu_logical_map(cpu);
> +                       unsigned long offset = percpu_offset;
> +#ifdef CONFIG_SMP
> +                       offset *= cpu_logical_map(cpu);
> +#endif
>                        *per_cpu_ptr(gic->dist_base.percpu_base, cpu) = dist_base + offset;
>                        *per_cpu_ptr(gic->cpu_base.percpu_base, cpu) = cpu_base + offset;
>                }
> --
> 1.7.5.4
>
>

Is this the right way to fix it? Or shall we do like this:

#ifdef CONFIG_SMP
...
#else
#define cpu_logical_map() 1
#endif

and leave the gic.c code unchanged.

Thanks,
Yang

Re: [PATCH] arm: Fix linux-next compile error in arch/arm/common/gic.c

[Joerg Roedel]

On Tue, Jan 03, 2012 at 08:44:01AM +0800, Yang Bai wrote:
> On Tue, Jan 3, 2012 at 12:33 AM, Joerg Roedel <joerg.roedel@amd.com> wrote:

> >                for_each_possible_cpu(cpu) {
> > -                       unsigned long offset = percpu_offset * cpu_logical_map(cpu);
> > +                       unsigned long offset = percpu_offset;
> > +#ifdef CONFIG_SMP
> > +                       offset *= cpu_logical_map(cpu);
> > +#endif
> >                        *per_cpu_ptr(gic->dist_base.percpu_base, cpu) = dist_base + offset;
> >                        *per_cpu_ptr(gic->cpu_base.percpu_base, cpu) = cpu_base + offset;
> >                }
> > --
> > 1.7.5.4
> >
> >
> 
> Is this the right way to fix it? Or shall we do like this:
> 
> #ifdef CONFIG_SMP
> ...
> #else
> #define cpu_logical_map() 1
> #endif
> 
> and leave the gic.c code unchanged.

Well, I don't care ;) But everywhere else in this file the use of
cpu_logical_map() is #ifdef'ed with CONFIG_SMP. So for consistency my
proposed variant is better, no?


	Joerg

Re: [PATCH] arm: Fix linux-next compile error in arch/arm/common/gic.c

[Marc Zyngier]

[Adding Will to the loop as he's the author of the logical map stuff,
though I'm not sure he'll read that before next week...]

On 03/01/12 00:44, Yang Bai wrote:
> On Tue, Jan 3, 2012 at 12:33 AM, Joerg Roedel <joerg.roedel@amd.com> wrote:
>> With CONFIG_SMP=n the following compile error occurs:
>>
>>  CC      arch/arm/common/gic.o
>> arch/arm/common/gic.c: In function 'gic_init_bases':
>> arch/arm/common/gic.c:679:4: error: implicit declaration of function 'cpu_logical_map' [-Werror=implicit-function-declaration]
>> cc1: some warnings being treated as errors
>>
>> This patch fixes the problem.
>>
>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> Cc: Rob Herring <rob.herring@calxeda.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>> ---
>>  arch/arm/common/gic.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>> index b2dc2dd..b33f6b0 100644
>> --- a/arch/arm/common/gic.c
>> +++ b/arch/arm/common/gic.c
>> @@ -676,7 +676,10 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>>                }
>>
>>                for_each_possible_cpu(cpu) {
>> -                       unsigned long offset = percpu_offset * cpu_logical_map(cpu);
>> +                       unsigned long offset = percpu_offset;
>> +#ifdef CONFIG_SMP
>> +                       offset *= cpu_logical_map(cpu);
>> +#endif
>>                        *per_cpu_ptr(gic->dist_base.percpu_base, cpu) = dist_base + offset;
>>                        *per_cpu_ptr(gic->cpu_base.percpu_base, cpu) = cpu_base + offset;
>>                }
>> --
>> 1.7.5.4
>>
>>
> 
> Is this the right way to fix it? Or shall we do like this:
> 
> #ifdef CONFIG_SMP
> ...
> #else
> #define cpu_logical_map() 1
> #endif
> 
> and leave the gic.c code unchanged.

Well, both patches are wrong. In the UP case (and assuming we're running
on physical CPU 0), offset should be 0.

The second patch would be my favorite approach, except that
cpu_logical_map(x) should return either "x" or 0. And I'm not sure how
to handle the (unlikely?) case of running a UP kernel on a secondary CPU...

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] arm: Fix linux-next compile error in arch/arm/common/gic.c

[Rob Herring]

On 01/03/2012 05:16 AM, Marc Zyngier wrote:
> [Adding Will to the loop as he's the author of the logical map stuff,
> though I'm not sure he'll read that before next week...]
> 
> On 03/01/12 00:44, Yang Bai wrote:
>> On Tue, Jan 3, 2012 at 12:33 AM, Joerg Roedel <joerg.roedel@amd.com> wrote:
>>> With CONFIG_SMP=n the following compile error occurs:
>>>
>>>  CC      arch/arm/common/gic.o
>>> arch/arm/common/gic.c: In function 'gic_init_bases':
>>> arch/arm/common/gic.c:679:4: error: implicit declaration of function 'cpu_logical_map' [-Werror=implicit-function-declaration]
>>> cc1: some warnings being treated as errors
>>>
>>> This patch fixes the problem.
>>>
>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>> Cc: Rob Herring <rob.herring@calxeda.com>
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>>> ---
>>>  arch/arm/common/gic.c |    5 ++++-
>>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>>> index b2dc2dd..b33f6b0 100644
>>> --- a/arch/arm/common/gic.c
>>> +++ b/arch/arm/common/gic.c
>>> @@ -676,7 +676,10 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>>>                }
>>>
>>>                for_each_possible_cpu(cpu) {
>>> -                       unsigned long offset = percpu_offset * cpu_logical_map(cpu);
>>> +                       unsigned long offset = percpu_offset;
>>> +#ifdef CONFIG_SMP
>>> +                       offset *= cpu_logical_map(cpu);
>>> +#endif
>>>                        *per_cpu_ptr(gic->dist_base.percpu_base, cpu) = dist_base + offset;
>>>                        *per_cpu_ptr(gic->cpu_base.percpu_base, cpu) = cpu_base + offset;
>>>                }
>>> --
>>> 1.7.5.4
>>>
>>>
>>
>> Is this the right way to fix it? Or shall we do like this:
>>
>> #ifdef CONFIG_SMP
>> ...
>> #else
>> #define cpu_logical_map() 1
>> #endif
>>
>> and leave the gic.c code unchanged.

IIRC, part of the problem is asm/smp.h is only included by linux/smp.h
for CONFIG_SMP, so this would have to change. Doing that could easily
break other arches.

> 
> Well, both patches are wrong. In the UP case (and assuming we're running
> on physical CPU 0), offset should be 0.
> 
> The second patch would be my favorite approach, except that
> cpu_logical_map(x) should return either "x" or 0. And I'm not sure how
> to handle the (unlikely?) case of running a UP kernel on a secondary CPU...

Wouldn't this be the case with 2 AMP instances of Linux running?

Rob

Re: [PATCH] arm: Fix linux-next compile error in arch/arm/common/gic.c

[Marc Zyngier]

On 03/01/12 13:53, Rob Herring wrote:
> 
> 
> On 01/03/2012 05:16 AM, Marc Zyngier wrote:
>> [Adding Will to the loop as he's the author of the logical map stuff,
>> though I'm not sure he'll read that before next week...]
>>
>> On 03/01/12 00:44, Yang Bai wrote:
>>> On Tue, Jan 3, 2012 at 12:33 AM, Joerg Roedel <joerg.roedel@amd.com> wrote:
>>>> With CONFIG_SMP=n the following compile error occurs:
>>>>
>>>>  CC      arch/arm/common/gic.o
>>>> arch/arm/common/gic.c: In function 'gic_init_bases':
>>>> arch/arm/common/gic.c:679:4: error: implicit declaration of function 'cpu_logical_map' [-Werror=implicit-function-declaration]
>>>> cc1: some warnings being treated as errors
>>>>
>>>> This patch fixes the problem.
>>>>
>>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
>>>> Cc: Rob Herring <rob.herring@calxeda.com>
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
>>>> ---
>>>>  arch/arm/common/gic.c |    5 ++++-
>>>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
>>>> index b2dc2dd..b33f6b0 100644
>>>> --- a/arch/arm/common/gic.c
>>>> +++ b/arch/arm/common/gic.c
>>>> @@ -676,7 +676,10 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>>>>                }
>>>>
>>>>                for_each_possible_cpu(cpu) {
>>>> -                       unsigned long offset = percpu_offset * cpu_logical_map(cpu);
>>>> +                       unsigned long offset = percpu_offset;
>>>> +#ifdef CONFIG_SMP
>>>> +                       offset *= cpu_logical_map(cpu);
>>>> +#endif
>>>>                        *per_cpu_ptr(gic->dist_base.percpu_base, cpu) = dist_base + offset;
>>>>                        *per_cpu_ptr(gic->cpu_base.percpu_base, cpu) = cpu_base + offset;
>>>>                }
>>>> --
>>>> 1.7.5.4
>>>>
>>>>
>>>
>>> Is this the right way to fix it? Or shall we do like this:
>>>
>>> #ifdef CONFIG_SMP
>>> ...
>>> #else
>>> #define cpu_logical_map() 1
>>> #endif
>>>
>>> and leave the gic.c code unchanged.
> 
> IIRC, part of the problem is asm/smp.h is only included by linux/smp.h
> for CONFIG_SMP, so this would have to change. Doing that could easily
> break other arches.

Ah... good point.

>>
>> Well, both patches are wrong. In the UP case (and assuming we're running
>> on physical CPU 0), offset should be 0.
>>
>> The second patch would be my favorite approach, except that
>> cpu_logical_map(x) should return either "x" or 0. And I'm not sure how
>> to handle the (unlikely?) case of running a UP kernel on a secondary CPU...
> 
> Wouldn't this be the case with 2 AMP instances of Linux running?

Yes, or kexec-ing a UP kernel on a secondary CPU. It really looks like
we ought to make this cpu_logical_map() independent from CONFIG_SMP.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] arm: Fix linux-next compile error in arch/arm/common/gic.c

[Will Deacon]

On Tue, Jan 03, 2012 at 02:05:40PM +0000, Marc Zyngier wrote:
> On 03/01/12 13:53, Rob Herring wrote:
> >>> On Tue, Jan 3, 2012 at 12:33 AM, Joerg Roedel <joerg.roedel@amd.com> wrote:
> >>>> With CONFIG_SMP=n the following compile error occurs:
> >>>>
> >>>>  CC      arch/arm/common/gic.o
> >>>> arch/arm/common/gic.c: In function 'gic_init_bases':
> >>>> arch/arm/common/gic.c:679:4: error: implicit declaration of function 'cpu_logical_map' [-Werror=implicit-function-declaration]
> >>>> cc1: some warnings being treated as errors

[...]

> > IIRC, part of the problem is asm/smp.h is only included by linux/smp.h
> > for CONFIG_SMP, so this would have to change. Doing that could easily
> > break other arches.
> 
> Ah... good point.

We could probably move smp_setup_processor_id out of smp.c if we need to as
it seems as though you can provide a definition of that function even when
!CONFIG_SMP.

> >>
> >> Well, both patches are wrong. In the UP case (and assuming we're running
> >> on physical CPU 0), offset should be 0.
> >>
> >> The second patch would be my favorite approach, except that
> >> cpu_logical_map(x) should return either "x" or 0. And I'm not sure how
> >> to handle the (unlikely?) case of running a UP kernel on a secondary CPU...
> > 
> > Wouldn't this be the case with 2 AMP instances of Linux running?

In this configuration, I think returning 0 would probably be the right thing
to do. If it's anything else, there's the implication that both of your
kernels are hanging off the same GIC distributor / SCU / L2 Cache etc. and
so you would need a single SMP kernel (or some funky message passing that we
don't yet have).

The bit we're currently missing is the notion of a cluster ID, but both CPU IDs
should be 0 for AMP.

> Yes, or kexec-ing a UP kernel on a secondary CPU. It really looks like
> we ought to make this cpu_logical_map() independent from CONFIG_SMP.

We could always prevent this from happening by hacking kexec / kexec-tools
but I think that booting a UP kernel on a CPU other than 0 is rather silly :)

So I'd be inclined to simply move the existing smp_setup_processor_id
implementation (and the map declaration) into somewhere like kernel/setup.c
and I think things should work.

Will