Hi Gustavo, On Fri, Feb 10, 2012 at 10:54 AM, Gustavo Padovan <padovan@xxxxxxxxxxxxxx> wrote: > Hi Mat, Ulisses, > > * Ulisses Furquim <ulisses@xxxxxxxxxxxxxx> [2012-02-08 23:01:16 -0200]: > >> Hi Mat, >> >> On Wed, Feb 8, 2012 at 8:33 PM, Mat Martineau <mathewm@xxxxxxxxxxxxxx> wrote: >> > >> > 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. >> >> Great. >> >> > 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. >> >> I agree here. You maybe can have some patches introducing new code >> that's going to be used in the commit that really switches the state >> machine handling. > > > The first step is to do a huge clean up on your patch, people already reported > many issues here. Then you can send all the macro definitions changes to > l2cap.h. Finally we look to your patch and check how bit it is and decide what > to do. It's much better waste time looking for bugs in your code than trying > to find a way to split in many patches. > The most important thing is do it fast, we are now in -rc3, we have 4 or 5 > weeks until 3.4 merge windows opens. > >> >> > 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. >> >> IMO the former approach is better. We could even have a module option >> to activate the new code for testing (have it experimental) and do the >> switch when we're confident it's ready. >> >> <snip> >> >> >> 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. >> >> I'd say we add your changes and then Andrei's when he's ready with >> them. Right now I have the feeling your changes are more mature and >> can be easily 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. >> >> Ok. >> >> >> 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 see. I haven't really checked the handlers but it's less error prone >> to guarantee that if we cancel a delayed work as soon as the function >> returns the handler won't be queued on our back. And I say that >> thinking of keeping it simpler to maintain. Also cancel_delayed_work() >> never sleeps, it can only spin on a spinlock waiting for the timer >> handler to return to guarantee the work won't be queued again. >> >> >> 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. >> >> Ok. >> >> > 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. >> >> We need to hear from Marcel and Padovan if they agree with us. I do >> think you can introduce new stuff with small commits and then have one >> commit to add the bulk of it with the module option to enable it. Then >> we test that and make it default later. > > If the concern here is that the current ERTM implementation does not work I'd > rather completely remove it and add the new one, no module options to choose. Sure, we can also do that. The other approaches were just more incremental. Regards, -- Ulisses Furquim ProFUSION embedded systems http://profusion.mobi Mobile: +55 19 9250 0942 Skype: ulissesffs -- 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
