Re: [PATCH] slimbus: preserve ports after failed disconnect

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] slimbus: retaining ports in slim_stream_unprepare() breaks stream re-prepare
--

commit a86f555a1b6f385fbf7a79c9ae53a6b9032ee169
Author: Avi Weiss <thnkslprpt@gmail.com>

slimbus: preserve ports after failed disconnect

This commit updates slim_disconnect_port() to set port disconnection state
only after a successful transfer, and modifies slim_stream_unprepare() to
propagate errors while retaining the port array on failure so it can be
retried or freed in slim_stream_free().

> diff --git a/drivers/slimbus/stream.c b/drivers/slimbus/stream.c
> index b4e70a7edd1cc..ab26b243802df 100644
> --- a/drivers/slimbus/stream.c
> +++ b/drivers/slimbus/stream.c

[ ... ]

> @@ -448,13 +453,22 @@ EXPORT_SYMBOL_GPL(slim_stream_disable);
>   */
>  int slim_stream_unprepare(struct slim_stream_runtime *stream)
>  {
> -	int i;
> +	int ret = 0, err, i;
>  
>  	if (!stream->ports || !stream->num_ports)
>  		return -EINVAL;
>  
> -	for (i = 0; i < stream->num_ports; i++)
> -		slim_disconnect_port(stream, &stream->ports[i]);
> +	for (i = 0; i < stream->num_ports; i++) {
> +		if (stream->ports[i].state == SLIM_PORT_DISCONNECTED)
> +			continue;
> +
> +		err = slim_disconnect_port(stream, &stream->ports[i]);
> +		if (err && !ret)
> +			ret = err;
> +	}
> +
> +	if (ret)
> +		return ret;

[Severity: Medium]
Can returning early here while retaining stream->ports prevent the stream
from recovering on subsequent trigger cycles and leak stream->ports?

When a SLIM_MSG_MC_DISCONNECT_PORT transaction fails in slim_do_transfer()
(for example with -ETIMEDOUT), slim_stream_unprepare() returns early with
stream->ports still allocated, while any ports processed before the failure
have already been disconnected in hardware and marked SLIM_PORT_DISCONNECTED.

The callers of this API in sound/soc/codecs/wcd9335.c (wcd9335_trigger())
and sound/soc/codecs/wcd934x.c (wcd934x_trigger()) do not retry
slim_stream_unprepare() on failure, and slim_stream_free() has no callers
in the tree:

sound/soc/codecs/wcd9335.c:wcd9335_trigger()
	case SNDRV_PCM_TRIGGER_START:
	case SNDRV_PCM_TRIGGER_RESUME:
	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
		...
		slim_stream_prepare(dai_data->sruntime, cfg);
		slim_stream_enable(dai_data->sruntime);
		break;
	case SNDRV_PCM_TRIGGER_STOP:
	case SNDRV_PCM_TRIGGER_SUSPEND:
	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
		slim_stream_disable(dai_data->sruntime);
		slim_stream_unprepare(dai_data->sruntime);
		break;

When the stream is subsequently restarted or resumed, slim_stream_prepare()
sees the retained rt->ports pointer and rejects the call with -EINVAL:

drivers/slimbus/stream.c:slim_stream_prepare()
	if (rt->ports) {
		dev_err(&rt->dev->dev, "Stream already Prepared\n");
		return -EINVAL;
	}

As a result, slim_connect_port_channel() is skipped and the ports that were
disconnected during the partial unprepare are never reconnected. Immediately
after that, slim_stream_enable() runs and overwrites port->state on those
disconnected ports:

drivers/slimbus/stream.c:slim_stream_enable()
	for (i = 0; i < stream->num_ports; i++) {
		struct slim_port *port = &stream->ports[i];

		slim_activate_channel(stream, port);
		port->state = SLIM_PORT_CONFIGURED;
	}

Furthermore, when wcd9335_slim_set_hw_params() or
wcd934x_slim_set_hw_params() is later called, dai_data->sruntime is
overwritten by a new slim_stream_allocate() call without calling
slim_stream_free(), leaking the retained stream->ports array.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028145508.1111792-2-thnkslprpt@gmail.com?part=1