[PATCH 1/2] ASoC: wcd9335: fix order of Slimbus unprepare/disable
Slimbus streams are first prepared and then enabled, so the cleanup path should reverse it. The unprepare sets stream->num_ports to 0 and frees the stream->ports. Calling disable after unprepare was not really effective (channels was not deactivated) and could lead to further issues due to making transfers on unprepared stream.
Fixes: 20aedafdf492 ("ASoC: wcd9335: add support to wcd9335 codec") Cc: stable@vger.kernel.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org --- sound/soc/codecs/wcd9335.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/wcd9335.c b/sound/soc/codecs/wcd9335.c index 06c6adbe5920..d2548fdf9ae5 100644 --- a/sound/soc/codecs/wcd9335.c +++ b/sound/soc/codecs/wcd9335.c @@ -1972,8 +1972,8 @@ static int wcd9335_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - slim_stream_unprepare(dai_data->sruntime); slim_stream_disable(dai_data->sruntime); + slim_stream_unprepare(dai_data->sruntime); break; default: break;
Slimbus streams are first prepared and then enabled, so the cleanup path should reverse it. The unprepare sets stream->num_ports to 0 and frees the stream->ports. Calling disable after unprepare was not really effective (channels was not deactivated) and could lead to further issues due to making transfers on unprepared stream.
Fixes: a61f3b4f476e ("ASoC: wcd934x: add support to wcd9340/wcd9341 codec") Cc: stable@vger.kernel.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org --- sound/soc/codecs/wcd934x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c index f56907d0942d..28175c746b9a 100644 --- a/sound/soc/codecs/wcd934x.c +++ b/sound/soc/codecs/wcd934x.c @@ -1913,8 +1913,8 @@ static int wcd934x_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - slim_stream_unprepare(dai_data->sruntime); slim_stream_disable(dai_data->sruntime); + slim_stream_unprepare(dai_data->sruntime); break; default: break;
On 21/09/2022 15:53, Krzysztof Kozlowski wrote:
Slimbus streams are first prepared and then enabled, so the cleanup path should reverse it. The unprepare sets stream->num_ports to 0 and frees the stream->ports. Calling disable after unprepare was not really effective (channels was not deactivated) and could lead to further issues due to making transfers on unprepared stream.
Fixes: a61f3b4f476e ("ASoC: wcd934x: add support to wcd9340/wcd9341 codec") Cc: stable@vger.kernel.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
Reviewed-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org
sound/soc/codecs/wcd934x.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c index f56907d0942d..28175c746b9a 100644 --- a/sound/soc/codecs/wcd934x.c +++ b/sound/soc/codecs/wcd934x.c @@ -1913,8 +1913,8 @@ static int wcd934x_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
slim_stream_unprepare(dai_data->sruntime);
slim_stream_unprepare(dai_data->sruntime);
On 9/21/22 16:53, Krzysztof Kozlowski wrote:
Slimbus streams are first prepared and then enabled, so the cleanup path should reverse it. The unprepare sets stream->num_ports to 0 and frees the stream->ports. Calling disable after unprepare was not really effective (channels was not deactivated) and could lead to further issues due to making transfers on unprepared stream.
Fixes: 20aedafdf492 ("ASoC: wcd9335: add support to wcd9335 codec") Cc: stable@vger.kernel.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
sound/soc/codecs/wcd9335.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/wcd9335.c b/sound/soc/codecs/wcd9335.c index 06c6adbe5920..d2548fdf9ae5 100644 --- a/sound/soc/codecs/wcd9335.c +++ b/sound/soc/codecs/wcd9335.c @@ -1972,8 +1972,8 @@ static int wcd9335_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
slim_stream_unprepare(dai_data->sruntime);
slim_stream_unprepare(dai_data->sruntime);
This looks logical but different from what the kernel doc says:
/** * slim_stream_disable() - Disable a SLIMbus Stream * * @stream: instance of slim stream runtime to disable * * This API will disable all the ports and channels associated with * SLIMbus stream * * Return: zero on success and error code on failure. From ASoC DPCM framework, * this state is linked to trigger() pause operation. */
/** * slim_stream_unprepare() - Un-prepare a SLIMbus Stream * * @stream: instance of slim stream runtime to unprepare * * This API will un allocate all the ports and channels associated with * SLIMbus stream * * Return: zero on success and error code on failure. From ASoC DPCM framework, * this state is linked to trigger() stop operation. */
I would bet the documentation is incorrect?
break;default: break;
On 21/09/2022 17:05, Pierre-Louis Bossart wrote:
diff --git a/sound/soc/codecs/wcd9335.c b/sound/soc/codecs/wcd9335.c index 06c6adbe5920..d2548fdf9ae5 100644 --- a/sound/soc/codecs/wcd9335.c +++ b/sound/soc/codecs/wcd9335.c @@ -1972,8 +1972,8 @@ static int wcd9335_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
slim_stream_unprepare(dai_data->sruntime);
slim_stream_unprepare(dai_data->sruntime);This looks logical but different from what the kernel doc says:
/**
- slim_stream_disable() - Disable a SLIMbus Stream
- @stream: instance of slim stream runtime to disable
- This API will disable all the ports and channels associated with
- SLIMbus stream
- Return: zero on success and error code on failure. From ASoC DPCM
framework,
- this state is linked to trigger() pause operation.
*/
/**
- slim_stream_unprepare() - Un-prepare a SLIMbus Stream
- @stream: instance of slim stream runtime to unprepare
- This API will un allocate all the ports and channels associated with
- SLIMbus stream
You mean this piece of doc? Indeed looks inaccurate. I'll update it.
Best regards, Krzysztof
On 21/09/2022 17:06, Krzysztof Kozlowski wrote:
On 21/09/2022 17:05, Pierre-Louis Bossart wrote:
diff --git a/sound/soc/codecs/wcd9335.c b/sound/soc/codecs/wcd9335.c index 06c6adbe5920..d2548fdf9ae5 100644 --- a/sound/soc/codecs/wcd9335.c +++ b/sound/soc/codecs/wcd9335.c @@ -1972,8 +1972,8 @@ static int wcd9335_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
slim_stream_unprepare(dai_data->sruntime);
slim_stream_unprepare(dai_data->sruntime);This looks logical but different from what the kernel doc says:
/**
- slim_stream_disable() - Disable a SLIMbus Stream
- @stream: instance of slim stream runtime to disable
- This API will disable all the ports and channels associated with
- SLIMbus stream
- Return: zero on success and error code on failure. From ASoC DPCM
framework,
- this state is linked to trigger() pause operation.
*/
/**
- slim_stream_unprepare() - Un-prepare a SLIMbus Stream
- @stream: instance of slim stream runtime to unprepare
- This API will un allocate all the ports and channels associated with
- SLIMbus stream
You mean this piece of doc? Indeed looks inaccurate. I'll update it.
Wait, no, this is correct. Please point to what is wrong in kernel doc. I don't see it. :(
Best regards, Krzysztof
On 9/21/22 17:08, Krzysztof Kozlowski wrote:
On 21/09/2022 17:06, Krzysztof Kozlowski wrote:
On 21/09/2022 17:05, Pierre-Louis Bossart wrote:
diff --git a/sound/soc/codecs/wcd9335.c b/sound/soc/codecs/wcd9335.c index 06c6adbe5920..d2548fdf9ae5 100644 --- a/sound/soc/codecs/wcd9335.c +++ b/sound/soc/codecs/wcd9335.c @@ -1972,8 +1972,8 @@ static int wcd9335_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
slim_stream_unprepare(dai_data->sruntime);
slim_stream_unprepare(dai_data->sruntime);This looks logical but different from what the kernel doc says:
/**
- slim_stream_disable() - Disable a SLIMbus Stream
- @stream: instance of slim stream runtime to disable
- This API will disable all the ports and channels associated with
- SLIMbus stream
- Return: zero on success and error code on failure. From ASoC DPCM
framework,
- this state is linked to trigger() pause operation.
*/
/**
- slim_stream_unprepare() - Un-prepare a SLIMbus Stream
- @stream: instance of slim stream runtime to unprepare
- This API will un allocate all the ports and channels associated with
- SLIMbus stream
You mean this piece of doc? Indeed looks inaccurate. I'll update it.
Wait, no, this is correct. Please point to what is wrong in kernel doc. I don't see it. :(
the TRIGGER_STOP and TRIGGER_PAUSE_PUSH do the same thing. There is no specific mapping of disable() to TRIGGER_STOP and unprepare() to TRIGGER_PAUSE_PUSH as the documentation hints at.
On 21/09/2022 17:11, Pierre-Louis Bossart wrote:
/**
- slim_stream_unprepare() - Un-prepare a SLIMbus Stream
- @stream: instance of slim stream runtime to unprepare
- This API will un allocate all the ports and channels associated with
- SLIMbus stream
You mean this piece of doc? Indeed looks inaccurate. I'll update it.
Wait, no, this is correct. Please point to what is wrong in kernel doc. I don't see it. :(
the TRIGGER_STOP and TRIGGER_PAUSE_PUSH do the same thing. There is no specific mapping of disable() to TRIGGER_STOP and unprepare() to TRIGGER_PAUSE_PUSH as the documentation hints at.
Which TRIGGER_STOP and TRIGGER_PAUSE_PUSH? In one specific codec driver? If yes, I don't think Slimbus documentation should care how actual users implement it (e.g. coalesce states).
Best regards, Krzysztof
On 9/21/22 17:19, Krzysztof Kozlowski wrote:
On 21/09/2022 17:11, Pierre-Louis Bossart wrote:
/**
- slim_stream_unprepare() - Un-prepare a SLIMbus Stream
- @stream: instance of slim stream runtime to unprepare
- This API will un allocate all the ports and channels associated with
- SLIMbus stream
You mean this piece of doc? Indeed looks inaccurate. I'll update it.
Wait, no, this is correct. Please point to what is wrong in kernel doc. I don't see it. :(
the TRIGGER_STOP and TRIGGER_PAUSE_PUSH do the same thing. There is no specific mapping of disable() to TRIGGER_STOP and unprepare() to TRIGGER_PAUSE_PUSH as the documentation hints at.
Which TRIGGER_STOP and TRIGGER_PAUSE_PUSH? In one specific codec driver? If yes, I don't think Slimbus documentation should care how actual users implement it (e.g. coalesce states).
In both of the patches you just modified :-)
diff --git a/sound/soc/codecs/wcd9335.c b/sound/soc/codecs/wcd9335.c index 06c6adbe5920..d2548fdf9ae5 100644 --- a/sound/soc/codecs/wcd9335.c +++ b/sound/soc/codecs/wcd9335.c @@ -1972,8 +1972,8 @@ static int wcd9335_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - slim_stream_unprepare(dai_data->sruntime); slim_stream_disable(dai_data->sruntime); + slim_stream_unprepare(dai_data->sruntime); break; default:
diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c index f56907d0942d..28175c746b9a 100644 --- a/sound/soc/codecs/wcd934x.c +++ b/sound/soc/codecs/wcd934x.c @@ -1913,8 +1913,8 @@ static int wcd934x_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: - slim_stream_unprepare(dai_data->sruntime); slim_stream_disable(dai_data->sruntime); + slim_stream_unprepare(dai_data->sruntime); break; default: break;
the bus provides helpers to be used in well-defined transitions. A codec driver doing whatever it wants whenever it wants would create chaos for the bus.
On 21/09/2022 17:25, Pierre-Louis Bossart wrote:
Wait, no, this is correct. Please point to what is wrong in kernel doc. I don't see it. :(
the TRIGGER_STOP and TRIGGER_PAUSE_PUSH do the same thing. There is no specific mapping of disable() to TRIGGER_STOP and unprepare() to TRIGGER_PAUSE_PUSH as the documentation hints at.
Which TRIGGER_STOP and TRIGGER_PAUSE_PUSH? In one specific codec driver? If yes, I don't think Slimbus documentation should care how actual users implement it (e.g. coalesce states).
In both of the patches you just modified :-)
Yeah, but this is just some implementation. How this implementation calls, e.g. whether they split STOP from PAUSE is not the concern of Slimbus.
diff --git a/sound/soc/codecs/wcd9335.c b/sound/soc/codecs/wcd9335.c index 06c6adbe5920..d2548fdf9ae5 100644 --- a/sound/soc/codecs/wcd9335.c +++ b/sound/soc/codecs/wcd9335.c @@ -1972,8 +1972,8 @@ static int wcd9335_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
slim_stream_unprepare(dai_data->sruntime);
slim_stream_unprepare(dai_data->sruntime);diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c index f56907d0942d..28175c746b9a 100644 --- a/sound/soc/codecs/wcd934x.c +++ b/sound/soc/codecs/wcd934x.c @@ -1913,8 +1913,8 @@ static int wcd934x_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
slim_stream_unprepare(dai_data->sruntime);
slim_stream_unprepare(dai_data->sruntime);the bus provides helpers to be used in well-defined transitions. A codec driver doing whatever it wants whenever it wants would create chaos for the bus.
True, but it's the problem of the codec, not the Slimbus.
Best regards, Krzysztof
Thanks Krzysztof
On 21/09/2022 15:53, Krzysztof Kozlowski wrote:
Slimbus streams are first prepared and then enabled, so the cleanup path should reverse it. The unprepare sets stream->num_ports to 0 and frees the stream->ports. Calling disable after unprepare was not really effective (channels was not deactivated) and could lead to further issues due to making transfers on unprepared stream.
Fixes: 20aedafdf492 ("ASoC: wcd9335: add support to wcd9335 codec") Cc: stable@vger.kernel.org Signed-off-by: Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
Nice catch,
Reviewed-by: Srinivas Kandagatla srinivas.kandagatla@linaro.org
sound/soc/codecs/wcd9335.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/codecs/wcd9335.c b/sound/soc/codecs/wcd9335.c index 06c6adbe5920..d2548fdf9ae5 100644 --- a/sound/soc/codecs/wcd9335.c +++ b/sound/soc/codecs/wcd9335.c @@ -1972,8 +1972,8 @@ static int wcd9335_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
slim_stream_unprepare(dai_data->sruntime);
slim_stream_unprepare(dai_data->sruntime);
On Wed, 21 Sep 2022 16:53:53 +0200, Krzysztof Kozlowski wrote:
Slimbus streams are first prepared and then enabled, so the cleanup path should reverse it. The unprepare sets stream->num_ports to 0 and frees the stream->ports. Calling disable after unprepare was not really effective (channels was not deactivated) and could lead to further issues due to making transfers on unprepared stream.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/2] ASoC: wcd9335: fix order of Slimbus unprepare/disable commit: ea8ef003aa53ad23e7705c5cab1c4e664faa6c79 [2/2] ASoC: wcd934x: fix order of Slimbus unprepare/disable commit: e96bca7eaa5747633ec638b065630ff83728982a
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (4)
-
Krzysztof Kozlowski -
Mark Brown -
Pierre-Louis Bossart -
Srinivas Kandagatla