Re: [PATCH] Replace nvidia timer override quirk with pci id list

[Prakash Punnoor]

On the day of Friday 08 February 2008 Andi Kleen hast written:
> On Fri, Feb 08, 2008 at 06:39:17PM +0100, Prakash Punnoor wrote:
> > On the day of Friday 08 February 2008 Andi Kleen hast written:
> > > On Fri, Feb 08, 2008 at 04:13:35PM +0100, Prakash Punnoor wrote:
> > > > Sorry, I meant the opposite. I needed the acpi_skip_timer_override
> > > > kernel parameter for nforce2, thus no override. So this chipset is
> > > > missing here. At least I remember that my nforce2 needed the
> > > > skipping,
> > >
> > > I hope you remember correctly and mean it this time. It would be better
> > > if you could double check.
> >
> > Yes, confirmed. timer w/o the skipping stays XT-PIC on nforce2.
>
> Confirmed what? Did you test my patch on both machines?
> What was the result?

I confirmed that it (nforce2) needed the acpi_skip_timer_override. If you read 
my mail, you knew I didn't test your patch.

> > lspci -n:
>
> Please always send lspci without -n too; I hate looking up hex codes
> by hand when a computer can do that for me.
>
> > 02:00.0 0300: 10de:0281 (rev a1)
> >
> > > I'm a little sceptical because we had this patch in OpenSUSE 10.3
> > > and I didn't think there were complaints from NF2 users.
> > > With the changes you're requesting it turns from something
> > > very well tested into something experimental.
> >
> > Well, even w/o the skipping my nforce2 system wasn't unstable, AFAIK. So
> > I don't think just because of the XT-PIC entry people would complain.
>
> Timer override only does something in APIC mode and when you see XT-PIC
> in /proc/interrupts then you're not in APIC mode. All these patches
> are a no-op in this state.

Perhaps I wasn't percise, Len Brown had it in his earlier patch descriptions:

"
workaround for nForce2 BIOS bug: XT-PIC timer in IOAPIC mode 
"acpi_skip_timer_override" boot parameter
"

or

"
Since the hardware is connected to APIC pin0, it is a BIOS bug 
 that an ACPI interrupt source override from pin2 to IRQ0 exists. 
 
With this simple 2.6.5 patch you can specify "acpi_skip_timer_override" 
 to ignore that bogus BIOS directive. The result is with your 
 ACPI-enabled APIC-enabled kernel, you'll get IRQ0 IO-APIC-edge timer. 
"

This is exactly what I observed on the nforce2.

My kernels are compiled and configured for APIC. With broken BIOSes the timer 
ends up as XT-PIC anyway. That is what I wanted to say and which you could 
see from my cat /proc/interrupts dumps.




Why can't the kernel check for this condition and only activate the quirk then 
instead of current and your proposed broken behaviour?




> > See why I don't want the quirk to be applied more than needed? *NOT*
> > applying the quirk on nforce2 didn't cause any obvious side effects.
> > APPLYING to mcp51 causes hard lock-ups.
>
> Can you please just test the patches instead of speculating what they
> might do or not do?

No, I do understand C code and I know the ID of my board. So I am not 
speculating, just saving myself time and hassle.

You are not even taking the time to really read what I say. I am not your   
guinea pig. Why should I simply waste my time? Esp. my nforce2 system is a 
productive system and I usually don't mess with it. So come up with a patch 
that makes sense (and triggers on my nforce2 and does not trigger on my 
mcp51) in my eyes, or I won't test anything and keep the NAK.

I don't think you did your research correctly coming up with the first version 
of the patch, as it ignored the nforce2 altogether. And the original version 
was made for nforce2 exclusively! So why should I trust that you know what 
you are doing? I don't get the impression. You also didn't gave references 
where you get your IDs in the patch. I at least tried to gave some references 
that putting in those IDs is *wrong*. If you can provide those references 
(and not some "search for it") you could strengthen your point. Provide some 
bug reports or lkml posts where users of nforce4, mcp51 needed the 
skip_override to get their system stable.




