Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Linux Kernel Mailing List wrote:
> commit 134a11f0c37c043d3ea557ea15b95b084e3cc2c8
> tree c23bfd643913ea2d8cd01b17c1572b9602de7fd5
> parent c387fd85f84b9d89a75596325d8d6a0f730baf64
> author Takashi Iwai <tiwai@suse.de> 1163156917 +0100
> committer Linus Torvalds <torvalds@woody.osdl.org> 1163549067 -0800
> 
> [PATCH] ALSA: hda-intel - Disable MSI support by default
> 
> Disable MSI support on HD-audio driver as default since there are too
> many broken devices.
> 
> The module option is changed from disable_msi to enable_msi, too.  For
> turning MSI support on, pass enable_msi=1, instead.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Linus Torvalds <torvalds@osdl.org>

:(  Like AHCI, PCI MSI has -always- worked wonderfully for HD audio AFAIK.

Is a whitelist patch forthcoming?

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Linus Torvalds]

On Tue, 14 Nov 2006, Jeff Garzik wrote:
> 
> :(  Like AHCI, PCI MSI has -always- worked wonderfully for HD audio AFAIK.

That "AFAIK" is shorthand for "As Far As I haven't read any of the 
bug-reports but Know", right?

> Is a whitelist patch forthcoming?

Probably not. The advantages of MSI aren't all that obvious, and the 
disadvantages seem to be that it just doesn't work all that well for some 
people.

The fact that it works for MOST people has absolutely zero relevance. 
We've had too many frigging patches that have apparently been of the "this 
works for me, I don't care if some other motherboard has problems" kind.

See for example:

	http://lkml.org/lkml/2006/10/7/164

and yes, that HDA MSI _does_ seem to be causing problems.

So don't blather about "MSI never causes problems". It's broken. Please 
stop living in denial.

When somebody can actually say what the huge advantages to MSI are that 
it's worth using when 

 (a) several motherboards are apparently known broken

 (b) microsoft apparently is of the same opinion and _also_ doesn't use it

 (c) the old non-MSI code works fine

 (d) there is apparently no fool-proof way to tell when it works and when 
     it doesn't.

then please holler. Btw, I'm not even _interested_ in any advantages 
unless you also have a solution for (d). Not a "it should work". I want to 
hear something that is _guaranteed_ to work.

Until that point, I will suggest that we always just disable MSI by 
default, and people who want to use it need to enable it BY HAND.

F*ck whitelists. It's better and easier to just NOT USE IT! There are 
basically zero advantages to using MSI anyway, until you get to big enough 
systems that you have a system maintainer that can make the decision.

			Linus

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Tue, 14 Nov 2006, Jeff Garzik wrote:
>> :(  Like AHCI, PCI MSI has -always- worked wonderfully for HD audio AFAIK.
> 
> That "AFAIK" is shorthand for "As Far As I haven't read any of the 
> bug-reports but Know", right?

None of the bug reports indicate Intel, thus following the well 
established pattern of "it works great on Intel, but not elsewhere"


>> Is a whitelist patch forthcoming?
> 
> Probably not. The advantages of MSI aren't all that obvious, and the 
> disadvantages seem to be that it just doesn't work all that well for some 
> people.
> 
> The fact that it works for MOST people has absolutely zero relevance. 
> We've had too many frigging patches that have apparently been of the "this 
> works for me, I don't care if some other motherboard has problems" kind.
> 
> See for example:
> 
> 	http://lkml.org/lkml/2006/10/7/164
> 
> and yes, that HDA MSI _does_ seem to be causing problems.

But not on Intel, hence the obvious whitelist question.


> So don't blather about "MSI never causes problems". It's broken. Please 
> stop living in denial.
> 
> When somebody can actually say what the huge advantages to MSI are that 
> it's worth using when 
> 
>  (a) several motherboards are apparently known broken

several non-Intel motherboards


>  (b) microsoft apparently is of the same opinion and _also_ doesn't use it

Yeah well, that's sage advice only when it's sage advice.  MS lags us by 
years.  We do some bleeding, on the bleeding edge.


>  (c) the old non-MSI code works fine
> 
>  (d) there is apparently no fool-proof way to tell when it works and when 
>      it doesn't.
> 
> then please holler. Btw, I'm not even _interested_ in any advantages 
> unless you also have a solution for (d). Not a "it should work". I want to 
> hear something that is _guaranteed_ to work.

if (intel) ...

That has a track record of working.

It's nice not to have to deal with shared interrupts.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Linus Torvalds]

On Tue, 14 Nov 2006, Jeff Garzik wrote:
> 
> But not on Intel, hence the obvious whitelist question.

Hmm. Maybe. I'd be happier with a global "we can do MSI" flag, and making 
it easier for people to enable it (rather than do this one driver at a 
time). And yes, _if_ it's true that MSI works on all Intel SB/NB 
combinations, then maybe we could enable it for those systems.

In the meantime, I'm really tired of continually hearing about MSI 
problems, when there really aren't that many advantages.

> It's nice not to have to deal with shared interrupts.

I don't think "nice" is enough of an advantage to overcome "doesn't work 
on God knows how many systems".

		Linus

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Randy Dunlap]

On Tue, 14 Nov 2006 21:40:33 -0500 Jeff Garzik wrote:

> Linus Torvalds wrote:
> > 
> > On Tue, 14 Nov 2006, Jeff Garzik wrote:
> >> :(  Like AHCI, PCI MSI has -always- worked wonderfully for HD audio AFAIK.
> > 
> > That "AFAIK" is shorthand for "As Far As I haven't read any of the 
> > bug-reports but Know", right?
> 
> None of the bug reports indicate Intel, thus following the well 
> established pattern of "it works great on Intel, but not elsewhere"
> 
> 
> >> Is a whitelist patch forthcoming?
> > 
> > Probably not. The advantages of MSI aren't all that obvious, and the 
> > disadvantages seem to be that it just doesn't work all that well for some 
> > people.
> > 
> > The fact that it works for MOST people has absolutely zero relevance. 
> > We've had too many frigging patches that have apparently been of the "this 
> > works for me, I don't care if some other motherboard has problems" kind.
> > 
> > See for example:
> > 
> > 	http://lkml.org/lkml/2006/10/7/164
> > 
> > and yes, that HDA MSI _does_ seem to be causing problems.
> 
> But not on Intel, hence the obvious whitelist question.
> 
> 
> > So don't blather about "MSI never causes problems". It's broken. Please 
> > stop living in denial.
> > 
> > When somebody can actually say what the huge advantages to MSI are that 
> > it's worth using when 
> > 
> >  (a) several motherboards are apparently known broken
> 
> several non-Intel motherboards
> 
> 
> >  (b) microsoft apparently is of the same opinion and _also_ doesn't use it
> 
> Yeah well, that's sage advice only when it's sage advice.  MS lags us by 
> years.  We do some bleeding, on the bleeding edge.
> 
> 
> >  (c) the old non-MSI code works fine
> > 
> >  (d) there is apparently no fool-proof way to tell when it works and when 
> >      it doesn't.
> > 
> > then please holler. Btw, I'm not even _interested_ in any advantages 
> > unless you also have a solution for (d). Not a "it should work". I want to 
> > hear something that is _guaranteed_ to work.
> 
> if (intel) ...
> 
> That has a track record of working.
> 
> It's nice not to have to deal with shared interrupts.

FWIW, I concur with it always working for me on my Intel motherboard and
Intel-based Lenovo ThinkPad for hdaudio, ethernet, and SATA/AHCI.

---
~Randy

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[David Miller]

From: Linus Torvalds <torvalds@osdl.org>
Date: Tue, 14 Nov 2006 18:49:43 -0800 (PST)

> In the meantime, I'm really tired of continually hearing about MSI 
> problems, when there really aren't that many advantages.
> 
> > It's nice not to have to deal with shared interrupts.
> 
> I don't think "nice" is enough of an advantage to overcome "doesn't work 
> on God knows how many systems".

>From what I see, that's not the advantage of MSI.  The following
doesn't apply so well to sound, but it does apply significantly
to the networking.

The big advantage is that it eliminates the ordering issue in
interrupt handlers that use a DMA'd status block.  Look at the Tigon3
driver for one good example.

If you take an INTX interrupt, it's over a pin on the motherboard and
thus can arrive before the DMA makes it to main memory, so you have to
have all of this fixup logic to handle that case and you don't even
know the interrupt is really for you so you have to actually check
the status block.

static irqreturn_t tg3_interrupt(int irq, void *dev_id)
{
	struct net_device *dev = dev_id;
	struct tg3 *tp = netdev_priv(dev);
	struct tg3_hw_status *sblk = tp->hw_status;
	unsigned int handled = 1;

	/* In INTx mode, it is possible for the interrupt to arrive at
	 * the CPU before the status block posted prior to the interrupt.
	 * Reading the PCI State register will confirm whether the
	 * interrupt is ours and will flush the status block.
	 */
	if ((sblk->status & SD_STATUS_UPDATED) ||
	    !(tr32(TG3PCI_PCISTATE) & PCISTATE_INT_NOT_ACTIVE)) {
		/*
		 * Writing any value to intr-mbox-0 clears PCI INTA# and
		 * chip-internal interrupt pending events.
		 * Writing non-zero to intr-mbox-0 additional tells the
		 * NIC to stop sending us irqs, engaging "in-intr-handler"
		 * event coalescing.
		 */
		tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
			     0x00000001);
		if (tg3_irq_sync(tp))
			goto out;
		sblk->status &= ~SD_STATUS_UPDATED;
		if (likely(tg3_has_work(tp))) {
			prefetch(&tp->rx_rcb[tp->rx_rcb_ptr]);
			netif_rx_schedule(dev);		/* schedule NAPI poll */
		} else {
			/* No work, shared interrupt perhaps?  re-enable
			 * interrupts, and flush that PCI write
			 */
			tw32_mailbox_f(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW,
			     	0x00000000);
		}
	} else {	/* shared interrupt */
		handled = 0;
	}
out:
	return IRQ_RETVAL(handled);
}

This whole initial if statement goes away with MSI, you don't even
need to check the status block, you know the interrupt is for you and
the device did generate it and has work pending.

/* MSI ISR - No need to check for interrupt sharing and no need to
 * flush status block and interrupt mailbox. PCI ordering rules
 * guarantee that MSI will arrive after the status block.
 */
static irqreturn_t tg3_msi(int irq, void *dev_id)
{
	struct net_device *dev = dev_id;
	struct tg3 *tp = netdev_priv(dev);

	prefetch(tp->hw_status);
	prefetch(&tp->rx_rcb[tp->rx_rcb_ptr]);
	/*
	 * Writing any value to intr-mbox-0 clears PCI INTA# and
	 * chip-internal interrupt pending events.
	 * Writing non-zero to intr-mbox-0 additional tells the
	 * NIC to stop sending us irqs, engaging "in-intr-handler"
	 * event coalescing.
	 */
	tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW, 0x00000001);
	if (likely(!tg3_irq_sync(tp)))
		netif_rx_schedule(dev);		/* schedule NAPI poll */

	return IRQ_RETVAL(1);
}

Taken to the next level MSI can be implemented in a "one-shot"
manner such that once the MSI is generated the chip shuts off
all further interrupts automatically.

This is perfect for networking devices since that is the first
thing we need to do in the interrupt handler for NAPI, and Tigon3
actually implements this.

/* One-shot MSI handler - Chip automatically disables interrupt
 * after sending MSI so driver doesn't have to do it.
 */
static irqreturn_t tg3_msi_1shot(int irq, void *dev_id)
{
	struct net_device *dev = dev_id;
	struct tg3 *tp = netdev_priv(dev);

	prefetch(tp->hw_status);
	prefetch(&tp->rx_rcb[tp->rx_rcb_ptr]);

	if (likely(!tg3_irq_sync(tp)))
		netif_rx_schedule(dev);		/* schedule NAPI poll */

	return IRQ_HANDLED;
}

It saves an MMIO write in the interrupt handler and makes a huge
different in performance.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Linus Torvalds]

On Tue, 14 Nov 2006, David Miller wrote:
> > 
> > I don't think "nice" is enough of an advantage to overcome "doesn't work 
> > on God knows how many systems".
> 
> From what I see, that's not the advantage of MSI.  The following
> doesn't apply so well to sound, but it does apply significantly
> to the networking.

You're totally missing the issue.

Read that sentence of mine again.

Yours was still an example of "nice". And it had absolutely nothing to do 
with the _PROBLEM_.

			Linus

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[D. Hazelton]

On Tuesday 14 November 2006 21:40, Jeff Garzik wrote:
> Linus Torvalds wrote:
> > On Tue, 14 Nov 2006, Jeff Garzik wrote:
> >> :(  Like AHCI, PCI MSI has -always- worked wonderfully for HD audio
> >> : AFAIK.
> >
> > That "AFAIK" is shorthand for "As Far As I haven't read any of the
> > bug-reports but Know", right?
>
> None of the bug reports indicate Intel, thus following the well
> established pattern of "it works great on Intel, but not elsewhere"

Since when does INTEL make policy about Linux?
It'd be better to just follow Linus' suggestion and *NOT* use it.

> >> Is a whitelist patch forthcoming?
> >
> > Probably not. The advantages of MSI aren't all that obvious, and the
> > disadvantages seem to be that it just doesn't work all that well for some
> > people.
> >
> > The fact that it works for MOST people has absolutely zero relevance.
> > We've had too many frigging patches that have apparently been of the
> > "this works for me, I don't care if some other motherboard has problems"
> > kind.
> >
> > See for example:
> >
> > 	http://lkml.org/lkml/2006/10/7/164
> >
> > and yes, that HDA MSI _does_ seem to be causing problems.
>
> But not on Intel, hence the obvious whitelist question.

AFAICT that is the problem. You are fixated on the fact that MSI for the Intel 
HD Audio works on INTEL motherboards. Pardon the language but - BIG FUCKING 
SURPRISE THERE! The systems I'm running right now might be Intel chipset 
boards, but that is simply a fluke. (They were given to me)

As a simple fact the only reason I see Linux users moving to systems with 
Intel chipsets is that Intel is actually making a lot of its driver code open 
source. Other than that... Well, let me just point out that not one of the 
Linux users I personally know have a system with an Intel chipset.

> > So don't blather about "MSI never causes problems". It's broken. Please
> > stop living in denial.
> > 
> > When somebody can actually say what the huge advantages to MSI are that
> > it's worth using when
> >
> >  (a) several motherboards are apparently known broken
>
> several non-Intel motherboards

Again you focus on Intel. Give it up already. You've proved that you have no 
real argument *FOR* having MSI in the driver.

> >  (b) microsoft apparently is of the same opinion and _also_ doesn't use
> > it
>
> Yeah well, that's sage advice only when it's sage advice.  MS lags us by
> years.  We do some bleeding, on the bleeding edge.

and is light years ahead in some parts. Have you seen the "usermode driver" 
system released as part of Vista and as a patch for XP? I, personally, cannot 
see how it could work or be secure while doing so, but it does have an 
advantage... Namely that a driver cannot bring the kernel down.

That Linux is ahead of Windows in a lot of cases is due more to the fact that 
it is extremely easy to get new technology implemented and working in Linux 
(and accepted into the kernel) than it is for the Windows. (Due more to the 
*MASSIVE* size of the windows code base and the fact that its a closed 
corporate project than anything)

But anyway...

> >  (c) the old non-MSI code works fine
> >
> >  (d) there is apparently no fool-proof way to tell when it works and when
> >      it doesn't.
> >
> > then please holler. Btw, I'm not even _interested_ in any advantages
> > unless you also have a solution for (d). Not a "it should work". I want
> > to hear something that is _guaranteed_ to work.
>
> if (intel) ...

Again crowing about how something works on ONE (1) (Uno, Eins, Un, Ichi) 
platform. A single working platform is no reason to complicate code with a 
whitelist.

>
> That has a track record of working.
>
> It's nice not to have to deal with shared interrupts.
>
> 	Jeff

Yes, I'm sure it is. But until it starts working on more than one platform 
it'd be stupid to add the code for a whitelist. Be easier to just *OFFER* MSI 
to people as a build option.  Then the people who *KNOW* it won't fubar their 
system can activate it.

(BTW, as Linus pointed out, MSI isn't really needed until you get to a system 
so large it has someone (or someones) there whose sole job is to make sure 
that the best and fastest ways of doing things are used and are fully 
functional)

DRH

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[David Miller]

From: Linus Torvalds <torvalds@osdl.org>
Date: Tue, 14 Nov 2006 19:10:42 -0800 (PST)

> Yours was still an example of "nice". And it had absolutely nothing
> to do with the _PROBLEM_.

Understood.

BTW, some drivers have taken the approch to add MSI self-tests
inside of the driver to ensure correct option of MSI on a given
machine.  There's a lot of resistence to that, the reasons for
which I grok fully, but I'm not sure other suggestions such as
black lists are any better.

Given current experience maybe white-lists are in fact the way
to go.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

D. Hazelton wrote:
> Again crowing about how something works on ONE (1) (Uno, Eins, Un, Ichi) 
> platform. A single working platform is no reason to complicate code with a 
> whitelist.

A rather high volume platform with lots of users.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[D. Hazelton]

On Tuesday 14 November 2006 22:30, Jeff Garzik wrote:
> D. Hazelton wrote:
> > Again crowing about how something works on ONE (1) (Uno, Eins, Un, Ichi)
> > platform. A single working platform is no reason to complicate code with
> > a whitelist.
>
> A rather high volume platform with lots of users.
>
> 	Jeff

Point is that it is still only one platform. Or don't you understand the point 
I was making?

DRH

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Linus Torvalds]

