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

Re: [Alsa-devel] 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

Re: [Alsa-devel] Au1550 OSS driver issues

[Sergei Shtylylov]

[-- Attachment #1: Type: text/plain, Size: 1529 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).

[au_readl() issue skipped]

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

    Unfortunately, the proposed solution was incorrect for that mutex case
because it was breaking the "critical section" by temporarily dropping the
spinlock to call start_dac(). So, here's the updated version of that patch in
which start_dac() and start_adc() don't grab the spinlocks anymore but their
callers do instead.

>         After having a look at sound/oss/au1000.c,

    Now I don't think that this trick is always correct but since that driver
is obsoleted by ALSA one I don't care that much. ;-)

> here's an updated 
> patch that deals with "nested" spinlocks the same way that driver does, 
> and adds spinlock to start_adc() as well.

    And the interrupt handlers also didn't grab the spinlock -- that's OK
in the usual kernel but not when the IRQ handlers are threaded. So, they're
grabbing the spinlock now (as every correct interrupt handler should do).

WBR, Sergei



[-- Attachment #2: Au1550-AC97-OSS-spinlocks.patch --]
[-- Type: text/plain, Size: 3020 bytes --]

diff --git a/sound/oss/au1550_ac97.c b/sound/oss/au1550_ac97.c
index cdce915..f70effd 100644
--- a/sound/oss/au1550_ac97.c
+++ b/sound/oss/au1550_ac97.c
@@ -579,17 +579,15 @@ set_recv_slots(int num_channels)
 	} while ((stat & PSC_AC97STAT_DR) == 0);
 }
 
+/* Hold spinlock for both start_dac() and start_adc() calls */
 static void
 start_dac(struct au1550_state *s)
 {
 	struct dmabuf  *db = &s->dma_dac;
-	unsigned long   flags;
 
 	if (!db->stopped)
 		return;
 
-	spin_lock_irqsave(&s->lock, flags);
-
 	set_xmit_slots(db->num_channels);
 	au_writel(PSC_AC97PCR_TC, PSC_AC97PCR);
 	au_sync();
@@ -599,8 +597,6 @@ start_dac(struct au1550_state *s)
 	au1xxx_dbdma_start(db->dmanr);
 
 	db->stopped = 0;
-
-	spin_unlock_irqrestore(&s->lock, flags);
 }
 
 static void
@@ -719,7 +715,6 @@ prog_dmabuf_dac(struct au1550_state *s)
 }
 
 
