| Message ID | 20221114113729.1022905-2-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: Intel: avs: DSP recovery and resume fixes | expand |
On Mon, 14 Nov 2022 12:37:28 +0100, Cezary Rojewski wrote: > > snd_pcm_stop() shall be called with stream lock held to prevent any > races between nonatomic streaming operations. > > Fixes: 2f1f570cd730 ("ASoC: Intel: avs: Coredump and recovery flow") > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > sound/soc/intel/avs/ipc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/sound/soc/intel/avs/ipc.c b/sound/soc/intel/avs/ipc.c > index 152f8d0bdf8e..07655298b6c7 100644 > --- a/sound/soc/intel/avs/ipc.c > +++ b/sound/soc/intel/avs/ipc.c > @@ -123,7 +123,9 @@ static void avs_dsp_recovery(struct avs_dev *adev) > if (!substream || !substream->runtime) > continue; > > + snd_pcm_stream_lock(substream); > snd_pcm_stop(substream, SNDRV_PCM_STATE_DISCONNECTED); > + snd_pcm_stream_unlock(substream); Isn't it in the interruptible context? If so, you need the _irq() variant. Takashi
On Mon, 14 Nov 2022 14:00:12 +0100, Takashi Iwai wrote: > > On Mon, 14 Nov 2022 12:37:28 +0100, > Cezary Rojewski wrote: > > > > snd_pcm_stop() shall be called with stream lock held to prevent any > > races between nonatomic streaming operations. > > > > Fixes: 2f1f570cd730 ("ASoC: Intel: avs: Coredump and recovery flow") > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > > --- > > sound/soc/intel/avs/ipc.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/sound/soc/intel/avs/ipc.c b/sound/soc/intel/avs/ipc.c > > index 152f8d0bdf8e..07655298b6c7 100644 > > --- a/sound/soc/intel/avs/ipc.c > > +++ b/sound/soc/intel/avs/ipc.c > > @@ -123,7 +123,9 @@ static void avs_dsp_recovery(struct avs_dev *adev) > > if (!substream || !substream->runtime) > > continue; > > > > + snd_pcm_stream_lock(substream); > > snd_pcm_stop(substream, SNDRV_PCM_STATE_DISCONNECTED); > > + snd_pcm_stream_unlock(substream); > > Isn't it in the interruptible context? If so, you need the _irq() > variant. Correction: when it's a non-atomic stream, that doesn't matter, both are identical. Takashi
On 2022-11-14 2:16 PM, Takashi Iwai wrote: > On Mon, 14 Nov 2022 14:00:12 +0100, > Takashi Iwai wrote: >> On Mon, 14 Nov 2022 12:37:28 +0100, >> Cezary Rojewski wrote: ... >>> @@ -123,7 +123,9 @@ static void avs_dsp_recovery(struct avs_dev *adev) >>> if (!substream || !substream->runtime) >>> continue; >>> >>> + snd_pcm_stream_lock(substream); >>> snd_pcm_stop(substream, SNDRV_PCM_STATE_DISCONNECTED); >>> + snd_pcm_stream_unlock(substream); >> >> Isn't it in the interruptible context? If so, you need the _irq() >> variant. Thanks for paying attention to detail, Takashi. > Correction: when it's a non-atomic stream, that doesn't matter, both > are identical. In regard to the point found above, that's precisely why I decided to use variant with shorten name. Matter of taste I guess. Can switch to _irq() if you believe it's the correct approach. Regards, Czarek
On Mon, 14 Nov 2022 14:25:31 +0100, Cezary Rojewski wrote: > > On 2022-11-14 2:16 PM, Takashi Iwai wrote: > > On Mon, 14 Nov 2022 14:00:12 +0100, > > Takashi Iwai wrote: > >> On Mon, 14 Nov 2022 12:37:28 +0100, > >> Cezary Rojewski wrote: > > ... > > >>> @@ -123,7 +123,9 @@ static void avs_dsp_recovery(struct avs_dev *adev) > >>> if (!substream || !substream->runtime) > >>> continue; > >>> + snd_pcm_stream_lock(substream); > >>> snd_pcm_stop(substream, SNDRV_PCM_STATE_DISCONNECTED); > >>> + snd_pcm_stream_unlock(substream); > >> > >> Isn't it in the interruptible context? If so, you need the _irq() > >> variant. > > Thanks for paying attention to detail, Takashi. > > > Correction: when it's a non-atomic stream, that doesn't matter, both > > are identical. > > In regard to the point found above, that's precisely why I decided to > use variant with shorten name. Matter of taste I guess. Can switch to > _irq() if you believe it's the correct approach. It's fine in both ways. For the snd_pcm_stream_lock(), maybe worth to put a comment, though. thanks, Takashi
diff --git a/sound/soc/intel/avs/ipc.c b/sound/soc/intel/avs/ipc.c index 152f8d0bdf8e..07655298b6c7 100644 --- a/sound/soc/intel/avs/ipc.c +++ b/sound/soc/intel/avs/ipc.c @@ -123,7 +123,9 @@ static void avs_dsp_recovery(struct avs_dev *adev) if (!substream || !substream->runtime) continue; + snd_pcm_stream_lock(substream); snd_pcm_stop(substream, SNDRV_PCM_STATE_DISCONNECTED); + snd_pcm_stream_unlock(substream); } } }
snd_pcm_stop() shall be called with stream lock held to prevent any races between nonatomic streaming operations. Fixes: 2f1f570cd730 ("ASoC: Intel: avs: Coredump and recovery flow") Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- sound/soc/intel/avs/ipc.c | 2 ++ 1 file changed, 2 insertions(+)