Au1550 OSS driver issues

Au1550 OSS driver issues

[Sergei Shtylylov]

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

Hello.

     We have found some issues with Au1550 AC'97 OSS driver in 2.6
(sound/oss/au1550_ac97.c), though it also should concern 2.4 driver
(drivers/sound/au1550_psc.c).
     First, we don't think that using readl() calls instead of au_readl() is
correct -- readl() is subject to byte-swapping etc., so may not work in
big-endian mode anymore and au_readl() is intended for embedded Au1550 devices
for which the byte swapping issue is resolved automagically, and BTW the same
PSC_AC97STAT register is read "both ways" in the driver. That's what the first
patch is about.
     Second, start_dac() grabs a spinlock already held by its caller, 
au1550_write(). This doesn't show up with the standard UP spinlock 
impelmentation but when the different one (mutex based) is in use, a lockup 
happens. The second patch demonstates a possible solution but here's a 
question: why there's no "symmetric" spinlock logic in start_adc(), may be 
here exits another potential issue?

WBR, Sergei


[-- Attachment #2: au1550_ac97_readl.patch --]
[-- Type: text/plain, Size: 1533 bytes --]

Index: linux/sound/oss/au1550_ac97.c
===================================================================
--- linux.orig/sound/oss/au1550_ac97.c
+++ linux/sound/oss/au1550_ac97.c
@@ -463,7 +463,7 @@ stop_dac(struct au1550_state *s)
 	/* Wait for Transmit Busy to show disabled.
 	*/
 	do {
-		stat = readl((void *)PSC_AC97STAT);
+		stat = au_readl(PSC_AC97STAT);
 		au_sync();
 	} while ((stat & PSC_AC97STAT_TB) != 0);
 
@@ -492,7 +492,7 @@ stop_adc(struct au1550_state *s)
 	/* Wait for Receive Busy to show disabled.
 	*/
 	do {
-		stat = readl((void *)PSC_AC97STAT);
+		stat = au_readl(PSC_AC97STAT);
 		au_sync();
 	} while ((stat & PSC_AC97STAT_RB) != 0);
 
@@ -542,7 +542,7 @@ set_xmit_slots(int num_channels)
 	/* Wait for Device ready.
 	*/
 	do {
-		stat = readl((void *)PSC_AC97STAT);
+		stat = au_readl(PSC_AC97STAT);
 		au_sync();
 	} while ((stat & PSC_AC97STAT_DR) == 0);
 }
@@ -574,7 +574,7 @@ set_recv_slots(int num_channels)
 	/* Wait for Device ready.
 	*/
 	do {
-		stat = readl((void *)PSC_AC97STAT);
+		stat = au_readl(PSC_AC97STAT);
 		au_sync();
 	} while ((stat & PSC_AC97STAT_DR) == 0);
 }
@@ -1989,7 +1989,7 @@ au1550_probe(void)
 	/* Wait for PSC ready.
 	*/
 	do {
-		val = readl((void *)PSC_AC97STAT);
+		val = au_readl(PSC_AC97STAT);
 		au_sync();
 	} while ((val & PSC_AC97STAT_SR) == 0);
 
@@ -2012,7 +2012,7 @@ au1550_probe(void)
 	/* Wait for Device ready.
 	*/
 	do {
-		val = readl((void *)PSC_AC97STAT);
+		val = au_readl(PSC_AC97STAT);
 		au_sync();
 	} while ((val & PSC_AC97STAT_DR) == 0);
 


[-- Attachment #3: au1550_ac97-spinlock.patch --]
[-- Type: text/plain, Size: 1574 bytes --]

Index: linux/sound/oss/au1550_ac97.c
===================================================================
--- linux.orig/sound/oss/au1550_ac97.c
+++ linux/sound/oss/au1550_ac97.c
@@ -74,6 +74,9 @@
 
 #define err(format, arg...) printk(KERN_ERR format "\n" , ## arg)
 
+#define START_DAC_NOSL 1
+#define START_DAC_SL 0
+
 /* Boot options
  * 0 = no VRA, 1 = use VRA if codec supports it
  */
@@ -580,7 +583,7 @@ set_recv_slots(int num_channels)
 }
 
 static void
