2006-08-12 22:36:59

by Daniel Drake

[permalink] [raw]
Subject: Re: [PATCH] VIA IRQ quirk fixup only in XT_PIC mode Take 2

Sergio Monteiro Basto wrote:
> 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?

> 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(-)
>
> --- linux-2.6.17.i686/drivers/pci/quirks.c.orig 2006-07-28 12:59:04.000000000 +0100
> +++ linux-2.6.17.i686/drivers/pci/quirks.c 2006-07-28 13:26:49.000000000 +0100
> @@ -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
>
>
>
>


2006-08-30 02:57:33

by Sergio Monteiro Basto

[permalink] [raw]
Subject: Re: [PATCH] VIA IRQ quirk fixup only in XT_PIC mode Take 2

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) {

On Sat, 2006-08-12 at 23:47 +0100, Daniel Drake wrote:
> Sergio Monteiro Basto wrote:
> > 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.


>
> 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?
>
> > 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(-)
> >
> > --- linux-2.6.17.i686/drivers/pci/quirks.c.orig 2006-07-28 12:59:04.000000000 +0100
> > +++ linux-2.6.17.i686/drivers/pci/quirks.c 2006-07-28 13:26:49.000000000 +0100
> > @@ -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
> >
> >
> >
> >
>


Attachments:
smime.p7s (2.12 kB)

2006-08-30 03:47:09

by Daniel Drake

[permalink] [raw]
Subject: Re: [PATCH] VIA IRQ quirk fixup only in XT_PIC mode Take 2

Sergio Monteiro Basto wrote:
> 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:

>> 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

2006-08-30 11:25:35

by Sergio Monteiro Basto

[permalink] [raw]
Subject: Re: [PATCH] VIA IRQ quirk fixup only in XT_PIC mode Take 2

On Tue, 2006-08-29 at 23:46 -0400, Daniel Drake wrote:
> Sergio Monteiro Basto wrote:
> > 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,

>
> The following comment still stands when we just revert Bjorn's change:
>
> >> 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

2006-08-31 03:46:19

by Daniel Drake

[permalink] [raw]
Subject: Re: [PATCH] VIA IRQ quirk fixup only in XT_PIC mode Take 2

Sergio Monteiro Basto wrote:
>> 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

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

> 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

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

Thanks,
Daniel

2006-08-31 11:33:29

by Sergio Monteiro Basto

[permalink] [raw]
Subject: Re: [PATCH] VIA IRQ quirk fixup only in XT_PIC mode Take 2

On Wed, 2006-08-30 at 12:13 -0400, Daniel Drake wrote:

> > 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
>
> 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.

2006-09-05 13:38:36

by Daniel Drake

[permalink] [raw]
Subject: Re: [PATCH] VIA IRQ quirk fixup only in XT_PIC mode Take 2

Sergio Monteiro Basto wrote:
> On Wed, 2006-08-30 at 12:13 -0400, Daniel Drake wrote:
>
>>> 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
>> 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


Daniel

2006-09-05 14:49:09

by Sergio Monteiro Basto

[permalink] [raw]
Subject: Re: [PATCH] VIA IRQ quirk fixup only in XT_PIC mode Take 2

On Tue, 2006-09-05 at 09:37 -0400, Daniel Drake wrote:
> Sergio Monteiro Basto wrote:
> > On Wed, 2006-08-30 at 12:13 -0400, Daniel Drake wrote:
> >
> >>> 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
> >> 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*...

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.