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: <[email protected]>
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
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;
--
2.34.1
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: <[email protected]>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
> ---
> 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);
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;
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: <[email protected]>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
> ---
Nice catch,
Reviewed-by: Srinivas Kandagatla <[email protected]>
> 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;
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