-start_dac(struct au1550_state *s)
+start_dac(struct au1550_state *s, int no_sl)
 {
 	struct dmabuf  *db = &s->dma_dac;
 	unsigned long   flags;
@@ -588,7 +591,8 @@ start_dac(struct au1550_state *s)
 	if (!db->stopped)
 		return;
 
-	spin_lock_irqsave(&s->lock, flags);
+	if( !no_sl )
+		spin_lock_irqsave(&s->lock, flags);
 
 	set_xmit_slots(db->num_channels);
 	au_writel(PSC_AC97PCR_TC, PSC_AC97PCR);
@@ -600,7 +604,8 @@ start_dac(struct au1550_state *s)
 
 	db->stopped = 0;
 
-	spin_unlock_irqrestore(&s->lock, flags);
+	if( !no_sl )
+		spin_unlock_irqrestore(&s->lock, flags);
 }
 
 static void
@@ -1182,7 +1187,7 @@ au1550_write(struct file *file, const ch
 				db->nextOut -= db->dmasize;
 			db->total_bytes += db->dma_fragsize;
 			if (db->dma_qcount == 0)
-				start_dac(s);
+				start_dac(s,START_DAC_NOSL);
 			db->dma_qcount++;
 		}
 		spin_unlock_irqrestore(&s->lock, flags);
@@ -1578,7 +1583,7 @@ au1550_ioctl(struct inode *inode, struct
 		}
 		if (file->f_mode & FMODE_WRITE) {
 			if (val & PCM_ENABLE_OUTPUT)
-				start_dac(s);
+				start_dac(s,START_DAC_SL);
 			else
 				stop_dac(s);
 		}

Re: Au1550 OSS driver issues

[Dan Malek]

On Oct 6, 2005, at 6:02 AM, Sergei Shtylylov wrote:

>     We have found some issues with Au1550 AC'97 OSS driver in 2.6

Thanks.  I have some other updates queued for this driver and
I'll take care of it.

	-- Dan



-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl

Re: [Alsa-devel] Au1550 OSS driver issues

[Sergei Shtylylov]

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

Hello, I wrote:

>     We have found some issues with Au1550 AC'97 OSS driver in 2.6
> (sound/oss/au1550_ac97.c), though it also should concern 2.4 driver
> (drivers/sound/au1550_psc.c).
>     First, we don't think that using readl() calls instead of au_readl() is
> correct -- readl() is subject to byte-swapping etc., so may not work in
> BE mode anymore and au_readl() is intended for embedded Au1550 devices for 
   > which the byte swapping issue is resolved automagically, and BTW the same
> PSC_AC97STAT register is read "both ways" in the driver.

         ... for no apparent reason?

> That's what the first patch is about.

>     Second, start_dac() grabs a spinlock already held by its caller, 
> au1550_write(). This doesn't show up with the standard UP spinlock 
> impelmentation but when the different one (mutex based) is in use, a 
> lockup happens. The second patch demonstates a possible solution but 
> here's a question: why there's no "symmetric" spinlock logic in 
> start_adc(), may be here exits another potential issue?

         After having a look at sound/oss/au1000.c, here's an updated patch 
that deals with "nested" spinlocks the same way that driver does, and adds 
spinlock to start_adc() as well.

WBR, Sergei


[-- Attachment #2: au1550_ac97_readl.patch --]
[-- Type: text/plain, Size: 1656 bytes --]

Signed-off-by: Konstantin Baydarov <kbaidarov@ru.mvista.com>
Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

Index: linux/sound/oss/au1550_ac97.c
===================================================================
--- linux.orig/sound/oss/au1550_ac97.c
+++ linux/sound/oss/au1550_ac97.c
@@ -463,7 +463,7 @@ stop_dac(struct au1550_state *s)
 	/* Wait for Transmit Busy to show disabled.
 	*/
 	do {
-		stat = readl((void *)PSC_AC97STAT);
+		stat = au_readl(PSC_AC97STAT);
 		au_sync();
 	} while ((stat & PSC_AC97STAT_TB) != 0);
 
@@ -492,7 +492,7 @@ stop_adc(struct au1550_state *s)
 	/* Wait for Receive Busy to show disabled.
 	*/
 	do {
-		stat = readl((void *)PSC_AC97STAT);
+		stat = au_readl(PSC_AC97STAT);
 		au_sync();
 	} while ((stat & PSC_AC97STAT_RB) != 0);
 
@@ -542,7 +542,7 @@ set_xmit_slots(int num_channels)
 	/* Wait for Device ready.
 	*/
 	do {
-		stat = readl((void *)PSC_AC97STAT);
+		stat = au_readl(PSC_AC97STAT);
 		au_sync();
 	} while ((stat & PSC_AC97STAT_DR) == 0);
 }
