[PATCH 1/4] soundwire: stream: Add missing clear of alloc_slave_rt

[PATCH 1/4] soundwire: stream: Add missing clear of alloc_slave_rt

[Charles Keepax]

The current path that skips allocating the slave runtime does not clear
the alloc_slave_rt flag, this is clearly incorrect. Add the missing
clear, so the runtime won't be erroneously cleaned up.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/soundwire/stream.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index c2191c07442b0..379228f221869 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -2021,8 +2021,10 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
 
 skip_alloc_master_rt:
 	s_rt = sdw_slave_rt_find(slave, stream);
-	if (s_rt)
+	if (s_rt) {
+		alloc_slave_rt = false;
 		goto skip_alloc_slave_rt;
+	}
 
 	s_rt = sdw_slave_rt_alloc(slave, m_rt);
 	if (!s_rt) {
-- 
2.30.2

[PATCH 2/4] soundwire: bandwidth allocation: Remove pointless variable

[Charles Keepax]

The block_offset variable in _sdw_compute_port_params adds nothing
either functionally or in terms of code clarity, remove it.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/soundwire/generic_bandwidth_allocation.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/soundwire/generic_bandwidth_allocation.c b/drivers/soundwire/generic_bandwidth_allocation.c
index 325c475b6a66d..31162f2b56381 100644
--- a/drivers/soundwire/generic_bandwidth_allocation.c
+++ b/drivers/soundwire/generic_bandwidth_allocation.c
@@ -139,20 +139,16 @@ static void _sdw_compute_port_params(struct sdw_bus *bus,
 {
 	struct sdw_master_runtime *m_rt;
 	int hstop = bus->params.col - 1;
-	int block_offset, port_bo, i;
+	int port_bo, i;
 
 	/* Run loop for all groups to compute transport parameters */
 	for (i = 0; i < count; i++) {
 		port_bo = 1;
-		block_offset = 1;
 
 		list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
-			sdw_compute_master_ports(m_rt, &params[i],
-						 port_bo, hstop);
+			sdw_compute_master_ports(m_rt, &params[i], port_bo, hstop);
 
-			block_offset += m_rt->ch_count *
-					m_rt->stream->params.bps;
-			port_bo = block_offset;
+			port_bo += m_rt->ch_count * m_rt->stream->params.bps;
 		}
 
 		hstop = hstop - params[i].hwidth;
-- 
2.30.2

Re: [PATCH 2/4] soundwire: bandwidth allocation: Remove pointless variable

[Pierre-Louis Bossart]

On 6/1/23 11:16, Charles Keepax wrote:
> The block_offset variable in _sdw_compute_port_params adds nothing
> either functionally or in terms of code clarity, remove it.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>

This one is easy enough, thanks!

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

> ---
>  drivers/soundwire/generic_bandwidth_allocation.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soundwire/generic_bandwidth_allocation.c b/drivers/soundwire/generic_bandwidth_allocation.c
> index 325c475b6a66d..31162f2b56381 100644
> --- a/drivers/soundwire/generic_bandwidth_allocation.c
> +++ b/drivers/soundwire/generic_bandwidth_allocation.c
> @@ -139,20 +139,16 @@ static void _sdw_compute_port_params(struct sdw_bus *bus,
>  {
>  	struct sdw_master_runtime *m_rt;
>  	int hstop = bus->params.col - 1;
> -	int block_offset, port_bo, i;
> +	int port_bo, i;
>  
>  	/* Run loop for all groups to compute transport parameters */
>  	for (i = 0; i < count; i++) {
>  		port_bo = 1;
> -		block_offset = 1;
>  
>  		list_for_each_entry(m_rt, &bus->m_rt_list, bus_node) {
> -			sdw_compute_master_ports(m_rt, &params[i],
> -						 port_bo, hstop);
> +			sdw_compute_master_ports(m_rt, &params[i], port_bo, hstop);
>  
> -			block_offset += m_rt->ch_count *
> -					m_rt->stream->params.bps;
> -			port_bo = block_offset;
> +			port_bo += m_rt->ch_count * m_rt->stream->params.bps;
>  		}
>  
>  		hstop = hstop - params[i].hwidth;

[PATCH 3/4] soundwire: stream: Remove unnecessary gotos

[Charles Keepax]

There is a lot of code using gotos to skip small sections of code, this
is a fairly dubious use of a goto, especially when the level of
intentation is really low. Most of this code doesn't even breach 80
characters when naively shifted over.

Simplify the code a bit, by replacing these unnecessary gotos with
simple ifs.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/soundwire/stream.c | 131 +++++++++++++++++--------------------
 1 file changed, 59 insertions(+), 72 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 379228f221869..248ab243ec6e4 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1355,25 +1355,23 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream,
 			return -EINVAL;
 		}
 
-		if (!update_params)
-			goto program_params;
-
-		/* Increment cumulative bus bandwidth */
-		/* TODO: Update this during Device-Device support */
-		bus->params.bandwidth += m_rt->stream->params.rate *
-			m_rt->ch_count * m_rt->stream->params.bps;
-
-		/* Compute params */
-		if (bus->compute_params) {
-			ret = bus->compute_params(bus);
-			if (ret < 0) {
-				dev_err(bus->dev, "Compute params failed: %d\n",
-					ret);
-				goto restore_params;
+		if (update_params) {
+			/* Increment cumulative bus bandwidth */
+			/* TODO: Update this during Device-Device support */
+			bus->params.bandwidth += m_rt->stream->params.rate *
+				m_rt->ch_count * m_rt->stream->params.bps;
+
+			/* Compute params */
+			if (bus->compute_params) {
+				ret = bus->compute_params(bus);
+				if (ret < 0) {
+					dev_err(bus->dev, "Compute params failed: %d\n",
+						ret);
+					goto restore_params;
+				}
 			}
 		}
 
-program_params:
 		/* Program params */
 		ret = sdw_program_params(bus, true);
 		if (ret < 0) {
@@ -1864,7 +1862,7 @@ int sdw_stream_add_master(struct sdw_bus *bus,
 			  struct sdw_stream_runtime *stream)
 {
 	struct sdw_master_runtime *m_rt;
-	bool alloc_master_rt = true;
+	bool alloc_master_rt = false;
 	int ret;
 
 	mutex_lock(&bus->bus_lock);
@@ -1886,30 +1884,25 @@ int sdw_stream_add_master(struct sdw_bus *bus,
 	 * it first), if so skip allocation and go to configuration
 	 */
 	m_rt = sdw_master_rt_find(bus, stream);
-	if (m_rt) {
-		alloc_master_rt = false;
-		goto skip_alloc_master_rt;
-	}
-
-	m_rt = sdw_master_rt_alloc(bus, stream);
 	if (!m_rt) {
-		dev_err(bus->dev, "%s: Master runtime alloc failed for stream:%s\n",
-			__func__, stream->name);
-		ret = -ENOMEM;
-		goto unlock;
-	}
-skip_alloc_master_rt:
-
-	if (sdw_master_port_allocated(m_rt))
-		goto skip_alloc_master_port;
+		m_rt = sdw_master_rt_alloc(bus, stream);
+		if (!m_rt) {
+			dev_err(bus->dev, "%s: Master runtime alloc failed for stream:%s\n",
+				__func__, stream->name);
+			ret = -ENOMEM;
+			goto unlock;
+		}
 
-	ret = sdw_master_port_alloc(m_rt, num_ports);
-	if (ret)
-		goto alloc_error;
+		alloc_master_rt = true;
+	}
 
-	stream->m_rt_count++;
+	if (!sdw_master_port_allocated(m_rt)) {
+		ret = sdw_master_port_alloc(m_rt, num_ports);
+		if (ret)
+			goto alloc_error;
 
-skip_alloc_master_port:
+		stream->m_rt_count++;
+	}
 
 	ret = sdw_master_rt_config(m_rt, stream_config);
 	if (ret < 0)
@@ -1990,8 +1983,8 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
 {
 	struct sdw_slave_runtime *s_rt;
 	struct sdw_master_runtime *m_rt;
-	bool alloc_master_rt = true;
-	bool alloc_slave_rt = true;
+	bool alloc_master_rt = false;
+	bool alloc_slave_rt = false;
 
 	int ret;
 
@@ -2002,47 +1995,41 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
 	 * and go to configuration
 	 */
 	m_rt = sdw_master_rt_find(slave->bus, stream);
-	if (m_rt) {
-		alloc_master_rt = false;
-		goto skip_alloc_master_rt;
-	}
-
-	/*
-	 * If this API is invoked by Slave first then m_rt is not valid.
-	 * So, allocate m_rt and add Slave to it.
-	 */
-	m_rt = sdw_master_rt_alloc(slave->bus, stream);
 	if (!m_rt) {
-		dev_err(&slave->dev, "%s: Master runtime alloc failed for stream:%s\n",
-			__func__, stream->name);
-		ret = -ENOMEM;
-		goto unlock;
-	}
+		/*
+		 * If this API is invoked by Slave first then m_rt is not valid.
+		 * So, allocate m_rt and add Slave to it.
+		 */
+		m_rt = sdw_master_rt_alloc(slave->bus, stream);
+		if (!m_rt) {
+			dev_err(&slave->dev, "%s: Master runtime alloc failed for stream:%s\n",
+				__func__, stream->name);
+			ret = -ENOMEM;
+			goto unlock;
+		}
 
-skip_alloc_master_rt:
-	s_rt = sdw_slave_rt_find(slave, stream);
-	if (s_rt) {
-		alloc_slave_rt = false;
-		goto skip_alloc_slave_rt;
+		alloc_master_rt = true;
 	}
 
-	s_rt = sdw_slave_rt_alloc(slave, m_rt);
+	s_rt = sdw_slave_rt_find(slave, stream);
 	if (!s_rt) {
-		dev_err(&slave->dev, "Slave runtime alloc failed for stream:%s\n", stream->name);
-		alloc_slave_rt = false;
-		ret = -ENOMEM;
-		goto alloc_error;
-	}
+		s_rt = sdw_slave_rt_alloc(slave, m_rt);
+		if (!s_rt) {
+			dev_err(&slave->dev, "Slave runtime alloc failed for stream:%s\n",
+				stream->name);
+			ret = -ENOMEM;
+			goto alloc_error;
+		}
 
-skip_alloc_slave_rt:
-	if (sdw_slave_port_allocated(s_rt))
-		goto skip_port_alloc;
+		alloc_slave_rt = true;
+	}
 
-	ret = sdw_slave_port_alloc(slave, s_rt, num_ports);
-	if (ret)
-		goto alloc_error;
+	if (!sdw_slave_port_allocated(s_rt)) {
+		ret = sdw_slave_port_alloc(slave, s_rt, num_ports);
+		if (ret)
+			goto alloc_error;
+	}
 
-skip_port_alloc:
 	ret =  sdw_master_rt_config(m_rt, stream_config);
 	if (ret)
 		goto unlock;
-- 
2.30.2

Re: [PATCH 3/4] soundwire: stream: Remove unnecessary gotos

[Pierre-Louis Bossart]

On 6/1/23 11:16, Charles Keepax wrote:
> There is a lot of code using gotos to skip small sections of code, this
> is a fairly dubious use of a goto, especially when the level of
> intentation is really low. Most of this code doesn't even breach 80
> characters when naively shifted over.
> 
> Simplify the code a bit, by replacing these unnecessary gotos with
> simple ifs.

it's probably ok but far from simple to review with the inverted states
for variables. I don't have the time and energy to revisit this...

I would err on the side of if it ain't broke don't fix it here.

> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  drivers/soundwire/stream.c | 131 +++++++++++++++++--------------------
>  1 file changed, 59 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 379228f221869..248ab243ec6e4 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1355,25 +1355,23 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream,
>  			return -EINVAL;
>  		}
>  
> -		if (!update_params)
> -			goto program_params;
> -
> -		/* Increment cumulative bus bandwidth */
> -		/* TODO: Update this during Device-Device support */
> -		bus->params.bandwidth += m_rt->stream->params.rate *
> -			m_rt->ch_count * m_rt->stream->params.bps;
> -
> -		/* Compute params */
> -		if (bus->compute_params) {
> -			ret = bus->compute_params(bus);
> -			if (ret < 0) {
> -				dev_err(bus->dev, "Compute params failed: %d\n",
> -					ret);
> -				goto restore_params;
> +		if (update_params) {
> +			/* Increment cumulative bus bandwidth */
> +			/* TODO: Update this during Device-Device support */
> +			bus->params.bandwidth += m_rt->stream->params.rate *
> +				m_rt->ch_count * m_rt->stream->params.bps;
> +
> +			/* Compute params */
> +			if (bus->compute_params) {
> +				ret = bus->compute_params(bus);
> +				if (ret < 0) {
> +					dev_err(bus->dev, "Compute params failed: %d\n",
> +						ret);
> +					goto restore_params;
> +				}
>  			}
>  		}
>  
> -program_params:
>  		/* Program params */
>  		ret = sdw_program_params(bus, true);
>  		if (ret < 0) {
> @@ -1864,7 +1862,7 @@ int sdw_stream_add_master(struct sdw_bus *bus,
>  			  struct sdw_stream_runtime *stream)
>  {
>  	struct sdw_master_runtime *m_rt;
> -	bool alloc_master_rt = true;
> +	bool alloc_master_rt = false;
>  	int ret;
>  
>  	mutex_lock(&bus->bus_lock);
> @@ -1886,30 +1884,25 @@ int sdw_stream_add_master(struct sdw_bus *bus,
>  	 * it first), if so skip allocation and go to configuration
>  	 */
>  	m_rt = sdw_master_rt_find(bus, stream);
> -	if (m_rt) {
> -		alloc_master_rt = false;
> -		goto skip_alloc_master_rt;
> -	}
> -
> -	m_rt = sdw_master_rt_alloc(bus, stream);
>  	if (!m_rt) {
> -		dev_err(bus->dev, "%s: Master runtime alloc failed for stream:%s\n",
> -			__func__, stream->name);
> -		ret = -ENOMEM;
> -		goto unlock;
> -	}
> -skip_alloc_master_rt:
> -
> -	if (sdw_master_port_allocated(m_rt))
> -		goto skip_alloc_master_port;
> +		m_rt = sdw_master_rt_alloc(bus, stream);
> +		if (!m_rt) {
> +			dev_err(bus->dev, "%s: Master runtime alloc failed for stream:%s\n",
> +				__func__, stream->name);
> +			ret = -ENOMEM;
> +			goto unlock;
> +		}
>  
> -	ret = sdw_master_port_alloc(m_rt, num_ports);
> -	if (ret)
> -		goto alloc_error;
> +		alloc_master_rt = true;
> +	}
>  
> -	stream->m_rt_count++;
> +	if (!sdw_master_port_allocated(m_rt)) {
> +		ret = sdw_master_port_alloc(m_rt, num_ports);
> +		if (ret)
> +			goto alloc_error;
>  
> -skip_alloc_master_port:
> +		stream->m_rt_count++;
> +	}
>  
>  	ret = sdw_master_rt_config(m_rt, stream_config);
>  	if (ret < 0)
> @@ -1990,8 +1983,8 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
>  {
>  	struct sdw_slave_runtime *s_rt;
>  	struct sdw_master_runtime *m_rt;
> -	bool alloc_master_rt = true;
> -	bool alloc_slave_rt = true;
> +	bool alloc_master_rt = false;
> +	bool alloc_slave_rt = false;
>  
>  	int ret;
>  
> @@ -2002,47 +1995,41 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
>  	 * and go to configuration
>  	 */
>  	m_rt = sdw_master_rt_find(slave->bus, stream);
> -	if (m_rt) {
> -		alloc_master_rt = false;
> -		goto skip_alloc_master_rt;
> -	}
> -
> -	/*
> -	 * If this API is invoked by Slave first then m_rt is not valid.
> -	 * So, allocate m_rt and add Slave to it.
> -	 */
> -	m_rt = sdw_master_rt_alloc(slave->bus, stream);
>  	if (!m_rt) {
> -		dev_err(&slave->dev, "%s: Master runtime alloc failed for stream:%s\n",
> -			__func__, stream->name);
> -		ret = -ENOMEM;
> -		goto unlock;
> -	}
> +		/*
> +		 * If this API is invoked by Slave first then m_rt is not valid.
> +		 * So, allocate m_rt and add Slave to it.
> +		 */
> +		m_rt = sdw_master_rt_alloc(slave->bus, stream);
> +		if (!m_rt) {
> +			dev_err(&slave->dev, "%s: Master runtime alloc failed for stream:%s\n",
> +				__func__, stream->name);
> +			ret = -ENOMEM;
> +			goto unlock;
> +		}
>  
> -skip_alloc_master_rt:
> -	s_rt = sdw_slave_rt_find(slave, stream);
> -	if (s_rt) {
> -		alloc_slave_rt = false;
> -		goto skip_alloc_slave_rt;
> +		alloc_master_rt = true;
>  	}
>  
> -	s_rt = sdw_slave_rt_alloc(slave, m_rt);
> +	s_rt = sdw_slave_rt_find(slave, stream);
>  	if (!s_rt) {
> -		dev_err(&slave->dev, "Slave runtime alloc failed for stream:%s\n", stream->name);
> -		alloc_slave_rt = false;
> -		ret = -ENOMEM;
> -		goto alloc_error;
> -	}
> +		s_rt = sdw_slave_rt_alloc(slave, m_rt);
> +		if (!s_rt) {
> +			dev_err(&slave->dev, "Slave runtime alloc failed for stream:%s\n",
> +				stream->name);
> +			ret = -ENOMEM;
> +			goto alloc_error;
> +		}
>  
> -skip_alloc_slave_rt:
> -	if (sdw_slave_port_allocated(s_rt))
> -		goto skip_port_alloc;
> +		alloc_slave_rt = true;
> +	}
>  
> -	ret = sdw_slave_port_alloc(slave, s_rt, num_ports);
> -	if (ret)
> -		goto alloc_error;
> +	if (!sdw_slave_port_allocated(s_rt)) {
> +		ret = sdw_slave_port_alloc(slave, s_rt, num_ports);
> +		if (ret)
> +			goto alloc_error;
> +	}
>  
> -skip_port_alloc:
>  	ret =  sdw_master_rt_config(m_rt, stream_config);
>  	if (ret)
>  		goto unlock;

Re: [PATCH 3/4] soundwire: stream: Remove unnecessary gotos

[Charles Keepax]

On Thu, Jun 01, 2023 at 11:37:33AM -0500, Pierre-Louis Bossart wrote:
> 
> 
> On 6/1/23 11:16, Charles Keepax wrote:
> > There is a lot of code using gotos to skip small sections of code, this
> > is a fairly dubious use of a goto, especially when the level of
> > intentation is really low. Most of this code doesn't even breach 80
> > characters when naively shifted over.
> > 
> > Simplify the code a bit, by replacing these unnecessary gotos with
> > simple ifs.
> 
> it's probably ok but far from simple to review with the inverted states
> for variables. I don't have the time and energy to revisit this...
> 
> I would err on the side of if it ain't broke don't fix it here.
> 

The current code is pretty oddly written, as you say it does work
through. I will try splitting the patch into separate patches for
inverting the varible and dropping the goto's. That should make
review slightly easier and both changes make the code clearer in
their own right anyway.

Thanks,
Charles

[PATCH 4/4] soundwire: stream: Tidy do_bank_switch error messages

[Charles Keepax]

Every error path in do_bank_switch prints an error message so there is no
need for the callers to also print error messages. Indeed in multi-master
cases these extra messages are confusing because they print out against a
random bus device whereas the do_bank_switch messages print against the bus
that actually failed.

This also allows clean up of a couple of if's and variable initialisations
that were only there to silence potentially uninitialised variable
warnings on the bus variable.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/soundwire/stream.c | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 248ab243ec6e4..b5c7a52aac19e 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1338,7 +1338,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream,
 			       bool update_params)
 {
 	struct sdw_master_runtime *m_rt;
-	struct sdw_bus *bus = NULL;
+	struct sdw_bus *bus;
 	struct sdw_master_prop *prop;
 	struct sdw_bus_params params;
 	int ret;
@@ -1380,16 +1380,9 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream,
 		}
 	}
 
-	if (!bus) {
-		pr_err("Configuration error in %s\n", __func__);
-		return -EINVAL;
-	}
-
 	ret = do_bank_switch(stream);
-	if (ret < 0) {
-		pr_err("%s: do_bank_switch failed: %d\n", __func__, ret);
+	if (ret < 0)
 		goto restore_params;
-	}
 
 	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
 		bus = m_rt->bus;
@@ -1465,7 +1458,7 @@ EXPORT_SYMBOL(sdw_prepare_stream);
 static int _sdw_enable_stream(struct sdw_stream_runtime *stream)
 {
 	struct sdw_master_runtime *m_rt;
-	struct sdw_bus *bus = NULL;
+	struct sdw_bus *bus;
 	int ret;
 
 	/* Enable Master(s) and Slave(s) port(s) associated with stream */
@@ -1488,16 +1481,9 @@ static int _sdw_enable_stream(struct sdw_stream_runtime *stream)
 		}
 	}
 
-	if (!bus) {
-		pr_err("Configuration error in %s\n", __func__);
-		return -EINVAL;
-	}
-
 	ret = do_bank_switch(stream);
-	if (ret < 0) {
-		pr_err("%s: do_bank_switch failed: %d\n", __func__, ret);
+	if (ret < 0)
 		return ret;
-	}
 
 	stream->state = SDW_STREAM_ENABLED;
 	return 0;
@@ -1571,10 +1557,8 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream)
 	}
 
 	ret = do_bank_switch(stream);
-	if (ret < 0) {
-		pr_err("%s: do_bank_switch failed: %d\n", __func__, ret);
+	if (ret < 0)
 		return ret;
-	}
 
 	/* make sure alternate bank (previous current) is also disabled */
 	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
-- 
2.30.2

Re: [PATCH 4/4] soundwire: stream: Tidy do_bank_switch error messages

[Pierre-Louis Bossart]

On 6/1/23 11:16, Charles Keepax wrote:
> Every error path in do_bank_switch prints an error message so there is no
> need for the callers to also print error messages. Indeed in multi-master
> cases these extra messages are confusing because they print out against a
> random bus device whereas the do_bank_switch messages print against the bus
> that actually failed.

Errm, what?

There is no way to know which bus failed in multi-master mode, and the
'stream' can span multiple buses so the use of pr_err was intentional.
There's just no other way to report issues, and I don't see why one
would remove such logs and fail silently.

I just don't get what you are trying to address.

> This also allows clean up of a couple of if's and variable initialisations
> that were only there to silence potentially uninitialised variable
> warnings on the bus variable.

That should be a separate patch IMHO.


> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  drivers/soundwire/stream.c | 26 +++++---------------------
>  1 file changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 248ab243ec6e4..b5c7a52aac19e 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1338,7 +1338,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream,
>  			       bool update_params)
>  {
>  	struct sdw_master_runtime *m_rt;
> -	struct sdw_bus *bus = NULL;
> +	struct sdw_bus *bus;
>  	struct sdw_master_prop *prop;
>  	struct sdw_bus_params params;
>  	int ret;
> @@ -1380,16 +1380,9 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream,
>  		}
>  	}
>  
> -	if (!bus) {
> -		pr_err("Configuration error in %s\n", __func__);
> -		return -EINVAL;
> -	}
> -
>  	ret = do_bank_switch(stream);
> -	if (ret < 0) {
> -		pr_err("%s: do_bank_switch failed: %d\n", __func__, ret);
> +	if (ret < 0)
>  		goto restore_params;
> -	}
>  
>  	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
>  		bus = m_rt->bus;
> @@ -1465,7 +1458,7 @@ EXPORT_SYMBOL(sdw_prepare_stream);
>  static int _sdw_enable_stream(struct sdw_stream_runtime *stream)
>  {
>  	struct sdw_master_runtime *m_rt;
> -	struct sdw_bus *bus = NULL;
> +	struct sdw_bus *bus;
>  	int ret;
>  
>  	/* Enable Master(s) and Slave(s) port(s) associated with stream */
> @@ -1488,16 +1481,9 @@ static int _sdw_enable_stream(struct sdw_stream_runtime *stream)
>  		}
>  	}
>  
> -	if (!bus) {
> -		pr_err("Configuration error in %s\n", __func__);
> -		return -EINVAL;
> -	}
> -
>  	ret = do_bank_switch(stream);
> -	if (ret < 0) {
> -		pr_err("%s: do_bank_switch failed: %d\n", __func__, ret);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	stream->state = SDW_STREAM_ENABLED;
>  	return 0;
> @@ -1571,10 +1557,8 @@ static int _sdw_disable_stream(struct sdw_stream_runtime *stream)
>  	}
>  
>  	ret = do_bank_switch(stream);
> -	if (ret < 0) {
> -		pr_err("%s: do_bank_switch failed: %d\n", __func__, ret);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	/* make sure alternate bank (previous current) is also disabled */
>  	list_for_each_entry(m_rt, &stream->master_list, stream_node) {

Re: [PATCH 4/4] soundwire: stream: Tidy do_bank_switch error messages

[Charles Keepax]

On Thu, Jun 01, 2023 at 11:34:10AM -0500, Pierre-Louis Bossart wrote:
> On 6/1/23 11:16, Charles Keepax wrote:
> > Every error path in do_bank_switch prints an error message so there is no
> > need for the callers to also print error messages. Indeed in multi-master
> > cases these extra messages are confusing because they print out against a
> > random bus device whereas the do_bank_switch messages print against the bus
> > that actually failed.
> 
> Errm, what?
> 
> There is no way to know which bus failed in multi-master mode, and the
> 'stream' can span multiple buses so the use of pr_err was intentional.

Apologies this is the commit message not quite keeping pace with
the code base. Originally when I wrote the patch the error
message after do_bank_switch were a "dev_err(bus->dev", that was
then fixed up in commit d014688eb373 ("soundwire: stream: remove
bus->dev from logs on multiple buses").

> There's just no other way to report issues, and I don't see why one
> would remove such logs and fail silently.
> 
> I just don't get what you are trying to address.
>

The current code would say produce something like:

Bank switch failed: -5
_sdw_prepare_stream: do_bank_switch failed: -5

I am sensing you are keen to keep both error messages, so fair
enough I will drop that. Although worth noting originally before
that patch I mention above this would have been:

Bank switch failed: -5
do_bank_switch failed: -5

Which is really what I was attempting to address, that is clearly
redundant. Now with the addition of the function in the print I
guess it is slightly less redundant.

> > This also allows clean up of a couple of if's and variable initialisations
> > that were only there to silence potentially uninitialised variable
> > warnings on the bus variable.
> 
> That should be a separate patch IMHO.
> 

I will trim the patch down to leave the duplicate error messages
and just remove those bits.

Thanks,
Charles

Re: [PATCH 1/4] soundwire: stream: Add missing clear of alloc_slave_rt

[Pierre-Louis Bossart]

On 6/1/23 11:16, Charles Keepax wrote:
> The current path that skips allocating the slave runtime does not clear
> the alloc_slave_rt flag, this is clearly incorrect. Add the missing
> clear, so the runtime won't be erroneously cleaned up.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Sounds about right, thanks

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

> ---
>  drivers/soundwire/stream.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index c2191c07442b0..379228f221869 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -2021,8 +2021,10 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
>  
>  skip_alloc_master_rt:
>  	s_rt = sdw_slave_rt_find(slave, stream);
> -	if (s_rt)
> +	if (s_rt) {
> +		alloc_slave_rt = false;
>  		goto skip_alloc_slave_rt;
> +	}
>  
>  	s_rt = sdw_slave_rt_alloc(slave, m_rt);
>  	if (!s_rt) {