On Tue, 14 Nov 2006, David Miller wrote:
> 
> > Yours was still an example of "nice". And it had absolutely nothing
> > to do with the _PROBLEM_.
> 
> Understood.

Btw, before somebody thinks I hate MSI - I'd absolutely _love_ for it to 
work, because I think MSI has the potential of getting rid of a lot of irq 
routing problems.

And that's more than just a "nice" - we've obviously had issues with 
machines not working right because we didn't get the magic PIRQ tables or 
ACPI crud right. 

So I'd actually be thrilled if we could use MSI more. (And here, "MSI" 
should be considered to also include "MSI-X" etc - please, no language 
lawyering).

> Given current experience maybe white-lists are in fact the way
> to go.

The thing that worries me is that I can well believe that the Intel NB/SB 
combination gets MSI 100% correct, and I'd love to whitelist it.

HOWEVER - that's only true on systems with no other PCI bridges. Even if 
you have an Intel NB/SB, what about other bridges in that same system, and 
the devices behind them? 

Now, I think that a MSI thing should look like a PCI write to a magic 
address (I'm really not very up on it, so correct me if I'm wrong), and 
thus maybe bridges are bound to get it right, and the only thing we really 
need to worry about is the host bridge. Maybe. In that case, it might be 
sensible to have a host-bridge white-table, and if we know all Intel 
bridges that claim to support MSI do so correctly, then maybe we can just 
say "ok, always enable it for Intel host bridges".

But right now I'm not convinced we really know what all goes wrong. Maybe 
it's just broken NVidia and AMD bridges. But maybe it's also individual 
devices that continue to (for example) raise _both_ the legacy IRQ line 
_and_ send an MSI request.

Maybe even Intel host bridges have all the same troubles with that, and 
the reason we haven't seen it is that _usually_ an Intel host bridge goes 
together with certain Intel MSI-capable chips (ie e1000, Intel HDA etc). 
So for all we know, it's not necessarily the Intel host bridge that is the 
magic thing to make things work, it could be something that just has a 
high _correlation_ with an Intel host bridge.

So call me a nervous nellie. 

		Linus

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Benjamin Herrenschmidt]

> If you take an INTX interrupt, it's over a pin on the motherboard and
> thus can arrive before the DMA makes it to main memory, so you have to
> have all of this fixup logic to handle that case and you don't even
> know the interrupt is really for you so you have to actually check
> the status block.

Out of curiosity. Are you sure there is no case of stupid bridge
converting the MSI into some APIC/whatever interrupt for the CPU
potentially before all previous DMA have been fully pushed to the
coherent domain (still in some internal store queue for example) ?

I suppose the Intel bridges get it right since they pretty much defined
them in the first place... but my experience with chipset designers is
that they have generally little regard for ordering issues (and the
impact those have on software) and no clue about anything related to
interrupt issues.

Ben.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Benjamin Herrenschmidt]

> I don't think "nice" is enough of an advantage to overcome "doesn't work 
> on God knows how many systems".

Note that the PCIe spec mandates MSIs while old style interrults (LSIs)
are optional... The result is that there are already platforms (though
not x86 just yet) being design with simply no support for LSIs on PCIe
and I've heard of devices doing that too (yeah, that's weird, they
wouldn't work in windows I suppose).

I suppose none of this affects x86 for now... For ppc, once we get our
new MSI layer in, however, I'll have to keep them enabled by default,
though there are fewer platforms involved and the chances at this point
are fairly high that they all use a working chipset.
  
Ben.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[David Miller]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Wed, 15 Nov 2006 15:01:00 +1100

> Out of curiosity. Are you sure there is no case of stupid bridge
> converting the MSI into some APIC/whatever interrupt for the CPU
> potentially before all previous DMA have been fully pushed to the
> coherent domain (still in some internal store queue for example) ?

That would really suck, wouldn't it :)

However, they have to do all the work of processing the memory
transation that the MSI is on the PCI bus, I don't think they would go
so far out of their way to reorder things even if they converted the
MSI packet into a PIN to the APIC, for example.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Linus Torvalds wrote:
> HOWEVER - that's only true on systems with no other PCI bridges. Even if 
> you have an Intel NB/SB, what about other bridges in that same system, and 
> the devices behind them? 
> 
> Now, I think that a MSI thing should look like a PCI write to a magic 
> address (I'm really not very up on it, so correct me if I'm wrong), and 
> thus maybe bridges are bound to get it right, and the only thing we really 
> need to worry about is the host bridge. Maybe. In that case, it might be 
> sensible to have a host-bridge white-table, and if we know all Intel 
> bridges that claim to support MSI do so correctly, then maybe we can just 
> say "ok, always enable it for Intel host bridges".

That's pretty much the idea behind MSI... it looks like any other PCI 
bus transaction, rather than needing a separate pin.


> But right now I'm not convinced we really know what all goes wrong. Maybe 
> it's just broken NVidia and AMD bridges. But maybe it's also individual 
> devices that continue to (for example) raise _both_ the legacy IRQ line 
> _and_ send an MSI request.

That reminds me of a potential driver bug -- MSI-aware drivers need to 
call pci_intx(pdev,0) to turn off the legacy PCI interrupt, before 
enabling MSI interrupts.

So far, MSI history on x86 has always followed these rules:
* it works on Intel
* it doesn't work [well | at all] on AMD/NV

The only thing that has changed recently is that people are trying to 
get it working on AMD/NV as well.  (Brice Goglin's stuff starting at 
6397c75cbc4d7dbc3d07278b57c82a47dafb21b5 in 'git log')

Seems to me this hda-intel driver patch is fallout from 
pci_msi_enabled() not failing on enough systems (all AMD/NV?).

Anyway, if you want your master switch, put it there, not in each driver...

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Roland Dreier]

 > Now, I think that a MSI thing should look like a PCI write to a magic 
 > address (I'm really not very up on it, so correct me if I'm wrong), and 
 > thus maybe bridges are bound to get it right, and the only thing we really 
 > need to worry about is the host bridge. Maybe.

Yes, an MSI message really is just a PCI write cycle (which is why it
obeys all the PCI ordering as DaveM pointed out, and also can travel
on different PCIe VCs, etc).  So it's hard to see how a bridge could
mess it up without screwing up lots of other stuff.  The big thing is
the host bridge that has to turn the PCI write into an interrupt, and
that's where all the chipset problems that I know of are.

But on the other hand I don't think we have much experience with
MSI-capable devices below multiple levels of bridges, so you're right,
there is an unknown risk here.

 > But right now I'm not convinced we really know what all goes wrong. Maybe 
 > it's just broken NVidia and AMD bridges. But maybe it's also individual 
 > devices that continue to (for example) raise _both_ the legacy IRQ line 
 > _and_ send an MSI request.

Well, since we now require drivers to "opt in" to MSI, I'm not
convinced that broken devices are a huge issue.  We can take things
case-by-case and, obviously, if some random subset of devices are
broken, then clearly the driver should be really careful about
enabling MSI.  But usually there's something like a device ID,
revision, FW version, etc. that we can test.

 > together with certain Intel MSI-capable chips (ie e1000, Intel HDA etc). 

Just for the record -- many e1000 chips that have an MSI capability in
their PCI config actually do not have working MSI.  Hence the

	if (adapter->hw.mac_type > e1000_82547_rev_2) {
		adapter->have_msi = TRUE;

in the e1000 driver I guess.

BTW, at the end of the day I agree with the "MSI off by default"
policy, especially for the sound driver where the only realy gain is
saving IRQ routing hassles as you said.  Even for the mthca InfiniBand
driver, where MSI-X with multiple vectors is a huge performance win, I
still have the user explicitly enable it.

 - R.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Benjamin Herrenschmidt wrote:
> I suppose none of this affects x86 for now... For ppc, once we get our
> new MSI layer in, however, I'll have to keep them enabled by default,
> though there are fewer platforms involved and the chances at this point
> are fairly high that they all use a working chipset.

Ironically there are a shitload more MSI-capable devices out there, than 
there are MSI-capable systems.

MSI simplifies things for the chip designers, so I don't doubt they are 
chomping at the bit to start putting out MSI-only devices.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Tue, 14 Nov 2006 23:11:54 -0500

> Linus Torvalds wrote:
> > HOWEVER - that's only true on systems with no other PCI bridges. Even if 
> > you have an Intel NB/SB, what about other bridges in that same system, and 
> > the devices behind them? 
> > 
> > Now, I think that a MSI thing should look like a PCI write to a magic 
> > address (I'm really not very up on it, so correct me if I'm wrong), and 
> > thus maybe bridges are bound to get it right, and the only thing we really 
> > need to worry about is the host bridge. Maybe. In that case, it might be 
> > sensible to have a host-bridge white-table, and if we know all Intel 
> > bridges that claim to support MSI do so correctly, then maybe we can just 
> > say "ok, always enable it for Intel host bridges".
> 
> That's pretty much the idea behind MSI... it looks like any other PCI 
> bus transaction, rather than needing a separate pin.

Yep.

> > But right now I'm not convinced we really know what all goes wrong. Maybe 
> > it's just broken NVidia and AMD bridges. But maybe it's also individual 
> > devices that continue to (for example) raise _both_ the legacy IRQ line 
> > _and_ send an MSI request.
> 
> That reminds me of a potential driver bug -- MSI-aware drivers need to 
> call pci_intx(pdev,0) to turn off the legacy PCI interrupt, before 
> enabling MSI interrupts.

Is this absolutely true?  I've never been sure about this point, and I
was rather convinced after reading various documents that once you
program up the MSI registers to start generating MSI this implicitly
disabled INTX and this was even in the PCI specification.

It would be great to get a definitive answer on this.

If it is mandatory, perhaps the driver shouldn't be doing it and
rather the PCI layer MSI enabling should.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Roland Dreier]

 > Out of curiosity. Are you sure there is no case of stupid bridge
 > converting the MSI into some APIC/whatever interrupt for the CPU
 > potentially before all previous DMA have been fully pushed to the
 > coherent domain (still in some internal store queue for example) ?

That's forbidden by the PCI spec:

    "An MSI or MSI-X message, by virtue of being a posted memory write
    (PMW) transaction, is prohibited by PCI ordering rules from
    passing PMW transactions sent earlier by the function. The system
    must guarantee that an interrupt service routine invoked as a
    result of a given message will observe any updates performed by
    PMW transactions arriving prior to that message."

which is not to say that there are no chipsets with errata in this
area.  But I've never heard of such a thing...

 - R.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

David Miller wrote:
> Is this absolutely true?  I've never been sure about this point, and I
> was rather convinced after reading various documents that once you
> program up the MSI registers to start generating MSI this implicitly
> disabled INTX and this was even in the PCI specification.
> 
> It would be great to get a definitive answer on this.
> 
> If it is mandatory, perhaps the driver shouldn't be doing it and
> rather the PCI layer MSI enabling should.


I can't answer for the spec, but at least two independent device vendors 
recommended to write an MSI driver that way (disable intx, enable msi).

Completely independent of MSI though, a PCI 2.2 compliant driver should 
be nice and disable intx on exit, just to avoid any potential interrupt 
hassles after driver unload.  And of course be aware that it might need 
to enable intx upon entry.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Roland Dreier]

 > and I've heard of devices doing that too (yeah, that's weird, they
 > wouldn't work in windows I suppose).

for example the QLogic PCIe InfiniBand adapter (drivers/infiniband/hw/ipath)
can't generate legacy INTx interrupts...

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[David Miller]

From: Jeff Garzik <jeff@garzik.org>
Date: Tue, 14 Nov 2006 23:24:04 -0500

> I can't answer for the spec, but at least two independent device vendors 
> recommended to write an MSI driver that way (disable intx, enable msi).

Ok.

> Completely independent of MSI though, a PCI 2.2 compliant driver should 
> be nice and disable intx on exit, just to avoid any potential interrupt 
> hassles after driver unload.  And of course be aware that it might need 
> to enable intx upon entry.

This also sounds like it should occur in the generic PCI layer when a
PCI driver is unregistered.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Roland Dreier]

 > That reminds me of a potential driver bug -- MSI-aware drivers need to
 > call pci_intx(pdev,0) to turn off the legacy PCI interrupt, before
 > enabling MSI interrupts.

Huh?  The device can't generate any legacy interrupts once MSI is
enabled.  As the PCI spec says:

    "While enabled for MSI or MSI-X operation, a function is prohibited
    from using its INTx# pin (if implemented) to request service (MSI,
    MSI-X, and INTx# are mutually exclusive)."

Although the MSI core does do pci_intx() for PCIe devices only, for
some reason I can't grok.

 > The only thing that has changed recently is that people are trying to
 > get it working on AMD/NV as well.  (Brice Goglin's stuff starting at
 > 6397c75cbc4d7dbc3d07278b57c82a47dafb21b5 in 'git log')

Actually NVidia/AMD was working on some systems long before that -- I
had it working at least 2 years ago.

 - R.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Andi Kleen]

Linus Torvalds <torvalds@osdl.org> writes:
> 
> But right now I'm not convinced we really know what all goes wrong. Maybe 
> it's just broken NVidia and AMD bridges.

At least AMD (PCI-X) and Serverworks bridges are known broken with MSI
They should be both quirked though. Or rather one SVW bridge is quirked
there might be more. AMD 8131 is quirked. AMD 8132 is broken too but 
should not have the capability structure in the first place.

BTW there seems to be a new ACPI FADT bit that says "MSI is broken"
We should probably check that too as a double check.

-Andi

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Roland Dreier wrote:
>  > That reminds me of a potential driver bug -- MSI-aware drivers need to
>  > call pci_intx(pdev,0) to turn off the legacy PCI interrupt, before
>  > enabling MSI interrupts.
> 
> Huh?  The device can't generate any legacy interrupts once MSI is
> enabled.  As the PCI spec says:
> 
>     "While enabled for MSI or MSI-X operation, a function is prohibited
>     from using its INTx# pin (if implemented) to request service (MSI,
>     MSI-X, and INTx# are mutually exclusive)."
> 
> Although the MSI core does do pci_intx() for PCIe devices only, for
> some reason I can't grok.

