* Re: QuiC AMP development
From: Mat Martineau @ 2010-08-11 2:55 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: Ron Shaffer, linux-bluetooth
In-Reply-To: <20100810220238.GB3203@vigoh>
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
^ permalink raw reply
* Re: patch for firmware download to Qualcomm Bluetooth chip
From: Ron Shaffer @ 2010-08-11 2:27 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth, johan.hedberg
In-Reply-To: <1281474848.12579.218.camel@localhost.localdomain>
On 8/10/2010 5:14 PM, Marcel Holtmann wrote:
> Hi Ron,
>
>> Here's the a ping regarding the patch submitted by Matt back in March? I
>> believe. For you convenience I've included the patch again. Please take
>> a look, and provide your general comments. I'm sure there are changes
>> that need to be made.
>>
>> Fyi. sorry about the column wrap in the patch. Don't feel like fixing it
>> for just a quick review.
>>
>> From 76d0bdd82a0a4e5b3b9544bb864c31888f20cea1 Mon Sep 17 00:00:00 2001
>> From: Wilson, Matt <mtwilson@codeaurora.org>
>> Date: Thu, 11 Feb 2010 11:53:29 -0600
>> Subject: [PATCH] Firmware download for Qualcomm Bluetooth devices
>>
>> ---
>> Makefile.tools | 3 +-
>> tools/hciattach.c | 9 ++
>> tools/hciattach.h | 4 +
>> tools/hciattach_qualcomm.c | 279
>> ++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 294 insertions(+), 1 deletions(-)
>> create mode 100644 tools/hciattach_qualcomm.c
>>
>> diff --git a/Makefile.tools b/Makefile.tools
>> index 2735d68..7b92c8f 100644
>> --- a/Makefile.tools
>> +++ b/Makefile.tools
>> @@ -23,7 +23,8 @@ tools_l2ping_LDADD = lib/libbluetooth.la
>> tools_hciattach_SOURCES = tools/hciattach.c tools/hciattach.h \
>> tools/hciattach_st.c \
>> tools/hciattach_ti.c \
>> - tools/hciattach_tialt.c
>> + tools/hciattach_tialt.c \
>> + tools/hciattach_qualcomm.c
>> tools_hciattach_LDADD = lib/libbluetooth.la
>>
>> tools_hciconfig_SOURCES = tools/hciconfig.c tools/csr.h tools/csr.c \
>> diff --git a/tools/hciattach.c b/tools/hciattach.c
>> index 364c5ff..d6aafbe 100644
>> --- a/tools/hciattach.c
>> +++ b/tools/hciattach.c
>> @@ -5,6 +5,7 @@
>> * Copyright (C) 2000-2001 Qualcomm Incorporated
>> * Copyright (C) 2002-2003 Maxim Krasnyansky <maxk@qualcomm.com>
>> * Copyright (C) 2002-2010 Marcel Holtmann <marcel@holtmann.org>
>> + * Copyright (C) 2010, Code Aurora Forum. All rights reserved.
>> *
>> *
>> * This program is free software; you can redistribute it and/or modify
>> @@ -299,6 +300,11 @@ static int texasalt(int fd, struct uart_t *u, struct
>> termios *ti)
>> return texasalt_init(fd, u->speed, ti);
>> }
>>
>> +static int qualcomm(int fd, struct uart_t *u, struct termios *ti)
>> +{
>> + return qualcomm_init(fd, u->speed, ti, u->bdaddr);
>> +}
>> +
>> static int read_check(int fd, void *buf, int count)
>> {
>> int res;
>> @@ -1071,6 +1077,9 @@ struct uart_t uart[] = {
>> /* Broadcom BCM2035 */
>> { "bcm2035", 0x0A5C, 0x2035, HCI_UART_H4, 115200, 460800, FLOW_CTL,
>> NULL, bcm2035 },
>>
>> + /* QUALCOMM BTS */
>> + { "qualcomm", 0x0000, 0x0000, HCI_UART_H4, 115200, 115200, FLOW_CTL,
>> NULL, qualcomm },
>> +
>> { NULL, 0 }
>> };
>>
>> diff --git a/tools/hciattach.h b/tools/hciattach.h
>> index 867563b..5c89013 100644
>> --- a/tools/hciattach.h
>> +++ b/tools/hciattach.h
>> @@ -3,6 +3,7 @@
>> * BlueZ - Bluetooth protocol stack for Linux
>> *
>> * Copyright (C) 2003-2010 Marcel Holtmann <marcel@holtmann.org>
>> + * Copyright (c) 2010, Code Aurora Forum. All rights reserved.
>> *
>> *
>> * This program is free software; you can redistribute it and/or modify
>> @@ -45,3 +46,6 @@ int texas_post(int fd, struct termios *ti);
>> int texasalt_init(int fd, int speed, struct termios *ti);
>> int stlc2500_init(int fd, bdaddr_t *bdaddr);
>> int bgb2xx_init(int dd, bdaddr_t *bdaddr);
>> +int qualcomm_init(int fd, int speed, struct termios *ti, const char
>> *bdaddr);
>> +
>> +
>
> I am actually fine with this. Except strip it from claiming copyright on
> hciattach.c and hciattach.h since these changes are not really anything
> new. They are just wrapping code.
>
> So fix this and submit a new clean patch.
>
> Regards
>
> Marcel
>
>
> --
> 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
>
Yes, will do.
--
Ron Shaffer
Employee of the Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply
* Re: [RFC 2/7] Bluetooth: Use enhanced L2CAP header structure and symbolic values.
From: Mat Martineau @ 2010-08-11 2:24 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: linux-bluetooth, marcel, rshaffer, linux-arm-msm
In-Reply-To: <20100810220712.GC3203@vigoh>
Gustavo -
On Tue, 10 Aug 2010, Gustavo F. Padovan wrote:
> Hi Mat,
>
> * Mat Martineau <mathewm@codeaurora.org> [2010-08-10 12:14:59 -0700]:
>
>>
>> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
>> ---
>> net/bluetooth/l2cap.c | 23 +++++++++++++----------
>> 1 files changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
>> index 5e78c18..aa69c84 100644
>> --- a/net/bluetooth/l2cap.c
>> +++ b/net/bluetooth/l2cap.c
>> @@ -1753,33 +1753,36 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m
>> {
>> struct l2cap_conn *conn = l2cap_pi(sk)->conn;
>> struct sk_buff *skb;
>> - int err, count, hlen = L2CAP_HDR_SIZE + 2;
>> - struct l2cap_hdr *lh;
>> + int err, count, hlen = L2CAP_ENHANCED_HDR_SIZE;
>> + struct l2cap_enhanced_hdr *lh;
>
> This patch shouldn't even compile. Since there no definition for
> struct l2cap_enhanced_hdr. Anyway we should not have that struct, keep
> the code to handle this as it is.
> We'll have to change this code again to add the extended control bit
> field.
Sorry about that - I did my git format-patch starting one commit too
late.
I also have an l2cap_extended_hdr struct to add later, but it sounds
like you don't want that one either.
>
>>
>> - BT_DBG("sk %p len %d", sk, (int)len);
>> + BT_DBG("sk %p, msg %p, len %d, control %x, sdulen %d",
>> + sk, msg, (int)len, control, (int)sdulen);
>>
>> if (!conn)
>> return ERR_PTR(-ENOTCONN);
>>
>> if (sdulen)
>> - hlen += 2;
>> + hlen += L2CAP_SDULEN_SIZE;
>
> Separated patch to add L2CAP_SDULEN_SIZE
Ok.
>>
>> if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16)
>> - hlen += 2;
>> + hlen += L2CAP_FCS_SIZE;
>
> Separated patch to ad L2CAP_FCS_SIZE
Ok. Does this need to be separate from the L2CAP_SDULEN_SIZE patch?
>
>>
>> count = min_t(unsigned int, (conn->mtu - hlen), len);
>> +
>> skb = bt_skb_send_alloc(sk, count + hlen,
>> msg->msg_flags & MSG_DONTWAIT, &err);
>> if (!skb)
>> - return ERR_PTR(-ENOMEM);
>> + return ERR_PTR(err);
>
> This also should not be in this patch.
No problem.
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply
* Re: why is hciops a plugin ?
From: Johan Hedberg @ 2010-08-11 2:16 UTC (permalink / raw)
To: Pavan Savoy; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <AANLkTino39jMNvdcV=EiekN33vrfNXLyJVNsHcHtq5wz@mail.gmail.com>
Hi,
On Tue, Aug 10, 2010, Pavan Savoy wrote:
> Any reason behind making hciops a plugin and bluetoothd making use of
> that plugin, wasn't everything straight up in hcid before ?
We're planning on redoing the kernel-userspace interface and in the long
run the traditional raw HCI access wont exist anymore in userspace. In
order to accommodate both old and new kernels in a clean way the
interface towards the kernel needs to be easily interchangable, which is
what hciops is trying to do (it's just a first step in that direction
though). One of the drivers for this change is the need to have the
security logic in one place instead of it being split between kernel and
userspace (this has caused all sorts of trouble with SSP for us).
Originally the plan for the new kernel interface was netlink but now the
idea is to simply extend the stack internal messages of raw HCI sockets
with a more complete two-way protocol between the kernel and userspace.
You should (hopefully) be seeing more patches for this still during this
year.
Johan
^ permalink raw reply
* [PATCH 2/2] HCI events related to AMP functionality
From: Inga Stotland @ 2010-08-11 0:12 UTC (permalink / raw)
To: linux-bluetooth; +Cc: rshaffer, johan.hedberg, marcel, Inga Stotland
In-Reply-To: <1281485576-32715-1-git-send-email-ingas@codeaurora.org>
Added constructs for the following HCI events:
PHYSICAL_LINK_COMPLETE, EVT_CHANNEL_SELECTED,
EVT_DISCONNECT_PHYSICAL_LINK_COMPLETE,
EVT_PHYSICAL_LINK_LOSS_EARLY_WARNING, EVT_PHYSICAL_LINK_RECOVERY,
EVT_LOGICAL_LINK_COMPLETE, EVT_DISCONNECT_LOGICAL_LINK_COMPLETE,
EVT_FLOW_SPEC_MODIFY_COMPLETE, EVT_NUMBER_COMPLETED_BLOCKS,
EVT_AMP_STATUS_CHANGE
---
lib/hci.h | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 57 insertions(+), 0 deletions(-)
diff --git a/lib/hci.h b/lib/hci.h
index ae7b6da..8f35422 100644
--- a/lib/hci.h
+++ b/lib/hci.h
@@ -2108,6 +2108,63 @@ typedef struct {
} __attribute__ ((packed)) evt_le_long_term_key_request;
#define EVT_LE_LTK_REQUEST_SIZE 12
+#define EVT_PHYSICAL_LINK_COMPLETE 0x40
+typedef struct {
+ uint8_t status;
+ uint8_t handle;
+} __attribute__ ((packed)) evt_physical_link_complete;
+#define EVT_PHYSICAL_LINK_COMPLETE_SIZE 2
+
+#define EVT_CHANNEL_SELECTED 0x41
+
+#define EVT_DISCONNECT_PHYSICAL_LINK_COMPLETE 0x42
+typedef struct {
+ uint8_t status;
+ uint8_t handle;
+ uint8_t reason;
+} __attribute__ ((packed)) evt_disconn_physical_link_complete;
+#define EVT_DISCONNECT_PHYSICAL_LINK_COMPLETE_SIZE 3
+
+#define EVT_PHYSICAL_LINK_LOSS_EARLY_WARNING 0x43
+typedef struct {
+ uint8_t handle;
+ uint8_t reason;
+} __attribute__ ((packed)) evt_physical_link_loss_warning;
+#define EVT_PHYSICAL_LINK_LOSS_WARNING_SIZE 2
+
+#define EVT_PHYSICAL_LINK_RECOVERY 0x44
+typedef struct {
+ uint8_t handle;
+} __attribute__ ((packed)) evt_physical_link_recovery;
+#define EVT_PHYSICAL_LINK_RECOVERY_SIZE 1
+
+#define EVT_LOGICAL_LINK_COMPLETE 0x45
+typedef struct {
+ uint8_t status;
+ uint16_t log_handle;
+ uint8_t handle;
+ uint8_t tx_flow_id;
+} __attribute__ ((packed)) evt_logical_link_complete;
+#define EVT_LOGICAL_LINK_COMPLETE_SIZE 5
+
+#define EVT_DISCONNECT_LOGICAL_LINK_COMPLETE 0x46
+
+#define EVT_FLOW_SPEC_MODIFY_COMPLETE 0x47
+typedef struct {
+ uint8_t status;
+ uint16_t handle;
+} __attribute__ ((packed)) evt_flow_spec_modify_complete;
+#define EVT_FLOW_SPEC_MODIFY_COMPLETE_SIZE 3
+
+#define EVT_NUMBER_COMPLETED_BLOCKS 0x48
+
+#define EVT_AMP_STATUS_CHANGE 0x4D
+typedef struct {
+ uint8_t status;
+ uint8_t amp_status;
+} __attribute__ ((packed)) evt_amp_status_change;
+#define EVT_AMP_STATUS_CHANGE_SIZE 2
+
#define EVT_TESTING 0xFE
#define EVT_VENDOR 0xFF
--
1.7.2
--
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply related
* [PATCH 1/2] HCI commands related to AMP functionality
From: Inga Stotland @ 2010-08-11 0:12 UTC (permalink / raw)
To: linux-bluetooth; +Cc: rshaffer, johan.hedberg, marcel, Inga Stotland
In-Reply-To: <1281485576-32715-1-git-send-email-ingas@codeaurora.org>
Added constructs for the following HCI commands:
CREATE_PHYSICAL_LINK, ACCEPT_PHYSICAL_LINK, DISCONNECT_PHYSICAL_LINK,
CREATE_LOGICAL_LINK, ACCEPT_LOGICAL_LINK, DISCONNECT_LOGICAL_LINK,
LOGICAL_LINK_CANCEL, FLOW_SPEC_MODIFY,
READ_LOGICAL_LINK_ACCEPT_TIMEOUT, WRITE_LOGICAL_LINK_ACCEPT_TIMEOUT,
SET_EVENT_MASK_PAGE_2, READ_LOCATION_DATA, WRITE_LOCATION_DATA,
READ_FLOW_CONTROL_MODE, WRITE_FLOW_CONTROL_MODE,
READ_ENHANCED_TRANSMIT_POWER_LEVEL, READ_BEST_EFFORT_FLUSH_TIMEOUT,
WRITE_BEST_EFFORT_FLUSH_TIMEOUT, READ_LOCAL_AMP_INFO,
READ_LOCAL_AMP_ASSOC, WRITE_REMOTE_AMP_ASSOC
---
lib/hci.h | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 145 insertions(+), 0 deletions(-)
diff --git a/lib/hci.h b/lib/hci.h
index f31918d..ae7b6da 100644
--- a/lib/hci.h
+++ b/lib/hci.h
@@ -528,6 +528,55 @@ typedef struct {
} __attribute__ ((packed)) io_capability_neg_reply_cp;
#define IO_CAPABILITY_NEG_REPLY_CP_SIZE 7
+#define OCF_CREATE_PHYSICAL_LINK 0x0035
+typedef struct {
+ uint8_t handle;
+ uint8_t key_length;
+ uint8_t key_type;
+ uint8_t key[32];
+} __attribute__ ((packed)) create_physical_link_cp;
+#define CREATE_PHYSICAL_LINK_CP_SIZE 35
+
+#define OCF_ACCEPT_PHYSICAL_LINK 0x0036
+
+#define OCF_DISCONNECT_PHYSICAL_LINK 0x0037
+typedef struct {
+ uint8_t handle;
+ uint8_t reason;
+} __attribute__ ((packed)) disconnect_physical_link_cp;
+#define DISCONNECT_PHYSICAL_LINK_CP_SIZE 2
+
+#define OCF_CREATE_LOGICAL_LINK 0x0038
+typedef struct {
+ uint8_t handle;
+ uint8_t tx_flow[16];
+ uint8_t rx_flow[16];
+} __attribute__ ((packed)) create_logical_link_cp;
+#define CREATE_LOGICAL_LINK_CP_SIZE 33
+
+#define OCF_ACCEPT_LOGICAL_LINK 0x0039
+
+#define OCF_DISCONNECT_LOGICAL_LINK 0x003A
+typedef struct {
+ uint16_t handle;
+} __attribute__ ((packed)) disconnect_logical_link_cp;
+#define DISCONNECT_LOGICAL_LINK_CP_SIZE 2
+
+#define OCF_LOGICAL_LINK_CANCEL 0x003B
+typedef struct {
+ uint8_t handle;
+ uint8_t tx_flow_id;
+} __attribute__ ((packed)) cancel_logical_link_cp;
+#define LOGICAL_LINK_CANCEL_CP_SIZE 2
+typedef struct {
+ uint8_t status;
+ uint8_t handle;
+ uint8_t tx_flow_id;
+} __attribute__ ((packed)) cancel_logical_link_rp;
+#define LOGICAL_LINK_CANCEL_RP_SIZE 3
+
+#define OCF_FLOW_SPEC_MODIFY 0x003C
+
/* Link Policy */
#define OGF_LINK_POLICY 0x02
@@ -1123,6 +1172,57 @@ typedef struct {
} __attribute__ ((packed)) send_keypress_notify_rp;
#define SEND_KEYPRESS_NOTIFY_RP_SIZE 1
+#define OCF_READ_LOGICAL_LINK_ACCEPT_TIMEOUT 0x0061
+typedef struct {
+ uint8_t status;
+ uint16_t timeout;
+} __attribute__ ((packed)) read_log_link_accept_timeout_rp;
+#define READ_LOGICAL_LINK_ACCEPT_TIMEOUT_RP_SIZE 3
+
+#define OCF_WRITE_LOGICAL_LINK_ACCEPT_TIMEOUT 0x0062
+typedef struct {
+ uint16_t timeout;
+} __attribute__ ((packed)) write_log_link_accept_timeout_cp;
+#define WRITE_LOGICAL_LINK_ACCEPT_TIMEOUT_CP_SIZE 2
+
+#define OCF_SET_EVENT_MASK_PAGE_2 0x0063
+
+#define OCF_READ_LOCATION_DATA 0x0064
+
+#define OCF_WRITE_LOCATION_DATA 0x0065
+
+#define OCF_READ_FLOW_CONTROL_MODE 0x0066
+
+#define OCF_WRITE_FLOW_CONTROL_MODE 0x0067
+
+#define OCF_READ_ENHANCED_TRANSMIT_POWER_LEVEL 0x0068
+typedef struct {
+ uint8_t status;
+ uint16_t handle;
+ int8_t level_gfsk;
+ int8_t level_dqpsk;
+ int8_t level_8dpsk;
+} __attribute__ ((packed)) read_enhanced_transmit_power_level_rp;
+#define READ_ENHANCED_TRANSMIT_POWER_LEVEL_RP_SIZE 6
+
+#define OCF_READ_BEST_EFFORT_FLUSH_TIMEOUT 0x0069
+typedef struct {
+ uint8_t status;
+ uint32_t timeout;
+} __attribute__ ((packed)) read_best_effort_flush_timeout_rp;
+#define READ_BEST_EFFORT_FLUSH_TIMEOUT_RP_SIZE 5
+
+#define OCF_WRITE_BEST_EFFORT_FLUSH_TIMEOUT 0x006A
+typedef struct {
+ uint16_t handle;
+ uint32_t timeout;
+} __attribute__ ((packed)) write_best_effort_flush_timeout_cp;
+#define WRITE_BEST_EFFORT_FLUSH_TIMEOUT_CP_SIZE 6
+typedef struct {
+ uint8_t status;
+} __attribute__ ((packed)) write_best_effort_flush_timeout_rp;
+#define WRITE_BEST_EFFORT_FLUSH_TIMEOUT_RP_SIZE 1
+
/* Informational Parameters */
#define OGF_INFO_PARAM 0x04
@@ -1240,6 +1340,51 @@ typedef struct {
} __attribute__ ((packed)) read_clock_rp;
#define READ_CLOCK_RP_SIZE 9
+#define OCF_READ_LOCAL_AMP_INFO 0x0009
+typedef struct {
+ uint8_t status;
+ uint8_t amp_status;
+ uint32_t total_bandwidth;
+ uint32_t max_guaranteed_bandwidth;
+ uint32_t min_latency;
+ uint32_t max_pdu_size;
+ uint8_t controller_type;
+ uint16_t pal_caps;
+ uint16_t max_amp_assoc_length;
+ uint32_t max_flush_timeout;
+ uint32_t best_effort_flush_timeout;
+} __attribute__ ((packed)) read_local_amp_info_rp;
+#define READ_LOCAL_AMP_INFO_RP_SIZE 31
+
+#define OCF_READ_LOCAL_AMP_ASSOC 0x000A
+typedef struct {
+ uint8_t handle;
+ uint16_t length_so_far;
+ uint16_t assoc_length;
+} __attribute__ ((packed)) read_local_amp_assoc_cp;
+#define READ_LOCAL_AMP_ASSOC_CP_SIZE 5
+typedef struct {
+ uint8_t status;
+ uint8_t handle;
+ uint16_t length;
+ uint8_t fragment[248];
+} __attribute__ ((packed)) read_local_amp_assoc_rp;
+#define READ_LOCAL_AMP_ASSOC_RP_SIZE 252
+
+#define OCF_WRITE_REMOTE_AMP_ASSOC 0x000B
+typedef struct {
+ uint8_t handle;
+ uint16_t length_so_far;
+ uint16_t assoc_length;
+ uint8_t fragment[248];
+} __attribute__ ((packed)) write_remote_amp_assoc_cp;
+#define WRITE_REMOTE_AMP_ASSOC_CP_SIZE 253
+typedef struct {
+ uint8_t status;
+ uint8_t handle;
+} __attribute__ ((packed)) write_remote_amp_assoc_rp;
+#define WRITE_REMOTE_AMP_ASSOC_RP_SIZE 2
+
/* Testing commands */
#define OGF_TESTING_CMD 0x3e
--
1.7.2
--
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply related
* Support for HCI commands and events related to AMP functionality
From: Inga Stotland @ 2010-08-11 0:12 UTC (permalink / raw)
To: linux-bluetooth; +Cc: rshaffer, johan.hedberg, marcel
Added new HCI commands and events constructs for AMP related development.
Will be used in updated testing tools for AMP.
--
Inga Stotland
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply
* why is hciops a plugin ?
From: Pavan Savoy @ 2010-08-10 22:18 UTC (permalink / raw)
To: linux-bluetooth@vger.kernel.org
Any reason behind making hciops a plugin and bluetoothd making use of
that plugin, wasn't everything straight up in hcid before ?
Thanks in advance.
^ permalink raw reply
* Re: [RFC 2/7] Bluetooth: Use enhanced L2CAP header structure and symbolic values.
From: Gustavo F. Padovan @ 2010-08-10 22:07 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, marcel, rshaffer, linux-arm-msm
In-Reply-To: <1281467704-5378-3-git-send-email-mathewm@codeaurora.org>
Hi Mat,
* Mat Martineau <mathewm@codeaurora.org> [2010-08-10 12:14:59 -0700]:
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/l2cap.c | 23 +++++++++++++----------
> 1 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 5e78c18..aa69c84 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -1753,33 +1753,36 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m
> {
> struct l2cap_conn *conn = l2cap_pi(sk)->conn;
> struct sk_buff *skb;
> - int err, count, hlen = L2CAP_HDR_SIZE + 2;
> - struct l2cap_hdr *lh;
> + int err, count, hlen = L2CAP_ENHANCED_HDR_SIZE;
> + struct l2cap_enhanced_hdr *lh;
This patch shouldn't even compile. Since there no definition for
struct l2cap_enhanced_hdr. Anyway we should not have that struct, keep
the code to handle this as it is.
We'll have to change this code again to add the extended control bit
field.
>
> - BT_DBG("sk %p len %d", sk, (int)len);
> + BT_DBG("sk %p, msg %p, len %d, control %x, sdulen %d",
> + sk, msg, (int)len, control, (int)sdulen);
>
> if (!conn)
> return ERR_PTR(-ENOTCONN);
>
> if (sdulen)
> - hlen += 2;
> + hlen += L2CAP_SDULEN_SIZE;
Separated patch to add L2CAP_SDULEN_SIZE
>
> if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16)
> - hlen += 2;
> + hlen += L2CAP_FCS_SIZE;
Separated patch to ad L2CAP_FCS_SIZE
>
> count = min_t(unsigned int, (conn->mtu - hlen), len);
> +
> skb = bt_skb_send_alloc(sk, count + hlen,
> msg->msg_flags & MSG_DONTWAIT, &err);
> if (!skb)
> - return ERR_PTR(-ENOMEM);
> + return ERR_PTR(err);
This also should not be in this patch.
--
Gustavo F. Padovan
http://padovan.org
^ permalink raw reply
* Re: QuiC AMP development
From: Gustavo F. Padovan @ 2010-08-10 22:02 UTC (permalink / raw)
To: Ron Shaffer; +Cc: linux-bluetooth
In-Reply-To: <4C5F6B58.6000702@codeaurora.org>
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
^ permalink raw reply related
* Re: [RFC 1/7] Bluetooth: Calculate L2CAP FCS on fragmented skbuffs.
From: Gustavo F. Padovan @ 2010-08-10 21:57 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, marcel, rshaffer, linux-arm-msm
In-Reply-To: <1281467704-5378-2-git-send-email-mathewm@codeaurora.org>
Hi Mat,
* Mat Martineau <mathewm@codeaurora.org> [2010-08-10 12:14:58 -0700]:
> When L2CAP PDUs are longer than the HCI MTU, they are stored in
> fragmented skbuffs. This adds a function to calculate the FCS
> on any skbuff (fragmented or not), and replaces direct calls
> to crc16 with calls to the new apply_fcs() function.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/l2cap.c | 57 ++++++++++++++++++++++++++++++++----------------
> 1 files changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 8d362d7..5e78c18 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -351,6 +351,33 @@ static inline u8 l2cap_get_ident(struct l2cap_conn *conn)
> return id;
> }
>
> +static void apply_fcs(struct sk_buff *skb)
I would call this l2cap_apply_fcs().
--
Gustavo F. Padovan
http://padovan.org
^ permalink raw reply
* Re: [PATCH] Bluetooth: Process HCI events in a workqueue instead of a tasklet
From: Marcel Holtmann @ 2010-08-10 21:41 UTC (permalink / raw)
To: David Vrabel; +Cc: linux-bluetooth
In-Reply-To: <4C6161D7.7070507@csr.com>
Hi David,
> >>>> Signed-off-by: Marcel Holtmann <marcel@holtmann.org>
> >>>> ---
> >>>> include/net/bluetooth/hci_core.h | 2 +
> >>>> net/bluetooth/hci_core.c | 58 ++++++++++++++++++++++++++++++++-----
> >>>> 2 files changed, 52 insertions(+), 8 deletions(-)
> >>> so I stuffed this now into bluetooth-testing tree and would like to see
> >>> some extra testing exposure. So far this has only been tested by myself.
> >>>
> >>> If there are no regression then this should make a lot of HCI and L2CAP
> >>> handling a lot simple.
> >> This may result in packets being processed in a different order to that
> >> which they were received in.
> >>
> >> e.g., what happens to an ACL packet processed before the connection
> >> complete event for that connection?
> >
> > good point. So we would either a) need to disable the RX tasklet when we
> > receive an event and schedule it for processing or b) process the ACL
> > data also in a workqueue.
>
> I think my preferred solution would be for the rx function called by the
> HCI transport drivers to be called in a thread context. There should be
> a helper function that is callable in atomic context that queues the
> packets to be processed in a workqueue.
>
> Our UWB drivers already process packets in a thread so would prefer to
> avoid another thread context switch.
>
> Shall I prepare a patch?
yes please. I need something to make the event handling simpler.
Otherwise every single subcomponent creates "hundreds" of workqueues to
solve exactly the same problem.
Regards
Marcel
^ permalink raw reply
* Re: [RFC 2/7] Bluetooth: Use enhanced L2CAP header structure and symbolic values.
From: Marcel Holtmann @ 2010-08-10 21:39 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1281467704-5378-3-git-send-email-mathewm@codeaurora.org>
Hi Mat,
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/l2cap.c | 23 +++++++++++++----------
> 1 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index 5e78c18..aa69c84 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -1753,33 +1753,36 @@ static struct sk_buff *l2cap_create_iframe_pdu(struct sock *sk, struct msghdr *m
> {
> struct l2cap_conn *conn = l2cap_pi(sk)->conn;
> struct sk_buff *skb;
> - int err, count, hlen = L2CAP_HDR_SIZE + 2;
> - struct l2cap_hdr *lh;
> + int err, count, hlen = L2CAP_ENHANCED_HDR_SIZE;
> + struct l2cap_enhanced_hdr *lh;
so where is that patch that actually introduces L2CAP_ENHANCED_HDR_SIZE
etc. I can't see that.
Regards
Marcel
^ permalink raw reply
* Re: [RFC 4/7] Bluetooth: Linearize received L2CAP skbuffs.
From: Marcel Holtmann @ 2010-08-10 21:38 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1281467704-5378-5-git-send-email-mathewm@codeaurora.org>
Hi Mat,
> Fragmented skbs are only encountered by in-kernel protocols and
> profiles when receiving ERTM or streaming mode L2CAP data. BNEP,
> CMTP, HIDP, and RFCOMM generally use basic mode, so will not
> generally need the extra memcpy's necessary to put the L2CAP
> payload in to a linear buffer. However, it is possible to
> use enhanced L2CAP in these modes so the possibility needs
> to be handled.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/bnep/core.c | 5 ++++-
> net/bluetooth/cmtp/core.c | 5 ++++-
> net/bluetooth/hidp/core.c | 10 ++++++++--
> net/bluetooth/rfcomm/core.c | 5 ++++-
> 4 files changed, 20 insertions(+), 5 deletions(-)
in general I am fine with this.
Regards
Marcel
^ permalink raw reply
* Re: [RFC 3/7] Bluetooth: Add FCS awareness to L2CAP HCI fragmentation.
From: Marcel Holtmann @ 2010-08-10 21:29 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1281467704-5378-4-git-send-email-mathewm@codeaurora.org>
Hi Mat,
> In order to not limit ERTM and streaming mode PDUs to the HCI MTU
> size, L2CAP must be able to split PDUs in to multple HCI fragments.
> This is done by allocating space for the FCS in the last fragment.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/l2cap.c | 39 ++++++++++++++++++++++++++++++++++-----
> 1 files changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index aa69c84..b485c4a 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -1664,31 +1664,63 @@ static inline int l2cap_skbuff_fromiovec(struct sock *sk, struct msghdr *msg, in
> {
> struct l2cap_conn *conn = l2cap_pi(sk)->conn;
> struct sk_buff **frag;
> + struct sk_buff *final;
> int err, sent = 0;
>
> + BT_DBG("sk %p, msg %p, len %d, count %d, skb %p", sk,
> + msg, (int)len, (int)count, skb);
> +
> if (memcpy_fromiovec(skb_put(skb, count), msg->msg_iov, count))
> return -EFAULT;
>
> sent += count;
> len -= count;
> + final = skb;
>
> /* Continuation fragments (no L2CAP header) */
> frag = &skb_shinfo(skb)->frag_list;
> while (len) {
> + int skblen;
> count = min_t(unsigned int, conn->mtu, len);
I prefer that you either make the int skblen global or add an empty
line. There should be always an empty line between variable declaration
and code.
>
> - *frag = bt_skb_send_alloc(sk, count, msg->msg_flags & MSG_DONTWAIT, &err);
> + /* Add room for the FCS if it fits */
> + if (l2cap_pi(sk)->fcs == L2CAP_FCS_CRC16 &&
> + len + L2CAP_FCS_SIZE <= conn->mtu)
> + skblen = count + L2CAP_FCS_SIZE;
So in general we do double indent for the second line of the if:
if (l2cap_pi ...
len + ...
skblen = count ...
This is not strictly enforced and every kernel code does it a little bit
different.
The rest looks just fine.
Regards
Marcel
^ permalink raw reply
* Re: [RFC 1/7] Bluetooth: Calculate L2CAP FCS on fragmented skbuffs.
From: Marcel Holtmann @ 2010-08-10 21:22 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1281467704-5378-2-git-send-email-mathewm@codeaurora.org>
Hi Mat,
> When L2CAP PDUs are longer than the HCI MTU, they are stored in
> fragmented skbuffs. This adds a function to calculate the FCS
> on any skbuff (fragmented or not), and replaces direct calls
> to crc16 with calls to the new apply_fcs() function.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/l2cap.c | 57 ++++++++++++++++++++++++++++++++----------------
> 1 files changed, 38 insertions(+), 19 deletions(-)
looks fine to me. Just lets get the other ones in first and then we can
apply this one.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 2/5] Bluetooth: Validate PSM values in calls to connect() and bind().
From: Marcel Holtmann @ 2010-08-10 21:21 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1281466850-5297-3-git-send-email-mathewm@codeaurora.org>
Hi Mat,
> Valid L2CAP PSMs are odd numbers, and the least significant bit of the
> most significant byte must be 0.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/l2cap.c | 23 +++++++++++++++++++----
> 1 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> index c784703..61aa961 100644
> --- a/net/bluetooth/l2cap.c
> +++ b/net/bluetooth/l2cap.c
> @@ -1008,10 +1008,19 @@ static int l2cap_sock_bind(struct socket *sock, struct sockaddr *addr, int alen)
> goto done;
> }
>
> - if (la.l2_psm && __le16_to_cpu(la.l2_psm) < 0x1001 &&
> - !capable(CAP_NET_BIND_SERVICE)) {
> - err = -EACCES;
> - goto done;
> + /* If specified, PSM must be odd and lsb of upper byte must be 0 */
> + if (la.l2_psm) {
> + __u16 psm = __le16_to_cpu(la.l2_psm);
> +
I would just put the comment here:
/* PSM must be odd and lsb of upper byte must be 0 */
> + if ((psm & 0x0101) != 0x0001) {
> + err = -EINVAL;
> + goto done;
> + }
And then while you at it:
/* Restrict usage of well known PSMs */
> + if (psm < 0x1001 && !capable(CAP_NET_BIND_SERVICE)) {
> + err = -EACCES;
> + goto done;
> + }
> }
Maybe there is actually a second place for adding that comment.
Otherwise I am fine with this, but since it is not a regression, this
will be only taking in the next tree.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/5] Bluetooth: Only enable L2CAP FCS for ERTM or streaming.
From: Marcel Holtmann @ 2010-08-10 21:17 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, rshaffer, linux-arm-msm
In-Reply-To: <1281466850-5297-2-git-send-email-mathewm@codeaurora.org>
Hi Mat,
> This fixes a bug which caused the FCS setting to show L2CAP_FCS_CRC16
> with L2CAP modes other than ERTM or streaming. At present, this only
> affects the FCS value shown with getsockopt() for basic mode.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/l2cap.c | 19 +++++++++++++------
> 1 files changed, 13 insertions(+), 6 deletions(-)
patch has been applied, but please stop adding a . at the end of your
subject line ;)
Regards
Marcel
^ permalink raw reply
* Re: patch for firmware download to Qualcomm Bluetooth chip
From: Marcel Holtmann @ 2010-08-10 21:14 UTC (permalink / raw)
To: 1267142545.30787.6.camel; +Cc: linux-bluetooth, johan.hedberg
In-Reply-To: <4C619FA6.8030606@codeaurora.org>
Hi Ron,
> Here's the a ping regarding the patch submitted by Matt back in March? I
> believe. For you convenience I've included the patch again. Please take
> a look, and provide your general comments. I'm sure there are changes
> that need to be made.
>
> Fyi. sorry about the column wrap in the patch. Don't feel like fixing it
> for just a quick review.
>
> From 76d0bdd82a0a4e5b3b9544bb864c31888f20cea1 Mon Sep 17 00:00:00 2001
> From: Wilson, Matt <mtwilson@codeaurora.org>
> Date: Thu, 11 Feb 2010 11:53:29 -0600
> Subject: [PATCH] Firmware download for Qualcomm Bluetooth devices
>
> ---
> Makefile.tools | 3 +-
> tools/hciattach.c | 9 ++
> tools/hciattach.h | 4 +
> tools/hciattach_qualcomm.c | 279
> ++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 294 insertions(+), 1 deletions(-)
> create mode 100644 tools/hciattach_qualcomm.c
>
> diff --git a/Makefile.tools b/Makefile.tools
> index 2735d68..7b92c8f 100644
> --- a/Makefile.tools
> +++ b/Makefile.tools
> @@ -23,7 +23,8 @@ tools_l2ping_LDADD = lib/libbluetooth.la
> tools_hciattach_SOURCES = tools/hciattach.c tools/hciattach.h \
> tools/hciattach_st.c \
> tools/hciattach_ti.c \
> - tools/hciattach_tialt.c
> + tools/hciattach_tialt.c \
> + tools/hciattach_qualcomm.c
> tools_hciattach_LDADD = lib/libbluetooth.la
>
> tools_hciconfig_SOURCES = tools/hciconfig.c tools/csr.h tools/csr.c \
> diff --git a/tools/hciattach.c b/tools/hciattach.c
> index 364c5ff..d6aafbe 100644
> --- a/tools/hciattach.c
> +++ b/tools/hciattach.c
> @@ -5,6 +5,7 @@
> * Copyright (C) 2000-2001 Qualcomm Incorporated
> * Copyright (C) 2002-2003 Maxim Krasnyansky <maxk@qualcomm.com>
> * Copyright (C) 2002-2010 Marcel Holtmann <marcel@holtmann.org>
> + * Copyright (C) 2010, Code Aurora Forum. All rights reserved.
> *
> *
> * This program is free software; you can redistribute it and/or modify
> @@ -299,6 +300,11 @@ static int texasalt(int fd, struct uart_t *u, struct
> termios *ti)
> return texasalt_init(fd, u->speed, ti);
> }
>
> +static int qualcomm(int fd, struct uart_t *u, struct termios *ti)
> +{
> + return qualcomm_init(fd, u->speed, ti, u->bdaddr);
> +}
> +
> static int read_check(int fd, void *buf, int count)
> {
> int res;
> @@ -1071,6 +1077,9 @@ struct uart_t uart[] = {
> /* Broadcom BCM2035 */
> { "bcm2035", 0x0A5C, 0x2035, HCI_UART_H4, 115200, 460800, FLOW_CTL,
> NULL, bcm2035 },
>
> + /* QUALCOMM BTS */
> + { "qualcomm", 0x0000, 0x0000, HCI_UART_H4, 115200, 115200, FLOW_CTL,
> NULL, qualcomm },
> +
> { NULL, 0 }
> };
>
> diff --git a/tools/hciattach.h b/tools/hciattach.h
> index 867563b..5c89013 100644
> --- a/tools/hciattach.h
> +++ b/tools/hciattach.h
> @@ -3,6 +3,7 @@
> * BlueZ - Bluetooth protocol stack for Linux
> *
> * Copyright (C) 2003-2010 Marcel Holtmann <marcel@holtmann.org>
> + * Copyright (c) 2010, Code Aurora Forum. All rights reserved.
> *
> *
> * This program is free software; you can redistribute it and/or modify
> @@ -45,3 +46,6 @@ int texas_post(int fd, struct termios *ti);
> int texasalt_init(int fd, int speed, struct termios *ti);
> int stlc2500_init(int fd, bdaddr_t *bdaddr);
> int bgb2xx_init(int dd, bdaddr_t *bdaddr);
> +int qualcomm_init(int fd, int speed, struct termios *ti, const char
> *bdaddr);
> +
> +
I am actually fine with this. Except strip it from claiming copyright on
hciattach.c and hciattach.h since these changes are not really anything
new. They are just wrapping code.
So fix this and submit a new clean patch.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/1] Use HCI device type to gate BR/EDR-specific HCI commands
From: Johan Hedberg @ 2010-08-10 20:15 UTC (permalink / raw)
To: David Scherba; +Cc: linux-bluetooth, marcel, rshaffer
In-Reply-To: <1281469904-30408-1-git-send-email-dscherba@codeaurora.org>
Hi David,
On Tue, Aug 10, 2010, David Scherba wrote:
> Use the hci_dev_info structure member 'type' to classify whether a HCI device
> is BR/EDR, or not. If not, gate BR/EDR-specific HCI commands.
> ---
> plugins/hciops.c | 20 +++++++++++++-------
> src/adapter.c | 2 +-
> src/hcid.h | 8 ++++++++
> src/security.c | 4 ++--
> 4 files changed, 24 insertions(+), 10 deletions(-)
Thanks. This one is now also pushed upstream.
Johan
^ permalink raw reply
* [PATCH 1/1] Use HCI device type to gate BR/EDR-specific HCI commands
From: David Scherba @ 2010-08-10 19:51 UTC (permalink / raw)
To: linux-bluetooth; +Cc: johan.hedberg, marcel, rshaffer, David Scherba
In-Reply-To: <20100810191616.GA26091@jh-x301>
Use the hci_dev_info structure member 'type' to classify whether a HCI device
is BR/EDR, or not. If not, gate BR/EDR-specific HCI commands.
---
plugins/hciops.c | 20 +++++++++++++-------
src/adapter.c | 2 +-
src/hcid.h | 8 ++++++++
src/security.c | 4 ++--
4 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/plugins/hciops.c b/plugins/hciops.c
index 3e3e172..9c97c5a 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -85,7 +85,7 @@ static void device_devup_setup(int index)
if (hci_devinfo(index, &di) < 0)
return;
- if (hci_test_bit(HCI_RAW, &di.flags))
+ if (ignore_device(&di))
return;
dd = hci_open_dev(index);
@@ -122,6 +122,7 @@ static void device_devup_setup(int index)
static void init_device(int index)
{
struct hci_dev_req dr;
+ struct hci_dev_info di;
pid_t pid;
int dd, err;
@@ -159,12 +160,17 @@ static void init_device(int index)
index, strerror(err), err);
}
- /* Set link policy */
- dr.dev_opt = main_opts.link_policy;
- if (ioctl(dd, HCISETLINKPOL, (unsigned long) &dr) < 0 &&
+ /* Set link policy for BR/EDR HCI devices */
+ if (hci_devinfo(index, &di) < 0)
+ goto fail;
+
+ if (!ignore_device(&di)) {
+ dr.dev_opt = main_opts.link_policy;
+ if (ioctl(dd, HCISETLINKPOL, (unsigned long) &dr) < 0 &&
errno != ENETDOWN) {
- error("Can't set link policy on hci%d: %s (%d)",
- index, strerror(errno), errno);
+ error("Can't set link policy on hci%d: %s (%d)",
+ index, strerror(errno), errno);
+ }
}
/* Start HCI device */
@@ -196,7 +202,7 @@ static void device_devreg_setup(int index)
devup = hci_test_bit(HCI_UP, &di.flags);
- if (!hci_test_bit(HCI_RAW, &di.flags))
+ if (!ignore_device(&di))
manager_register_adapter(index, devup);
}
diff --git a/src/adapter.c b/src/adapter.c
index af44a59..fc1e123 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2300,7 +2300,7 @@ int adapter_start(struct btd_adapter *adapter)
if (hci_devinfo(adapter->dev_id, &di) < 0)
return -errno;
- if (hci_test_bit(HCI_RAW, &di.flags)) {
+ if (ignore_device(&di)) {
dev->ignore = 1;
return -1;
}
diff --git a/src/hcid.h b/src/hcid.h
index 9c5f1d6..50bdb65 100644
--- a/src/hcid.h
+++ b/src/hcid.h
@@ -23,6 +23,9 @@
*
*/
+#include <bluetooth/hci.h>
+#include <bluetooth/hci_lib.h>
+
/* When all services should trust a remote device */
#define GLOBAL_TRUST "[all]"
@@ -99,3 +102,8 @@ void rfkill_exit(void);
void __probe_servers(const char *adapter);
void __remove_servers(const char *adapter);
+
+static inline int ignore_device(struct hci_dev_info *di)
+{
+ return hci_test_bit(HCI_RAW, &di->flags) || di->type >> 4 != HCI_BREDR;
+}
diff --git a/src/security.c b/src/security.c
index ca394e1..667f1f1 100644
--- a/src/security.c
+++ b/src/security.c
@@ -999,7 +999,7 @@ static gboolean io_security_event(GIOChannel *chan, GIOCondition cond,
ioctl(dev, HCIGETDEVINFO, (void *) di);
- if (hci_test_bit(HCI_RAW, &di->flags))
+ if (ignore_device(di))
return TRUE;
switch (eh->evt) {
@@ -1185,7 +1185,7 @@ void start_security_manager(int hdev)
io_data[hdev].channel = chan;
io_data[hdev].pin_length = -1;
- if (hci_test_bit(HCI_RAW, &di->flags))
+ if (ignore_device(di))
return;
bacpy(&cp.bdaddr, BDADDR_ANY);
--
1.7.0.2
--
David Scherba
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply related
* Re: [PATCH 2/2] Use HCI device type to gate BR/EDR-specific HCI commands
From: David Scherba @ 2010-08-10 19:49 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth, marcel, rshaffer
In-Reply-To: <20100810191616.GA26091@jh-x301>
On 8/10/2010 2:16 PM, Johan Hedberg wrote:
>> +static inline int ignore_device(struct hci_dev_info *di)
>> +{
>> + return hci_test_bit(HCI_RAW,&di->flags) || di->type>> 4 != HCI_BREDR;
>> +}
>
> Since this is hci_lib.h (something I didn't realize in the previous
> review) the function should follow the same namespace as all other
> functions in the header file, i.e. hci_*. However, does the function
> really need to be exported in this public library header file, i.e.
> isn't there some private header file that could be used instead?
It looks like src/hcid.h fits the bill, although the modified hcid.h
will need to include the hci headers to have the appropriate defines...
Modified patch coming up.
Thanks,
David
--
David Scherba
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply
* Re: [PATCH 1/2] Remove non-functional hci_devinfo calls in init_device()
From: Johan Hedberg @ 2010-08-10 19:20 UTC (permalink / raw)
To: David Scherba; +Cc: linux-bluetooth, marcel, rshaffer
In-Reply-To: <1281453304-30408-2-git-send-email-dscherba@codeaurora.org>
Hi David,
On Tue, Aug 10, 2010, David Scherba wrote:
> ---
> plugins/hciops.c | 8 --------
> 1 files changed, 0 insertions(+), 8 deletions(-)
This patch is now upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH 2/2] Use HCI device type to gate BR/EDR-specific HCI commands
From: Johan Hedberg @ 2010-08-10 19:16 UTC (permalink / raw)
To: David Scherba; +Cc: linux-bluetooth, marcel, rshaffer
In-Reply-To: <1281455145-1777-1-git-send-email-dscherba@codeaurora.org>
Hi David,
On Tue, Aug 10, 2010, David Scherba wrote:
> --- a/lib/hci_lib.h
> +++ b/lib/hci_lib.h
> @@ -169,6 +169,11 @@ static inline int hci_test_bit(int nr, void *addr)
> return *((uint32_t *) addr + (nr >> 5)) & (1 << (nr & 31));
> }
>
> +static inline int ignore_device(struct hci_dev_info *di)
> +{
> + return hci_test_bit(HCI_RAW, &di->flags) || di->type >> 4 != HCI_BREDR;
> +}
Since this is hci_lib.h (something I didn't realize in the previous
review) the function should follow the same namespace as all other
functions in the header file, i.e. hci_*. However, does the function
really need to be exported in this public library header file, i.e.
isn't there some private header file that could be used instead?
Johan
^ permalink raw reply
* [RFC 7/7] Bluetooth: Do not limit enhanced L2CAP max PDU size to HCI MTU.
From: Mat Martineau @ 2010-08-10 19:15 UTC (permalink / raw)
To: linux-bluetooth; +Cc: marcel, gustavo, rshaffer, linux-arm-msm, Mat Martineau
In-Reply-To: <1281467704-5378-1-git-send-email-mathewm@codeaurora.org>
HCI MTU and PDU size have no relationship, since ERTM and streaming PDUs
can be fragmented over HCI just like other L2CAP frames. The only
applicable limit for PDU size is the L2CAP MTU, which is now checked
during L2CAP configuration.
Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
---
net/bluetooth/l2cap.c | 16 ++++++++--------
1 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 0212035..15fbb1d 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -2621,8 +2621,8 @@ done:
rfc.retrans_timeout = 0;
rfc.monitor_timeout = 0;
rfc.max_pdu_size = cpu_to_le16(L2CAP_DEFAULT_MAX_PDU_SIZE);
- if (L2CAP_DEFAULT_MAX_PDU_SIZE > pi->conn->mtu - 10)
- rfc.max_pdu_size = cpu_to_le16(pi->conn->mtu - 10);
+ if (L2CAP_DEFAULT_MAX_PDU_SIZE > pi->imtu)
+ rfc.max_pdu_size = cpu_to_le16(pi->imtu);
l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc),
(unsigned long) &rfc);
@@ -2644,8 +2644,8 @@ done:
rfc.retrans_timeout = 0;
rfc.monitor_timeout = 0;
rfc.max_pdu_size = cpu_to_le16(L2CAP_DEFAULT_MAX_PDU_SIZE);
- if (L2CAP_DEFAULT_MAX_PDU_SIZE > pi->conn->mtu - 10)
- rfc.max_pdu_size = cpu_to_le16(pi->conn->mtu - 10);
+ if (L2CAP_DEFAULT_MAX_PDU_SIZE > pi->imtu)
+ rfc.max_pdu_size = cpu_to_le16(pi->imtu);
l2cap_add_conf_opt(&ptr, L2CAP_CONF_RFC, sizeof(rfc),
(unsigned long) &rfc);
@@ -2778,8 +2778,8 @@ done:
pi->remote_tx_win = rfc.txwin_size;
pi->remote_max_tx = rfc.max_transmit;
- if (le16_to_cpu(rfc.max_pdu_size) > pi->conn->mtu - 10)
- rfc.max_pdu_size = cpu_to_le16(pi->conn->mtu - 10);
+ if (le16_to_cpu(rfc.max_pdu_size) > pi->omtu)
+ rfc.max_pdu_size = cpu_to_le16(pi->omtu);
pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
@@ -2796,8 +2796,8 @@ done:
break;
case L2CAP_MODE_STREAMING:
- if (le16_to_cpu(rfc.max_pdu_size) > pi->conn->mtu - 10)
- rfc.max_pdu_size = cpu_to_le16(pi->conn->mtu - 10);
+ if (le16_to_cpu(rfc.max_pdu_size) > pi->omtu)
+ rfc.max_pdu_size = cpu_to_le16(pi->omtu);
pi->remote_mps = le16_to_cpu(rfc.max_pdu_size);
--
1.7.1
--
Mat Martineau
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox