[Sound-open-firmware] [PATCH] ASoC: SOF: fix uninitialised "work" with VirtIO

[Sridharan, Ranjani]

On Mon, Mar 23, 2020 at 2:20 AM Guennadi Liakhovetski <
guennadi.liakhovetski at linux.intel.com> wrote:

> Hi Ranjani,
>
> On Fri, Mar 20, 2020 at 09:31:50AM -0700, Sridharan, Ranjani wrote:
> > On Fri, Mar 20, 2020 at 5:36 AM Guennadi Liakhovetski <
> > guennadi.liakhovetski at linux.intel.com> wrote:
> >
> > > In the VirtIO case the sof_pcm_open() function isn't called on the
> > > host during guest streaming, which then leaves "work" structures
> > > uninitialised. However it is then used to handle position update
> > > messages from the DSP. Move their initialisation to immediately after
> > > allocation of the containing structure.
> > >
> > > Signed-off-by: Guennadi Liakhovetski <
> > > guennadi.liakhovetski at linux.intel.com>
> > > ---
> > >
> > > This is a re-send of "[PATCH 08/14] ASoC: SOF: fix uninitialised "work"
> > > with VirtIO" as suggested by Mark, also taking into account a comment
> > > from Ranjani - thanks. Note: I haven't sent patches before from mutt,
> > > hope this will work, if not - will have to re-send.
> > >
> > >  sound/soc/sof/pcm.c       |  4 +---
> > >  sound/soc/sof/sof-audio.h |  3 +++
> > >  sound/soc/sof/topology.c  | 17 ++++++++++++-----
> > >  3 files changed, 16 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
> > > index f4769e1..47cd741 100644
> > > --- a/sound/soc/sof/pcm.c
> > > +++ b/sound/soc/sof/pcm.c
> > > @@ -57,7 +57,7 @@ static int sof_pcm_dsp_params(struct snd_sof_pcm
> *spcm,
> > > struct snd_pcm_substream
> > >  /*
> > >   * sof pcm period elapse work
> > >   */
> > > -static void sof_pcm_period_elapsed_work(struct work_struct *work)
> > > +void snd_sof_pcm_period_elapsed_work(struct work_struct *work)
> > >  {
> > >         struct snd_sof_pcm_stream *sps =
> > >                 container_of(work, struct snd_sof_pcm_stream,
> > > @@ -475,8 +475,6 @@ static int sof_pcm_open(struct snd_soc_component
> > > *component,
> > >         dev_dbg(component->dev, "pcm: open stream %d dir %d\n",
> > >                 spcm->pcm.pcm_id, substream->stream);
> > >
> > > -       INIT_WORK(&spcm->stream[substream->stream].period_elapsed_work,
> > > -                 sof_pcm_period_elapsed_work);
> > >
> > >         caps = &spcm->pcm.caps[substream->stream];
> > >
> > > diff --git a/sound/soc/sof/sof-audio.h b/sound/soc/sof/sof-audio.h
> > > index eacd10e..bf65f31a 100644
> > > --- a/sound/soc/sof/sof-audio.h
> > > +++ b/sound/soc/sof/sof-audio.h
> > > @@ -11,6 +11,8 @@
> > >  #ifndef __SOUND_SOC_SOF_AUDIO_H
> > >  #define __SOUND_SOC_SOF_AUDIO_H
> > >
> > > +#include <linux/workqueue.h>
> > > +
> > >  #include <sound/soc.h>
> > >  #include <sound/control.h>
> > >  #include <sound/sof/stream.h> /* needs to be included before
> control.h */
> > > @@ -189,6 +191,7 @@ struct snd_sof_pcm *snd_sof_find_spcm_comp(struct
> > > snd_soc_component *scomp,
> > >  struct snd_sof_pcm *snd_sof_find_spcm_pcm_id(struct snd_soc_component
> > > *scomp,
> > >                                              unsigned int pcm_id);
> > >  void snd_sof_pcm_period_elapsed(struct snd_pcm_substream *substream);
> > > +void snd_sof_pcm_period_elapsed_work(struct work_struct *work);
> > >
> > >  /*
> > >   * Mixer IPC
> > > diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
> > > index 058de94..fe8ba3e 100644
> > > --- a/sound/soc/sof/topology.c
> > > +++ b/sound/soc/sof/topology.c
> > > @@ -9,6 +9,7 @@
> > >  //
> > >
> > >  #include <linux/firmware.h>
> > > +#include <linux/workqueue.h>
> > >  #include <sound/tlv.h>
> > >  #include <sound/pcm_params.h>
> > >  #include <uapi/sound/sof/tokens.h>
> > > @@ -2448,7 +2449,7 @@ static int sof_dai_load(struct snd_soc_component
> > > *scomp, int index,
> > >         struct snd_soc_tplg_stream_caps *caps;
> > >         struct snd_soc_tplg_private *private = &pcm->priv;
> > >         struct snd_sof_pcm *spcm;
> > > -       int stream = SNDRV_PCM_STREAM_PLAYBACK;
> > > +       int stream;
> > >         int ret = 0;
> > >
> > >         /* nothing to do for BEs atm */
> > > @@ -2460,8 +2461,12 @@ static int sof_dai_load(struct snd_soc_component
> > > *scomp, int index,
> > >                 return -ENOMEM;
> > >
> > >         spcm->scomp = scomp;
> > > -       spcm->stream[SNDRV_PCM_STREAM_PLAYBACK].comp_id =
> > > COMP_ID_UNASSIGNED;
> > > -       spcm->stream[SNDRV_PCM_STREAM_CAPTURE].comp_id =
> > > COMP_ID_UNASSIGNED;
> > > +
> > > +       for_each_pcm_streams(stream) {
> > > +               spcm->stream[stream].comp_id = COMP_ID_UNASSIGNED;
> > > +               INIT_WORK(&spcm->stream[stream].period_elapsed_work,
> > > +                         snd_sof_pcm_period_elapsed_work);
> > > +       }
> > >
> > >         spcm->pcm = *pcm;
> > >         dev_dbg(scomp->dev, "tplg: load pcm %s\n", pcm->dai_name);
> > > @@ -2482,8 +2487,10 @@ static int sof_dai_load(struct snd_soc_component
> > > *scomp, int index,
> > >         if (!spcm->pcm.playback)
> > >                 goto capture;
> > >
> > > +       stream = SNDRV_PCM_STREAM_PLAYBACK;
> > > +
> > >         dev_vdbg(scomp->dev, "tplg: pcm %s stream tokens: playback
> > > d0i3:%d\n",
> > > -                spcm->pcm.pcm_name, spcm->stream[0].d0i3_compatible);
> > > +                spcm->pcm.pcm_name,
> spcm->stream[stream].d0i3_compatible);
> > >
> > Hi Guennadi,
> >
> > This cleanup is unrelated to the commit message (and the one below)?
> Should
> > it be a separate patch?
>
> I think it is a matter of judgement. It was your request to use
> for_each_pcm_streams() which I did. While transitioning to it I noticed a
> couple of related inconsistencies which I then aso fixed. So IMHO the
> change is minor, obvious and related. But I agree that this now makes the
> patch a bit controversial.

Hi Guennadi,

May I suggest to break it up into 2 patches: First, to move to using the
new for_each_pcm_stream_macro() and  the second to add the add the work
init. That way there would be no confusion.

Thanks,
Ranjani