VIA IRQ quirk fixup only in XT_PIC mode Take 2

PATCH In review Sergio Monteiro Basto 3 participants 11 messages Jul 28, 2006 – Sep 5, 2006 PCI

lore

Share & tools

lore

Sergio Monteiro Basto

author

2006-07-28 13:01 UTC #0 lore raw

PATCH 1/1 VIA IRQ quirk fixup only in XT_PIC mode Take 2

Hi, this patch (now for 2.6.18-rc2) is more readable.

Thanks,
--
Sérgio M. B.

Cc Alan Cox <[email protected]>

Cc "Scott J. Harmon" <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Chris Wedgwood <[email protected]>

Cc Greg KH <[email protected]>

Signed-off-by Sergio Monteiro Basto <[email protected]>

---
 quirks.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/pci/quirks.c renamed from drivers/pci/quirks.c.orig +7 -7

@@ -648,11 +648,17 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_V
  *
  * Some of the on-chip devices are actually '586 devices' so they are
  * listed here.
+ * 
+ * if flags say that we have working apic(s), we don't need to quirk these
+ * devices
  */
 static void quirk_via_irq(struct pci_dev *dev)
 {
 	u8 irq, new_irq;
 
+	if ((smp_found_config && !skip_ioapic_setup && nr_ioapics) || cpu_has_apic)
+		return;
+
 	new_irq = dev->irq & 0xf;
 	pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
 	if (new_irq != irq) {

@@ -662,13 +668,7 @@ static void quirk_via_irq(struct pci_dev
 		pci_write_config_byte(dev, PCI_INTERRUPT_LINE, new_irq);
 	}
 }
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_0, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_1, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_2, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_3, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C686, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C686_4, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C686_5, quirk_via_irq);
+DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_ANY_ID, quirk_via_irq);
 
 /*
  * VIA VT82C598 has its device ID settable and many BIOSes

Andrew Morton

2006-07-28 19:24 UTC #1 lore raw

Quoted 1 line · On Fri, 28 Jul 2006 14:01:01 +0100 Sergio Monteiro Basto…

Hi, this patch (now for 2.6.18-rc2) is more readable.

It has no changelog, and this sort of patch does need a lenghty one, please.

What relationship does it have to ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc2/2.6.18-rc2-mm1/broken-out/pci-quirk_via_irq-behaviour-change.patch?
If it is better, why?

ok,
Begging by answer the question, Daniel Drake, clearly, say:
"Nobody really has a full understanding of the problem and the issues ...".
As I can understand, make a sort of blacklist with IDs of the problematic VIAs, which probably will always miss some IDs, but the main problem is in lasts lines of this email. The way how is design the list, could be one excellent idea, inclusive for other situations on VIAs, I don't put this in question.

After reviewing what I had write about this, the changelog:
(please correct me the English and cut some parts)
Well, a good new, I already have my old laptop again which have the most problematic VIA (PCI_DEVICE_ID_VIA_82C686). At the time I bought it, I had problematic stuffs when kernel was trying enable APIC, on this laptop, was one Local APIC, later we had the boot option nolapic to workaround this. So the VIA works very well without lapic enabled. Someone give to this problem a name of "the APIC victims", happened on VIAs, Nvidias and probably on others boards.
But this laptop need the quirks; nobody have put this in question, VIA_82C686 has been always on list of the quirks. And btw, is important, _need_ ACPI, with acpi=off, 1/2 of the computer: power manager and hot-key, don't work at all.
A few of the story about my laptop http://sergiomb.no-ip.org/laptop/

So, a few month ago, I bought, what I bought, one VIA of course :) with this IDs: PCI_DEVICE_ID_VIA_82C586_1 and _2.
With this new VIA, I have exactly, the opposite problem, I found, on this VIA with IO-APIC working well, that quirks aren't good/needed . After, I found this interesting email http://lkml.org/lkml/2005/8/13/30
by Karsten Wiese, after, Alan Cox writes this
http://lkml.org/lkml/2005/8/16/160 (on same thread) and Karsten Wiese end ups with the solution on http://lkml.org/lkml/2005/8/18/92, which I want try to implement and that's it, come this idea, that may the full understanding the problem.
Conclusion, I have 2 VIAs with almost same IDs (others reporters have with exactly the same IDs) and in ones I need the quirks and in others don't, why ? one don't have apics enabled, the other have it !?!

Thanks,

Quoted 10 lines · Andrew Morton

Quoted 1 line · On Fri, 28 Jul 2006 14:01:01 +0100 Sergio Monteiro Basto…
Hi, this patch (now for 2.6.18-rc2) is more readable.
It has no changelog, and this sort of patch does need a lenghty one, please.
What relationship does it have to ftp://ftp.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.18-rc2/2.6.18-rc2-mm1/broken-out/pci-quirk_via_irq-behaviour-change.patch?
If it is better, why?

Signature · 1 line

Sérgio M.B.

Daniel Drake

2006-08-12 22:36 UTC #3 lore raw

In reply to Sergio Monteiro Basto

Quoted 1 line · Sergio Monteiro Basto

Hi, this patch (now for 2.6.18-rc2) is more readable.

Ok, it looks like I was wrong in my earlier mails, ACPI vs APIC confusion. In the bug reports I mentioned, APIC was not being used in any circumstances (however it is still a bit strange how these systems do not need quirks when acpi=off).

I'm reasonably certain that this patch will apply the quirks on the affected systems again, so I'm happy for it to be applied, people will be able to use their hardware again. However I'm not sure how good a solution it is, because in some circumstances it will apply the quirks to VIA PCI cards on non-VIA boards, which was the reason we messed with this code in the first place. We could possibly merge it with the southbridge detection hack, but it gets a bit silly at that point...

Jeff/Chris: any thoughts?

Quoted 49 lines

Cc Alan Cox <[email protected]>

Cc "Scott J. Harmon" <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Chris Wedgwood <[email protected]>

Cc Greg KH <[email protected]>

Signed-off-by Sergio Monteiro Basto <[email protected]>

---
 quirks.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/pci/quirks.c renamed from drivers/pci/quirks.c.orig +7 -7

@@ -648,11 +648,17 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_V
  *
  * Some of the on-chip devices are actually '586 devices' so they are
  * listed here.
+ * 
+ * if flags say that we have working apic(s), we don't need to quirk these
+ * devices
  */
 static void quirk_via_irq(struct pci_dev *dev)
 {
 	u8 irq, new_irq;
 
+	if ((smp_found_config && !skip_ioapic_setup && nr_ioapics) || cpu_has_apic)
+		return;
+
 	new_irq = dev->irq & 0xf;
 	pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
 	if (new_irq != irq) {

@@ -662,13 +668,7 @@ static void quirk_via_irq(struct pci_dev
 		pci_write_config_byte(dev, PCI_INTERRUPT_LINE, new_irq);
 	}
 }
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_0, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_1, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_2, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_3, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C686, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C686_4, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C686_5, quirk_via_irq);
+DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_ANY_ID, quirk_via_irq);
 
 /*
  * VIA VT82C598 has its device ID settable and many BIOSes

Sergio Monteiro Basto

author

2006-08-30 02:57 UTC #4 lore raw

Hi, Sorry for so late answer , I have been in holidays and busy.

I remember check my emails that I send to Len Brown about this subject. And I found, what I want, is just revert one patch of Bjorn Helgaas :) between kernel 2.6.12-rc5 and 6.13.

Check this out
http://sourceforge.net/mailarchive/message.php?msg_id=11858102

so I propose this patch like this :

static void quirk_via_irq(struct pci_dev *dev)
{
u8 irq, new_irq;

+#ifdef CONFIG_X86_IO_APIC
+ if (nr_ioapics && !skip_ioapic_setup)
+ return;
+#endif
+#ifdef CONFIG_ACPI
+ if (acpi_irq_model != ACPI_IRQ_MODEL_PIC)
+ return;
+#endif
new_irq = dev->irq & 0xf; pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq); if (new_irq != irq) {

Quoted 7 lines · Daniel Drake

Quoted 1 line · Sergio Monteiro Basto
Hi, this patch (now for 2.6.18-rc2) is more readable.
Ok, it looks like I was wrong in my earlier mails, ACPI vs APIC confusion. In the bug reports I mentioned, APIC was not being used in any circumstances (however it is still a bit strange how these systems do not need quirks when acpi=off).

I still convinced that we need patch even with acpi=off , the problem doesn't appear only on boot time.

Quoted 61 lines

I'm reasonably certain that this patch will apply the quirks on the affected systems again, so I'm happy for it to be applied, people will be able to use their hardware again. However I'm not sure how good a solution it is, because in some circumstances it will apply the quirks to VIA PCI cards on non-VIA boards, which was the reason we messed with this code in the first place. We could possibly merge it with the southbridge detection hack, but it gets a bit silly at that point...
Jeff/Chris: any thoughts?
Quoted 49 lines
Cc Alan Cox <[email protected]>
Cc "Scott J. Harmon" <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Chris Wedgwood <[email protected]>
Cc Greg KH <[email protected]>
Signed-off-by Sergio Monteiro Basto <[email protected]>
---
 quirks.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/pci/quirks.c renamed from drivers/pci/quirks.c.orig +7 -7
@@ -648,11 +648,17 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_V
  *
  * Some of the on-chip devices are actually '586 devices' so they are
  * listed here.
+ * 
+ * if flags say that we have working apic(s), we don't need to quirk these
+ * devices
  */
 static void quirk_via_irq(struct pci_dev *dev)
 {
 	u8 irq, new_irq;
 
+	if ((smp_found_config && !skip_ioapic_setup && nr_ioapics) || cpu_has_apic)
+		return;
+
 	new_irq = dev->irq & 0xf;
 	pci_read_config_byte(dev, PCI_INTERRUPT_LINE, &irq);
 	if (new_irq != irq) {
@@ -662,13 +668,7 @@ static void quirk_via_irq(struct pci_dev
 		pci_write_config_byte(dev, PCI_INTERRUPT_LINE, new_irq);
 	}
 }
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_0, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_1, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_2, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C586_3, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C686, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C686_4, quirk_via_irq);
-DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C686_5, quirk_via_irq);
+DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_VIA, PCI_ANY_ID, quirk_via_irq);
 
 /*
  * VIA VT82C598 has its device ID settable and many BIOSes

Daniel Drake

2006-08-30 03:47 UTC #5 lore raw

Quoted 3 lines · Sergio Monteiro Basto

I remember check my emails that I send to Len Brown about this subject. And I found, what I want, is just revert one patch of Bjorn Helgaas :) between kernel 2.6.12-rc5 and 6.13.

It does look like this patch was under discussion of being reverted before. See http://lkml.org/lkml/2005/9/26/183

The following comment still stands when we just revert Bjorn's change:

Quoted 1 line

Quoted 1 line
I'm reasonably certain that this patch will apply the quirks on the affected systems again, so I'm happy for it to be applied, people will be able to use their hardware again. However I'm not sure how good a solution it is, because in some circumstances it will apply the quirks to VIA PCI cards on non-VIA boards, which was the reason we messed with this code in the first place. We could possibly merge it with the southbridge detection hack, but it gets a bit silly at that point...

So perhaps the best solution is a combination of reverting Bjorn's patch, adding Linus' suggested change, and adding my southbridge hack.

Daniel

Sergio Monteiro Basto

author

2006-08-30 11:25 UTC #6 lore raw

Quoted 7 lines · Daniel Drake

Quoted 3 lines · Sergio Monteiro Basto
I remember check my emails that I send to Len Brown about this subject. And I found, what I want, is just revert one patch of Bjorn Helgaas :) between kernel 2.6.12-rc5 and 6.13.
It does look like this patch was under discussion of being reverted before. See http://lkml.org/lkml/2005/9/26/183

To be honest, I prefer put again this :

+#ifdef CONFIG_X86_IO_APIC
+ if (nr_ioapics && !skip_ioapic_setup)
+ return;
+#endif
+#ifdef CONFIG_ACPI
+ if (acpi_irq_model != ACPI_IRQ_MODEL_PIC)
+ return;
+#endif

about Linus suggestion :
- new_irq = dev->irq & 0xf;
+ new_irq = dev->irq;
+ if (!new_irq || new_irq >= 15)
+ return;

no, we have problem with VIA SATA controllers which have irq lower than 15

Thanks,

Quoted 15 lines

The following comment still stands when we just revert Bjorn's change:
Quoted 7 lines
Quoted 7 lines
I'm reasonably certain that this patch will apply the quirks on the affected systems again, so I'm happy for it to be applied, people will be able to use their hardware again. However I'm not sure how good a solution it is, because in some circumstances it will apply the quirks to VIA PCI cards on non-VIA boards, which was the reason we messed with this code in the first place. We could possibly merge it with the southbridge detection hack, but it gets a bit silly at that point...
So perhaps the best solution is a combination of reverting Bjorn's patch, adding Linus' suggested change, and adding my southbridge hack.
Daniel

Daniel Drake

2006-08-31 03:46 UTC #7 lore raw

Quoted 4 lines · Sergio Monteiro Basto

Quoted 1 line
It does look like this patch was under discussion of being reverted before. See http://lkml.org/lkml/2005/9/26/183
To be honest, I prefer put again this :

Quoted 8 lines

+#ifdef CONFIG_X86_IO_APIC
+ if (nr_ioapics && !skip_ioapic_setup)
+ return;
+#endif
+#ifdef CONFIG_ACPI
+ if (acpi_irq_model != ACPI_IRQ_MODEL_PIC)
+ return;
+#endif

Isn't this exactly the same as what was being suggested?

Quoted 8 lines

about Linus suggestion :
(quoted diff) +3 -1
-	new_irq = dev->irq & 0xf;
+	new_irq = dev->irq;
+	if (!new_irq || new_irq >= 15)
+		return;
no, we have problem with VIA SATA controllers which have irq lower than 15

Any chance you can provide a link to this example so that we can document the decision in the commit message?

Thanks,
Daniel

Sergio Monteiro Basto

author

2006-08-31 11:33 UTC #8 lore raw

Quoted 11 lines · Daniel Drake

Quoted 8 lines
about Linus suggestion :
(quoted diff) +3 -1
-	new_irq = dev->irq & 0xf;
+	new_irq = dev->irq;
+	if (!new_irq || new_irq >= 15)
+		return;
no, we have problem with VIA SATA controllers which have irq lower than 15
Any chance you can provide a link to this example so that we can document the decision in the commit message?

http://lkml.org/lkml/2006/7/30/59

Thanks,
--
Sérgio M. B.

Daniel Drake

2006-09-05 13:38 UTC #9 lore raw

Quoted 8 lines · Sergio Monteiro Basto

Quoted 6 lines · Daniel Drake
Quoted 6 lines
about Linus suggestion :
(quoted diff) +3 -1
-	new_irq = dev->irq & 0xf;
+	new_irq = dev->irq;
+	if (!new_irq || new_irq >= 15)
+		return;

Quoted 5 lines

Quoted 3 lines
Quoted 2 lines
no, we have problem with VIA SATA controllers which have irq lower than 15
Any chance you can provide a link to this example so that we can document the decision in the commit message?
http://lkml.org/lkml/2006/7/30/59

I'm confused. Heikki's report is about a sata_sil controller and he didn't include any dmesg output so I don't know how you can conclude that quirking an IRQ to something less than 15 was the fix...

Also note that the fix was *not* quirking the device at all (your patch ensured that the quirks didn't run because IO-APIC was enabled), this hardly seems like an accurate way of arguing that quirks that change the IRQ to something less than 15 are *required*...

Daniel

Sergio Monteiro Basto

author

2006-09-05 14:49 UTC #10 lore raw

Quoted 22 lines · Daniel Drake

Quoted 9 lines · Sergio Monteiro Basto
Quoted 7 lines · Daniel Drake
Quoted 7 lines
about Linus suggestion :
(quoted diff) +3 -1
-   new_irq = dev->irq & 0xf;
+   new_irq = dev->irq;
+   if (!new_irq || new_irq >= 15)
+           return;
no, we have problem with VIA SATA controllers which have irq lower
than
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
15
Any chance you can provide a link to this example so that we can document the decision in the commit message?
http://lkml.org/lkml/2006/7/30/59
I'm confused. Heikki's report is about a sata_sil controller and he didn't include any dmesg output so I don't know how you can conclude that quirking an IRQ to something less than 15 was the fix...

Quoted 6 lines

Also note that the fix was *not* quirking the device at all (your patch
ensured that the quirks didn't run because IO-APIC was enabled), this hardly seems like an accurate way of arguing that quirks that change the
IRQ to something less than 15 are *required*...

hum hum, "Vladimir B. Savkin" report on Asus A8V (kernel 2.6.14+) and Heikki Orsila talks about is ASUS K8V SE Deluxe on amd64 with SiI 3114 SATA controller.
yes, when Heikki Orsila reports a positive test, I thought was talking about "Vladimir B. Savkin" reports.
Is petty that "Vladimir B. Savkin" don't response.
and Heikki Orsila don't show his dmesg :)

thanks,
--
Sérgio M.