Re: [PATCH v5 2/4] ALSA: compress: Add function to indicate the stream has gone bad

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 04/21/2016 03:26 AM, Charles Keepax wrote:
> On Wed, Apr 20, 2016 at 11:10:21AM -0500, Pierre-Louis Bossart wrote:
>>> + * snd_compr_stop_error: Report a fatal error on a stream
>>> + * @stream: pointer to stream
>>> + * @state: state to transition the stream to
>>> + *
>>> + * Stop the stream and set its state.
>>> + *
>>> + * Should be called with compressed device lock held.
>>> + */
>>> +int snd_compr_stop_error(struct snd_compr_stream *stream,
>>> +			 snd_pcm_state_t state)
>>> +{
>>> +	if (stream->runtime->state == state)
>>> +		return 0;
>>> +
>>> +	stream->runtime->state = state;
>> Minor nit-pick: should there be a consistency check to make sure the new
>> state makes sense - or maybe just a log to help debug? e.g. XRUN should only
>> come if the state in run or draining stages, applying the new state
>> unconditionally could lead to issues.
> I think given the function can now report more than just a XRUN
> it probably makes sense to set it unconditionally. As you might
> be reporting some error that doesn't require the stream to be
> running.
>
> It probably would make sense to only call trigger with
> TRIGGER_STOP if the stream is already running through. How about
> I add a check for the state in the delayed work? And I can
> certainly add a print to say the state was set, that probably
> makes sense anyway as it is an error being reported.
ok, a log would be fine.
>
>> And question for my education since I see no lock/mutex: is the state always
>> consistent or is there a risk of this state being changed while some other
>> thread or interrupt handling modifies it was well?
> As the comment says it is expected the lock should be held when
> calling the function. I could put a lockdep assert in, if we want
> to be cautious on this front?
I missed the comment, thanks the clarification.
No objections from me, this sounds good.