@@ -574,7 +574,7 @@ set_recv_slots(int num_channels)
 	/* Wait for Device ready.
 	*/
 	do {
-		stat = readl((void *)PSC_AC97STAT);
+		stat = au_readl(PSC_AC97STAT);
 		au_sync();
 	} while ((stat & PSC_AC97STAT_DR) == 0);
 }
@@ -1989,7 +1989,7 @@ au1550_probe(void)
 	/* Wait for PSC ready.
 	*/
 	do {
-		val = readl((void *)PSC_AC97STAT);
+		val = au_readl(PSC_AC97STAT);
 		au_sync();
 	} while ((val & PSC_AC97STAT_SR) == 0);
 
@@ -2012,7 +2012,7 @@ au1550_probe(void)
 	/* Wait for Device ready.
 	*/
 	do {
-		val = readl((void *)PSC_AC97STAT);
+		val = au_readl(PSC_AC97STAT);
 		au_sync();
 	} while ((val & PSC_AC97STAT_DR) == 0);
 






[-- Attachment #3: au1550_ac7-spinlocks.patch --]
[-- Type: text/plain, Size: 1120 bytes --]

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

Index: sound/oss/au1550_ac97.c
===================================================================
--- sound/oss/au1550_ac97.c~	10 Jul 2005 10:29:03 -0000
+++ sound/oss/au1550_ac97.c	7 Nov 2005 18:14:59 -0000
@@ -607,11 +607,14 @@ static void
 start_adc(struct au1550_state *s)
 {
 	struct dmabuf  *db = &s->dma_adc;
+	unsigned long   flags;
 	int	i;
 
 	if (!db->stopped)
 		return;
 
+	spin_lock_irqsave(&s->lock, flags);
+
 	/* Put two buffers on the ring to get things started.
 	*/
 	for (i=0; i<2; i++) {
@@ -630,6 +633,8 @@ start_adc(struct au1550_state *s)
 	au_sync();
 
 	db->stopped = 0;
+
+	spin_unlock_irqrestore(&s->lock, flags);
 }
 
 static int
@@ -1181,8 +1186,11 @@ au1550_write(struct file *file, const ch
 			if (db->nextOut >= db->rawbuf + db->dmasize)
 				db->nextOut -= db->dmasize;
 			db->total_bytes += db->dma_fragsize;
-			if (db->dma_qcount == 0)
+			if (db->dma_qcount == 0) {
+				spin_unlock(&s->lock);
 				start_dac(s);
+				spin_lock(&s->lock);
+			}
 			db->dma_qcount++;
 		}
 		spin_unlock_irqrestore(&s->lock, flags);

Re: Au1550 OSS driver issues

[Lee Revell]

On Mon, 2005-11-07 at 22:58 +0300, Sergei Shtylylov wrote:
>          After having a look at sound/oss/au1000.c, here's an updated patch 
> that deals with "nested" spinlocks the same way that driver does, and adds 
> spinlock to start_adc() as well.

The OSS drivers are scheduled for removal, it's unlikely that this will
be accepted into the kernel.  It also has nothing to do with ALSA.

Lee



-------------------------------------------------------
SF.Net email is sponsored by:
Tame your development challenges with Apache's Geronimo App Server. Download
it for free - -and be entered to win a 42" plasma tv or your very own
Sony(tm)PSP.  Click here to play: http://sourceforge.net/geronimo.php