Return-Path: MIME-Version: 1.0 In-Reply-To: <20120210125444.GB5022@joana> References: <1328678855.28848.6.camel@aeonflux> <20120208090945.GB5917@aemeltch-MOBL1>

<20120210125444.GB5022@joana> Date: Fri, 10 Feb 2012 11:49:34 -0200 Message-ID: Subject: Re: Getting L2CAP ERTM support into better upstream state From: Ulisses Furquim To: Ulisses Furquim , Mat Martineau , Luiz Augusto von Dentz , Andrei Emeltchenko , Marcel Holtmann , Peter Krystad , linux-bluetooth@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 List-ID: Hi Gustavo, On Fri, Feb 10, 2012 at 10:54 AM, Gustavo Padovan wrote: > Hi Mat, Ulisses, > > * Ulisses Furquim [2012-02-08 23:01:16 -0200]: > >> Hi Mat, >> >> On Wed, Feb 8, 2012 at 8:33 PM, Mat Martineau w= rote: >> > >> > On Wed, 8 Feb 2012, Ulisses Furquim wrote: >> > >> >> Hi everybody, >> >> >> >> On Wed, Feb 8, 2012 at 7:32 AM, Luiz Augusto von Dentz >> >> wrote: >> >>> >> >>> Hi Andrei, >> >>> >> >>> On Wed, Feb 8, 2012 at 11:09 AM, Andrei Emeltchenko >> >>> wrote: >> >>>> >> >>>> >> >>>> I think this would be good to send as patch series so that people c= an >> >>>> comment. What comes to my mind is that the patch might be reduced i= f it >> >>>> does not change order of functions and defines like: >> >>>> >> > >> > ... ... >> > >> > >> >>>> those magic number does not look nice IMO and the code is not looki= ng >> >>>> 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 tak= e in >> > to account many of Andrei's upstream improvements. =A0I don't want to = move >> > backwards in terms of magic numbers, and there is some extra noise in = there >> > due to symbol names. =A0I need to address these issues before merging. >> >> Great. >> >> > I'm also looking for ideas on how to do this as a patch series. =A0Sin= ce it >> > takes a very different approach from the original code, it's hard to m= ake >> > meaningful, small patches that don't break functionality. =A0At some p= oint >> > 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 rep= orted > 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 try= ing > 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 h= ave >> > one commit that calls in to the new code instead of the old code. Then= the >> > old code can be removed. =A0I talked to Gustavo about that a while ago= , and he >> > preferred finding a different way. =A0Maybe an intermediate step is to= put the >> > state machine code in there, but call the existing frame handling func= tions >> > 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. >> >> >> >> >> 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. =A0It 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. =A0I can do incoming and outgoing >> > connections, and send/receive. =A0It is unstable when disconnecting. = =A0I 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 taskl= et >> > context that couldn't block, and the delayed work handlers were writte= n such >> > that it was ok if the functions were called after cancel. =A0I will up= date 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. =A0I will fix this up bef= ore >> > submitting. >> >> Ok. >> >> > Thanks for the feedback, everyone. =A0Please let me know if you have >> > preferences for how to structure this patch set. =A0I'll work on the i= ssues >> > 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 cho= ose. Sure, we can also do that. The other approaches were just more incremental. Regards, --=20 Ulisses Furquim ProFUSION embedded systems http://profusion.mobi Mobile: +55 19 9250 0942 Skype: ulissesffs