[PATCH] dmaengine: apple-admac: Fix position one period ahead at period boundary

[PATCH] dmaengine: apple-admac: Fix position one period ahead at period boundary

[Jacob Procee]

Speaker playback on a MacBook Pro 14" (M1 Pro, J314s) underruns at
irregular intervals, audible as a tick: 8 times in 3.7 hours of
playback. Where an underrun was caught (4 of 8), the ALSA hw_ptr had
jumped by one buffer size while about 70 ms of audio was still queued,
and a kernel trace showed the DMA position stepping back by exactly one
period at each of the 6 underruns it covered.

admac_cyclic_read_residue() derives the position from the report ring
(periods finished but not yet reclaimed) and REG_RESIDUE (bytes left in
the current descriptor). When a descriptor finishes, the residue first
drops to zero, then the report is posted and the next descriptor is
loaded. Usually the last two happen together, but occasionally the
report comes first. A read in that window counts the finished period
twice:

 - residue2 == 0 with the report already in ring2: the period is
   counted in nreports and again as the fully consumed current
   descriptor;
 - residue1 == 0 with the report already in ring1, and the next
   descriptor loaded by the time residue2 is read: the
   residue2 > residue1 branch adds one to a ring1 that already holds
   the report.

Either way the position is one period ahead. The next read is behind
it, snd_pcm_update_hw_ptr0() takes the backwards step for a wrap of the
whole buffer, and the stream underruns.

While the channel runs, a zero residue only occurs between descriptors,
so read the registers again while either residue is zero. The retries
are bounded, as the residue also stays zero on a stopped channel or one
that ran out of descriptors; after the last retry the position is
computed as before.

With a debug build counting retries, one hour of playback needed a
retry for 2210 of 3.39 million reads, at most three retries (3.5 us),
also across pause and stream restart. Both states above showed up in
the logged register values. There were no underruns in three hours of
playback with the fix.

