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
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.