Bug 8172

Summary:	OSS trident driver: race or erroneous comment?
Product:	Drivers	Reporter:	Lin Tan (tammy000)
Component:	Sound(OSS)	Assignee:	Muli Ben-Yehuda (muli)
Status:	CLOSED CODE_FIX
Severity:	normal	CC:	alishajonwal5050, bunk, iamanikamalhotra, kumarjyoti7008, rabecahescorts, saanvihott69, sharmidey33, talakshe, xosamop642
Priority:	P2
Hardware:	i386
OS:	Linux
Kernel Version:	2.6.20.1	Subsystem:
Regression:	---	Bisected commit-id:
Attachments:	fix locking around write_voice_regs

Description Lin Tan 2007-03-10 18:43:53 UTC

857 /* called with spin lock held */
858 static int
859 trident_write_voice_regs(struct trident_state *state)

But the function is called with lock held in some cases but not always.

One violating call chain is:
2699 trident_open -> 
2784 trident_set_dac_rate(state, 8000);
-> trident_write_voice_regs

======================================
One example that follows the comment is:

2289                                 spin_lock_irqsave(&state->card->lock, flags);
2290                                 trident_set_dac_rate(state, val);
2291                                 spin_unlock_irqrestore(&state->card->lock,
flags);

trident_set_dac_rate calls  trident_write_voice_regs

Comment 1 Muli Ben-Yehuda 2007-03-11 10:58:59 UTC

Looking into it.

Comment 2 Lin Tan 2007-03-13 14:48:40 UTC

I noticed that trident_open is called by function pointers. Maybe a lock is
acquired before it is called via the function pointer?

Comment 3 Muli Ben-Yehuda 2007-03-18 22:48:29 UTC

Ok, I'm pretty sure that it's indeed a bug, but its been a (long...) while since
I've looked at trident.c in depth.

Basically, we allocate a new virtual channel in open, and then we frob some
hardware registers (in write_voice_regs) for that channel. But the hardware
registers are shared with other channels, which may be trying to frob the some
registers (albeit for another channel) via ioctl(SNDCTL_DSP_SPEED). Not very
likely to happen, but a bug is a bug.

I'll whip up a patch and run it through its paces and then send it to mainline.
Thanks for the bug report!

Comment 4 Lin Tan 2007-03-19 01:19:56 UTC

Thank you very much for the confirmation!

Comment 5 Lin Tan 2007-03-19 01:31:15 UTC

trident_open also calls 
2802                 trident_set_adc_rate(state, 8000);
which needs the same lock. So you may want to fix both places.

Comment 6 Muli Ben-Yehuda 2007-05-13 02:24:45 UTC

Created attachment 11489 [details]
fix locking around write_voice_regs

Finally got around to it. Patch attached, will test and send it out to akpm
later today.

Comment 7 Natalie Protasevich 2007-11-20 03:03:00 UTC

Muli, 
This is commit 3b20b9b4e985fcc48b4eea401cb289a856422c93 right?
I suppose the bug can be closed now.