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

["Cássio Gabriel Monteiro Pires" <cassiogabrielcontato@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3046 bytes --]



On 4/15/26 11:20, Takashi Iwai wrote:
> 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.

That sounds right, I'll drop this change for now.

-- 
Thanks,
Cássio


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]