-/* hold spinlock for the following */
 static void
 dac_dma_interrupt(int irq, void *dev_id, struct pt_regs *regs)
 {
@@ -727,6 +722,8 @@ dac_dma_interrupt(int irq, void *dev_id,
 	struct dmabuf  *db = &s->dma_dac;
 	u32	ac97c_stat;
 
+	spin_lock(&s->lock);
+
 	ac97c_stat = au_readl(PSC_AC97STAT);
 	if (ac97c_stat & (AC97C_XU | AC97C_XO | AC97C_TE))
 		pr_debug("AC97C status = 0x%08x\n", ac97c_stat);
@@ -748,6 +745,8 @@ dac_dma_interrupt(int irq, void *dev_id,
 	/* wake up anybody listening */
 	if (waitqueue_active(&db->wait))
 		wake_up(&db->wait);
+
+	spin_unlock(&s->lock);
 }
 
 
@@ -759,6 +758,8 @@ adc_dma_interrupt(int irq, void *dev_id,
 	u32	obytes;
 	char	*obuf;
 
+	spin_lock(&s->lock);
+
 	/* Pull the buffer from the dma queue.
 	*/
 	au1xxx_dbdma_get_dest(dp->dmanr, (void *)(&obuf), &obytes);
@@ -766,6 +767,7 @@ adc_dma_interrupt(int irq, void *dev_id,
 	if ((dp->count + obytes) > dp->dmasize) {
 		/* Overrun. Stop ADC and log the error
 		*/
+		spin_unlock(&s->lock);
 		stop_adc(s);
 		dp->error++;
 		err("adc overrun");
@@ -788,6 +790,7 @@ adc_dma_interrupt(int irq, void *dev_id,
 	if (waitqueue_active(&dp->wait))
 		wake_up(&dp->wait);
 
+	spin_unlock(&s->lock);
 }
 
 static loff_t
@@ -1049,9 +1052,9 @@ au1550_read(struct file *file, char *buf
 		/* wait for samples in ADC dma buffer
 		*/
 		do {
+			spin_lock_irqsave(&s->lock, flags);
 			if (db->stopped)
 				start_adc(s);
-			spin_lock_irqsave(&s->lock, flags);
 			avail = db->count;
 			if (avail <= 0)
 				__set_current_state(TASK_INTERRUPTIBLE);
@@ -1571,15 +1574,19 @@ au1550_ioctl(struct inode *inode, struct
 		if (get_user(val, (int *) arg))
 			return -EFAULT;
 		if (file->f_mode & FMODE_READ) {
-			if (val & PCM_ENABLE_INPUT)
+			if (val & PCM_ENABLE_INPUT) {
+				spin_lock_irqsave(&s->lock, flags);
 				start_adc(s);
-			else
+				spin_unlock_irqrestore(&s->lock, flags);
+			} else
 				stop_adc(s);
 		}
 		if (file->f_mode & FMODE_WRITE) {
-			if (val & PCM_ENABLE_OUTPUT)
+			if (val & PCM_ENABLE_OUTPUT) {
+				spin_lock_irqsave(&s->lock, flags);
 				start_dac(s);
-			else
+				spin_unlock_irqrestore(&s->lock, flags);
+			} else
 				stop_dac(s);
 		}
 		return 0;

Re: [Alsa-devel] Au1550 OSS driver issues

[Sergei Shtylylov]

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

Sergei Shtylylov wrote:
> 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).

> [au_readl() issue skipped]

>>>     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?
> 
> 
>    Unfortunately, the proposed solution was incorrect for that mutex case
> because it was breaking the "critical section" by temporarily dropping the
> spinlock to call start_dac(). So, here's the updated version of that 
> patch in
> which start_dac() and start_adc() don't grab the spinlocks anymore but 
> their
> callers do instead.
> 
>>         After having a look at sound/oss/au1000.c,
> 
> 
>    Now I don't think that this trick is always correct but since that 
> driver
> is obsoleted by ALSA one I don't care that much. ;-)
> 
>> here's an updated patch that deals with "nested" spinlocks the same 
>> way that driver does, and adds spinlock to start_adc() as well.
> 
> 
>    And the interrupt handlers also didn't grab the spinlock -- that's OK
> in the usual kernel but not when the IRQ handlers are threaded. So, they're
> grabbing the spinlock now (as every correct interrupt handler should do).

    Failed to change the the subject and forgot about the sign-off, silly 
me... :-(

WBR, Sergei

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


[-- Attachment #2: Au1550-AC97-OSS-spinlocks.patch --]
[-- Type: text/plain, Size: 3017 bytes --]

diff --git a/sound/oss/au1550_ac97.c b/sound/oss/au1550_ac97.c
index cdce915..f70effd 100644
--- a/sound/oss/au1550_ac97.c
+++ b/sound/oss/au1550_ac97.c
@@ -579,17 +579,15 @@ set_recv_slots(int num_channels)
 	} while ((stat & PSC_AC97STAT_DR) == 0);
 }
 
+/* Hold spinlock for both start_dac() and start_adc() calls */
 static void
 start_dac(struct au1550_state *s)
 {
 	struct dmabuf  *db = &s->dma_dac;
-	unsigned long   flags;
 
 	if (!db->stopped)
 		return;
 
-	spin_lock_irqsave(&s->lock, flags);
-
 	set_xmit_slots(db->num_channels);
 	au_writel(PSC_AC97PCR_TC, PSC_AC97PCR);
 	au_sync();
@@ -599,8 +597,6 @@ start_dac(struct au1550_state *s)
 	au1xxx_dbdma_start(db->dmanr);
 
 	db->stopped = 0;
-
-	spin_unlock_irqrestore(&s->lock, flags);
 }
 
 static void
@@ -719,7 +715,6 @@ prog_dmabuf_dac(struct au1550_state *s)
 }
 
 
-/* hold spinlock for the following */
 static void
 dac_dma_interrupt(int irq, void *dev_id, struct pt_regs *regs)
 {
@@ -727,6 +722,8 @@ dac_dma_interrupt(int irq, void *dev_id,
 	struct dmabuf  *db = &s->dma_dac;
 	u32	ac97c_stat;
 
+	spin_lock(&s->lock);
+
 	ac97c_stat = au_readl(PSC_AC97STAT);
 	if (ac97c_stat & (AC97C_XU | AC97C_XO | AC97C_TE))
 		pr_debug("AC97C status = 0x%08x\n", ac97c_stat);
@@ -748,6 +745,8 @@ dac_dma_interrupt(int irq, void *dev_id,
 	/* wake up anybody listening */
 	if (waitqueue_active(&db->wait))
 		wake_up(&db->wait);
+
+	spin_unlock(&s->lock);
 }
 
 
@@ -759,6 +758,8 @@ adc_dma_interrupt(int irq, void *dev_id,
 	u32	obytes;
 	char	*obuf;
 
+	spin_lock(&s->lock);
+
 	/* Pull the buffer from the dma queue.
 	*/
 	au1xxx_dbdma_get_dest(dp->dmanr, (void *)(&obuf), &obytes);
@@ -766,6 +767,7 @@ adc_dma_interrupt(int irq, void *dev_id,
 	if ((dp->count + obytes) > dp->dmasize) {
 		/* Overrun. Stop ADC and log the error
 		*/
+		spin_unlock(&s->lock);
 		stop_adc(s);
 		dp->error++;
 		err("adc overrun");
@@ -788,6 +790,7 @@ adc_dma_interrupt(int irq, void *dev_id,
 	if (waitqueue_active(&dp->wait))
 		wake_up(&dp->wait);
 
+	spin_unlock(&s->lock);
 }
 
 static loff_t
@@ -1049,9 +1052,9 @@ au1550_read(struct file *file, char *buf
 		/* wait for samples in ADC dma buffer
 		*/
 		do {
+			spin_lock_irqsave(&s->lock, flags);
 			if (db->stopped)
 				start_adc(s);
-			spin_lock_irqsave(&s->lock, flags);
 			avail = db->count;
 			if (avail <= 0)
 				__set_current_state(TASK_INTERRUPTIBLE);
@@ -1571,15 +1574,19 @@ au1550_ioctl(struct inode *inode, struct
 		if (get_user(val, (int *) arg))
 			return -EFAULT;
 		if (file->f_mode & FMODE_READ) {
-			if (val & PCM_ENABLE_INPUT)
+			if (val & PCM_ENABLE_INPUT) {
+				spin_lock_irqsave(&s->lock, flags);
 				start_adc(s);
-			else
+				spin_unlock_irqrestore(&s->lock, flags);
+			} else
 				stop_adc(s);
 		}
 		if (file->f_mode & FMODE_WRITE) {
-			if (val & PCM_ENABLE_OUTPUT)
+			if (val & PCM_ENABLE_OUTPUT) {
+				spin_lock_irqsave(&s->lock, flags);
 				start_dac(s);
-			else
+				spin_unlock_irqrestore(&s->lock, flags);
+			} else
 				stop_dac(s);
 		}
 		return 0;

[PATCH] Au1xx0: replace casual readl() with au_readl() in the drivers

[Sergei Shtylylov]

[-- Attachment #1: Type: text/plain, Size: 789 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.

[skipped]

     Additionally, I've found one unjustified call to readl() in the Au1xx0 USB
code, so adding the fix for it to the patch. Andrew, I'm sending the patch to
you as was advised by Ralf and Jordan...

WBR, Sergei


[-- Attachment #2: Au1xx0-use-au_readl.patch --]
[-- Type: text/plain, Size: 1986 bytes --]

diff --git a/drivers/usb/host/ohci-au1xxx.c b/drivers/usb/host/ohci-au1xxx.c
index 486202d..0fc728e 100644
--- a/drivers/usb/host/ohci-au1xxx.c
+++ b/drivers/usb/host/ohci-au1xxx.c
@@ -66,7 +66,7 @@ static void au1xxx_stop_hc(struct platfo
 	       ": stopping Au1xxx OHCI USB Controller\n");
 
 	/* Disable clock */
-	au_writel(readl((void *)USB_HOST_CONFIG) & ~USBH_ENABLE_CE, USB_HOST_CONFIG);
+	au_writel(au_readl(USB_HOST_CONFIG) & ~USBH_ENABLE_CE, USB_HOST_CONFIG);
 }
 
 
diff --git a/sound/oss/au1550_ac97.c b/sound/oss/au1550_ac97.c
index f70effd..64e2e46 100644
--- a/sound/oss/au1550_ac97.c
+++ b/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);
 }
@@ -1996,7 +1996,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);
 
@@ -2019,7 +2019,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);
 

Re: Au1xx0: replace casual readl() with au_readl() in the drivers

[Jordan Crouse]

Acked-by: Jordan CRouse <jordan.crouse@amd.com>

On 31/12/05 02:07 +0300, Sergei Shtylylov wrote:
> 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.
> 
> [skipped]
> 
>     Additionally, I've found one unjustified call to readl() in the Au1xx0 
>     USB
> code, so adding the fix for it to the patch. Andrew, I'm sending the patch 
> to
> you as was advised by Ralf and Jordan...
> 
> WBR, Sergei
> 

> diff --git a/drivers/usb/host/ohci-au1xxx.c b/drivers/usb/host/ohci-au1xxx.c
> index 486202d..0fc728e 100644
> --- a/drivers/usb/host/ohci-au1xxx.c
> +++ b/drivers/usb/host/ohci-au1xxx.c
> @@ -66,7 +66,7 @@ static void au1xxx_stop_hc(struct platfo
>  	       ": stopping Au1xxx OHCI USB Controller\n");
>  
>  	/* Disable clock */
> -	au_writel(readl((void *)USB_HOST_CONFIG) & ~USBH_ENABLE_CE, USB_HOST_CONFIG);
> +	au_writel(au_readl(USB_HOST_CONFIG) & ~USBH_ENABLE_CE, USB_HOST_CONFIG);
>  }
>  
>  
> diff --git a/sound/oss/au1550_ac97.c b/sound/oss/au1550_ac97.c
> index f70effd..64e2e46 100644
> --- a/sound/oss/au1550_ac97.c
> +++ b/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);
>  }
> @@ -1996,7 +1996,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);
>  
> @@ -2019,7 +2019,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);
>  


-- 
Jordan Crouse
Senior Linux Engineer
AMD - Personal Connectivity Solutions Group
<www.amd.com/embeddedprocessors>