> >>> + 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
