Re: [PATCH 2/2] ALSA: compress: Propagate real pointer errors in avail paths

[Takashi Iwai <tiwai@suse.de>]

On Tue, 14 Apr 2026 19:45:52 +0200,
Cássio Gabriel Monteiro Pires wrote:
> 
> On 4/14/26 13:53, Mark Brown wrote:
> > On Tue, Apr 14, 2026 at 01:47:54PM -0300, Cássio Gabriel wrote:
> > 
> >> Commit 61327f3d817c ("ALSA: compress: Pay attention if drivers
> >> error out retrieving pointers") made snd_compr_update_tstamp()
> >> return driver pointer() failures, but snd_compr_calc_avail()
> >> still ignores that status and continues with stale runtime
> >> counters.
> > 
> >> The fallback for an unsupported pointer callback remains
> >> intentional, so keep ignoring -EOPNOTSUPP there. Propagate
> >> real pointer-refresh failures instead.
> > 
> >> -static size_t snd_compr_calc_avail(struct snd_compr_stream *stream,
> >> -				   struct snd_compr_avail64 *avail)
> >> +static int snd_compr_calc_avail(struct snd_compr_stream *stream,
> >> +				struct snd_compr_avail64 *avail)
> >>  {
> >> +	int ret;
> >> +
> >>  	memset(avail, 0, sizeof(*avail));
> >> -	snd_compr_update_tstamp(stream, &avail->tstamp);
> >> -	/* Still need to return avail even if tstamp can't be filled in */
> >> +	ret = snd_compr_update_tstamp(stream, &avail->tstamp);
> >> +	if (ret < 0 && ret != -EOPNOTSUPP)
> >> +		return ret;
> >> +	/* Still need to return avail when no timestamp is available */
> > 
> > It's not clear to me that the intent there is to only ignore in the case
> > where we fail to update due to the operation not being supported rather
> > than to also ignore any other runtime errors we might encounter (eg, due
> > to the stream configuration).
> 
> Cool, thanks for replying!
> 
> I rechecked the current tree, and the hard-failure class does exist:
> wm_adsp_compr_pointer() can return -EIO and switch the stream to
> XRUN when dsp->fatal_error, !buf, or buf->error, so that is a
> real in-tree case where pointer() failure means the stream itself is
> broken rather than timestamp data simply being unavailable.
> 
> That said, I agree this does not fully justify the generic change as I
> sent it. In particular, SNDRV_COMPRESS_AVAIL and poll() both recheck
> the stream state after the pointer refresh, and there are other drivers
> such as SOF where pointer() can return -EBUSY for a not-ready case,
> which is not the same class as the wm_adsp -EIO/XRUN path.
> 
> I also agree that the current patch is too broad because it uses the raw pointer()
> errno as the policy boundary for avail handling, and that conflates hard
> stream-failure cases such as wm_adsp's -EIO/XRUN transition with
> other conditions like SOF's -EBUSY not-ready return. I can rework patch
> 2 so the behavior is based on a better-defined failure condition instead
> of treating every non-EOPNOTSUPP return as fatal for avail.
> 
> WDYT?

IMO, keeping the behavior could be safer for now.
The erroneous timestamp can be identified by zeros, at least.


thanks,

Takashi