* Re: [RFC] Bluetooth: Allow getting SCO options for not connected sockets
From: Gustavo Padovan @ 2013-01-09 21:41 UTC (permalink / raw)
To: Vinicius Costa Gomes; +Cc: linux-bluetooth
In-Reply-To: <1357138055-28765-1-git-send-email-vinicius.gomes@openbossa.org>
Hi Vinicius,
* Vinicius Costa Gomes <vinicius.gomes@openbossa.org> [2013-01-02 11:47:35 -0300]:
> Now, that we have proper support for deferred setup for SCO sockets it
> is very convenient that we are able to get the socket options for
> sockets that are not in the 'connected' state.
>
> And makes the behaviour more consistent with what L2CAP does, for
> example.
>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@openbossa.org>
> ---
>
> Sending this as a RFC, because even though I couldn't think of any cases
> that this new behaviour would surprise userspace applications, there
> may be still be some.
>
> net/bluetooth/sco.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 531a93d..271320c 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -730,11 +730,6 @@ static int sco_sock_getsockopt_old(struct socket *sock, int optname, char __user
>
> switch (optname) {
> case SCO_OPTIONS:
> - if (sk->sk_state != BT_CONNECTED) {
> - err = -ENOTCONN;
> - break;
> - }
> -
> opts.mtu = sco_pi(sk)->conn->mtu;
>
> BT_DBG("mtu %d", opts.mtu);
I'm ok with this patch, if someone else has something against it, please tell
me.
Gustavo
^ permalink raw reply
* Re: [PATCH v2 2/2] Bluetooth: Fix stop discovery while in STARTING state
From: Gustavo Padovan @ 2013-01-09 21:37 UTC (permalink / raw)
To: Jaganath Kanakkassery; +Cc: linux-bluetooth
In-Reply-To: <D76CDD18C783483EAF643D1B392FFAEA@sisodomain.com>
Hi Jaganath,
* Jaganath Kanakkassery <jaganath.k@samsung.com> [2013-01-04 13:16:11 +0530]:
> Hi Gustavo,
>
> --------------------------------------------------
> From: "Gustavo Padovan" <gustavo@padovan.org>
> Sent: Friday, January 04, 2013 1:08 AM
> To: "Jaganath Kanakkassery" <jaganath.k@samsung.com>
> Cc: <linux-bluetooth@vger.kernel.org>
> Subject: Re: [PATCH v2 2/2] Bluetooth: Fix stop discovery while in
> STARTING state
>
> >Hi Jaganath,
> >
> >* Jaganath Kanakkassery <jaganath.k@samsung.com> [2012-12-21
> >18:20:25 +0530]:
> >
> >>If stop_discovery() is called when discovery state is STARTING, it
> >>will be failed currently. This patch fixes this.
> >>
> >>Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
> >>---
> >> include/net/bluetooth/hci_core.h | 2 ++
> >> net/bluetooth/hci_event.c | 14 ++++++++++++--
> >> net/bluetooth/mgmt.c | 31 ++++++++++++++++++++++++++++++-
> >> 3 files changed, 44 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/include/net/bluetooth/hci_core.h
> >>b/include/net/bluetooth/hci_core.h
> >>index 119fcb6..c2886b7 100644
> >>--- a/include/net/bluetooth/hci_core.h
> >>+++ b/include/net/bluetooth/hci_core.h
> >>@@ -64,6 +64,7 @@ struct discovery_state {
> >> DISCOVERY_RESOLVING,
> >> DISCOVERY_STOPPING,
> >> } state;
> >>+ u8 discovering;
> >> struct list_head all; /* All devices found during inquiry */
> >> struct list_head unknown; /* Name state not known */
> >> struct list_head resolve; /* Name needs to be resolved */
> >>@@ -1066,6 +1067,7 @@ int mgmt_device_found(struct hci_dev
> >>*hdev, bdaddr_t *bdaddr, u8 link_type,
> >> int mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8
> >>link_type,
> >> u8 addr_type, s8 rssi, u8 *name, u8 name_len);
> >> int mgmt_start_discovery_failed(struct hci_dev *hdev, u8 status);
> >>+int mgmt_start_discovery_complete(struct hci_dev *hdev, u8 status);
> >> int mgmt_stop_discovery_failed(struct hci_dev *hdev, u8 status);
> >> int mgmt_discovering(struct hci_dev *hdev, u8 discovering);
> >> int mgmt_interleaved_discovery(struct hci_dev *hdev);
> >>diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >>index e248e7c..b486458 100644
> >>--- a/net/bluetooth/hci_event.c
> >>+++ b/net/bluetooth/hci_event.c
> >>@@ -1092,7 +1092,12 @@ static void
> >>hci_cc_le_set_scan_enable(struct hci_dev *hdev,
> >> set_bit(HCI_LE_SCAN, &hdev->dev_flags);
> >>
> >> hci_dev_lock(hdev);
> >>- hci_discovery_set_state(hdev, DISCOVERY_FINDING);
> >>+ if (hdev->discovery.state == DISCOVERY_STOPPING) {
> >>+ hci_cancel_le_scan(hdev);
> >>+ mgmt_start_discovery_complete(hdev, 0);
> >
> >Reply to mgmt with an error here might be better.
>
> I think the best error which can be given here is
> MGMT_STATUS_CANCELLED. But this error is not accessible in hci_event.c
>
> >>+ } else {
> >>+ hci_discovery_set_state(hdev, DISCOVERY_FINDING);
> >>+ }
> >> hci_dev_unlock(hdev);
> >> break;
> >>
> >>@@ -1189,7 +1194,12 @@ static void hci_cs_inquiry(struct hci_dev
> >>*hdev, __u8 status)
> >> set_bit(HCI_INQUIRY, &hdev->flags);
> >>
> >> hci_dev_lock(hdev);
> >>- hci_discovery_set_state(hdev, DISCOVERY_FINDING);
> >>+ if (hdev->discovery.state == DISCOVERY_STOPPING) {
> >>+ hci_cancel_inquiry(hdev);
> >>+ mgmt_start_discovery_complete(hdev, 0);
> >>+ } else {
> >>+ hci_discovery_set_state(hdev, DISCOVERY_FINDING);
> >>+ }
> >> hci_dev_unlock(hdev);
> >> }
> >>
> >>diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> >>index d6c0d78..ba4171f 100644
> >>--- a/net/bluetooth/mgmt.c
> >>+++ b/net/bluetooth/mgmt.c
> >>@@ -2385,7 +2385,8 @@ static int stop_discovery(struct sock *sk,
> >>struct hci_dev *hdev, void *data,
> >>
> >> hci_dev_lock(hdev);
> >>
> >>- if (!hci_discovery_active(hdev)) {
> >>+ if (hdev->discovery.state != DISCOVERY_STARTING &&
> >>+ !hci_discovery_active(hdev)) {
> >> err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY,
> >> MGMT_STATUS_REJECTED, &mgmt_cp->type,
> >> sizeof(mgmt_cp->type));
> >>@@ -2433,6 +2434,10 @@ static int stop_discovery(struct sock
> >>*sk, struct hci_dev *hdev, void *data,
> >>
> >> break;
> >>
> >>+ case DISCOVERY_STARTING:
> >>+ err = 0;
> >>+ break;
> >>+
> >> default:
> >> BT_DBG("unknown discovery state %u", hdev->discovery.state);
> >> err = -EFAULT;
> >>@@ -3624,6 +3629,25 @@ int mgmt_start_discovery_failed(struct
> >>hci_dev *hdev, u8 status)
> >> return err;
> >> }
> >>
> >>+int mgmt_start_discovery_complete(struct hci_dev *hdev, u8 status)
> >>+{
> >>+ struct pending_cmd *cmd;
> >>+ u8 type;
> >>+ int err;
> >>+
> >>+ cmd = mgmt_pending_find(MGMT_OP_START_DISCOVERY, hdev);
> >>+ if (!cmd)
> >>+ return -ENOENT;
> >>+
> >>+ type = hdev->discovery.type;
> >>+
> >>+ err = cmd_complete(cmd->sk, hdev->id, cmd->opcode, mgmt_status(status),
> >>+ &type, sizeof(type));
> >>+ mgmt_pending_remove(cmd);
> >>+
> >>+ return err;
> >>+}
> >
> >This is exactly the same thing as mgmt_start_discovery_failed(),
> >just rename it
> >to _complete() as you did with mgmt_stop_discovery_failed(). Do it as a
> >separate patch.
>
> mgmt_start_discovery_failed() sets discovery state to STOPPED which
> also sends
> stop_discovery_complete internally. I think both are inappropriate
> at the point
> where mgmt_start_discovery_complete() is called.
>
> How abt renaming the new function mgmt_start_discovery_complete() to
> mgmt_start_discovery_cancelled and send MGMT_STATUS_CANCELLED in that?
Ok, go with this approach.
Gustavo
^ permalink raw reply
* Re: [PATCH v1] Bluetooth: Fix authentication if acl data comes before remote feature evt
From: Gustavo Padovan @ 2013-01-09 21:10 UTC (permalink / raw)
To: Jaganath Kanakkassery; +Cc: linux-bluetooth
In-Reply-To: <1357543751-12204-1-git-send-email-jaganath.k@samsung.com>
Hi Jaganath,
* Jaganath Kanakkassery <jaganath.k@samsung.com> [2013-01-07 12:59:11 +0530]:
> If remote device sends l2cap info request before read_remote_ext_feature
> completes then mgmt_connected will be sent in hci_acldata_packet() and
> remote name request wont be sent and eventually authentication wont happen
>
> Hcidump log of the issue
>
> < HCI Command: Create Connection (0x01|0x0005) plen 13
> bdaddr BC:85:1F:74:7F:29 ptype 0xcc18 rswitch 0x01 clkoffset 0x4bf7 (valid)
> Packet type: DM1 DM3 DM5 DH1 DH3 DH5
> > HCI Event: Command Status (0x0f) plen 4
> Create Connection (0x01|0x0005) status 0x00 ncmd 1
> > HCI Event: Connect Complete (0x03) plen 11
> status 0x00 handle 12 bdaddr BC:85:1F:74:7F:29 type ACL encrypt 0x00
> < HCI Command: Read Remote Supported Features (0x01|0x001b) plen 2
> handle 12
> > HCI Event: Command Status (0x0f) plen 4
> Read Remote Supported Features (0x01|0x001b) status 0x00 ncmd 1
> > HCI Event: Read Remote Supported Features (0x0b) plen 11
> status 0x00 handle 12
> Features: 0xbf 0xfe 0xcf 0xfe 0xdb 0xff 0x7b 0x87
> > HCI Event: Max Slots Change (0x1b) plen 3
> handle 12 slots 5
> < HCI Command: Read Remote Extended Features (0x01|0x001c) plen 3
> handle 12 page 1
> > HCI Event: Command Status (0x0f) plen 4
> Read Remote Extended Features (0x01|0x001c) status 0x00 ncmd 1
> > ACL data: handle 12 flags 0x02 dlen 10
> L2CAP(s): Info req: type 2
> < ACL data: handle 12 flags 0x00 dlen 16
> L2CAP(s): Info rsp: type 2 result 0
> Extended feature mask 0x00b8
> Enhanced Retransmission mode
> Streaming mode
> FCS Option
> Fixed Channels
> > HCI Event: Read Remote Extended Features (0x23) plen 13
> status 0x00 handle 12 page 1 max 1
> Features: 0x01 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> > ACL data: handle 12 flags 0x02 dlen 10
> L2CAP(s): Info req: type 3
> < ACL data: handle 12 flags 0x00 dlen 20
> L2CAP(s): Info rsp: type 3 result 0
> Fixed channel list 0x00000002
> L2CAP Signalling Channel
> > HCI Event: Number of Completed Packets (0x13) plen 5
> handle 12 packets 2
>
> This patch moves sending mgmt_connected from hci_acldata_packet() to
> l2cap_connect_req() since this code is to handle the scenario remote
> device sends l2cap connect req too fast
> ---
> v1 ---> Incorporated Johan's comments - Instead of fixing in hci_acldata_packet(),
> move sending mgmt_connected to l2cap_connect_req since this code is mainly to
> handle the scenario if remote device sends l2cap connection too fast
>
> net/bluetooth/hci_core.c | 8 --------
> net/bluetooth/l2cap_core.c | 11 +++++++++++
> 2 files changed, 11 insertions(+), 8 deletions(-)
Could you please add your Signed-off-by line to the patch?
Gustavo
^ permalink raw reply
* Re: [PATCH] Bluetooth: fix the oops due to conn->hcon == NULL in shutdown case
From: Gustavo Padovan @ 2013-01-09 20:34 UTC (permalink / raw)
To: Liu, Chuansheng
Cc: marcel@holtmann.org, johan.hedberg@gmail.com,
linux-bluetooth@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <27240C0AC20F114CBF8149A2696CBE4A1FB42B@SHSMSX101.ccr.corp.intel.com>
Hi Liu,
* Liu, Chuansheng <chuansheng.liu@intel.com> [2013-01-04 00:55:26 +0000]:
>
>
> > -----Original Message-----
> > From: Gustavo Padovan [mailto:gustavo@padovan.org]
> > Sent: Friday, January 04, 2013 6:03 AM
> > To: Liu, Chuansheng
> > Cc: marcel@holtmann.org; johan.hedberg@gmail.com;
> > linux-bluetooth@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH] Bluetooth: fix the oops due to conn->hcon == NULL in
> > shutdown case
> >
> > Hi Chuansheng,
> >
> > * Chuansheng Liu <chuansheng.liu@intel.com> [2012-12-25 18:04:17 +0800]:
> >
> > >
> > > Meet one panic issue as below stack:
>
>
> > > Disassemble the code:
> > > base address of __sco_sock_close is 0xc184f410
> > > 0xc184f4f8 <+232>: lock decl 0x8(%ebx) < == crash here, ebx is 0x0,
> > >
> > > the related source code is:
> > > (gdb) l *0xc184f4f8
> > > 0xc184f4f8 is in __sco_sock_close (arch/x86/include/asm/atomic.h:123)
> > > 119 static inline int atomic_dec_and_test(atomic_t *v)
> > > 123 asm volatile(LOCK_PREFIX "decl %0; sete %1"
> > >
> > > The whole call stack is:
> > > sys_shutdown()
> > > sco_sock_shutdown()
> > > __sco_sock_close()
> > > hci_conn_put()
> > > atomic_dec_and_test()
> > >
> > > Due to the conn->hcon is NULL, and the member hcon->refcnt is at offset 0x8,
> > > so "BUG: unable to handle kernel NULL pointer dereference at 00000008"
> > > appears.
> Could you add the above crash info to indicate where crashed? Thanks.
>
> > >
> > > Here fix it that adding the condition if conn->hcon is NULL, just like
> > > in sco_chan_del().
> > >
> > > Signed-off-by: liu chuansheng <chuansheng.liu@intel.com>
> > > ---
> > > net/bluetooth/sco.c | 6 ++++--
> > > 1 files changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> > > index 531a93d..190f70c 100644
> > > --- a/net/bluetooth/sco.c
> > > +++ b/net/bluetooth/sco.c
> > > @@ -355,8 +355,10 @@ static void __sco_sock_close(struct sock *sk)
> > > if (sco_pi(sk)->conn) {
> > > sk->sk_state = BT_DISCONN;
> > > sco_sock_set_timer(sk, SCO_DISCONN_TIMEOUT);
> > > - hci_conn_put(sco_pi(sk)->conn->hcon);
> > > - sco_pi(sk)->conn->hcon = NULL;
> > > + if (sco_pi(sk)->conn->hcon) {
> > > + hci_conn_put(sco_pi(sk)->conn->hcon);
> > > + sco_pi(sk)->conn->hcon = NULL;
> > > + }
> > > } else
> > > sco_chan_del(sk, ECONNRESET);
> > > break;
> >
> > Please check if the following patch fixes the issue for you:
> >
> > commit ae5668c1fc155d3034d0eedcdb52798390975a39 (HEAD, master)
> > Author: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > Date: Thu Jan 3 19:59:28 2013 -0200
> >
> > Bluetooth: Check if the hci connection exists in SCO shutdown
> >
> > Checking only for sco_conn seems to not be enough and lead to NULL
> > dereferences in the code, check for hcon instead.
> >
> > <1>[11340.226404] BUG: unable to handle kernel NULL pointer
> > dereference at
> > 0000000
> > 8
> > <4>[11340.226619] EIP is at __sco_sock_close+0xe8/0x1a0
> > <4>[11340.226629] EAX: f063a740 EBX: 00000000 ECX: f58f4544 EDX:
> > 00000000
> > <4>[11340.226640] ESI: dec83e00 EDI: 5f9a081f EBP: e0fdff38 ESP:
> > e0fdff1c
> > <0>[11340.226674] Stack:
> > <4>[11340.226682] c184db87 c1251028 dec83e00 e0fdff38 c1754aef
> > dec83e00
> > 00000000
> > e0fdff5c
> > <4>[11340.226718] c184f587 e0fdff64 e0fdff68 5f9a081f e0fdff5c
> > c1751852
> > d7813800
> > 62262f10
> > <4>[11340.226752] e0fdff70 c1753c00 00000000 00000001 0000000d
> > e0fdffac
> > c175425c
> > 00000041
> > <0>[11340.226793] Call Trace:
> > <4>[11340.226813] [<c184db87>] ? sco_sock_clear_timer+0x27/0x60
> > <4>[11340.226831] [<c1251028>] ? local_bh_enable+0x68/0xd0
> > <4>[11340.226846] [<c1754aef>] ? lock_sock_nested+0x4f/0x60
> > <4>[11340.226862] [<c184f587>] sco_sock_shutdown+0x67/0xb0
> > <4>[11340.226879] [<c1751852>] ? sockfd_lookup_light+0x22/0x80
> > <4>[11340.226897] [<c1753c00>] sys_shutdown+0x30/0x60
> > <4>[11340.226912] [<c175425c>] sys_socketcall+0x1dc/0x2a0
> > <4>[11340.226929] [<c149ba78>] ? trace_hardirqs_on_thunk+0xc/0x10
> > <4>[11340.226944] [<c18860f1>] syscall_call+0x7/0xb
> > <4>[11340.226960] [<c1880000>] ? restore_cur+0x5e/0xd7
> > <0>[11340.226969] Code: <f0> ff 4b 08 0f 94 c0 84 c0 74 20 80 7b 19 01 74
> > 2f b8 0a 00 00
> >
> > Reported-by: Chuansheng Liu <chuansheng.liu@intel.com>
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> >
> > diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> > index 531a93d..57f250c 100644
> > --- a/net/bluetooth/sco.c
> > +++ b/net/bluetooth/sco.c
> > @@ -352,7 +352,7 @@ static void __sco_sock_close(struct sock *sk)
> >
> > case BT_CONNECTED:
> > case BT_CONFIG:
> > - if (sco_pi(sk)->conn) {
> > + if (sco_pi(sk)->conn->hcon) {
> Your fix is incomplete, at least it should be:
> if ( (sco_pi(sk)->conn) && (sco_pi(sk)->conn->hcon)) {
> Otherwise, it will bring another crash case. So could you add signed-off-by me also?
Can you point any code flow that can crash with my patch? Otherwise I'm just
pushing this patch. I don't think we need to check for sco_pi(sk)->conn here.
Gustavo
^ permalink raw reply
* Re: [PATCH v2 BlueZ 3/3] unit: Implement tests for sdp_extract_attr()
From: Anderson Lizardo @ 2013-01-09 20:13 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1357761036.1806.48.camel@aeonflux>
Hi Marcel,
On Wed, Jan 9, 2013 at 3:50 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> just remove both TODOs from the patch. I assume you will send updates on
> this.
Yes, I will prepare a series with tests for other valid input.
Using this macro for testing sequences is not possible given it is
implemented as a linked list. SEQ*/ALT* stuff need to be tested
separately.
Testing invalid input (and checking for error) is also important,
specially if it triggers some memory leak/corruption. But they will
use a different test function because it should always return a NULL
pointer instead.
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH 7/8 v2] Bluetooth: Fix checking for exact values of boolean mgmt parameters
From: Marcel Holtmann @ 2013-01-09 20:13 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <1357740319-5737-1-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> All mgmt_set_* commands that take a boolean value encoded in the form of
> a byte should only accept the values 0x00 and 0x01. This patch adds the
> necessary checks for this and returns "invalid params" responses if
> anything else is provided as the value.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> v2: Fix s/SET_SSP/SET_LE/ copy-paste issue
>
> net/bluetooth/mgmt.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 6/8] Bluetooth: Move non-critical sections outside of the dev lock
From: Marcel Holtmann @ 2013-01-09 20:12 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <1357738180-4128-7-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> This patch fixes sections of code that do not need hci_lock_dev to be
> outside of the lock. Such sections include code that do not touch the
> hdev at all as well as sections which just read a single byte from the
> supported_features value (i.e. all lmp_*_capable() macros).
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 46 ++++++++++++++++++----------------------------
> 1 file changed, 18 insertions(+), 28 deletions(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 5/8] Bluetooth: Fix returning proper command status for start_discovery
From: Marcel Holtmann @ 2013-01-09 20:10 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <1357738180-4128-6-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> Management commands should whenever possible fail with proper command
> status or command complete events. This patch fixes the
> mgmt_start_discovery command to do this for the failure cases where an
> incorrect parameter value was passed to it ("not supported" if the
> parameter value was valid but the controller doesn't support it and
> "invalid params" if it isn't valid at all).
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 35 +++++++++++++++++++++++++----------
> 1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 6e6de9e..4f60540 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2377,31 +2377,46 @@ static int start_discovery(struct sock *sk, struct hci_dev *hdev,
>
> switch (hdev->discovery.type) {
> case DISCOV_TYPE_BREDR:
> - if (lmp_bredr_capable(hdev))
> + if (lmp_bredr_capable(hdev)) {
> err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
> - else
> - err = -ENOTSUPP;
> + } else {
> + err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
> + MGMT_STATUS_NOT_SUPPORTED);
> + mgmt_pending_remove(cmd);
> + goto failed;
> + }
I would have done this the other way around.
if (!lmp_bredr_capable(...)) {
...
got failed;
}
err = hci_do_inquiry(hdev, INQUIRY_LEN_BREDR);
break;
> break;
>
> case DISCOV_TYPE_LE:
> - if (lmp_host_le_capable(hdev))
> + if (lmp_host_le_capable(hdev)) {
> err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
> LE_SCAN_WIN, LE_SCAN_TIMEOUT_LE_ONLY);
> - else
> - err = -ENOTSUPP;
> + } else {
> + err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
> + MGMT_STATUS_NOT_SUPPORTED);
> + mgmt_pending_remove(cmd);
> + goto failed;
> + }
And same here.
> break;
>
> case DISCOV_TYPE_INTERLEAVED:
> - if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev))
> + if (lmp_host_le_capable(hdev) && lmp_bredr_capable(hdev)) {
> err = hci_le_scan(hdev, LE_SCAN_TYPE, LE_SCAN_INT,
> LE_SCAN_WIN,
> LE_SCAN_TIMEOUT_BREDR_LE);
> - else
> - err = -ENOTSUPP;
> + } else {
> + err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
> + MGMT_STATUS_NOT_SUPPORTED);
> + mgmt_pending_remove(cmd);
> + goto failed;
> + }
> break;
Also here.
>
> default:
> - err = -EINVAL;
> + err = cmd_status(sk, hdev->id, MGMT_OP_START_DISCOVERY,
> + MGMT_STATUS_INVALID_PARAMS);
> + mgmt_pending_remove(cmd);
> + goto failed;
> }
>
> if (err < 0)
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 4/8] Bluetooth: Fix accepting set_dev_class for non-BR/EDR controllers
From: Marcel Holtmann @ 2013-01-09 20:08 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <1357738180-4128-5-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> The concept of Class of Device only exists for BR/EDR controllers. The
> mgmt_set_dev_class command should therefore return a proper "not
> supported" error if it is attempted for a controller that doesn't
> support BR/EDR (e.g. a single mode LE-only one).
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 6 ++++++
> 1 file changed, 6 insertions(+)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 3/8] Bluetooth: Fix checking for valid device class values
From: Marcel Holtmann @ 2013-01-09 20:07 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <1357738180-4128-4-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> The two lowest bits of the minor device class value are reserved and
> should be zero, and the three highest bits of the major device class
> likewise. The management code should therefore test for this and return
> a proper "invalid params" error if the condition is not met.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index aaf9ce6..2785de2 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -1430,6 +1430,12 @@ static int set_dev_class(struct sock *sk, struct hci_dev *hdev, void *data,
> goto unlock;
> }
>
> + if ((cp->minor & 0x03) != 0 || (cp->major & 0xe0) != 0) {
> + err = cmd_status(sk, hdev->id, MGMT_OP_SET_DEV_CLASS,
> + MGMT_STATUS_INVALID_PARAMS);
> + goto unlock;
> + }
we could do
if ((cp->minor & 0x03) || ...) {
However I am not sure what is preferred and I am fine both ways.
> +
> hdev->major_class = cp->major;
> hdev->minor_class = cp->minor;
>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH BlueZ 1/3] unit: Reuse define_test() macro for /TP/SERVER/BRW/* tests
From: Anderson Lizardo @ 2013-01-09 20:06 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1357758529.1806.46.camel@aeonflux>
Hi Marcel,
On Wed, Jan 9, 2013 at 3:08 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> -#define define_test(name, args...) \
>> +#define define_test(name, _mtu, args...) \
>> do { \
>> const struct sdp_pdu pdus[] = { \
>> args, { }, { } \
>> }; \
>> static struct test_data data; \
>> - data.mtu = 48; \
>> + data.mtu = _mtu; \
>
> so using _mtu instead of mtu was the trick here. For the heck of it I
> could not figure that one out. So in the end I gave up on it and
> hardcoded it :(
Yes, I was caught by it as well, until I realized that the
preprocessor might be replacing the "mtu" part of "data.mtu" (I
confirmed when I saw the cpp output).
>
> Patch has been applied.
Thanks,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH 2/8] Bluetooth: Fix missing command complete for mgmt_load_long_term_keys
From: Marcel Holtmann @ 2013-01-09 20:04 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <1357738180-4128-3-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> All management events are expected to indicate successful completion
> through a command complete event, however the load long term keys
> command was missing this. This patch adds the missing event.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/8] Bluetooth: Fix missing command complete event for mgmt_confirm_name
From: Marcel Holtmann @ 2013-01-09 20:02 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <1357738180-4128-2-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> All management commands are expected to indicate successful completion
> through a command complete event however the confirm name command was
> missing it. This patch add the sending of the missing event.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/mgmt.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] Bluetooth device 04ca:3008 should use ath3k
From: Gustavo Padovan @ 2013-01-09 19:53 UTC (permalink / raw)
To: Sergio Cambra; +Cc: linux-bluetooth
In-Reply-To: <10056676.fQr0UMXTN5@tablet>
Hi Sergio,
* Sergio Cambra <sergio@enpijama.es> [2013-01-04 00:22:29 +0100]:
> Output of /sys/kernel/debug/usb/devices
> T: Bus=03 Lev=02 Prnt=02 Port=00 Cnt=01 Dev#= 6 Spd=12 MxCh= 0
> D: Ver= 1.10 Cls=e0(wlcon) Sub=01 Prot=01 MxPS=64 #Cfgs= 1
> P: Vendor=04ca ProdID=3008 Rev= 0.02
> C:* #Ifs= 2 Cfg#= 1 Atr=e0 MxPwr=100mA
> I:* If#= 0 Alt= 0 #EPs= 3 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
> E: Ad=81(I) Atr=03(Int.) MxPS= 16 Ivl=1ms
> E: Ad=82(I) Atr=02(Bulk) MxPS= 64 Ivl=0ms
> E: Ad=02(O) Atr=02(Bulk) MxPS= 64 Ivl=0ms
> I:* If#= 1 Alt= 0 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
> E: Ad=83(I) Atr=01(Isoc) MxPS= 0 Ivl=1ms
> E: Ad=03(O) Atr=01(Isoc) MxPS= 0 Ivl=1ms
> I: If#= 1 Alt= 1 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
> E: Ad=83(I) Atr=01(Isoc) MxPS= 9 Ivl=1ms
> E: Ad=03(O) Atr=01(Isoc) MxPS= 9 Ivl=1ms
> I: If#= 1 Alt= 2 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
> E: Ad=83(I) Atr=01(Isoc) MxPS= 17 Ivl=1ms
> E: Ad=03(O) Atr=01(Isoc) MxPS= 17 Ivl=1ms
> I: If#= 1 Alt= 3 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
> E: Ad=83(I) Atr=01(Isoc) MxPS= 25 Ivl=1ms
> E: Ad=03(O) Atr=01(Isoc) MxPS= 25 Ivl=1ms
> I: If#= 1 Alt= 4 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
> E: Ad=83(I) Atr=01(Isoc) MxPS= 33 Ivl=1ms
> E: Ad=03(O) Atr=01(Isoc) MxPS= 33 Ivl=1ms
> I: If#= 1 Alt= 5 #EPs= 2 Cls=e0(wlcon) Sub=01 Prot=01 Driver=btusb
> E: Ad=83(I) Atr=01(Isoc) MxPS= 49 Ivl=1ms
> E: Ad=03(O) Atr=01(Isoc) MxPS= 49 Ivl=1ms
>
> Signed-off-by: Sergio Cambra <sergio@programatica.es>
> ---
> drivers/bluetooth/ath3k.c | 2 ++
> drivers/bluetooth/btusb.c | 1 +
> 2 files changed, 3 insertions(+)
Your patch is broken, please use git send-email to send it to the mailing
list.
Gustavo
^ permalink raw reply
* Re: [PATCH v2 BlueZ 3/3] unit: Implement tests for sdp_extract_attr()
From: Marcel Holtmann @ 2013-01-09 19:50 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <1357744762-5111-3-git-send-email-anderson.lizardo@openbossa.org>
Hi Anderson,
> These tests are for valid data. Other tests might be added later to
> check for error paths, but will require separate macros.
>
> As example of failure output, if commit
> 504a0cf46ad89cab8005ce9cffb22e41048f6a30 is reverted for testing, the
> STR16 test will fail with:
>
> ERROR:unit/test-sdp.c:776:test_sdp_de_attr: assertion failed
> (test->input_size == size): (7 == 11)
> ---
> unit/test-sdp.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 65 insertions(+)
>
> diff --git a/unit/test-sdp.c b/unit/test-sdp.c
> index 77a4c6c..d511ef4 100644
> --- a/unit/test-sdp.c
> +++ b/unit/test-sdp.c
> @@ -85,6 +85,29 @@ struct test_data {
> #define define_ssa(name, args...) define_test("/TP/SERVER/SSA/" name, 48, args)
> #define define_brw(name, args...) define_test("/TP/SERVER/BRW/" name, 672, args)
>
> +/* SDP Data Element (DE) tests */
> +struct test_data_de {
> + const void *input_data;
> + size_t input_size;
> + sdp_data_t expected;
> +};
> +
> +#define exp_data(_dtd, val_type, val_data) \
> + ((const sdp_data_t) { \
> + .dtd = _dtd, \
> + .val.val_type = val_data, \
> + })
> +
> +#define define_test_de_attr(name, input, exp) \
> + do { \
> + static struct test_data_de data; \
> + data.input_data = input; \
> + data.input_size = sizeof(input); \
> + data.expected = exp; \
> + g_test_add_data_func("/sdp/DE/ATTR/" name, &data, \
> + test_sdp_de_attr); \
> + } while (0)
> +
> struct context {
> GMainLoop *main_loop;
> guint server_source;
> @@ -742,6 +765,35 @@ static void test_sdp(gconstpointer data)
> g_free(test->pdu_list);
> }
>
> +static void test_sdp_de_attr(gconstpointer data)
> +{
> + const struct test_data_de *test = data;
> + sdp_data_t *d;
> + int size = 0;
> +
> + d = sdp_extract_attr(test->input_data, test->input_size, &size, NULL);
> + g_assert(d != NULL);
> + g_assert_cmpuint(test->input_size, ==, size);
> + g_assert_cmpuint(test->expected.dtd, ==, d->dtd);
> +
> + if (g_test_verbose() == TRUE)
> + g_print("DTD=0x%02x\n", d->dtd);
> +
> + switch (d->dtd) {
> + case SDP_TEXT_STR8:
> + case SDP_TEXT_STR16:
> + case SDP_URL_STR8:
> + case SDP_URL_STR16:
> + g_assert_cmpstr(test->expected.val.str, ==, d->val.str);
> + break;
> + /* TODO: implement other DTDs here */
just remove both TODOs from the patch. I assume you will send updates on
this.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH] Bluetooth: Fix incorrect strncpy() in hidp_setup_hid()
From: Gustavo Padovan @ 2013-01-09 19:40 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <1357511333-5276-1-git-send-email-anderson.lizardo@openbossa.org>
Hi Anderson,
* Anderson Lizardo <anderson.lizardo@openbossa.org> [2013-01-06 18:28:53 -0400]:
> The length parameter should be sizeof(req->name) - 1 because there is no
> guarantee that string provided by userspace will contain the trailing
> '\0'.
>
> Can be easily reproduced by manually setting req->name to 128 non-zero
> bytes prior to ioctl(HIDPCONNADD) and checking the device name setup on
> input subsystem:
>
> $ cat /sys/devices/pnp0/00\:04/tty/ttyS0/hci0/hci0\:1/input8/name
> AAAAAA[...]AAAAAAAAf0:af:f0:af:f0:af
>
> ("f0:af:f0:af:f0:af" is the device bluetooth address, taken from "phys"
> field in struct hid_device due to overflow.)
>
> Signed-off-by: Anderson Lizardo <anderson.lizardo@openbossa.org>
> ---
> net/bluetooth/hidp/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Patch has been applied to bluetooth.git. Thanks. I'm also sending it to
stable.
Gustavo
^ permalink raw reply
* usb device removed from sysfs before input children devices
From: Karl Relton @ 2013-01-09 19:27 UTC (permalink / raw)
To: linux-usb, linux-bluetooth
On coming out of suspend my usb bluetooth adaptor is being reset by the
system.
In linux 3.7 the usb devices are being removed from the sysfs tree
first, and then the various 'child' devices (like my bluetooth mouse &
keyboard related devices) afterwards. This is causing the udev events
for the input devices to have 'orphaned' sysfs paths in the udev events.
This in turn means the Xorg evdev driver does not recognise the events,
and so doesn't see the removal of the input devices.
This has been picked by some downstream distributions, e.g. see this
thread by Google Chrome developers:
http://code.google.com/p/chromium-os/issues/detail?id=33813
Back on linux 3.2 this was not the case. The usb adaptor was reset, but
device removal was orderly: first the input devices (will full paths in
the udev events), then the usb devices walking up the tree.
To illustrate the issue, here is the output of 'udevadm monitor' in 3.7:
udevadm monitor
monitor will print the received events for:
KERNEL - the kernel uevent
KERNEL[2203.173080] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0/rfkill2 (rfkill)
KERNEL[2203.173148] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0 (bluetooth)
KERNEL[2203.173420] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0 (usb)
KERNEL[2203.173451] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.1 (usb)
KERNEL[2203.173475] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.2 (usb)
KERNEL[2203.173693] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2 (usb)
KERNEL[2213.152339] remove /hci0/hci0:46/input14/mouse2 (input)
KERNEL[2213.160374] remove /hci0/hci0:46/input14/event10 (input)
KERNEL[2213.168366] remove /hci0/hci0:46/input14 (input)
KERNEL[2213.169058] remove /hci0/hci0:46/0005:050D:0031.0005/hidraw/hidraw0 (hidraw)
KERNEL[2213.169198] remove /hci0/hci0:46/0005:050D:0031.0005 (hid)
KERNEL[2213.169242] remove /hci0/hci0:46 (bluetooth)
KERNEL[2218.176527] remove /hci0/hci0:49/input13/event11 (input)
KERNEL[2218.180403] remove /hci0/hci0:49/input13 (input)
KERNEL[2218.180481] remove /hci0/hci0:49/0005:05AC:0256.0004/hidraw/hidraw1 (hidraw)
KERNEL[2218.180538] remove /hci0/hci0:49/0005:05AC:0256.0004 (hid)
KERNEL[2218.182005] remove /hci0/hci0:49 (bluetooth)
See how the usb devices are moved first, and then the input/bluetooth related stuff
with path-heads removed (paths are now /hci0/... instead of /devices/...)
Here is the equiv sequence back in 3.2:
KERNEL[158.378301] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0/hci0:49/input11/mouse2 (input)
KERNEL[158.388283] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0/hci0:49/input11/event11 (input)
KERNEL[158.409885] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0/hci0:49/input11 (input)
KERNEL[158.411565] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0/hci0:49/0005:050D:0031.0002/hidraw/hidraw1 (hidraw)
KERNEL[158.411598] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0/hci0:49/0005:050D:0031.0002 (hid)
KERNEL[158.411621] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0/hci0:49 (bluetooth)
KERNEL[158.436894] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0/hci0:46/input10/event10 (input)
KERNEL[158.452211] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0/hci0:46/input10 (input)
KERNEL[158.452628] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0/hci0:46/0005:05AC:0256.0001/hidraw/hidraw0 (hidraw)
KERNEL[158.452662] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0/hci0:46/0005:05AC:0256.0001 (hid)
KERNEL[158.452752] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0/hci0:46 (bluetooth)
KERNEL[158.629847] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0/rfkill2 (rfkill)
KERNEL[158.629920] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0/bluetooth/hci0 (bluetooth)
KERNEL[158.635562] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.0 (usb)
KERNEL[158.635701] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.1 (usb)
KERNEL[158.635807] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2/3-2:1.2 (usb)
KERNEL[158.637238] remove /devices/pci0000:00/0000:00:1a.0/usb3/3-2 (usb)
The end result (for the user) is that even when the bluetooth
mouse/keyboard is re-added, Xorg ignores it - thinking it is some hoax
duplicate device. The keyboard/mouse is then non-operational.
Karl
^ permalink raw reply
* Re: [PATCH BlueZ 2/3] unit: Rename x_pdu() macro on SDP test program
From: Marcel Holtmann @ 2013-01-09 19:10 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <1357744762-5111-2-git-send-email-anderson.lizardo@openbossa.org>
Hi Anderson,
> Using the "raw_data" name makes sense, given the macro is just casting
> input (raw) data. It will also be reused in other tests with raw input
> data.
>
> Also fix this minor checkpatch.pl error:
>
> ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in
> parenthesis
> +#define raw_data(args...) (const unsigned char[]) { args }
> ---
> unit/test-sdp.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
patch has been applied.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH BlueZ 1/3] unit: Reuse define_test() macro for /TP/SERVER/BRW/* tests
From: Marcel Holtmann @ 2013-01-09 19:08 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <1357744762-5111-1-git-send-email-anderson.lizardo@openbossa.org>
Hi Anderson,
> This is made possible by adding the mtu parameter, given
> /TP/SERVER/BRW/* tests use MTU of 672.
> ---
> unit/test-sdp.c | 24 ++++++------------------
> 1 file changed, 6 insertions(+), 18 deletions(-)
>
> diff --git a/unit/test-sdp.c b/unit/test-sdp.c
> index 315a5cd..e9cbcdf 100644
> --- a/unit/test-sdp.c
> +++ b/unit/test-sdp.c
> @@ -68,34 +68,22 @@ struct test_data {
> .cont_len = cont, \
> }
>
> -#define define_test(name, args...) \
> +#define define_test(name, _mtu, args...) \
> do { \
> const struct sdp_pdu pdus[] = { \
> args, { }, { } \
> }; \
> static struct test_data data; \
> - data.mtu = 48; \
> + data.mtu = _mtu; \
so using _mtu instead of mtu was the trick here. For the heck of it I
could not figure that one out. So in the end I gave up on it and
hardcoded it :(
Patch has been applied.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v1] Bluetooth: Fix authentication if acl data comes before remote feature evt
From: Mikel Astiz @ 2013-01-09 16:27 UTC (permalink / raw)
To: Jaganath Kanakkassery; +Cc: linux-bluetooth
In-Reply-To: <1357543751-12204-1-git-send-email-jaganath.k@samsung.com>
Hi Jaganath,
On Mon, Jan 7, 2013 at 8:29 AM, Jaganath Kanakkassery
<jaganath.k@samsung.com> wrote:
> If remote device sends l2cap info request before read_remote_ext_feature
> completes then mgmt_connected will be sent in hci_acldata_packet() and
> remote name request wont be sent and eventually authentication wont happen
>
> Hcidump log of the issue
>
> < HCI Command: Create Connection (0x01|0x0005) plen 13
> bdaddr BC:85:1F:74:7F:29 ptype 0xcc18 rswitch 0x01 clkoffset 0x4bf7 (valid)
> Packet type: DM1 DM3 DM5 DH1 DH3 DH5
>> HCI Event: Command Status (0x0f) plen 4
> Create Connection (0x01|0x0005) status 0x00 ncmd 1
>> HCI Event: Connect Complete (0x03) plen 11
> status 0x00 handle 12 bdaddr BC:85:1F:74:7F:29 type ACL encrypt 0x00
> < HCI Command: Read Remote Supported Features (0x01|0x001b) plen 2
> handle 12
>> HCI Event: Command Status (0x0f) plen 4
> Read Remote Supported Features (0x01|0x001b) status 0x00 ncmd 1
>> HCI Event: Read Remote Supported Features (0x0b) plen 11
> status 0x00 handle 12
> Features: 0xbf 0xfe 0xcf 0xfe 0xdb 0xff 0x7b 0x87
>> HCI Event: Max Slots Change (0x1b) plen 3
> handle 12 slots 5
> < HCI Command: Read Remote Extended Features (0x01|0x001c) plen 3
> handle 12 page 1
>> HCI Event: Command Status (0x0f) plen 4
> Read Remote Extended Features (0x01|0x001c) status 0x00 ncmd 1
>> ACL data: handle 12 flags 0x02 dlen 10
> L2CAP(s): Info req: type 2
> < ACL data: handle 12 flags 0x00 dlen 16
> L2CAP(s): Info rsp: type 2 result 0
> Extended feature mask 0x00b8
> Enhanced Retransmission mode
> Streaming mode
> FCS Option
> Fixed Channels
>> HCI Event: Read Remote Extended Features (0x23) plen 13
> status 0x00 handle 12 page 1 max 1
> Features: 0x01 0x00 0x00 0x00 0x00 0x00 0x00 0x00
>> ACL data: handle 12 flags 0x02 dlen 10
> L2CAP(s): Info req: type 3
> < ACL data: handle 12 flags 0x00 dlen 20
> L2CAP(s): Info rsp: type 3 result 0
> Fixed channel list 0x00000002
> L2CAP Signalling Channel
>> HCI Event: Number of Completed Packets (0x13) plen 5
> handle 12 packets 2
>
> This patch moves sending mgmt_connected from hci_acldata_packet() to
> l2cap_connect_req() since this code is to handle the scenario remote
> device sends l2cap connect req too fast
> ---
> v1 ---> Incorporated Johan's comments - Instead of fixing in hci_acldata_packet(),
> move sending mgmt_connected to l2cap_connect_req since this code is mainly to
> handle the scenario if remote device sends l2cap connection too fast
>
> net/bluetooth/hci_core.c | 8 --------
> net/bluetooth/l2cap_core.c | 11 +++++++++++
> 2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 596660d..0f78e34 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2810,14 +2810,6 @@ static void hci_acldata_packet(struct hci_dev *hdev, struct sk_buff *skb)
> if (conn) {
> hci_conn_enter_active_mode(conn, BT_POWER_FORCE_ACTIVE_OFF);
>
> - hci_dev_lock(hdev);
> - if (test_bit(HCI_MGMT, &hdev->dev_flags) &&
> - !test_and_set_bit(HCI_CONN_MGMT_CONNECTED, &conn->flags))
> - mgmt_device_connected(hdev, &conn->dst, conn->type,
> - conn->dst_type, 0, NULL, 0,
> - conn->dev_class);
> - hci_dev_unlock(hdev);
> -
> /* Send to upper protocol */
> l2cap_recv_acldata(conn, skb, flags);
> return;
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 82a3bdc..7c7e932 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -3722,6 +3722,17 @@ sendresp:
> static int l2cap_connect_req(struct l2cap_conn *conn,
> struct l2cap_cmd_hdr *cmd, u8 *data)
> {
> + struct hci_dev *hdev = conn->hcon->hdev;
> + struct hci_conn *hcon = conn->hcon;
> +
> + hci_dev_lock(hdev);
> + if (test_bit(HCI_MGMT, &hdev->dev_flags) &&
> + !test_and_set_bit(HCI_CONN_MGMT_CONNECTED, &hcon->flags))
> + mgmt_device_connected(hdev, &hcon->dst, hcon->type,
> + hcon->dst_type, 0, NULL, 0,
> + hcon->dev_class);
> + hci_dev_unlock(hdev);
> +
> l2cap_connect(conn, cmd, data, L2CAP_CONN_RSP, 0);
> return 0;
> }
I've tested this patch and it seems to solve another issue: during an
incoming pairing procedure with a phone, the name of the device could
be left unresolved.
The issue was 100% reproducible in my setup with certain phones, for
example iPhone 5, and it gets fixed with this patch.
Cheers,
Mikel
^ permalink raw reply
* [PATCH v2 BlueZ 3/3] unit: Implement tests for sdp_extract_attr()
From: Anderson Lizardo @ 2013-01-09 15:19 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
In-Reply-To: <1357744762-5111-1-git-send-email-anderson.lizardo@openbossa.org>
These tests are for valid data. Other tests might be added later to
check for error paths, but will require separate macros.
As example of failure output, if commit
504a0cf46ad89cab8005ce9cffb22e41048f6a30 is reverted for testing, the
STR16 test will fail with:
ERROR:unit/test-sdp.c:776:test_sdp_de_attr: assertion failed
(test->input_size == size): (7 == 11)
---
unit/test-sdp.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 65 insertions(+)
diff --git a/unit/test-sdp.c b/unit/test-sdp.c
index 77a4c6c..d511ef4 100644
--- a/unit/test-sdp.c
+++ b/unit/test-sdp.c
@@ -85,6 +85,29 @@ struct test_data {
#define define_ssa(name, args...) define_test("/TP/SERVER/SSA/" name, 48, args)
#define define_brw(name, args...) define_test("/TP/SERVER/BRW/" name, 672, args)
+/* SDP Data Element (DE) tests */
+struct test_data_de {
+ const void *input_data;
+ size_t input_size;
+ sdp_data_t expected;
+};
+
+#define exp_data(_dtd, val_type, val_data) \
+ ((const sdp_data_t) { \
+ .dtd = _dtd, \
+ .val.val_type = val_data, \
+ })
+
+#define define_test_de_attr(name, input, exp) \
+ do { \
+ static struct test_data_de data; \
+ data.input_data = input; \
+ data.input_size = sizeof(input); \
+ data.expected = exp; \
+ g_test_add_data_func("/sdp/DE/ATTR/" name, &data, \
+ test_sdp_de_attr); \
+ } while (0)
+
struct context {
GMainLoop *main_loop;
guint server_source;
@@ -742,6 +765,35 @@ static void test_sdp(gconstpointer data)
g_free(test->pdu_list);
}
+static void test_sdp_de_attr(gconstpointer data)
+{
+ const struct test_data_de *test = data;
+ sdp_data_t *d;
+ int size = 0;
+
+ d = sdp_extract_attr(test->input_data, test->input_size, &size, NULL);
+ g_assert(d != NULL);
+ g_assert_cmpuint(test->input_size, ==, size);
+ g_assert_cmpuint(test->expected.dtd, ==, d->dtd);
+
+ if (g_test_verbose() == TRUE)
+ g_print("DTD=0x%02x\n", d->dtd);
+
+ switch (d->dtd) {
+ case SDP_TEXT_STR8:
+ case SDP_TEXT_STR16:
+ case SDP_URL_STR8:
+ case SDP_URL_STR16:
+ g_assert_cmpstr(test->expected.val.str, ==, d->val.str);
+ break;
+ /* TODO: implement other DTDs here */
+ default:
+ g_assert_not_reached();
+ }
+
+ sdp_data_free(d);
+}
+
int main(int argc, char *argv[])
{
g_test_init(&argc, &argv, NULL);
@@ -2697,5 +2749,18 @@ int main(int argc, char *argv[])
0x08, 0x09, 0x00, 0x01, 0x35, 0x03, 0x19, 0x11,
0x06, 0x00));
+ /*
+ * SDP Data Element (DE) tests
+ *
+ * Test extraction of valid DEs supported by sdp_extract_attr().
+ */
+ define_test_de_attr("STR8",
+ raw_data(0x25, 0x04, 0x41, 0x42, 0x43, 0x44),
+ exp_data(SDP_TEXT_STR8, str, "ABCD"));
+ define_test_de_attr("STR16",
+ raw_data(0x26, 0x00, 0x04, 0x41, 0x42, 0x43, 0x44),
+ exp_data(SDP_TEXT_STR16, str, "ABCD"));
+ /* TODO: implement other tests for valid DEs */
+
return g_test_run();
}
--
1.7.9.5
^ permalink raw reply related
* [PATCH BlueZ 2/3] unit: Rename x_pdu() macro on SDP test program
From: Anderson Lizardo @ 2013-01-09 15:19 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
In-Reply-To: <1357744762-5111-1-git-send-email-anderson.lizardo@openbossa.org>
Using the "raw_data" name makes sense, given the macro is just casting
input (raw) data. It will also be reused in other tests with raw input
data.
Also fix this minor checkpatch.pl error:
ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in
parenthesis
+#define raw_data(args...) (const unsigned char[]) { args }
---
unit/test-sdp.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/unit/test-sdp.c b/unit/test-sdp.c
index e9cbcdf..77a4c6c 100644
--- a/unit/test-sdp.c
+++ b/unit/test-sdp.c
@@ -51,20 +51,20 @@ struct test_data {
struct sdp_pdu *pdu_list;
};
-#define x_pdu(args...) (const unsigned char[]) { args }
+#define raw_data(args...) ((const unsigned char[]) { args })
#define raw_pdu(args...) \
{ \
.valid = true, \
- .raw_data = x_pdu(args), \
- .raw_size = sizeof(x_pdu(args)), \
+ .raw_data = raw_data(args), \
+ .raw_size = sizeof(raw_data(args)), \
}
#define raw_pdu_cont(cont, args...) \
{ \
.valid = true, \
- .raw_data = x_pdu(args), \
- .raw_size = sizeof(x_pdu(args)), \
+ .raw_data = raw_data(args), \
+ .raw_size = sizeof(raw_data(args)), \
.cont_len = cont, \
}
--
1.7.9.5
^ permalink raw reply related
* [PATCH BlueZ 1/3] unit: Reuse define_test() macro for /TP/SERVER/BRW/* tests
From: Anderson Lizardo @ 2013-01-09 15:19 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
This is made possible by adding the mtu parameter, given
/TP/SERVER/BRW/* tests use MTU of 672.
---
unit/test-sdp.c | 24 ++++++------------------
1 file changed, 6 insertions(+), 18 deletions(-)
diff --git a/unit/test-sdp.c b/unit/test-sdp.c
index 315a5cd..e9cbcdf 100644
--- a/unit/test-sdp.c
+++ b/unit/test-sdp.c
@@ -68,34 +68,22 @@ struct test_data {
.cont_len = cont, \
}
-#define define_test(name, args...) \
+#define define_test(name, _mtu, args...) \
do { \
const struct sdp_pdu pdus[] = { \
args, { }, { } \
}; \
static struct test_data data; \
- data.mtu = 48; \
+ data.mtu = _mtu; \
data.pdu_list = g_malloc(sizeof(pdus)); \
memcpy(data.pdu_list, pdus, sizeof(pdus)); \
g_test_add_data_func(name, &data, test_sdp); \
} while (0)
-#define define_ss(name, args...) define_test("/TP/SERVER/SS/" name, args)
-#define define_sa(name, args...) define_test("/TP/SERVER/SA/" name, args)
-#define define_ssa(name, args...) define_test("/TP/SERVER/SSA/" name, args)
-
-#define define_brw(name, args...) \
- do { \
- const struct sdp_pdu pdus[] = { \
- args, { }, { } \
- }; \
- static struct test_data data; \
- data.mtu = 672; \
- data.pdu_list = g_malloc(sizeof(pdus)); \
- memcpy(data.pdu_list, pdus, sizeof(pdus)); \
- g_test_add_data_func("/TP/SERVER/BRW/" name, \
- &data, test_sdp); \
- } while (0)
+#define define_ss(name, args...) define_test("/TP/SERVER/SS/" name, 48, args)
+#define define_sa(name, args...) define_test("/TP/SERVER/SA/" name, 48, args)
+#define define_ssa(name, args...) define_test("/TP/SERVER/SSA/" name, 48, args)
+#define define_brw(name, args...) define_test("/TP/SERVER/BRW/" name, 672, args)
struct context {
GMainLoop *main_loop;
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH BlueZ 1/2] unit: Add initial tests for sdp_extract_attr()
From: Anderson Lizardo @ 2013-01-09 15:14 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1357685187.1806.40.camel@aeonflux>
Hi Marcel,
On Tue, Jan 8, 2013 at 6:46 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> A better test case data could look like this:
>
> struct test_data {
> const void *input_data;
> size_t input_size;
> uint8_t dtd;
> };
>
> And we can add combinations of structs and unions here to represent the
> different testes.
>
> Also I would create a nice helper define like I did with define_ssa()
> for example to create the test data and the test with a nice name.
I just sent a v2 of the SDP DE tests based on your suggestions (the 2
other patches are just refactoring of existing code).
Let me know if this new format is fine, so I can proceed with adding
other tests.
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* [PATCH 7/8 v2] Bluetooth: Fix checking for exact values of boolean mgmt parameters
From: Johan Hedberg @ 2013-01-09 14:05 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1357738180-4128-8-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
All mgmt_set_* commands that take a boolean value encoded in the form of
a byte should only accept the values 0x00 and 0x01. This patch adds the
necessary checks for this and returns "invalid params" responses if
anything else is provided as the value.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
v2: Fix s/SET_SSP/SET_LE/ copy-paste issue
net/bluetooth/mgmt.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 69221ce..3cf7e1d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -777,6 +777,10 @@ static int set_powered(struct sock *sk, struct hci_dev *hdev, void *data,
BT_DBG("request for %s", hdev->name);
+ if (cp->val != 0x00 && cp->val != 0x01)
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_POWERED,
+ MGMT_STATUS_INVALID_PARAMS);
+
hci_dev_lock(hdev);
if (test_and_clear_bit(HCI_AUTO_OFF, &hdev->dev_flags)) {
@@ -872,6 +876,10 @@ static int set_discoverable(struct sock *sk, struct hci_dev *hdev, void *data,
return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
MGMT_STATUS_NOT_SUPPORTED);
+ if (cp->val != 0x00 && cp->val != 0x01)
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
+ MGMT_STATUS_INVALID_PARAMS);
+
timeout = __le16_to_cpu(cp->timeout);
if (!cp->val && timeout > 0)
return cmd_status(sk, hdev->id, MGMT_OP_SET_DISCOVERABLE,
@@ -971,6 +979,10 @@ static int set_connectable(struct sock *sk, struct hci_dev *hdev, void *data,
return cmd_status(sk, hdev->id, MGMT_OP_SET_CONNECTABLE,
MGMT_STATUS_NOT_SUPPORTED);
+ if (cp->val != 0x00 && cp->val != 0x01)
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_CONNECTABLE,
+ MGMT_STATUS_INVALID_PARAMS);
+
hci_dev_lock(hdev);
if (!hdev_is_powered(hdev)) {
@@ -1041,6 +1053,10 @@ static int set_pairable(struct sock *sk, struct hci_dev *hdev, void *data,
BT_DBG("request for %s", hdev->name);
+ if (cp->val != 0x00 && cp->val != 0x01)
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_PAIRABLE,
+ MGMT_STATUS_INVALID_PARAMS);
+
hci_dev_lock(hdev);
if (cp->val)
@@ -1073,6 +1089,10 @@ static int set_link_security(struct sock *sk, struct hci_dev *hdev, void *data,
return cmd_status(sk, hdev->id, MGMT_OP_SET_LINK_SECURITY,
MGMT_STATUS_NOT_SUPPORTED);
+ if (cp->val != 0x00 && cp->val != 0x01)
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_LINK_SECURITY,
+ MGMT_STATUS_INVALID_PARAMS);
+
hci_dev_lock(hdev);
if (!hdev_is_powered(hdev)) {
@@ -1137,6 +1157,10 @@ static int set_ssp(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
return cmd_status(sk, hdev->id, MGMT_OP_SET_SSP,
MGMT_STATUS_NOT_SUPPORTED);
+ if (cp->val != 0x00 && cp->val != 0x01)
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_SSP,
+ MGMT_STATUS_INVALID_PARAMS);
+
hci_dev_lock(hdev);
val = !!cp->val;
@@ -1197,6 +1221,10 @@ static int set_hs(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
return cmd_status(sk, hdev->id, MGMT_OP_SET_HS,
MGMT_STATUS_NOT_SUPPORTED);
+ if (cp->val != 0x00 && cp->val != 0x01)
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_HS,
+ MGMT_STATUS_INVALID_PARAMS);
+
if (cp->val)
set_bit(HCI_HS_ENABLED, &hdev->dev_flags);
else
@@ -1219,6 +1247,10 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
return cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
MGMT_STATUS_NOT_SUPPORTED);
+ if (cp->val != 0x00 && cp->val != 0x01)
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
+ MGMT_STATUS_INVALID_PARAMS);
+
hci_dev_lock(hdev);
val = !!cp->val;
@@ -2630,6 +2662,10 @@ static int set_fast_connectable(struct sock *sk, struct hci_dev *hdev,
return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
MGMT_STATUS_NOT_SUPPORTED);
+ if (cp->val != 0x00 && cp->val != 0x01)
+ return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
+ MGMT_STATUS_INVALID_PARAMS);
+
if (!hdev_is_powered(hdev))
return cmd_status(sk, hdev->id, MGMT_OP_SET_FAST_CONNECTABLE,
MGMT_STATUS_NOT_POWERED);
--
1.7.10.4
^ 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