Re: [PATCH v4 4/7] soundwire: Handle multiple master instances in a stream

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

On 07/03/2018 11:03 AM, Nc, Shreyas wrote:
>>>>> +	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;
>>>> When a master is removed, there is an explicit test to make sure the
>>>> stream state changes when there are no masters left in the list, but...
>>>>
>>>>>    	mutex_unlock(&bus->bus_lock);
>>>>> @@ -1127,7 +1169,7 @@ int sdw_stream_add_master(struct sdw_bus
>> *bus,
>>>>>    	stream->state = SDW_STREAM_CONFIGURED;
>>>> ... it's not symmetrical for the add_master case. The stream state
>>>> changes on the first added master. In addition the stream state
>>>> changes both when a slave is added and a master is added.
>>>> Is this intentional or not - and are there side effects resulting
>>>> from this inconsistency?
>>>>
>>> For remove_master, we already know the number of Masters in the stream
>>> and hence we change the state to RELEASED only when there are no
>>> Masters left in stream.
>>>
>>> But, in the add_master case, we have no idea on how many master
>>> instances are part of the stream and hence how many times add_master
>> would be called.
>>> So, we change the stream state to CONFIGURED when the first Master is
>> added.
>>> I can add a comment if that helps :)
>> I get the point, but you currently change the state for the first slave that's
>> added, so there is an inconsistency here (even before we add the multi-master
>> support).
> When we add a slave, we check if there is an existing m_rt for that bus instance
> and if It does not exist we create a m_rt for that master instance, add the slave
> runtime and change the state to CONFIGURED. So technically we still change
> the state after adding the first m_rt.

If you made no assumption on the order in which slaves and masters are 
added then you need this assignment to CONFIGURED twice. But if you know 
the slaves are added first then the second assignment is irrelevant.
It's hard to memorize really since there are parts where you assume 
things are triggered by external events and others where there is no 
assumption on API use...

>
>> If I wanted to split hair I'd also note it's almost like the state is CONFIGURING
>> rather than CONFIGURED if you don't really control when all configurations are
>> complete at the bus level and depend on external transitions (e.g. DAPM/ALSA
>> initiated) to go in PREPARED mode.
>>
>>> Yes, it is added intentionally (or rather because we could not think
>>> of any other suitable way) and we dont see any side effects. Anything
>>> that you could think of?
>> We should check what happens if the stream is removed before being enabled,
>> or all cases of testing stream->state.
> While releasing the stream we unconditionally clean up things without checking
> stream state. So, we should be fine that way. But, I can check these :)
A quick check for stream->state gives me this:

     stream->state = SDW_STREAM_CONFIGURED;

 >>> so here it's a success case but we fallback to an error case...

stream_error:
     sdw_release_master_stream(m_rt, stream);
error:
     mutex_unlock(&bus->bus_lock);
     return ret;

You are probably missing a

     stream->state = SDW_STREAM_CONFIGURED;
     goto error; //<<< Add this

as done for the slave?

Wondering how this ever worked, you may want to triple-check the regular 
stream management before adding the multi-master stuff?

>
> --Shreyas
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel