[CFT/PATCH] give sound/oss/trident a holiday cleanup for 2.6

[CFT/PATCH] give sound/oss/trident a holiday cleanup for 2.6

[Muli Ben-Yehuda]

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

I've had a bit of time on my hands in the last few days, and gave
sound/oss/trident.c a much needed cleanup. The changes are mostly
cleanups, a few bug fixes where we wouldn't do cleanup properly on
error paths, and the removal of lock_kernel()/unlock_kernel().

If anyone who has this sound card wishes to take the driver for a
spin, I'd love bug/success reports. Especially on SMP and/or machines
with multiple trident cards. Andrew, if you feel it's appropriate for
-mm, please include it. 

The patch against 2.6.0 is rather large. 140kb unzipped, 30kb
gzipped. It's available at
http://www.mulix.org/patches/trident-cleanup-B1-2.6.0.diff (gzipped:
http://www.mulix.org/patches/trident-cleanup-B1-2.6.0.diff.gz)

Summary of changes: 

- run the code through Lindent, and then fix it manually (this is the
bulk of the patch) 
- switch lock_set_fmt() and unlock_set_fmt() from macros to inline
functions. Macros that call return() are EVIL. 
- simplify lock_set_fmt() and implement it via test_and_set_bit(). 
- fix a bug wherein we would do an up() on a semaphore that hasn't
been down()ed if a signal happened after timeout in trident_write()
- fix a bug where we would not release the open_sem on OOM
- make the arguments for prog_dmabuf clearer (int -> enum) 
- fix a bug where we would call VALIDATE_STATE after
lock_kernel(). Since VALIDATE_STATE does 'return' if validation fails,
bad things can happen. Thanks to Dawson Engler <engler@stanford.edu>
and the Stanford checker for spotting.
- remove the calls to lock_kernel() from trident_release() and
trident_mmap(). trident_release() appears to be covered by the
open_sem, and trident_mmap() is covered by state->sem. 
- s/TRUE/1/, s/FALSE/0/

Cheers, 
Muli 
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

"the nucleus of linux oscillates my world" - gccbot@#offtopic


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

Re: [CFT/PATCH] give sound/oss/trident a holiday cleanup for 2.6

[Linus Torvalds]

On Mon, 29 Dec 2003, Muli Ben-Yehuda wrote:
> 
> Summary of changes: 
> 
> - run the code through Lindent, and then fix it manually (this is the
> bulk of the patch) 

When doing things like this, can you split up the patches into two 
separate things: one that _only_ does whitespace changes, and that is 
guaranteed not to change anything else, and another that does the rest.

It's a total b*tch to try to figure out which change resulted in some 
difference, if the changes are intermixed with large whitespace cleanups.

		Linus

Re: [CFT/PATCH] give sound/oss/trident a holiday cleanup for 2.6

[Mike Fedyk]

On Mon, Dec 29, 2003 at 08:38:47PM +0200, Muli Ben-Yehuda wrote:
> - run the code through Lindent, and then fix it manually (this is the
> bulk of the patch) 

Any chance you could split the changes before and after the lindent?

Re: [CFT/PATCH] give sound/oss/trident a holiday cleanup for 2.6

[Muli Ben-Yehuda]

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

On Mon, Dec 29, 2003 at 10:50:46AM -0800, Linus Torvalds wrote:

> > - run the code through Lindent, and then fix it manually (this is the
> > bulk of the patch) 
> 
> When doing things like this, can you split up the patches into two 
> separate things: one that _only_ does whitespace changes, and that is 
> guaranteed not to change anything else, and another that does the
> rest.

You're 100% right. Internally, the patch I sent is composed of 30
different patches. The reason I didn't seperate it into two patches is
that the changes were interleaved and inter-dependant and seperating
them was a b*tch. If Andrew wishes to include it in -mm or you wish to
include it in -vanilla and would like it in two seperate patches, I'll
go back and redo it that way. 

Thanks and cheers, 
Muli 
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

"the nucleus of linux oscillates my world" - gccbot@#offtopic


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

Re: [CFT/PATCH] give sound/oss/trident a holiday cleanup for 2.6

[Jeff Garzik]

Muli Ben-Yehuda wrote:
> You're 100% right. Internally, the patch I sent is composed of 30
> different patches. The reason I didn't seperate it into two patches is
> that the changes were interleaved and inter-dependant and seperating
> them was a b*tch. If Andrew wishes to include it in -mm or you wish to
> include it in -vanilla and would like it in two seperate patches, I'll
> go back and redo it that way. 


Thirty separate patches is OK.

We have scripts to handle "patchbombs".

	Jeff

Re: [CFT/PATCH] give sound/oss/trident a holiday cleanup for 2.6

[Linus Torvalds]

On Mon, 29 Dec 2003, Jeff Garzik wrote:
> 
> Thirty separate patches is OK.
> 
> We have scripts to handle "patchbombs".

Yes and no.

Thirty separate patches make sense if they are independent and really do 
conceptually different things. Then it makes sense to have them as 
separate checkins, and be able to tell people "ok, try undoing that one, 
maybe that's the problem".

However, if they are all just "fix silly bugs in xxx", then I'd much 
rather see it as one big patch. Having it split up into "fix bug on line 
50" and "fix bug on line 75" just doesn't make any sense - it only makes 
the patch history harder to follow.

So "many small patches" aren't automatically any better than one big one. 
The thing that matters is to keep things _conceptually_ separated. If one 
patch fixes whitespace, and another one fixes bugs, then that's good.

		Linus

Re: [CFT/PATCH] give sound/oss/trident a holiday cleanup for 2.6

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Mon, 29 Dec 2003, Jeff Garzik wrote:
> 
>>Thirty separate patches is OK.
>>
>>We have scripts to handle "patchbombs".
> 
> 
> Yes and no.
> 
> Thirty separate patches make sense if they are independent and really do 
> conceptually different things. Then it makes sense to have them as 
> separate checkins, and be able to tell people "ok, try undoing that one, 
> maybe that's the problem".
> 
> However, if they are all just "fix silly bugs in xxx", then I'd much 
> rather see it as one big patch. Having it split up into "fix bug on line 
> 50" and "fix bug on line 75" just doesn't make any sense - it only makes 
> the patch history harder to follow.


There's certainly a middle ground.  For drivers I generally request that 
bug fixes for separate bugs be split up, since inevitably one bug fix 
out of twenty breaks for somebody on that somebody's weird hardware.

	Jeff

Re: [CFT/PATCH] give sound/oss/trident a holiday cleanup for 2.6

[Muli Ben-Yehuda]

On Mon, Dec 29, 2003 at 10:50:46AM -0800, Linus Torvalds wrote:

> On Mon, 29 Dec 2003, Muli Ben-Yehuda wrote:

[trident OSS driver holiday cleanup] 

> > Summary of changes: 
> > 
> > - run the code through Lindent, and then fix it manually (this is the
> > bulk of the patch) 
> 
> When doing things like this, can you split up the patches into two 
> separate things: one that _only_ does whitespace changes, and that is 
> guaranteed not to change anything else, and another that does the rest.

Ok, I've done this now. The indentation patch is 137k, and is
available at
http://www.mulix.org/patches/trident-cleanup-indentation-D1-2.6.0.patch
(gzipped:
http://www.mulix.org/patches/trident-cleanup-indentation-D1-2.6.0.patch.gz).

All of the non-indentation changes are in the
trident-cleanup-fixes-D1-2.6.0 patch, attached here inline. It needs
the indentation patch to be applied before it to apply
cleanly. Compiles, boots and plays music fine. Patch is against
2.6.0. Andrew, please add these two patches to -mm1 instead of the
"humongopatch" currently there. Thanks! 

diff -Naur -X /home/muli/w/dontdiff 2.6.0/sound/oss/trident.c 2.6.0-trident/sound/oss/trident.c
--- 2.6.0/sound/oss/trident.c	Fri Jan  2 01:12:19 2004
+++ 2.6.0-trident/sound/oss/trident.c	Fri Jan  2 01:12:49 2004
@@ -1,5 +1,5 @@
 /*
- *	OSS driver for Linux 2.4.x for
+ *	OSS driver for Linux 2.[46].x for
  *
  *	Trident 4D-Wave
  *	SiS 7018
@@ -37,6 +37,11 @@
  *	Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  *
  *  History
+ *  v0.14.10i 
+ *      December 29 2003 Muli Ben-Yehuda <mulix@mulix.org> 
+ *      major cleanup for 2.6, fix a few error patch buglets 
+ *      with returning without properly cleaning up first, 
+ *      get rid of lock_kernel(). 
  *  v0.14.10h
  *	Sept 10 2002 Pascal Schmidt <der.eremit@email.de>
  *	added support for ALi 5451 joystick port
@@ -218,7 +223,7 @@
 
 #include "trident.h"
 
-#define DRIVER_VERSION "0.14.10h-2.5"
+#define DRIVER_VERSION "0.14.10i-2.6"
 
 /* magic numbers to protect our data structures */
 #define TRIDENT_CARD_MAGIC	0x5072696E	/* "Prin" */
@@ -337,7 +342,7 @@
 	struct trident_state *other_states[4];
 	int multi_channels_adjust_count;
 	unsigned chans_num;
-	unsigned fmt_flag:1;
+	unsigned long fmt_flag;
 	/* Guard against mmap/write/read races */
 	struct semaphore sem;
 
@@ -436,6 +441,11 @@
 	struct gameport gameport;
 };
 
+enum dmabuf_mode { 
+	DM_PLAYBACK = 0, 
+	DM_RECORD
+}; 
+
 /* table to map from CHANNELMASK to channel attribute for SiS 7018 */
 static u16 mask2attr[] = {
 	PCM_LR, PCM_LR, SURR_LR, CENTER_LFE,
@@ -504,21 +514,18 @@
 	(copy_count) += (offset); \
 } while (0)
 
-#define lock_set_fmt(state) do { \
-        spin_lock_irqsave(&state->card->lock, flags);			\
-	if (state->fmt_flag) {						\
-	       spin_unlock_irqrestore(&state->card->lock, flags);	\
-               return -EFAULT;					        \
-	}								\
-	state->fmt_flag = 1;						\
-	spin_unlock_irqrestore(&state->card->lock, flags);              \
-} while (0)
+static inline int lock_set_fmt(struct trident_state* state)
+{
+	if (test_and_set_bit(0, &state->fmt_flag))
+		return -EFAULT; 
 
-#define unlock_set_fmt(state)  do {                             \
-        spin_lock_irqsave(&state->card->lock, flags);		\
-	state->fmt_flag = 0;					\
-	spin_unlock_irqrestore(&state->card->lock, flags);      \
-} while (0)
+	return 0; 
+}
+
+static inline void unlock_set_fmt(struct trident_state* state)
+{
+	clear_bit(0, &state->fmt_flag); 
+}
 
 static int
 trident_enable_loop_interrupts(struct trident_card *card)
@@ -538,7 +545,7 @@
 		global_control |= (ENDLP_IE | MIDLP_IE);
 		break;
 	default:
-		return FALSE;
+		return 0;
 	}
 
 	outl(global_control, TRID_REG(card, T4D_LFO_GC_CIR));
@@ -546,7 +553,7 @@
 	TRDBG("trident: Enable Loop Interrupts, globctl = 0x%08X\n", 
 	      inl(TRID_REG(card, T4D_LFO_GC_CIR)));
 
-	return (TRUE);
+	return 1;
 }
 
 static int
@@ -561,7 +568,7 @@
 	TRDBG("trident: Disabled Loop Interrupts, globctl = 0x%08X\n", 
 	      global_control);
 
-	return (TRUE);
+	return 1;
 }
 
 static void
@@ -663,11 +670,10 @@
 
 #ifdef DEBUG
 	if (reg & mask)
-		TRDBG("trident: channel %d has interrupt, %s = 0x%08x\n", 
-		      channel, reg == T4D_AINT_B ? "AINT_B" : "AINT_A", 
-		      reg);
-#endif /* DEBUG */
-	return (reg & mask) ? TRUE : FALSE;
+		TRDBG("trident: channel %d has interrupt, %s = 0x%08x\n",
+		      channel, reg == T4D_AINT_B ? "AINT_B" : "AINT_A", reg);
+#endif /* DEBUG */ 
+	return (reg & mask) ? 1 : 0;
 }
 
 static void
@@ -830,7 +836,7 @@
 	int i;
 
 	if (channel > 63)
-		return FALSE;
+		return 0;
 
 	/* select hardware channel to write */
 	outb(channel, TRID_REG(card, T4D_LFO_GC_CIR));
@@ -847,7 +853,7 @@
 		outl(ALI_EMOD_Still, TRID_REG(card, ALI_EBUF1));
 		outl(ALI_EMOD_Still, TRID_REG(card, ALI_EBUF2));
 	}
-	return TRUE;
+	return 1;
 }
 
 /* called with spin lock held */
@@ -886,7 +892,7 @@
 		data[3] = channel->fm_vol & 0xffff;
 		break;
 	default:
-		return FALSE;
+		return 0;
 	}
 
 	return trident_load_channel_registers(state->card, data, channel->num);
@@ -1315,16 +1321,18 @@
 }
 
 static int
-prog_dmabuf(struct trident_state *state, unsigned rec)
+prog_dmabuf(struct trident_state *state, enum dmabuf_mode rec)
 {
 	struct dmabuf *dmabuf = &state->dmabuf;
 	unsigned bytepersec;
 	struct trident_state *s = state;
 	unsigned bufsize, dma_nums;
 	unsigned long flags;
-	int ret, i, order;
+	int ret, i, order; 
+
+	if ((ret = lock_set_fmt(state)) < 0)
+		return ret; 
 
-	lock_set_fmt(state);
 	if (state->chans_num == 6)
 		dma_nums = 5;
 	else
@@ -1352,8 +1360,11 @@
 				}
 			} else {
 				ret = -ENOMEM;
-				if ((order = state->dmabuf.buforder - 1) >= DMABUF_MINORDER) {
-					ret = alloc_dmabuf(dmabuf, state->card->pci_dev, order);
+				order = state->dmabuf.buforder - 1; 
+				if (order >= DMABUF_MINORDER) {
+					ret = alloc_dmabuf(dmabuf,
+							   state->card->pci_dev,
+							   order);
 				}
 				if (ret) {
 					/* release the main DMA buffer */
@@ -1394,9 +1405,9 @@
 		       dmabuf->dmasize);
 
 		spin_lock_irqsave(&s->card->lock, flags);
-		if (rec)
+		if (rec == DM_RECORD)
 			trident_rec_setup(s);
-		else
+		else /* DM_PLAYBACK */ 
 			trident_play_setup(s);
 
 		spin_unlock_irqrestore(&s->card->lock, flags);
@@ -1414,6 +1425,17 @@
 	return 0;
 }
 
+
+static inline int prog_dmabuf_record(struct trident_state* state)
+{
+	return prog_dmabuf(state, DM_RECORD); 
+}
+
+static inline int prog_dmabuf_playback(struct trident_state* state)
+{
+	return prog_dmabuf(state, DM_PLAYBACK); 
+}
+
 /* we are doing quantum mechanics here, the buffer can only be empty, half or full filled i.e.
    |------------|------------|   or   |xxxxxxxxxxxx|------------|   or   |xxxxxxxxxxxx|xxxxxxxxxxxx|
    but we almost always get this
@@ -1854,7 +1876,7 @@
 		return -EFAULT;
 
 	down(&state->sem);
-	if (!dmabuf->ready && (ret = prog_dmabuf(state, 1)))
+	if (!dmabuf->ready && (ret = prog_dmabuf_record(state)))
 		goto out;
 
 	while (count > 0) {
@@ -1958,6 +1980,7 @@
 	int cnt;
 	unsigned int state_cnt;
 	unsigned int copy_count;
+	int lret; /* for lock_set_fmt */ 
 
 	TRDBG("trident: trident_write called, count = %d\n", count);
 
@@ -1975,7 +1998,7 @@
 		ret = -ENXIO;
 		goto out;
 	}
