[PATCH 0/2] ASoC: SOF: initialise work immediately

[PATCH 0/2] ASoC: SOF: initialise work immediately

[Guennadi Liakhovetski]

Fix uninitialised work errors by moving initialisation to directly
after allocation.

Guennadi Liakhovetski (2):
  ASoC: SOF: (cosmetic) use for_each_pcm_streams() in sof_dai_load()
  ASoC: SOF: fix uninitialised "work" with VirtIO

 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(-)

-- 
1.9.3

[PATCH 1/2] ASoC: SOF: (cosmetic) use for_each_pcm_streams() in sof_dai_load()

[Guennadi Liakhovetski]

Use for_each_pcm_streams() to enumerate streams in sof_dai_load()
instead of doing that manually.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
---
 sound/soc/sof/topology.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
index 058de94..54437ca 100644
--- a/sound/soc/sof/topology.c
+++ b/sound/soc/sof/topology.c
@@ -2448,7 +2448,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 +2460,9 @@ 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;
 
 	spcm->pcm = *pcm;
 	dev_dbg(scomp->dev, "tplg: load pcm %s\n", pcm->dai_name);
@@ -2482,8 +2483,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);
 
 	caps = &spcm->pcm.caps[stream];
 
@@ -2513,7 +2516,7 @@ static int sof_dai_load(struct snd_soc_component *scomp, int index,
 		return ret;
 
 	dev_vdbg(scomp->dev, "tplg: pcm %s stream tokens: capture d0i3:%d\n",
-		 spcm->pcm.pcm_name, spcm->stream[1].d0i3_compatible);
+		 spcm->pcm.pcm_name, spcm->stream[stream].d0i3_compatible);
 
 	caps = &spcm->pcm.caps[stream];
 
-- 
1.9.3

Re: [PATCH 1/2] ASoC: SOF: (cosmetic) use for_each_pcm_streams() in sof_dai_load()

[Pierre-Louis Bossart]

Guennadi, who are the intended reviewers/maintainers here and which tree 
is this supposed to be merged on?

Patch0 has everyone copied, patch 1/2 only target mailing lists, and I 
haven't seen any PR/CI for this?

I am flexible with processes, I am just lost here on what you had in mind.

Thanks
-Pierre

On 3/24/20 7:29 AM, Guennadi Liakhovetski wrote:
> Use for_each_pcm_streams() to enumerate streams in sof_dai_load()
> instead of doing that manually.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> ---
>   sound/soc/sof/topology.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
> index 058de94..54437ca 100644
> --- a/sound/soc/sof/topology.c
> +++ b/sound/soc/sof/topology.c
> @@ -2448,7 +2448,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 +2460,9 @@ 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;
>   
>   	spcm->pcm = *pcm;
>   	dev_dbg(scomp->dev, "tplg: load pcm %s\n", pcm->dai_name);
> @@ -2482,8 +2483,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);
>   
>   	caps = &spcm->pcm.caps[stream];
>   
> @@ -2513,7 +2516,7 @@ static int sof_dai_load(struct snd_soc_component *scomp, int index,
>   		return ret;
>   
>   	dev_vdbg(scomp->dev, "tplg: pcm %s stream tokens: capture d0i3:%d\n",
> -		 spcm->pcm.pcm_name, spcm->stream[1].d0i3_compatible);
> +		 spcm->pcm.pcm_name, spcm->stream[stream].d0i3_compatible);
>   
>   	caps = &spcm->pcm.caps[stream];
>   
> 

Re: [PATCH 1/2] ASoC: SOF: (cosmetic) use for_each_pcm_streams() in sof_dai_load()

[Guennadi Liakhovetski]

Hi Pierre,

On Tue, Mar 24, 2020 at 08:29:43AM -0500, Pierre-Louis Bossart wrote:
> Guennadi, who are the intended reviewers/maintainers here and which tree is
> this supposed to be merged on?
> 
> Patch0 has everyone copied, patch 1/2 only target mailing lists, and I
> haven't seen any PR/CI for this?

Yes, as explained in my reply to Mark, I didn't want to spam everybody's mail 
boxes with all the patches, so I only copied the "patch 0" to maintainers 
and sent the patches themselves to lists only.

> I am flexible with processes, I am just lost here on what you had in mind.

I was advised to make this a separate patch from my VirtIO patch series, so 
I did that and sent it again directly to the mailing lists. But we can take 
it back to the local review process, if you prefer.

Thanks
Guennadi

> Thanks
> -Pierre
> 
> On 3/24/20 7:29 AM, Guennadi Liakhovetski wrote:
> > Use for_each_pcm_streams() to enumerate streams in sof_dai_load()
> > instead of doing that manually.
> > 
> > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> > ---
> >   sound/soc/sof/topology.c | 13 ++++++++-----
> >   1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
> > index 058de94..54437ca 100644
> > --- a/sound/soc/sof/topology.c
> > +++ b/sound/soc/sof/topology.c
> > @@ -2448,7 +2448,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 +2460,9 @@ 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;
> >   	spcm->pcm = *pcm;
> >   	dev_dbg(scomp->dev, "tplg: load pcm %s\n", pcm->dai_name);
> > @@ -2482,8 +2483,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);
> >   	caps = &spcm->pcm.caps[stream];
> > @@ -2513,7 +2516,7 @@ static int sof_dai_load(struct snd_soc_component *scomp, int index,
> >   		return ret;
> >   	dev_vdbg(scomp->dev, "tplg: pcm %s stream tokens: capture d0i3:%d\n",
> > -		 spcm->pcm.pcm_name, spcm->stream[1].d0i3_compatible);
> > +		 spcm->pcm.pcm_name, spcm->stream[stream].d0i3_compatible);
> >   	caps = &spcm->pcm.caps[stream];
> > 

[PATCH 2/2] ASoC: SOF: fix uninitialised "work" with VirtIO

[Guennadi Liakhovetski]

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@linux.intel.com>
---
 sound/soc/sof/pcm.c       | 4 +---
 sound/soc/sof/sof-audio.h | 3 +++
 sound/soc/sof/topology.c  | 6 +++++-
 3 files changed, 9 insertions(+), 4 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 54437ca..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>
@@ -2461,8 +2462,11 @@ static int sof_dai_load(struct snd_soc_component *scomp, int index,
 
 	spcm->scomp = scomp;
 
-	for_each_pcm_streams(stream)
+	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);
-- 
1.9.3

Re: [PATCH 0/2] ASoC: SOF: initialise work immediately

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

On Tue, Mar 24, 2020 at 01:29:19PM +0100, Guennadi Liakhovetski wrote:
> Fix uninitialised work errors by moving initialisation to directly
> after allocation.
> 
> Guennadi Liakhovetski (2):
>   ASoC: SOF: (cosmetic) use for_each_pcm_streams() in sof_dai_load()
>   ASoC: SOF: fix uninitialised "work" with VirtIO

As documented in submitting-patches.rst please send patches to the 
maintainers for the code you would like to change.  The normal kernel
workflow is that people apply patches from their inboxes, if they aren't
copied they are likely to not see the patch at all and it is much more
difficult to apply patches.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] ASoC: SOF: initialise work immediately

[Guennadi Liakhovetski]

Hi Mark,

On Tue, Mar 24, 2020 at 01:30:42PM +0000, Mark Brown wrote:
> On Tue, Mar 24, 2020 at 01:29:19PM +0100, Guennadi Liakhovetski wrote:
> > Fix uninitialised work errors by moving initialisation to directly
> > after allocation.
> > 
> > Guennadi Liakhovetski (2):
> >   ASoC: SOF: (cosmetic) use for_each_pcm_streams() in sof_dai_load()
> >   ASoC: SOF: fix uninitialised "work" with VirtIO
> 
> As documented in submitting-patches.rst please send patches to the 
> maintainers for the code you would like to change.  The normal kernel
> workflow is that people apply patches from their inboxes, if they aren't
> copied they are likely to not see the patch at all and it is much more
> difficult to apply patches.

I know that different maintainers have different preferences. For example 
in the subsysteem, where I'd worked for about 10 years the maintainer 
preferred not to be CCed on patches, he preferred to pick up patches from 
his mailing list folders, or whatever arrangement his mail filters 
provided for. I learned already that in ALSA / ASoC it's usual to CC 
maintainers. But I wasn't sure whether that also holds for larger patch 
series. E.g. my main patch series now consists of 14 patches, so, I 
thought, that maybe you would rather not receive multiple copies of the 
entire seriees for each new version both directly in your inbox and in 
the mailing list folder. Or is it indeed your preference to always be 
CCed on all patches? I apologise for re-iterating a question, that 
probably had been addressed multiple times before, maybe it's worth 
documenting this somewhere on ALSA web?

Thanks
Guennadi

Re: [PATCH 0/2] ASoC: SOF: initialise work immediately

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1725 bytes --]

On Tue, Mar 24, 2020 at 02:58:56PM +0100, Guennadi Liakhovetski wrote:
> On Tue, Mar 24, 2020 at 01:30:42PM +0000, Mark Brown wrote:

> > As documented in submitting-patches.rst please send patches to the 
> > maintainers for the code you would like to change.  The normal kernel
> > workflow is that people apply patches from their inboxes, if they aren't
> > copied they are likely to not see the patch at all and it is much more
> > difficult to apply patches.

