On Wed, 8 Feb 2012, Ulisses Furquim wrote:
Hi everybody,
On Wed, Feb 8, 2012 at 7:32 AM, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
Hi Andrei,
On Wed, Feb 8, 2012 at 11:09 AM, Andrei Emeltchenko
<andrei.emeltchenko.news@xxxxxxxxx> wrote:
I think this would be good to send as patch series so that people can
comment. What comes to my mind is that the patch might be reduced if it
does not change order of functions and defines like:
... <snip> ...
those magic number does not look nice IMO and the code is not looking any
better.
In this aspect perhaps, but we don't need to take the code as it is,
but the point here is following the states defined by the spec and
that is IMO much better.
Right - the code as it stands now was ported quickly, and does not
take in to account many of Andrei's upstream improvements. I don't
want to move backwards in terms of magic numbers, and there is some
extra noise in there due to symbol names. I need to address these
issues before merging.
I'm also looking for ideas on how to do this as a patch series. Since
it takes a very different approach from the original code, it's hard
to make meaningful, small patches that don't break functionality. At
some point there has to be a major switchover but there is a lot of
room to split this up.
One approach is to add inactive code until everything is there, then
have one commit that calls in to the new code instead of the old code.
Then the old code can be removed. I talked to Gustavo about that a
while ago, and he preferred finding a different way. Maybe an
intermediate step is to put the state machine code in there, but call
the existing frame handling functions in every state.
I've taken a quick look last week at their code and indeed it looks
better regarding following the states. In particular they track
rx_state and tx_state and then decide what to do based on that and
what happened. I like it because it's explicit about that instead of
demanding us to reason a lot on what state we are and what we should
do. I'm all for changing our ERTM to that so it'll be more
maintainable.
That was the goal of the design. It has worked well during in-house
testing, and has been through several UPFs.
Marcel mentioned the separation of L2CAP channel and socket. That is a
work in progress by Andrei and judging by what you said, Marcel, you
want that merged before we change ERTM, is that it?
Andrei and Marcel, let's figure out this order now. It doesn't make
sense for one of us to have a bunch of changes staged, only to have
major merge/rebase conflicts when another far-reaching patch set gets
merged.
Mat, thanks a lot for doing this. I have just a few questions. Have
you tested the stack with this patch? How was it? Quickly looking
through the code I feel it's almost there. I agree with Andrei
regarding constants and control field handling but apart from that it
looks good, in general.
It's only lightly tested right now. I can do incoming and outgoing
connections, and send/receive. It is unstable when disconnecting. I
have not tested with dropped frames yet.
Regarding delayed work handling, are you sure about usage of
__cancel_delayed_work()? I do think it's safer to use
cancel_delayed_work() instead as it'll just spin on a lock if the
timer to queue the work is running on another CPU.
This code is coming from a kernel that still had code running in
tasklet context that couldn't block, and the delayed work handlers
were written such that it was ok if the functions were called after
cancel. I will update to the safer cancel_delayed_work().
I saw a comment about channel ref counting and kind of audit that
would be great. It'd be good to see a patch for that after transition
to new state handling.
The "do channel ref counting" comment is there because I saw that the
existing ack/monitor/retrans timers do that. I will fix this up
before submitting.
Thanks for the feedback, everyone. Please let me know if you have
preferences for how to structure this patch set. I'll work on the
issues mentioned in this thread and start splitting up the changes.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
