Skip to contentSkip to conversation

2.5.29: some compilation fixes for irq frenzy [OSS + i8x0 audio]

PATCH In review Andy Pfiffer 3 participants 10 messages Jul 30, 2002 – Jul 31, 2002 Sound

lore
Share & tools
lore

Andy Pfiffer

author
#0 lore raw

PATCH 1/1 2.5.29: some compilation fixes for irq frenzy [OSS + i8x0 audio]

This patch cleans up the save_flags()/cli() craziness in the OSS portion of the i8x0 audio driver.

Apply to ChangeSet 1.524

Built, booted, and played some tunes.

Regards,
Andy

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:

#	           ChangeSet	1.525   -> 1.527  
#	   sound/oss/cmpci.c	1.13    -> 1.14   
#	  sound/oss/dmabuf.c	1.2     -> 1.3    
#	sound/oss/sound_timer.c	1.2     -> 1.3    
#	   sound/oss/audio.c	1.3     -> 1.4    
#	 sound/oss/midibuf.c	1.4     -> 1.5    
#	sound/oss/sys_timer.c	1.2     -> 1.3    
#	sound/oss/soundcard.c	1.9     -> 1.10   
#	sound/oss/sequencer.c	1.3     -> 1.4

# sound/oss/midi_synth.c 1.2 -> 1.3
# sound/oss/mpu401.c 1.5 -> 1.6
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 02/07/29 [email protected] 1.526
# Compilation fixes:
# save_flags() --> local_save_flags()
# restore_flags() --> local_irq_restore()
# cli() --> local_irq_disable()
# sti() --> local_irq_enable()
# --------------------------------------------
# 02/07/30 [email protected] 1.527
# Compilation fixes:
# save_flags() --> local_save_flags()
# restore_flags() --> local_irq_restore()
# cli() --> local_irq_disable()
# sti() --> local_irq_enable()
# --------------------------------------------
#

sound/oss/audio.c +18 -18
diff -Nru a/sound/oss/audio.c b/sound/oss/audio.c
@@ -294,7 +294,7 @@
 			/*
 			 * This just allows interrupts while the conversion is running
 			 */
-			sti();
+			local_irq_enable();
 			translate_bytes(ulaw_dsp, (unsigned char *) dma_buf, l);
 		}
 		c -= used;

@@ -355,7 +355,7 @@
 			/*
 			 * This just allows interrupts while the conversion is running
 			 */
-			sti();
+			local_irq_enable();
 
 			translate_bytes(dsp_ulaw, (unsigned char *) dmabuf, l);
 		}

@@ -515,8 +515,8 @@
 				break;
 			}
 		
-			save_flags (flags);
-			cli();
+			local_save_flags (flags);
+			local_irq_disable();
 			/* Compute number of bytes that have been played */
 			count = DMAbuf_get_buffer_pointer (dev, dmap, DMODE_OUTPUT);
 			if (count < dmap->fragment_size && dmap->qhead != 0)

@@ -527,7 +527,7 @@
 			count = dmap->user_counter - count;
 			if (count < 0)
 				count = 0;
-			restore_flags (flags);
+			local_irq_restore (flags);
 			val = count;
 			break;
 		

@@ -836,15 +836,15 @@
 			if (!(audio_devs[dev]->flags & DMA_DUPLEX) && (bits & PCM_ENABLE_INPUT) &&
 				(bits & PCM_ENABLE_OUTPUT))
 				return -EINVAL;