-	if (!dmabuf->ready && (ret = prog_dmabuf(state, 0)))
+	if (!dmabuf->ready && (ret = prog_dmabuf_playback(state)))
 		goto out;
 
 	if (!access_ok(VERIFY_READ, buffer, count)) {
@@ -2044,7 +2067,7 @@
 			if (signal_pending(current)) {
 				if (!ret)
 					ret = -ERESTARTSYS;
-				goto out;
+				goto out_nolock;
 			}
 			down(&state->sem);
 			if (dmabuf->mapped) {
@@ -2054,7 +2077,11 @@
 			}
 			continue;
 		}
-		lock_set_fmt(state);
+		if ((lret = lock_set_fmt(state)) < 0) { 
+			ret = lret; 
+			goto out; 
+		}
+
 		if (state->chans_num == 6) {
 			copy_count = 0;
 			state_cnt = 0;
@@ -2101,6 +2128,7 @@
 	}
 out:
 	up(&state->sem);
+out_nolock: 
 	return ret;
 }
 
@@ -2123,14 +2151,14 @@
 	down(&state->sem);
 
 	if (file->f_mode & FMODE_WRITE) {
-		if (!dmabuf->ready && prog_dmabuf(state, 0)) {
+		if (!dmabuf->ready && prog_dmabuf_playback(state)) {
 			up(&state->sem);
 			return 0;
 		}
 		poll_wait(file, &dmabuf->wait, wait);
 	}
 	if (file->f_mode & FMODE_READ) {
-		if (!dmabuf->ready && prog_dmabuf(state, 1)) {
+		if (!dmabuf->ready && prog_dmabuf_record(state)) {
 			up(&state->sem);
 			return 0;
 		}
@@ -2169,7 +2197,6 @@
 	unsigned long size;
 
 	VALIDATE_STATE(state);
-	lock_kernel();
 
 	/*
 	 *      Lock against poll read write or mmap creating buffers. Also lock
@@ -2179,10 +2206,10 @@
 	down(&state->sem);
 
 	if (vma->vm_flags & VM_WRITE) {
-		if ((ret = prog_dmabuf(state, 0)) != 0)
+		if ((ret = prog_dmabuf_playback(state)) != 0)
 			goto out;
 	} else if (vma->vm_flags & VM_READ) {
-		if ((ret = prog_dmabuf(state, 1)) != 0)
+		if ((ret = prog_dmabuf_record(state)) != 0)
 			goto out;
 	} else
 		goto out;
@@ -2201,7 +2228,6 @@
 	ret = 0;
 out:
 	up(&state->sem);
-	unlock_kernel();
 	return ret;
 }
 
@@ -2219,9 +2245,11 @@
 	struct trident_card *card = state->card;
 
 	VALIDATE_STATE(state);
-	mapped = ((file->f_mode & FMODE_WRITE) && dmabuf->mapped) || 
-		((file->f_mode & FMODE_READ) && dmabuf->mapped);
-	TRDBG("trident: trident_ioctl, command = %2d, arg = 0x%08x\n", 
+
+	
+	mapped = ((file->f_mode & (FMODE_WRITE | FMODE_READ)) && dmabuf->mapped); 
+
+	TRDBG("trident: trident_ioctl, command = %2d, arg = 0x%08x\n",
 	      _IOC_NR(cmd), arg ? *(int *) arg : 0);
 
 	switch (cmd) {
@@ -2281,7 +2309,9 @@
 			ret = -EFAULT;
 			break;
 		}
-		lock_set_fmt(state);
+		if ((ret = lock_set_fmt(state)) < 0)
+			return ret; 
+
 		if (file->f_mode & FMODE_WRITE) {
 			stop_dac(state);
 			dmabuf->ready = 0;
@@ -2303,19 +2333,23 @@
 
 	case SNDCTL_DSP_GETBLKSIZE:
 		if (file->f_mode & FMODE_WRITE) {
-			if ((val = prog_dmabuf(state, 0)))
+			if ((val = prog_dmabuf_playback(state)))
 				ret = val;
 			else
 				ret = put_user(dmabuf->fragsize, (int *) arg);
 			break;
 		}
 		if (file->f_mode & FMODE_READ) {
-			if ((val = prog_dmabuf(state, 1)))
+			if ((val = prog_dmabuf_record(state)))
 				ret = val;
 			else
 				ret = put_user(dmabuf->fragsize, (int *) arg);
 			break;
 		}
+		/* neither READ nor WRITE? is this even possible? */ 
+		ret = -EINVAL; 
+		break; 
+
 
 	case SNDCTL_DSP_GETFMTS: /* Returns a mask of supported sample format */
 		ret = put_user(AFMT_S16_LE | AFMT_U16_LE | AFMT_S8 | 
@@ -2327,7 +2361,9 @@
 			ret = -EFAULT;
 			break;
 		}
-		lock_set_fmt(state);
+		if ((ret = lock_set_fmt(state)) < 0)
+			return ret; 
+
 		if (val != AFMT_QUERY) {
 			if (file->f_mode & FMODE_WRITE) {
 				stop_dac(state);
@@ -2357,7 +2393,9 @@
 			break;
 		}
 		if (val != 0) {
-			lock_set_fmt(state);
+			if ((ret = lock_set_fmt(state)) < 0)
+				return ret; 
+
 			if (file->f_mode & FMODE_WRITE) {
 				stop_dac(state);
 				dmabuf->ready = 0;
@@ -2459,7 +2497,7 @@
 			ret = -EINVAL;
 			break;
 		}
-		if (!dmabuf->ready && (val = prog_dmabuf(state, 0)) != 0) {
+		if (!dmabuf->ready && (val = prog_dmabuf_playback(state)) != 0) {
 			ret = val;
 			break;
 		}
@@ -2479,7 +2517,7 @@
 			ret = -EINVAL;
 			break;
 		}
-		if (!dmabuf->ready && (val = prog_dmabuf(state, 1)) != 0) {
+		if (!dmabuf->ready && (val = prog_dmabuf_record(state)) != 0) {
 			ret = val;
 			break;
 		}
@@ -2520,7 +2558,7 @@
 		if (file->f_mode & FMODE_READ) {
 			if (val & PCM_ENABLE_INPUT) {
 				if (!dmabuf->ready && 
-				    (ret = prog_dmabuf(state, 1)))
+				    (ret = prog_dmabuf_record(state)))
 					break;
 				start_adc(state);
 			} else
@@ -2529,7 +2567,7 @@
 		if (file->f_mode & FMODE_WRITE) {
 			if (val & PCM_ENABLE_OUTPUT) {
 				if (!dmabuf->ready && 
-				    (ret = prog_dmabuf(state, 0)))
+				    (ret = prog_dmabuf_playback(state)))
 					break;
 				start_dac(state);
 			} else
@@ -2542,7 +2580,7 @@
 			ret = -EINVAL;
 			break;
 		}
-		if (!dmabuf->ready && (val = prog_dmabuf(state, 1)) 
+		if (!dmabuf->ready && (val = prog_dmabuf_record(state)) 
 		    != 0) {
 			ret = val;
 			break;
@@ -2564,7 +2602,7 @@
 			ret = -EINVAL;
 			break;
 		}
-		if (!dmabuf->ready && (val = prog_dmabuf(state, 0)) 
+		if (!dmabuf->ready && (val = prog_dmabuf_playback(state)) 
 		    != 0) {
 			ret = val;
 			break;
@@ -2591,7 +2629,7 @@
 			ret = -EINVAL;
 			break;
 		}
-		if (!dmabuf->ready && (val = prog_dmabuf(state, 0)) != 0) {
+		if (!dmabuf->ready && (val = prog_dmabuf_playback(state)) != 0) {
 			ret = val;
 			break;
 		}
@@ -2670,6 +2708,10 @@
 	struct dmabuf *dmabuf = NULL;
 
 	/* Added by Matt Wu 01-05-2001 */
+	/* TODO: there's some redundacy here wrt the check below */ 
+	/* for multi_use_count > 0. Should we return -EBUSY or find */ 
+	/* a different card? for now, don't break current behaviour */ 
+	/* -- mulix */ 
 	if (file->f_mode & FMODE_READ) {
 		if (card->pci_id == PCI_DEVICE_ID_ALI_5451) {
 			if (card->multi_channel_use_count > 0)
@@ -2690,12 +2732,12 @@
 		}
 		for (i = 0; i < NR_HW_CH; i++) {
 			if (card->states[i] == NULL) {
-				state = card->states[i] = (struct trident_state *)
-				    kmalloc(sizeof (struct trident_state), GFP_KERNEL);
+				state = card->states[i] = kmalloc(sizeof(*state), GFP_KERNEL);
 				if (state == NULL) {
+					up(&card->open_sem); 
 					return -ENOMEM;
 				}
-				memset(state, 0, sizeof (struct trident_state));
+				memset(state, 0, sizeof(*state));
 				init_MUTEX(&state->sem);
 				dmabuf = &state->dmabuf;
 				goto found_virt;
@@ -2783,10 +2825,10 @@
 	struct trident_card *card;
 	struct dmabuf *dmabuf;
 
-	lock_kernel();
+	VALIDATE_STATE(state);
+
 	card = state->card;
 	dmabuf = &state->dmabuf;
-	VALIDATE_STATE(state);
 
 	if (file->f_mode & FMODE_WRITE) {
 		trident_clear_tail(state);
@@ -2832,7 +2874,6 @@
 
 	/* we're covered by the open_sem */
 	up(&card->open_sem);
-	unlock_kernel();
 
 	return 0;
 }
@@ -3543,56 +3584,64 @@
 	int i, state_count = 0;
 	struct trident_pcm_bank *bank;
 	struct trident_channel *channel;
+	unsigned long num; 
 
 	bank = &card->banks[BANK_A];
 
-	if (chan_nums == 6) {
-		for (i = 0; (i < ALI_CHANNELS) && (state_count != 4); i++) {
-			if (!card->states[i]) {
-				if (!(bank->bitmap & (1 << ali_multi_channels_5_1[state_count]))) {
-					bank->bitmap |= (1 << ali_multi_channels_5_1[state_count]);
-					channel = &bank->channels[ali_multi_channels_5_1[state_count]];
-					channel->num = ali_multi_channels_5_1[state_count];
-				} else {
-					state_count--;
-					for (; state_count >= 0; state_count--) {
-						kfree(state->other_states[state_count]);
-						ali_free_pcm_channel(card, ali_multi_channels_5_1[state_count]);
-					}
-					return -EBUSY;
-				}
-				s = card->states[i] = (struct trident_state *)
-				    kmalloc(sizeof (struct trident_state), GFP_KERNEL);
-				if (!s) {
-					ali_free_pcm_channel(card, ali_multi_channels_5_1[state_count]);
-					state_count--;
-					for (; state_count >= 0; state_count--) {
-						ali_free_pcm_channel(card, ali_multi_channels_5_1[state_count]);
-						kfree(state->other_states[state_count]);
-					}
-					return -ENOMEM;
-				}
-				memset(s, 0, sizeof (struct trident_state));
+	if (chan_nums != 6)
+		return 0; 
 
-				s->dmabuf.channel = channel;
-				s->dmabuf.ossfragshift = s->dmabuf.ossmaxfrags = s->dmabuf.subdivision = 0;
-				init_waitqueue_head(&s->dmabuf.wait);
-				s->magic = card->magic;
-				s->card = card;
-				s->virt = i;
-				ali_enable_special_channel(s);
-				state->other_states[state_count++] = s;
+	for (i = 0; (i < ALI_CHANNELS) && (state_count != 4); i++) {
+		if (card->states[i])
+			continue; 
+
+		num = ali_multi_channels_5_1[state_count]; 
+		if (!(bank->bitmap & (1 << num))) {
+			bank->bitmap |= 1 << num;
+			channel = &bank->channels[num];
+			channel->num = num; 
+		} else {
+			state_count--;
+			for (; state_count >= 0; state_count--) {
+				kfree(state->other_states[state_count]);
+				num = ali_multi_channels_5_1[state_count]; 
+					ali_free_pcm_channel(card, num);
 			}
+			return -EBUSY;
 		}
-
-		if (state_count != 4) {
+		s = card->states[i] = kmalloc(sizeof(*state), GFP_KERNEL);
+		if (!s) {
+			num = ali_multi_channels_5_1[state_count]; 
+			ali_free_pcm_channel(card, num); 
 			state_count--;
 			for (; state_count >= 0; state_count--) {
+				num = ali_multi_channels_5_1[state_count]; 
+				ali_free_pcm_channel(card, num); 
 				kfree(state->other_states[state_count]);
-				ali_free_pcm_channel(card, ali_multi_channels_5_1[state_count]);
 			}
-			return -EBUSY;
+			return -ENOMEM;
+		}
+		memset(s, 0, sizeof(*state)); 
+		
+		s->dmabuf.channel = channel;
+		s->dmabuf.ossfragshift = s->dmabuf.ossmaxfrags =
+			s->dmabuf.subdivision = 0;
+		init_waitqueue_head(&s->dmabuf.wait);
+		s->magic = card->magic;
+		s->card = card;
+		s->virt = i;
+		ali_enable_special_channel(s);
+		state->other_states[state_count++] = s;
+	}
+	
+	if (state_count != 4) {
+		state_count--;
+		for (; state_count >= 0; state_count--) {
+			kfree(state->other_states[state_count]);
+			num = ali_multi_channels_5_1[state_count]; 
+			ali_free_pcm_channel(card, num); 
 		}
+		return -EBUSY;
 	}
 	return 0;
 }
@@ -3918,8 +3967,8 @@
 				(*state_cnt) += sample_s;
 
 				if (cnt_for_multi_channel > 0) {
-					loop = state->multi_channels_adjust_count - 
-						(state->chans_num - other_dma_nums);
+					int diff = state->chans_num - other_dma_nums;
+					loop = state->multi_channels_adjust_count - diff; 
 					for (i = 0; i < loop; i++) {
 						dmabuf_temp = &state->other_states[i]->dmabuf;
 						if (copy_from_user(dmabuf_temp->rawbuf + 
@@ -4071,6 +4120,8 @@
 	pci_write_config_dword(pci_dev, 0x44, dwVal & 0xfffbffff);
 	udelay(5000);
 
+	/* TODO: recognize if we have a PM capable codec and only do this */ 
+	/* if the codec is PM capable */ 
 	wCount = 2000;
 	while (wCount--) {
 		wReg = ali_ac97_get(card, 0, AC97_POWER_CONTROL);
@@ -4169,7 +4220,8 @@
 		if (ac97_probe_codec(codec) == 0)
 			break;
 
-		if ((codec->dev_mixer = register_sound_mixer(&trident_mixer_fops, -1)) < 0) {
+		codec->dev_mixer = register_sound_mixer(&trident_mixer_fops, -1); 
+		if (codec->dev_mixer < 0) {
 			printk(KERN_ERR "trident: couldn't register mixer!\n");
 			ac97_release_codec(codec);
 			break;
@@ -4186,9 +4238,10 @@
 		for (num_ac97 = 0; num_ac97 < NR_AC97; num_ac97++) {
 			if (card->ac97_codec[num_ac97] == NULL)
 				break;
-			for (i = 0; i < 64; i++)
-				card->mixer_regs[i][num_ac97] = ali_ac97_get(card, num_ac97, 
-									     i * 2);
+			for (i = 0; i < 64; i++) { 
+				u16 reg = ali_ac97_get(card, num_ac97, i * 2);
+				card->mixer_regs[i][num_ac97] = reg;
+			}
 		}
 	}
 	return num_ac97 + 1;
@@ -4291,7 +4344,7 @@
 	}
 
 	rc = -ENOMEM;
-	if ((card = kmalloc(sizeof (struct trident_card), GFP_KERNEL)) == NULL) {
+	if ((card = kmalloc(sizeof(*card), GFP_KERNEL)) == NULL) {
 		printk(KERN_ERR "trident: out of memory\n");
 		goto out_release_region;
 	}
@@ -4400,8 +4453,9 @@
 		/* unregister audio devices */
 		for (i = 0; i < NR_AC97; i++) {
 			if (card->ac97_codec[i] != NULL) {
-				unregister_sound_mixer(card->ac97_codec[i]->dev_mixer);
-				ac97_release_codec(card->ac97_codec[i]);
+				struct ac97_codec* codec = card->ac97_codec[i]; 
+				unregister_sound_mixer(codec->dev_mixer);
+				ac97_release_codec(codec);
 			}
 		}
 		goto out_unregister_sound_dsp;
@@ -4514,9 +4568,9 @@
 	pci_set_drvdata(pci_dev, NULL);
 }
 
-MODULE_AUTHOR("Alan Cox, Aaron Holtzman, Ollie Lho, Ching Ling Lee");
-MODULE_DESCRIPTION("Trident 4DWave/SiS 7018/ALi 5451 and Tvia/IGST "
-		   "CyberPro5050 PCI Audio Driver");
+MODULE_AUTHOR("Alan Cox, Aaron Holtzman, Ollie Lho, Ching Ling Lee, Muli Ben-Yehuda");
+MODULE_DESCRIPTION("Trident 4DWave/SiS 7018/ALi 5451 and Tvia/IGST CyberPro5050 PCI "
+		   "Audio Driver");
 MODULE_LICENSE("GPL");
 
 #define TRIDENT_MODULE_NAME "trident"
diff -Naur -X /home/muli/w/dontdiff 2.6.0/sound/oss/trident.h 2.6.0-trident/sound/oss/trident.h
--- 2.6.0/sound/oss/trident.h	Fri Jan  2 01:12:19 2004
+++ 2.6.0-trident/sound/oss/trident.h	Fri Jan  2 00:52:13 2004
@@ -56,11 +56,6 @@
 #define PCI_DEVICE_ID_ALI_1533		0x1533
 #endif
 
-#ifndef FALSE
-#define FALSE 		0
-#define TRUE  		1
-#endif
-
 #define CHANNEL_REGS	5
 #define CHANNEL_START	0xe0   // The first bytes of the contiguous register space.
 
@@ -363,7 +358,7 @@
 #ifdef DEBUG
 
 #define TRDBG(msg, args...) do {          \
-        printk(KERN_DEBUG msg , ##args ); \
+        printk(DEBUG msg , ##args );      \
 } while (0)
 
 #else /* !defined(DEBUG) */ 


-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

"the nucleus of linux oscillates my world" - gccbot@#offtopic

Re: [CFT/PATCH] give sound/oss/trident a holiday cleanup for 2.6

[Andrew Morton]

Muli Ben-Yehuda <mulix@mulix.org> wrote:
>
> Ok, I've done this now. The indentation patch is 137k, and is
>  available at
>  http://www.mulix.org/patches/trident-cleanup-indentation-D1-2.6.0.patch
>  (gzipped:
>  http://www.mulix.org/patches/trident-cleanup-indentation-D1-2.6.0.patch.gz).

hmm, how come a whitespace cleanup patch adds nearly 200 lines which have
trailing whitespace?

>  All of the non-indentation changes are in the
>  trident-cleanup-fixes-D1-2.6.0 patch, attached here inline. It needs
>  the indentation patch to be applied before it to apply
>  cleanly. Compiles, boots and plays music fine. Patch is against
>  2.6.0. Andrew, please add these two patches to -mm1 instead of the
>  "humongopatch" currently there. Thanks! 

Could we please have a description of the substantive changes in
this patch?

Thanks.

Re: [CFT/PATCH] give sound/oss/trident a holiday cleanup for 2.6

[Muli Ben-Yehuda]

On Thu, Jan 01, 2004 at 04:04:20PM -0800, Andrew Morton wrote:

> hmm, how come a whitespace cleanup patch adds nearly 200 lines which have
> trailing whitespace?

That would be either xemacs's or indent's fault. Can't be my
fault. No sir. Anyway, unless whitespace-mode is lying to me now, no
line has more than at most one character of whitespace added. If it
bugs you, I'll clean it up - it's a slow night tonight ;-) 

> >  All of the non-indentation changes are in the
> >  trident-cleanup-fixes-D1-2.6.0 patch, attached here inline. It needs
> >  the indentation patch to be applied before it to apply
> >  cleanly. Compiles, boots and plays music fine. Patch is against
> >  2.6.0. Andrew, please add these two patches to -mm1 instead of the
> >  "humongopatch" currently there. Thanks! 
> 
> Could we please have a description of the substantive changes in
> this patch?

Sure thing: 

- switch lock_set_fmt() and unlock_set_fmt() from macros to inline
functions. Macros that call return() are EVIL.
- simplify lock_set_fmt() and implement it via test_and_set_bit()
rather than a spinlock protecting an int.
- fix a bug wherein we would do an up() on a semaphore that hasn't
been down()ed if a signal happened after timeout in trident_write().
- fix a bug where we would not release the open_sem on OOM.
- make the arguments for prog_dmabuf clearer (int -> enum), and add
two wrapper functions around it, one for record and one for playback. 
- fix a bug where we would call VALIDATE_STATE after
lock_kernel(). Since VALIDATE_STATE does 'return' if validation fails,
bad things can happen. Thanks to Dawson Engler <engler@stanford.edu>
and the Stanford checker for spotting.
- remove the calls to lock_kernel() from trident_release() and
trident_mmap(). trident_release() appears to be covered by the
open_sem, and trident_mmap() is covered by state->sem.
- s/TRUE/1/, s/FALSE/0/

> Thanks.

Entirely my pleasure. 

Cheers, 
Muli 
-- 
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/

"the nucleus of linux oscillates my world" - gccbot@#offtopic

Re: [CFT/PATCH] give sound/oss/trident a holiday cleanup for 2.6

[Andrew Morton]

Muli Ben-Yehuda <mulix@mulix.org> wrote:
>
> On Thu, Jan 01, 2004 at 04:04:20PM -0800, Andrew Morton wrote:
> 
> > hmm, how come a whitespace cleanup patch adds nearly 200 lines which have
> > trailing whitespace?
> 
> That would be either xemacs's or indent's fault. Can't be my
> fault. No sir. Anyway, unless whitespace-mode is lying to me now, no
> line has more than at most one character of whitespace added. If it
> bugs you, I'll clean it up - it's a slow night tonight ;-) 

Nah, leave it as is.  I'm just having a little whine.  I added a nifty
trailing-whitespace-detector to patch-scripts a while back and it's telling
me that a *lot* of people use broken editors.

> > Could we please have a description of the substantive changes in
> > this patch?
> 
> Sure thing: 

Thanks.

Re: [CFT/PATCH] give sound/oss/trident a holiday cleanup for 2.6

[Mike Fedyk]

On Thu, Jan 01, 2004 at 04:26:43PM -0800, Andrew Morton wrote:
> Muli Ben-Yehuda <mulix@mulix.org> wrote:
> >
> > On Thu, Jan 01, 2004 at 04:04:20PM -0800, Andrew Morton wrote:
> > 
> > > hmm, how come a whitespace cleanup patch adds nearly 200 lines which have
> > > trailing whitespace?
> > 
> > That would be either xemacs's or indent's fault. Can't be my
> > fault. No sir. Anyway, unless whitespace-mode is lying to me now, no
> > line has more than at most one character of whitespace added. If it
> > bugs you, I'll clean it up - it's a slow night tonight ;-) 
> 
> Nah, leave it as is.  I'm just having a little whine.  I added a nifty
> trailing-whitespace-detector to patch-scripts a while back and it's telling
> me that a *lot* of people use broken editors.
> 

And if their editor changed the whitespace you'd get a lot more patches with
shitespace cleanups mixed in, and they'd have to clean up thhose patches... ;)

Re: [CFT/PATCH] give sound/oss/trident a holiday cleanup for 2.6

[Andrew Morton]

Mike Fedyk <mfedyk@matchmail.com> wrote:
>
> > Nah, leave it as is.  I'm just having a little whine.  I added a nifty
>  > trailing-whitespace-detector to patch-scripts a while back and it's telling
>  > me that a *lot* of people use broken editors.
>  > 
> 
>  And if their editor changed the whitespace you'd get a lot more patches with
>  shitespace cleanups mixed in, and they'd have to clean up thhose patches... ;)

That's different.  I refer to patches which *add* trailing
whitespace: ^+.*[ ]$

I usually strip it out for them.  This has the potential to irritate people
because it can cause rejects for followup patches, but nobody has yet
mentioned it.