On 7/10/18 12:02 PM, Sanyog Kale wrote:
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;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.
Yes, it's fine to take the lock from sdw_stream_remove_slave(), the point was to avoid taking the lock twice when the master is removed first.
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??
Yes, something like the change below:
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); + sdw_slave_port_release() + sdw_release_slave_stream() list_del(&m_rt->stream_node); list_del(&m_rt->bus_node); kfree(m_rt); }