linux-kernel - Re: sound skipping regression introduced in 2.6.30-rc8

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <s5hy6rtesjo.wl%tiwai@suse.de>
Date:	Mon, 15 Jun 2009 11:48:43 +0200
From:	Takashi Iwai <tiwai@...e.de>
To:	David Miller <davem@...emloft.net>
Cc:	bzolnier@...il.com, sven.koehler@...il.com,
	linux-kernel@...r.kernel.org
Subject: Re: sound skipping regression introduced in 2.6.30-rc8

At Mon, 15 Jun 2009 02:25:47 -0700 (PDT),
David Miller wrote:
> 
> From: Takashi Iwai <tiwai@...e.de>
> Date: Mon, 15 Jun 2009 10:39:50 +0200
> 
> > If that patch also doesn't work, we need to track the position update
> > sequence in more details.
> > The patch below will dump the each position update (CAUTION: it can be
> > long logs!).  Please apply it instead of the previous one, run with
> > xrun_debug=5 on 2.6.31 (or xrun_debug=1 on 2.6.30), and give the
> > outputs around a skip occurs.
> 
> The patch doesn't work.
> 
> I put up a debugging log at:
> 
> 	http://vger.kernel.org/~davem/asound.log
> 
> there is a "PCM: hw_ptr skipping! ..." message near the end.

Thanks!  I see the problem now.  It's a race in the update of the
current buffer position.  The counter was reset to the full fragment
size, but its index still points the previous position.  So, the
driver was confused and put the position back.

The below is a revised patch.  It has an additional sanity check.
Please give it a try.


Takashi

---
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index 173bebf..474e40a 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -356,8 +356,6 @@ struct ichdev {
         unsigned int position;
 	unsigned int pos_shift;
 	unsigned int last_pos;
-	unsigned long last_pos_jiffies;
-	unsigned int jiffy_to_bytes;
         int frags;
         int lvi;
         int lvi_frag;
@@ -844,7 +842,6 @@ static int snd_intel8x0_pcm_trigger(struct snd_pcm_substream *substream, int cmd
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 		val = ICH_IOCE | ICH_STARTBM;
 		ichdev->last_pos = ichdev->position;
-		ichdev->last_pos_jiffies = jiffies;
 		break;
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 		ichdev->suspended = 1;
@@ -1048,7 +1045,6 @@ static int snd_intel8x0_pcm_prepare(struct snd_pcm_substream *substream)
 			ichdev->pos_shift = (runtime->sample_bits > 16) ? 2 : 1;
 	}
 	snd_intel8x0_setup_periods(chip, ichdev);
-	ichdev->jiffy_to_bytes = (runtime->rate * 4 * ichdev->pos_shift) / HZ;
 	return 0;
 }
 
@@ -1073,19 +1069,19 @@ static snd_pcm_uframes_t snd_intel8x0_pcm_pointer(struct snd_pcm_substream *subs
 		    ptr1 == igetword(chip, ichdev->reg_offset + ichdev->roff_picb))
 			break;
 	} while (timeout--);
+	ptr = ichdev->last_pos;
 	if (ptr1 != 0) {
 		ptr1 <<= ichdev->pos_shift;
 		ptr = ichdev->fragsize1 - ptr1;
 		ptr += position;
-		ichdev->last_pos = ptr;
-		ichdev->last_pos_jiffies = jiffies;
-	} else {
-		ptr1 = jiffies - ichdev->last_pos_jiffies;
-		if (ptr1)
-			ptr1 -= 1;
-		ptr = ichdev->last_pos + ptr1 * ichdev->jiffy_to_bytes;
-		ptr %= ichdev->size;
+		if (ptr < ichdev->last_pos) {
+			/* invalid position; likely ptr1 went back to
+			 * fragsize1 while the base position wasn't updated yet
+			 */
+			ptr = ichdev->last_pos;
+		}
 	}
+	ichdev->last_pos = ptr;
 	spin_unlock(&chip->reg_lock);
 	if (ptr >= ichdev->size)
 		return 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/