QuiC AMP development

QuiC AMP development

[Ron Shaffer]

As requested in today's summit discussions, here's a link that can be
used to inspect the code we've done to add AMP support on the latest
next tree.

https://www.codeaurora.org/gitweb/quic/bluetooth/?p=bluetooth-next-2.6.git;a=summary
branch pk-upstream

And for those who missed the original RFC, you can find it here:
http://www.spinics.net/lists/linux-bluetooth/msg04674.html

-- 
Ron Shaffer
Employee of the Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: QuiC AMP development

[Gustavo F. Padovan]

Hi all,

* Ron Shaffer <rshaffer@codeaurora.org> [2010-08-08 21:43:36 -0500]:

> As requested in today's summit discussions, here's a link that can be
> used to inspect the code we've done to add AMP support on the latest
> next tree.
> 
> https://www.codeaurora.org/gitweb/quic/bluetooth/?p=bluetooth-next-2.6.git;a=summary
> branch pk-upstream


I'll put here some comments I have about some L2CAP patches:


+static int l2cap_seq_list_init(struct l2cap_seq_list *seq_list, u16 size)
+{
+       u16 allocSize;
                                                                               
No capital letter here. Do alloc_size. Check the rest of your code for similar
stuff.                                                                         
                                                                               
                                                                               
        u8 event;                                                              
        struct sk_buff *skb;                                                   
 };                                                                            
+                                                                              
 #endif /* __AMP_H */                                                          
diff --git a/net/bluetooth/amp.c b/net/bluetooth/amp.c                         
index f43d7d8..16e74f6 100644                                                  
--- a/net/bluetooth/amp.c                                                      
+++ b/net/bluetooth/amp.c                                                      
@@ -40,6 +40,7 @@ static struct workqueue_struct *amp_workqueue;               
 LIST_HEAD(amp_mgr_list);
 DEFINE_RWLOCK(amp_mgr_list_lock);
 
