| Message ID | 20230601161622.1808135-1-ckeepax@opensource.cirrus.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/4] soundwire: stream: Add missing clear of alloc_slave_rt | expand |
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;
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
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) {
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(-)