Probably the same reason I just mentioned in a reply to DaveM.  Wildly 
speculating, some chips may depend on software to disable INTX -- i.e. 
depend on software to provide this aspect of PCI spec compliance.


>  > The only thing that has changed recently is that people are trying to
>  > get it working on AMD/NV as well.  (Brice Goglin's stuff starting at
>  > 6397c75cbc4d7dbc3d07278b57c82a47dafb21b5 in 'git log')
> 
> Actually NVidia/AMD was working on some systems long before that -- I
> had it working at least 2 years ago.

Well, just noting that AMD/NV seems to be a common pattern in the 
referenced bug reports, and the set of Linux platforms which claim to 
support MSI had recent churn.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Roland Dreier]

 > At least AMD (PCI-X) and Serverworks bridges are known broken with MSI
 > They should be both quirked though. Or rather one SVW bridge is quirked
 > there might be more. AMD 8131 is quirked. AMD 8132 is broken too but 
 > should not have the capability structure in the first place.

I thought people had AMD 8132 working?  The only MSI erratum I see for
the 8132 is that a write to the MSI address with all byte-enables
deasserted is bad.  But do any devices really do that?  It seems like
a really odd thing to do.

 - R.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Andi Kleen]

On Wednesday 15 November 2006 05:57, Roland Dreier wrote:
>  > At least AMD (PCI-X) and Serverworks bridges are known broken with MSI
>  > They should be both quirked though. Or rather one SVW bridge is quirked
>  > there might be more. AMD 8131 is quirked. AMD 8132 is broken too but 
>  > should not have the capability structure in the first place.
> 
> I thought people had AMD 8132 working?  The only MSI erratum I see for
> the 8132 is that a write to the MSI address with all byte-enables
> deasserted is bad.  But do any devices really do that?  It seems like
> a really odd thing to do.

Perhaps with special user space quirks, but not in mainline kernel because it's
not force enabled (see erratum #78).

-Andi

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Benjamin Herrenschmidt]

On Tue, 2006-11-14 at 20:07 -0800, David Miller wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Wed, 15 Nov 2006 15:01:00 +1100
> 
> > Out of curiosity. Are you sure there is no case of stupid bridge
> > converting the MSI into some APIC/whatever interrupt for the CPU
> > potentially before all previous DMA have been fully pushed to the
> > coherent domain (still in some internal store queue for example) ?
> 
> That would really suck, wouldn't it :)
> 
> However, they have to do all the work of processing the memory
> transation that the MSI is on the PCI bus, I don't think they would go
> so far out of their way to reorder things even if they converted the
> MSI packet into a PIN to the APIC, for example.

That would suck and not surprise me that much in fact... Take the Apple
bridge... it's unclear at which point they actually decode the MSI and
HT interrupts (the later are just internally converted to MSI-like
stores) to turn them into toggling of MPIC lines, but it probably
happens as an MMIO slave on the main xbar and I'm not 100% certain it
provides ordering vs. the previous stores to memory as they are in the
coherent domain and the MMIO is not. I just hope very much :-) Because
the store queue to memory can re-order on U4.

Ben.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Benjamin Herrenschmidt]

> which is not to say that there are no chipsets with errata in this
> area.  But I've never heard of such a thing...

Ok. Well, I've seen my load of PCI host bridges that were designed with
the PCI spec as toilet paper...

Ben.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Segher Boessenkool]

>> However, they have to do all the work of processing the memory
>> transation that the MSI is on the PCI bus, I don't think they  
>> would go
>> so far out of their way to reorder things even if they converted the
>> MSI packet into a PIN to the APIC, for example.
>
> That would suck and not surprise me that much in fact... Take the  
> Apple
> bridge... it's unclear at which point they actually decode the MSI and
> HT interrupts (the later are just internally converted to MSI-like
> stores) to turn them into toggling of MPIC lines,

That's all documented just fine.

> but it probably
> happens as an MMIO slave on the main xbar

Yes indeed.

> and I'm not 100% certain it
> provides ordering vs. the previous stores to memory as they are in the
> coherent domain and the MMIO is not. I just hope very much :-)

All those DMA stores get reflected to the CPUs (the north
bridge itself doesn't keep a cache directory).  All this
happens in-order.  There probably _is_ a window where the
last DMA store isn't yet globally visible but the CPU
interrupt was already signaled, but the act of trying to
access that data orders it itself :-)

> Because
> the store queue to memory can re-order on U4.

That's only the store queue to the actual memory devices,
it's outside of the coherent domain anyway.


Segher

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Takashi Iwai]

At Tue, 14 Nov 2006 19:21:17 -0800 (PST),
David Miller wrote:
> 
> From: Linus Torvalds <torvalds@osdl.org>
> Date: Tue, 14 Nov 2006 19:10:42 -0800 (PST)
> 
> > Yours was still an example of "nice". And it had absolutely nothing
> > to do with the _PROBLEM_.
> 
> Understood.
> 
> BTW, some drivers have taken the approch to add MSI self-tests
> inside of the driver to ensure correct option of MSI on a given
> machine.  There's a lot of resistence to that, the reasons for
> which I grok fully, but I'm not sure other suggestions such as
> black lists are any better.

The snd-hda-intel driver has a test of MSI, but it seems not working
on every machine.  It caused non-cared interrupts and the kernel
disabled that irq.

> Given current experience maybe white-lists are in fact the way
> to go.

Could it be whitelisted in the PCI driver side?  I don't think it's
good to have a huge white/blacklist in each device driver.


Takashi

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Alan]

> Point is that it is still only one platform. Or don't you understand the point 
> I was making?

Given the Intel chipset platform appears to be over half the market it
makes sense to do things right there. It also encourages other vendors to
fix their stuff or send us patches to do so.

So I don't think anyone gives a damn about your personal issues with
Intel.

Alan

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Krzysztof Halasa]

Jeff Garzik <jeff@garzik.org> writes:

> So far, MSI history on x86 has always followed these rules:
> * it works on Intel
> * it doesn't work [well | at all] on AMD/NV

I don't know how does it look when it doesn't work etc. but certainly
both NV Ethernet and HDA seem to work for me and:

$ cat /proc/interrupts 
           CPU0       
  0:     596146   IO-APIC-edge      timer
  1:      15425   IO-APIC-edge      i8042
  8:     103741   IO-APIC-edge      rtc
  9:          0   IO-APIC-fasteoi   acpi
 12:      25168   IO-APIC-edge      i8042
 14:         61   IO-APIC-edge      libata
 15:          0   IO-APIC-edge      libata
 20:          2   IO-APIC-fasteoi   ehci_hcd:usb1
 21:          0   IO-APIC-fasteoi   libata
 22:       4881   IO-APIC-fasteoi   libata
 23:      17010   IO-APIC-fasteoi   libata, ohci_hcd:usb2
279:     598326   PCI-MSI-edge      eth0
280:      21497   PCI-MSI-edge      eth0
281:      20448   PCI-MSI-edge      eth0
282:       4881   PCI-MSI-edge      HDA Intel
NMI:         70 
LOC:     596126 
ERR:          0

$ /sbin/lspci|grep nV 
00:00.0 RAM memory: nVidia Corporation MCP55 Memory Controller (rev a1)
00:01.0 ISA bridge: nVidia Corporation MCP55 LPC Bridge (rev a2)
00:01.1 SMBus: nVidia Corporation MCP55 SMBus (rev a2)
00:01.2 RAM memory: nVidia Corporation MCP55 Memory Controller (rev a2)
00:02.0 USB Controller: nVidia Corporation MCP55 USB Controller (rev a1)
00:02.1 USB Controller: nVidia Corporation MCP55 USB Controller (rev a2)
00:04.0 IDE interface: nVidia Corporation MCP55 IDE (rev a1)
00:05.0 IDE interface: nVidia Corporation MCP55 SATA Controller (rev a2)
00:05.1 IDE interface: nVidia Corporation MCP55 SATA Controller (rev a2)
00:05.2 IDE interface: nVidia Corporation MCP55 SATA Controller (rev a2)
00:06.0 PCI bridge: nVidia Corporation MCP55 PCI bridge (rev a2)
00:06.1 Audio device: nVidia Corporation MCP55 High Definition Audio (rev a2)
00:08.0 Bridge: nVidia Corporation MCP55 Ethernet (rev a2)
00:09.0 Bridge: nVidia Corporation MCP55 Ethernet (rev a2)
00:0b.0 PCI bridge: nVidia Corporation MCP55 PCI Express bridge (rev a2)
00:0c.0 PCI bridge: nVidia Corporation MCP55 PCI Express bridge (rev a2)
00:0d.0 PCI bridge: nVidia Corporation MCP55 PCI Express bridge (rev a2)
00:0e.0 PCI bridge: nVidia Corporation MCP55 PCI Express bridge (rev a2)
00:0f.0 PCI bridge: nVidia Corporation MCP55 PCI Express bridge (rev a2)

> Anyway, if you want your master switch, put it there, not in each driver...

Definitely.
-- 
Krzysztof Halasa

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Linus Torvalds]

On Tue, 14 Nov 2006, Roland Dreier wrote:
> 
> Huh?  The device can't generate any legacy interrupts once MSI is
> enabled.  As the PCI spec says:

PLEASE.

The spec is just so much toilet paper. The ONLY thing that matters is what 
real hardware does. So please please please PLEASE don't start quoting 
specs as a way to "prove your point". It is totally meaningless.

The fact is, we _know_ there are broken devices out there. Not just wrt 
MSI - pretty much every single sentence of the PCI spec is probably broken 
by _some_ device or subsystem. For example, I would expect that the DMA 
ordering rule is likely broken by the big SGI boxes, because they do 
memory coherency traffic separately from the IO traffic.

This should be the #1 rule for every kernel programmer: read the 
documentation, but don't actually assume it's complete, or that it is 
always true.

One of the big reasons user-level programming is so much simpler than 
kernel programming is that you can basically "think" about your 
algorithms. You seldom have cases where you literally just need to test 
out whether all hardware really works that way.

The bottom line: MSI looks great on paper. I think we might make it a rule 
that we can enable it by default on bridges we trust (which would seem to 
be mainly just Intel). But even then, I think it makes sense for 
per-driver rules, because

 - some drivers may care more than others (sound, for example, certainly 
   doesn't really care at all), apart from irq routing issues, and right 
   now those are _worse_ with MSI due to bugs.

 - we definitely have the potential for per-driver bugs (eg the "disable 
   INTx explicitly" kind of situation), so even if MSI works on a system 
   level, I think we all know that it doesn't always work on a device 
   level, and the safe thing tends to be to keep it disabled unless you 
   have some really overriding concern to use it.

End result: I think that at least for 2.6.19, and at least for the HDA 
sound driver, keeping it disabled by default is the right choice. We 
should probably _also_ make "pci_msi_supported()" just return an error 
(probably by just clearing "pci_msi_enable") for any non-intel host bridge 
for now.

I don't dispute AT ALL that we should try to enable things more in the 
future. I'm just saying that we're better off being careful, and realizing 
that documentation and "official rules" seldom match reality.

(People seem to trust documentation, because for _most_ devices, the 
documentation is correct. But if it's wrong for 1% of all devices, you 
should still realize that IT IS WRONG. You can't quote "the standard" to 
prove a point - a single exception is enough to show that a standard is 
incomplete)

			Linus

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Linus Torvalds]

On Wed, 15 Nov 2006, Takashi Iwai wrote:
> 
> The snd-hda-intel driver has a test of MSI, but it seems not working
> on every machine.  It caused non-cared interrupts and the kernel
> disabled that irq.

Yes. 

Btw, this was why I was claiming that maybe some devices might raise 
_both_ the MSI and the INTx interrupt, which can indeed cause problems 
like that: because we see spurious interrupts on some other irq line (the 
INTx one), we might decide to end up disabling that one, just because we 
can't seem to shut it up.

However, the same kind of schenario may happen if the MSI irq from a 
device simply doesn't _work_ - the device may claim MSI capabilities but 
always uses INTx, and you'd get the same behaviour from just _testing_ the 
MSI line - the irq comes in on the wrong vector, and since you're not 
handling that vector, the kernel has no choice but to say "I will have to 
disable this screaming irq".

So "testing" that an MSI works isn't actually goign to solve any real 
problems. It may or may not show that the MSI works, but regardless of the 
success of the test, it can have deadly consequences for _other_ devices 
if the irq routing (which may be INTx) is broken.

This is why I'm so adamant that we need to _know_ that it works before we 
use it. When irq's get mis-routed, things go downhill real fast. We're 
usually talking "dead machines", and there is very little that a driver 
can do about it.

			Linus

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Arjan van de Ven]

> This is why I'm so adamant that we need to _know_ that it works before we 
> use it. When irq's get mis-routed, things go downhill real fast. We're 
> usually talking "dead machines", and there is very little that a driver 
> can do about it.

well we could cheat some. And have the generic code for this just
register the irq handler for both somehow. At least the case for "only
does old style and no actual MSI" will work then.
For the duplicate issue... if the generic irq layer keeps track of the
"mirror" property of the handler it could be dealt with (eg clear the
unhandled count of the "mirror" as well on each handled irq).. 

I'll grant you that it's a bit evil, and probably ulgy to keep the
mirror information, but if it gets the thing reliable.....  it could
solve a lot of the mess.

(Oh and I would *love* for this to printk once if the kernel detects
cheating behavior so that we can make things like the linux firmware
test kit complain to the hw/bios folks early on)


-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Linus Torvalds]

On Wed, 15 Nov 2006, Arjan van de Ven wrote:
> 
> well we could cheat some. And have the generic code for this just
> register the irq handler for both somehow.

Well, not generic code. It would have to be the driver itself that does 
it, since generic code doesn't even know (at irq request time - and when 
they are generated - it just gets the irq number).

And the thing is, once you do that, all the advantages of MSI totally go 
away - both the "nice" ones and the "really good ones" (the latter being 
the hopeful eventual removal of irq routing confusions). So if you do 
that, the better solution is for the driver to say "I won't use MSI at 
all".

Really.

It all boils down to the same thing: either we have to know that MSI works 
(where "know" is obviously relative - it's not like you can avoid _all_ 
bugs, but dammit, even a single report of "not working" means that there 
are probably a ton of machines like that, and we did something wrong), or 
we shouldn't use it. There is no middle ground. You can't really safely 
"test" for it, and while you _can_ say "just do both", it doesn't really 
help anything (and potentially exposes you to just more bugs: if enablign 
MSI actually _does_ disable INTx, but then doesn't work, at a minimum you 
end up with a device that doesn't work, even if the rest of the kernel 
might be ok).

And btw, I say this as a person whose new main machine used to have HDA 
routed over MSI, and the decision to default to it off meant that it went 
back to the regular INTx thing.

(Btw, MSI interrupts also seem to not participate in CPU balancing:

 22:      41556      43005   IO-APIC-fasteoi   HDA Intel
506:     110417          0   PCI-MSI-edge      eth0

which is another semantic change introduced by using MSI)

			Linus

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Linus Torvalds wrote:
> The spec is just so much toilet paper. The ONLY thing that matters is what 
> real hardware does. So please please please PLEASE don't start quoting 
> specs as a way to "prove your point". It is totally meaningless.

Amen.


> End result: I think that at least for 2.6.19, and at least for the HDA 
> sound driver, keeping it disabled by default is the right choice. We 
> should probably _also_ make "pci_msi_supported()" just return an error 
> (probably by just clearing "pci_msi_enable") for any non-intel host bridge 
> for now.

If you update, pci_msi_{enable,supported} then you can -- and should -- 
revert the HD-audio driver change.  Just reviewed the driver, and it 
properly checks all the return values from PCI MSI API functions.

(though, HD-audio shouldn't be using IRQF_DISABLED at all, and shouldn't 
be using IRQF_SHARED for PCI MSI interrupts)

Though maybe for 2.6.19 the current state of things is at least a stable 
state.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Takashi Iwai wrote:
> The snd-hda-intel driver has a test of MSI, but it seems not working
> on every machine.  It caused non-cared interrupts and the kernel
> disabled that irq.

Possibly the test was broken.  Did you have IRQF_DISABLED and 
IRQF_SHARED flags set?


>> Given current experience maybe white-lists are in fact the way
>> to go.
> 
> Could it be whitelisted in the PCI driver side?  I don't think it's
> good to have a huge white/blacklist in each device driver.

Certainly, we should not have such lists in the driver.  I don't think 
anybody wants that.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Jeff Garzik wrote:
> Takashi Iwai wrote:
>> The snd-hda-intel driver has a test of MSI, but it seems not working
>> on every machine.  It caused non-cared interrupts and the kernel
>> disabled that irq.
> 
> Possibly the test was broken.  Did you have IRQF_DISABLED and 
> IRQF_SHARED flags set?

And also call pci_intx() properly in the driver, which I noticed isn't 
being done.

(PCI MSI layer does it automatically for PCI-Express devices, only)

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Linus Torvalds wrote:
> And the thing is, once you do that, all the advantages of MSI totally go 
> away - both the "nice" ones and the "really good ones" (the latter being 
> the hopeful eventual removal of irq routing confusions). So if you do 
> that, the better solution is for the driver to say "I won't use MSI at 
> all".

I certainly agree with this.

The driver should either (a) know MSI works for the device [it calls 
pci_msi_enable()] or (b) never bothers with MSI.

Combine that with accurate pci_msi_enable() failure, and life is good.

No 'test if MSI works' crapola in drivers.  I've disliked such tests 
since the day MSI was introduced into Linux.


> It all boils down to the same thing: either we have to know that MSI works 
> (where "know" is obviously relative - it's not like you can avoid _all_ 
> bugs, but dammit, even a single report of "not working" means that there 
> are probably a ton of machines like that, and we did something wrong), or 
> we shouldn't use it. There is no middle ground. You can't really safely 
> "test" for it, and while you _can_ say "just do both", it doesn't really 
> help anything (and potentially exposes you to just more bugs: if enablign 
> MSI actually _does_ disable INTx, but then doesn't work, at a minimum you 
> end up with a device that doesn't work, even if the rest of the kernel 
> might be ok).

Agreed.


> And btw, I say this as a person whose new main machine used to have HDA 
> routed over MSI, and the decision to default to it off meant that it went 
> back to the regular INTx thing.

Yeah, but you don't care if that happens, so this is an ineffective 
'btw'  It's not like you went to sleep crying over the loss, like I did 
[just kidding!]


> (Btw, MSI interrupts also seem to not participate in CPU balancing:
> 
>  22:      41556      43005   IO-APIC-fasteoi   HDA Intel
> 506:     110417          0   PCI-MSI-edge      eth0
> 
> which is another semantic change introduced by using MSI)

No, that's most likely because ethernet is always intentionally locked 
to a single CPU by irqbalance.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Krzysztof Halasa wrote:
> Jeff Garzik <jeff@garzik.org> writes:
> 
>> So far, MSI history on x86 has always followed these rules:
>> * it works on Intel
>> * it doesn't work [well | at all] on AMD/NV
> 
> I don't know how does it look when it doesn't work etc. but certainly
> both NV Ethernet and HDA seem to work for me and:

Oh I certainly agree (and it appears that Roland agrees) that MSI works 
/somewhere/ on NV.  I give kudos to NV to working on forcedeth to make 
sure it works well with MSI.  But unfortunately NV was also in the bug 
report(s) linked.

Though OTOH, the driver wasn't calling pci_intx() nor setting irq flags 
correctly, so who knows.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Roland Dreier]

 > > Huh?  The device can't generate any legacy interrupts once MSI is
 > > enabled.  As the PCI spec says:
 > 
 > PLEASE.
 > 
 > The spec is just so much toilet paper. The ONLY thing that matters is what 
 > real hardware does. So please please please PLEASE don't start quoting 
 > specs as a way to "prove your point". It is totally meaningless.

Sorry, I think you misunderstood.  I've certainly seen my share of
crazy devices doing things like endian-reversing the MSI address and
sending interrupt messages off into random space.  The only thing I
was disagreeing with was that Jeff suggested it was a driver bug not
to do pci_intx(0) when enabling MSI.

I don't doubt that there are broken devices out there that do generate
wire interrupts even when MSI is enabled.  I just don't think we
should go around "fixing" working, correct drivers for devices that
_do_ follow the spec.

 - R.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Linus Torvalds]

On Wed, 15 Nov 2006, Jeff Garzik wrote:
> 
> > And btw, I say this as a person whose new main machine used to have HDA
> > routed over MSI, and the decision to default to it off meant that it went
> > back to the regular INTx thing.
> 
> Yeah, but you don't care if that happens, so this is an ineffective 'btw'
> It's not like you went to sleep crying over the loss, like I did [just
> kidding!]

Heh. I'm really hoping that nobody will cry themselves to sleep over MSI 
not being enabled..

> > (Btw, MSI interrupts also seem to not participate in CPU balancing:
> > 
> >  22:      41556      43005   IO-APIC-fasteoi   HDA Intel
> > 506:     110417          0   PCI-MSI-edge      eth0
> > 
> > which is another semantic change introduced by using MSI)
> 
> No, that's most likely because ethernet is always intentionally locked to a
> single CPU by irqbalance.

Nope, the same thing happened to "HDA Intel" when it was an MSI interrupt 
(ie before I applied the change that made it not use MSI by default).

So I think it's either: (a) irqbalance doesn't balance MSI interrupts at 
all or (b) the MSI interrupt code doesn't honor balancing requests even if 
it does.

I didn't look any closer. It's not like it's a huge problem for me (or 
likely for anybody else), but it was interesting to see another 
"unintended consequence" of the "use MSI or not" choice.

The suspend problem reported by Stephen is another such thing - where MSI 
itself wasn't a problem, but stupid (probably broken) firmware code at 
wakeup broke it by an unforseen interaction. Again, that is probably 
related to the fact that nobody has ever really tested it (ie firmware 
"engineers" obviously didn't actually ever test anything with MSI enabled 
and in use, and there really is no excuse for firmware messing with the 
MSI setting - other than the usual "firmware is inevitably buggy" thing).

			Linus

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Takashi Iwai]

At Wed, 15 Nov 2006 13:32:02 -0500,
Jeff Garzik wrote:
> 
> Takashi Iwai wrote:
> > The snd-hda-intel driver has a test of MSI, but it seems not working
> > on every machine.  It caused non-cared interrupts and the kernel
> > disabled that irq.
> 
> Possibly the test was broken.  Did you have IRQF_DISABLED and 
> IRQF_SHARED flags set?

I think IRQF_* is irrelevant there.
It looks like that the hardware issues INTX regardless whether MSI is
enabled or not.  Thus it ends up with unexpected irqs which are never
caught by the driver (the driver expects a different irq from MSI),
and eventually the kernel kills this irq.

Possibly calling pci_intx() as you suggested might help to avoid this
situation.  Can anyone test the patch below?


Takashi

---
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index e35cfd3..bdb92b3 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -544,6 +544,7 @@ static unsigned int azx_rirb_get_respons
 		free_irq(chip->irq, chip);
 		chip->irq = -1;
 		pci_disable_msi(chip->pci);
+		pci_intx(chip->pci, 1);
 		chip->msi = 0;
 		if (azx_acquire_irq(chip, 1) < 0)
 			return -1;
@@ -830,14 +831,15 @@ static irqreturn_t azx_interrupt(int irq
 {
 	struct azx *chip = dev_id;
 	struct azx_dev *azx_dev;
+	unsigned long flags;
 	u32 status;
 	int i;
 
-	spin_lock(&chip->reg_lock);
+	spin_lock_irqsave(&chip->reg_lock, flags);
 
 	status = azx_readl(chip, INTSTS);
 	if (status == 0) {
-		spin_unlock(&chip->reg_lock);
+		spin_unlock_irqrestore(&chip->reg_lock, flags);
 		return IRQ_NONE;
 	}
 	
@@ -867,7 +869,7 @@ static irqreturn_t azx_interrupt(int irq
 	if (azx_readb(chip, STATESTS) & 0x04)
 		azx_writeb(chip, STATESTS, 0x04);
 #endif
-	spin_unlock(&chip->reg_lock);
+	spin_unlock_irqrestore(&chip->reg_lock, flags);
 	
 	return IRQ_HANDLED;
 }
@@ -1380,7 +1382,8 @@ static int __devinit azx_init_stream(str
 
 static int azx_acquire_irq(struct azx *chip, int do_disconnect)
 {
-	if (request_irq(chip->pci->irq, azx_interrupt, IRQF_DISABLED|IRQF_SHARED,
+	if (request_irq(chip->pci->irq, azx_interrupt,
+			chip->msi ? 0 : IRQF_SHARED,
 			"HDA Intel", chip)) {
 		printk(KERN_ERR "hda-intel: unable to grab IRQ %d, "
 		       "disabling device\n", chip->pci->irq);
@@ -1435,9 +1438,12 @@ static int azx_resume(struct pci_dev *pc
 		return -EIO;
 	}
 	pci_set_master(pci);
-	if (chip->msi)
+	if (chip->msi) {
 		if (pci_enable_msi(pci) < 0)
 			chip->msi = 0;
+		else
+			pci_intx(pci, 0);
+	}
 	if (azx_acquire_irq(chip, 1) < 0)
 		return -EIO;
 	azx_init_chip(chip);
@@ -1561,9 +1567,12 @@ static int __devinit azx_create(struct s
 		goto errout;
 	}
 
-	if (chip->msi)
+	if (chip->msi) {
 		if (pci_enable_msi(pci) < 0)
 			chip->msi = 0;
+		else
+			pci_intx(pci, 0);
+	}
 
 	if (azx_acquire_irq(chip, 0) < 0) {
 		err = -EBUSY;

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Arjan van de Ven]

On Wed, 2006-11-15 at 10:51 -0800, Linus Torvalds wrote:
> So I think it's either: (a) irqbalance doesn't balance MSI interrupts at 
> all or (b) the MSI interrupt code doesn't honor balancing requests even if 
> it does.

it's (A) right now for sure for irqbalanced at least.

> The suspend problem reported by Stephen is another such thing - where MSI 
> itself wasn't a problem, but stupid (probably broken) firmware code at 
> wakeup broke it by an unforseen interaction. Again, that is probably 
> related to the fact that nobody has ever really tested it (ie firmware 
> "engineers" obviously didn't actually ever test anything with MSI enabled 
> and in use, and there really is no excuse for firmware messing with the 
> MSI setting - other than the usual "firmware is inevitably buggy" thing).

ok so maybe this should be in the linux firmware kit.. it has
suspend/resume tests after all ;)

if there is a "do this then this to reproduce" scenario it's even likely
it's trivial to put into a testcase...

> 
> 			Linus
-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Linus Torvalds]

On Wed, 15 Nov 2006, Jeff Garzik wrote:
>
> Though OTOH, the driver wasn't calling pci_intx() nor setting irq flags
> correctly, so who knows.

The thing is, if the HDA driver happened to be alone on its legacy INTx 
vector, and it is now empty, then the kernel won't ever care about the 
fact that INTx may be still being generated. The legacy vector won't be 
enabled unless somebody else is on that vector.

So yes, this NV HDA sound breakage could easily be because of the 
interrupt being sent both over legacy INTx _and_ MSI. It would only break 
for people who happened to have another device sharing the INTx pin (like 
the report that caused us to disable it by default).

However, I really think that this should be a generic PCI layer thing. If 
some device asks for MSI interrupts, the PCI layer should try to turn off 
a INTx routing on its own. Asking drivers to do both is just silly, 
especially since driver writers really shouldn't be expected to know about 
all these issues (sure, the best ones do, but a lot of driver writers will 
just say "it works for me").

So I don't think the HDA driver should need disable INTx on its own 
explicitly.

If somebody were to write up such a patch and send it to Olivier (and 
others - I think there was another report of this) for testing (he'd now 
need to ask for msi irqs explicitly), that might be interesting. Hint, 
hint.

(See the original http://lkml.org/lkml/2006/11/8/98 report for one broken 
case, although it does seem different: we literally have a

	hda_intel: No response from codec, disabling MSI...

indicating that the MSI case simply didn't work at _all_ rather than 
duplicating interrupts).

		Linus

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Stephen Hemminger]

On Tue, 14 Nov 2006 23:24:04 -0500
Jeff Garzik <jeff@garzik.org> wrote:

> David Miller wrote:
> > Is this absolutely true?  I've never been sure about this point, and I
> > was rather convinced after reading various documents that once you
> > program up the MSI registers to start generating MSI this implicitly
> > disabled INTX and this was even in the PCI specification.
> > 
> > It would be great to get a definitive answer on this.
> > 
> > If it is mandatory, perhaps the driver shouldn't be doing it and
> > rather the PCI layer MSI enabling should.

pci_enable_msi() calls msi_capability_init() and that disables intx
already.


> 
> I can't answer for the spec, but at least two independent device vendors 
> recommended to write an MSI driver that way (disable intx, enable msi).
> 
> Completely independent of MSI though, a PCI 2.2 compliant driver should 
> be nice and disable intx on exit, just to avoid any potential interrupt 
> hassles after driver unload.  And of course be aware that it might need 
> to enable intx upon entry.
> 
> 	Jeff

The driver shouldn't deal with this, pci_disable_msi() does.

-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Takashi Iwai wrote:
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index e35cfd3..bdb92b3 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -544,6 +544,7 @@ static unsigned int azx_rirb_get_respons
>  		free_irq(chip->irq, chip);
>  		chip->irq = -1;
>  		pci_disable_msi(chip->pci);
> +		pci_intx(chip->pci, 1);
>  		chip->msi = 0;
>  		if (azx_acquire_irq(chip, 1) < 0)
>  			return -1;
> @@ -830,14 +831,15 @@ static irqreturn_t azx_interrupt(int irq
>  {
>  	struct azx *chip = dev_id;
>  	struct azx_dev *azx_dev;
> +	unsigned long flags;
>  	u32 status;
>  	int i;
>  
> -	spin_lock(&chip->reg_lock);
> +	spin_lock_irqsave(&chip->reg_lock, flags);
>  
>  	status = azx_readl(chip, INTSTS);
>  	if (status == 0) {
> -		spin_unlock(&chip->reg_lock);
> +		spin_unlock_irqrestore(&chip->reg_lock, flags);
>  		return IRQ_NONE;
>  	}
>  	
> @@ -867,7 +869,7 @@ static irqreturn_t azx_interrupt(int irq
>  	if (azx_readb(chip, STATESTS) & 0x04)
>  		azx_writeb(chip, STATESTS, 0x04);
>  #endif
> -	spin_unlock(&chip->reg_lock);
> +	spin_unlock_irqrestore(&chip->reg_lock, flags);

ACK the pci_intx() calls, NAK the obviously overweight spinlock changes. 
  The spinlock changes are completely unnecessary.  Just look at any 
other (non-ALSA) PCI driver.  Existing "spin_lock()" is fine for both 
PCI shared irq handlers and MSI irq handlers.

It sounds like you are trying to work around a reentrancy problem that 
does not exist.

Only weird drivers like ps2kbd/mouse or IDE need spin_lock_irqsave(), 
where separate interrupt sources call the same function.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Stephen Hemminger]

On Wed, 15 Nov 2006 11:31:04 +0100
Takashi Iwai <tiwai@suse.de> wrote:

> At Tue, 14 Nov 2006 19:21:17 -0800 (PST),
> David Miller wrote:
> > 
> > From: Linus Torvalds <torvalds@osdl.org>
> > Date: Tue, 14 Nov 2006 19:10:42 -0800 (PST)
> > 
> > > Yours was still an example of "nice". And it had absolutely nothing
> > > to do with the _PROBLEM_.
> > 
> > Understood.
> > 
> > BTW, some drivers have taken the approch to add MSI self-tests
> > inside of the driver to ensure correct option of MSI on a given
> > machine.  There's a lot of resistence to that, the reasons for
> > which I grok fully, but I'm not sure other suggestions such as
> > black lists are any better.
> 
> The snd-hda-intel driver has a test of MSI, but it seems not working
> on every machine.  It caused non-cared interrupts and the kernel
> disabled that irq.
> 
> > Given current experience maybe white-lists are in fact the way
> > to go.
> 
> Could it be whitelisted in the PCI driver side?  I don't think it's
> good to have a huge white/blacklist in each device driver.
> 

A whitelist is an awkward solution, the problem is the number of
chipsets available with MSI will continue to grow. And the assumption
is that after Microsoft OS supports MSI, that newer chipsets will work.

So by having a whitelist, you force a growing whitelist (in the kernel)
to know about all the possible chipsets.  Since non-whitelisted systems
will end up using INTX and working fine, most users will never try MSI
and the whitelist will end up stale.

A better solution is to have more robust IRQ management that can
deal with misrouted IRQ's and try and recover correctly. How hard would
it be to:
	* remember original IRQ before MSI was enabled
	* make sure all MSI irq's are not flagged SHARED
	* in case of bogus IRQ walk the list and try and correct
	  the problem by reverting to INTX mode.
	* add interface?
		pci_request_irq_msi(pdev, regular_irq_handler, msi_irq_handler, flags, name, context)

All this should be done by the MSI layer, not the device drivers.

-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Linus Torvalds wrote:
> However, I really think that this should be a generic PCI layer thing. If 
> some device asks for MSI interrupts, the PCI layer should try to turn off 
> a INTx routing on its own. Asking drivers to do both is just silly, 
> especially since driver writers really shouldn't be expected to know about 
> all these issues (sure, the best ones do, but a lot of driver writers will 
> just say "it works for me").
> 
> So I don't think the HDA driver should need disable INTx on its own 
> explicitly.

Your thinking is correct, but there is one hitch.

As Roland noted, PCI layer /already/ does this for PCI-Express devices.

The reason we cannot do this in the generic layer for non-PCI-Ex is only 
the driver knows whether that PCI 2.2 bit was actually implemented in 
the device or mapped to some other weird behavior we don't want to 
touch.  DISABLE-INTX is a new bit not present in PCI 2.1 (alas!!).

pci_intx() was my five minute solution to this problem, and it got moved 
outside of libata as soon as somebody needed the same thing :)

Maybe a better solution is letting the driver say "pci_dev->intx_ok = 1" 
right before it calls pci_enable_device().

And if we do this, we can follow through on another suggestion I made: 
disabling INTx on driver exit, to help eliminate any possibility of 
screaming interrupts after driver unload.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Stephen Hemminger wrote:
> On Tue, 14 Nov 2006 23:24:04 -0500
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> David Miller wrote:
>>> Is this absolutely true?  I've never been sure about this point, and I
>>> was rather convinced after reading various documents that once you
>>> program up the MSI registers to start generating MSI this implicitly
>>> disabled INTX and this was even in the PCI specification.
>>>
>>> It would be great to get a definitive answer on this.
>>>
>>> If it is mandatory, perhaps the driver shouldn't be doing it and
>>> rather the PCI layer MSI enabling should.
> 
> pci_enable_msi() calls msi_capability_init() and that disables intx
> already.
[...]
> The driver shouldn't deal with this, pci_disable_msi() does.

Explicit code reference please?

AFAICS the PCI layer only touched INTx bit for PCI-Express devices.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Stephen Clark]

Linus Torvalds wrote:

>On Wed, 15 Nov 2006, Arjan van de Ven wrote:
>  
>
>>well we could cheat some. And have the generic code for this just
>>register the irq handler for both somehow.
>>    
>>
>
>Well, not generic code. It would have to be the driver itself that does 
>it, since generic code doesn't even know (at irq request time - and when 
>they are generated - it just gets the irq number).
>
>And the thing is, once you do that, all the advantages of MSI totally go 
>away - both the "nice" ones and the "really good ones" (the latter being 
>the hopeful eventual removal of irq routing confusions). So if you do 
>that, the better solution is for the driver to say "I won't use MSI at 
>all".
>
>Really.
>
>It all boils down to the same thing: either we have to know that MSI works 
>(where "know" is obviously relative - it's not like you can avoid _all_ 
>bugs, but dammit, even a single report of "not working" means that there 
>are probably a ton of machines like that, and we did something wrong), or 
>we shouldn't use it. There is no middle ground. You can't really safely 
>"test" for it, and while you _can_ say "just do both", it doesn't really 
>help anything (and potentially exposes you to just more bugs: if enablign 
>MSI actually _does_ disable INTx, but then doesn't work, at a minimum you 
>end up with a device that doesn't work, even if the rest of the kernel 
>might be ok).
>
>And btw, I say this as a person whose new main machine used to have HDA 
>routed over MSI, and the decision to default to it off meant that it went 
>back to the regular INTx thing.
>
>(Btw, MSI interrupts also seem to not participate in CPU balancing:
>
> 22:      41556      43005   IO-APIC-fasteoi   HDA Intel
>506:     110417          0   PCI-MSI-edge      eth0
>
>  
>
mine do:$ cat /proc/interrupts
           CPU0       CPU1
  0:    7986702    7971263   IO-APIC-edge      timer
...
 20:      90626      95073   IO-APIC-fasteoi   uhci_hcd:usb2
220:       1722       1415   PCI-MSI-edge      HDA Intel
NMI:          0          0
LOC:   15957069   15957071
ERR:          0
MIS:          0

Also, I find it disturbing that we are forcing users to have know about 
all these
magic options that have to be put on the kernel boot line. My hard drive 
on my
new laptop would only run at 1.2mbs until I found out I had to use 
combined_mode=libata
and build a new ramdisk that included ata_piix.

My $.02
Steve

>which is another semantic change introduced by using MSI)
>
>			Linus
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/
>
>  
>


-- 

"They that give up essential liberty to obtain temporary safety, 
deserve neither liberty nor safety."  (Ben Franklin)

"The course of history shows that as a government grows, liberty 
decreases."  (Thomas Jefferson)

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Linus Torvalds]

On Wed, 15 Nov 2006, Jeff Garzik wrote:
> 
> The reason we cannot do this in the generic layer for non-PCI-Ex is only the
> driver knows whether that PCI 2.2 bit was actually implemented in the device
> or mapped to some other weird behavior we don't want to touch.  DISABLE-INTX
> is a new bit not present in PCI 2.1 (alas!!).

Ok, I would have a few suggestions, then:

 - devices that don't support INTx disable should basically be considered 
   to not support MSI at all (ie a driver simply shouldn't even try to 
   enable MSI, since enabling MSI includes the implicit DISABLE-INTX)

   This is likely ok, since MSI wasn't in PCI 2.1 _either_ afaik. So if 
   your hardware has MSI, it probably _does_ have DISABLE-INTX (or at 
   least that bit doesn't do anything bad, which is the other case)

 - add a flag to "pci_enable_msi()". There really aren't that many users, 
   and they basically _all_ want this functionality. Making them call 
   another function is just a recipe for disaster, since somebody will 
   forget. Having to just say "do I support INTx disable" is much better, 
   since it makes the driver writer aware of having to _explicitly_ make 
   that choice.

> Maybe a better solution is letting the driver say "pci_dev->intx_ok = 1" right
> before it calls pci_enable_device().

I hate that, for exactly the same reason I hate "pci_intx()". It just 
means that most drivers won't do it, because it's not even part of the 
normal sequence, and most people don't care. So again, it would actually 
be better in that case to just add a "flags" field to pci_enable_device(), 
although that's a _hell_ of a lot more painful than it would be to do the 
same to "pci_enable_msi()".

> And if we do this, we can follow through on another suggestion I made:
> disabling INTx on driver exit, to help eliminate any possibility of screaming
> interrupts after driver unload.

The thing is, I think that's a bad idea for the same reason it's a bad 
idea to disable the BAR's (which we tried and then reverted). It's just 
going to cause problems for soft rebooting etc with firmware that doesn't 
expect it.

A driver should obviously quiesce the device on shutdown, and if it leaves 
a device in a state where it may still generate interrupts, that's a 
_bug_, so disabling INTx is just papering over a much more serious issue.

		Linus

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Stephen Clark wrote:
> Also, I find it disturbing that we are forcing users to have know about 
> all these
> magic options that have to be put on the kernel boot line. My hard drive 
> on my
> new laptop would only run at 1.2mbs until I found out I had to use 
> combined_mode=libata
> and build a new ramdisk that included ata_piix.

That's what happens when two drivers want to drive the same hardware. 
The "slow and safe" default is the only proven-stable option, with the 
proven-stable PATA driver.  The other two options (drivers/ide for 
PATA+SATA -> leads to SATA locksup) and (libata for PATA -> ok but 
breaks existing configs, and less field time) are considered less safe.

Combined mode is ugly no matter how you look at it.  Just turn it off in 
BIOS (or pressure system vendor for this ability if BIOS lacks it, e.g. 
some Dell servers)

And throw some annoyance at Intel for creating such a headache.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Stephen Hemminger]

On Wed, 15 Nov 2006 14:23:20 -0500
Jeff Garzik <jeff@garzik.org> wrote:

> Stephen Hemminger wrote:
> > On Tue, 14 Nov 2006 23:24:04 -0500
> > Jeff Garzik <jeff@garzik.org> wrote:
> > 
> >> David Miller wrote:
> >>> Is this absolutely true?  I've never been sure about this point, and I
> >>> was rather convinced after reading various documents that once you
> >>> program up the MSI registers to start generating MSI this implicitly
> >>> disabled INTX and this was even in the PCI specification.
> >>>
> >>> It would be great to get a definitive answer on this.
> >>>
> >>> If it is mandatory, perhaps the driver shouldn't be doing it and
> >>> rather the PCI layer MSI enabling should.
> > 
> > pci_enable_msi() calls msi_capability_init() and that disables intx
> > already.
> [...]
> > The driver shouldn't deal with this, pci_disable_msi() does.
> 
> Explicit code reference please?
> 
> AFAICS the PCI layer only touched INTx bit for PCI-Express devices.

Yeah, why is that? Shouldn't it always be adjusting intx. 
Are there are any MSI capable devices on non-PCI express?
Sorry, don't have PCI spec (costs real $$) to check.

--- 2.6.19-rc5.orig/drivers/pci/msi.c	2006-11-15 11:46:23.000000000 -0800
+++ 2.6.19-rc5/drivers/pci/msi.c	2006-11-15 11:46:55.000000000 -0800
@@ -255,10 +255,7 @@
 		pci_write_config_word(dev, msi_control_reg(pos), control);
 		dev->msix_enabled = 1;
 	}
-    	if (pci_find_capability(dev, PCI_CAP_ID_EXP)) {
-		/* PCI Express Endpoint device detected */
-		pci_intx(dev, 0);  /* disable intx */
-	}
+	pci_intx(dev, 0);  /* disable intx */
 }
 
 void disable_msi_mode(struct pci_dev *dev, int pos, int type)
@@ -276,10 +273,8 @@
 		pci_write_config_word(dev, msi_control_reg(pos), control);
 		dev->msix_enabled = 0;
 	}
-    	if (pci_find_capability(dev, PCI_CAP_ID_EXP)) {
-		/* PCI Express Endpoint device detected */
-		pci_intx(dev, 1);  /* enable intx */
-	}
+
+	pci_intx(dev, 1);  /* re-enable intx */
 }
 
 static int msi_lookup_irq(struct pci_dev *dev, int type)

		


-- 
Stephen Hemminger <shemminger@osdl.org>

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Mws]

[-- Attachment #1: Type: text/plain, Size: 4061 bytes --]

On Wednesday 15 November 2006 20:35, Linus Torvalds wrote:
> 
> On Wed, 15 Nov 2006, Jeff Garzik wrote:
> > 
> > The reason we cannot do this in the generic layer for non-PCI-Ex is only the
> > driver knows whether that PCI 2.2 bit was actually implemented in the device
> > or mapped to some other weird behavior we don't want to touch.  DISABLE-INTX
> > is a new bit not present in PCI 2.1 (alas!!).
> 
> Ok, I would have a few suggestions, then:
> 
>  - devices that don't support INTx disable should basically be considered 
>    to not support MSI at all (ie a driver simply shouldn't even try to 
>    enable MSI, since enabling MSI includes the implicit DISABLE-INTX)
> 
>    This is likely ok, since MSI wasn't in PCI 2.1 _either_ afaik. So if 
>    your hardware has MSI, it probably _does_ have DISABLE-INTX (or at 
>    least that bit doesn't do anything bad, which is the other case)
> 
>  - add a flag to "pci_enable_msi()". There really aren't that many users, 
>    and they basically _all_ want this functionality. Making them call 
>    another function is just a recipe for disaster, since somebody will 
>    forget. Having to just say "do I support INTx disable" is much better, 
>    since it makes the driver writer aware of having to _explicitly_ make 
>    that choice.
> 
> > Maybe a better solution is letting the driver say "pci_dev->intx_ok = 1" right
> > before it calls pci_enable_device().
> 
> I hate that, for exactly the same reason I hate "pci_intx()". It just 
> means that most drivers won't do it, because it's not even part of the 
> normal sequence, and most people don't care. So again, it would actually 
> be better in that case to just add a "flags" field to pci_enable_device(), 
> although that's a _hell_ of a lot more painful than it would be to do the 
> same to "pci_enable_msi()".
> 
> > And if we do this, we can follow through on another suggestion I made:
> > disabling INTx on driver exit, to help eliminate any possibility of screaming
> > interrupts after driver unload.
> 
> The thing is, I think that's a bad idea for the same reason it's a bad 
> idea to disable the BAR's (which we tried and then reverted). It's just 
> going to cause problems for soft rebooting etc with firmware that doesn't 
> expect it.
> 
> A driver should obviously quiesce the device on shutdown, and if it leaves 
> a device in a state where it may still generate interrupts, that's a 
> _bug_, so disabling INTx is just papering over a much more serious issue.
> 
> 		Linus
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
i just recognised this thread, and i found a solution for some weird stuff
also reported days ago on alsa-users list.

short summary for your information:

asus m2n32 ws professional 
amd x2 5000+
everything compiled as 64 bit.

i recognised strange behaviour from 2.6.19-rc1 on.
sound got crappy and if i tried to use snd_hda_intel as modules
rmmod & afterwards modprobe froze the machine completely.
even the kernel sys req key combination didn't work anymore.

after some small discussions on alsa-user ml i recognised this
thread today. 
i thought my problem could also exist on this msi stuff.
i disabled msi in kernel config, reboot, and, after starting x & kde
i got immediately a freeze.
last and maybe important last try has been to
enable msi support _but_ boot kernel with cmdline pci=nomsi
this finally did work out. i got a working sound environment again.

if i should supply more information on that, please contact me.

i find it a bit abnormal that the disabling msi in kernel config behaviour
is different from kernel cmdline pci=nomsi option.

best regards
marcel

ps. i don't want to break this thread up into something different, but maybe my 
     report works out as usefull.



[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Stephen Clark]

Jeff Garzik wrote:

>Stephen Clark wrote:
>  
>
>>Also, I find it disturbing that we are forcing users to have know about 
>>all these
>>magic options that have to be put on the kernel boot line. My hard drive 
>>on my
>>new laptop would only run at 1.2mbs until I found out I had to use 
>>combined_mode=libata
>>and build a new ramdisk that included ata_piix.
>>    
>>
>
>That's what happens when two drivers want to drive the same hardware. 
>The "slow and safe" default is the only proven-stable option, with the 
>proven-stable PATA driver.  The other two options (drivers/ide for 
>PATA+SATA -> leads to SATA locksup) and (libata for PATA -> ok but 
>breaks existing configs, and less field time) are considered less safe.
>
>  
>
The problem with this approach is the average user will just think linux 
sucks - it is so
slow, they won't know how to investigate or trouble shoot the problem, 
and just go back to winblows.
I've got an ich7 chipset that supports sata and ide. My laptop has only 
ide devices so why
is there a conflict that has to be resolved by combined mode? Why can't 
the sata driver
be smart enough to know there are no sata devices for it to handle?

>Combined mode is ugly no matter how you look at it.  Just turn it off in 
>BIOS (or pressure system vendor for this ability if BIOS lacks it, e.g. 
>some Dell servers)
>
>And throw some annoyance at Intel for creating such a headache.
>
>	Jeff
>
>
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/
>
>  
>


-- 

"They that give up essential liberty to obtain temporary safety, 
deserve neither liberty nor safety."  (Ben Franklin)

"The course of history shows that as a government grows, liberty 
decreases."  (Thomas Jefferson)

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Linus Torvalds]

On Wed, 15 Nov 2006, Mws wrote:
> 
> after some small discussions on alsa-user ml i recognised this
> thread today. 
> i thought my problem could also exist on this msi stuff.
> i disabled msi in kernel config, reboot, and, after starting x & kde
> i got immediately a freeze.
> last and maybe important last try has been to
> enable msi support _but_ boot kernel with cmdline pci=nomsi
> this finally did work out. i got a working sound environment again.

I expect that you have the exact same issue as Olivier: the "hang" is 
probably because you started using the sound device (beeping on the 
console is handled by the old built-in speaker, but in X a single beep 
tends to be due to sound device drivers), and because of sound irq 
misrouting you had some other device (like your harddisk) that got their 
irq disabled due to the "nobody cared" issue.

> i find it a bit abnormal that the disabling msi in kernel config behaviour
> is different from kernel cmdline pci=nomsi option.

Now, that does actually worry me. It _should_ have worked with CONFIG_MSI 
disabled. I wonder if some of the MSI workarounds actually broke the HDA 
driver subtly.

Anyway, it would be a good idea to test the current -git tree if you can, 
both with CONFIG_MSI and without (and _without_ any "pci=nomsi" kernel 
command line). It should hopefully work, exactly because the HDA driver 
now shouldn't even try to do any MSI stuff by default.

Knock wood.

		Linus

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Olivier Nicolas]

Linus Torvalds wrote:
> 
> On Wed, 15 Nov 2006, Mws wrote:
>> after some small discussions on alsa-user ml i recognised this
>> thread today. 
>> i thought my problem could also exist on this msi stuff.
>> i disabled msi in kernel config, reboot, and, after starting x & kde
>> i got immediately a freeze.
>> last and maybe important last try has been to
>> enable msi support _but_ boot kernel with cmdline pci=nomsi
>> this finally did work out. i got a working sound environment again.

Usually, the shared IRQ is disabled but it has happened that the system 
boots without disabling the IRQ. In that case, the sound is distorted 
and the distortion increase when I use the device that shares the IRQ.
(see the boot messages: 
http://olivn.trollprod.org/irq_not_disabled_sound_distortion.dmesg )


I can boot the system with pci=nomsi without any problem.


Yinghai Lu who send me some patches to trace irq assignment
said:
"Then it is not caused by transition from MSI to IOAPIC.
Weird,
MSI is enabled, but chip still send int to ioapic.
YH"



Olivier

> 
> I expect that you have the exact same issue as Olivier: the "hang" is 
> probably because you started using the sound device (beeping on the 
> console is handled by the old built-in speaker, but in X a single beep 
> tends to be due to sound device drivers), and because of sound irq 
> misrouting you had some other device (like your harddisk) that got their 
> irq disabled due to the "nobody cared" issue.
> 
>> i find it a bit abnormal that the disabling msi in kernel config behaviour
>> is different from kernel cmdline pci=nomsi option.
> 
> Now, that does actually worry me. It _should_ have worked with CONFIG_MSI 
> disabled. I wonder if some of the MSI workarounds actually broke the HDA 
> driver subtly.
> 
> Anyway, it would be a good idea to test the current -git tree if you can, 
> both with CONFIG_MSI and without (and _without_ any "pci=nomsi" kernel 
> command line). It should hopefully work, exactly because the HDA driver 
> now shouldn't even try to do any MSI stuff by default.
> 
> Knock wood.
> 
> 		Linus

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Mws]

On Wednesday 15 November 2006 21:14, Linus Torvalds wrote:
> 
> On Wed, 15 Nov 2006, Mws wrote:
> > 
> > after some small discussions on alsa-user ml i recognised this
> > thread today. 
> > i thought my problem could also exist on this msi stuff.
> > i disabled msi in kernel config, reboot, and, after starting x & kde
> > i got immediately a freeze.
> > last and maybe important last try has been to
> > enable msi support _but_ boot kernel with cmdline pci=nomsi
> > this finally did work out. i got a working sound environment again.
> 
> I expect that you have the exact same issue as Olivier: the "hang" is 
> probably because you started using the sound device (beeping on the 
> console is handled by the old built-in speaker, but in X a single beep 
> tends to be due to sound device drivers), and because of sound irq 
> misrouting you had some other device (like your harddisk) that got their 
> irq disabled due to the "nobody cared" issue.
> 
> > i find it a bit abnormal that the disabling msi in kernel config behaviour
> > is different from kernel cmdline pci=nomsi option.
> 
> Now, that does actually worry me. It _should_ have worked with CONFIG_MSI 
> disabled. I wonder if some of the MSI workarounds actually broke the HDA 
> driver subtly.
> 
> Anyway, it would be a good idea to test the current -git tree if you can, 
> both with CONFIG_MSI and without (and _without_ any "pci=nomsi" kernel 
> command line). It should hopefully work, exactly because the HDA driver 
> now shouldn't even try to do any MSI stuff by default.
> 
> Knock wood.
> 
> 		Linus
hi,

linus, just cloned current git, but will test it tomorrow morning.

will inform you on the results.

thanks 
marcel

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Roland Dreier]

 > Are there are any MSI capable devices on non-PCI express?

Yes, e.g. the mthca driver handles PCI-X adapters that do MSI-X.

 - R.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Roland Dreier]

 > A whitelist is an awkward solution, the problem is the number of
 > chipsets available with MSI will continue to grow. And the assumption
 > is that after Microsoft OS supports MSI, that newer chipsets will work.

Maybe a whitelist for older systems and then enable everything after a
DMI cutoff date by default?  Doesn't work on non-PC stuff though...

 - R.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Roland Dreier]

 > Perhaps with special user space quirks, but not in mainline kernel because it's
 > not force enabled (see erratum #78).

I just looked at the revision guide again, and I don't see why erratum
#78 would matter -- that seems to be saying that the bridge itself
doesn't have an MSI capability so it can't generate MSI interrupts
itself.  But why does that matter for devices below the bridge?

Anyway I have some HP DL145 G2 servers with 8132 bridges in them.
I'll plug an MSI-capable PCI-X card in and see what happens.  So far
I've only used PCIe cards in my systems (but MSI-X works fine there,
with nVidia host bridges).

 - R.

RE: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Lu, Yinghai]

I just tried latest git and one MCP55+IO55 and had_intel, ALC883 codec
MB.

It works well with routeirq or not.

The BIOS disable the MSI cap.

YH

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Benjamin Herrenschmidt]

> HOWEVER - that's only true on systems with no other PCI bridges. Even if 
> you have an Intel NB/SB, what about other bridges in that same system, and 
> the devices behind them? 

Well... MSIs are just normal stores as far as PCI is concerned. Thus, if
you have P2P bridges in your system and they can't get store ordering
right, I think you have a much bigger problem than getting MSIs wrong.

I think the problem is mostly with host bridges doing crappy stuffs like
decoding MSIs up front and sending them to the CPU out of order or
HT<->PCI (or some home made backbone <-> PCI) bridges where MSIs gets
turned into something else with potentially ordering breakage at the
transformation point or the upstream bus.

> Now, I think that a MSI thing should look like a PCI write to a magic 
> address (I'm really not very up on it, so correct me if I'm wrong), and 
> thus maybe bridges are bound to get it right, and the only thing we really 
> need to worry about is the host bridge. 

Host bridges or something <-> PCI (something = HT, or some of the fancy
NB<->SB links other vendors use).

> Maybe. In that case, it might be sensible to have a host-bridge white-table,
> and if we know all Intel 
> bridges that claim to support MSI do so correctly, then maybe we can just 
> say "ok, always enable it for Intel host bridges".
> 
> But right now I'm not convinced we really know what all goes wrong. Maybe 
> it's just broken NVidia and AMD bridges. But maybe it's also individual 
> devices that continue to (for example) raise _both_ the legacy IRQ line 
> _and_ send an MSI request.

Yeah, well, the later can be solved in software by masking the LSI when
MSI is enabled for a device.

Cheers,
Ben.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Benjamin Herrenschmidt]

> Is this absolutely true?  I've never been sure about this point, and I
> was rather convinced after reading various documents that once you
> program up the MSI registers to start generating MSI this implicitly
> disabled INTX and this was even in the PCI specification.

I think it is in the spec, that doesn't mean all device vendors get it
right. I have a vendor spec under my eyes at the moment (sorry, can't
say what it is) which has exactly this bug.

Ben.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Benjamin Herrenschmidt]

On Tue, 2006-11-14 at 20:28 -0800, David Miller wrote:
> From: Jeff Garzik <jeff@garzik.org>
> Date: Tue, 14 Nov 2006 23:24:04 -0500
> 
> > I can't answer for the spec, but at least two independent device vendors 
> > recommended to write an MSI driver that way (disable intx, enable msi).
> 
> Ok.
> 
> > Completely independent of MSI though, a PCI 2.2 compliant driver should 
> > be nice and disable intx on exit, just to avoid any potential interrupt 
> > hassles after driver unload.  And of course be aware that it might need 
> > to enable intx upon entry.
> 
> This also sounds like it should occur in the generic PCI layer when a
> PCI driver is unregistered.

Is this disable_intx() thingy something x86 specific ? I mean, you can't
just call disable_irq() for LSIs since you can be sharing it. If you
aren't sharing, free_irq() will mask for you.

Ben.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Benjamin Herrenschmidt]

> Is this disable_intx() thingy something x86 specific ? I mean, you can't
> just call disable_irq() for LSIs since you can be sharing it. If you
> aren't sharing, free_irq() will mask for you.

Oops .. my bad, forgot about that new command register bit. When was it
added ? pci 2.3 ?

Also, it should probably be done by pci_disable_device() ... in fact, to
be safe, the driver should disable that before free_irq().

Ben.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Benjamin Herrenschmidt]

On Tue, 2006-11-14 at 20:30 -0800, Roland Dreier wrote:
>  > That reminds me of a potential driver bug -- MSI-aware drivers need to
>  > call pci_intx(pdev,0) to turn off the legacy PCI interrupt, before
>  > enabling MSI interrupts.
> 
> Huh?  The device can't generate any legacy interrupts once MSI is
> enabled.  As the PCI spec says:

No, the device is _not_supposed_to_ :-)

Cheers,
Ben.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Benjamin Herrenschmidt wrote:
> On Tue, 2006-11-14 at 20:28 -0800, David Miller wrote:
>> From: Jeff Garzik <jeff@garzik.org>
>> Date: Tue, 14 Nov 2006 23:24:04 -0500
>>
>>> I can't answer for the spec, but at least two independent device vendors 
>>> recommended to write an MSI driver that way (disable intx, enable msi).
>> Ok.
>>
>>> Completely independent of MSI though, a PCI 2.2 compliant driver should 
>>> be nice and disable intx on exit, just to avoid any potential interrupt 
>>> hassles after driver unload.  And of course be aware that it might need 
>>> to enable intx upon entry.
>> This also sounds like it should occur in the generic PCI layer when a
>> PCI driver is unregistered.
> 
> Is this disable_intx() thingy something x86 specific ? I mean, you can't
> just call disable_irq() for LSIs since you can be sharing it. If you
> aren't sharing, free_irq() will mask for you.

We are referring to the standard PCI 2.2 bit, PCI_COMMAND_INTX_DISABLE.

	Jeff

RE: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Lu, Yinghai]

[-- Attachment #1: Type: text/plain, Size: 163 bytes --]

Add pci_intx to diable intx could make MSI work with pci.

Olivier, Please test it attached patch with latest git ... I hardcode to
make enable_msi=1.

YH


[-- Attachment #2: hda_intel_pci_intx.diff --]
[-- Type: application/octet-stream, Size: 1452 bytes --]

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index e35cfd3..e8c4bf5 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -547,6 +547,7 @@ static unsigned int azx_rirb_get_respons
 		chip->msi = 0;
 		if (azx_acquire_irq(chip, 1) < 0)
 			return -1;
+		pci_intx(chip->pci, 1);
 		goto again;
 	}
 
@@ -1435,11 +1436,17 @@ static int azx_resume(struct pci_dev *pc
 		return -EIO;
 	}
 	pci_set_master(pci);
-	if (chip->msi)
-		if (pci_enable_msi(pci) < 0)
+	if (chip->msi) {
+		pci_intx(pci, 0);
+		if (pci_enable_msi(pci) < 0) {
 			chip->msi = 0;
+		}
+	}
 	if (azx_acquire_irq(chip, 1) < 0)
 		return -EIO;
+
+	if (!chip->msi) 
+		pci_intx(pci, 1);
 	azx_init_chip(chip);
 	snd_hda_resume(chip->bus);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
@@ -1531,7 +1538,8 @@ static int __devinit azx_create(struct s
 	chip->pci = pci;
 	chip->irq = -1;
 	chip->driver_type = driver_type;
-	chip->msi = enable_msi;
+//	chip->msi = enable_msi;
+	chip->msi = 1;
 
 	chip->position_fix = position_fix;
 	chip->single_cmd = single_cmd;
@@ -1561,14 +1569,20 @@ #endif
 		goto errout;
 	}
 
-	if (chip->msi)
-		if (pci_enable_msi(pci) < 0)
+	if (chip->msi) {
+		pci_intx(pci, 0);
+		if (pci_enable_msi(pci) < 0) {
 			chip->msi = 0;
+		}
+	}
 
 	if (azx_acquire_irq(chip, 0) < 0) {
 		err = -EBUSY;
 		goto errout;
 	}
+	
+	if(!chip->msi)
+		pci_intx(pci, 1);
 
 	pci_set_master(pci);
 	synchronize_irq(chip->irq);

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[D. Hazelton]

On Wednesday 15 November 2006 06:40, Alan wrote:
> > Point is that it is still only one platform. Or don't you understand the
> > point I was making?
>
> Given the Intel chipset platform appears to be over half the market it
> makes sense to do things right there. It also encourages other vendors to
> fix their stuff or send us patches to do so.
>
> So I don't think anyone gives a damn about your personal issues with
> Intel.
>
> Alan

I have no personal issues with Intel. I just always avoided their stuff 
because it always cost more and wasn't as well supported in Linux back when I 
first started using Linux.

Anyway, my point still stands. One platform does not a whitelist make. Instead 
why not make the MSI support optional and a build-option? I do believe that I 
even suggested such. This would allow for the people that want it (or need 
it) to have it and would keep a potentially dangerous option from being used.

Now, since I'm at the end of options in explaining my point I'm going to start 
ignoring further messages on this topic.

DRH

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Benjamin Herrenschmidt]

> We are referring to the standard PCI 2.2 bit, PCI_COMMAND_INTX_DISABLE.

Yeah, I figured it, I somewhat forgot about it ... it got introduced in
2.3 though, no ?

Cheers,
Ben.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[linux]

> It all boils down to the same thing: either we have to know that MSI works 
> (where "know" is obviously relative - it's not like you can avoid _all_ 
> bugs, but dammit, even a single report of "not working" means that there 
> are probably a ton of machines like that, and we did something wrong), or 
> we shouldn't use it. There is no middle ground. You can't really safely 
> "test" for it, and while you _can_ say "just do both", it doesn't really 
> help anything (and potentially exposes you to just more bugs: if enablign 
> MSI actually _does_ disable INTx, but then doesn't work, at a minimum you 
> end up with a device that doesn't work, even if the rest of the kernel 
> might be ok).

Er... why can't you test it?

The fundamental problem in IRQ routing is that if you have it wrong,
you have a screaming interrupt that you can't shut up.

Well, before giving up entirely, assume that *some* device owns that
interrupt, it's just mis-routed.

So start calling the IRQ handlers for *every* PCI device until the
damn interrupt goes away, or you've really proved that it can't
be shut up.

If you have interrupts coming in fast, you might have to retry a few
times to be sure there's nothing to be done, but that's nothing new.

Now, if you get really nasty with the locking, you can disable all
other interrupt handlers on all processors until you've dealt with
the screaming interrupt, and when it goes away, you can point the
finger conclusively at the device which has the misrouted interrupt.
And you've also found where its interrupt *is* routed.

If you don't have such nasty locking, then you only have a strong
suspiscion about what did it, but a few repetitions can firm that up.
Any time a device handles an interrupt that arrived from the expected
place, its index of suspiscion goes down.  You can even use this
to select an order to call the various PCI drivers.

All of this can be rather time-consuming and mess up real-time response,
but it's only once per boot, and being able to point the finger accurately
at buggy hardware rather than not working at all for mysterious reasons is
quite nice.  And an increasing number of BIOS vendors and OEMs are testing
with Linux, even if only cursorily, so there is some (slow) feedback.

Whether you actually learn to cope with misrouted interrupts is
a separate issue that I won't raise.

RE: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Lu, Yinghai]

I think the root cause in hda_intel driver's self.

It gets io-apic irq initialized at first, and it will use
azx_acquire_irq to install handler after check if MSI can be enabled.
And when it try to enable the MSI, that will start the int in the chip.
Even handler for MSI is not installed.

YH

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 66 bytes --]

Please try the patch about using pci_intx.

it could use msi.

YH

[-- Attachment #2: hda_intel_pci_intx.diff --]
[-- Type: text/x-patch, Size: 1374 bytes --]

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index e35cfd3..88b99ab 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -547,6 +547,7 @@ static unsigned int azx_rirb_get_respons
 		chip->msi = 0;
 		if (azx_acquire_irq(chip, 1) < 0)
 			return -1;
+		pci_intx(chip->pci, 1);
 		goto again;
 	}
 
@@ -1435,11 +1436,16 @@ static int azx_resume(struct pci_dev *pc
 		return -EIO;
 	}
 	pci_set_master(pci);
-	if (chip->msi)
+	if (chip->msi) {
+		pci_intx(pci, 0);
 		if (pci_enable_msi(pci) < 0)
 			chip->msi = 0;
+	}
 	if (azx_acquire_irq(chip, 1) < 0)
 		return -EIO;
+
+	if (!chip->msi) 
+		pci_intx(pci, 1);
 	azx_init_chip(chip);
 	snd_hda_resume(chip->bus);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
@@ -1531,7 +1537,8 @@ static int __devinit azx_create(struct s
 	chip->pci = pci;
 	chip->irq = -1;
 	chip->driver_type = driver_type;
-	chip->msi = enable_msi;
+//	chip->msi = enable_msi;
+	chip->msi = 1;
 
 	chip->position_fix = position_fix;
 	chip->single_cmd = single_cmd;
@@ -1561,14 +1568,19 @@ #endif
 		goto errout;
 	}
 
-	if (chip->msi)
+	if (chip->msi) {
+		pci_intx(pci, 0);
 		if (pci_enable_msi(pci) < 0)
 			chip->msi = 0;
+	}
 
 	if (azx_acquire_irq(chip, 0) < 0) {
 		err = -EBUSY;
 		goto errout;
 	}
+	
+	if(!chip->msi)
+		pci_intx(pci, 1);
 
 	pci_set_master(pci);
 	synchronize_irq(chip->irq);

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Benjamin Herrenschmidt wrote:
>> We are referring to the standard PCI 2.2 bit, PCI_COMMAND_INTX_DISABLE.
> 
> Yeah, I figured it, I somewhat forgot about it ... it got introduced in
> 2.3 though, no ?

It's pretty new.  2.2 or 2.3.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Takashi Iwai]

At Wed, 15 Nov 2006 14:15:45 -0500,
Jeff Garzik wrote:
> 
> ACK the pci_intx() calls, NAK the obviously overweight spinlock changes. 
>   The spinlock changes are completely unnecessary.  Just look at any 
> other (non-ALSA) PCI driver.  Existing "spin_lock()" is fine for both 
> PCI shared irq handlers and MSI irq handlers.
> 
> It sounds like you are trying to work around a reentrancy problem that 
> does not exist.
> 
> Only weird drivers like ps2kbd/mouse or IDE need spin_lock_irqsave(), 
> where separate interrupt sources call the same function.

OK, I revised it, also referring to a similar patch by Yinghai.
I think we can simplify the change like below.
Olivier, could you test this patch, too?

(Here the first chunk (enable_msi=1) is just for testing.
 The patch isn't for merge yet.)


Takashi

---
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index e35cfd3..130ee12 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -55,7 +55,7 @@ static char *model;
 static int position_fix;
 static int probe_mask = -1;
 static int single_cmd;
-static int enable_msi;
+static int enable_msi = 1;
 
 module_param(index, int, 0444);
 MODULE_PARM_DESC(index, "Index value for Intel HD audio interface.");
@@ -1380,7 +1380,8 @@ static int __devinit azx_init_stream(str
 
 static int azx_acquire_irq(struct azx *chip, int do_disconnect)
 {
-	if (request_irq(chip->pci->irq, azx_interrupt, IRQF_DISABLED|IRQF_SHARED,
+	if (request_irq(chip->pci->irq, azx_interrupt,
+			chip->msi ? 0 : IRQF_SHARED,
 			"HDA Intel", chip)) {
 		printk(KERN_ERR "hda-intel: unable to grab IRQ %d, "
 		       "disabling device\n", chip->pci->irq);
@@ -1389,6 +1390,7 @@ static int azx_acquire_irq(struct azx *c
 		return -1;
 	}
 	chip->irq = chip->pci->irq;
+	pci_intx(chi->pci, !chip->msi);
 	return 0;
 }
 

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Alan]

On 15 Nov 2006 23:24:32 -0500
linux@horizon.com wrote:

> Well, before giving up entirely, assume that *some* device owns that
> interrupt, it's just mis-routed.
> 
> So start calling the IRQ handlers for *every* PCI device until the
> damn interrupt goes away, or you've really proved that it can't
> be shut up.

See the "irqpoll" option. We don't however try and learn IRQ errors
from it although we certainly could do so (and indeed Ingo semi jokingly
suggested it when the patch went in)

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Mws]

On Wednesday 15 November 2006 22:10, Mws wrote:
> On Wednesday 15 November 2006 21:14, Linus Torvalds wrote:
> > 
> > On Wed, 15 Nov 2006, Mws wrote:
> > > 
> > > after some small discussions on alsa-user ml i recognised this
> > > thread today. 
> > > i thought my problem could also exist on this msi stuff.
> > > i disabled msi in kernel config, reboot, and, after starting x & kde
> > > i got immediately a freeze.
> > > last and maybe important last try has been to
> > > enable msi support _but_ boot kernel with cmdline pci=nomsi
> > > this finally did work out. i got a working sound environment again.
> > 
> > I expect that you have the exact same issue as Olivier: the "hang" is 
> > probably because you started using the sound device (beeping on the 
> > console is handled by the old built-in speaker, but in X a single beep 
> > tends to be due to sound device drivers), and because of sound irq 
> > misrouting you had some other device (like your harddisk) that got their 
> > irq disabled due to the "nobody cared" issue.
> > 
> > > i find it a bit abnormal that the disabling msi in kernel config behaviour
> > > is different from kernel cmdline pci=nomsi option.
> > 
> > Now, that does actually worry me. It _should_ have worked with CONFIG_MSI 
> > disabled. I wonder if some of the MSI workarounds actually broke the HDA 
> > driver subtly.
> > 
> > Anyway, it would be a good idea to test the current -git tree if you can, 
> > both with CONFIG_MSI and without (and _without_ any "pci=nomsi" kernel 
> > command line). It should hopefully work, exactly because the HDA driver 
> > now shouldn't even try to do any MSI stuff by default.
> > 
> > Knock wood.
> > 
> > 		Linus
> hi,
> 
> linus, just cloned current git, but will test it tomorrow morning.
> 
> will inform you on the results.
> 
> thanks 
> marcel

hi, 

i just tried the git that i cloned yesterday, when you asked me to, linus.

i don't get any freezes and sound is fine.
no cmdline option pci=nomsi and msi enabled in .config

thanks for your help.

best regards
marcel

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Krzysztof Halasa]

Jeff Garzik <jeff@garzik.org> writes:

>>> We are referring to the standard PCI 2.2 bit, PCI_COMMAND_INTX_DISABLE.
>> Yeah, I figured it, I somewhat forgot about it ... it got introduced
>> in
>> 2.3 though, no ?
>
> It's pretty new.  2.2 or 2.3.

2.3.

PCI 2.2 defines bits 0-9 only (bit 7 = Stepping Control)
PCI 2.3 and 3.0: bit 7 = Reserved, bit 10 = Interrupt Disable.

OTOH many devices have "interrupt disable" bit somewhere else, in
their specific PCI config registers or in regular config registers
(accessible with normal Memory Read/Write cycles).

MSI was first defined in PCI 2.2.

Perhaps I can check NV (MCP55) for that problem with a module claiming
all free interrupts. Tomorrow maybe.
-- 
Krzysztof Halasa

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Jeff Garzik]

Krzysztof Halasa wrote:
> OTOH many devices have "interrupt disable" bit somewhere else, in
> their specific PCI config registers or in regular config registers
> (accessible with normal Memory Read/Write cycles).

Most interrupt-driven devices have an interrupt mask register of some 
sort.  The nice thing about PCI_COMMAND_INTX_DISABLE is that it is 
standardized.

	Jeff

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Linus Torvalds]

On Wed, 15 Nov 2006, linux@horizon.com wrote:
>
> Er... why can't you test it?

Because if it is level-triggered kind of thing and comes in on the wrong 
IRQ, and that IRQ is already used by something else, you're basically 
dead.

IRQ probing is essentially unworkable. We support it for ISA cards (and 
"ISA" here tends to mean PCMCIA - there are almost no other ISA forms 
left), but it just doesn't work very well.

> Well, before giving up entirely, assume that *some* device owns that
> interrupt, it's just mis-routed.
> 
> So start calling the IRQ handlers for *every* PCI device until the
> damn interrupt goes away, or you've really proved that it can't
> be shut up.

Sure, you can add crap upon crap upon crap for these things. I pretty much 
guarantee you that it's not going to work, though. Why? Because developers 
won't even be working on those machines that have irq routing problems, so 
they won't be testing things that way, and because if you have irq routing 
problems, your problems are so fundamental that it really isn't worth it 
any more.

We've added basic _debugging_ facilities (the whole "nobody cared" thing 
is really a debugging thing, not a "make it work" thing). That was big, 
because it used to be that a screaming interrupt just locked up the whole 
machine - notably you inserted a PCMCIA card, and the machine would just 
lock up (sometimes it would come back when you removed the card again).

Trying to work around it is basically not worthwhile. You need to fix the 
problem. Interrupts are very fundamental, you don't want to be guessing 
about them. If they are wrong, you're screwed.

		Linus

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Roland Dreier]

 > Most interrupt-driven devices have an interrupt mask register of some
 > sort.  The nice thing about PCI_COMMAND_INTX_DISABLE is that it is
 > standardized.

You all got on me about quoting the spec about not generating INTx
interrupts after MSI is enabled.  What makes you think that setting
some bit in the command register, which wasn't even standardized until
PCI 2.3 and which most hardware designers probably forgot about, is
going to work on broken devices?

 - R.

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[D. Hazelton]

On Wednesday 15 November 2006 23:25, Lu, Yinghai wrote:
> I think the root cause in hda_intel driver's self.
>
> It gets io-apic irq initialized at first, and it will use
> azx_acquire_irq to install handler after check if MSI can be enabled.
> And when it try to enable the MSI, that will start the int in the chip.
> Even handler for MSI is not installed.
>
> YH

That seems rather stupid. Perhaps installing a conditional handler before 
doing anything with MSI will solve that problem... The problem then just 
revolves around replacing the conditional handler with the real one when MSI 
is fully enabled. If I understood the code for this driver better I'd try to 
make this work. As it stands I'm still trying to recover from the loss of the 
work I did on another project. (DRM based uniform kernel graphics interface - 
had a blackout here that caused my devel machine to crash and left me with a 
trashed drive)

DRH

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Olivier Nicolas]

Takashi Iwai wrote:
> At Wed, 15 Nov 2006 14:15:45 -0500,
> Jeff Garzik wrote:
>> ACK the pci_intx() calls, NAK the obviously overweight spinlock changes. 
>>   The spinlock changes are completely unnecessary.  Just look at any 
>> other (non-ALSA) PCI driver.  Existing "spin_lock()" is fine for both 
>> PCI shared irq handlers and MSI irq handlers.
>>
>> It sounds like you are trying to work around a reentrancy problem that 
>> does not exist.
>>
>> Only weird drivers like ps2kbd/mouse or IDE need spin_lock_irqsave(), 
>> where separate interrupt sources call the same function.
> 
> OK, I revised it, also referring to a similar patch by Yinghai.
> I think we can simplify the change like below.
> Olivier, could you test this patch, too?

Applied to 2.6.19-rc6, the module tries MSI but this time no IRQ get 
disabled. The result is equivalent to 2.6.19-rc6


ALSA sound/pci/hda/hda_intel.c:543: hda_intel: No response from codec, 
disabling MSI...
hda_codec: Unknown model for AD1988, trying auto-probe from BIOS...


Full details:
http://olivn.trollprod.com/19-rc6/19-rc6-takashi-routeirq1.dmesg
http://olivn.trollprod.com/19-rc6/19-rc6-takashi-routeirq1.irq

Olivier



> 
> (Here the first chunk (enable_msi=1) is just for testing.
>  The patch isn't for merge yet.)
> 
> 
> Takashi
> 
> ---
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index e35cfd3..130ee12 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -55,7 +55,7 @@ static char *model;
>  static int position_fix;
>  static int probe_mask = -1;
>  static int single_cmd;
> -static int enable_msi;
> +static int enable_msi = 1;
>  
>  module_param(index, int, 0444);
>  MODULE_PARM_DESC(index, "Index value for Intel HD audio interface.");
> @@ -1380,7 +1380,8 @@ static int __devinit azx_init_stream(str
>  
>  static int azx_acquire_irq(struct azx *chip, int do_disconnect)
>  {
> -	if (request_irq(chip->pci->irq, azx_interrupt, IRQF_DISABLED|IRQF_SHARED,
> +	if (request_irq(chip->pci->irq, azx_interrupt,
> +			chip->msi ? 0 : IRQF_SHARED,
>  			"HDA Intel", chip)) {
>  		printk(KERN_ERR "hda-intel: unable to grab IRQ %d, "
>  		       "disabling device\n", chip->pci->irq);
> @@ -1389,6 +1390,7 @@ static int azx_acquire_irq(struct azx *c
>  		return -1;
>  	}
>  	chip->irq = chip->pci->irq;
> +	pci_intx(chi->pci, !chip->msi);
>  	return 0;
>  }
>  


-- 
Laudator temporis acti (Horace, 173)
Donner c'est donner, repeindre ses volets

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Olivier Nicolas]

Yinghai Lu wrote:
> Please try the patch about using pci_intx.
> 
> it could use msi.

It can't cold boot with this one (with pci=routeirq or not)

The last visible boot messages are
Interrupt Link [AAZA] enabled at IRQ 20
Interrupt 0000:00:0e.1[B]->Link [AAZA] -> GSI 20 (level,low) -> IRQ 20



Olivier

> 
> YH
> 
> 
> ------------------------------------------------------------------------
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index e35cfd3..88b99ab 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -547,6 +547,7 @@ static unsigned int azx_rirb_get_respons
>  		chip->msi = 0;
>  		if (azx_acquire_irq(chip, 1) < 0)
>  			return -1;
> +		pci_intx(chip->pci, 1);
>  		goto again;
>  	}
>  
> @@ -1435,11 +1436,16 @@ static int azx_resume(struct pci_dev *pc
>  		return -EIO;
>  	}
>  	pci_set_master(pci);
> -	if (chip->msi)
> +	if (chip->msi) {
> +		pci_intx(pci, 0);
>  		if (pci_enable_msi(pci) < 0)
>  			chip->msi = 0;
> +	}
>  	if (azx_acquire_irq(chip, 1) < 0)
>  		return -EIO;
> +
> +	if (!chip->msi) 
> +		pci_intx(pci, 1);
>  	azx_init_chip(chip);
>  	snd_hda_resume(chip->bus);
>  	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
> @@ -1531,7 +1537,8 @@ static int __devinit azx_create(struct s
>  	chip->pci = pci;
>  	chip->irq = -1;
>  	chip->driver_type = driver_type;
> -	chip->msi = enable_msi;
> +//	chip->msi = enable_msi;
> +	chip->msi = 1;
>  
>  	chip->position_fix = position_fix;
>  	chip->single_cmd = single_cmd;
> @@ -1561,14 +1568,19 @@ #endif
>  		goto errout;
>  	}
>  
> -	if (chip->msi)
> +	if (chip->msi) {
> +		pci_intx(pci, 0);
>  		if (pci_enable_msi(pci) < 0)
>  			chip->msi = 0;
> +	}
>  
>  	if (azx_acquire_irq(chip, 0) < 0) {
>  		err = -EBUSY;
>  		goto errout;
>  	}
> +	
> +	if(!chip->msi)
> +		pci_intx(pci, 1);
>  
>  	pci_set_master(pci);
>  	synchronize_irq(chip->irq);


-- 
Laudator temporis acti (Horace, 173)
Donner c'est donner, repeindre ses volets

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Olivier Nicolas]

Lu, Yinghai wrote:
> Add pci_intx to diable intx could make MSI work with pci.
> 
> Olivier, Please test it attached patch with latest git ... I hardcode to
> make enable_msi=1.
> 
> YH
> 

The kernel boots only with pci=routeirq, no IRQ get disabled but the 
sound driver does not work.


http://olivn.trollprod.org/19-rc6/19-rc6-yinghai1-routeirq.dmesg
http://olivn.trollprod.org/19-rc6/19-rc6-yinghai1-routeirq.irq



In order to get reproductible result, I halt the system and remove the 
power cord for 30 seconds.But once, I just reboot and get that strange 
result

IRQ 22 is disabled but snd_hda_intel seems to get a MSI interrupt! (It 
cannot be reproduced)

http://olivn.trollprod.org/19-rc5-git7-patch1.dmesg

           CPU0       CPU1
   0:        614    1107801   IO-APIC-edge      timer
   1:          2        361   IO-APIC-edge      i8042
   6:          0          5   IO-APIC-edge      floppy
   8:          0          0   IO-APIC-edge      rtc
   9:          0          0   IO-APIC-fasteoi   acpi
  12:          0        163   IO-APIC-edge      i8042
  14:         10      11446   IO-APIC-edge      ide0
  16:          0          3   IO-APIC-fasteoi   libata, ohci1394
  17:          4          8   IO-APIC-fasteoi   bttv0
  20:          2         22   IO-APIC-fasteoi   ehci_hcd:usb2
  21:          0          4   IO-APIC-fasteoi   libata, ohci_hcd:usb1
  22:         15      99985   IO-APIC-fasteoi   libata
  23:         30       7639   IO-APIC-fasteoi   libata
307:        156     443303   PCI-MSI-edge      eth1
308:          0        311   PCI-MSI-edge      eth1
309:          0        401   PCI-MSI-edge      eth1
310:        156     443333   PCI-MSI-edge      eth0
311:          0          0   PCI-MSI-edge      eth0
312:          0          0   PCI-MSI-edge      eth0
313:          0          1   PCI-MSI-edge      HDA Intel
NMI:         65         47
LOC:    1108404    1108429
ERR:          0




Olivier

RE: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Lu, Yinghai]

[-- Attachment #1: Type: text/plain, Size: 337 bytes --]

-----Original Message-----
From: Olivier Nicolas [mailto:olivn@trollprod.org] 


>IRQ 22 is disabled but snd_hda_intel seems to get a MSI interrupt! (It 
>cannot be reproduced)

>313:          0          1   PCI-MSI-edge      HDA Intel

Please try attached one, also build hda_intel into kernel directly if it
is not.

YH


[-- Attachment #2: hda_intel_pci_intx_11162006.diff --]
[-- Type: application/octet-stream, Size: 2585 bytes --]

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index e35cfd3..98d25d8 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -55,7 +55,7 @@ static char *model;
 static int position_fix;
 static int probe_mask = -1;
 static int single_cmd;
-static int enable_msi;
+static int disable_msi;
 
 module_param(index, int, 0444);
 MODULE_PARM_DESC(index, "Index value for Intel HD audio interface.");
@@ -69,8 +69,8 @@ module_param(probe_mask, int, 0444);
 MODULE_PARM_DESC(probe_mask, "Bitmask to probe codecs (default = -1).");
 module_param(single_cmd, bool, 0444);
 MODULE_PARM_DESC(single_cmd, "Use single command to communicate with codecs (for debugging only).");
-module_param(enable_msi, int, 0);
-MODULE_PARM_DESC(enable_msi, "Enable Message Signaled Interrupt (MSI)");
+module_param(disable_msi, int, 0);
+MODULE_PARM_DESC(disable_msi, "Disable Message Signaled Interrupt (MSI)");
 
 
 /* just for backward compatibility */
@@ -547,6 +547,7 @@ static unsigned int azx_rirb_get_respons
 		chip->msi = 0;
 		if (azx_acquire_irq(chip, 1) < 0)
 			return -1;
+		pci_intx(chip->pci, 1);
 		goto again;
 	}
 
@@ -1380,7 +1381,8 @@ static int __devinit azx_init_stream(str
 
 static int azx_acquire_irq(struct azx *chip, int do_disconnect)
 {
-	if (request_irq(chip->pci->irq, azx_interrupt, IRQF_DISABLED|IRQF_SHARED,
+	if (request_irq(chip->pci->irq, azx_interrupt,
+			chip->msi ? 0 : IRQF_SHARED,
 			"HDA Intel", chip)) {
 		printk(KERN_ERR "hda-intel: unable to grab IRQ %d, "
 		       "disabling device\n", chip->pci->irq);
@@ -1435,11 +1437,16 @@ static int azx_resume(struct pci_dev *pc
 		return -EIO;
 	}
 	pci_set_master(pci);
-	if (chip->msi)
+	if (chip->msi) {
+		pci_intx(pci, 0);
 		if (pci_enable_msi(pci) < 0)
 			chip->msi = 0;
+	}
 	if (azx_acquire_irq(chip, 1) < 0)
 		return -EIO;
+
+	if (!chip->msi) 
+		pci_intx(pci, 1);
 	azx_init_chip(chip);
 	snd_hda_resume(chip->bus);
 	snd_power_change_state(card, SNDRV_CTL_POWER_D0);
@@ -1531,7 +1538,7 @@ static int __devinit azx_create(struct s
 	chip->pci = pci;
 	chip->irq = -1;
 	chip->driver_type = driver_type;
-	chip->msi = enable_msi;
+	chip->msi = !disable_msi;
 
 	chip->position_fix = position_fix;
 	chip->single_cmd = single_cmd;
@@ -1561,14 +1568,19 @@ #endif
 		goto errout;
 	}
 
-	if (chip->msi)
+	if (chip->msi) {
+		pci_intx(pci, 0);
 		if (pci_enable_msi(pci) < 0)
 			chip->msi = 0;
+	}
 
 	if (azx_acquire_irq(chip, 0) < 0) {
 		err = -EBUSY;
 		goto errout;
 	}
+	
+	if(!chip->msi)
+		pci_intx(pci, 1);
 
 	pci_set_master(pci);
 	synchronize_irq(chip->irq);

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Olivier Nicolas]

Lu, Yinghai wrote:
> -----Original Message-----
> From: Olivier Nicolas [mailto:olivn@trollprod.org] 
> 
> 
>> IRQ 22 is disabled but snd_hda_intel seems to get a MSI interrupt! (It 
>> cannot be reproduced)
> 
>> 313:          0          1   PCI-MSI-edge      HDA Intel
> 
> Please try attached one, also build hda_intel into kernel directly if it
> is not.
> 
> YH
> 

ALSA is build directly in the kernel

messages: htt://olivn.trollprod.org/19-rc6/hda_intel_pci_intx_11162006.dmesg

cat /proc/interrupts
            CPU0       CPU1
   0:        627     164040   IO-APIC-edge      timer
   1:          5       1363   IO-APIC-edge      i8042
   6:          0          5   IO-APIC-edge      floppy
   7:          0          0   IO-APIC-edge      MOTU MTPAV
   8:          0          0   IO-APIC-edge      rtc
   9:          0          0   IO-APIC-fasteoi   acpi
  12:        142      39757   IO-APIC-edge      i8042
  14:          5       1725   IO-APIC-edge      ide0
  16:         34      45618   IO-APIC-fasteoi   libata, ohci1394, nvidia
  17:          0          4   IO-APIC-fasteoi   Bt87x audio, bttv0
  20:         84      30048   IO-APIC-fasteoi   libata
  21:          2        451   IO-APIC-fasteoi   HDA Intel, ehci_hcd:usb2
  22:          0          1   IO-APIC-fasteoi   libata, ohci_hcd:usb1
  23:          0          0   IO-APIC-fasteoi   libata
308:        179      63652   PCI-MSI-edge      eth1
309:          1         98   PCI-MSI-edge      eth1
310:          1        117   PCI-MSI-edge      eth1
311:        179      63673   PCI-MSI-edge      eth0
312:          0          0   PCI-MSI-edge      eth0
313:          0          1   PCI-MSI-edge      eth0
NMI:         84         72
LOC:     164581     164616
ERR:          0


cat /proc/asound/cards
  0 [Dummy          ]: Dummy - Dummy
                       Dummy 1
  1 [VirMIDI        ]: VirMIDI - VirMIDI
                       Virtual MIDI Card 1
  2 [port           ]: MTPAV - MTPAV on parallel port
                       MTPAV on parallel port at 0x378
  3 [Bt878          ]: Bt87x - Brooktree Bt878
                       Brooktree Bt878 at 0xfdffe000, irq 17
  4 [NVidia         ]: HDA-Intel - HDA NVidia
                       HDA NVidia at 0xfe020000 irq 313

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Takashi Iwai]

At Fri, 17 Nov 2006 00:01:25 +0100,
Olivier Nicolas wrote:
> 
> Takashi Iwai wrote:
> > At Wed, 15 Nov 2006 14:15:45 -0500,
> > Jeff Garzik wrote:
> >> ACK the pci_intx() calls, NAK the obviously overweight spinlock changes. 
> >>   The spinlock changes are completely unnecessary.  Just look at any 
> >> other (non-ALSA) PCI driver.  Existing "spin_lock()" is fine for both 
> >> PCI shared irq handlers and MSI irq handlers.
> >>
> >> It sounds like you are trying to work around a reentrancy problem that 
> >> does not exist.
> >>
> >> Only weird drivers like ps2kbd/mouse or IDE need spin_lock_irqsave(), 
> >> where separate interrupt sources call the same function.
> > 
> > OK, I revised it, also referring to a similar patch by Yinghai.
> > I think we can simplify the change like below.
> > Olivier, could you test this patch, too?
> 
> Applied to 2.6.19-rc6, the module tries MSI but this time no IRQ get 
> disabled. The result is equivalent to 2.6.19-rc6

Does it mean that the driver works as expected with this patch?

> 
> ALSA sound/pci/hda/hda_intel.c:543: hda_intel: No response from codec, 
> disabling MSI...
> hda_codec: Unknown model for AD1988, trying auto-probe from BIOS...
> 
> 
> Full details:
> http://olivn.trollprod.com/19-rc6/19-rc6-takashi-routeirq1.dmesg
> http://olivn.trollprod.com/19-rc6/19-rc6-takashi-routeirq1.irq

These look OK to me.


thanks,

Takashi

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Yinghai Lu]

the fallback path from MSI test to ioapic still not look good.

I think you could seperate azx_interrupt_test later.

It seems on C51+MCP55 has problem to use MSI for hda.
and I have tried two MCP55 only systems, the MSI for hda works well.

YH

Re: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Linus Torvalds]

On Fri, 17 Nov 2006, Yinghai Lu wrote:
>
> the fallback path from MSI test to ioapic still not look good.

I don't think there should be a fallback at all.

Let's just:

 - keep MSI disabled by default for now, until we know what's really going 
   on (and we can try enabling it every once in a while in a -rc1, just to 
   get more information ;)

 - when MSI is selected (ie both the HDA driver _and_ the system decides 
   that MSI is ok), it should just get used. No testing. No fallbacks. No 
   strange code at all. Just use it.

The whole notion of trying to see if MSI works is very suspect, and likely 
fragile. We should never _ever_ "switch" between the two modes. We choose 
one mode, and we stick to it. 

Eventually, MSI will hopefully be the more robust of the two methods. 
Maybe it will take a year. And maybe we'll end up not using MSI on a lot 
of the _current_ crop of motherboards. We don't want to carry the "fall 
back from MSI to INTx" code around. We just want a flag saying "use MSI" 
or "use INTx".

		Linus

RE: [PATCH] ALSA: hda-intel - Disable MSI support by default

[Lu, Yinghai]

-----Original Message-----
From: Linus Torvalds [mailto:torvalds@osdl.org] 

>Eventually, MSI will hopefully be the more robust of the two methods. 
>Maybe it will take a year. And maybe we'll end up not using MSI on a
lot 
>of the _current_ crop of motherboards. We don't want to carry the "fall

>back from MSI to INTx" code around. We just want a flag saying "use
MSI" 
>or "use INTx".

Good, that will be simple and clear.

YH