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.
Fixes: f3016b891c8c ("soundwire: stream: sdw_stream_add_ functions can be called multiple times")
Reviewed-by: Pierre-Louis Bossart <[email protected]>
Signed-off-by: Charles Keepax <[email protected]>
---
Changes since v1:
- Added missing fixes tag
Thanks,
Charles
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
sdw_stream_add_slave/master have flags to indicate if the master or
slave runtime where allocated in that call to the function. Currently
these flags are cleared on all the paths where the runtime is not
allocated, it is more logic and simpler to set the flag on the one path
where the runtime is allocated.
Signed-off-by: Charles Keepax <[email protected]>
---
Changes since v1:
- Split out of the goto patch to ease review
Also worth noting I guess this patch could be squashed with patch 1 in
the series really, but I opted to leave them separate as patch 1 is a
much simpler fix to be cherry-picked back to older kernels if someone
needs the fixup, rather than mixing the fixup and tidy up.
Thanks,
Charles
drivers/soundwire/stream.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 6595f47b403b5..df5600a80c174 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1854,7 +1854,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);
@@ -1876,10 +1876,8 @@ 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;
+ if (m_rt)
goto skip_alloc_master_rt;
- }
m_rt = sdw_master_rt_alloc(bus, stream);
if (!m_rt) {
@@ -1888,6 +1886,8 @@ int sdw_stream_add_master(struct sdw_bus *bus,
ret = -ENOMEM;
goto unlock;
}
+
+ alloc_master_rt = true;
skip_alloc_master_rt:
if (sdw_master_port_allocated(m_rt))
@@ -1980,8 +1980,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;
@@ -1992,10 +1992,8 @@ 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;
+ if (m_rt)
goto skip_alloc_master_rt;
- }
/*
* If this API is invoked by Slave first then m_rt is not valid.
@@ -2009,21 +2007,22 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
goto unlock;
}
+ alloc_master_rt = true;
+
skip_alloc_master_rt:
s_rt = sdw_slave_rt_find(slave, stream);
- if (s_rt) {
- alloc_slave_rt = false;
+ if (s_rt)
goto skip_alloc_slave_rt;
- }
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);
- alloc_slave_rt = false;
ret = -ENOMEM;
goto alloc_error;
}
+ alloc_slave_rt = true;
+
skip_alloc_slave_rt:
if (sdw_slave_port_allocated(s_rt))
goto skip_port_alloc;
--
2.30.2
On 6/2/23 05:11, Charles Keepax wrote:
> sdw_stream_add_slave/master have flags to indicate if the master or
> slave runtime where allocated in that call to the function. Currently
> these flags are cleared on all the paths where the runtime is not
> allocated, it is more logic and simpler to set the flag on the one path
> where the runtime is allocated.
>
> Signed-off-by: Charles Keepax <[email protected]>
Much easier to review indeed, thanks!
Reviewed-by: Pierre-Louis Bossart <[email protected]>
> ---
>
> Changes since v1:
> - Split out of the goto patch to ease review
>
> Also worth noting I guess this patch could be squashed with patch 1 in
> the series really, but I opted to leave them separate as patch 1 is a
> much simpler fix to be cherry-picked back to older kernels if someone
> needs the fixup, rather than mixing the fixup and tidy up.
>
> Thanks,
> Charles
>
> drivers/soundwire/stream.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 6595f47b403b5..df5600a80c174 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1854,7 +1854,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);
> @@ -1876,10 +1876,8 @@ 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;
> + if (m_rt)
> goto skip_alloc_master_rt;
> - }
>
> m_rt = sdw_master_rt_alloc(bus, stream);
> if (!m_rt) {
> @@ -1888,6 +1886,8 @@ int sdw_stream_add_master(struct sdw_bus *bus,
> ret = -ENOMEM;
> goto unlock;
> }
> +
> + alloc_master_rt = true;
> skip_alloc_master_rt:
>
> if (sdw_master_port_allocated(m_rt))
> @@ -1980,8 +1980,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;
>
> @@ -1992,10 +1992,8 @@ 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;
> + if (m_rt)
> goto skip_alloc_master_rt;
> - }
>
> /*
> * If this API is invoked by Slave first then m_rt is not valid.
> @@ -2009,21 +2007,22 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
> goto unlock;
> }
>
> + alloc_master_rt = true;
> +
> skip_alloc_master_rt:
> s_rt = sdw_slave_rt_find(slave, stream);
> - if (s_rt) {
> - alloc_slave_rt = false;
> + if (s_rt)
> goto skip_alloc_slave_rt;
> - }
>
> 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);
> - alloc_slave_rt = false;
> ret = -ENOMEM;
> goto alloc_error;
> }
>
> + alloc_slave_rt = true;
> +
> skip_alloc_slave_rt:
> if (sdw_slave_port_allocated(s_rt))
> goto skip_port_alloc;
On 02-06-23, 11:11, 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.
Applied all, thanks
--
~Vinod