LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Daniel Barkalow <barkalow@iabervon.org>
To: Petr Vandrovec <petr@vandrovec.name>
Cc: jeff@garzik.org, linux-kernel@vger.kernel.org, akpm@osdl.org
Subject: Re: [PATCH] Unbreak MSI on ATI devices
Date: Sun, 24 Dec 2006 23:01:43 -0500 (EST)	[thread overview]
Message-ID: <Pine.LNX.4.64.0612242116090.20138@iabervon.org> (raw)
In-Reply-To: <20061221075540.GA21152@vana.vc.cvut.cz>

On Thu, 21 Dec 2006, Petr Vandrovec wrote:

> So my question is - what is real reason for disabling INTX when in MSI mode?
> According to PCI spec it should not be needed.

The PCI spec is at least not clear enough on the matter to keep nVidia 
from thinking that it's the OS's responsibility to make legacy interrupts 
not happen, by disabling INTX.

> None of devices in the box assert INTX while in MSI even if INTX is enabled.

I've got a forcedeth-driven ethernet card that does, and people have 
reported that nVidia "Intel HDA" sound does as well.

> So I'd like to see first patch below accepted.  If there are some 
> devices which require INTX disabling, then apparently decision whether 
> to disable it or no has to be moved to device drivers, or some 
> blacklist/whitelist must be created...

PCI Express (IIRC) had the pci_intx() calls already, so it's probably 
actually required by the spec (or at least common implementations) there. 

I'd guess that it's more common for hardware to be unhappy with intx 
enabled than to be unhappy with intx disabled, since the hardware is 
supposed to not send legacy interrupts.

> I'm not sure about second one - I have it in my tree for months, but I run 
> that kernel only on hardware mentioned above, so it is probably too dangerous 
> until pci_enable_msi() gets answer whether MSI works or no always right.

I think it'd be better to add an module parameter, like in the later 
drivers in your patch. Figuring out how to get MSI working whenever it's 
available isn't going to move forward unless there's an easy way to test 
it, especially since (according to rumor) Windows doesn't use it at all.

> /proc/interrupts after patch.  Before patch *hci_hcd:usb* were at zero, 
> IRQ21 was stuck with IRQ count at 10000, and HCD complained about 
> "Unlink after no-IRQ?".

Maybe the intx disable is just totally broken for your device? It 
certainly shouldn't cause the delivery of *more* legacy interrupts, and if 
it does with MSI enabled, I'd be surprised if it didn't without MSI. My 
guess is that that device should get a quirk to just leave the INTx 
disable bit alone (such that pci_intx doesn't do anything, regardless of 
context).

> diff -uprdN linux/sound/pci/atiixp.c linux/sound/pci/atiixp.c
> --- linux/sound/pci/atiixp.c	2006-12-16 13:35:47.000000000 -0800
> +++ linux/sound/pci/atiixp.c	2006-12-16 13:57:09.000000000 -0800
> @@ -1442,6 +1446,11 @@ static int snd_atiixp_suspend(struct pci
>  	snd_atiixp_aclink_down(chip);
>  	snd_atiixp_chip_stop(chip);
>  
> +	if (chip->have_msi) {
> +		pci_disable_msi(pci);
> +	} else {
> +		pci_intx(pci, 0);
> +	}

This doesn't look right, at least for !chip->have_msi. Or is disabling 
intx desirable here for non-MSI? I'd guess that devices that freak out if 
you fiddle with intx are likely to be old, and therefore likely to not 
support MSI.

> @@ -1532,6 +1546,11 @@ static int snd_atiixp_free(struct atiixp
>  	if (chip->remap_addr)
>  		iounmap(chip->remap_addr);
>  	pci_release_regions(chip->pci);
> +	if (chip->have_msi) {
> +		pci_disable_msi(chip->pci);
> +	} else {
> +		pci_intx(chip->pci, 0);
> +	}

My playing with forcedeth trying to get my system working suggests that 
the expected intx state for a device with no driver is "not disabled". I 
think the else clause here would cause the device to not work if you used 
this driver, unloaded the module, and loaded a version without the patch 
(or kexeced an older kernel, or soft-rebooted into some operating system 
without MSI support).

	-Daniel
*This .sig left intentionally blank*

  parent reply	other threads:[~2006-12-25  4:01 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2006-12-21  7:55 Petr Vandrovec
2006-12-21  8:21 ` Jeff Garzik
2006-12-25  4:01 ` Daniel Barkalow [this message]
2007-01-04 21:59 ` Roland Dreier
2007-01-05  9:07   ` Petr Vandrovec
2007-01-06  0:54     ` Daniel Barkalow
2007-01-05 23:57   ` Daniel Barkalow
     [not found] <fa.yZrxrHh1AWLcv/+D2xYZ1VhVYb8@ifi.uio.no>
2006-12-21 14:36 ` Robert Hancock

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Pine.LNX.4.64.0612242116090.20138@iabervon.org \
    --to=barkalow@iabervon.org \
    --cc=akpm@osdl.org \
    --cc=jeff@garzik.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=petr@vandrovec.name \
    --subject='Re: [PATCH] Unbreak MSI on ATI devices' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).