+
 static void remove_amp_mgr(struct amp_mgr *rem)
 {
        struct amp_mgr *mgr = NULL;

Do not add new lines random places into your patch. Check you code for others
things like that. Also check for lines overstepping the column 80.



+struct l2cap_enhanced_hdr {
+       __le16     len;
+       __le16     cid;
+       __le16     control;
+} __attribute__ ((packed));
+#define L2CAP_ENHANCED_HDR_SIZE                6

Get ride off this struct, that actually doesn't help. We tried that before and
Marcel and I agreed that it does not help.



-               if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
+
+               if (memcpy_fromiovec(skb_put(*frag, count),
+                               msg->msg_iov, count))
                        return -EFAULT;

Avoid changes like that in your patches, you are just breaking a line here. If
you want to do that do in a separeted patch.



+/* L2CAP ERTM / Streaming extra field lengths */
+#define L2CAP_FCS_SIZE          2

Separated patch for that, so then you replace 2 for L2CAP_FCS_SIZE in one
patch, instead of doing that in many patches like you are doing now. The same
goes for L2CAP_SDULEN_SIZE.



-/* L2CAP Supervisory Function */
+/* L2CAP Supervisory Frame Types */
+#define L2CAP_SFRAME_RR            0x00
+#define L2CAP_SFRAME_REJ           0x01
+#define L2CAP_SFRAME_RNR           0x02
+#define L2CAP_SFRAME_SREJ          0x03
 #define L2CAP_SUPER_RCV_READY           0x0000
 #define L2CAP_SUPER_REJECT              0x0004
 #define L2CAP_SUPER_RCV_NOT_READY       0x0008
 #define L2CAP_SUPER_SELECT_REJECT       0x000C
 
 /* L2CAP Segmentation and Reassembly */
+#define L2CAP_SAR_UNSEGMENTED      0x00
+#define L2CAP_SAR_START            0x01
+#define L2CAP_SAR_END              0x02
+#define L2CAP_SAR_CONTINUE         0x03
 #define L2CAP_SDU_UNSEGMENTED       0x0000
 #define L2CAP_SDU_START             0x4000
 #define L2CAP_SDU_END               0x8000

What is that? we already have such defines.



commit 162e6de6d5c11b8ffea91a9fa2d597340afdeb6e
Author: Mat Martineau <mathewm@codeaurora.org>
Date:   Thu Aug 5 16:59:46 2010 -0700

    Bluetooth: Remove unused L2CAP code.

 net/bluetooth/l2cap.c | 1104 +------------------------------------------------
 1 files changed, 15 insertions(+), 1089 deletions(-)

That's not acceptable, we have to modify the existent base code and make it
work with the new features, instead writing a new one and then switch to it.



commit 09850f68219572288fe62a59235fda3d2629c7b2
Author: Peter Krystad <pkrystad@codeaurora.org>
Date:   Wed Aug 4 16:55:11 2010 -0700

    Rename l2cap.c to el2cap.c.
    
    Necessary before additional files can be added to l2cap module.
    A module cannot contain a file with the same name as the module.

We don't neeed that. We might split l2cap.c into smaller chunks before merge
all the AMP stuff into it, so we won't have the module name problem anymore. 

-- 
Gustavo F. Padovan
http://padovan.org

Re: QuiC AMP development

[Mat Martineau]

Gustavo -

On Tue, 10 Aug 2010, Gustavo F. Padovan wrote:

> Hi all,
>
> * Ron Shaffer <rshaffer@codeaurora.org> [2010-08-08 21:43:36 -0500]:
>
>> As requested in today's summit discussions, here's a link that can be
>> used to inspect the code we've done to add AMP support on the latest
>> next tree.
>>
>> https://www.codeaurora.org/gitweb/quic/bluetooth/?p=bluetooth-next-2.6.git;a=summary
>> branch pk-upstream
>
>
> I'll put here some comments I have about some L2CAP patches:

Thanks for taking the time to look over all these changes!

>
>
> +static int l2cap_seq_list_init(struct l2cap_seq_list *seq_list, u16 size)
> +{
> +       u16 allocSize;
>
> No capital letter here. Do alloc_size. Check the rest of your code for similar
> stuff.

Yes, someone caught that in our internal code review too.  I think 
this was the only one.

>
>
>        u8 event;
>        struct sk_buff *skb;
> };
> +
> #endif /* __AMP_H */
> diff --git a/net/bluetooth/amp.c b/net/bluetooth/amp.c
> index f43d7d8..16e74f6 100644
> --- a/net/bluetooth/amp.c
> +++ b/net/bluetooth/amp.c
> @@ -40,6 +40,7 @@ static struct workqueue_struct *amp_workqueue;
> LIST_HEAD(amp_mgr_list);
> DEFINE_RWLOCK(amp_mgr_list_lock);
>
> +
> static void remove_amp_mgr(struct amp_mgr *rem)
> {
>        struct amp_mgr *mgr = NULL;
>
> Do not add new lines random places into your patch. Check you code for others
> things like that. Also check for lines overstepping the column 80.

We definitely know we have work to do to clean up these patches and 
add proper commit messages.  Please be assured that we'll have these 
more tidied up before posting patches to linux-bluetooth.

>
>
> +struct l2cap_enhanced_hdr {
> +       __le16     len;
> +       __le16     cid;
> +       __le16     control;
> +} __attribute__ ((packed));
> +#define L2CAP_ENHANCED_HDR_SIZE                6
>
> Get ride off this struct, that actually doesn't help. We tried that before and
> Marcel and I agreed that it does not help.

If you guys don't like it, I'll take it out.

>
> -               if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
> +
> +               if (memcpy_fromiovec(skb_put(*frag, count),
> +                               msg->msg_iov, count))
>                        return -EFAULT;
>
> Avoid changes like that in your patches, you are just breaking a line here. If
> you want to do that do in a separeted patch.
>
>
>
> +/* L2CAP ERTM / Streaming extra field lengths */
> +#define L2CAP_FCS_SIZE          2
>
> Separated patch for that, so then you replace 2 for L2CAP_FCS_SIZE in one
> patch, instead of doing that in many patches like you are doing now. The same
> goes for L2CAP_SDULEN_SIZE.

Yes, I already split that commit up to send the "RFC" patch set 
earlier today.  Unfortunately I missed that commit when generating my 
patches :(


> -/* L2CAP Supervisory Function */
> +/* L2CAP Supervisory Frame Types */
> +#define L2CAP_SFRAME_RR            0x00
> +#define L2CAP_SFRAME_REJ           0x01
> +#define L2CAP_SFRAME_RNR           0x02
> +#define L2CAP_SFRAME_SREJ          0x03
> #define L2CAP_SUPER_RCV_READY           0x0000
> #define L2CAP_SUPER_REJECT              0x0004
> #define L2CAP_SUPER_RCV_NOT_READY       0x0008
> #define L2CAP_SUPER_SELECT_REJECT       0x000C
>
> /* L2CAP Segmentation and Reassembly */
> +#define L2CAP_SAR_UNSEGMENTED      0x00
> +#define L2CAP_SAR_START            0x01
> +#define L2CAP_SAR_END              0x02
> +#define L2CAP_SAR_CONTINUE         0x03
> #define L2CAP_SDU_UNSEGMENTED       0x0000
> #define L2CAP_SDU_START             0x4000
> #define L2CAP_SDU_END               0x8000
>
> What is that? we already have such defines.

I was trying to not break existing code, while also creating constants 
that were useful for modified code that will work with either extended 
or enhanced headers.  The new values are important, but I agree that 
this is not the way to introduce the changes.  See below for more 
detail.

> commit 162e6de6d5c11b8ffea91a9fa2d597340afdeb6e
> Author: Mat Martineau <mathewm@codeaurora.org>
> Date:   Thu Aug 5 16:59:46 2010 -0700
>
>    Bluetooth: Remove unused L2CAP code.
>
> net/bluetooth/l2cap.c | 1104 +------------------------------------------------
> 1 files changed, 15 insertions(+), 1089 deletions(-)
>
> That's not acceptable, we have to modify the existent base code and make it
> work with the new features, instead writing a new one and then switch to it.

When setting up the commits for this git branch, I had to pick between 
two approaches:

  * Build up a modified state machine in parallel, so the switchover 
could happen in one patch.  The code would compile and run after every 
patch.

  * Or, start modifying the state machine piece by piece.  Until all of 
the modifications were done, ERTM would not work.

I don't think my approach worked out well (mostly because it doesn't 
preserve history adequately, and doesn't make it clear where code has 
been modified instead of newly written).  However, it's what we had to 
share coming in to the Bluetooth summit, and we felt it was very 
important to share it.  I want to refine the approach to these patches 
to put them in some acceptable form, so lets talk about the best way 
to do that.


> commit 09850f68219572288fe62a59235fda3d2629c7b2
> Author: Peter Krystad <pkrystad@codeaurora.org>
> Date:   Wed Aug 4 16:55:11 2010 -0700
>
>    Rename l2cap.c to el2cap.c.
>
>    Necessary before additional files can be added to l2cap module.
>    A module cannot contain a file with the same name as the module.
>
> We don't neeed that. We might split l2cap.c into smaller chunks before merge
> all the AMP stuff into it, so we won't have the module name problem anymore.

There is a technical reason for this.  We changed the Makefile to 
create l2cap.ko from two source files - one for L2CAP and one for AMP. 
However, the build system fails if you try to link l2cap.c and amp.c 
in to l2cap.ko.  We had to come up with some other name for l2cap.c in 
order to keep the module name the same.  There's probably a better 
choice than el2cap.c - any suggestions are welcome.


--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum

Re: QuiC AMP development

[Gustavo F. Padovan]

Hi Mat,

* Mat Martineau <mathewm@codeaurora.org> [2010-08-10 19:55:34 -0700]:

> 
> Gustavo -
> 
> On Tue, 10 Aug 2010, Gustavo F. Padovan wrote:
> 
> >Hi all,
> >
> >* Ron Shaffer <rshaffer@codeaurora.org> [2010-08-08 21:43:36 -0500]:
> >
> >>As requested in today's summit discussions, here's a link that can be
> >>used to inspect the code we've done to add AMP support on the latest
> >>next tree.
> >>
> >>https://www.codeaurora.org/gitweb/quic/bluetooth/?p=bluetooth-next-2.6.git;a=summary
> >>branch pk-upstream
> >
> >
> >I'll put here some comments I have about some L2CAP patches:
> 
> Thanks for taking the time to look over all these changes!
> 
> >
> >
> >+static int l2cap_seq_list_init(struct l2cap_seq_list *seq_list, u16 size)
> >+{
> >+       u16 allocSize;
> >
> >No capital letter here. Do alloc_size. Check the rest of your code for similar
> >stuff.
> 
> Yes, someone caught that in our internal code review too.  I think
> this was the only one.
> 
> >
> >
> >       u8 event;
> >       struct sk_buff *skb;
> >};
> >+
> >#endif /* __AMP_H */
> >diff --git a/net/bluetooth/amp.c b/net/bluetooth/amp.c
> >index f43d7d8..16e74f6 100644
> >--- a/net/bluetooth/amp.c
> >+++ b/net/bluetooth/amp.c
> >@@ -40,6 +40,7 @@ static struct workqueue_struct *amp_workqueue;
> >LIST_HEAD(amp_mgr_list);
> >DEFINE_RWLOCK(amp_mgr_list_lock);
> >
> >+
> >static void remove_amp_mgr(struct amp_mgr *rem)
> >{
> >       struct amp_mgr *mgr = NULL;
> >
> >Do not add new lines random places into your patch. Check you code for others
> >things like that. Also check for lines overstepping the column 80.
> 
> We definitely know we have work to do to clean up these patches and
> add proper commit messages.  Please be assured that we'll have these
> more tidied up before posting patches to linux-bluetooth.

;)

> 
> >
> >
> >+struct l2cap_enhanced_hdr {
> >+       __le16     len;
> >+       __le16     cid;
> >+       __le16     control;
> >+} __attribute__ ((packed));
> >+#define L2CAP_ENHANCED_HDR_SIZE                6
> >
> >Get ride off this struct, that actually doesn't help. We tried that before and
> >Marcel and I agreed that it does not help.
> 
> If you guys don't like it, I'll take it out.
> 
> >
> >-               if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov, count))
> >+
> >+               if (memcpy_fromiovec(skb_put(*frag, count),
> >+                               msg->msg_iov, count))
> >                       return -EFAULT;
> >
> >Avoid changes like that in your patches, you are just breaking a line here. If
> >you want to do that do in a separeted patch.
> >
> >
> >
> >+/* L2CAP ERTM / Streaming extra field lengths */
> >+#define L2CAP_FCS_SIZE          2
> >
> >Separated patch for that, so then you replace 2 for L2CAP_FCS_SIZE in one
> >patch, instead of doing that in many patches like you are doing now. The same
> >goes for L2CAP_SDULEN_SIZE.
> 
> Yes, I already split that commit up to send the "RFC" patch set
> earlier today.  Unfortunately I missed that commit when generating
> my patches :(
> 
> 
> >-/* L2CAP Supervisory Function */
> >+/* L2CAP Supervisory Frame Types */
> >+#define L2CAP_SFRAME_RR            0x00
> >+#define L2CAP_SFRAME_REJ           0x01
> >+#define L2CAP_SFRAME_RNR           0x02
> >+#define L2CAP_SFRAME_SREJ          0x03
> >#define L2CAP_SUPER_RCV_READY           0x0000
> >#define L2CAP_SUPER_REJECT              0x0004
> >#define L2CAP_SUPER_RCV_NOT_READY       0x0008
> >#define L2CAP_SUPER_SELECT_REJECT       0x000C
> >
> >/* L2CAP Segmentation and Reassembly */
> >+#define L2CAP_SAR_UNSEGMENTED      0x00
> >+#define L2CAP_SAR_START            0x01
> >+#define L2CAP_SAR_END              0x02
> >+#define L2CAP_SAR_CONTINUE         0x03
> >#define L2CAP_SDU_UNSEGMENTED       0x0000
> >#define L2CAP_SDU_START             0x4000
> >#define L2CAP_SDU_END               0x8000
> >
> >What is that? we already have such defines.
> 
> I was trying to not break existing code, while also creating
> constants that were useful for modified code that will work with
> either extended or enhanced headers.  The new values are important,
> but I agree that this is not the way to introduce the changes.  See
> below for more detail.
> 
> >commit 162e6de6d5c11b8ffea91a9fa2d597340afdeb6e
> >Author: Mat Martineau <mathewm@codeaurora.org>
> >Date:   Thu Aug 5 16:59:46 2010 -0700
> >
> >   Bluetooth: Remove unused L2CAP code.
> >
> >net/bluetooth/l2cap.c | 1104 +------------------------------------------------
> >1 files changed, 15 insertions(+), 1089 deletions(-)
> >
> >That's not acceptable, we have to modify the existent base code and make it
> >work with the new features, instead writing a new one and then switch to it.
> 
> When setting up the commits for this git branch, I had to pick
> between two approaches:
> 
>  * Build up a modified state machine in parallel, so the switchover
> could happen in one patch.  The code would compile and run after
> every patch.
> 
>  * Or, start modifying the state machine piece by piece.  Until all
> of the modifications were done, ERTM would not work.
> 
> I don't think my approach worked out well (mostly because it doesn't
> preserve history adequately, and doesn't make it clear where code
> has been modified instead of newly written).  However, it's what we
> had to share coming in to the Bluetooth summit, and we felt it was
> very important to share it.  I want to refine the approach to these
> patches to put them in some acceptable form, so lets talk about the
> best way to do that.

I see. But now that they are public we can talk about and put them in
shape for the merge into the mainline.
I think it's possible to add AMP stuff without break ERTM (and we have
to take care about that, because now ERTM pass all PTS tests). We can
arrange your commits to not break ERTM any time.

> 
> 
> >commit 09850f68219572288fe62a59235fda3d2629c7b2
> >Author: Peter Krystad <pkrystad@codeaurora.org>
> >Date:   Wed Aug 4 16:55:11 2010 -0700
> >
> >   Rename l2cap.c to el2cap.c.
> >
> >   Necessary before additional files can be added to l2cap module.
> >   A module cannot contain a file with the same name as the module.
> >
> >We don't neeed that. We might split l2cap.c into smaller chunks before merge
> >all the AMP stuff into it, so we won't have the module name problem anymore.
> 
> There is a technical reason for this.  We changed the Makefile to
> create l2cap.ko from two source files - one for L2CAP and one for
> AMP. However, the build system fails if you try to link l2cap.c and
> amp.c in to l2cap.ko.  We had to come up with some other name for
> l2cap.c in order to keep the module name the same.  There's probably
> a better choice than el2cap.c - any suggestions are welcome.

Yeah, I know, that's why the l2cap.c split will help this. Marcel
told that on the meeting Sunday.

-- 
Gustavo F. Padovan
http://padovan.org