-			save_flags(flags);
-			cli();
+			local_save_flags(flags);
+			local_irq_disable();
 			changed = audio_devs[dev]->enable_bits ^ bits;
 			if ((changed & bits) & PCM_ENABLE_INPUT && audio_devs[dev]->go) 
 			{
 				reorganize_buffers(dev, dmap_in, 1);
 				if ((err = audio_devs[dev]->d->prepare_for_input(dev,
 					     dmap_in->fragment_size, dmap_in->nbufs)) < 0) {
-					restore_flags(flags);
+					local_irq_restore(flags);
 					return -err;
 				}
 				dmap_in->dma_mode = DMODE_INPUT;

@@ -867,7 +867,7 @@
 			if (changed && audio_devs[dev]->d->trigger)
 				audio_devs[dev]->d->trigger(dev, bits * audio_devs[dev]->go);
 #endif				
-			restore_flags(flags);
+			local_irq_restore(flags);
 			/* Falls through... */
 
 		case SNDCTL_DSP_GETTRIGGER:

@@ -884,8 +884,8 @@
 		case SNDCTL_DSP_GETIPTR:
 			if (!(audio_devs[dev]->open_mode & OPEN_READ))
 				return -EINVAL;
-			save_flags(flags);
-			cli();
+			local_save_flags(flags);
+			local_irq_disable();
 			cinfo.bytes = dmap_in->byte_counter;
 			cinfo.ptr = DMAbuf_get_buffer_pointer(dev, dmap_in, DMODE_INPUT) & ~3;
 			if (cinfo.ptr < dmap_in->fragment_size && dmap_in->qtail != 0)

@@ -894,7 +894,7 @@
 			cinfo.bytes += cinfo.ptr;
 			if (dmap_in->mapping_flags & DMA_MAP_MAPPED)
 				dmap_in->qlen = 0;	/* Reset interrupt counter */
-			restore_flags(flags);
+			local_irq_restore(flags);
 			if (copy_to_user(arg, &cinfo, sizeof(cinfo)))
 				return -EFAULT;
 			return 0;

@@ -903,8 +903,8 @@
 			if (!(audio_devs[dev]->open_mode & OPEN_WRITE))
 				return -EINVAL;
 
-			save_flags(flags);
-			cli();
+			local_save_flags(flags);
+			local_irq_disable();
 			cinfo.bytes = dmap_out->byte_counter;
 			cinfo.ptr = DMAbuf_get_buffer_pointer(dev, dmap_out, DMODE_OUTPUT) & ~3;
 			if (cinfo.ptr < dmap_out->fragment_size && dmap_out->qhead != 0)

@@ -913,7 +913,7 @@
 			cinfo.bytes += cinfo.ptr;
 			if (dmap_out->mapping_flags & DMA_MAP_MAPPED)
 				dmap_out->qlen = 0;	/* Reset interrupt counter */
-			restore_flags(flags);
+			local_irq_restore(flags);
 			if (copy_to_user(arg, &cinfo, sizeof(cinfo)))
 				return -EFAULT;
 			return 0;

@@ -926,8 +926,8 @@
 				ret=0;
 				break;
 			}
-			save_flags(flags);
-			cli();
+			local_save_flags(flags);
+			local_irq_disable();
 			/* Compute number of bytes that have been played */
 			count = DMAbuf_get_buffer_pointer (dev, dmap_out, DMODE_OUTPUT);
 			if (count < dmap_out->fragment_size && dmap_out->qhead != 0)

@@ -937,7 +937,7 @@
 			count = dmap_out->user_counter - count;
 			if (count < 0)
 				count = 0;
-			restore_flags (flags);
+			local_irq_restore (flags);
 			ret = count;
 			break;
sound/oss/cmpci.c +3 -3
diff -Nru a/sound/oss/cmpci.c b/sound/oss/cmpci.c
@@ -1791,14 +1791,14 @@
         case SNDCTL_DSP_RESET:
 		if (file->f_mode & FMODE_WRITE) {
 			stop_dac(s);
-			synchronize_irq();
+			synchronize_irq(s->irq);
 			s->dma_dac.swptr = s->dma_dac.hwptr = s->dma_dac.count = s->dma_dac.total_bytes = 0;
 			if (s->status & DO_DUAL_DAC)
 				s->dma_adc.swptr = s->dma_adc.hwptr = s->dma_adc.count = s->dma_adc.total_bytes = 0;
 		}
 		if (file->f_mode & FMODE_READ) {
 			stop_adc(s);
-			synchronize_irq();
+			synchronize_irq(s->irq);
 			s->dma_adc.swptr = s->dma_adc.hwptr = s->dma_adc.count = s->dma_adc.total_bytes = 0;
 		}
 		return 0;

@@ -3166,7 +3166,7 @@
 	while ((s = devs)) {
 		devs = devs->next;
 		outb(0, s->iobase + CODEC_CMI_INT_HLDCLR + 2);  /* disable ints */
-		synchronize_irq();
+		synchronize_irq(s->irq);
 		outb(0, s->iobase + CODEC_CMI_FUNCTRL0 + 2); /* disable channels */
 		free_irq(s->irq, s);
sound/oss/dmabuf.c +48 -48

280 lines not on this page · raw message

sound/oss/midi_synth.c +3 -3
diff -Nru a/sound/oss/midi_synth.c b/sound/oss/midi_synth.c
@@ -433,14 +433,14 @@
 		return err;
 	inc = &midi_devs[orig_dev]->in_info;
 
-	save_flags(flags);
-	cli();
+	local_save_flags(flags);
+	local_irq_disable();
 	inc->m_busy = 0;
 	inc->m_state = MST_INIT;
 	inc->m_ptr = 0;
 	inc->m_left = 0;
 	inc->m_prev_status = 0x00;
-	restore_flags(flags);
+	local_irq_restore(flags);
 
 	return 1;
 }
