On Mon, Sep 24, 2007 at 05:53:57PM -0500, [email protected] wrote:
> I was building a kernel for an iPaq {SA1110} and ran into this.
>
> linux-2.6.22.7/arch/arm/mach-sa1100/generic.c:
> Has a: #include <linux/cpufreq.h>
> Then afterwards there is a: #if defined(CONFIG_CPU_FREQ_SA1100) ||
> defined(CONFIG_CPU_FREQ_SA1110)
> who's else section redefines the cpufreq_get function inhereited from
> the header....
>
> I'm guessing no one ever ended up in the "else" section until now, and
> that the header was added some time ago and no one caught this.
> This patch worked for me to get rid of the compile time problems. I'm
> having issues with the kernel, but as far as I can tell they are form
> the Frame buffer and not because of this. If this assessment is correct
> {the not needing this code anymore} then please pass this along so it
> makes it into an upcoming release.
>
> --- linux-2.6.22.7/arch/arm/mach-sa1100/generic.c.orig 2007-09-24
> 17:36:21.000000000 -0500
> +++ linux-2.6.22.7/arch/arm/mach-sa1100/generic.c 2007-09-24
> 17:40:02.000000000 -0500
> @@ -107,15 +107,6 @@ unsigned int sa11x0_getspeed(unsigned in
> return cclk_frequency_100khz[PPCR & 0xf] * 100;
> }
>
> -#else
> -/*
> - * We still need to provide this so building without cpufreq works.
> - */
> -unsigned int cpufreq_get(unsigned int cpu)
> -{
> - return cclk_frequency_100khz[PPCR & 0xf] * 100;
> -}
> -EXPORT_SYMBOL(cpufreq_get);
> #endif
>
> /*
No. That code is required - the StrongARM 1100 framebuffer driver
*needs* to know what the CPU frequency is so it can set the pixel
clock divisor.
The real problem is the silly people who added this to cpufreq.h:
#ifdef CONFIG_CPU_FREQ
unsigned int cpufreq_quick_get(unsigned int cpu);
unsigned int cpufreq_get(unsigned int cpu);
#else
static inline unsigned int cpufreq_quick_get(unsigned int cpu)
{
return 0;
}
static inline unsigned int cpufreq_get(unsigned int cpu)
{
return 0;
}
#endif
which utterly bogus.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
On Tue, Sep 25, 2007 at 08:31:32AM +0100, Russell King wrote:
> On Mon, Sep 24, 2007 at 05:53:57PM -0500, [email protected] wrote:
> > I was building a kernel for an iPaq {SA1110} and ran into this.
> >
> > linux-2.6.22.7/arch/arm/mach-sa1100/generic.c:
> > Has a: #include <linux/cpufreq.h>
> > Then afterwards there is a: #if defined(CONFIG_CPU_FREQ_SA1100) ||
> > defined(CONFIG_CPU_FREQ_SA1110)
> > who's else section redefines the cpufreq_get function inhereited from
> > the header....
> >
> > I'm guessing no one ever ended up in the "else" section until now, and
> > that the header was added some time ago and no one caught this.
> > This patch worked for me to get rid of the compile time problems. I'm
> > having issues with the kernel, but as far as I can tell they are form
> > the Frame buffer and not because of this. If this assessment is correct
> > {the not needing this code anymore} then please pass this along so it
> > makes it into an upcoming release.
> >
> > --- linux-2.6.22.7/arch/arm/mach-sa1100/generic.c.orig 2007-09-24
> > 17:36:21.000000000 -0500
> > +++ linux-2.6.22.7/arch/arm/mach-sa1100/generic.c 2007-09-24
> > 17:40:02.000000000 -0500
> > @@ -107,15 +107,6 @@ unsigned int sa11x0_getspeed(unsigned in
> > return cclk_frequency_100khz[PPCR & 0xf] * 100;
> > }
> >
> > -#else
> > -/*
> > - * We still need to provide this so building without cpufreq works.
> > - */
> > -unsigned int cpufreq_get(unsigned int cpu)
> > -{
> > - return cclk_frequency_100khz[PPCR & 0xf] * 100;
> > -}
> > -EXPORT_SYMBOL(cpufreq_get);
> > #endif
> >
> > /*
>
> No. That code is required - the StrongARM 1100 framebuffer driver
> *needs* to know what the CPU frequency is so it can set the pixel
> clock divisor.
>
> The real problem is the silly people who added this to cpufreq.h:
>
> #ifdef CONFIG_CPU_FREQ
> unsigned int cpufreq_quick_get(unsigned int cpu);
> unsigned int cpufreq_get(unsigned int cpu);
> #else
> static inline unsigned int cpufreq_quick_get(unsigned int cpu)
> {
> return 0;
> }
> static inline unsigned int cpufreq_get(unsigned int cpu)
> {
> return 0;
> }
> #endif
>
> which utterly bogus.
Which came from ...
commit 184c44d2049c4db7ef6ec65794546954da2c6a0e
Author: Andrew Morton <[email protected]>
Date: Wed May 2 19:27:08 2007 +0200
[PATCH] x86-64: fix x86_64-mm-sched-clock-share
Fix for the following patch. Provide dummy cpufreq functions when
CPUFREQ is not compiled in.
Cc: Andi Kleen <[email protected]>
Cc: Dave Jones <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Andi Kleen <[email protected]>
I don't remember seeing any problem here, so I'm not entirely sure what
this was supposed to be fixing. Perhaps the -mm-esque patch name
will provide Andrew/Andi clues. It lacks sufficient information for
my brain to guess what the problem was.
"Fix for the following patch" is also something that really should
never be added to a git changelog too, because 'next' means absolutely
nothing to me, nor I expect 99% of changelog readers.
Cc's added.
Dave
--
http://www.codemonkey.org.uk
On Tue, 25 Sep 2007 10:36:51 -0400 Dave Jones <[email protected]> wrote:
> On Tue, Sep 25, 2007 at 08:31:32AM +0100, Russell King wrote:
> > On Mon, Sep 24, 2007 at 05:53:57PM -0500, [email protected] wrote:
> > > I was building a kernel for an iPaq {SA1110} and ran into this.
> > >
> > > linux-2.6.22.7/arch/arm/mach-sa1100/generic.c:
> > > Has a: #include <linux/cpufreq.h>
> > > Then afterwards there is a: #if defined(CONFIG_CPU_FREQ_SA1100) ||
> > > defined(CONFIG_CPU_FREQ_SA1110)
> > > who's else section redefines the cpufreq_get function inhereited from
> > > the header....
> > >
> > > I'm guessing no one ever ended up in the "else" section until now, and
> > > that the header was added some time ago and no one caught this.
> > > This patch worked for me to get rid of the compile time problems. I'm
> > > having issues with the kernel, but as far as I can tell they are form
> > > the Frame buffer and not because of this. If this assessment is correct
> > > {the not needing this code anymore} then please pass this along so it
> > > makes it into an upcoming release.
> > >
> > > --- linux-2.6.22.7/arch/arm/mach-sa1100/generic.c.orig 2007-09-24
> > > 17:36:21.000000000 -0500
> > > +++ linux-2.6.22.7/arch/arm/mach-sa1100/generic.c 2007-09-24
> > > 17:40:02.000000000 -0500
> > > @@ -107,15 +107,6 @@ unsigned int sa11x0_getspeed(unsigned in
> > > return cclk_frequency_100khz[PPCR & 0xf] * 100;
> > > }
> > >
> > > -#else
> > > -/*
> > > - * We still need to provide this so building without cpufreq works.
> > > - */
> > > -unsigned int cpufreq_get(unsigned int cpu)
> > > -{
> > > - return cclk_frequency_100khz[PPCR & 0xf] * 100;
> > > -}
> > > -EXPORT_SYMBOL(cpufreq_get);
> > > #endif
> > >
> > > /*
> >
> > No. That code is required - the StrongARM 1100 framebuffer driver
> > *needs* to know what the CPU frequency is so it can set the pixel
> > clock divisor.
> >
> > The real problem is the silly people who added this to cpufreq.h:
> >
> > #ifdef CONFIG_CPU_FREQ
> > unsigned int cpufreq_quick_get(unsigned int cpu);
> > unsigned int cpufreq_get(unsigned int cpu);
> > #else
> > static inline unsigned int cpufreq_quick_get(unsigned int cpu)
> > {
> > return 0;
> > }
> > static inline unsigned int cpufreq_get(unsigned int cpu)
> > {
> > return 0;
> > }
> > #endif
> >
> > which utterly bogus.
>
> Which came from ...
>
> commit 184c44d2049c4db7ef6ec65794546954da2c6a0e
> Author: Andrew Morton <[email protected]>
> Date: Wed May 2 19:27:08 2007 +0200
>
> [PATCH] x86-64: fix x86_64-mm-sched-clock-share
>
> Fix for the following patch. Provide dummy cpufreq functions when
> CPUFREQ is not compiled in.
>
> Cc: Andi Kleen <[email protected]>
> Cc: Dave Jones <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> Signed-off-by: Andi Kleen <[email protected]>
>
> I don't remember seeing any problem here, so I'm not entirely sure what
> this was supposed to be fixing. Perhaps the -mm-esque patch name
> will provide Andrew/Andi clues. It lacks sufficient information for
> my brain to guess what the problem was.
Oh geeze. sched-clock-share went through about 18 different versions, was
merged, unmerged, remerged, dropped, etc. I don't recall at what stage in
this mess the above fix was inserted, sorry.
> "Fix for the following patch" is also something that really should
> never be added to a git changelog too, because 'next' means absolutely
> nothing to me, nor I expect 99% of changelog readers.
184c44d2049c4db7ef6ec65794546954da2c6a0e should never have existed,
actually. I intended that Andi fold it into the base patch prior to
sending it to Linus. He normally does that, but it looks like this
one was handled as a standalone commit for some reason.
On Tue, Sep 25, 2007 at 09:52:29AM -0700, Andrew Morton wrote:
> On Tue, 25 Sep 2007 10:36:51 -0400 Dave Jones <[email protected]> wrote:
> >
> > commit 184c44d2049c4db7ef6ec65794546954da2c6a0e
> > Author: Andrew Morton <[email protected]>
> > Date: Wed May 2 19:27:08 2007 +0200
> >
> > [PATCH] x86-64: fix x86_64-mm-sched-clock-share
> >
> > Fix for the following patch. Provide dummy cpufreq functions when
> > CPUFREQ is not compiled in.
> >
> > Cc: Andi Kleen <[email protected]>
> > Cc: Dave Jones <[email protected]>
> > Signed-off-by: Andrew Morton <[email protected]>
> > Signed-off-by: Andi Kleen <[email protected]>
> >
> > I don't remember seeing any problem here, so I'm not entirely sure what
> > this was supposed to be fixing. Perhaps the -mm-esque patch name
> > will provide Andrew/Andi clues. It lacks sufficient information for
> > my brain to guess what the problem was.
>
> Oh geeze. sched-clock-share went through about 18 different versions, was
> merged, unmerged, remerged, dropped, etc. I don't recall at what stage in
> this mess the above fix was inserted, sorry.
>
> > "Fix for the following patch" is also something that really should
> > never be added to a git changelog too, because 'next' means absolutely
> > nothing to me, nor I expect 99% of changelog readers.
>
> 184c44d2049c4db7ef6ec65794546954da2c6a0e should never have existed,
> actually. I intended that Andi fold it into the base patch prior to
> sending it to Linus. He normally does that, but it looks like this
> one was handled as a standalone commit for some reason.
So lets see what happens if we revert it ?
Dave
--
http://www.codemonkey.org.uk
On Tue, 25 Sep 2007 12:58:34 -0400 Dave Jones <[email protected]> wrote:
> On Tue, Sep 25, 2007 at 09:52:29AM -0700, Andrew Morton wrote:
> > On Tue, 25 Sep 2007 10:36:51 -0400 Dave Jones <[email protected]> wrote:
> > >
> > > commit 184c44d2049c4db7ef6ec65794546954da2c6a0e
> > > Author: Andrew Morton <[email protected]>
> > > Date: Wed May 2 19:27:08 2007 +0200
> > >
> > > [PATCH] x86-64: fix x86_64-mm-sched-clock-share
> > >
> > > Fix for the following patch. Provide dummy cpufreq functions when
> > > CPUFREQ is not compiled in.
> > >
> > > Cc: Andi Kleen <[email protected]>
> > > Cc: Dave Jones <[email protected]>
> > > Signed-off-by: Andrew Morton <[email protected]>
> > > Signed-off-by: Andi Kleen <[email protected]>
> > >
> > > I don't remember seeing any problem here, so I'm not entirely sure what
> > > this was supposed to be fixing. Perhaps the -mm-esque patch name
> > > will provide Andrew/Andi clues. It lacks sufficient information for
> > > my brain to guess what the problem was.
> >
> > Oh geeze. sched-clock-share went through about 18 different versions, was
> > merged, unmerged, remerged, dropped, etc. I don't recall at what stage in
> > this mess the above fix was inserted, sorry.
> >
> > > "Fix for the following patch" is also something that really should
> > > never be added to a git changelog too, because 'next' means absolutely
> > > nothing to me, nor I expect 99% of changelog readers.
> >
> > 184c44d2049c4db7ef6ec65794546954da2c6a0e should never have existed,
> > actually. I intended that Andi fold it into the base patch prior to
> > sending it to Linus. He normally does that, but it looks like this
> > one was handled as a standalone commit for some reason.
>
> So lets see what happens if we revert it ?
>
<grep flurry>
OK, here: ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.21-rc5/2.6.21-rc5-mm3/broken-out/fix-x86_64-mm-sched-clock-share.patch
So I guess what we want to do here is to revert that patch, test i386
allnoconfig and then fix up anything which breaks.
On Tue, Sep 25, 2007 at 10:08:39AM -0700, Andrew Morton wrote:
> On Tue, 25 Sep 2007 12:58:34 -0400 Dave Jones <[email protected]> wrote:
>
> > On Tue, Sep 25, 2007 at 09:52:29AM -0700, Andrew Morton wrote:
> > > On Tue, 25 Sep 2007 10:36:51 -0400 Dave Jones <[email protected]> wrote:
> > > >
> > > > commit 184c44d2049c4db7ef6ec65794546954da2c6a0e
> > > > Author: Andrew Morton <[email protected]>
> > > > Date: Wed May 2 19:27:08 2007 +0200
> > > >
> > > > [PATCH] x86-64: fix x86_64-mm-sched-clock-share
> > > >
> > > > Fix for the following patch. Provide dummy cpufreq functions when
> > > > CPUFREQ is not compiled in.
> > > >
> > > > Cc: Andi Kleen <[email protected]>
> > > > Cc: Dave Jones <[email protected]>
> > > > Signed-off-by: Andrew Morton <[email protected]>
> > > > Signed-off-by: Andi Kleen <[email protected]>
> > > >
> > > > I don't remember seeing any problem here, so I'm not entirely sure what
> > > > this was supposed to be fixing. Perhaps the -mm-esque patch name
> > > > will provide Andrew/Andi clues. It lacks sufficient information for
> > > > my brain to guess what the problem was.
> > >
> > > Oh geeze. sched-clock-share went through about 18 different versions, was
> > > merged, unmerged, remerged, dropped, etc. I don't recall at what stage in
> > > this mess the above fix was inserted, sorry.
> > >
> > > > "Fix for the following patch" is also something that really should
> > > > never be added to a git changelog too, because 'next' means absolutely
> > > > nothing to me, nor I expect 99% of changelog readers.
> > >
> > > 184c44d2049c4db7ef6ec65794546954da2c6a0e should never have existed,
> > > actually. I intended that Andi fold it into the base patch prior to
> > > sending it to Linus. He normally does that, but it looks like this
> > > one was handled as a standalone commit for some reason.
> >
> > So lets see what happens if we revert it ?
> >
>
> <grep flurry>
>
> OK, here: ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.21-rc5/2.6.21-rc5-mm3/broken-out/fix-x86_64-mm-sched-clock-share.patch
>
> So I guess what we want to do here is to revert that patch, test i386
> allnoconfig and then fix up anything which breaks.
Nothing breaks for me with make ARCH=i386 bzImage on my x86-64 box
(which should be the same as a native build).
The functions that complain in that patch header don't seem to actually
exist in mainline at all. (`init_sched_clock' and `call_r_s_f')
Did this patch perhaps jump the gun, and these are -mm only ?
Dave
--
http://www.codemonkey.org.uk
On Tue, 25 Sep 2007 13:22:55 -0400 Dave Jones <[email protected]> wrote:
> >
> > OK, here: ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.21-rc5/2.6.21-rc5-mm3/broken-out/fix-x86_64-mm-sched-clock-share.patch
> >
> > So I guess what we want to do here is to revert that patch, test i386
> > allnoconfig and then fix up anything which breaks.
>
> Nothing breaks for me with make ARCH=i386 bzImage on my x86-64 box
> (which should be the same as a native build).
Was that with allnoconfig?
> The functions that complain in that patch header don't seem to actually
> exist in mainline at all. (`init_sched_clock' and `call_r_s_f')
> Did this patch perhaps jump the gun, and these are -mm only ?
Could be that this patch fixed version 17 of sched-clock-share and we ended
up merging verion 56. It was awful.
On Tue, Sep 25, 2007 at 10:31:42AM -0700, Andrew Morton wrote:
> On Tue, 25 Sep 2007 13:22:55 -0400 Dave Jones <[email protected]> wrote:
>
> > >
> > > OK, here: ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.21-rc5/2.6.21-rc5-mm3/broken-out/fix-x86_64-mm-sched-clock-share.patch
> > >
> > > So I guess what we want to do here is to revert that patch, test i386
> > > allnoconfig and then fix up anything which breaks.
> >
> > Nothing breaks for me with make ARCH=i386 bzImage on my x86-64 box
> > (which should be the same as a native build).
>
> Was that with allnoconfig?
yeah.
> > The functions that complain in that patch header don't seem to actually
> > exist in mainline at all. (`init_sched_clock' and `call_r_s_f')
> > Did this patch perhaps jump the gun, and these are -mm only ?
>
> Could be that this patch fixed version 17 of sched-clock-share and we ended
> up merging verion 56. It was awful.
heh.
I think just reverting that change for .23 makes sense. It doesn't
seem that anything breaks by not having it there, and we know it
definitly breaks arm at least.
Dave
--
http://www.codemonkey.org.uk
On Tue, Sep 25, 2007 at 10:36:51AM -0400, Dave Jones wrote:
> On Tue, Sep 25, 2007 at 08:31:32AM +0100, Russell King wrote:
> > On Mon, Sep 24, 2007 at 05:53:57PM -0500, [email protected] wrote:
> > > I was building a kernel for an iPaq {SA1110} and ran into this.
> > >
> > > linux-2.6.22.7/arch/arm/mach-sa1100/generic.c:
> > > Has a: #include <linux/cpufreq.h>
> > > Then afterwards there is a: #if defined(CONFIG_CPU_FREQ_SA1100) ||
> > > defined(CONFIG_CPU_FREQ_SA1110)
> > > who's else section redefines the cpufreq_get function inhereited from
> > > the header....
> > >
> > > I'm guessing no one ever ended up in the "else" section until now, and
> > > that the header was added some time ago and no one caught this.
> > > This patch worked for me to get rid of the compile time problems. I'm
> > > having issues with the kernel, but as far as I can tell they are form
> > > the Frame buffer and not because of this. If this assessment is correct
> > > {the not needing this code anymore} then please pass this along so it
> > > makes it into an upcoming release.
> > >
> > > --- linux-2.6.22.7/arch/arm/mach-sa1100/generic.c.orig 2007-09-24
> > > 17:36:21.000000000 -0500
> > > +++ linux-2.6.22.7/arch/arm/mach-sa1100/generic.c 2007-09-24
> > > 17:40:02.000000000 -0500
> > > @@ -107,15 +107,6 @@ unsigned int sa11x0_getspeed(unsigned in
> > > return cclk_frequency_100khz[PPCR & 0xf] * 100;
> > > }
> > >
> > > -#else
> > > -/*
> > > - * We still need to provide this so building without cpufreq works.
> > > - */
> > > -unsigned int cpufreq_get(unsigned int cpu)
> > > -{
> > > - return cclk_frequency_100khz[PPCR & 0xf] * 100;
> > > -}
> > > -EXPORT_SYMBOL(cpufreq_get);
> > > #endif
> > >
> > > /*
> >
> > No. That code is required - the StrongARM 1100 framebuffer driver
> > *needs* to know what the CPU frequency is so it can set the pixel
> > clock divisor.
> >
> > The real problem is the silly people who added this to cpufreq.h:
> >
> > #ifdef CONFIG_CPU_FREQ
> > unsigned int cpufreq_quick_get(unsigned int cpu);
> > unsigned int cpufreq_get(unsigned int cpu);
> > #else
> > static inline unsigned int cpufreq_quick_get(unsigned int cpu)
> > {
> > return 0;
> > }
> > static inline unsigned int cpufreq_get(unsigned int cpu)
> > {
> > return 0;
> > }
> > #endif
> >
> > which utterly bogus.
>
> Which came from ...
>
> commit 184c44d2049c4db7ef6ec65794546954da2c6a0e
> Author: Andrew Morton <[email protected]>
> Date: Wed May 2 19:27:08 2007 +0200
>
> [PATCH] x86-64: fix x86_64-mm-sched-clock-share
>
> Fix for the following patch. Provide dummy cpufreq functions when
> CPUFREQ is not compiled in.
>
> Cc: Andi Kleen <[email protected]>
> Cc: Dave Jones <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> Signed-off-by: Andi Kleen <[email protected]>
>
Following up on this from yesterday, Linus please revert the above cset.
It doesn't seem to be necessary (it was added to fix a miscompile in
'make allnoconfig' which doesn't seem to be repeatable with it reverted)
and actively breaks the ARM SA1100 framebuffer driver.
(If you'd prefer a patch reverting it, I'll send one, but I'm
hoping that git-revert will just dtrt).
Thanks,
Dave
--
http://www.codemonkey.org.uk
On Wed, Sep 26, 2007 at 04:53:14PM -0400, Dave Jones wrote:
> Following up on this from yesterday, Linus please revert the above cset.
> It doesn't seem to be necessary (it was added to fix a miscompile in
> 'make allnoconfig' which doesn't seem to be repeatable with it reverted)
> and actively breaks the ARM SA1100 framebuffer driver.
>
> (If you'd prefer a patch reverting it, I'll send one, but I'm
> hoping that git-revert will just dtrt).
Dave,
Thanks for checking this out on x86, and getting it reverted.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: