[alsa-devel] [PATCH v5 6/9] soundwire: Handle multiple master instances in a stream

[Sanyog Kale]

On Mon, Jul 09, 2018 at 06:42:34PM -0500, Pierre-Louis Bossart wrote:
> Sorry, another issue that I found while reviewing the entire section.
> >  }
> >@@ -888,6 +918,7 @@ static void sdw_release_slave_stream(struct sdw_slave *slave,
> >  /**
> >   * sdw_release_master_stream() - Free Master runtime handle
> >   *
> >+ * @m_rt: Master runtime node
> >   * @stream: Stream runtime handle.
> >   *
> >   * This function is to be called with bus_lock held
> >@@ -895,16 +926,18 @@ static void sdw_release_slave_stream(struct sdw_slave *slave,
> >   * handle. If this is called first then sdw_release_slave_stream() will have
> >   * no effect as Slave(s) runtime handle would already be freed up.
> >   */
> >-static void sdw_release_master_stream(struct sdw_stream_runtime *stream)
> >+static void sdw_release_master_stream(struct sdw_master_runtime *m_rt,
> >+			struct sdw_stream_runtime *stream)
> >  {
> >-	struct sdw_master_runtime *m_rt = stream->m_rt;
> >  	struct sdw_slave_runtime *s_rt, *_s_rt;
> >  	list_for_each_entry_safe(s_rt, _s_rt,
> >  			&m_rt->slave_rt_list, m_rt_node)
> >  		sdw_stream_remove_slave(s_rt->slave, stream);
> >+	list_del(&m_rt->stream_node);
> >  	list_del(&m_rt->bus_node);
> >+	kfree(m_rt);
> >  }
> >  /**
> >@@ -918,13 +951,22 @@ static void sdw_release_master_stream(struct sdw_stream_runtime *stream)
> >  int sdw_stream_remove_master(struct sdw_bus *bus,
> >  		struct sdw_stream_runtime *stream)
> >  {
> >+	struct sdw_master_runtime *m_rt, *_m_rt;
> >+
> >  	mutex_lock(&bus->bus_lock);
> >-	sdw_release_master_stream(stream);
> >-	sdw_master_port_release(bus, stream->m_rt);
> >-	stream->state = SDW_STREAM_RELEASED;
> >-	kfree(stream->m_rt);
> >-	stream->m_rt = NULL;
> >+	list_for_each_entry_safe(m_rt, _m_rt,
> >+			&stream->master_list, stream_node) {
> >+
> >+		if (m_rt->bus != bus)
> >+			continue;
> >+
> >+		sdw_master_port_release(bus, m_rt);
> >+		sdw_release_master_stream(m_rt, stream);
> >+	}
> >+
> >+	if (list_empty(&stream->master_list))
> >+		stream->state = SDW_STREAM_RELEASED;
> >  	mutex_unlock(&bus->bus_lock);
> >
> So the sequence is
> 
> mutex_lock
> sdw_master_port_release()
> sdw_release_master_stream()
> ?????? sdw_stream_remove_slave()
> ?????? ?????? mutex_lock
> 
> Is this intentional to take the same mutex twice (not sure if it even
> works).

sdw_stream_remove_slave is called from sdw_release_master_stream to make
sure all Slave(s) resources are freed up before freeing Master.
sdw_stream_remove_slave is also called by Slave driver to free up Slave
resources. In either case, we wanted to make sure the bus_lock is held
hence the bus lock is held in sdw_stream_remove_slave API as well.

It doesnt look correct to take same mutex twice. Will check on this.

> what you probably wanted is to replace sdw_stream_remove_slave() by the
> equivalent sequence
> 
> sdw_slave_port_release()
> sdw_release_slave_stream()
> 
> which are both supposed to be called with a bus_lock held.

you mean to say perform sdw_slave_port_release and
sdw_release_slave_stream in sdw_release_master_stream instead of calling
sdw_stream_remove_slave??

--