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

[Nc, Shreyas]

> >>> +	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 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 :)

--Shreyas