* [RFC BlueZ 2/2] AVDTP: Fix passing uninitialized err to ind->open
From: Luiz Augusto von Dentz @ 2013-02-19 14:33 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1361284436-4584-1-git-send-email-luiz.dentz@gmail.com>
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
profiles/audio/avdtp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/profiles/audio/avdtp.c b/profiles/audio/avdtp.c
index 403a22b..0205644 100644
--- a/profiles/audio/avdtp.c
+++ b/profiles/audio/avdtp.c
@@ -1706,7 +1706,7 @@ static gboolean avdtp_open_cmd(struct avdtp *session, uint8_t transaction,
stream = sep->stream;
if (sep->ind && sep->ind->open) {
- if (!sep->ind->open(session, sep, stream, &err,
+ if (!sep->ind->open(session, sep, stream, NULL,
sep->user_data))
goto failed;
}
--
1.8.1.2
^ permalink raw reply related
* [RFC BlueZ 1/2] A2DP: Mark start flag if resume happen while in configured state
From: Luiz Augusto von Dentz @ 2013-02-19 14:33 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
If SEP is in configured state that means OPEN is about to happen so just
mark start flag and send START once OPEN completes.
---
profiles/audio/a2dp.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/profiles/audio/a2dp.c b/profiles/audio/a2dp.c
index f1646ee..d9680a7 100644
--- a/profiles/audio/a2dp.c
+++ b/profiles/audio/a2dp.c
@@ -336,6 +336,20 @@ static struct a2dp_setup *find_setup_by_dev(struct audio_device *dev)
return NULL;
}
+static struct a2dp_setup *find_setup_by_stream(struct avdtp_stream *stream)
+{
+ GSList *l;
+
+ for (l = setups; l != NULL; l = l->next) {
+ struct a2dp_setup *setup = l->data;
+
+ if (setup->stream == stream)
+ return setup;
+ }
+
+ return NULL;
+}
+
static void stream_state_changed(struct avdtp_stream *stream,
avdtp_state_t old_state,
avdtp_state_t new_state,
@@ -344,6 +358,29 @@ static void stream_state_changed(struct avdtp_stream *stream,
{
struct a2dp_sep *sep = user_data;
+ if (new_state == AVDTP_STATE_OPEN) {
+ struct a2dp_setup *setup;
+ int err;
+
+ setup = find_setup_by_stream(stream);
+ if (!setup || !setup->start)
+ return;
+
+ setup->start = FALSE;
+
+ err = avdtp_start(setup->session, stream);
+ if (err < 0 && err != -EINPROGRESS) {
+ error("avdtp_start: %s (%d)", strerror(-err), -err);
+ finalize_setup_errno(setup, err, finalize_resume,
+ NULL);
+ return;
+ }
+
+ sep->starting = TRUE;
+
+ return;
+ }
+
if (new_state != AVDTP_STATE_IDLE)
return;
@@ -661,6 +698,12 @@ static gboolean open_ind(struct avdtp *session, struct avdtp_local_sep *sep,
finalize_config(setup);
+ if (!setup->start || !err)
+ return TRUE;
+
+ setup->start = FALSE;
+ finalize_resume(setup);
+
return TRUE;
}
@@ -689,6 +732,14 @@ static void open_cfm(struct avdtp *session, struct avdtp_local_sep *sep,
}
finalize_config(setup);
+
+ if (!setup->start || !err)
+ return;
+
+ setup->start = FALSE;
+ finalize_resume(setup);
+
+ return;
}
static gboolean suspend_timeout(struct a2dp_sep *sep)
@@ -1718,6 +1769,9 @@ unsigned int a2dp_resume(struct avdtp *session, struct a2dp_sep *sep,
case AVDTP_STATE_IDLE:
goto failed;
break;
+ case AVDTP_STATE_CONFIGURED:
+ setup->start = TRUE;
+ break;
case AVDTP_STATE_OPEN:
if (avdtp_start(session, sep->stream) < 0) {
error("avdtp_start failed");
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH 1/2] tools: Fix compilation error with GINT_TO_POINTER
From: Syam Sidhardhan @ 2013-02-19 9:30 UTC (permalink / raw)
To: Lucas De Marchi, linux-bluetooth
In-Reply-To: <CAMOw1v4sYhps4ABc5Z059j9hxPQjQHSAfb3PJfVr53q+OPQH=A@mail.gmail.com>
Hi Johan,
-----Original Message-----
From: Lucas De Marchi
Sent: Monday, February 18, 2013 10:14 PM
To: Syam Sidhardhan ; linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH 1/2] tools: Fix compilation error with GINT_TO_POINTER
On Mon, Feb 18, 2013 at 1:13 PM, Johan Hedberg <johan.hedberg@gmail.com>
wrote:
> Hi Syam,
>
> On Mon, Feb 18, 2013, Syam Sidhardhan wrote:
>> Fixes the following error:
>> tools/btmgmt.c: In function ‘index_rsp’:
>> tools/btmgmt.c:756:10: error: cast to pointer from integer of different
>> size [-Werror=int-to-pointer-cast]
>> tools/btmgmt.c: In function ‘cmd_info’:
>> tools/btmgmt.c:791:9: error: cast to pointer from integer of different
>> size [-Werror=int-to-pointer-cast]
>> cc1: all warnings being treated as errors
>> make[1]: *** [tools/btmgmt.o] Error 1
>> make: *** [all] Error 2
>> ---
>> tools/btmgmt.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> This patch has been applied. For whatever reason I did not get this
> warning/error in my environment. Which gcc version and architecture are
> you using?
The problem is more likely to be in the different glib versions and
running on 32 bits:
https://bugzilla.gnome.org/show_bug.cgi?id=661546
I found this in gcc version 4.6.3 for PC arch(i386 on ubuntu 12.04) with
glib version 2.28.0.
Regards,
Syam
^ permalink raw reply
* Re: [PATCH v3 5/5] Bluetooth: Fallback transparent SCO from T2 to T1
From: Marcel Holtmann @ 2013-02-18 23:22 UTC (permalink / raw)
To: Frédéric Dalleau; +Cc: linux-bluetooth
In-Reply-To: <1359986188-24432-5-git-send-email-frederic.dalleau@linux.intel.com>
Hi Fred,
> When initiating a transparent eSCO connection, make use of T2 settings at
> first try. T2 is the recommended settings from HFP 1.6 WideBand Speech. Upon
> connection failure, try T1 settings.
> To know which of T2 or T1 should be used, the connection attempt index is used.
> T2 failure is detected if Synchronous Connection Complete Event fails with
> error 0x0d. This error code has been found experimentally by sending a T2
> request to a T1 only SCO listener. It means "Connection Rejected due to
> Limited resource".
>
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
> net/bluetooth/hci_conn.c | 11 ++++++++++-
> net/bluetooth/hci_event.c | 1 +
> 2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 504367e..a3f4e29 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -179,12 +179,21 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
> cp.tx_bandwidth = __constant_cpu_to_le32(0x00001f40);
> cp.rx_bandwidth = __constant_cpu_to_le32(0x00001f40);
>
> - if (test_and_clear_bit(HCI_CONN_SCO_TRANSPARENT, &conn->flags)) {
> + if (conn->attempt == 1 &&
> + test_bit(HCI_CONN_SCO_TRANSPARENT, &conn->flags)) {
> cp.pkt_type = __constant_cpu_to_le16(EDR_ESCO_MASK &
> ~ESCO_2EV3);
> cp.max_latency = __constant_cpu_to_le16(0x000d);
> cp.voice_setting = __constant_cpu_to_le16(0x0003);
> cp.retrans_effort = 0x02;
> + } else if (conn->attempt == 2 &&
> + test_bit(HCI_CONN_SCO_TRANSPARENT, &conn->flags)) {
I do not really like abusing a simple counter as a state machine. We
should do something more explicit with tracking if T2 vs T1 sco_flags or
something.
> + cp.pkt_type = __constant_cpu_to_le16(EDR_ESCO_MASK |
> + ESCO_EV3);
> + cp.max_latency = __constant_cpu_to_le16(0x0007);
> + cp.voice_setting = __constant_cpu_to_le16(0x0003);
> + cp.retrans_effort = 0x02;
> + clear_bit(HCI_CONN_SCO_TRANSPARENT, &conn->flags);
If we clear this here, does it mean we end up creating a CVSD link. That
can not be correct. I think we need to fail the connection procedure in
that case. We can not magically turn a transparent link into a CVSD
link. That would confuse the hell out of HFP.
> } else {
> cp.pkt_type = cpu_to_le16(conn->pkt_type);
> cp.max_latency = __constant_cpu_to_le16(0xffff);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 3fd0212..6c1aec9 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3330,6 +3330,7 @@ static void hci_sync_conn_complete_evt(struct hci_dev *hdev,
> hci_conn_add_sysfs(conn);
> break;
>
> + case 0x0d: /* No resource available */
> case 0x11: /* Unsupported Feature or Parameter Value */
> case 0x1c: /* SCO interval rejected */
> case 0x1a: /* Unsupported Remote Feature */
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v3 4/5] Bluetooth: Parameters for outgoing SCO connections
From: Marcel Holtmann @ 2013-02-18 23:18 UTC (permalink / raw)
To: Frédéric Dalleau; +Cc: linux-bluetooth
In-Reply-To: <1359986188-24432-4-git-send-email-frederic.dalleau@linux.intel.com>
Hi Fred,
> In order to establish a transparent SCO connection, the correct settings must
> be specified in the SetupSynchronousConnection request. For that, a bit is set
> in ACL connection flags to set up the desired parameters. If this bit is not
> set, a legacy SCO connection will be requested.
> This patch uses T2 settings.
>
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
> include/net/bluetooth/hci_core.h | 3 +++
> net/bluetooth/hci_conn.c | 29 +++++++++++++++++++----------
> net/bluetooth/sco.c | 3 +--
> 3 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index f3be454..4a216e5 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -443,6 +443,7 @@ enum {
> HCI_CONN_SSP_ENABLED,
> HCI_CONN_POWER_SAVE,
> HCI_CONN_REMOTE_OOB,
> + HCI_CONN_SCO_TRANSPARENT,
why are we tracking it like this and not actually the voice setting
itself?
> };
>
> static inline bool hci_conn_ssp_enabled(struct hci_conn *conn)
> @@ -590,6 +591,8 @@ struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle);
>
> struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
> __u8 dst_type, __u8 sec_level, __u8 auth_type);
> +struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
> + u8 mode);
> int hci_conn_check_link_mode(struct hci_conn *conn);
> int hci_conn_check_secure(struct hci_conn *conn, __u8 sec_level);
> int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type);
> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> index 25bfce0..504367e 100644
> --- a/net/bluetooth/hci_conn.c
> +++ b/net/bluetooth/hci_conn.c
> @@ -175,13 +175,22 @@ void hci_setup_sync(struct hci_conn *conn, __u16 handle)
> conn->attempt++;
>
> cp.handle = cpu_to_le16(handle);
> - cp.pkt_type = cpu_to_le16(conn->pkt_type);
>
> cp.tx_bandwidth = __constant_cpu_to_le32(0x00001f40);
> cp.rx_bandwidth = __constant_cpu_to_le32(0x00001f40);
> - cp.max_latency = __constant_cpu_to_le16(0xffff);
> - cp.voice_setting = cpu_to_le16(hdev->voice_setting);
> - cp.retrans_effort = 0xff;
> +
> + if (test_and_clear_bit(HCI_CONN_SCO_TRANSPARENT, &conn->flags)) {
> + cp.pkt_type = __constant_cpu_to_le16(EDR_ESCO_MASK &
> + ~ESCO_2EV3);
> + cp.max_latency = __constant_cpu_to_le16(0x000d);
> + cp.voice_setting = __constant_cpu_to_le16(0x0003);
> + cp.retrans_effort = 0x02;
> + } else {
> + cp.pkt_type = cpu_to_le16(conn->pkt_type);
> + cp.max_latency = __constant_cpu_to_le16(0xffff);
> + cp.voice_setting = cpu_to_le16(hdev->voice_setting);
> + cp.retrans_effort = 0xff;
> + }
>
> hci_send_cmd(hdev, HCI_OP_SETUP_SYNC_CONN, sizeof(cp), &cp);
> }
> @@ -551,13 +560,13 @@ static struct hci_conn *hci_connect_acl(struct hci_dev *hdev, bdaddr_t *dst,
> return acl;
> }
>
> -static struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type,
> - bdaddr_t *dst, u8 sec_level, u8 auth_type)
> +struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type, bdaddr_t *dst,
> + u8 mode)
> {
> struct hci_conn *acl;
> struct hci_conn *sco;
>
> - acl = hci_connect_acl(hdev, dst, sec_level, auth_type);
> + acl = hci_connect_acl(hdev, dst, BT_SECURITY_LOW, HCI_AT_NO_BONDING);
I do not understand this change. Why are you doing this one?
> if (IS_ERR(acl))
> return acl;
>
> @@ -575,6 +584,9 @@ static struct hci_conn *hci_connect_sco(struct hci_dev *hdev, int type,
>
> hci_conn_hold(sco);
>
> + if (mode)
> + set_bit(HCI_CONN_SCO_TRANSPARENT, &sco->flags);
> +
> if (acl->state == BT_CONNECTED &&
> (sco->state == BT_OPEN || sco->state == BT_CLOSED)) {
> set_bit(HCI_CONN_POWER_SAVE, &acl->flags);
> @@ -603,9 +615,6 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst,
> return hci_connect_le(hdev, dst, dst_type, sec_level, auth_type);
> case ACL_LINK:
> return hci_connect_acl(hdev, dst, sec_level, auth_type);
> - case SCO_LINK:
> - case ESCO_LINK:
> - return hci_connect_sco(hdev, type, dst, sec_level, auth_type);
> }
>
> return ERR_PTR(-EINVAL);
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index 7fbb4c4..1d79a2e 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -176,8 +176,7 @@ static int sco_connect(struct sock *sk)
> else
> type = SCO_LINK;
>
> - hcon = hci_connect(hdev, type, dst, BDADDR_BREDR, BT_SECURITY_LOW,
> - HCI_AT_NO_BONDING);
> + hcon = hci_connect_sco(hdev, type, dst, sco_pi(sk)->mode);
> if (IS_ERR(hcon)) {
> err = PTR_ERR(hcon);
> goto done;
If we really split hci_connect into two entities, then we should have a
preparation patch and then end up with hci_connect_acl and
hci_connect_sco.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v3 2/5] Bluetooth: Add option for SCO socket mode
From: Marcel Holtmann @ 2013-02-18 23:14 UTC (permalink / raw)
To: Frédéric Dalleau; +Cc: linux-bluetooth
In-Reply-To: <1359986188-24432-2-git-send-email-frederic.dalleau@linux.intel.com>
Hi Fred,
> This patch extends the current SCO socket option to add a 'mode' field. This
> field is intended to choose data type at runtime. Current modes are CVSD and
> transparent SCO, but adding new modes could allow support for CSA2 and fine
> tuning a sco connection, for example latency, bandwith, voice setting. Incoming
> connections will be setup during defered setup. Outgoing connections have to
> be setup before connect(). The selected type is stored in the sco socket info.
> This patch declares needed members, modifies getsockopt() and implements
> setsockopt(). Setting the mtu is not supported.
>
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
> include/net/bluetooth/sco.h | 7 +++++
> net/bluetooth/sco.c | 59 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 66 insertions(+)
>
> diff --git a/include/net/bluetooth/sco.h b/include/net/bluetooth/sco.h
> index 1e35c43..4de67ef 100644
> --- a/include/net/bluetooth/sco.h
> +++ b/include/net/bluetooth/sco.h
> @@ -41,8 +41,14 @@ struct sockaddr_sco {
>
> /* SCO socket options */
> #define SCO_OPTIONS 0x01
> +
> +#define SCO_MODE_CVSD 0x00
> +#define SCO_MODE_TRANSPARENT 0x01
> +#define SCO_MODE_MAX 0x01
> +
> struct sco_options {
> __u16 mtu;
> + __u8 mode;
> };
>
> #define SCO_CONNINFO 0x02
> @@ -73,6 +79,7 @@ struct sco_conn {
> struct sco_pinfo {
> struct bt_sock bt;
> __u32 flags;
> + __u8 mode;
> struct sco_conn *conn;
> };
so I have been reading the 4.0 core spec and CSA2 again. So with CSA2
and the enhanced eSCO setup they are splitting the PCM data format from
the the content format and make them independent in output and input.
While 4.0 core spec is essentially based around the 2-byte voice
setting.
I think we have two choices here, one is to just ignore CSA2 for now and
add a uint16 for voice setting value here, or go ahead with the full
blown crazy that is CSA2.
Splitting out voice setting into 5 byte content format and 1 byte PCM
data format seems straight forward, but then you have input and output
and also the routing path. And instead of a socket option, that looks
more like things for the socket address.
So I would actually propose to add a voice settings uint16 value as SCO
socket field. Using the full voice settings make sense since then we can
also retrieve the fact if we are doing 8-bit or 16-bit PCM. One detail
we kinda ignored since we always defaulted to 16-bit, but would be worth
while fixing while at it.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v3 1/5] Bluetooth: Move and rename hci_conn_accept
From: Marcel Holtmann @ 2013-02-18 22:58 UTC (permalink / raw)
To: Frédéric Dalleau; +Cc: linux-bluetooth
In-Reply-To: <1359986188-24432-1-git-send-email-frederic.dalleau@linux.intel.com>
Hi Fred,
> Since this function is only used by sco, move it from hci_event.c to
> sco.c and rename to sco_conn_defer_accept.
>
> Signed-off-by: Frédéric Dalleau <frederic.dalleau@linux.intel.com>
> ---
> include/net/bluetooth/hci_core.h | 1 -
> net/bluetooth/hci_event.c | 36 ------------------------------------
> net/bluetooth/sco.c | 38 +++++++++++++++++++++++++++++++++++++-
> 3 files changed, 37 insertions(+), 38 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 90cf75a..f3be454 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -582,7 +582,6 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst);
> int hci_conn_del(struct hci_conn *conn);
> void hci_conn_hash_flush(struct hci_dev *hdev);
> void hci_conn_check_pending(struct hci_dev *hdev);
> -void hci_conn_accept(struct hci_conn *conn, int mask);
>
> struct hci_chan *hci_chan_create(struct hci_conn *conn);
> void hci_chan_del(struct hci_chan *chan);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index d4fcba6..3fd0212 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2095,42 +2095,6 @@ unlock:
> hci_conn_check_pending(hdev);
> }
>
> -void hci_conn_accept(struct hci_conn *conn, int mask)
> -{
> - struct hci_dev *hdev = conn->hdev;
> -
> - BT_DBG("conn %p", conn);
> -
> - conn->state = BT_CONFIG;
> -
> - if (!lmp_esco_capable(hdev)) {
> - struct hci_cp_accept_conn_req cp;
> -
> - bacpy(&cp.bdaddr, &conn->dst);
> -
> - if (lmp_rswitch_capable(hdev) && (mask & HCI_LM_MASTER))
> - cp.role = 0x00; /* Become master */
> - else
> - cp.role = 0x01; /* Remain slave */
> -
> - hci_send_cmd(hdev, HCI_OP_ACCEPT_CONN_REQ, sizeof(cp), &cp);
> - } else /* lmp_esco_capable(hdev)) */ {
> - struct hci_cp_accept_sync_conn_req cp;
> -
> - bacpy(&cp.bdaddr, &conn->dst);
> - cp.pkt_type = cpu_to_le16(conn->pkt_type);
> -
> - cp.tx_bandwidth = __constant_cpu_to_le32(0x00001f40);
> - cp.rx_bandwidth = __constant_cpu_to_le32(0x00001f40);
> - cp.max_latency = __constant_cpu_to_le16(0xffff);
> - cp.content_format = cpu_to_le16(hdev->voice_setting);
> - cp.retrans_effort = 0xff;
> -
> - hci_send_cmd(hdev, HCI_OP_ACCEPT_SYNC_CONN_REQ,
> - sizeof(cp), &cp);
> - }
> -}
> -
> static void hci_conn_request_evt(struct hci_dev *hdev, struct sk_buff *skb)
> {
> struct hci_ev_conn_request *ev = (void *) skb->data;
> diff --git a/net/bluetooth/sco.c b/net/bluetooth/sco.c
> index e435641..97177be6 100644
> --- a/net/bluetooth/sco.c
> +++ b/net/bluetooth/sco.c
> @@ -654,6 +654,42 @@ static int sco_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
> return err;
> }
>
> +static void sco_conn_defer_accept(struct hci_conn *conn, int mask)
> +{
> + struct hci_dev *hdev = conn->hdev;
> +
> + BT_DBG("conn %p", conn);
> +
> + conn->state = BT_CONFIG;
> +
> + if (!lmp_esco_capable(hdev)) {
> + struct hci_cp_accept_conn_req cp;
> +
> + bacpy(&cp.bdaddr, &conn->dst);
> +
> + if (lmp_rswitch_capable(hdev) && (mask & HCI_LM_MASTER))
> + cp.role = 0x00; /* Become master */
> + else
> + cp.role = 0x01; /* Remain slave */
> +
> + hci_send_cmd(hdev, HCI_OP_ACCEPT_CONN_REQ, sizeof(cp), &cp);
> + } else /* lmp_esco_capable(hdev)) */ {
I realize that you copied this, but just remove this pointless comment.
The if statement is less than 10 lines above, so no big deal.
> + struct hci_cp_accept_sync_conn_req cp;
> +
> + bacpy(&cp.bdaddr, &conn->dst);
> + cp.pkt_type = cpu_to_le16(conn->pkt_type);
> +
> + cp.tx_bandwidth = __constant_cpu_to_le32(0x00001f40);
> + cp.rx_bandwidth = __constant_cpu_to_le32(0x00001f40);
> + cp.max_latency = __constant_cpu_to_le16(0xffff);
> + cp.content_format = cpu_to_le16(hdev->voice_setting);
> + cp.retrans_effort = 0xff;
> +
> + hci_send_cmd(hdev, HCI_OP_ACCEPT_SYNC_CONN_REQ,
> + sizeof(cp), &cp);
> + }
> +}
> +
> static int sco_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
> struct msghdr *msg, size_t len, int flags)
> {
> @@ -664,7 +700,7 @@ static int sco_sock_recvmsg(struct kiocb *iocb, struct socket *sock,
>
> if (sk->sk_state == BT_CONNECT2 &&
> test_bit(BT_SK_DEFER_SETUP, &bt_sk(sk)->flags)) {
> - hci_conn_accept(pi->conn->hcon, 0);
> + sco_conn_defer_accept(pi->conn->hcon, 0);
> sk->sk_state = BT_CONFIG;
>
> release_sock(sk);
Regards
Marcel
^ permalink raw reply
* Re: [RFC v2 3/8] Bluetooth: LE connection state machine
From: Marcel Holtmann @ 2013-02-18 22:50 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <CACJA=fVRr3NSdRD9rx5DN8dW6n6w=UdWADGVOdQOp2TLvRFTZQ@mail.gmail.com>
Hi Andre,
> >> This patch implements a state machine for carrying out the general
> >> connection establishment procedure described in Core spec.
> >>
> >> The state machine should be used as follows: when the kernel
> >> receives a new LE connection attempt, it should go to HCI_CONN_LE_
> >> SCAN state, starting the passive LE scanning. Once the target remote
> >> device is in-range, it should go to HCI_CONN_LE_FOUND, stopping the
> >> ongoing LE scanning. After the LE scanning is disabled, it should go
> >> to HCI_CONN_LE_INITIATE state where the LE connection is created.
> >>
> >> This state machine will be used by the LE connection routine in
> >> order to establish LE connections.
> >>
> >> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> >> ---
> >> include/net/bluetooth/hci_core.h | 13 +++++++++++++
> >> net/bluetooth/hci_conn.c | 26 ++++++++++++++++++++++++++
> >> 2 files changed, 39 insertions(+)
> >>
> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >> index 48c28ca..c704737 100644
> >> --- a/include/net/bluetooth/hci_core.h
> >> +++ b/include/net/bluetooth/hci_core.h
> >> @@ -300,6 +300,16 @@ struct hci_dev {
> >>
> >> #define HCI_PHY_HANDLE(handle) (handle & 0xff)
> >>
> >> +/*
> >> + * States from LE connection establishment state machine.
> >> + * State 0 is reserved and indicates the state machine is not running.
> >> + */
> >> +enum {
> >> + HCI_CONN_LE_SCAN = 1,
> >> + HCI_CONN_LE_FOUND,
> >> + HCI_CONN_LE_INITIATE,
> >> +};
> >> +
> >
> > and why don't we have HCI_CONN_LE_IDLE = 0 then. I do not get this magic
> > number handling when you introduce an enum anyway. You spent more time
> > explaining it with a comment instead of just using an extra enum entry.
>
> Ok, I'll introduce the HCI_CONN_LE_IDLE state.
>
> >> struct hci_conn {
> >> struct list_head list;
> >>
> >> @@ -356,6 +366,8 @@ struct hci_conn {
> >>
> >> struct hci_conn *link;
> >>
> >> + atomic_t le_state;
> >> +
> >
> > Why is this atomic_t. I am lost on what you are trying to solve here.
> > This requires a detailed explanation or you just get rid of it and use
> > the enum.
>
> le_state was defined as atomic_t so read and write are atomic
> operations, avoiding to acquire hdev->lock every time we want to read
> or write this variable.
that seems pretty much short sighted. Use a proper look and try not to
mix an atomic in here.
> >> void (*connect_cfm_cb) (struct hci_conn *conn, u8 status);
> >> void (*security_cfm_cb) (struct hci_conn *conn, u8 status);
> >> void (*disconn_cfm_cb) (struct hci_conn *conn, u8 reason);
> >> @@ -599,6 +611,7 @@ int hci_conn_check_secure(struct hci_conn *conn, __u8 sec_level);
> >> int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type);
> >> int hci_conn_change_link_key(struct hci_conn *conn);
> >> int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
> >> +void hci_conn_set_le_state(struct hci_conn *conn, int state);
> >
> > External state setting. That does not look like a good idea in the first
> > place. Why do you want to do it like this. Shouldn't be this self
> > contained.
>
> We need to set le_state variable in hci_event.c (see patch 5/8). That
> is why this function was added to hci_core.h.
That is not a good enough explanation. Doing it like this opens a can of
worms. I do not not get why we can not just trigger the next operation
from within the event callbacks.
You need to explain the point of all these indirection. Who is going to
read or even understand that in 12 month. I have a hard time to follow
by just reviewing these patches. That does not sound like a good
approach for re-designing the LE connection handling.
> >> void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
> >>
> >> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> >> index 25bfce0..d54c2a0 100644
> >> --- a/net/bluetooth/hci_conn.c
> >> +++ b/net/bluetooth/hci_conn.c
> >> @@ -391,6 +391,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
> >> (unsigned long) conn);
> >>
> >> atomic_set(&conn->refcnt, 0);
> >> + atomic_set(&conn->le_state, 0);
> >>
> >> hci_dev_hold(hdev);
> >>
> >> @@ -1027,3 +1028,28 @@ struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle)
> >>
> >> return hchan;
> >> }
> >> +
> >> +void hci_conn_set_le_state(struct hci_conn *conn, int state)
> >> +{
> >> + struct hci_dev *hdev = conn->hdev;
> >> +
> >> + BT_DBG("conn %p state %d -> %d", conn, atomic_read(&conn->le_state),
> >> + state);
> >> +
> >> + if (state == atomic_read(&conn->le_state))
> >> + return;
> >> +
> >> + switch (state) {
> >> + case HCI_CONN_LE_SCAN:
> >> + hci_le_scan(hdev, LE_SCAN_PASSIVE, 0x60, 0x30, 0);
> >> + break;
> >> + case HCI_CONN_LE_FOUND:
> >> + hci_cancel_le_scan(hdev);
> >> + break;
> >> + case HCI_CONN_LE_INITIATE:
> >> + hci_le_create_connection(conn);
> >> + break;
> >> + }
> >> +
> >> + atomic_set(&conn->le_state, state);
> >> +}
> >
> > The more I read this, the more I think this is the wrong approach to
> > this. The kernel should have a list of device it wants to connect to. If
> > that list is non-empty, start scanning, if one device is found, try to
> > connect it, once done, keep scanning if other devices are not connected.
> >
> > You need to build a foundation for LE devices that the kernel wants to
> > connect to. And not hack in some state machinery.
>
> As commented in the cover letter, this RFC series is an initial work,
> it supports only one LE connection attempt at a time. It doesn't
> support multiple LE connection attempts and doesn't handle concurrent
> device discovery properly yet.
>
> Let me try to give you the big picture:
> LE connection attempts come from user-space through the connect()
> call. Each connect() call adds a hci_conn object into a connection
> list (hdev->conn_hash) and start the LE passive scan if not already
> running. So the kernel has a list of devices it wants to connect to
> (hci_conn objects in HCI_CONN_LE_SCAN le_state). When the target
> device is found, we stop the scanning and initiate the connection.
> Once the connection is done (successfully or not), we start passive
> scanning again if we still have devices we want to connect to.
>
> I think this is pretty much the approach you described. Are we on the
> same page about this approach?
I got what you are trying to achieve, but that is just for connection
handling of connect().
What I really care is about a bigger picture for handling auto-connect
and background scanning.
Same as I care about being able to use the white list as much as
possible. As long as we do not require IRK and the white list is large
enough, lets use it. If it is possible to use a white list, the power
impact will be large. If we can not use the white list, we have to
consider down periods for the passive scanning to allow the host to
sleep for certain amount of time.
> > And of course, can we please do proper error handling here. This whole
> > thing is broken. If any of the HCI commands fail, your state machine is
> > screwed up.
>
> Sure, as I said in the cover letter, the next step is to handle corner
> cases like that.
Error cases are not corner cases. Can we please stop such a thinking. At
random times the controller will have resource limits and thus HCI
commands will fail. Either you handle that right away or this is a
pretty much useless attempt.
The reason I pointed you to Johan's work for transaction is actually the
case of being able to handle error conditions. With LE is has become
obvious that in a lot of cases you have more than just one HCI command
to trigger some actions. The handling gets a lot more complicated. We
will have many small state machines to keep track. This needs clean code
that can be understood by more than just 2 people.
Regards
Marcel
^ permalink raw reply
* Re: [RFC v2 1/8] Bluetooth: Setup LE scan with no timeout
From: Marcel Holtmann @ 2013-02-18 22:41 UTC (permalink / raw)
To: Andre Guedes; +Cc: linux-bluetooth
In-Reply-To: <CACJA=fVD_0z5o6D0L-1roj19GTaOCYiroAXEWWhXMY4tC5+1Xw@mail.gmail.com>
Hi Andre,
> >> This patch modifies hci_do_le_scan and hci_cancel_le_scan helpers so
> >> we are able to start and stop LE scan with no timeout. This feature
> >> will be used by the LE connection routine.
> >>
> >> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> >> ---
> >> net/bluetooth/hci_core.c | 17 +++++++++--------
> >> 1 file changed, 9 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> >> index 22e77a7..3aa0345 100644
> >> --- a/net/bluetooth/hci_core.c
> >> +++ b/net/bluetooth/hci_core.c
> >> @@ -1618,26 +1618,27 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
> >> if (err < 0)
> >> return err;
> >>
> >> - queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
> >> - msecs_to_jiffies(timeout));
> >> + if (timeout > 0)
> >> + queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
> >> + msecs_to_jiffies(timeout));
> >>
> >> return 0;
> >> }
> >
> > I really do not like this magic handling of scan disable. Maybe you
> > better remove the timeout handling completely and put it into the
> > discovery functionality.
> >
> > Doing it like this seems pretty much hacked together. It no longer looks
> > like the right place to do handle it.
>
> The le_scan_disable work should be scheduled only if LE scanning was
> started successfully.
>
> So hci_do_le_scan seems to be a better place than discovery
> functionality for the timeout handling. Besides, these LE scan helpers
> were originally designed to provide a self-contained framework to
> start and stop LE scanning.
>
> The framework lacks support for starting LE scanning with no timeout.
> This patch aims to address this issue.
>
> Anyway, if you still think it is a good idea to remove this timeout
> handling, I'll remove it in the next series.
look into what Johan is working on with the transaction support. I am
getting the feeling that moving everything over to proper transaction
and not just trying it hack stuff in seems to be the right approach.
For example the actual discovery handling needs to be able to report
errors anyway. So that can be handled there.
Regards
Marcel
^ permalink raw reply
* Re: [RFC v2 3/8] Bluetooth: LE connection state machine
From: Andre Guedes @ 2013-02-18 22:08 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1361053940.1583.14.camel@aeonflux>
Hi Marcel,
On Sat, Feb 16, 2013 at 8:32 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andre,
>
>> This patch implements a state machine for carrying out the general
>> connection establishment procedure described in Core spec.
>>
>> The state machine should be used as follows: when the kernel
>> receives a new LE connection attempt, it should go to HCI_CONN_LE_
>> SCAN state, starting the passive LE scanning. Once the target remote
>> device is in-range, it should go to HCI_CONN_LE_FOUND, stopping the
>> ongoing LE scanning. After the LE scanning is disabled, it should go
>> to HCI_CONN_LE_INITIATE state where the LE connection is created.
>>
>> This state machine will be used by the LE connection routine in
>> order to establish LE connections.
>>
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci_core.h | 13 +++++++++++++
>> net/bluetooth/hci_conn.c | 26 ++++++++++++++++++++++++++
>> 2 files changed, 39 insertions(+)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 48c28ca..c704737 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -300,6 +300,16 @@ struct hci_dev {
>>
>> #define HCI_PHY_HANDLE(handle) (handle & 0xff)
>>
>> +/*
>> + * States from LE connection establishment state machine.
>> + * State 0 is reserved and indicates the state machine is not running.
>> + */
>> +enum {
>> + HCI_CONN_LE_SCAN = 1,
>> + HCI_CONN_LE_FOUND,
>> + HCI_CONN_LE_INITIATE,
>> +};
>> +
>
> and why don't we have HCI_CONN_LE_IDLE = 0 then. I do not get this magic
> number handling when you introduce an enum anyway. You spent more time
> explaining it with a comment instead of just using an extra enum entry.
Ok, I'll introduce the HCI_CONN_LE_IDLE state.
>> struct hci_conn {
>> struct list_head list;
>>
>> @@ -356,6 +366,8 @@ struct hci_conn {
>>
>> struct hci_conn *link;
>>
>> + atomic_t le_state;
>> +
>
> Why is this atomic_t. I am lost on what you are trying to solve here.
> This requires a detailed explanation or you just get rid of it and use
> the enum.
le_state was defined as atomic_t so read and write are atomic
operations, avoiding to acquire hdev->lock every time we want to read
or write this variable.
>> void (*connect_cfm_cb) (struct hci_conn *conn, u8 status);
>> void (*security_cfm_cb) (struct hci_conn *conn, u8 status);
>> void (*disconn_cfm_cb) (struct hci_conn *conn, u8 reason);
>> @@ -599,6 +611,7 @@ int hci_conn_check_secure(struct hci_conn *conn, __u8 sec_level);
>> int hci_conn_security(struct hci_conn *conn, __u8 sec_level, __u8 auth_type);
>> int hci_conn_change_link_key(struct hci_conn *conn);
>> int hci_conn_switch_role(struct hci_conn *conn, __u8 role);
>> +void hci_conn_set_le_state(struct hci_conn *conn, int state);
>
> External state setting. That does not look like a good idea in the first
> place. Why do you want to do it like this. Shouldn't be this self
> contained.
We need to set le_state variable in hci_event.c (see patch 5/8). That
is why this function was added to hci_core.h.
>> void hci_conn_enter_active_mode(struct hci_conn *conn, __u8 force_active);
>>
>> diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
>> index 25bfce0..d54c2a0 100644
>> --- a/net/bluetooth/hci_conn.c
>> +++ b/net/bluetooth/hci_conn.c
>> @@ -391,6 +391,7 @@ struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst)
>> (unsigned long) conn);
>>
>> atomic_set(&conn->refcnt, 0);
>> + atomic_set(&conn->le_state, 0);
>>
>> hci_dev_hold(hdev);
>>
>> @@ -1027,3 +1028,28 @@ struct hci_chan *hci_chan_lookup_handle(struct hci_dev *hdev, __u16 handle)
>>
>> return hchan;
>> }
>> +
>> +void hci_conn_set_le_state(struct hci_conn *conn, int state)
>> +{
>> + struct hci_dev *hdev = conn->hdev;
>> +
>> + BT_DBG("conn %p state %d -> %d", conn, atomic_read(&conn->le_state),
>> + state);
>> +
>> + if (state == atomic_read(&conn->le_state))
>> + return;
>> +
>> + switch (state) {
>> + case HCI_CONN_LE_SCAN:
>> + hci_le_scan(hdev, LE_SCAN_PASSIVE, 0x60, 0x30, 0);
>> + break;
>> + case HCI_CONN_LE_FOUND:
>> + hci_cancel_le_scan(hdev);
>> + break;
>> + case HCI_CONN_LE_INITIATE:
>> + hci_le_create_connection(conn);
>> + break;
>> + }
>> +
>> + atomic_set(&conn->le_state, state);
>> +}
>
> The more I read this, the more I think this is the wrong approach to
> this. The kernel should have a list of device it wants to connect to. If
> that list is non-empty, start scanning, if one device is found, try to
> connect it, once done, keep scanning if other devices are not connected.
>
> You need to build a foundation for LE devices that the kernel wants to
> connect to. And not hack in some state machinery.
As commented in the cover letter, this RFC series is an initial work,
it supports only one LE connection attempt at a time. It doesn't
support multiple LE connection attempts and doesn't handle concurrent
device discovery properly yet.
Let me try to give you the big picture:
LE connection attempts come from user-space through the connect()
call. Each connect() call adds a hci_conn object into a connection
list (hdev->conn_hash) and start the LE passive scan if not already
running. So the kernel has a list of devices it wants to connect to
(hci_conn objects in HCI_CONN_LE_SCAN le_state). When the target
device is found, we stop the scanning and initiate the connection.
Once the connection is done (successfully or not), we start passive
scanning again if we still have devices we want to connect to.
I think this is pretty much the approach you described. Are we on the
same page about this approach?
> And of course, can we please do proper error handling here. This whole
> thing is broken. If any of the HCI commands fail, your state machine is
> screwed up.
Sure, as I said in the cover letter, the next step is to handle corner
cases like that.
In next RFC series I'll cover all those corner cases and I'll also add
support for multiple LE connections attempts. I believe the next RFC
series will clarify things a little bit.
Regards,
Andre
^ permalink raw reply
* Re: [RFC v2 1/8] Bluetooth: Setup LE scan with no timeout
From: Andre Guedes @ 2013-02-18 22:07 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <1361053591.1583.9.camel@aeonflux>
Hi Marcel,
On Sat, Feb 16, 2013 at 8:26 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andre,
>
>> This patch modifies hci_do_le_scan and hci_cancel_le_scan helpers so
>> we are able to start and stop LE scan with no timeout. This feature
>> will be used by the LE connection routine.
>>
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> net/bluetooth/hci_core.c | 17 +++++++++--------
>> 1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 22e77a7..3aa0345 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1618,26 +1618,27 @@ static int hci_do_le_scan(struct hci_dev *hdev, u8 type, u16 interval,
>> if (err < 0)
>> return err;
>>
>> - queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
>> - msecs_to_jiffies(timeout));
>> + if (timeout > 0)
>> + queue_delayed_work(hdev->workqueue, &hdev->le_scan_disable,
>> + msecs_to_jiffies(timeout));
>>
>> return 0;
>> }
>
> I really do not like this magic handling of scan disable. Maybe you
> better remove the timeout handling completely and put it into the
> discovery functionality.
>
> Doing it like this seems pretty much hacked together. It no longer looks
> like the right place to do handle it.
The le_scan_disable work should be scheduled only if LE scanning was
started successfully.
So hci_do_le_scan seems to be a better place than discovery
functionality for the timeout handling. Besides, these LE scan helpers
were originally designed to provide a self-contained framework to
start and stop LE scanning.
The framework lacks support for starting LE scanning with no timeout.
This patch aims to address this issue.
Anyway, if you still think it is a good idea to remove this timeout
handling, I'll remove it in the next series.
Regards,
Andre
^ permalink raw reply
* Re: [PATCH 1/2] tools: Fix compilation error with GINT_TO_POINTER
From: Lucas De Marchi @ 2013-02-18 16:44 UTC (permalink / raw)
To: Syam Sidhardhan, linux-bluetooth
In-Reply-To: <20130218161331.GA5539@x220.P-661HNU-F1>
On Mon, Feb 18, 2013 at 1:13 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Syam,
>
> On Mon, Feb 18, 2013, Syam Sidhardhan wrote:
>> Fixes the following error:
>> tools/btmgmt.c: In function ‘index_rsp’:
>> tools/btmgmt.c:756:10: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>> tools/btmgmt.c: In function ‘cmd_info’:
>> tools/btmgmt.c:791:9: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
>> cc1: all warnings being treated as errors
>> make[1]: *** [tools/btmgmt.o] Error 1
>> make: *** [all] Error 2
>> ---
>> tools/btmgmt.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> This patch has been applied. For whatever reason I did not get this
> warning/error in my environment. Which gcc version and architecture are
> you using?
The problem is more likely to be in the different glib versions and
running on 32 bits:
https://bugzilla.gnome.org/show_bug.cgi?id=661546
Lucas De Marchi
^ permalink raw reply
* Re: [PATCH 1/2] tools: Fix compilation error with GINT_TO_POINTER
From: Johan Hedberg @ 2013-02-18 16:13 UTC (permalink / raw)
To: Syam Sidhardhan; +Cc: linux-bluetooth
In-Reply-To: <1361203483-32117-1-git-send-email-s.syam@samsung.com>
Hi Syam,
On Mon, Feb 18, 2013, Syam Sidhardhan wrote:
> Fixes the following error:
> tools/btmgmt.c: In function ‘index_rsp’:
> tools/btmgmt.c:756:10: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
> tools/btmgmt.c: In function ‘cmd_info’:
> tools/btmgmt.c:791:9: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
> cc1: all warnings being treated as errors
> make[1]: *** [tools/btmgmt.o] Error 1
> make: *** [all] Error 2
> ---
> tools/btmgmt.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
This patch has been applied. For whatever reason I did not get this
warning/error in my environment. Which gcc version and architecture are
you using?
Johan
^ permalink raw reply
* [PATCH 2/2] avctp: Fix invalid file descriptor close
From: Syam Sidhardhan @ 2013-02-18 16:04 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Syam Sidhardhan
In-Reply-To: <1361203483-32117-1-git-send-email-s.syam@samsung.com>
During avctp_confirm_cb(), if any error happens we set the session
state to AVCTP_STATE_DISCONNECTED, which inturn try to close fd 0.
---
I'm not sure about this fix in the latest upstream code,
but in the case of Bluez 4.101, I got the following
error log(with extra fd print) and this patch fixes the same.
audio/avctp.c:avctp_confirm_cb() AVCTP: incoming connect from BC:47:60:F5:88:89
Refusing unexpected connect from BC:47:60:F5:88:89
audio/avctp.c:avctp_set_state() AVCTP Disconnected
audio/avctp.c:avctp_disconnected()
AVCTP: closing uinput[fd=0] for BC:47:60:F5:88:89
profiles/audio/avctp.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
index 13dd4c3..c276c52 100644
--- a/profiles/audio/avctp.c
+++ b/profiles/audio/avctp.c
@@ -1201,6 +1201,7 @@ static struct avctp *avctp_get_internal(struct btd_device *device)
session->server = server;
session->device = btd_device_ref(device);
session->state = AVCTP_STATE_DISCONNECTED;
+ session->uinput = -1;
server->sessions = g_slist_append(server->sessions, session);
--
1.7.9.5
^ permalink raw reply related
* [PATCH 1/2] tools: Fix compilation error with GINT_TO_POINTER
From: Syam Sidhardhan @ 2013-02-18 16:04 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Syam Sidhardhan
Fixes the following error:
tools/btmgmt.c: In function ‘index_rsp’:
tools/btmgmt.c:756:10: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
tools/btmgmt.c: In function ‘cmd_info’:
tools/btmgmt.c:791:9: error: cast to pointer from integer of different size [-Werror=int-to-pointer-cast]
cc1: all warnings being treated as errors
make[1]: *** [tools/btmgmt.o] Error 1
make: *** [all] Error 2
---
tools/btmgmt.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/btmgmt.c b/tools/btmgmt.c
index 85e790b..715efde 100644
--- a/tools/btmgmt.c
+++ b/tools/btmgmt.c
@@ -753,7 +753,7 @@ static void index_rsp(uint8_t status, uint16_t len, const void *param,
if (monitor)
printf("hci%u ", index);
- data = GINT_TO_POINTER(index);
+ data = GINT_TO_POINTER((gint) index);
if (mgmt_send(mgmt, MGMT_OP_READ_INFO, index, 0, NULL,
info_rsp, data, NULL) == 0) {
@@ -788,7 +788,7 @@ static void cmd_info(struct mgmt *mgmt, uint16_t index, int argc, char **argv)
return;
}
- data = GINT_TO_POINTER(index);
+ data = GINT_TO_POINTER((gint) index);
if (mgmt_send(mgmt, MGMT_OP_READ_INFO, index, 0, NULL, info_rsp,
data, NULL) == 0) {
--
1.7.9.5
^ permalink raw reply related
* Re: mgmt-api.txt
From: Randy Yates @ 2013-02-18 16:00 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <20130218085806.GB11034@x220>
Thanks much, Johan. That clarifies things nicely.
--Randy
Johan Hedberg <johan.hedberg@gmail.com> writes:
> Hi,
>
> On Mon, Feb 18, 2013, Johan Hedberg wrote:
>> On Mon, Feb 18, 2013, Randy Yates wrote:
>> > Almost all text files in doc/ are consistently-documented dbus APIs,
>> > specifying service, interface, and object path.
>> >
>> > However, doc/mgmt-api.txt is in a completely different format. It states
>> > something about "Packet Structures" but I have no idea which packets the
>> > document is referring to.
>> >
>> > Can someone explain how to use this API? We have a requirement to set
>> > some of these properties, such as discoverability and link level
>> > security.
>>
>> This is just a special socket type that kernel versions from 3.4 onwards
>> provide. See e.g. the mgmt_new_default() function in src/shared/mgmt.c
>> for an example of creating a mgmt socket that you can use to communicate
>> using the protocol described in mgmt-api.txt. There are also several
>> tools in the source tree that use mgmt sockets, e.g. tools/btmgmt
>> client/bluetoothctl and monitor/btmon. Looking at the source code for
>> those tools may also be helpful.
>
> Minor mistake there: bluetoothctl doesn't use mgmt sockets (it uses just
> the D-Bus interface). I also went ahead and pushed a description of how
> to create mgmt sockets to mgmt-api.txt since this was a quite obvious
> omission in the file. Another file you'll probably find useful is
> lib/mgmt.h which has structs and numerical definitions for the mgmt
> protocol.
>
> Johan
> --
> 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
--
Randy Yates
Digital Signal Labs
http://www.digitalsignallabs.com
^ permalink raw reply
* Resend [PATCH] hidp: Make hidp_get_raw_report abort if the session is terminating
From: Karl Relton @ 2013-02-18 14:27 UTC (permalink / raw)
To: linux-bluetooth
From: Karl Relton <karllinuxtest.relton@ntlworld.com>
After linux 3.2 the hid_destroy_device call in hidp_session cleaning up
invokes a hook to the power_supply code which in turn tries to read the
battery capacity. This read will trigger a call to hidp_get_raw_report
which is bound to fail because the device is being taken away - so rather
than wait for the 5 second timeout failure this change enables it to fail
straight away.
Signed-off-by: Karl Relton <karllinuxtest.relton@ntlworld.com>
---
net/bluetooth/hidp/core.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index b2bcbe2..a4c1bb0 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -311,6 +311,9 @@ static int hidp_get_raw_report(struct hid_device *hid,
int numbered_reports = hid->report_enum[report_type].numbered;
int ret;
+ if (atomic_read(&session->terminate))
+ return -EIO;
+
switch (report_type) {
case HID_FEATURE_REPORT:
report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_FEATURE;
@@ -695,8 +698,10 @@ static int hidp_session(void *arg)
set_current_state(TASK_INTERRUPTIBLE);
while (!atomic_read(&session->terminate)) {
if (ctrl_sk->sk_state != BT_CONNECTED ||
- intr_sk->sk_state != BT_CONNECTED)
+ intr_sk->sk_state != BT_CONNECTED) {
+ atomic_inc(&session->terminate);
break;
+ }
while ((skb = skb_dequeue(&intr_sk->sk_receive_queue))) {
skb_orphan(skb);
--
1.7.9.5
^ permalink raw reply related
* Resend RFC [PATCH] hci: Fix race between hidp_session and hci code
From: Karl Relton @ 2013-02-18 14:26 UTC (permalink / raw)
To: linux-bluetooth
From: Karl Relton <karllinuxtest.relton@ntlworld.com>
Fix race between hidp_session and hci code that can lead to
hci device being removed from sysfs before its children
devices. The removal is now delayed until the last
reference to it in the hci code is removed.
Signed-off-by: Karl Relton <karllinuxtest.relton@ntlworld.com>
---
include/net/bluetooth/hci_core.h | 10 +++-------
net/bluetooth/hci_core.c | 15 +++++++++++++--
2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 90cf75a..47c9d30 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -198,6 +198,7 @@ struct hci_dev {
unsigned long quirks;
+ atomic_t ref_cnt;
atomic_t cmd_cnt;
unsigned int acl_cnt;
unsigned int sco_cnt;
@@ -646,19 +647,14 @@ static inline void hci_conn_put(struct hci_conn *conn)
}
/* ----- HCI Devices ----- */
-static inline void hci_dev_put(struct hci_dev *d)
-{
- BT_DBG("%s orig refcnt %d", d->name,
- atomic_read(&d->dev.kobj.kref.refcount));
-
- put_device(&d->dev);
-}
+void hci_dev_put(struct hci_dev *d);
static inline struct hci_dev *hci_dev_hold(struct hci_dev *d)
{
BT_DBG("%s orig refcnt %d", d->name,
atomic_read(&d->dev.kobj.kref.refcount));
+ atomic_inc(&d->ref_cnt);
get_device(&d->dev);
return d;
}
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 618ca1a..c187a84 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -891,6 +891,19 @@ int hci_dev_close(__u16 dev)
return err;
}
+void hci_dev_put(struct hci_dev *d)
+{
+ BT_DBG("%s orig refcnt %d", d->name,
+ atomic_read(&d->dev.kobj.kref.refcount));
+
+ if(atomic_dec_and_test(&d->ref_cnt)) {
+ hci_del_sysfs(d);
+ }
+ put_device(&d->dev);
+}
+EXPORT_SYMBOL(hci_dev_put);
+
+
int hci_dev_reset(__u16 dev)
{
struct hci_dev *hdev;
@@ -1884,8 +1897,6 @@ void hci_unregister_dev(struct hci_dev *hdev)
rfkill_destroy(hdev->rfkill);
}
- hci_del_sysfs(hdev);
-
destroy_workqueue(hdev->workqueue);
destroy_workqueue(hdev->req_workqueue);
--
1.7.9.5
^ permalink raw reply related
* Resend [PATCH] hidp: Make hidp_get_raw_report abort if the session is terminating
From: Karl and Karen Relton @ 2013-02-18 14:25 UTC (permalink / raw)
To: linux-bluetooth
From: Karl Relton <karllinuxtest.relton@ntlworld.com>
After linux 3.2 the hid_destroy_device call in hidp_session cleaning up
invokes a hook to the power_supply code which in turn tries to read the
battery capacity. This read will trigger a call to hidp_get_raw_report
which is bound to fail because the device is being taken away - so rather
than wait for the 5 second timeout failure this change enables it to fail
straight away.
Signed-off-by: Karl Relton <karllinuxtest.relton@ntlworld.com>
---
net/bluetooth/hidp/core.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index b2bcbe2..a4c1bb0 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -311,6 +311,9 @@ static int hidp_get_raw_report(struct hid_device *hid,
int numbered_reports = hid->report_enum[report_type].numbered;
int ret;
+ if (atomic_read(&session->terminate))
+ return -EIO;
+
switch (report_type) {
case HID_FEATURE_REPORT:
report_type = HIDP_TRANS_GET_REPORT | HIDP_DATA_RTYPE_FEATURE;
@@ -695,8 +698,10 @@ static int hidp_session(void *arg)
set_current_state(TASK_INTERRUPTIBLE);
while (!atomic_read(&session->terminate)) {
if (ctrl_sk->sk_state != BT_CONNECTED ||
- intr_sk->sk_state != BT_CONNECTED)
+ intr_sk->sk_state != BT_CONNECTED) {
+ atomic_inc(&session->terminate);
break;
+ }
while ((skb = skb_dequeue(&intr_sk->sk_receive_queue))) {
skb_orphan(skb);
--
1.7.9.5
--
----------------------------------------------------------------
Karen & Karl Relton ka_ka@reltons.freeserve.co.uk
^ permalink raw reply related
* Re: [PATCH 02/12 v3] Bluetooth: Add basic start/complete HCI transaction functions
From: Marcel Holtmann @ 2013-02-18 12:47 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
In-Reply-To: <20130218074329.GA17232@x220>
Hi Johan,
> > > +int hci_start_transaction(struct hci_dev *hdev)
> > > +{
> > > + struct hci_transaction *transaction;
> > > + int err;
> > > +
> > > + hci_transaction_lock(hdev);
> > > +
> > > + /* We can't start a new transaction if there's another one in
> > > + * progress of being built.
> > > + */
> > > + if (hdev->build_transaction) {
> > > + err = -EBUSY;
> > > + goto unlock;
> > > + }
> >
> > I do not get this part. Why not use a common mutex on
> > hci_start_transaction and have it close with hci_complete_transaction.
> >
> > This sounds more like a double protection.
> >
> > > +
> > > + transaction = kzalloc(sizeof(*transaction), GFP_KERNEL);
> > > + if (!transaction) {
> > > + err = -ENOMEM;
> > > + goto unlock;
> > > + }
> > > +
> > > + skb_queue_head_init(&transaction->cmd_q);
> > > +
> > > + hdev->build_transaction = transaction;
> > > +
> >
> > I am bit confused with this build_transaction concept. So we are
> > expecting to build transaction inside the same function. Meaning that
> > start and complete functions will be called in the same context.
> >
> > Maybe some concept of DECLARE_TRANSACTION declaring a local variable and
> > then start and complete transaction would be better.
>
> Both of the above two issues are related to the desire to not have to
> introduce a new function next to hci_send_cmd() (that would take this
> local variable "context") and to be able to keep all current
> hci_send_cmd() calls as they are.
not having to modify hci_send_cmd seems fine. I was more thinking of
tracking this stuff. Since it only requires to know start and end of the
transaction. So that tracking could be localized.
I do like the idea of replacing cmd_q with a more advanced queue that
also has the ability for callbacks when a command finished. That is
pretty neat. The API itself seems also fine, but the actual
implementation feels rather hackish to have this all in hdev struct
while in reality it should be localized to the calling functions (except
for the queue itself of course).
The way I see this, we just need to take a proper lock protecting the
queue. And then mark the begin of a transaction and when it finishes.
When you call finish the markers get supplied to the queue.
Of course the queue can not be processed at the same time you create a
transaction, but that should be just fine.
> In my initial iteration of the patch set I did have the kind of locking
> you describe, but keeping the requirement for hci_send_cmd() like I
> stated means that you'll then either end up having a potential race
> or a deadlock inside the function since sometimes you're entering with
> the transaction lock held (when using begin/finish transaction) and
> sometimes without the lock held (all existing calls to hci_send_cmd().
I think we need to protect the access the queue against the queue
processing and that should be enough. I can see the desire to make
hci_send_cmd essentially a single transaction without callback. That is
a nice idea and should be most likely kept. However I am not sure if not
better use new command to add commands to a transaction. It does not
feel right to overload hci_send_cmd with two semantics.
The kernel historically makes the caller being aware of what it is doing
and what context it is in. See GFP_KERNEL and alike and locked vs
unlocked functions. If we are trying to be super smart now seems going
against that.
Regards
Marcel
^ permalink raw reply
* Re: mgmt-api.txt
From: Johan Hedberg @ 2013-02-18 8:58 UTC (permalink / raw)
To: Randy Yates, linux-bluetooth
In-Reply-To: <20130218081221.GA8590@x220>
Hi,
On Mon, Feb 18, 2013, Johan Hedberg wrote:
> On Mon, Feb 18, 2013, Randy Yates wrote:
> > Almost all text files in doc/ are consistently-documented dbus APIs,
> > specifying service, interface, and object path.
> >
> > However, doc/mgmt-api.txt is in a completely different format. It states
> > something about "Packet Structures" but I have no idea which packets the
> > document is referring to.
> >
> > Can someone explain how to use this API? We have a requirement to set
> > some of these properties, such as discoverability and link level
> > security.
>
> This is just a special socket type that kernel versions from 3.4 onwards
> provide. See e.g. the mgmt_new_default() function in src/shared/mgmt.c
> for an example of creating a mgmt socket that you can use to communicate
> using the protocol described in mgmt-api.txt. There are also several
> tools in the source tree that use mgmt sockets, e.g. tools/btmgmt
> client/bluetoothctl and monitor/btmon. Looking at the source code for
> those tools may also be helpful.
Minor mistake there: bluetoothctl doesn't use mgmt sockets (it uses just
the D-Bus interface). I also went ahead and pushed a description of how
to create mgmt sockets to mgmt-api.txt since this was a quite obvious
omission in the file. Another file you'll probably find useful is
lib/mgmt.h which has structs and numerical definitions for the mgmt
protocol.
Johan
^ permalink raw reply
* Re: BT_ADDR not updated
From: Johan Hedberg @ 2013-02-18 8:55 UTC (permalink / raw)
To: Randy Yates; +Cc: linux-bluetooth
In-Reply-To: <87bobkjoqs.fsf@randy.site>
Hi Randy,
On Sat, Feb 16, 2013, Randy Yates wrote:
> This question is regarding bluez 5.2:
>
> i'm changing bt addr "on-the-fly" via a vendor-specific hcitool
> command but the bluetoothctl is not seeing the new address, even
> after a hciconfig hci0 reset
>
> however i do see the new address via "hcitool dev", so it's getting
> changed in the device
>
> is there some bug in bluez updating the device address on a reset?
The address is assumed to stay unchanged from the moment that the
existence of the controller is presented to user space, i.e. there is no
event through which user space would be notified of changes to its
address. If your hardware requires this kind of initialization (e.g. if
it doesn't have a proper address from the start) the best place for
writing the address would probably be the HCI driver's initialization
routine.
Johan
^ permalink raw reply
* Re: mgmt-api.txt
From: Johan Hedberg @ 2013-02-18 8:12 UTC (permalink / raw)
To: Randy Yates; +Cc: linux-bluetooth
In-Reply-To: <87d2vygjyy.fsf@randy.site>
Hi Randy,
On Mon, Feb 18, 2013, Randy Yates wrote:
> Almost all text files in doc/ are consistently-documented dbus APIs,
> specifying service, interface, and object path.
>
> However, doc/mgmt-api.txt is in a completely different format. It states
> something about "Packet Structures" but I have no idea which packets the
> document is referring to.
>
> Can someone explain how to use this API? We have a requirement to set
> some of these properties, such as discoverability and link level
> security.
This is just a special socket type that kernel versions from 3.4 onwards
provide. See e.g. the mgmt_new_default() function in src/shared/mgmt.c
for an example of creating a mgmt socket that you can use to communicate
using the protocol described in mgmt-api.txt. There are also several
tools in the source tree that use mgmt sockets, e.g. tools/btmgmt
client/bluetoothctl and monitor/btmon. Looking at the source code for
those tools may also be helpful.
Johan
^ permalink raw reply
* Re: [PATCH v3 BlueZ 00/13] Fix SDP DE Type Descriptor validation issues
From: Johan Hedberg @ 2013-02-18 8:05 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: linux-bluetooth
In-Reply-To: <1360940876-6314-1-git-send-email-anderson.lizardo@openbossa.org>
Hi Lizardo,
On Fri, Feb 15, 2013, Anderson Lizardo wrote:
> Change since v2:
> * Fix commit author mangled during import from GMANE
>
> Changes since v1:
> * Fix license header to match BlueZ license (GPL v2 or later)
> * Rename test source file and SDP tests to account for future addition of other
> libbluetooth tests
>
> This series adds various missing DTD validations, specially for SEQ* types. The
> lack of these validations allows for a remote device to crash BlueZ due to
> invalid memory access.
>
> I also added unit tests for all affected functions. They are in a separate C
> file (unit/test-lib.c), which will in future contain tests for other
> libbluetooth API functions.
>
> The only pending related fixes from my part are some missing NULL pointer
> checks when accessing empty sequences. These will take some time to fix as they
> affect profile code as well.
>
> Best Regards,
>
> Anderson Lizardo (13):
> unit: Add initial SDP library unit tests
> lib: Add SDP_IS_ALT() macro
> lib: Reuse identical code in sdp_get_{add,}_access_protos()
> lib: Cleanup coding style in sdp_get_proto_descs()
> lib: Fix missing DTD validation while accessing SDP data elements
> unit: Add tests for sdp_get_lang_attr()
> lib: Add missing DTD validation in sdp_record_print()
> lib: Validate DTDs when parsing LanguageBaseAttributeIDList
> lib: Validate DTDs when parsing BluetoothProfileDescriptorList
> lib: Add comment to BluetoothProfileDescriptorList parsing workaround
> lib: Validate DTDs when parsing VersionNumberList
> unit: Add tests for sdp_get_profile_descs()
> unit: Add tests for sdp_get_server_ver()
>
> .gitignore | 1 +
> Makefile.am | 5 +
> lib/sdp.c | 164 +++++++++++++------
> lib/sdp.h | 1 +
> unit/test-lib.c | 471 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 596 insertions(+), 46 deletions(-)
> create mode 100644 unit/test-lib.c
All patches in this set have been applied. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH 12/12] Bluetooth: Remove empty HCI event handlers
From: Johan Hedberg @ 2013-02-18 7:51 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: Bluetooth Linux
In-Reply-To: <CAMXE1-pn2V-6mgvqpwDygaqgsUr90yV7BS_txyzGQytA4gTABw@mail.gmail.com>
Hi Andrei,
On Sun, Feb 17, 2013, Andrei Emeltchenko wrote:
> > With the removal of hci_req_complete() several HCI event handles have
> > essentially become empty and can be removed.
>
> It is still good to have debug traces.
If you need to trace exactly what HCI events are happening btmon and
hcidump are probably much more useful tools for that than kernel debug
logs. And even if you do want debug logs we can just add single BT_DBT()
calls to the hci_cmd_complete() and hci_cmd_status() functions instead
of creating a new function for every single event just for the sake of
having a debug log for them.
Johan
^ permalink raw reply
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