[PATCH 0/2] ASoC: SOF: initialise work immediately
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(-)
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];
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];
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;@@ -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);@@ -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);
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);
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.
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
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.
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!
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
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.
participants (3)
-
Guennadi Liakhovetski -
Mark Brown -
Pierre-Louis Bossart