* Re: [PATCH]gx-suspmod.c use boot_cpu_data instead of current_cpu_data [not found] <20070725141905.GA4567@darkstar.te-china.tietoenator.com> @ 2007-07-25 6:40 ` Andrew Morton 2007-07-26 4:20 ` Dave Young 2007-07-26 7:55 ` Hiroshi Miura 0 siblings, 2 replies; 6+ messages in thread From: Andrew Morton @ 2007-07-25 6:40 UTC (permalink / raw) To: Dave Young; +Cc: linux-kernel, Dave Jones, cpufreq On Wed, 25 Jul 2007 14:19:05 +0000 Dave Young <hidave.darkstar@gmail.com> wrote: > Hi, > in preemptible kernel will report BUG: using smp_processor_id() in preemptible, so use boot_cpu_data instead of current_cpu_data. > > Signed-off-by: Dave Young <hidave.darkstar@gmail.com> > > --- > arch/i386/kernel/cpu/cpufreq/gx-suspmod.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff -pur linux/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c linux.new/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c > --- linux/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c 2007-07-25 14:11:06.000000000 +0000 > +++ linux.new/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c 2007-07-25 13:57:29.000000000 +0000 > @@ -181,8 +181,8 @@ static __init struct pci_dev *gx_detect_ > struct pci_dev *gx_pci = NULL; > > /* check if CPU is a MediaGX or a Geode. */ > - if ((current_cpu_data.x86_vendor != X86_VENDOR_NSC) && > - (current_cpu_data.x86_vendor != X86_VENDOR_CYRIX)) { > + if ((boot_cpu_data.x86_vendor != X86_VENDOR_NSC) && > + (boot_cpu_data.x86_vendor != X86_VENDOR_CYRIX)) { > dprintk("error: no MediaGX/Geode processor found!\n"); > return NULL; > } um, I suspect it really wants to get at the current CPU. But putting a preempt_disable() around just that code is meaningless: the current CPU could change immediately before or after the code block. It needs deeper fixing, methinks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH]gx-suspmod.c use boot_cpu_data instead of current_cpu_data 2007-07-25 6:40 ` [PATCH]gx-suspmod.c use boot_cpu_data instead of current_cpu_data Andrew Morton @ 2007-07-26 4:20 ` Dave Young 2007-07-26 4:26 ` Andrew Morton 2007-07-26 7:55 ` Hiroshi Miura 1 sibling, 1 reply; 6+ messages in thread From: Dave Young @ 2007-07-26 4:20 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, Dave Jones, cpufreq >On 7/25/07, Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 25 Jul 2007 14:19:05 +0000 Dave Young <hidave.darkstar@gmail.com> wrote: > > > Hi, > > in preemptible kernel will report BUG: using smp_processor_id() in preemptible, so use boot_cpu_data instead of current_cpu_data. > > > > Signed-off-by: Dave Young <hidave.darkstar@gmail.com> > > > > --- > > arch/i386/kernel/cpu/cpufreq/gx-suspmod.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff -pur linux/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c linux.new/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c > > --- linux/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c 2007-07-25 14:11:06.000000000 +0000 > > +++ linux.new/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c 2007-07-25 13:57:29.000000000 +0000 > > @@ -181,8 +181,8 @@ static __init struct pci_dev *gx_detect_ > > struct pci_dev *gx_pci = NULL; > > > > /* check if CPU is a MediaGX or a Geode. */ > > - if ((current_cpu_data.x86_vendor != X86_VENDOR_NSC) && > > - (current_cpu_data.x86_vendor != X86_VENDOR_CYRIX)) { > > + if ((boot_cpu_data.x86_vendor != X86_VENDOR_NSC) && > > + (boot_cpu_data.x86_vendor != X86_VENDOR_CYRIX)) { > > dprintk("error: no MediaGX/Geode processor found!\n"); > > return NULL; > > } > > um, I suspect it really wants to get at the current CPU. But putting a > preempt_disable() around just that code is meaningless: the current CPU > could change immediately before or after the code block. It needs deeper > fixing, methinks. The only target is to get the cpu vendor, so boot_cpu_data is enough, the drivers/mtd/nand/cs553x_nand.c has the same usage. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH]gx-suspmod.c use boot_cpu_data instead of current_cpu_data 2007-07-26 4:20 ` Dave Young @ 2007-07-26 4:26 ` Andrew Morton 2007-07-26 16:46 ` Dave Jones 0 siblings, 1 reply; 6+ messages in thread From: Andrew Morton @ 2007-07-26 4:26 UTC (permalink / raw) To: Dave Young; +Cc: linux-kernel, Dave Jones, cpufreq On Thu, 26 Jul 2007 04:20:10 +0000 "Dave Young" <hidave.darkstar@gmail.com> wrote: > >On 7/25/07, Andrew Morton <akpm@linux-foundation.org> wrote: > > On Wed, 25 Jul 2007 14:19:05 +0000 Dave Young <hidave.darkstar@gmail.com> wrote: > > > > > Hi, > > > in preemptible kernel will report BUG: using smp_processor_id() in preemptible, so use boot_cpu_data instead of current_cpu_data. > > > > > > Signed-off-by: Dave Young <hidave.darkstar@gmail.com> > > > > > > --- > > > arch/i386/kernel/cpu/cpufreq/gx-suspmod.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff -pur linux/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c linux.new/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c > > > --- linux/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c 2007-07-25 14:11:06.000000000 +0000 > > > +++ linux.new/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c 2007-07-25 13:57:29.000000000 +0000 > > > @@ -181,8 +181,8 @@ static __init struct pci_dev *gx_detect_ > > > struct pci_dev *gx_pci = NULL; > > > > > > /* check if CPU is a MediaGX or a Geode. */ > > > - if ((current_cpu_data.x86_vendor != X86_VENDOR_NSC) && > > > - (current_cpu_data.x86_vendor != X86_VENDOR_CYRIX)) { > > > + if ((boot_cpu_data.x86_vendor != X86_VENDOR_NSC) && > > > + (boot_cpu_data.x86_vendor != X86_VENDOR_CYRIX)) { > > > dprintk("error: no MediaGX/Geode processor found!\n"); > > > return NULL; > > > } > > > > um, I suspect it really wants to get at the current CPU. But putting a > > preempt_disable() around just that code is meaningless: the current CPU > > could change immediately before or after the code block. It needs deeper > > fixing, methinks. > The only target is to get the cpu vendor, so boot_cpu_data is enough, > the drivers/mtd/nand/cs553x_nand.c has the same usage. I think there's some vague ambition in there to support non-identical CPUs. In which case reading from the local CPU would make more sense. (waves frantically at cpufreq developers). otoh, it'll take some work I suspect. It'll need to sort out the overall scope of "local cpu". At what point and for how long should this code pin itself on a cpu? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH]gx-suspmod.c use boot_cpu_data instead of current_cpu_data 2007-07-26 4:26 ` Andrew Morton @ 2007-07-26 16:46 ` Dave Jones 2007-07-26 17:11 ` Alan Cox 0 siblings, 1 reply; 6+ messages in thread From: Dave Jones @ 2007-07-26 16:46 UTC (permalink / raw) To: Andrew Morton; +Cc: Dave Jones, cpufreq, Dave Young, linux-kernel On Wed, Jul 25, 2007 at 09:26:38PM -0700, Andrew Morton wrote: > On Thu, 26 Jul 2007 04:20:10 +0000 "Dave Young" <hidave.darkstar@gmail.com> wrote: > > > >On 7/25/07, Andrew Morton <akpm@linux-foundation.org> wrote: > > > On Wed, 25 Jul 2007 14:19:05 +0000 Dave Young <hidave.darkstar@gmail.com> wrote: > > > > > > > Hi, > > > > in preemptible kernel will report BUG: using smp_processor_id() in preemptible, so use boot_cpu_data instead of current_cpu_data. > > > > > > > > Signed-off-by: Dave Young <hidave.darkstar@gmail.com> > > > > > > > > --- > > > > arch/i386/kernel/cpu/cpufreq/gx-suspmod.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff -pur linux/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c linux.new/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c > > > > --- linux/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c 2007-07-25 14:11:06.000000000 +0000 > > > > +++ linux.new/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c 2007-07-25 13:57:29.000000000 +0000 > > > > @@ -181,8 +181,8 @@ static __init struct pci_dev *gx_detect_ > > > > struct pci_dev *gx_pci = NULL; > > > > > > > > /* check if CPU is a MediaGX or a Geode. */ > > > > - if ((current_cpu_data.x86_vendor != X86_VENDOR_NSC) && > > > > - (current_cpu_data.x86_vendor != X86_VENDOR_CYRIX)) { > > > > + if ((boot_cpu_data.x86_vendor != X86_VENDOR_NSC) && > > > > + (boot_cpu_data.x86_vendor != X86_VENDOR_CYRIX)) { > > > > dprintk("error: no MediaGX/Geode processor found!\n"); > > > > return NULL; > > > > } > > > > > > um, I suspect it really wants to get at the current CPU. But putting a > > > preempt_disable() around just that code is meaningless: the current CPU > > > could change immediately before or after the code block. It needs deeper > > > fixing, methinks. > > The only target is to get the cpu vendor, so boot_cpu_data is enough, > > the drivers/mtd/nand/cs553x_nand.c has the same usage. > > I think there's some vague ambition in there to support non-identical CPUs. > In which case reading from the local CPU would make more sense. (waves > frantically at cpufreq developers). a non-identical CPU system with a geode + something else doesn't exist and is very unlikely to happen. Dave -- http://www.codemonkey.org.uk ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH]gx-suspmod.c use boot_cpu_data instead of current_cpu_data 2007-07-26 16:46 ` Dave Jones @ 2007-07-26 17:11 ` Alan Cox 0 siblings, 0 replies; 6+ messages in thread From: Alan Cox @ 2007-07-26 17:11 UTC (permalink / raw) To: Dave Jones; +Cc: Andrew Morton, Dave Young, linux-kernel, Dave Jones, cpufreq O> a non-identical CPU system with a geode + something else doesn't exist > and is very unlikely to happen. As the Geode isn't used SMP (and afaik isnt SMP capable) that would be a reasonable assumption ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH]gx-suspmod.c use boot_cpu_data instead of current_cpu_data 2007-07-25 6:40 ` [PATCH]gx-suspmod.c use boot_cpu_data instead of current_cpu_data Andrew Morton 2007-07-26 4:20 ` Dave Young @ 2007-07-26 7:55 ` Hiroshi Miura 1 sibling, 0 replies; 6+ messages in thread From: Hiroshi Miura @ 2007-07-26 7:55 UTC (permalink / raw) To: Andrew Morton; +Cc: Dave Young, Dave Jones, linux-kernel, cpufreq 2007/7/25, Andrew Morton <akpm@linux-foundation.org>: > On Wed, 25 Jul 2007 14:19:05 +0000 Dave Young <hidave.darkstar@gmail.com> wrote: > > > Hi, > > in preemptible kernel will report BUG: using smp_processor_id() in preemptible, so use boot_cpu_data instead of current_cpu_data. > > > > Signed-off-by: Dave Young <hidave.darkstar@gmail.com> > > > > --- > > arch/i386/kernel/cpu/cpufreq/gx-suspmod.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff -pur linux/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c linux.new/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c > > --- linux/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c 2007-07-25 14:11:06.000000000 +0000 > > +++ linux.new/arch/i386/kernel/cpu/cpufreq/gx-suspmod.c 2007-07-25 13:57:29.000000000 +0000 > > @@ -181,8 +181,8 @@ static __init struct pci_dev *gx_detect_ > > struct pci_dev *gx_pci = NULL; > > > > /* check if CPU is a MediaGX or a Geode. */ > > - if ((current_cpu_data.x86_vendor != X86_VENDOR_NSC) && > > - (current_cpu_data.x86_vendor != X86_VENDOR_CYRIX)) { > > + if ((boot_cpu_data.x86_vendor != X86_VENDOR_NSC) && > > + (boot_cpu_data.x86_vendor != X86_VENDOR_CYRIX)) { > > dprintk("error: no MediaGX/Geode processor found!\n"); > > return NULL; > > } > > um, I suspect it really wants to get at the current CPU. But putting a > preempt_disable() around just that code is meaningless: the current CPU > could change immediately before or after the code block. It needs deeper > fixing, methinks. I think we can remove these part. It checks chipset 's pci id (vendor id/device id) after this part as follows; /* detect which companion chip is used */ while ((gx_pci = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, gx_pci)) != NULL) { if ((pci_match_id(gx_chipset_tbl, gx_pci)) != NULL) return gx_pci; } It drives cpu frequency through chipset register, not CPU regsiter. It may be enough to check chipset pci id. Hiroshi -- HIroshi Miura NTT DATA Corp. and IPA OSS center miura@da-cha.org （株）NTTデータ /（独）情報処理推進機構 三浦広志 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-07-26 17:11 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20070725141905.GA4567@darkstar.te-china.tietoenator.com>
2007-07-25 6:40 ` [PATCH]gx-suspmod.c use boot_cpu_data instead of current_cpu_data Andrew Morton
2007-07-26 4:20 ` Dave Young
2007-07-26 4:26 ` Andrew Morton
2007-07-26 16:46 ` Dave Jones
2007-07-26 17:11 ` Alan Cox
2007-07-26 7:55 ` Hiroshi Miura
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox