* [PATCH v3 8/8] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
From: johan.hedberg @ 2013-09-14 17:28 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379179711-16436-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
In the case of blocking sockets we should not proceed with sendmsg() if
the socket has the BT_SK_SUSPEND flag set. So far the code was only
ensuring that POLLOUT doesn't get set for non-blocking sockets using
poll() but there was no code in place to ensure that blocking sockets do
the right thing when writing to them.
This patch adds a new bt_sock_wait_ready helper function to sleep in the
sendmsg call if the BT_SK_SUSPEND flag is set, and wake up as soon as it
is unset. It also updates the L2CAP and RFCOMM sendmsg callbacks to take
advantage of this new helper function.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
v3: Use wait_ready instead of wait_unsuspend
include/net/bluetooth/bluetooth.h | 1 +
net/bluetooth/af_bluetooth.c | 39 +++++++++++++++++++++++++++++++++++++++
net/bluetooth/l2cap_sock.c | 6 ++++++
net/bluetooth/rfcomm/sock.c | 9 +++++++--
4 files changed, 53 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
index 10d43d8..afbc711 100644
--- a/include/net/bluetooth/bluetooth.h
+++ b/include/net/bluetooth/bluetooth.h
@@ -249,6 +249,7 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
uint bt_sock_poll(struct file *file, struct socket *sock, poll_table *wait);
int bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
+int bt_sock_wait_ready(struct sock *sk, unsigned long flags);
void bt_accept_enqueue(struct sock *parent, struct sock *sk);
void bt_accept_unlink(struct sock *sk);
diff --git a/net/bluetooth/af_bluetooth.c b/net/bluetooth/af_bluetooth.c
index 9096137..a883171c 100644
--- a/net/bluetooth/af_bluetooth.c
+++ b/net/bluetooth/af_bluetooth.c
@@ -525,6 +525,45 @@ int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo)
}
EXPORT_SYMBOL(bt_sock_wait_state);
+int bt_sock_wait_ready(struct sock *sk, unsigned long flags)
+{
+ DECLARE_WAITQUEUE(wait, current);
+ unsigned long timeo;
+ int err = 0;
+
+ BT_DBG("sk %p", sk);
+
+ timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
+
+ add_wait_queue(sk_sleep(sk), &wait);
+ set_current_state(TASK_INTERRUPTIBLE);
+ while (test_bit(BT_SK_SUSPEND, &bt_sk(sk)->flags)) {
+ if (!timeo) {
+ err = -EAGAIN;
+ break;
+ }
+
+ if (signal_pending(current)) {
+ err = sock_intr_errno(timeo);
+ break;
+ }
+
+ release_sock(sk);
+ timeo = schedule_timeout(timeo);
+ lock_sock(sk);
+ set_current_state(TASK_INTERRUPTIBLE);
+
+ err = sock_error(sk);
+ if (err)
+ break;
+ }
+ __set_current_state(TASK_RUNNING);
+ remove_wait_queue(sk_sleep(sk), &wait);
+
+ return err;
+}
+EXPORT_SYMBOL(bt_sock_wait_ready);
+
#ifdef CONFIG_PROC_FS
struct bt_seq_state {
struct bt_sock_list *l;
diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 0098af8..ad95b42 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -777,6 +777,12 @@ static int l2cap_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
if (sk->sk_state != BT_CONNECTED)
return -ENOTCONN;
+ lock_sock(sk);
+ err = bt_sock_wait_ready(sk, msg->msg_flags);
+ release_sock(sk);
+ if (err)
+ return err;
+
l2cap_chan_lock(chan);
err = l2cap_chan_send(chan, msg, len, sk->sk_priority);
l2cap_chan_unlock(chan);
diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index 30b3721..02d96e4 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -544,7 +544,7 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
struct sock *sk = sock->sk;
struct rfcomm_dlc *d = rfcomm_pi(sk)->dlc;
struct sk_buff *skb;
- int sent = 0;
+ int err, sent = 0;
if (test_bit(RFCOMM_DEFER_SETUP, &d->flags))
return -ENOTCONN;
@@ -559,9 +559,14 @@ static int rfcomm_sock_sendmsg(struct kiocb *iocb, struct socket *sock,
lock_sock(sk);
+ err = bt_sock_wait_ready(sk, msg->msg_flags);
+ if (err) {
+ release_sock(sk);
+ return err;
+ }
+
while (len) {
size_t size = min_t(size_t, len, d->mtu);
- int err;
skb = sock_alloc_send_skb(sk, size + RFCOMM_SKB_RESERVE,
msg->msg_flags & MSG_DONTWAIT, &err);
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v3 7/8] Bluetooth: Fix responding to invalid L2CAP signaling commands
From: johan.hedberg @ 2013-09-14 17:28 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379179711-16436-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
When we have an LE link we should not respond to any data on the BR/EDR
L2CAP signaling channel (0x0001) and vice-versa when we have a BR/EDR
link we should not respond to LE L2CAP (CID 0x0005) signaling commands.
This patch fixes this issue by checking for a valid link type and
ignores data if it is wrong.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Acked-by: Marcel Holtmann <marcel@holtmann.org>
---
net/bluetooth/l2cap_core.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 7d36051..247a955 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5311,6 +5311,7 @@ static u16 l2cap_err_to_reason(int err)
static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
struct sk_buff *skb)
{
+ struct hci_conn *hcon = conn->hcon;
u8 *data = skb->data;
int len = skb->len;
struct l2cap_cmd_hdr cmd;
@@ -5318,6 +5319,9 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
l2cap_raw_recv(conn, skb);
+ if (hcon->type != LE_LINK)
+ return;
+
while (len >= L2CAP_CMD_HDR_SIZE) {
u16 cmd_len;
memcpy(&cmd, data, L2CAP_CMD_HDR_SIZE);
@@ -5356,6 +5360,7 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
static inline void l2cap_sig_channel(struct l2cap_conn *conn,
struct sk_buff *skb)
{
+ struct hci_conn *hcon = conn->hcon;
u8 *data = skb->data;
int len = skb->len;
struct l2cap_cmd_hdr cmd;
@@ -5363,6 +5368,9 @@ static inline void l2cap_sig_channel(struct l2cap_conn *conn,
l2cap_raw_recv(conn, skb);
+ if (hcon->type != ACL_LINK)
+ return;
+
while (len >= L2CAP_CMD_HDR_SIZE) {
u16 cmd_len;
memcpy(&cmd, data, L2CAP_CMD_HDR_SIZE);
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v3 6/8] Bluetooth: Fix sending responses to identified L2CAP response packets
From: johan.hedberg @ 2013-09-14 17:28 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379179711-16436-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
When L2CAP packets return a non-zero error and the value is passed
onwards by l2cap_bredr_sig_cmd this will trigger a command reject packet
to be sent. However, the core specification (page 1416 in core 4.0) says
the following: "Command Reject packets should not be sent in response to
an identified Response packet.".
This patch ensures that a command reject packet is not sent for any
identified response packet by ignoring the error return value from the
response handler functions.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/l2cap_core.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index d134501..7d36051 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5212,7 +5212,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
case L2CAP_CONN_RSP:
case L2CAP_CREATE_CHAN_RSP:
- err = l2cap_connect_create_rsp(conn, cmd, cmd_len, data);
+ l2cap_connect_create_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_CONF_REQ:
@@ -5220,7 +5220,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_CONF_RSP:
- err = l2cap_config_rsp(conn, cmd, cmd_len, data);
+ l2cap_config_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_DISCONN_REQ:
@@ -5228,7 +5228,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_DISCONN_RSP:
- err = l2cap_disconnect_rsp(conn, cmd, cmd_len, data);
+ l2cap_disconnect_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_ECHO_REQ:
@@ -5243,7 +5243,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_INFO_RSP:
- err = l2cap_information_rsp(conn, cmd, cmd_len, data);
+ l2cap_information_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_CREATE_CHAN_REQ:
@@ -5255,7 +5255,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_MOVE_CHAN_RSP:
- err = l2cap_move_channel_rsp(conn, cmd, cmd_len, data);
+ l2cap_move_channel_rsp(conn, cmd, cmd_len, data);
break;
case L2CAP_MOVE_CHAN_CFM:
@@ -5263,7 +5263,7 @@ static inline int l2cap_bredr_sig_cmd(struct l2cap_conn *conn,
break;
case L2CAP_MOVE_CHAN_CFM_RSP:
- err = l2cap_move_channel_confirm_rsp(conn, cmd, cmd_len, data);
+ l2cap_move_channel_confirm_rsp(conn, cmd, cmd_len, data);
break;
default:
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v3 5/8] Bluetooth: Fix error code for invalid CID in l2cap_config_req
From: johan.hedberg @ 2013-09-14 17:28 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379179711-16436-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
The convention of the L2CAP code is to return EFAULT when a CID in the
request packet is invalid. This patch fixes the l2cap_config_req to use
that error code instead of ENOENT.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/l2cap_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 9ec1561f..d134501 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -3978,7 +3978,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
chan = l2cap_get_chan_by_scid(conn, dcid);
if (!chan)
- return -ENOENT;
+ return -EFAULT;
if (chan->state != BT_CONFIG && chan->state != BT_CONNECT2) {
struct l2cap_cmd_rej_cid rej;
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v3 4/8] Bluetooth: Fix L2CAP Disconnect response for unknown CID
From: johan.hedberg @ 2013-09-14 17:28 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379179711-16436-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
If we receive an L2CAP Disconnect Request for an unknown CID we should
not just silently drop it but reply with a proper Command Reject
response. This patch fixes this by ensuring that the disconnect handler
returns a proper error instead of 0 and will cause the function caller
to send the right response.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/l2cap_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 6515804..9ec1561f 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4206,7 +4206,7 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn,
chan = __l2cap_get_chan_by_scid(conn, dcid);
if (!chan) {
mutex_unlock(&conn->chan_lock);
- return 0;
+ return -EFAULT;
}
l2cap_chan_lock(chan);
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v3 3/8] Bluetooth: Fix double error response for l2cap_create_chan_req
From: johan.hedberg @ 2013-09-14 17:28 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379179711-16436-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
When an L2CAP request handler returns non-zero the calling code will
send a command reject response. The l2cap_create_chan_req function will
in some cases send its own response but then still return a -EFAULT
error which would cause two responses to be sent. This patch fixes this
by making the function return 0 after sending its own response.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/l2cap_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 83acb28..6515804 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -4462,7 +4462,7 @@ error:
l2cap_send_cmd(conn, cmd->ident, L2CAP_CREATE_CHAN_RSP,
sizeof(rsp), &rsp);
- return -EFAULT;
+ return 0;
}
static void l2cap_send_move_chan_req(struct l2cap_chan *chan, u8 dest_amp_id)
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v3 2/8] Bluetooth: Fix L2CAP command reject reason
From: johan.hedberg @ 2013-09-14 17:28 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379179711-16436-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
There are several possible reason codes that can be sent in the command
reject L2CAP packet. Before this patch the code has used a hard-coded
single response code ("command not understood"). This patch adds a
helper function to map the return value of an L2CAP handler function to
the correct command reject reason.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
net/bluetooth/l2cap_core.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index b3bb7bc..83acb28 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -5294,6 +5294,20 @@ static inline int l2cap_le_sig_cmd(struct l2cap_conn *conn,
}
}
+static u16 l2cap_err_to_reason(int err)
+{
+ switch (err) {
+ case -EFAULT:
+ return L2CAP_REJ_INVALID_CID;
+ case -EMSGSIZE:
+ return L2CAP_REJ_MTU_EXCEEDED;
+ case -EINVAL:
+ case -EPROTO:
+ default:
+ return L2CAP_REJ_NOT_UNDERSTOOD;
+ }
+}
+
static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
struct sk_buff *skb)
{
@@ -5323,11 +5337,11 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
err = l2cap_le_sig_cmd(conn, &cmd, data);
if (err) {
struct l2cap_cmd_rej_unk rej;
+ u16 reason = l2cap_err_to_reason(err);
BT_ERR("Wrong link type (%d)", err);
- /* FIXME: Map err to a valid reason */
- rej.reason = __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+ rej.reason = cpu_to_le16(reason);
l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
sizeof(rej), &rej);
}
@@ -5368,11 +5382,11 @@ static inline void l2cap_sig_channel(struct l2cap_conn *conn,
err = l2cap_bredr_sig_cmd(conn, &cmd, cmd_len, data);
if (err) {
struct l2cap_cmd_rej_unk rej;
+ u16 reason = l2cap_err_to_reason(err);
BT_ERR("Wrong link type (%d)", err);
- /* FIXME: Map err to a valid reason */
- rej.reason = __constant_cpu_to_le16(L2CAP_REJ_NOT_UNDERSTOOD);
+ rej.reason = cpu_to_le16(reason);
l2cap_send_cmd(conn, cmd.ident, L2CAP_COMMAND_REJ,
sizeof(rej), &rej);
}
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v3 1/8] Bluetooth: Remove unused event mask struct
From: johan.hedberg @ 2013-09-14 17:28 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379179711-16436-1-git-send-email-johan.hedberg@gmail.com>
From: Johan Hedberg <johan.hedberg@intel.com>
The struct for HCI_Set_Event_Mask is never used. Instead a local 8-byte
array is used for sending this command. Therefore, remove the
unnecessary struct definition.
Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
include/net/bluetooth/hci.h | 3 ---
1 file changed, 3 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 57aa408..7ede266 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -695,9 +695,6 @@ struct hci_cp_sniff_subrate {
} __packed;
#define HCI_OP_SET_EVENT_MASK 0x0c01
-struct hci_cp_set_event_mask {
- __u8 mask[8];
-} __packed;
#define HCI_OP_RESET 0x0c03
--
1.8.4.rc3
^ permalink raw reply related
* [PATCH v3 0/8] Bluetooth: Various L2CAP fixes
From: johan.hedberg @ 2013-09-14 17:28 UTC (permalink / raw)
To: linux-bluetooth
Hi,
This set started off with just three patches but after looking into the
code based on feedback from Marcel there were several other issues that
were found which prompted for more patches.
Johan
----------------------------------------------------------------
Johan Hedberg (8):
Bluetooth: Remove unused event mask struct
Bluetooth: Fix L2CAP command reject reason
Bluetooth: Fix double error response for l2cap_create_chan_req
Bluetooth: Fix L2CAP Disconnect response for unknown CID
Bluetooth: Fix error code for invalid CID in l2cap_config_req
Bluetooth: Fix sending responses to identified L2CAP response packets
Bluetooth: Fix responding to invalid L2CAP signaling commands
Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
include/net/bluetooth/bluetooth.h | 1 +
include/net/bluetooth/hci.h | 3 ---
net/bluetooth/af_bluetooth.c | 39 ++++++++++++++++++++++++++++++
net/bluetooth/l2cap_core.c | 48 +++++++++++++++++++++++++++----------
net/bluetooth/l2cap_sock.c | 6 +++++
net/bluetooth/rfcomm/sock.c | 9 +++++--
6 files changed, 88 insertions(+), 18 deletions(-)
^ permalink raw reply
* Re: [PATCH 1/3] Bluetooth: Fix L2CAP Disconnect response for unknown CID
From: Johan Hedberg @ 2013-09-14 16:11 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <34565D06-3DDE-40FE-A9D7-D2667630C5A7@holtmann.org>
Hi Marcel,
On Fri, Sep 13, 2013, Marcel Holtmann wrote:
> > If we receive an L2CAP Disconnect Request for an unknown CID we should
> > not just silently drop it but reply with a proper Command Reject
> > response. This patch fixes this by ensuring that the disconnect handler
> > returns a proper error instead of 0 and will cause the function caller
> > to send the right response.
> >
> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> > ---
> > net/bluetooth/l2cap_core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> > index b3bb7bc..ea3792f 100644
> > --- a/net/bluetooth/l2cap_core.c
> > +++ b/net/bluetooth/l2cap_core.c
> > @@ -4206,7 +4206,7 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn,
> > chan = __l2cap_get_chan_by_scid(conn, dcid);
> > if (!chan) {
> > mutex_unlock(&conn->chan_lock);
> > - return 0;
> > + return -EINVAL;
> > }
>
> the only problem that I have here is that we end up with the wrong
> error code. The L2CAP core still contains this comment:
>
> /* FIXME: Map err to a valid reason */
>
> And now we default to L2CAP_REJ_NOT_UNDERSTOOD, but in this case it
> should be L2CAP_REJ_INVALID_CID.
Well, starting to look at this opened quite a can of worms. There are
several existing inconsistencies in what error codes request handlers
return in various situations. Some are even broken in that they send
their own response but still return non-zero (causing a second response
to be sent).
The current code will also sometimes send a command reject as a
consequence of incorrectly formatted response packets, which is also
wrong according to the core spec (p.1416): "Command Reject packets
should not be sent in response to an identified Response packet.".
I'll fix all of the above issues before introducing a helper function to
map the handler return value to an L2CAP error code.
Johan
^ permalink raw reply
* Re: [PATCH 2/2] Bluetooth: Add event mask page 2 setting support
From: Johan Hedberg @ 2013-09-14 15:58 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <49AE7668-B559-42DC-BF73-59A574C23C96@holtmann.org>
Hi Marcel,
On Fri, Sep 13, 2013, Marcel Holtmann wrote:
> > For those controller that support the HCI_Set_Event_Mask_Page_2 command
> > we should include it in the init sequence. This patch implements sending
> > of the command and enables the events in it based on supported features
> > (currently only CSB is checked).
> >
> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> > ---
> > include/net/bluetooth/hci.h | 2 ++
> > net/bluetooth/hci_core.c | 32 ++++++++++++++++++++++++++++++++
> > 2 files changed, 34 insertions(+)
> >
> > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> > index 63cce97..57aa408 100644
> > --- a/include/net/bluetooth/hci.h
> > +++ b/include/net/bluetooth/hci.h
> > @@ -827,6 +827,8 @@ struct hci_rp_read_inq_rsp_tx_power {
> > __s8 tx_power;
> > } __packed;
> >
> > +#define HCI_OP_SET_EVENT_MASK_PAGE_2 0x0c63
> > +
>
> I am a little bit torn here. We define the struct for event mask, but
> never use it. Now we only define the opcode. So either we add the
> struct here for completeness sake or you add a cleanup patch that
> removes the event mask struct.
I'll create a separate cleanup patch to remove the unused struct.
Johan
^ permalink raw reply
* Re: [PATCH 3/3] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
From: Johan Hedberg @ 2013-09-14 5:50 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <4A92D017-F4C7-49FF-BFA3-D9E508E51DED@holtmann.org>
Hi Marcel,
On Fri, Sep 13, 2013, Marcel Holtmann wrote:
> > In the case of blocking sockets we should not proceed with sendmsg() if
> > the socket has the BT_SK_SUSPEND flag set. So far the code was only
> > ensuring that POLLOUT doesn't get set for non-blocking sockets using
> > poll() but there was no code in place to ensure that blocking sockets do
> > the right thing when writing to them.
> >
> > This patch adds a new bt_sock_wait_unsuspend helper function to sleep in
> > the sendmsg call if the BT_SK_SUSPEND flag is set, and wake up as soon
> > as it is unset. It also updates the L2CAP and RFCOMM sendmsg callbacks
> > to take advantage of this new helper function.
> >
> > Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> > ---
> > include/net/bluetooth/bluetooth.h | 1 +
> > net/bluetooth/af_bluetooth.c | 38 ++++++++++++++++++++++++++++++++++++++
> > net/bluetooth/l2cap_sock.c | 6 ++++++
> > net/bluetooth/rfcomm/sock.c | 7 +++++--
> > 4 files changed, 50 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> > index 10d43d8..3299d42 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -249,6 +249,7 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
> > uint bt_sock_poll(struct file *file, struct socket *sock, poll_table *wait);
> > int bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
> > int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
> > +int bt_sock_wait_unsuspend(struct sock *sk, unsigned long flags);
>
> I am not a huge fan of the term unsuspend. Maybe wait_ready is better here.
I'll change it to wait_ready in the next version. I used unsuspend
simply because it waits for the clearing of a flag called SUSPEND.
Johan
^ permalink raw reply
* Re: [PATCH 2/3] Bluetooth: Fix responding to invalid L2CAP signaling commands
From: Johan Hedberg @ 2013-09-14 5:48 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
In-Reply-To: <56BC5629-3745-4561-90CA-7BB66D52E205@holtmann.org>
Hi Marcel,
On Fri, Sep 13, 2013, Marcel Holtmann wrote:
> > @@ -5304,6 +5305,9 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
> >
> > l2cap_raw_recv(conn, skb);
> >
> > + if (hcon->type != LE_LINK)
> > + return;
> > +
>
> However I do have a question here. Can we just drop the packet or
> should we even in this case return invalid CID. What does the core
> spec. recommend in this case?
I faced this issue at the last UPF and had to do some thinking about it
as well as discuss it with some other stack developers. The conclusion
was that dropping is the right thing since on non-LE links this is in
practice an unknown CID and unknown data.
The core spec doesn't explicitly say that data should be dropped in this
case (it doesn't say anything about responding either). Instead, the LE
signaling channel is simply not defined for non-LE links (the exact
words in the spec are "not available" and "not supported"). It's like
you'd receive data on any unknown CID. Responding something on it would
be just as bad behavior as the peer sending the initial command.
Johan
^ permalink raw reply
* Re: [PATCH v4] Bluetooth: btmrvl: add calibration data download support
From: Mike Frysinger @ 2013-09-14 4:04 UTC (permalink / raw)
To: Bing Zhao
Cc: linux-bluetooth, Marcel Holtmann, Gustavo Padovan, Johan Hedberg,
linux-wireless, Hyuckjoo Lee, Amitkumar Karwar
In-Reply-To: <1379115162-10194-1-git-send-email-bzhao@marvell.com>
On Fri, Sep 13, 2013 at 7:32 PM, Bing Zhao wrote:
> --- a/drivers/bluetooth/btmrvl_main.c
> +++ b/drivers/bluetooth/btmrvl_main.c
>
> +static int btmrvl_parse_cal_cfg(const u8 *src, u32 len, u8 *dst, u32 dst_size)
would be nice if you put a comment above this func explaining the
expected format. otherwise, we see arbitrary parsing with no idea if
it's correct.
> + while ((s - src) < len) {
> + if (isspace(*s)) {
> + s++;
> + continue;
> + }
> +
> + if (isxdigit(*s)) {
> + if ((d - dst) >= dst_size) {
> + BT_ERR("calibration data file too big!!!");
> + return -EINVAL;
> + }
> +
> + memcpy(tmp, s, 2);
> +
> + ret = kstrtou8(tmp, 16, d++);
> + if (ret < 0)
> + return ret;
> +
> + s += 2;
> + } else {
> + s++;
> + }
> + }
so if it's a space, you skip it. if it's a hexdigit, you parse two
bytes. if it's anything else, you skip it. i'd imagine the "non
space and non hexdigit" case should throw a warning if not reject the
file out right. otherwise, if you want to keep this logic, punt the
explicit "isspace" check.
you might also copy one more byte than you should ? your limit is "(s
- src) < len", yet the isxdigit code always copies two bytes.
> +static int btmrvl_load_cal_data(struct btmrvl_private *priv,
> + u8 *config_data)
> +{
> + struct sk_buff *skb;
> + struct btmrvl_cmd *cmd;
> + int i;
> +
> + skb = bt_skb_alloc(sizeof(*cmd), GFP_ATOMIC);
maybe i'm unfamiliar with bluetooth and this is common, but why is
your code so special as to require GFP_ATOMIC allocations ?
> + for (i = 4; i < BT_CMD_DATA_SIZE; i++)
> + cmd->data[i] = config_data[(i/4)*8 - 1 - i];
style nit, but there should be spacing around those math operators.
ignoring the fact that this is some funky funky buffer offsets.
i = 4
config_data[(4 / 4) * 8 - 1 - 4] ->
config_data[8 - 1 - 4] ->
config_data[3]
i = 5
config_data[(5 / 4) * 8 - 1 - 5] ->
config_data[8 - 1 - 5] ->
config_data[2]
i = 6
config_data[(6 / 4) * 8 - 1 - 6] ->
config_data[8 - 1 - 6] ->
config_data[1]
i = 7
config_data[(7 / 4) * 8 - 1 - 7] ->
config_data[8 - 1 - 7] ->
config_data[0]
i = 8
config_data[(8 / 4) * 8 - 1 - 8] ->
config_data[16 - 1 - 8] ->
config_data[7]
i = {4,5,6,7} -> config_data[{3,2,1,0}]
i = {8,9,10,11} -> config_data[{7,6,5,4}]
that really could do with a comment explaining the mapping of input
bytes to output bytes.
-mike
^ permalink raw reply
* Re: [PATCH 2/8] core: Minor whitespace fix
From: Marcel Holtmann @ 2013-09-14 1:50 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth
In-Reply-To: <1379071852-10094-2-git-send-email-szymon.janc@tieto.com>
Hi Szymon,
> ---
> src/main.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
patch 1/8 and patch 2/8 are obviously correct. Get Johan to apply them.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 4/8] sdp: Decouple Device ID profile implementation
From: Marcel Holtmann @ 2013-09-14 1:49 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth
In-Reply-To: <1379071852-10094-4-git-send-email-szymon.janc@tieto.com>
Hi Szymon,
> Make DeviceID profile similar to other profiles implementations. Use
> btd_profile for handling DeviceID profile while adding/removing
> adapters. The nice drawback is that SDP code no longer depends on
> main_opts.
> ---
> Makefile.plugins | 3 +
> profiles/deviceid/deviceid.c | 181 +++++++++++++++++++++++++++++++++++++++++++
> src/sdpd-server.c | 4 -
> src/sdpd-service.c | 58 --------------
> src/sdpd.h | 2 -
> 5 files changed, 184 insertions(+), 64 deletions(-)
> create mode 100644 profiles/deviceid/deviceid.c
>
> diff --git a/Makefile.plugins b/Makefile.plugins
> index 7c5f71d..df5d2a1 100644
> --- a/Makefile.plugins
> +++ b/Makefile.plugins
> @@ -82,6 +82,9 @@ builtin_sources += profiles/scanparam/scan.c
> builtin_modules += deviceinfo
> builtin_sources += profiles/deviceinfo/deviceinfo.c
>
> +builtin_modules += deviceid
> +builtin_sources += profiles/deviceid/deviceid.c
> +
this is changing the semantics a little bit. Device Id record is guaranteed to have 0x10000 handle. I would prefer if we can keep it that way. Also we need to find a way to make this work with the extended inquiry response that can also included device id information.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 3/3] Bluetooth: Fix waiting for clearing of BT_SK_SUSPEND flag
From: Marcel Holtmann @ 2013-09-14 1:43 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1379061827-8595-4-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> In the case of blocking sockets we should not proceed with sendmsg() if
> the socket has the BT_SK_SUSPEND flag set. So far the code was only
> ensuring that POLLOUT doesn't get set for non-blocking sockets using
> poll() but there was no code in place to ensure that blocking sockets do
> the right thing when writing to them.
>
> This patch adds a new bt_sock_wait_unsuspend helper function to sleep in
> the sendmsg call if the BT_SK_SUSPEND flag is set, and wake up as soon
> as it is unset. It also updates the L2CAP and RFCOMM sendmsg callbacks
> to take advantage of this new helper function.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/bluetooth.h | 1 +
> net/bluetooth/af_bluetooth.c | 38 ++++++++++++++++++++++++++++++++++++++
> net/bluetooth/l2cap_sock.c | 6 ++++++
> net/bluetooth/rfcomm/sock.c | 7 +++++--
> 4 files changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 10d43d8..3299d42 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -249,6 +249,7 @@ int bt_sock_stream_recvmsg(struct kiocb *iocb, struct socket *sock,
> uint bt_sock_poll(struct file *file, struct socket *sock, poll_table *wait);
> int bt_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
> int bt_sock_wait_state(struct sock *sk, int state, unsigned long timeo);
> +int bt_sock_wait_unsuspend(struct sock *sk, unsigned long flags);
I am not a huge fan of the term unsuspend. Maybe wait_ready is better here.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 2/3] Bluetooth: Fix responding to invalid L2CAP signaling commands
From: Marcel Holtmann @ 2013-09-14 1:42 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1379061827-8595-3-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> When we have an LE link we should not respond to any data on the BR/EDR
> L2CAP signaling channel (0x0001) and vice-versa when we have a BR/EDR
> link we should not respond to LE L2CAP (CID 0x0005) signaling commands.
> This patch fixes this issue by checking for a valid link type and
> ignores data if it is wrong.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/l2cap_core.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index ea3792f..1d03644 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -5297,6 +5297,7 @@ static inline int l2cap_le_sig_cmd(struct l2cap_conn *conn,
> static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
> struct sk_buff *skb)
> {
> + struct hci_conn *hcon = conn->hcon;
> u8 *data = skb->data;
> int len = skb->len;
> struct l2cap_cmd_hdr cmd;
> @@ -5304,6 +5305,9 @@ static inline void l2cap_le_sig_channel(struct l2cap_conn *conn,
>
> l2cap_raw_recv(conn, skb);
>
> + if (hcon->type != LE_LINK)
> + return;
> +
However I do have a question here. Can we just drop the packet or should we even in this case return invalid CID. What does the core spec. recommend in this case?
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/3] Bluetooth: Fix L2CAP Disconnect response for unknown CID
From: Marcel Holtmann @ 2013-09-14 1:40 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1379061827-8595-2-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> If we receive an L2CAP Disconnect Request for an unknown CID we should
> not just silently drop it but reply with a proper Command Reject
> response. This patch fixes this by ensuring that the disconnect handler
> returns a proper error instead of 0 and will cause the function caller
> to send the right response.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> net/bluetooth/l2cap_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index b3bb7bc..ea3792f 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -4206,7 +4206,7 @@ static inline int l2cap_disconnect_req(struct l2cap_conn *conn,
> chan = __l2cap_get_chan_by_scid(conn, dcid);
> if (!chan) {
> mutex_unlock(&conn->chan_lock);
> - return 0;
> + return -EINVAL;
> }
the only problem that I have here is that we end up with the wrong error code. The L2CAP core still contains this comment:
/* FIXME: Map err to a valid reason */
And now we default to L2CAP_REJ_NOT_UNDERSTOOD, but in this case it should be L2CAP_REJ_INVALID_CID.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/2] Bluetooth: Add synchronization train parameters reading support
From: Marcel Holtmann @ 2013-09-14 1:36 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1379061602-8290-2-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> This patch adds support for reading the synchronization train parameters
> for controllers that support the feature. Since the feature is
> detectable through the local features page 2, which is retreived only in
> stage 3 of the HCI init sequence, there is no other option than to add a
> fourth stage to the init sequence.
>
> For now the patch doesn't yet add storing of the parameters, but it is
> nevertheless convenient to have around to see what kind of parameters
> various controllers use by default (analyzable e.g. with the btmon user
> space tool).
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/hci.h | 2 ++
> net/bluetooth/hci_core.c | 15 ++++++++++++++-
> 2 files changed, 16 insertions(+), 1 deletion(-)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 2/2] Bluetooth: Add event mask page 2 setting support
From: Marcel Holtmann @ 2013-09-14 1:35 UTC (permalink / raw)
To: johan.hedberg; +Cc: linux-bluetooth
In-Reply-To: <1379061602-8290-3-git-send-email-johan.hedberg@gmail.com>
Hi Johan,
> For those controller that support the HCI_Set_Event_Mask_Page_2 command
> we should include it in the init sequence. This patch implements sending
> of the command and enables the events in it based on supported features
> (currently only CSB is checked).
>
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
> include/net/bluetooth/hci.h | 2 ++
> net/bluetooth/hci_core.c | 32 ++++++++++++++++++++++++++++++++
> 2 files changed, 34 insertions(+)
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index 63cce97..57aa408 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -827,6 +827,8 @@ struct hci_rp_read_inq_rsp_tx_power {
> __s8 tx_power;
> } __packed;
>
> +#define HCI_OP_SET_EVENT_MASK_PAGE_2 0x0c63
> +
I am a little bit torn here. We define the struct for event mask, but never use it. Now we only define the opcode. So either we add the struct here for completeness sake or you add a cleanup patch that removes the event mask struct.
Otherwise, the patch is fine.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH v4] Bluetooth: btmrvl: add calibration data download support
From: Marcel Holtmann @ 2013-09-14 1:15 UTC (permalink / raw)
To: Bing Zhao
Cc: linux-bluetooth, Gustavo Padovan, Johan Hedberg, linux-wireless,
Mike Frysinger, Hyuckjoo Lee, Amitkumar Karwar
In-Reply-To: <1379115162-10194-1-git-send-email-bzhao@marvell.com>
Hi Bing,
> A text file containing calibration data in hex format can
> be provided at following path:
>
> /lib/firmware/mrvl/sd8797_caldata.conf
>
> The data will be downloaded to firmware during initialization.
>
> Reviewed-by: Mike Frysinger <vapier@chromium.org>
> Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> Signed-off-by: Hyuckjoo Lee <hyuckjoo.lee@samsung.com>
> ---
> v2: Remove module parameter. The calibration data will be downloaded
> only when the device speicific data file is provided.
> (Marcel Holtmann)
> v3: Fix crash (misaligned memory access) on ARM
> v4: Simplify white space parsing and save some CPU cycles (Mike Frysinger)
>
> drivers/bluetooth/btmrvl_drv.h | 10 ++-
> drivers/bluetooth/btmrvl_main.c | 140 +++++++++++++++++++++++++++++++++++++++-
> drivers/bluetooth/btmrvl_sdio.c | 9 ++-
> drivers/bluetooth/btmrvl_sdio.h | 2 +
> 4 files changed, 157 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
> index 27068d1..5ef5e84 100644
> --- a/drivers/bluetooth/btmrvl_drv.h
> +++ b/drivers/bluetooth/btmrvl_drv.h
> @@ -23,6 +23,8 @@
> #include <linux/bitops.h>
> #include <linux/slab.h>
> #include <net/bluetooth/bluetooth.h>
> +#include <linux/ctype.h>
> +#include <linux/firmware.h>
>
> #define BTM_HEADER_LEN 4
> #define BTM_UPLD_SIZE 2312
> @@ -41,6 +43,8 @@ struct btmrvl_thread {
> struct btmrvl_device {
> void *card;
> struct hci_dev *hcidev;
> + struct device *dev;
> + const char *cal_data;
>
> u8 dev_type;
>
> @@ -91,6 +95,7 @@ struct btmrvl_private {
> #define BT_CMD_HOST_SLEEP_CONFIG 0x59
> #define BT_CMD_HOST_SLEEP_ENABLE 0x5A
> #define BT_CMD_MODULE_CFG_REQ 0x5B
> +#define BT_CMD_LOAD_CONFIG_DATA 0x61
>
> /* Sub-commands: Module Bringup/Shutdown Request/Response */
> #define MODULE_BRINGUP_REQ 0xF1
> @@ -116,10 +121,13 @@ struct btmrvl_private {
> #define PS_SLEEP 0x01
> #define PS_AWAKE 0x00
>
> +#define BT_CMD_DATA_SIZE 32
> +#define BT_CAL_DATA_SIZE 28
> +
> struct btmrvl_cmd {
> __le16 ocf_ogf;
> u8 length;
> - u8 data[4];
> + u8 data[BT_CMD_DATA_SIZE];
> } __packed;
>
> struct btmrvl_event {
> diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
> index 9a9f518..77e940e 100644
> --- a/drivers/bluetooth/btmrvl_main.c
> +++ b/drivers/bluetooth/btmrvl_main.c
> @@ -57,8 +57,9 @@ bool btmrvl_check_evtpkt(struct btmrvl_private *priv, struct sk_buff *skb)
> ocf = hci_opcode_ocf(opcode);
> ogf = hci_opcode_ogf(opcode);
>
> - if (ocf == BT_CMD_MODULE_CFG_REQ &&
> - priv->btmrvl_dev.sendcmdflag) {
> + if ((ocf == BT_CMD_MODULE_CFG_REQ ||
> + ocf == BT_CMD_LOAD_CONFIG_DATA) &&
> + priv->btmrvl_dev.sendcmdflag) {
> priv->btmrvl_dev.sendcmdflag = false;
> priv->adapter->cmd_complete = true;
> wake_up_interruptible(&priv->adapter->cmd_wait_q);
> @@ -552,6 +553,132 @@ static int btmrvl_service_main_thread(void *data)
> return 0;
> }
>
> +static int btmrvl_parse_cal_cfg(const u8 *src, u32 len, u8 *dst, u32 dst_size)
> +{
> + const u8 *s = src;
> + u8 *d = dst;
> + int ret;
> + u8 tmp[3];
> +
> + tmp[2] = '\0';
> + while ((s - src) < len) {
> + if (isspace(*s)) {
> + s++;
> + continue;
> + }
> +
> + if (isxdigit(*s)) {
> + if ((d - dst) >= dst_size) {
> + BT_ERR("calibration data file too big!!!");
> + return -EINVAL;
> + }
> +
> + memcpy(tmp, s, 2);
> +
> + ret = kstrtou8(tmp, 16, d++);
> + if (ret < 0)
> + return ret;
> +
> + s += 2;
> + } else {
> + s++;
> + }
> + }
> + if (d == dst)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int btmrvl_load_cal_data(struct btmrvl_private *priv,
> + u8 *config_data)
> +{
> + struct sk_buff *skb;
> + struct btmrvl_cmd *cmd;
> + int i;
> +
> + skb = bt_skb_alloc(sizeof(*cmd), GFP_ATOMIC);
> + if (!skb)
> + return -ENOMEM;
> +
> + cmd = (struct btmrvl_cmd *)skb->data;
> + cmd->ocf_ogf =
> + cpu_to_le16(hci_opcode_pack(OGF, BT_CMD_LOAD_CONFIG_DATA));
> + cmd->length = BT_CMD_DATA_SIZE;
> + cmd->data[0] = 0x00;
> + cmd->data[1] = 0x00;
> + cmd->data[2] = 0x00;
> + cmd->data[3] = BT_CMD_DATA_SIZE - 4;
> +
> + /* swap cal-data bytes */
> + for (i = 4; i < BT_CMD_DATA_SIZE; i++)
> + cmd->data[i] = config_data[(i/4)*8 - 1 - i];
> +
> + bt_cb(skb)->pkt_type = MRVL_VENDOR_PKT;
> + skb_put(skb, sizeof(*cmd));
> + skb->dev = (void *)priv->btmrvl_dev.hcidev;
> + skb_queue_head(&priv->adapter->tx_queue, skb);
> + priv->btmrvl_dev.sendcmdflag = true;
> + priv->adapter->cmd_complete = false;
since the Bluetooth HCI core got ->setup() support with proper synchronous HCI request handling available for every single driver (see the Intel support in btusb.c), why not start using that with this driver as well.
Regards
Marcel
^ permalink raw reply
* [PATCH v4] Bluetooth: btmrvl: add calibration data download support
From: Bing Zhao @ 2013-09-13 23:32 UTC (permalink / raw)
To: linux-bluetooth
Cc: Marcel Holtmann, Gustavo Padovan, Johan Hedberg, linux-wireless,
Mike Frysinger, Hyuckjoo Lee, Bing Zhao, Amitkumar Karwar
From: Amitkumar Karwar <akarwar@marvell.com>
A text file containing calibration data in hex format can
be provided at following path:
/lib/firmware/mrvl/sd8797_caldata.conf
The data will be downloaded to firmware during initialization.
Reviewed-by: Mike Frysinger <vapier@chromium.org>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
Signed-off-by: Hyuckjoo Lee <hyuckjoo.lee@samsung.com>
---
v2: Remove module parameter. The calibration data will be downloaded
only when the device speicific data file is provided.
(Marcel Holtmann)
v3: Fix crash (misaligned memory access) on ARM
v4: Simplify white space parsing and save some CPU cycles (Mike Frysinger)
drivers/bluetooth/btmrvl_drv.h | 10 ++-
drivers/bluetooth/btmrvl_main.c | 140 +++++++++++++++++++++++++++++++++++++++-
drivers/bluetooth/btmrvl_sdio.c | 9 ++-
drivers/bluetooth/btmrvl_sdio.h | 2 +
4 files changed, 157 insertions(+), 4 deletions(-)
diff --git a/drivers/bluetooth/btmrvl_drv.h b/drivers/bluetooth/btmrvl_drv.h
index 27068d1..5ef5e84 100644
--- a/drivers/bluetooth/btmrvl_drv.h
+++ b/drivers/bluetooth/btmrvl_drv.h
@@ -23,6 +23,8 @@
#include <linux/bitops.h>
#include <linux/slab.h>
#include <net/bluetooth/bluetooth.h>
+#include <linux/ctype.h>
+#include <linux/firmware.h>
#define BTM_HEADER_LEN 4
#define BTM_UPLD_SIZE 2312
@@ -41,6 +43,8 @@ struct btmrvl_thread {
struct btmrvl_device {
void *card;
struct hci_dev *hcidev;
+ struct device *dev;
+ const char *cal_data;
u8 dev_type;
@@ -91,6 +95,7 @@ struct btmrvl_private {
#define BT_CMD_HOST_SLEEP_CONFIG 0x59
#define BT_CMD_HOST_SLEEP_ENABLE 0x5A
#define BT_CMD_MODULE_CFG_REQ 0x5B
+#define BT_CMD_LOAD_CONFIG_DATA 0x61
/* Sub-commands: Module Bringup/Shutdown Request/Response */
#define MODULE_BRINGUP_REQ 0xF1
@@ -116,10 +121,13 @@ struct btmrvl_private {
#define PS_SLEEP 0x01
#define PS_AWAKE 0x00
+#define BT_CMD_DATA_SIZE 32
+#define BT_CAL_DATA_SIZE 28
+
struct btmrvl_cmd {
__le16 ocf_ogf;
u8 length;
- u8 data[4];
+ u8 data[BT_CMD_DATA_SIZE];
} __packed;
struct btmrvl_event {
diff --git a/drivers/bluetooth/btmrvl_main.c b/drivers/bluetooth/btmrvl_main.c
index 9a9f518..77e940e 100644
--- a/drivers/bluetooth/btmrvl_main.c
+++ b/drivers/bluetooth/btmrvl_main.c
@@ -57,8 +57,9 @@ bool btmrvl_check_evtpkt(struct btmrvl_private *priv, struct sk_buff *skb)
ocf = hci_opcode_ocf(opcode);
ogf = hci_opcode_ogf(opcode);
- if (ocf == BT_CMD_MODULE_CFG_REQ &&
- priv->btmrvl_dev.sendcmdflag) {
+ if ((ocf == BT_CMD_MODULE_CFG_REQ ||
+ ocf == BT_CMD_LOAD_CONFIG_DATA) &&
+ priv->btmrvl_dev.sendcmdflag) {
priv->btmrvl_dev.sendcmdflag = false;
priv->adapter->cmd_complete = true;
wake_up_interruptible(&priv->adapter->cmd_wait_q);
@@ -552,6 +553,132 @@ static int btmrvl_service_main_thread(void *data)
return 0;
}
+static int btmrvl_parse_cal_cfg(const u8 *src, u32 len, u8 *dst, u32 dst_size)
+{
+ const u8 *s = src;
+ u8 *d = dst;
+ int ret;
+ u8 tmp[3];
+
+ tmp[2] = '\0';
+ while ((s - src) < len) {
+ if (isspace(*s)) {
+ s++;
+ continue;
+ }
+
+ if (isxdigit(*s)) {
+ if ((d - dst) >= dst_size) {
+ BT_ERR("calibration data file too big!!!");
+ return -EINVAL;
+ }
+
+ memcpy(tmp, s, 2);
+
+ ret = kstrtou8(tmp, 16, d++);
+ if (ret < 0)
+ return ret;
+
+ s += 2;
+ } else {
+ s++;
+ }
+ }
+ if (d == dst)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int btmrvl_load_cal_data(struct btmrvl_private *priv,
+ u8 *config_data)
+{
+ struct sk_buff *skb;
+ struct btmrvl_cmd *cmd;
+ int i;
+
+ skb = bt_skb_alloc(sizeof(*cmd), GFP_ATOMIC);
+ if (!skb)
+ return -ENOMEM;
+
+ cmd = (struct btmrvl_cmd *)skb->data;
+ cmd->ocf_ogf =
+ cpu_to_le16(hci_opcode_pack(OGF, BT_CMD_LOAD_CONFIG_DATA));
+ cmd->length = BT_CMD_DATA_SIZE;
+ cmd->data[0] = 0x00;
+ cmd->data[1] = 0x00;
+ cmd->data[2] = 0x00;
+ cmd->data[3] = BT_CMD_DATA_SIZE - 4;
+
+ /* swap cal-data bytes */
+ for (i = 4; i < BT_CMD_DATA_SIZE; i++)
+ cmd->data[i] = config_data[(i/4)*8 - 1 - i];
+
+ bt_cb(skb)->pkt_type = MRVL_VENDOR_PKT;
+ skb_put(skb, sizeof(*cmd));
+ skb->dev = (void *)priv->btmrvl_dev.hcidev;
+ skb_queue_head(&priv->adapter->tx_queue, skb);
+ priv->btmrvl_dev.sendcmdflag = true;
+ priv->adapter->cmd_complete = false;
+
+ print_hex_dump_bytes("Calibration data: ",
+ DUMP_PREFIX_OFFSET, cmd->data, BT_CMD_DATA_SIZE);
+
+ wake_up_interruptible(&priv->main_thread.wait_q);
+ if (!wait_event_interruptible_timeout(priv->adapter->cmd_wait_q,
+ priv->adapter->cmd_complete,
+ msecs_to_jiffies(WAIT_UNTIL_CMD_RESP))) {
+ BT_ERR("Timeout while loading calibration data");
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
+static int
+btmrvl_process_cal_cfg(struct btmrvl_private *priv, u8 *data, u32 size)
+{
+ u8 cal_data[BT_CAL_DATA_SIZE];
+ int ret;
+
+ ret = btmrvl_parse_cal_cfg(data, size, cal_data, sizeof(cal_data));
+ if (ret)
+ return ret;
+
+ ret = btmrvl_load_cal_data(priv, cal_data);
+ if (ret) {
+ BT_ERR("Fail to load calibrate data");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int btmrvl_cal_data_config(struct btmrvl_private *priv)
+{
+ const struct firmware *cfg;
+ int ret;
+ const char *cal_data = priv->btmrvl_dev.cal_data;
+
+ if (!cal_data)
+ return 0;
+
+ ret = request_firmware(&cfg, cal_data, priv->btmrvl_dev.dev);
+ if (ret < 0) {
+ BT_DBG("Failed to get %s file, skipping cal data download",
+ cal_data);
+ ret = 0;
+ goto done;
+ }
+
+ ret = btmrvl_process_cal_cfg(priv, (u8 *)cfg->data, cfg->size);
+done:
+ if (cfg)
+ release_firmware(cfg);
+
+ return ret;
+}
+
int btmrvl_register_hdev(struct btmrvl_private *priv)
{
struct hci_dev *hdev = NULL;
@@ -583,12 +710,21 @@ int btmrvl_register_hdev(struct btmrvl_private *priv)
goto err_hci_register_dev;
}
+ ret = btmrvl_cal_data_config(priv);
+ if (ret) {
+ BT_ERR("Set cal data failed");
+ goto err_cal_data_config;
+ }
+
#ifdef CONFIG_DEBUG_FS
btmrvl_debugfs_init(hdev);
#endif
return 0;
+err_cal_data_config:
+ hci_unregister_dev(hdev);
+
err_hci_register_dev:
hci_free_dev(hdev);
diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 00da6df..af7f48d 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -18,7 +18,6 @@
* this warranty disclaimer.
**/
-#include <linux/firmware.h>
#include <linux/slab.h>
#include <linux/mmc/sdio_ids.h>
@@ -102,6 +101,7 @@ static const struct btmrvl_sdio_card_reg btmrvl_reg_88xx = {
static const struct btmrvl_sdio_device btmrvl_sdio_sd8688 = {
.helper = "mrvl/sd8688_helper.bin",
.firmware = "mrvl/sd8688.bin",
+ .cal_data = NULL,
.reg = &btmrvl_reg_8688,
.sd_blksz_fw_dl = 64,
};
@@ -109,6 +109,7 @@ static const struct btmrvl_sdio_device btmrvl_sdio_sd8688 = {
static const struct btmrvl_sdio_device btmrvl_sdio_sd8787 = {
.helper = NULL,
.firmware = "mrvl/sd8787_uapsta.bin",
+ .cal_data = NULL,
.reg = &btmrvl_reg_87xx,
.sd_blksz_fw_dl = 256,
};
@@ -116,6 +117,7 @@ static const struct btmrvl_sdio_device btmrvl_sdio_sd8787 = {
static const struct btmrvl_sdio_device btmrvl_sdio_sd8797 = {
.helper = NULL,
.firmware = "mrvl/sd8797_uapsta.bin",
+ .cal_data = "mrvl/sd8797_caldata.conf",
.reg = &btmrvl_reg_87xx,
.sd_blksz_fw_dl = 256,
};
@@ -123,6 +125,7 @@ static const struct btmrvl_sdio_device btmrvl_sdio_sd8797 = {
static const struct btmrvl_sdio_device btmrvl_sdio_sd8897 = {
.helper = NULL,
.firmware = "mrvl/sd8897_uapsta.bin",
+ .cal_data = NULL,
.reg = &btmrvl_reg_88xx,
.sd_blksz_fw_dl = 256,
};
@@ -1006,6 +1009,7 @@ static int btmrvl_sdio_probe(struct sdio_func *func,
struct btmrvl_sdio_device *data = (void *) id->driver_data;
card->helper = data->helper;
card->firmware = data->firmware;
+ card->cal_data = data->cal_data;
card->reg = data->reg;
card->sd_blksz_fw_dl = data->sd_blksz_fw_dl;
}
@@ -1034,6 +1038,8 @@ static int btmrvl_sdio_probe(struct sdio_func *func,
}
card->priv = priv;
+ priv->btmrvl_dev.dev = &card->func->dev;
+ priv->btmrvl_dev.cal_data = card->cal_data;
/* Initialize the interface specific function pointers */
priv->hw_host_to_card = btmrvl_sdio_host_to_card;
@@ -1222,4 +1228,5 @@ MODULE_FIRMWARE("mrvl/sd8688_helper.bin");
MODULE_FIRMWARE("mrvl/sd8688.bin");
MODULE_FIRMWARE("mrvl/sd8787_uapsta.bin");
MODULE_FIRMWARE("mrvl/sd8797_uapsta.bin");
+MODULE_FIRMWARE("mrvl/sd8797_caldata.conf");
MODULE_FIRMWARE("mrvl/sd8897_uapsta.bin");
diff --git a/drivers/bluetooth/btmrvl_sdio.h b/drivers/bluetooth/btmrvl_sdio.h
index 43d35a6..6872d9e 100644
--- a/drivers/bluetooth/btmrvl_sdio.h
+++ b/drivers/bluetooth/btmrvl_sdio.h
@@ -85,6 +85,7 @@ struct btmrvl_sdio_card {
u32 ioport;
const char *helper;
const char *firmware;
+ const char *cal_data;
const struct btmrvl_sdio_card_reg *reg;
u16 sd_blksz_fw_dl;
u8 rx_unit;
@@ -94,6 +95,7 @@ struct btmrvl_sdio_card {
struct btmrvl_sdio_device {
const char *helper;
const char *firmware;
+ const char *cal_data;
const struct btmrvl_sdio_card_reg *reg;
u16 sd_blksz_fw_dl;
};
--
1.8.0
^ permalink raw reply related
* [PATCH 6/6] obexd: Remove msg from MAP session structure
From: Christian Fetzer @ 2013-09-13 15:28 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379086116-22617-1-git-send-email-christian.fetzer@oss.bmw-carit.de>
From: Christian Fetzer <christian.fetzer@bmw-carit.de>
The D-Bus message for a pending method call is now stored in the
pending_request struct.
---
obexd/client/map.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/obexd/client/map.c b/obexd/client/map.c
index 8b56143..9fe872d 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -95,7 +95,6 @@ static const char * const filter_list[] = {
struct map_data {
struct obc_session *session;
- DBusMessage *msg;
GHashTable *messages;
int16_t mas_instance_id;
uint8_t supported_message_types;
--
1.8.3.4
^ permalink raw reply related
* [PATCH 5/6] obexd: Use pending request in UpdateInbox
From: Christian Fetzer @ 2013-09-13 15:28 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1379086116-22617-1-git-send-email-christian.fetzer@oss.bmw-carit.de>
From: Christian Fetzer <christian.fetzer@bmw-carit.de>
diff --git a/obexd/client/map.c b/obexd/client/map.c
index 290cfee..8b56143 100644
--- a/obexd/client/map.c
+++ b/obexd/client/map.c
@@ -1559,21 +1559,21 @@ static void update_inbox_cb(struct obc_session *session,
struct obc_transfer *transfer,
GError *err, void *user_data)
{
- struct map_data *map = user_data;
+ struct pending_request *request = user_data;
DBusMessage *reply;
if (err != NULL) {
- reply = g_dbus_create_error(map->msg,
+ reply = g_dbus_create_error(request->msg,
ERROR_INTERFACE ".Failed",
"%s", err->message);
goto done;
}
- reply = dbus_message_new_method_return(map->msg);
+ reply = dbus_message_new_method_return(request->msg);
done:
g_dbus_send_message(conn, reply);
- dbus_message_unref(map->msg);
+ pending_request_free(request);
}
static DBusMessage *map_update_inbox(DBusConnection *connection,
@@ -1584,6 +1584,7 @@ static DBusMessage *map_update_inbox(DBusConnection *connection,
char contents[2];
struct obc_transfer *transfer;
GError *err = NULL;
+ struct pending_request *request;
contents[0] = FILLER_BYTE;
contents[1] = '\0';
@@ -1594,11 +1595,13 @@ static DBusMessage *map_update_inbox(DBusConnection *connection,
if (transfer == NULL)
goto fail;
+ request = pending_request_new(map, message);
+
if (!obc_session_queue(map->session, transfer, update_inbox_cb,
- map, &err))
+ request, &err)) {
+ pending_request_free(request);
goto fail;
-
- map->msg = dbus_message_ref(message);
+ }
return NULL;
--
1.8.3.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