[PATCH 1/2] ASoC: wcd9335: fix order of Slimbus unprepare/disable

[PATCH 1/2] ASoC: wcd9335: fix order of Slimbus unprepare/disable

[Krzysztof Kozlowski]

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;
-- 
2.34.1

[PATCH 2/2] ASoC: wcd934x: fix order of Slimbus unprepare/disable

[Krzysztof Kozlowski]

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;
-- 
2.34.1

Re: [PATCH 2/2] ASoC: wcd934x: fix order of Slimbus unprepare/disable

[Srinivas Kandagatla]

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_disable(dai_data->sruntime);
> +		slim_stream_unprepare(dai_data->sruntime);
>   		break;
>   	default:
>   		break;

Re: [PATCH 1/2] ASoC: wcd9335: fix order of Slimbus unprepare/disable

[Pierre-Louis Bossart]

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_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;

Re: [PATCH 1/2] ASoC: wcd9335: fix order of Slimbus unprepare/disable

[Krzysztof Kozlowski]

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_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

You mean this piece of doc? Indeed looks inaccurate. I'll update it.

Best regards,
Krzysztof

Re: [PATCH 1/2] ASoC: wcd9335: fix order of Slimbus unprepare/disable

[Krzysztof Kozlowski]

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_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
> 
> 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

Re: [PATCH 1/2] ASoC: wcd9335: fix order of Slimbus unprepare/disable

[Pierre-Louis Bossart]

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_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
>>
>> 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.

Re: [PATCH 1/2] ASoC: wcd9335: fix order of Slimbus unprepare/disable

[Krzysztof Kozlowski]

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

Re: [PATCH 1/2] ASoC: wcd9335: fix order of Slimbus unprepare/disable

[Pierre-Louis Bossart]

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.

Re: [PATCH 1/2] ASoC: wcd9335: fix order of Slimbus unprepare/disable

[Krzysztof Kozlowski]

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_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.

True, but it's the problem of the codec, not the Slimbus.

Best regards,
Krzysztof

Re: [PATCH 1/2] ASoC: wcd9335: fix order of Slimbus unprepare/disable

[Srinivas Kandagatla]

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_disable(dai_data->sruntime);
> +		slim_stream_unprepare(dai_data->sruntime);
>   		break;
>   	default:
>   		break;

Re: [PATCH 1/2] ASoC: wcd9335: fix order of Slimbus unprepare/disable

[Mark Brown]

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