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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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;
>>>   	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.

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);
  }