> > The following patch fixes SMP hangs w/ cmpci v5.64 ( k244-ac17 ).
>
> Let me suggest a different approach
>
> > - - spin_lock_irqsave(&s->lock, flags);
> > set_spdifout(s, rate);
> > + spin_lock_irqsave(&s->lock, flags);
>
> Split the various locked versions stuff stuff like set_adc_rate out as
> __set_adc_rate which doesnt take the lock, and set_adc_rate which takes the
> lock and calls the other one.
>
> That is how several other drivers were done and avoids messy lock/unlocks
> some of which in your changes I think introduce races
>
> Ditto for enable_dac as the old code and a newer __enable_dac, as well
> I suspect as __set_spdifout (although is that ever called by anyone not
> holding the lock ???)
I tought to implement this, but first I tryied to correct the problem.
The new patch (again revised use of spin*lock*) follows:
--- linux-244ac/drivers/sound/cmpci.c Fri May 25 05:26:27 2001
+++ linux/drivers/sound/cmpci.c Fri May 25 22:59:36 2001
@@ -1,5 +1,4 @@
/****************************************************************************
*/
-
/*
* cmpci.c -- C-Media PCI audio driver.
*
@@ -76,6 +75,11 @@
* was calling prog_dmabuf with s->lock held, call missing
* unlock_kernel in cm_midi_release
*
+ * Fri May 25 2001 - Carlos Eduardo Gorges <carlos@techlinux.com.br>
+ * - some driver cleanups
+ * - spin[un]lock* revision ( fix SMP support )
+ * - cosmetic code changes
+ *
*/
/****************************************************************************
*/
@@ -226,10 +230,6 @@
#define SND_DEV_DSP16 5
-#define set_dac1_rate set_adc_rate
-#define stop_dac1 stop_adc
-#define get_dmadac1 get_dmaadc
-
/* --------------------------------------------------------------------- */
struct cm_state {
@@ -342,7 +342,6 @@
/* --------------------------------------------------------------------- */
static struct cm_state *devs;
-static struct cm_state *devaudio;
static unsigned long wavetable_mem;
/* --------------------------------------------------------------------- */
@@ -496,15 +495,20 @@
return v;
}
-static void set_fmt(struct cm_state *s, unsigned char mask, unsigned char
data)
+static void set_fmt_unlocked(struct cm_state *s, unsigned char mask,
unsigned char data)
{
- unsigned long flags;
-
- spin_lock_irqsave(&s->lock, flags);
if (mask)
s->fmt = inb(s->iobase + CODEC_CMI_CHFORMAT);
s->fmt = (s->fmt & mask) | data;
outb(s->fmt, s->iobase + CODEC_CMI_CHFORMAT);
+}
+
+static void set_fmt(struct cm_state *s, unsigned char mask, unsigned char
data)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&s->lock, flags);
+ set_fmt_unlocked(s,mask,data);
spin_unlock_irqrestore(&s->lock, flags);
}
@@ -531,11 +535,8 @@
{ 48000, (44100 + 48000) / 2, 48000, 7 }
};
-static void set_spdifout(struct cm_state *s, unsigned rate)
+static void set_spdifout_unlocked(struct cm_state *s, unsigned rate)
{
- unsigned long flags;
-
- spin_lock_irqsave(&s->lock, flags);
if (rate == 48000 || rate == 44100) {
// SPDIFI48K SPDF_ACc97
maskl(s->iobase + CODEC_CMI_MISC_CTRL, ~0x01008000, rate == 48000 ?
0x01008000 : 0);
@@ -554,6 +555,14 @@
maskb(s->iobase + CODEC_CMI_MIXER1, ~1, 0);
s->status &= ~DO_SPDIF_OUT;
}
+}
+
+static void set_spdifout(struct cm_state *s, unsigned rate)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&s->lock, flags);
+ set_spdifout_unlocked(s,rate);
spin_unlock_irqrestore(&s->lock, flags);
}
@@ -573,12 +582,8 @@
return parity & 1;
}
-static void set_ac3(struct cm_state *s, unsigned rate)
+static void set_ac3_unlocked(struct cm_state *s, unsigned rate)
{
- unsigned long flags;
-
- spin_lock_irqsave(&s->lock, flags);
- set_spdifout(s, rate);
/* enable AC3 */
if (rate == 48000 || rate == 44100) {
// mute DAC
@@ -618,6 +623,16 @@
s->status &= ~DO_AC3;
}
s->spdif_counter = 0;
+
+}
+
+static void set_ac3(struct cm_state *s, unsigned rate)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&s->lock, flags);
+ set_spdifout_unlocked(s, rate);
+ set_ac3_unlocked(s,rate);
spin_unlock_irqrestore(&s->lock, flags);
}
@@ -648,6 +663,28 @@
} while (--i);
}
+static void set_adc_rate_unlocked(struct cm_state *s, unsigned rate)
+{
+ unsigned char freq = 4;
+ int i;
+
+ if (rate > 48000)
+ rate = 48000;
+ if (rate < 8000)
+ rate = 8000;
+ for (i = 0; i < sizeof(rate_lookup) / sizeof(rate_lookup[0]); i++) {
+ if (rate > rate_lookup[i].lower && rate <= rate_lookup[i].upper) {
+ rate = rate_lookup[i].rate;
+ freq = rate_lookup[i].freq;
+ break;
+ }
+ }
+ s->rateadc = rate;
+ freq <<= 2;
+
+ maskb(s->iobase + CODEC_CMI_FUNCTRL1 + 1, ~0x1c, freq);
+}
+
static void set_adc_rate(struct cm_state *s, unsigned rate)
{
unsigned long flags;
@@ -667,6 +704,7 @@
}
s->rateadc = rate;
freq <<= 2;
+
spin_lock_irqsave(&s->lock, flags);
maskb(s->iobase + CODEC_CMI_FUNCTRL1 + 1, ~0x1c, freq);
spin_unlock_irqrestore(&s->lock, flags);
@@ -691,13 +729,17 @@
}
s->ratedac = rate;
freq <<= 5;
+
spin_lock_irqsave(&s->lock, flags);
maskb(s->iobase + CODEC_CMI_FUNCTRL1 + 1, ~0xe0, freq);
- spin_unlock_irqrestore(&s->lock, flags);
+
+
if (s->curr_channels <= 2)
- set_spdifout(s, rate);
+ set_spdifout_unlocked(s, rate);
if (s->status & DO_DUAL_DAC)
- set_dac1_rate(s, rate);
+ set_adc_rate_unlocked(s, rate);
+
+ spin_unlock_irqrestore(&s->lock, flags);
}
/* --------------------------------------------------------------------- */
@@ -757,7 +799,7 @@
maskb(s->iobase + CODEC_CMI_FUNCTRL0, ~4, 0);
}
-extern inline void enable_dac(struct cm_state *s)
+extern inline void enable_dac_unlocked(struct cm_state *s)
{
if (!(s->enable & CM_ENABLE_CH1)) {
/* enable channel */
@@ -765,78 +807,114 @@
outb(s->enable, s->iobase + CODEC_CMI_FUNCTRL0 + 2);
}
maskb(s->iobase + CODEC_CMI_FUNCTRL0, ~8, 0);
+
if (s->status & DO_DUAL_DAC)
enable_adc(s);
}
-extern inline void stop_adc(struct cm_state *s)
+extern inline void enable_dac(struct cm_state *s)
{
unsigned long flags;
spin_lock_irqsave(&s->lock, flags);
+ enable_dac_unlocked(s);
+ spin_unlock_irqrestore(&s->lock, flags);
+}
+
+extern inline void stop_adc_unlocked(struct cm_state *s)
+{
if (s->enable & CM_ENABLE_CH0) {
/* disable interrupt */
maskb(s->iobase + CODEC_CMI_INT_HLDCLR + 2, ~1, 0);
disable_adc(s);
}
- spin_unlock_irqrestore(&s->lock, flags);
}
-extern inline void stop_dac(struct cm_state *s)
+extern inline void stop_adc(struct cm_state *s)
{
unsigned long flags;
spin_lock_irqsave(&s->lock, flags);
+ stop_adc_unlocked(s);
+ spin_unlock_irqrestore(&s->lock, flags);
+
+}
+
+extern inline void stop_dac_unlocked(struct cm_state *s)
+{
if (s->enable & CM_ENABLE_CH1) {
/* disable interrupt */
maskb(s->iobase + CODEC_CMI_INT_HLDCLR + 2, ~2, 0);
disable_dac(s);
}
- spin_unlock_irqrestore(&s->lock, flags);
if (s->status & DO_DUAL_DAC)
- stop_dac1(s);
+ stop_adc_unlocked(s);
}
-static void start_adc(struct cm_state *s)
+extern inline void stop_dac(struct cm_state *s)
{
unsigned long flags;
spin_lock_irqsave(&s->lock, flags);
+ stop_dac_unlocked(s);
+ spin_unlock_irqrestore(&s->lock, flags);
+}
+
+static void start_adc_unlocked(struct cm_state *s)
+{
if ((s->dma_adc.mapped || s->dma_adc.count < (signed)(s->dma_adc.dmasize -
2*s->dma_adc.fragsize))
&& s->dma_adc.ready) {
/* enable interrupt */
maskb(s->iobase + CODEC_CMI_INT_HLDCLR + 2, ~0, 1);
enable_adc(s);
}
- spin_unlock_irqrestore(&s->lock, flags);
-}
+}
-static void start_dac1(struct cm_state *s)
+static void start_adc(struct cm_state *s)
{
unsigned long flags;
spin_lock_irqsave(&s->lock, flags);
+ start_adc_unlocked(s);
+ spin_unlock_irqrestore(&s->lock, flags);
+}
+
+static void start_dac1_unlocked(struct cm_state *s)
+{
if ((s->dma_adc.mapped || s->dma_adc.count > 0) && s->dma_adc.ready) {
/* enable interrupt */
// maskb(s->iobase + CODEC_CMI_INT_HLDCLR + 2, ~0, 1);
- enable_dac(s);
+ enable_dac_unlocked(s);
}
- spin_unlock_irqrestore(&s->lock, flags);
-}
+}
-static void start_dac(struct cm_state *s)
-{
- unsigned long flags;
+//static void start_dac1(struct cm_state *s)
+//{
+// unsigned long flags;
+//
+// spin_lock_irqsave(&s->lock, flags);
+// start_dac1_unlocked(s);
+// spin_unlock_irqrestore(&s->lock, flags);
+//}
- spin_lock_irqsave(&s->lock, flags);
+static void start_dac_unlocked(struct cm_state *s)
+{
if ((s->dma_dac.mapped || s->dma_dac.count > 0) && s->dma_dac.ready) {
/* enable interrupt */
maskb(s->iobase + CODEC_CMI_INT_HLDCLR + 2, ~0, 2);
- enable_dac(s);
+ enable_dac_unlocked(s);
}
+ if (s->status & DO_DUAL_DAC)
+ start_dac1_unlocked(s);
+}
+
+static void start_dac(struct cm_state *s)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&s->lock, flags);
+ start_dac_unlocked(s);
spin_unlock_irqrestore(&s->lock, flags);
- if (s->status & DO_DUAL_DAC)
- start_dac1(s);
}
static int prog_dmabuf(struct cm_state *s, unsigned rec);
@@ -844,11 +922,11 @@
static int set_dac_channels(struct cm_state *s, int channels)
{
unsigned long flags;
-
spin_lock_irqsave(&s->lock, flags);
+
if ((channels > 2) && (channels <= s->max_channels)
&& (((s->fmt >> CM_CFMT_DACSHIFT) & CM_CFMT_MASK) == (CM_CFMT_STEREO |
CM_CFMT_16BIT))) {
- set_spdifout(s, 0);
+ set_spdifout_unlocked(s, 0);
if (s->capability & CAN_MULTI_CH_HW) {
// NXCHG
maskb(s->iobase + CODEC_CMI_LEGACY_CTRL + 3, ~0, 0x80);
@@ -868,10 +946,12 @@
maskb(s->iobase + CODEC_CMI_MISC_CTRL + 2, ~0, 0xC0);
s->status |= DO_DUAL_DAC;
// prepare secondary buffer
+
spin_unlock_irqrestore(&s->lock, flags);
ret = prog_dmabuf(s, 1);
- if (ret) return ret;
spin_lock_irqsave(&s->lock, flags);
+
+ if (ret) return ret;
// copy the hw state
fmtm &= ~((CM_CFMT_STEREO | CM_CFMT_16BIT) << CM_CFMT_DACSHIFT);
fmtm &= ~((CM_CFMT_STEREO | CM_CFMT_16BIT) << CM_CFMT_ADCSHIFT);
@@ -880,8 +960,10 @@
fmts |= CM_CFMT_16BIT << CM_CFMT_ADCSHIFT;
fmts |= CM_CFMT_STEREO << CM_CFMT_DACSHIFT;
fmts |= CM_CFMT_STEREO << CM_CFMT_ADCSHIFT;
- set_fmt(s, fmtm, fmts);
- set_dac1_rate(s, s->ratedac);
+
+ set_fmt_unlocked(s, fmtm, fmts);
+ set_adc_rate_unlocked(s, s->ratedac);
+
}
// N4SPK3D, disable 4 speaker mode (analog duplicate)
if (s->speakers > 2)
@@ -901,6 +983,7 @@
s->status &= ~DO_MULTI_CH;
s->curr_channels = s->fmt & (CM_CFMT_STEREO << CM_CFMT_DACSHIFT) ? 2 :
1;
}
+
spin_unlock_irqrestore(&s->lock, flags);
return s->curr_channels;
}
@@ -925,7 +1008,6 @@
db->mapped = db->ready = 0;
}
-
/* Ch1 is used for playback, Ch0 is used for recording */
static int prog_dmabuf(struct cm_state *s, unsigned rec)
@@ -939,7 +1021,6 @@
unsigned char fmt;
unsigned long flags;
- spin_lock_irqsave(&s->lock, flags);
fmt = s->fmt;
if (rec) {
stop_adc(s);
@@ -948,7 +1029,7 @@
stop_dac(s);
fmt >>= CM_CFMT_DACSHIFT;
}
- spin_unlock_irqrestore(&s->lock, flags);
+
fmt &= CM_CFMT_MASK;
db->hwptr = db->swptr = db->total_bytes = db->count = db->error =
db->endcleared = 0;
if (!db->rawbuf) {
@@ -1212,6 +1293,7 @@
[SOUND_MIXER_CD] = { DSP_MIX_CDVOLIDX_L, DSP_MIX_CDVOLIDX_R,
MT_5MUTE, 0x04, 0x02 },
[SOUND_MIXER_LINE] = { DSP_MIX_LINEVOLIDX_L, DSP_MIX_LINEVOLIDX_R,
MT_5MUTE, 0x10, 0x08 },
[SOUND_MIXER_MIC] = { DSP_MIX_MICVOLIDX, DSP_MIX_MICVOLIDX,
MT_5MUTEMONO, 0x01, 0x01 },
+
[SOUND_MIXER_SYNTH] = { DSP_MIX_FMVOLIDX_L, DSP_MIX_FMVOLIDX_R,
MT_5MUTE, 0x40, 0x00 },
[SOUND_MIXER_VOLUME] = { DSP_MIX_MASTERVOLIDX_L, DSP_MIX_MASTERVOLIDX_R,
MT_5MUTE, 0x00, 0x00 },
[SOUND_MIXER_PCM] = { DSP_MIX_VOICEVOLIDX_L, DSP_MIX_VOICEVOLIDX_R,
MT_5MUTE, 0x00, 0x00 }
@@ -1607,8 +1689,8 @@
printk(KERN_DEBUG "cm: read: chip lockup? dmasz %u fragsz %u count %i
hwptr %u swptr %u\n",
s->dma_adc.dmasize, s->dma_adc.fragsize, s->dma_adc.count,
s->dma_adc.hwptr, s->dma_adc.swptr);
- stop_adc(s);
spin_lock_irqsave(&s->lock, flags);
+ stop_adc_unlocked(s);
set_dmaadc(s, s->dma_adc.rawphys, s->dma_adc.dmasamples);
/* program sample counts */
set_countadc(s, s->dma_adc.fragsamples);
@@ -1625,11 +1707,11 @@
spin_lock_irqsave(&s->lock, flags);
s->dma_adc.swptr = swptr;
s->dma_adc.count -= cnt;
- spin_unlock_irqrestore(&s->lock, flags);
count -= cnt;
buffer += cnt;
ret += cnt;
- start_adc(s);
+ start_adc_unlocked(s);
+ spin_unlock_irqrestore(&s->lock, flags);
}
return ret;
}
@@ -1698,8 +1780,8 @@
printk(KERN_DEBUG "cm: write: chip lockup? dmasz %u fragsz %u count %i
hwptr %u swptr %u\n",
s->dma_dac.dmasize, s->dma_dac.fragsize, s->dma_dac.count,
s->dma_dac.hwptr, s->dma_dac.swptr);
- stop_dac(s);
spin_lock_irqsave(&s->lock, flags);
+ stop_dac_unlocked(s);
set_dmadac(s, s->dma_dac.rawphys, s->dma_dac.dmasamples);
/* program sample counts */
set_countdac(s, s->dma_dac.fragsamples);
@@ -1870,9 +1952,11 @@
return -EFAULT;
if (val >= 0) {
if (file->f_mode & FMODE_READ) {
- stop_adc(s);
+ spin_lock_irqsave(&s->lock, flags);
+ stop_adc_unlocked(s);
s->dma_adc.ready = 0;
- set_adc_rate(s, val);
+ set_adc_rate_unlocked(s, val);
+ spin_unlock_irqrestore(&s->lock, flags);
}
if (file->f_mode & FMODE_WRITE) {
stop_dac(s);
--_________________________ Carlos E Gorges (carlos@techlinux.com.br) Tech informática LTDA Brazil _________________________
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/