Bug 8172 - OSS trident driver: race or erroneous comment?
Summary: OSS trident driver: race or erroneous comment?
Status: CLOSED CODE_FIX
Alias: None
Product: Drivers
Classification: Unclassified
Component: Sound(OSS) (show other bugs)
Hardware: i386 Linux
: P2 normal
Assignee: Muli Ben-Yehuda
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-03-10 18:43 UTC by Lin Tan
Modified: 2007-11-20 03:03 UTC (History)
1 user (show)

See Also:
Kernel Version: 2.6.20.1
Tree: Mainline
Regression: ---


Attachments
fix locking around write_voice_regs (1.94 KB, patch)
2007-05-13 02:24 UTC, Muli Ben-Yehuda
Details | Diff

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.

Note You need to log in before you can comment on or make changes to this bug.