> I know that different maintainers have different preferences. For example 
> in the subsysteem, where I'd worked for about 10 years the maintainer 
> preferred not to be CCed on patches, he preferred to pick up patches from 
> his mailing list folders, or whatever arrangement his mail filters 
> provided for. I learned already that in ALSA / ASoC it's usual to CC 
> maintainers. But I wasn't sure whether that also holds for larger patch 
> series. E.g. my main patch series now consists of 14 patches, so, I 
> thought, that maybe you would rather not receive multiple copies of the 
> entire seriees for each new version both directly in your inbox and in 
> the mailing list folder. Or is it indeed your preference to always be 
> CCed on all patches? I apologise for re-iterating a question, that 
> probably had been addressed multiple times before, maybe it's worth 
> documenting this somewhere on ALSA web?

Yes, copy me on patches.  This is, as covered in what I wrote above, the
standard and documented approach for the kernel - unless you explicitly
know that there is some unusual approach for a specific subsystem you
should assume that if you want people to see your patches you need to
send the patches to them.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/2] ASoC: SOF: initialise work immediately

[Guennadi Liakhovetski]

On Tue, Mar 24, 2020 at 02:48:22PM +0000, Mark Brown wrote:
> On Tue, Mar 24, 2020 at 02:58:56PM +0100, Guennadi Liakhovetski wrote:
> > On Tue, Mar 24, 2020 at 01:30:42PM +0000, Mark Brown wrote:
> 
> > > As documented in submitting-patches.rst please send patches to the 
> > > maintainers for the code you would like to change.  The normal kernel
> > > workflow is that people apply patches from their inboxes, if they aren't
> > > copied they are likely to not see the patch at all and it is much more
> > > difficult to apply patches.
> 
> > I know that different maintainers have different preferences. For example 
> > in the subsysteem, where I'd worked for about 10 years the maintainer 
> > preferred not to be CCed on patches, he preferred to pick up patches from 
> > his mailing list folders, or whatever arrangement his mail filters 
> > provided for. I learned already that in ALSA / ASoC it's usual to CC 
> > maintainers. But I wasn't sure whether that also holds for larger patch 
> > series. E.g. my main patch series now consists of 14 patches, so, I 
> > thought, that maybe you would rather not receive multiple copies of the 
> > entire seriees for each new version both directly in your inbox and in 
> > the mailing list folder. Or is it indeed your preference to always be 
> > CCed on all patches? I apologise for re-iterating a question, that 
> > probably had been addressed multiple times before, maybe it's worth 
> > documenting this somewhere on ALSA web?
> 
> Yes, copy me on patches.  This is, as covered in what I wrote above, the
> standard and documented approach for the kernel - unless you explicitly
> know that there is some unusual approach for a specific subsystem you
> should assume that if you want people to see your patches you need to
> send the patches to them.

Got it, thanks!

Re: [PATCH 0/2] ASoC: SOF: initialise work immediately

[Guennadi Liakhovetski]

Hi Mark,

On Tue, Mar 24, 2020 at 01:30:42PM +0000, Mark Brown wrote:
> On Tue, Mar 24, 2020 at 01:29:19PM +0100, Guennadi Liakhovetski wrote:
> > Fix uninitialised work errors by moving initialisation to directly
> > after allocation.
> > 
> > Guennadi Liakhovetski (2):
> >   ASoC: SOF: (cosmetic) use for_each_pcm_streams() in sof_dai_load()
> >   ASoC: SOF: fix uninitialised "work" with VirtIO
> 
> As documented in submitting-patches.rst please send patches to the 
> maintainers for the code you would like to change.  The normal kernel
> workflow is that people apply patches from their inboxes, if they aren't
> copied they are likely to not see the patch at all and it is much more
> difficult to apply patches.

An update: these patches got merged into the SOF tree, so, no need to 
merge them to ASoC directly.

Thanks
Guennadi

Re: [PATCH 0/2] ASoC: SOF: initialise work immediately

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

On Wed, Mar 25, 2020 at 12:10:12PM +0100, Guennadi Liakhovetski wrote:
> On Tue, Mar 24, 2020 at 01:30:42PM +0000, Mark Brown wrote:

> > As documented in submitting-patches.rst please send patches to the 
> > maintainers for the code you would like to change.  The normal kernel
> > workflow is that people apply patches from their inboxes, if they aren't
> > copied they are likely to not see the patch at all and it is much more
> > difficult to apply patches.

> An update: these patches got merged into the SOF tree, so, no need to 
> merge them to ASoC directly.

OK, for reference it's not normal to submit things to both SOF and to
upstream - one or the other is more normal.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]