sound/oss/midibuf.c +18 -18
diff -Nru a/sound/oss/midibuf.c b/sound/oss/midibuf.c
@@ -63,20 +63,20 @@
 	if (SPACE_AVAIL(q)) \
 	{ \
 	  unsigned long flags; \
-	  save_flags( flags);cli(); \
+	  local_save_flags( flags);local_irq_disable(); \
 	  q->queue[q->tail] = (data); \
 	  q->len++; q->tail = (q->tail+1) % MAX_QUEUE_SIZE; \
-	  restore_flags(flags); \
+	  local_irq_restore(flags); \
 	}
 
 #define REMOVE_BYTE(q, data) \
 	if (DATA_AVAIL(q)) \
 	{ \
 	  unsigned long flags; \
-	  save_flags( flags);cli(); \
+	  local_save_flags( flags);local_irq_disable(); \
 	  data = q->queue[q->head]; \
 	  q->len--; q->head = (q->head+1) % MAX_QUEUE_SIZE; \
-	  restore_flags(flags); \
+	  local_irq_restore(flags); \
 	}
 
 static void drain_midi_queue(int dev)

@@ -122,8 +122,8 @@
 	unsigned long   flags;
 	int             dev;
 
-	save_flags(flags);
-	cli();
+	local_save_flags(flags);
+	local_irq_disable();
 	if (open_devs)
 	{
 		for (dev = 0; dev < num_midis; dev++)

@@ -135,9 +135,9 @@
 				{
 					int c = midi_out_buf[dev]->queue[midi_out_buf[dev]->head];
 
-					restore_flags(flags);	/* Give some time to others */
+					local_irq_restore(flags);	/* Give some time to others */
 					ok = midi_devs[dev]->outputc(dev, c);
-					cli();
+					local_irq_disable();
 					midi_out_buf[dev]->head = (midi_out_buf[dev]->head + 1) % MAX_QUEUE_SIZE;
 					midi_out_buf[dev]->len--;
 				}

@@ -151,7 +151,7 @@
 		 * Come back later
 		 */
 	}
-	restore_flags(flags);
+	local_irq_restore(flags);
 }
 
 int MIDIbuf_open(int dev, struct file *file)

@@ -225,8 +225,8 @@
 	if (dev < 0 || dev >= num_midis || midi_devs[dev] == NULL)
 		return;
 
-	save_flags(flags);
-	cli();
+	local_save_flags(flags);
+	local_irq_disable();
 
 	/*
 	 * Wait until the queue is empty

@@ -249,7 +249,7 @@
 					 * Ensure the output queues are empty
 					 */
 	}
-	restore_flags(flags);
+	local_irq_restore(flags);
 
 	midi_devs[dev]->close(dev);
 

@@ -276,8 +276,8 @@
 	if (!count)
 		return 0;
 
-	save_flags(flags);
-	cli();
+	local_save_flags(flags);
+	local_irq_disable();
 
 	c = 0;
 

@@ -317,7 +317,7 @@
 		}
 	}
 out:
-	restore_flags(flags);
+	local_irq_restore(flags);
 	return c;
 }
 

@@ -330,8 +330,8 @@
 
 	dev = dev >> 4;
 
