Return-Path: Message-ID: <4C0754CD.7020902@Atheros.com> Date: Thu, 3 Jun 2010 12:37:57 +0530 From: Suraj MIME-Version: 1.0 To: Marcel Holtmann CC: Suraj Sumangala , "linux-bluetooth@vger.kernel.org" , "Luis Rodriguez" , Jothikumar Mothilal Subject: Re: [PATCH v2] frame reassembly implementation for data stream References: <1275467080.13946.3.camel@atheros013-desktop> <1275490955.2182.21.camel@localhost.localdomain> <4C071A73.70407@Atheros.com> <1275547087.2182.28.camel@localhost.localdomain> In-Reply-To: <1275547087.2182.28.camel@localhost.localdomain> Content-Type: text/plain; charset="UTF-8"; format=flowed List-ID: Hi Marcel, On 6/3/2010 12:08 PM, Marcel Holtmann wrote: > Hi Suraj, > >>> I don't like this implementation at all. The biggest problem is that you >>> are misusing __reassembly(hdev, HCI_ACLDATA_PKT) for getting your SKB. I >>> don't wanna intermix this. I am also missing checks for the packet >>> length matching or when packets are too big or the header size is not >>> matching up. >>> >>> So in theory both functions do exactly the same. Only minor exception is >>> that one knows the packet type up-front, the other has to read it from >>> the stream as a 1-byte header. I don't wanna maintain two functions that >>> do exactly the same. >>> >>> Creating an internal helper function that can maintain the current state >>> of the reassembly sounds a lot better. Then re-use that function and >>> ensure that the reassembly logic is inside the helper. >> >> I appreciate if you can take a closer look at the patch and compare it >> with hci_recv_fragment implementation. >> Eventhough it looks similar, there are major differences on the way data >> is reassembled. It would not have worked if I had reused the same code >> from hci_recv_fragment(). >> >> Having a common reassembly helper could work. But I am not sure whether >> that would be a better solution. > > I did have a closer look at it already. I clearly see possibilities to > combine them into a more generic helper and not to maintain two > different functions that do exactly the same. The cases that the reassembly code has to consider are 1. We receive only packet type in one fragment 2. packet header is fragmented 3. packet data is fragmented 4. We receive multiple frames in one fragment. hci_recv_fragment(), handles only case 3 and 4 as it always knows the packet type and the full packet header in one call itself. For stream reassembly we need to take care of all cases 1-4 . This is where both functions differ. This actually makes a lot of difference in the way reassembly is done. Now regarding having a common helper function. I agree to the fact that it makes sense to have a helper function. My points against a common helper function are 1. It has to cover all 1-4 cases. This make it bit inefficient for USB as not considering 1 and 2 saves a lot of effort( allocating buffer before knowing the length, trying to verify if we have received the packet header or not etc etc.). 2. For case 4: the helper will not know from which context it was called. If it was called from a stream context, it will have to take the first byte of the next frame as packet type and do reassembly accordingly. This involves the caller telling the helper function from what context it was called. Which IMHO makes it less generic I understand that these issues can be worked around, but I believe makes the generic implementation kind of "trying to do too many things" helper. > > Regards > > Marcel > > Please do let me know your comments, I appreciate it. Regards Suraj