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