I don't care whether this patch has been in some kernel for some time. It is 
still wrong (worse for nforce2 users, though better for newer nvidia chipset 
users as at last the quirk doesn't get appield for them)!





BTW, nforce2 lspci:
00:00.0 Host bridge: nVidia Corporation nForce2 AGP (different version?) (rev 
c1)
00:00.1 RAM memory: nVidia Corporation nForce2 Memory Controller 1 (rev c1)
00:00.2 RAM memory: nVidia Corporation nForce2 Memory Controller 4 (rev c1)
00:00.3 RAM memory: nVidia Corporation nForce2 Memory Controller 3 (rev c1)
00:00.4 RAM memory: nVidia Corporation nForce2 Memory Controller 2 (rev c1)
00:00.5 RAM memory: nVidia Corporation nForce2 Memory Controller 5 (rev c1)
00:01.0 ISA bridge: nVidia Corporation nForce2 ISA Bridge (rev a3)
00:01.1 SMBus: nVidia Corporation nForce2 SMBus (MCP) (rev a2)
00:02.0 USB Controller: nVidia Corporation nForce2 USB Controller (rev a3)
00:02.1 USB Controller: nVidia Corporation nForce2 USB Controller (rev a3)
00:02.2 USB Controller: nVidia Corporation nForce2 USB Controller (rev a3)
00:04.0 Ethernet controller: nVidia Corporation nForce2 Ethernet Controller 
(rev a1)
00:05.0 Multimedia audio controller: nVidia Corporation nForce Audio 
Processing Unit (rev a2)
00:06.0 Multimedia audio controller: nVidia Corporation nForce2 AC97 Audio 
Controler (MCP) (rev a1)
00:08.0 PCI bridge: nVidia Corporation nForce2 External PCI Bridge (rev a3)
00:09.0 IDE interface: nVidia Corporation nForce2 IDE (rev a2)
00:0d.0 FireWire (IEEE 1394): nVidia Corporation nForce2 FireWire (IEEE 1394) 
Controller (rev a3)
00:1e.0 PCI bridge: nVidia Corporation nForce2 AGP (rev c1)
01:08.0 Network controller: Techsan Electronics Co Ltd B2C2 FlexCopII DVB 
chip / Technisat SkyStar2 DVB card (rev 01)
02:00.0 VGA compatible controller: nVidia Corporation NV28 [GeForce4 Ti 4200 
AGP 8x] (rev a1)


lspci -n
00:00.0 0600: 10de:01e0 (rev c1)
00:00.1 0500: 10de:01eb (rev c1)
00:00.2 0500: 10de:01ee (rev c1)
00:00.3 0500: 10de:01ed (rev c1)
00:00.4 0500: 10de:01ec (rev c1)
00:00.5 0500: 10de:01ef (rev c1)
00:01.0 0601: 10de:0060 (rev a3)
00:01.1 0c05: 10de:0064 (rev a2)
00:02.0 0c03: 10de:0067 (rev a3)
00:02.1 0c03: 10de:0067 (rev a3)
00:02.2 0c03: 10de:0068 (rev a3)
00:04.0 0200: 10de:0066 (rev a1)
00:05.0 0401: 10de:006b (rev a2)
00:06.0 0401: 10de:006a (rev a1)
00:08.0 0604: 10de:006c (rev a3)
00:09.0 0101: 10de:0065 (rev a2)
00:0d.0 0c00: 10de:006e (rev a3)
00:1e.0 0604: 10de:01e8 (rev c1)
01:08.0 0280: 13d0:2103 (rev 01)
02:00.0 0300: 10de:0281 (rev a1)


bye,
-- 
(°=                 =°)
//\ Prakash Punnoor /\\
V_/                 \_V

Attachment: signature.asc
Description: This is a digitally signed message part.