Fixes: b127315d9a78 ("dmaengine: apple-admac: Add Apple ADMAC driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Jacob Procee <sn125164@gmail.com>
---
How this was found and tested:
- The underruns were traced with bpftrace on snd_dmaengine_pcm_pointer():
  a position read 19-37 us before the period interrupt was exactly one
  period ahead of the interrupt's own read. The register states were then
  confirmed with a debug build of this driver that logged the raw report
  ring and residue values on every retry.
- There is no hardware documentation for the ordering; all of this is
  measured. In the 1906 logged retries the next read always showed both
  the report and the new descriptor, or neither, never the new descriptor
  with the old report count.
- The reverse order (next descriptor loaded before the report is posted)
  would put the position one period behind and is not handled here. The
  bpftrace probe checked every position read for a backward step, so it
  would have caught that too: none occurred in the three hours with the
  fix, roughly 10 million reads.
- RESIDUE_RETRIES is empirical as well: of the logged retries, 1189 needed
  one, 621 two and 96 three, none more; the longest complete retry
  sequence took 3.5 us. A round takes about 1 us, so the limit keeps the
  additional polling time, with interrupts off, to roughly 10 us in the
  observed environment. If the limit is reached, the position is computed
  using the existing calculation, so the original error can still occur
  in that case.
- Tested on a MacBook Pro 14" (M1 Pro, J314s) as an out-of-tree module on
  the Asahi 7.2.12 kernel, whose apple-admac.c matches this tree apart from
  the unrelated admac_hw rework. The three hours above used a limit of 100
  retries; the code as posted here (limit 10) ran for another 30 minutes.
  On this tree the driver builds cleanly with W=1 and C=1 (arm64
  defconfig), but I have not run it here, nor on other Apple SoCs.

AI assistance: the tracing, analysis, the code change and a first draft of
this changelog were done with an LLM (Claude) in a session with me. I have
reviewed and tested the result and can answer for it.

 drivers/dma/apple-admac.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/apple-admac.c b/drivers/dma/apple-admac.c
index 9a1c79c12..9b284c658 100644
--- a/drivers/dma/apple-admac.c
+++ b/drivers/dma/apple-admac.c
@@ -28,6 +28,12 @@
  */
 #define SRAM_BLOCK	2048
 
+/*
+ * How often to re-read the residue while a descriptor boundary is in
+ * progress, see admac_cyclic_read_residue().
+ */
+#define RESIDUE_RETRIES	10
+
 #define RING_WRITE_SLOT		GENMASK(1, 0)
 #define RING_READ_SLOT		GENMASK(5, 4)
 #define RING_FULL		BIT(9)
@@ -358,11 +364,24 @@ static u32 admac_cyclic_read_residue(struct admac_data *ad, int channo,
 	u32 residue1, residue2;
 	int nreports;
 	size_t pos;
+	int tries = 0;
 
-	ring1 =    readl_relaxed(ad->base + REG_REPORT_RING(channo));
-	residue1 = readl_relaxed(ad->base + REG_RESIDUE(channo));
-	ring2 =    readl_relaxed(ad->base + REG_REPORT_RING(channo));
-	residue2 = readl_relaxed(ad->base + REG_RESIDUE(channo));
+	/*
+	 * The residue drops to zero when a descriptor finishes, before the
+	 * next one is loaded. If the finished descriptor's report is already
+	 * in the ring at that point, the position below comes out one period
+	 * ahead: with residue2 == 0 directly, with residue1 == 0 through the
+	 * residue2 > residue1 branch. The next read is then behind it, which
+	 * ALSA takes for a wrap of the whole buffer. Re-read until the next
+	 * descriptor is loaded; the limit covers a stopped or starved channel,
+	 * whose residue stays zero.
+	 */
+	do {
+		ring1 =    readl_relaxed(ad->base + REG_REPORT_RING(channo));
+		residue1 = readl_relaxed(ad->base + REG_RESIDUE(channo));
+		ring2 =    readl_relaxed(ad->base + REG_REPORT_RING(channo));
+		residue2 = readl_relaxed(ad->base + REG_RESIDUE(channo));
+	} while ((!residue1 || !residue2) && ++tries < RESIDUE_RETRIES);
 
 	if (residue2 > residue1) {
 		/*

base-commit: aff5e6a77fc69a7c490246465940d18422d7c631
-- 
2.56.0

Re: [PATCH] dmaengine: apple-admac: Fix position one period ahead at period boundary

[Frank Li]

On Thu, Oct 29, 2026 at 11:59:08AM +0100, Jacob Procee wrote:
> [You don't often get email from sn125164@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Speaker playback on a MacBook Pro 14" (M1 Pro, J314s) underruns at
> irregular intervals, audible as a tick: 8 times in 3.7 hours of
> playback. Where an underrun was caught (4 of 8), the ALSA hw_ptr had
> jumped by one buffer size while about 70 ms of audio was still queued,
> and a kernel trace showed the DMA position stepping back by exactly one
> period at each of the 6 underruns it covered.
>
> admac_cyclic_read_residue() derives the position from the report ring
> (periods finished but not yet reclaimed) and REG_RESIDUE (bytes left in
> the current descriptor). When a descriptor finishes, the residue first
> drops to zero, then the report is posted and the next descriptor is
> loaded. Usually the last two happen together, but occasionally the
> report comes first. A read in that window counts the finished period
> twice:
>
>  - residue2 == 0 with the report already in ring2: the period is
>    counted in nreports and again as the fully consumed current
>    descriptor;
>  - residue1 == 0 with the report already in ring1, and the next
>    descriptor loaded by the time residue2 is read: the
>    residue2 > residue1 branch adds one to a ring1 that already holds
>    the report.
>
> Either way the position is one period ahead. The next read is behind
> it, snd_pcm_update_hw_ptr0() takes the backwards step for a wrap of the
> whole buffer, and the stream underruns.
>
> While the channel runs, a zero residue only occurs between descriptors,
> so read the registers again while either residue is zero. The retries
> are bounded, as the residue also stays zero on a stopped channel or one
> that ran out of descriptors; after the last retry the position is
> computed as before.
>
> With a debug build counting retries, one hour of playback needed a
> retry for 2210 of 3.39 million reads, at most three retries (3.5 us),
> also across pause and stream restart. Both states above showed up in
> the logged register values. There were no underruns in three hours of
> playback with the fix.
>
> Fixes: b127315d9a78 ("dmaengine: apple-admac: Add Apple ADMAC driver")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Jacob Procee <sn125164@gmail.com>
> ---
Reviewed-by: Frank Li <Frank.Li@nxp.com>

Re: [PATCH] dmaengine: apple-admac: Fix position one period ahead at period boundary

[Sasha Finkelstein]

On Oct 29, 2026, at 11:59, Jacob Procee <sn125164@gmail.com> wrote:
> 
> - residue2 = readl_relaxed(ad->base + REG_RESIDUE(channo));
> + /*
> + * The residue drops to zero when a descriptor finishes, before the
> + * next one is loaded. If the finished descriptor's report is already
> + * in the ring at that point, the position below comes out one period
> + * ahead: with residue2 == 0 directly, with residue1 == 0 through the
> + * residue2 > residue1 branch. The next read is then behind it, which
> + * ALSA takes for a wrap of the whole buffer. Re-read until the next
> + * descriptor is loaded; the limit covers a stopped or starved channel,
> + * whose residue stays zero.
> + */

This wall of text belongs in the commit message, not in a comment.

Re: [PATCH] dmaengine: apple-admac: Fix position one period ahead at period boundary

[Martin Povišer]

> On 29. 10. 2026, at 11:59, Jacob Procee <sn125164@gmail.com> wrote:
> 
> Speaker playback on a MacBook Pro 14" (M1 Pro, J314s) underruns at
> irregular intervals, audible as a tick: 8 times in 3.7 hours of
> playback. Where an underrun was caught (4 of 8), the ALSA hw_ptr had
> jumped by one buffer size while about 70 ms of audio was still queued,
> and a kernel trace showed the DMA position stepping back by exactly one
> period at each of the 6 underruns it covered.
> 
> admac_cyclic_read_residue() derives the position from the report ring
> (periods finished but not yet reclaimed) and REG_RESIDUE (bytes left in
> the current descriptor). When a descriptor finishes, the residue first
> drops to zero, then the report is posted and the next descriptor is
> loaded. Usually the last two happen together, but occasionally the
> report comes first.

Hi Jacob,

this is an interesting change. I agree with your assessment that this boils down to assuming REG_REPORT_RING and REG_RESIDUE get updated in one atomic transaction after completion of each descriptor. To my recollection, the driver is written assuming this behavior as a guess. I have reverse engineered the block's interface by looking at MMIO traces and by direct experimentation, neither gives certainty on this point.

Rewriting admac_cyclic_read_residue to avoid the race altogether would be best but until then, your change looks to be a strict improvement.

Feel free to add
Acked-by: Martin Povišer <povik+lin@cutebit.org>

Best regards,
Martin

Re: [PATCH] dmaengine: apple-admac: Fix position one period ahead at period boundary

[Jacob Procee]

On 10/29/26 17:10, Martin Povišer wrote:
> Rewriting admac_cyclic_read_residue to avoid the race altogether would be
> best but until then, your change looks to be a strict improvement.

Hi Martin,

Thanks for the ack and the extra background info. I would love to take a
look at a race-free version of admac_cyclic_read_residue() as a separate
follow-up patch; v2 of this fix follows shortly. A complete test setup
is ready on my machine that traces every position read and logs the raw
registers. If you have any pointers on how REG_DESC_RING behaves, or
already have a direction in mind, that would help.

Best regards,
Jacob

Re: [PATCH] dmaengine: apple-admac: Fix position one period ahead at period boundary

[Jacob Procee]

On 10/30/26 08:35, Jacob Procee wrote:
> Thanks for the ack and the extra background info. I would love to take a
> look at a race-free version of admac_cyclic_read_residue() as a separate
> follow-up patch; v2 of this fix follows shortly.
Hello Martin,

I looked into this using REG_DESC_RING. It only contains descriptors
that the hardware hasn't fetched yet, and its read slot is effectively
the hardware's fetch pointer.

Unfortunately, it isn't updated atomically with the residue reload
either. Across 1.49 M reads, I hit 126 period boundaries. In 4 cases,
DESC_RING still showed the old state even though the residue had already
reloaded. In another case, DESC_RING had advanced while the residue was
still 0 and no report had been posted. So I don't see a way to get a
consistent, retry-free read from these registers.

One could make the retry check more comprehensive by using three
counters: outstanding = queued + playing + reported, with playing being
1 unless the residue is 0. This would also catch the reverse ordering,
where the report is posted after the descriptor is loaded, which the v2
check doesn't cover.

However, I left the debug module running overnight (10.65 h, with music
playing through the speakers), and never observed that reverse ordering.
There were no XRuns or backward steps. The speaker channel had 96.6 M
reads and 15,483 boundary hits; the second channel had 29.3 M reads and
86 hits.

All 1,498 snapshots that failed the three-counter check were explained
by read skew: 822 where DESC_RING was read before the fetch while the
residue was already full, and 676 where it was read after the fetch
while the residue was still 0. The positions returned to ALSA never
stepped backwards.

Given that the more complicated check would only handle a case I haven't
observed, I'd prefer to stick with the simpler v2 retry.

Unless you know something about the hardware ordering that these traces
wouldn't show, I'd prefer to stick with the simpler v2 retry.

Kind regards,
Jacob

Re: [PATCH] dmaengine: apple-admac: Fix position one period ahead at period boundary

[Martin Povišer]

> On 2. 11. 2026, at 10:45, Jacob Procee <sn125164@gmail.com> wrote:
> 
> On 10/30/26 08:35, Jacob Procee wrote:
>> Thanks for the ack and the extra background info. I would love to take a
>> look at a race-free version of admac_cyclic_read_residue() as a separate
>> follow-up patch; v2 of this fix follows shortly.
> Hello Martin,
> 
> I looked into this using REG_DESC_RING. It only contains descriptors
> that the hardware hasn't fetched yet, and its read slot is effectively
> the hardware's fetch pointer.
> 
> Unfortunately, it isn't updated atomically with the residue reload
> either. Across 1.49 M reads, I hit 126 period boundaries. In 4 cases,
> DESC_RING still showed the old state even though the residue had already
> reloaded. In another case, DESC_RING had advanced while the residue was
> still 0 and no report had been posted. So I don't see a way to get a
> consistent, retry-free read from these registers.
> 
> One could make the retry check more comprehensive by using three
> counters: outstanding = queued + playing + reported, with playing being
> 1 unless the residue is 0. This would also catch the reverse ordering,
> where the report is posted after the descriptor is loaded, which the v2
> check doesn't cover.
> 
> However, I left the debug module running overnight (10.65 h, with music
> playing through the speakers), and never observed that reverse ordering.
> There were no XRuns or backward steps. The speaker channel had 96.6 M
> reads and 15,483 boundary hits; the second channel had 29.3 M reads and
> 86 hits.
> 
> All 1,498 snapshots that failed the three-counter check were explained
> by read skew: 822 where DESC_RING was read before the fetch while the
> residue was already full, and 676 where it was read after the fetch
> while the residue was still 0. The positions returned to ALSA never
> stepped backwards.
> 
> Given that the more complicated check would only handle a case I haven't
> observed, I'd prefer to stick with the simpler v2 retry.
> 
> Unless you know something about the hardware ordering that these traces
> wouldn't show, I'd prefer to stick with the simpler v2 retry.
> 
> Kind regards,
> Jacob

Hi Jacob,

I don’t think it’s worth doing the more complicated check either. Your v2 patch looks to be good enough given the available information. Perhaps there’s a way to do the residue read race free by involving additional features of the block incl. registers the driver currently ignores but until someone discovers it I support going with your patch.

Best regards,
Martin