emu10k1: fix P16V breakage for non Audigy2 cards

The P16V patch unconditionally checks the IPR2 register in the interrupt handler resulting in infinite loop and system lockup on any non Audigy2 cards. I really hate checking emu->is_audigy and emu->revision in a fast path like the IRQ handler but I don't see another way.

Also, don't bother allocating/freeing the DMA buffer for P16V unless it's really present.

This is a critical fix and should trigger an immediate rc2 release IMO. Currently any emu10k1 users other than Audigy 2 will lock up hard as soon as they play any sound.

Index: alsa/alsa-kernel/pci/emu10k1/emu10k1.c
===================================================================
RCS file: /cvsroot/alsa/alsa-kernel/pci/emu10k1/emu10k1.c,v retrieving revision 1.32

diff -u -r1.32 emu10k1.c
@@ -140,9 +140,11 @@
 		return err;
 	}
 	/* This stores the periods table. */
-	if(snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, snd_dma_pci_data(pci), 1024, &emu->p16v_buffer) < 0) {
-		snd_p16v_free(emu);
-		return -ENOMEM;
+	if (emu->audigy && emu->revision == 4) { /* P16V */	
+		if(snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, snd_dma_pci_data(pci), 1024, &emu->p16v_buffer) < 0) {
+			snd_p16v_free(emu);
+			return -ENOMEM;
+		}
 	}
 
 	if ((err = snd_emu10k1_mixer(emu)) < 0) {

Index: alsa/alsa-kernel/pci/emu10k1/emu10k1_main.c
===================================================================
RCS file: /cvsroot/alsa/alsa-kernel/pci/emu10k1/emu10k1_main.c,v retrieving revision 1.44

diff -u -r1.44 emu10k1_main.c
@@ -600,7 +600,8 @@
 	if (emu->port)
 		pci_release_regions(emu->pci);
 	pci_disable_device(emu->pci);
-	snd_p16v_free(emu);
+	if (emu->audigy && emu->revision == 4) /* P16V */	
+		snd_p16v_free(emu);
 	kfree(emu);
 	return 0;
 }

Index: alsa/alsa-kernel/pci/emu10k1/irq.c
===================================================================
RCS file: /cvsroot/alsa/alsa-kernel/pci/emu10k1/irq.c,v retrieving revision 1.13

diff -u -r1.13 irq.c
@@ -149,7 +149,7 @@
 		}
 		if (status) {
 			unsigned int bits;
-			//snd_printk(KERN_ERR "emu10k1: unhandled interrupt: 0x%08x\n", status);
+			snd_printk(KERN_ERR "emu10k1: unhandled interrupt: 0x%08x\n", status);
 			//make sure any interrupts we don't handle are disabled:
 			bits = INTE_FXDSPENABLE |
 				INTE_PCIERRORENABLE |

@@ -170,19 +170,20 @@
 		}
 		outl(orig_status, emu->port + IPR); /* ack all */
 	}
-	while ((status2 = inl(emu->port + IPR2)) != 0) {
-		u32 mask = INTE2_PLAYBACK_CH_0_LOOP;  /* Full Loop */
-		emu10k1_voice_t *pvoice = &(emu->p16v_voices[0]);
-		orig_status2 = status2;
-		if(status2 & mask) {
-			if(pvoice->use) {
-				snd_pcm_period_elapsed(pvoice->epcm->substream);
-			} else { 
-				snd_printk(KERN_ERR "p16v: status: 0x%08x, mask=0x%08x, pvoice=%p, use=%d\n", status2, mask, pvoice, pvoice->use);
+	if (emu->audigy && emu->revision == 4) { /* P16V */	
+		while ((status2 = inl(emu->port + IPR2)) != 0) {
+			u32 mask = INTE2_PLAYBACK_CH_0_LOOP;  /* Full Loop */
+			emu10k1_voice_t *pvoice = &(emu->p16v_voices[0]);
+			orig_status2 = status2;
+			if(status2 & mask) {
+				if(pvoice->use) {
+					snd_pcm_period_elapsed(pvoice->epcm->substream);
+				} else { 
+					snd_printk(KERN_ERR "p16v: status: 0x%08x, mask=0x%08x, pvoice=%p, use=%d\n", status2, mask, pvoice, pvoice->use);
+				}
 			}
+			outl(orig_status2, emu->port + IPR2); /* ack all */
 		}
-		outl(orig_status2, emu->port + IPR2); /* ack all */
 	}
-
 	return IRQ_RETVAL(handled);
 }

Oh, one more thing: this patch restores the "unhandled interrupt" printk() that was commented out by the P16V patch. I am sure this was useful for development but seems like a bad idea for production.

James, were you still getting unhandled interrupts with the version of the patch that was merged?

Lee

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Thank you for fixing the bug.
The "unhandled interrupt" should be commented out. It is used for reverse engineering certain features.

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

OK, updated patch attached.

Please apply to CVS ASAP, this is a bad bug!

Lee

Index: alsa/alsa-kernel/pci/emu10k1/emu10k1.c
===================================================================
RCS file: /cvsroot/alsa/alsa-kernel/pci/emu10k1/emu10k1.c,v retrieving revision 1.32

diff -u -r1.32 emu10k1.c
@@ -140,9 +140,11 @@
 		return err;
 	}
 	/* This stores the periods table. */
-	if(snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, snd_dma_pci_data(pci), 1024, &emu->p16v_buffer) < 0) {
-		snd_p16v_free(emu);
-		return -ENOMEM;
+	if (emu->audigy && emu->revision == 4) { /* P16V */	
+		if(snd_dma_alloc_pages(SNDRV_DMA_TYPE_DEV, snd_dma_pci_data(pci), 1024, &emu->p16v_buffer) < 0) {
+			snd_p16v_free(emu);
+			return -ENOMEM;
+		}
 	}
 
 	if ((err = snd_emu10k1_mixer(emu)) < 0) {

Index: alsa/alsa-kernel/pci/emu10k1/emu10k1_main.c
===================================================================
RCS file: /cvsroot/alsa/alsa-kernel/pci/emu10k1/emu10k1_main.c,v retrieving revision 1.44

diff -u -r1.44 emu10k1_main.c
@@ -600,7 +600,8 @@
 	if (emu->port)
 		pci_release_regions(emu->pci);
 	pci_disable_device(emu->pci);
-	snd_p16v_free(emu);
+	if (emu->audigy && emu->revision == 4) /* P16V */	
+		snd_p16v_free(emu);
 	kfree(emu);
 	return 0;
 }

Index: alsa/alsa-kernel/pci/emu10k1/irq.c
===================================================================
RCS file: /cvsroot/alsa/alsa-kernel/pci/emu10k1/irq.c,v retrieving revision 1.13

diff -u -r1.13 irq.c
@@ -170,19 +170,20 @@
 		}
 		outl(orig_status, emu->port + IPR); /* ack all */
 	}
-	while ((status2 = inl(emu->port + IPR2)) != 0) {
-		u32 mask = INTE2_PLAYBACK_CH_0_LOOP;  /* Full Loop */
-		emu10k1_voice_t *pvoice = &(emu->p16v_voices[0]);
-		orig_status2 = status2;
-		if(status2 & mask) {
-			if(pvoice->use) {
-				snd_pcm_period_elapsed(pvoice->epcm->substream);
-			} else { 
-				snd_printk(KERN_ERR "p16v: status: 0x%08x, mask=0x%08x, pvoice=%p, use=%d\n", status2, mask, pvoice, pvoice->use);
+	if (emu->audigy && emu->revision == 4) { /* P16V */	
+		while ((status2 = inl(emu->port + IPR2)) != 0) {
+			u32 mask = INTE2_PLAYBACK_CH_0_LOOP;  /* Full Loop */
+			emu10k1_voice_t *pvoice = &(emu->p16v_voices[0]);
+			orig_status2 = status2;
+			if(status2 & mask) {
+				if(pvoice->use) {
+					snd_pcm_period_elapsed(pvoice->epcm->substream);
+				} else { 
+					snd_printk(KERN_ERR "p16v: status: 0x%08x, mask=0x%08x, pvoice=%p, use=%d\n", status2, mask, pvoice, pvoice->use);
+				}
 			}
+			outl(orig_status2, emu->port + IPR2); /* ack all */
 		}
-		outl(orig_status2, emu->port + IPR2); /* ack all */
 	}
-
 	return IRQ_RETVAL(handled);
 }

At Thu, 17 Mar 2005 10:53:32 -0500,

Already applied the older one except but the change to printk. (Is that identical with the new one, right?)

Thanks.

Takashi

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Yes, that's the only change.

Thanks, I didn't see it on the CVS commit list.

Lee

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Are you sure you committed it? I still haven't seen the CVS commit message. And there does not seem to be a list delay since I got the commit message for the "misc oopses" patch immediately.

Lee

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

At Thu, 17 Mar 2005 11:20:59 -0500,

Don't worry. The CVS-commit message order is sometimes screwed.

Takashi

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

OK, now I see it.

Anyway, the CVS commit list is still way more reliable than waiting for sourceforge CVS...

Lee

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

I think that this "bad bug" will also effect Audigy 1 cards.
Does that new if statement in the irq handler correctly exclude Audigy 1 cards ?

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Yes it uses the same logic you used to detect the P16V, if (emu->audigy && emu->revision==4) IIRC.

Lee

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

Also pzad once said he suspected the Audigy1 card also has a P16V. Might be worth a try. But I have no Audigy1 hardware.

Lee

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

fix more oopses:

Index: emumixer.c
===================================================================
RCS file: /cvsroot/alsa/alsa-kernel/pci/emu10k1/emumixer.c,v retrieving revision 1.32

diff -u -r1.32 emumixer.c
@@ -482,9 +482,13 @@
 			change = 1;
 		}
 	}	
-	if (change && mix->epcm->voices[ch])
-		update_emu10k1_fxrt(emu, mix->epcm->voices[ch]->number,
-				    &mix->send_routing[0][0]);
+
+	if (change && mix->epcm) {
+		if (mix->epcm->voices[ch]) {
+			update_emu10k1_fxrt(emu, mix->epcm->voices[ch]->number,
+					&mix->send_routing[0][0]);
+		}
+	}
 	spin_unlock_irqrestore(&emu->reg_lock, flags);
 	return change;
 }

@@ -544,9 +548,12 @@
 			change = 1;
 		}
 	}
-	if (change && mix->epcm->voices[ch])
-		update_emu10k1_send_volume(emu, mix->epcm->voices[ch]->number,
-					   &mix->send_volume[0][0]);
+	if (change && mix->epcm) {
+		if (mix->epcm->voices[ch]) {
+			update_emu10k1_send_volume(emu, mix->epcm->voices[ch]->number,
+						   &mix->send_volume[0][0]);
+		}
+	}
 	spin_unlock_irqrestore(&emu->reg_lock, flags);
 	return change;
 }

@@ -600,8 +607,11 @@
 		mix->attn[0] = val;
 		change = 1;
 	}
-	if (change && mix->epcm->voices[ch])
-		snd_emu10k1_ptr_write(emu, VTFT_VOLUMETARGET, mix->epcm->voices[ch]->number, mix->attn[0]);
+	if (change && mix->epcm) {
+		if (mix->epcm->voices[ch]) {
+			snd_emu10k1_ptr_write(emu, VTFT_VOLUMETARGET, mix->epcm->voices[ch]->number, mix->attn[0]);
+		}
+	}
 	spin_unlock_irqrestore(&emu->reg_lock, flags);
 	return change;
 }

OK, those are clearly my fault.

Sounds like this could also fix OSDL bug 4282:

http://bugme.osdl.org/show_bug.cgi?id=4282

Lee

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

At Thu, 17 Mar 2005 15:11:19 +0100,

Thanks. Applied now.

Takashi

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click