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

[Nc, Shreyas]

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

Ok, will fix this. 
Vinod, would you like to have a patch for fixing this in the same series or a
patch later on ?

> 
> >
> >> 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?

Yes, you are right. It works because of the change being a part of another change
that is yet to be up streamed :(

Wondering how this slipped, this should have been part of the stream series itself.
Will fix this up :)

--Shreyas