-	save_flags(flags);
-	cli();
+	local_save_flags(flags);
+	local_irq_disable();
 
 	if (!DATA_AVAIL(midi_in_buf[dev])) {	/*
 						 * No data yet, wait

@@ -369,7 +369,7 @@
 		}
 	}
 out:
-	restore_flags(flags);
+	local_irq_restore(flags);
 	return c;
 }
sound/oss/mpu401.c +30 -30

165 lines not on this page · raw message

sound/oss/sequencer.c +30 -30

160 lines not on this page · raw message

sound/oss/sound_timer.c +3 -3
diff -Nru a/sound/oss/sound_timer.c b/sound/oss/sound_timer.c
@@ -80,15 +80,15 @@
 {
 	unsigned long   flags;
 
-	save_flags(flags);
-	cli();
+	local_save_flags(flags);
+	local_irq_disable();
 	tmr_offs = 0;
 	ticks_offs = 0;
 	tmr_ctr = 0;
 	next_event_time = (unsigned long) -1;
 	prev_event_time = 0;
 	curr_ticks = 0;
-	restore_flags(flags);
+	local_irq_restore(flags);
 }
 
 static int timer_open(int dev, int mode)
sound/oss/soundcard.c +8 -8
diff -Nru a/sound/oss/soundcard.c b/sound/oss/soundcard.c
@@ -664,16 +664,16 @@
 		printk(KERN_ERR "sound_open_dma: Invalid DMA channel %d\n", chn);
 		return 1;
 	}
-	save_flags(flags);
-	cli();
+	local_save_flags(flags);
+	local_irq_disable();
 
 	if (dma_alloc_map[chn] != DMA_MAP_FREE) {
 		printk("sound_open_dma: DMA channel %d busy or not allocated (%d)\n", chn, dma_alloc_map[chn]);
-		restore_flags(flags);
+		local_irq_restore(flags);
 		return 1;
 	}
 	dma_alloc_map[chn] = DMA_MAP_BUSY;
-	restore_flags(flags);
+	local_irq_restore(flags);
 	return 0;
 }
 

@@ -691,16 +691,16 @@
 {
 	unsigned long   flags;
 
-	save_flags(flags);
-	cli();
+	local_save_flags(flags);
+	local_irq_disable();
 
 	if (dma_alloc_map[chn] != DMA_MAP_BUSY) {
 		printk(KERN_ERR "sound_close_dma: Bad access to DMA channel %d\n", chn);
-		restore_flags(flags);
+		local_irq_restore(flags);
 		return;
 	}
 	dma_alloc_map[chn] = DMA_MAP_FREE;
-	restore_flags(flags);
+	local_irq_restore(flags);
 }
 
 static void do_sequencer_timer(unsigned long dummy)
sound/oss/sys_timer.c +3 -3
diff -Nru a/sound/oss/sys_timer.c b/sound/oss/sys_timer.c
@@ -79,15 +79,15 @@
 {
 	unsigned long   flags;
 
-	save_flags(flags);
-	cli();
+	local_save_flags(flags);
+	local_irq_disable();
 	tmr_offs = 0;
 	ticks_offs = 0;
 	tmr_ctr = 0;
 	next_event_time = (unsigned long) -1;
 	prev_event_time = 0;
 	curr_ticks = 0;
-	restore_flags(flags);
+	local_irq_restore(flags);
 }
 
 static int
#1 lore raw
Quoted 6 lines · Andy Pfiffer

This patch cleans up the save_flags()/cli() craziness in the OSS portion of the i8x0 audio driver.

Apply to ChangeSet 1.524

Built, booted, and played some tunes.

But not it appears on an SMP system

author
#2 lore raw
Quoted 9 lines · Alan Cox
Quoted 6 lines · Andy Pfiffer

This patch cleans up the save_flags()/cli() craziness in the OSS portion of the i8x0 audio driver.

Apply to ChangeSet 1.524

Built, booted, and played some tunes.

But not it appears on an SMP system

As luck would have it, I booted the kernel and played the tunes on a 2-way P4 Xeon system. Nothing wedged, but then again, I didn't try to break it.

So, what did I break?

Andy

#3 lore raw
Quoted 5 lines · Andy Pfiffer

As luck would have it, I booted the kernel and played the tunes on a 2-way P4 Xeon system. Nothing wedged, but then again, I didn't try to break it.

So, what did I break?

SMP support I think. A lot of the save_flags/cli stuff you changed looks like it needs to also lock out interrupts on the other processors, otherwise you can get a DMA pointer being updated under you.

author
#4 lore raw
Quoted 11 lines · Alan Cox
Quoted 5 lines · Andy Pfiffer

As luck would have it, I booted the kernel and played the tunes on a 2-way P4 Xeon system. Nothing wedged, but then again, I didn't try to break it.

So, what did I break?

SMP support I think. A lot of the save_flags/cli stuff you changed looks like it needs to also lock out interrupts on the other processors, otherwise you can get a DMA pointer being updated under you.

Thanks. I'll take a look at it. --Andy

author
#5 lore raw
In reply to Alan Cox
Quoted 10 lines · Alan Cox
Quoted 5 lines · Andy Pfiffer

As luck would have it, I booted the kernel and played the tunes on a 2-way P4 Xeon system. Nothing wedged, but then again, I didn't try to break it.

So, what did I break?

SMP support I think. A lot of the save_flags/cli stuff you changed looks like it needs to also lock out interrupts on the other processors, otherwise you can get a DMA pointer being updated under you.

After reading the code in question, it appears to me that the existing save_flags/cli constructs are being used to atomically query/modify elements of an audio_devs[N] entry. I can see how it might be possible for the patch to break SMP.

Would a spinlock_t for each "struct audio_operations" be the preferred solution over, say, a global spinlock_t for all "struct audio_operations?"

And while I'm asking: I'm a bit perplexed by the "naked" sti()'s in audio_write() and audio_read() for the case where CNV_MU_LAW conversion is required. The intent appears to be to force interrupts back on during the format conversion. I don't see any corresponding cli() on the return path, however.

Does anyone have any idea why those sti()'s just seem to be stuck there?

Regards,
Andy

#6 lore raw
Quoted 4 lines · Andy Pfiffer

After reading the code in question, it appears to me that the existing save_flags/cli constructs are being used to atomically query/modify elements of an audio_devs[N] entry. I can see how it might be possible for the patch to break SMP.

The cli() is doing a lock across all processors. The local_irqsave won't protect other processors against the modifications. The ISA side of the OSS audio world is midbogglingly ugly for this and for lock_kernel horrors but the PCI side isnt too bad.

The DMA pointers in ISA OSS are also touched by interrupt handling code
- which again cli() locked across all processors and lock_irq* doesn't.

Rusty submitted a much better patch for this - deleting all the old OSS ISA drivers, so that people can just fix and use the ALSA code instead. I'm not saying don't analyse the locks and interrupt paths and fix the OSS audio for ISA stuff, just that it might not be the best use of time even if you do get it sorted out.

#7 lore raw
Quoted 5 lines · Alan Cox

Rusty submitted a much better patch for this - deleting all the old OSS ISA drivers, so that people can just fix and use the ALSA code instead. I'm not saying don't analyse the locks and interrupt paths and fix the OSS audio for ISA stuff, just that it might not be the best use of time even if you do get it sorted out.

Are there any OSS drivers for any particular cards for which we don't have an equivalent ALSA driver ? If we're ultimately going to be dropping any of the OSS drivers, I'd rather know about it so I don't waste time pushing the ~200kb of patches in that area I'm currently carrying towards Linus. (Given that most of them don't compile right now due to the collateral damage from the cli() etc changes , I'd *love* to take the lazy^Weasy option and just drop them)

Dave

Signature · 2 lines
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs
#8 lore raw
Quoted 7 lines · Dave Jones

Are there any OSS drivers for any particular cards for which we don't have an equivalent ALSA driver ? If we're ultimately going to be dropping any of the OSS drivers, I'd rather know about it so I don't waste time pushing the ~200kb of patches in that area I'm currently carrying towards Linus. (Given that most of them don't compile right now due to the collateral damage from the cli() etc changes , I'd *love* to take the lazy^Weasy option and just drop them)

ALSA should have complete coverage of everything but some weird corner cases like the bose speaker setup. For those its going to be far better to fix ALSA (and plugging a new isa card into alsa is really easy) than lug the entire OSS mess around for it.

#9 lore raw
In reply to Andy Pfiffer
Quoted 33 lines · Andy Pfiffer
Quoted 10 lines · Alan Cox
Quoted 5 lines · Andy Pfiffer

As luck would have it, I booted the kernel and played the tunes on a 2-way P4 Xeon system. Nothing wedged, but then again, I didn't try to break it.

So, what did I break?

SMP support I think. A lot of the save_flags/cli stuff you changed looks like it needs to also lock out interrupts on the other processors, otherwise you can get a DMA pointer being updated under you.

After reading the code in question, it appears to me that the existing save_flags/cli constructs are being used to atomically query/modify elements of an audio_devs[N] entry. I can see how it might be possible for the patch to break SMP.

Would a spinlock_t for each "struct audio_operations" be the preferred solution over, say, a global spinlock_t for all "struct audio_operations?"

And while I'm asking: I'm a bit perplexed by the "naked" sti()'s in audio_write() and audio_read() for the case where CNV_MU_LAW conversion is required. The intent appears to be to force interrupts back on during the format conversion. I don't see any corresponding cli() on the return path, however.

Does anyone have any idea why those sti()'s just seem to be stuck there?

Regards,
Andy