Re: QuiC AMP development

[sober song]

Hi Shaffer/Peter

static inline int send_a2mp_cmd(struct amp_mgr *mgr, u8 ident, u8
code, u16 len, void *data) {
	struct a2mp_cmd_hdr *hdr;
	int plen;
	u8 *buf;

	BT_DBG("ident %d code %d", ident, code);
	plen = sizeof(*hdr) + len;
	buf = kzalloc(plen, GFP_ATOMIC);
	if (!buf)
		return -ENOMEM;
	hdr = (struct a2mp_cmd_hdr *) buf;
	hdr->code  = code;
	hdr->ident = ident;
	hdr->len   = cpu_to_le16(len);
	buf += sizeof(*hdr);
	memcpy(buf, data, len);
	return send_a2mp(mgr->a2mp_sock, (u8 *) hdr, plen);
}

I see that here have a malloc, but i don't see free, doesn't it cause memleak?

Regards
sober


On 8/11/10, Gustavo F. Padovan <gustavo@padovan.org> wrote:
> Hi Mat,
>
> * Mat Martineau <mathewm@codeaurora.org> [2010-08-10 19:55:34 -0700]:
>
>>
>> Gustavo -
>>
>> On Tue, 10 Aug 2010, Gustavo F. Padovan wrote:
>>
>> >Hi all,
>> >
>> >* Ron Shaffer <rshaffer@codeaurora.org> [2010-08-08 21:43:36 -0500]:
>> >
>> >>As requested in today's summit discussions, here's a link that can be
>> >>used to inspect the code we've done to add AMP support on the latest
>> >>next tree.
>> >>
>> >>https://www.codeaurora.org/gitweb/quic/bluetooth/?p=bluetooth-next-2.6.git;a=summary
>> >>branch pk-upstream
>> >
>> >
>> >I'll put here some comments I have about some L2CAP patches:
>>
>> Thanks for taking the time to look over all these changes!
>>
>> >
>> >
>> >+static int l2cap_seq_list_init(struct l2cap_seq_list *seq_list, u16
>> > size)
>> >+{
>> >+       u16 allocSize;
>> >
>> >No capital letter here. Do alloc_size. Check the rest of your code for
>> > similar
>> >stuff.
>>
>> Yes, someone caught that in our internal code review too.  I think
>> this was the only one.
>>
>> >
>> >
>> >       u8 event;
>> >       struct sk_buff *skb;
>> >};
>> >+
>> >#endif /* __AMP_H */
>> >diff --git a/net/bluetooth/amp.c b/net/bluetooth/amp.c
>> >index f43d7d8..16e74f6 100644
>> >--- a/net/bluetooth/amp.c
>> >+++ b/net/bluetooth/amp.c
>> >@@ -40,6 +40,7 @@ static struct workqueue_struct *amp_workqueue;
>> >LIST_HEAD(amp_mgr_list);
>> >DEFINE_RWLOCK(amp_mgr_list_lock);
>> >
>> >+
>> >static void remove_amp_mgr(struct amp_mgr *rem)
>> >{
>> >       struct amp_mgr *mgr = NULL;
>> >
>> >Do not add new lines random places into your patch. Check you code for
>> > others
>> >things like that. Also check for lines overstepping the column 80.
>>
>> We definitely know we have work to do to clean up these patches and
>> add proper commit messages.  Please be assured that we'll have these
>> more tidied up before posting patches to linux-bluetooth.
>
> ;)
>
>>
>> >
>> >
>> >+struct l2cap_enhanced_hdr {
>> >+       __le16     len;
>> >+       __le16     cid;
>> >+       __le16     control;
>> >+} __attribute__ ((packed));
>> >+#define L2CAP_ENHANCED_HDR_SIZE                6
>> >
>> >Get ride off this struct, that actually doesn't help. We tried that
>> > before and
>> >Marcel and I agreed that it does not help.
>>
>> If you guys don't like it, I'll take it out.
>>
>> >
>> >-               if (memcpy_fromiovec(skb_put(*frag, count), msg->msg_iov,
>> > count))
>> >+
>> >+               if (memcpy_fromiovec(skb_put(*frag, count),
>> >+                               msg->msg_iov, count))
>> >                       return -EFAULT;
>> >
>> >Avoid changes like that in your patches, you are just breaking a line
>> > here. If
>> >you want to do that do in a separeted patch.
>> >
>> >
>> >
>> >+/* L2CAP ERTM / Streaming extra field lengths */
>> >+#define L2CAP_FCS_SIZE          2
>> >
>> >Separated patch for that, so then you replace 2 for L2CAP_FCS_SIZE in one
>> >patch, instead of doing that in many patches like you are doing now. The
>> > same
>> >goes for L2CAP_SDULEN_SIZE.
>>
>> Yes, I already split that commit up to send the "RFC" patch set
>> earlier today.  Unfortunately I missed that commit when generating
>> my patches :(
>>
>>
>> >-/* L2CAP Supervisory Function */
>> >+/* L2CAP Supervisory Frame Types */
>> >+#define L2CAP_SFRAME_RR            0x00
>> >+#define L2CAP_SFRAME_REJ           0x01
>> >+#define L2CAP_SFRAME_RNR           0x02
>> >+#define L2CAP_SFRAME_SREJ          0x03
>> >#define L2CAP_SUPER_RCV_READY           0x0000
>> >#define L2CAP_SUPER_REJECT              0x0004
>> >#define L2CAP_SUPER_RCV_NOT_READY       0x0008
>> >#define L2CAP_SUPER_SELECT_REJECT       0x000C
>> >
>> >/* L2CAP Segmentation and Reassembly */
>> >+#define L2CAP_SAR_UNSEGMENTED      0x00
>> >+#define L2CAP_SAR_START            0x01
>> >+#define L2CAP_SAR_END              0x02
>> >+#define L2CAP_SAR_CONTINUE         0x03
>> >#define L2CAP_SDU_UNSEGMENTED       0x0000
>> >#define L2CAP_SDU_START             0x4000
>> >#define L2CAP_SDU_END               0x8000
>> >
>> >What is that? we already have such defines.
>>
>> I was trying to not break existing code, while also creating
>> constants that were useful for modified code that will work with
>> either extended or enhanced headers.  The new values are important,
>> but I agree that this is not the way to introduce the changes.  See
>> below for more detail.
>>
>> >commit 162e6de6d5c11b8ffea91a9fa2d597340afdeb6e
>> >Author: Mat Martineau <mathewm@codeaurora.org>
>> >Date:   Thu Aug 5 16:59:46 2010 -0700
>> >
>> >   Bluetooth: Remove unused L2CAP code.
>> >
>> >net/bluetooth/l2cap.c | 1104
>> > +------------------------------------------------
>> >1 files changed, 15 insertions(+), 1089 deletions(-)
>> >
>> >That's not acceptable, we have to modify the existent base code and make
>> > it
>> >work with the new features, instead writing a new one and then switch to
>> > it.
>>
>> When setting up the commits for this git branch, I had to pick
>> between two approaches:
>>
>>  * Build up a modified state machine in parallel, so the switchover
>> could happen in one patch.  The code would compile and run after
>> every patch.
>>
>>  * Or, start modifying the state machine piece by piece.  Until all
>> of the modifications were done, ERTM would not work.
>>
>> I don't think my approach worked out well (mostly because it doesn't
>> preserve history adequately, and doesn't make it clear where code
>> has been modified instead of newly written).  However, it's what we
>> had to share coming in to the Bluetooth summit, and we felt it was
>> very important to share it.  I want to refine the approach to these
>> patches to put them in some acceptable form, so lets talk about the
>> best way to do that.
>
> I see. But now that they are public we can talk about and put them in
> shape for the merge into the mainline.
> I think it's possible to add AMP stuff without break ERTM (and we have
> to take care about that, because now ERTM pass all PTS tests). We can
> arrange your commits to not break ERTM any time.
>
>>
>>
>> >commit 09850f68219572288fe62a59235fda3d2629c7b2
>> >Author: Peter Krystad <pkrystad@codeaurora.org>
>> >Date:   Wed Aug 4 16:55:11 2010 -0700
>> >
>> >   Rename l2cap.c to el2cap.c.
>> >
>> >   Necessary before additional files can be added to l2cap module.
>> >   A module cannot contain a file with the same name as the module.
>> >
>> >We don't neeed that. We might split l2cap.c into smaller chunks before
>> > merge
>> >all the AMP stuff into it, so we won't have the module name problem
>> > anymore.
>>
>> There is a technical reason for this.  We changed the Makefile to
>> create l2cap.ko from two source files - one for L2CAP and one for
>> AMP. However, the build system fails if you try to link l2cap.c and
>> amp.c in to l2cap.ko.  We had to come up with some other name for
>> l2cap.c in order to keep the module name the same.  There's probably
>> a better choice than el2cap.c - any suggestions are welcome.
>
> Yeah, I know, that's why the l2cap.c split will help this. Marcel
> told that on the meeting Sunday.
>
> --
> Gustavo F. Padovan
> http://padovan.org
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: QuiC AMP development

