Re: snd_pcsp locking mess

[Stas Sergeev <stsp@aknet.ru>]

Hello.

Takashi Iwai wrote:
> Well, let's take a look back how the callbacks are called.
> The callbacks that are called during PCM running are the following:
> 
> - hrtimer callback gets called from hrtimer:
>   This updates the pcsp port and updates the PCM pointer.
>   
> - PCM pointer callback:
>   This returns the current PCM pointer
> 
> So, actually between these calls, we have only one thing to protect -
> the PCM pointer.  The DMA-buffer readiness is necessary only for
> hrtimer callback.
OK.

> And, in addition, we need to call somewhere snd_pcm_period_elapsed()
> when the period is finished.  This can be done in a tasklet.
What is the difference between calling
everything in softirq (current solution)
and calling snd_pcm_period_elapsed() in
a tasklet, with the rest in a hardirq
context? What will this solve?

> Now, remember that hrtimer is started only via trigger start
> callback.  That is, hrtimer callback isn called first after PCM is
> actually started; hence the DMA buffer is ready when it's called.
> 
> Then, if we assure that the DMA buffer isn't changed and the PCM
> substream still exists during hrtimer callback (and/or its execution
> finishes), hrtimer callback can safely run without *any* lock.
OK so how do you protect the PCM pointer
after all? Even in your example you use
the chip->substream_lock for that (why?),
so it doesn't look like you run a callback
without any lock at all.

> This is pretty easy.  We just need to add appropriate calls to
> synchronize the finish of hrtimer (and tasklet) at each place that can
> change the DMA buffers.
I wonder if this is SMP-safe (comments
below).

> +/*
> + * Force to stop and sync the stream
> + */
> +void pcsp_sync_stop(struct snd_pcsp *chip)
> +{
> +	local_irq_disable();
So what will happen if the timer callback
is executing on another CPU at that point?

> +	pcsp_stop_playing(chip);
This will set chip->timer_active to 0,
but who knows if the callback on another
CPU have already passed the check or not?

> +	local_irq_enable();
> +	hrtimer_cancel(&chip->timer);
> +	tasklet_kill(&pcsp_pcm_tasklet);
Ouch, IIRC all three are discouraged to
use...

I (hopefully) now understand the logic
you are trying to explain, and I don't
think it is completely trouble-free (I
may be wrong of course, but certainly
it won't be straightforward for every
reader why is that code correct).
So the question is still why not to
simply use snd_pcm_stream_lock(). And
if right now it is barely usefull, then
why it can't be improved to simply not
require a second lock that guards a
substream itself?