[Peter Krystad]

Hi Sober,

Be careful not to top-post on this mailing list.

> Hi Shaffer/Peter
>
> static inline int send_a2mp_cmd(struct amp_mgr *mgr, u8 ident, u8
> code, u16 len, void *data) {
> 	struct a2mp_cmd_hdr *hdr;
> 	int plen;
> 	u8 *buf;
>
> 	BT_DBG("ident %d code %d", ident, code);
> 	plen = sizeof(*hdr) + len;
> 	buf = kzalloc(plen, GFP_ATOMIC);
> 	if (!buf)
> 		return -ENOMEM;
> 	hdr = (struct a2mp_cmd_hdr *) buf;
> 	hdr->code  = code;
> 	hdr->ident = ident;
> 	hdr->len   = cpu_to_le16(len);
> 	buf += sizeof(*hdr);
> 	memcpy(buf, data, len);
> 	return send_a2mp(mgr->a2mp_sock, (u8 *) hdr, plen);
> }
>
> I see that here have a malloc, but i don't see free, doesn't it cause
> memleak?
>

Looks like it, thanks. As Mat and I explained to Kevin and Dan at the
mini-summit the git repo we posted is a snapshot of our development tree,
and was provided so interested parties could review our approach. As we
post patches to the mailing list your review will be much appreciated.

> Regards
> sober
>
>

>>> >
>>> >* Ron Shaffer <rshaffer@codeaurora.org> [2010-08-08 21:43:36 -0500]:
>>> >
>>> >>As requested in today's summit discussions, here's a link that can be
>>> >>used to inspect the code we've done to add AMP support on the latest
>>> >>next tree.
>>> >>
>>> >>https://www.codeaurora.org/gitweb/quic/bluetooth/?p=bluetooth-next-2.6.git;a=summary
>>> >>branch pk-upstream
>>> >
>>> >


Peter.

-- 

Peter Krystad

Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum