* [PATCH v1 1/4] Bluetooth: Fix HCI UART HCI_UART_PROTO_SET locking
2016-09-07 14:15 [PATCH v1 0/4] Bluetooth HCI LDISC and BCSP fixes Dean Jenkins
@ 2016-09-07 14:15 ` Dean Jenkins
2016-09-07 14:18 ` Marcel Holtmann
2016-09-07 14:15 ` [PATCH v1 2/4] Bluetooth: prevent a race condition on hci_uart_tty_ioctl() call Dean Jenkins
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Dean Jenkins @ 2016-09-07 14:15 UTC (permalink / raw)
To: Dean Jenkins, Marcel Holtmann, Gustavo Padovan, Johan Hedberg
Cc: linux-bluetooth
From: Vignesh Raman <Vignesh_Raman@mentor.com>
Here is a NULL pointer dereference that causes a Bluetooth crash:
(hci_uart_tty_receive+0x0/0x84 [hci_uart]) from (flush_to_ldisc+0x104/0x190)
(flush_to_ldisc+0x0/0x190) from (tty_flip_buffer_push+0x50/0x5c)
(tty_flip_buffer_push+0x0/0x5c) from (imx_rxint+0x228/0x23c)
(imx_rxint+0x0/0x23c) from (imx_int+0xa8/0x174)
(imx_int+0x0/0x174) from (handle_irq_event_percpu+0x90/0x280)
(handle_irq_event_percpu+0x0/0x280) from (handle_irq_event+0x44/0x64)
(handle_irq_event+0x0/0x64) from (handle_fasteoi_irq+0xc0/0x108)
(handle_fasteoi_irq+0x0/0x108) from (generic_handle_irq+0x28/0x38)
(generic_handle_irq+0x0/0x38) from (handle_IRQ+0x6c/0x94)
(handle_IRQ+0x0/0x94) from (gic_handle_irq+0x44/0x68)
(gic_handle_irq+0x0/0x68) from (__irq_svc+0x40/0x70)
(bcsp_open+0x0/0xcc [hci_uart]) from (hci_uart_tty_ioctl+0xa8/0x238 [hci_uart])
(hci_uart_tty_ioctl+0x0/0x238 [hci_uart]) from (tty_ioctl+0xa88/0xae8)
(tty_ioctl+0x0/0xae8) from (vfs_ioctl+0x30/0x44)
(vfs_ioctl+0x0/0x44) from (do_vfs_ioctl+0x53c/0x590)
(do_vfs_ioctl+0x0/0x590) from (sys_ioctl+0x40/0x68)
(sys_ioctl+0x0/0x68) from (ret_fast_syscall+0x0/0x30)
It can be seen that bcsp_open() is pre-empted by an interrupt relating
to reception of UART characters. bcsp_open() is called from
hci_uart_set_proto() which is called from hci_uart_tty_ioctl() due to
HCIUARTSETPROTO handling.
The crash occurs in hci_uart_tty_receive() and analysis shows:
if (!test_bit(HCI_UART_PROTO_SET, &hu->flags))
return;
spin_lock(&hu->rx_lock);
hu->proto->recv(hu, (void *) data, count);
The crash occurs in the dereference hu->proto->recv. Presumably no recv
function has been set and a NULL pointer dereference occurs.
Note that the flag HCI_UART_PROTO_SET being clear should prevent the failure
but in fact fails. This indicates that HCI_UART_PROTO_SET is in the set state.
The HCI_UART_PROTO_SET locking fails because HCI_UART_PROTO_SET is put into
the set state in hci_uart_tty_ioctl() BEFORE calling hci_uart_set_proto()
to set the recv function pointer. Therefore, there is a race condition.
The solution is to set the flag HCI_UART_PROTO_SET after successfully
calling hci_uart_set_proto() in the HCIUARTSETPROTO handling in
hci_uart_tty_ioctl(). This will prevent hci_uart_tty_receive() from
performing the recv function pointer dereference until hci_uart_set_proto()
has set the recv function pointer.
Signed-off-by: Vignesh Raman <Vignesh_Raman@mentor.com>
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Mark Craske <Mark_Craske@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index dda9739..1e338d5 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -695,12 +695,12 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
switch (cmd) {
case HCIUARTSETPROTO:
- if (!test_and_set_bit(HCI_UART_PROTO_SET, &hu->flags)) {
+ if (!test_bit(HCI_UART_PROTO_SET, &hu->flags)) {
err = hci_uart_set_proto(hu, arg);
- if (err) {
- clear_bit(HCI_UART_PROTO_SET, &hu->flags);
+ if (!err)
+ set_bit(HCI_UART_PROTO_SET, &hu->flags);
+ else
return err;
- }
} else
return -EBUSY;
break;
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v1 1/4] Bluetooth: Fix HCI UART HCI_UART_PROTO_SET locking
2016-09-07 14:15 ` [PATCH v1 1/4] Bluetooth: Fix HCI UART HCI_UART_PROTO_SET locking Dean Jenkins
@ 2016-09-07 14:18 ` Marcel Holtmann
0 siblings, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2016-09-07 14:18 UTC (permalink / raw)
To: Dean Jenkins; +Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth
Hi Dean,
> Here is a NULL pointer dereference that causes a Bluetooth crash:
>
> (hci_uart_tty_receive+0x0/0x84 [hci_uart]) from (flush_to_ldisc+0x104/0x190)
> (flush_to_ldisc+0x0/0x190) from (tty_flip_buffer_push+0x50/0x5c)
> (tty_flip_buffer_push+0x0/0x5c) from (imx_rxint+0x228/0x23c)
> (imx_rxint+0x0/0x23c) from (imx_int+0xa8/0x174)
> (imx_int+0x0/0x174) from (handle_irq_event_percpu+0x90/0x280)
> (handle_irq_event_percpu+0x0/0x280) from (handle_irq_event+0x44/0x64)
> (handle_irq_event+0x0/0x64) from (handle_fasteoi_irq+0xc0/0x108)
> (handle_fasteoi_irq+0x0/0x108) from (generic_handle_irq+0x28/0x38)
> (generic_handle_irq+0x0/0x38) from (handle_IRQ+0x6c/0x94)
> (handle_IRQ+0x0/0x94) from (gic_handle_irq+0x44/0x68)
> (gic_handle_irq+0x0/0x68) from (__irq_svc+0x40/0x70)
> (bcsp_open+0x0/0xcc [hci_uart]) from (hci_uart_tty_ioctl+0xa8/0x238 [hci_uart])
> (hci_uart_tty_ioctl+0x0/0x238 [hci_uart]) from (tty_ioctl+0xa88/0xae8)
> (tty_ioctl+0x0/0xae8) from (vfs_ioctl+0x30/0x44)
> (vfs_ioctl+0x0/0x44) from (do_vfs_ioctl+0x53c/0x590)
> (do_vfs_ioctl+0x0/0x590) from (sys_ioctl+0x40/0x68)
> (sys_ioctl+0x0/0x68) from (ret_fast_syscall+0x0/0x30)
>
> It can be seen that bcsp_open() is pre-empted by an interrupt relating
> to reception of UART characters. bcsp_open() is called from
> hci_uart_set_proto() which is called from hci_uart_tty_ioctl() due to
> HCIUARTSETPROTO handling.
>
> The crash occurs in hci_uart_tty_receive() and analysis shows:
> if (!test_bit(HCI_UART_PROTO_SET, &hu->flags))
> return;
>
> spin_lock(&hu->rx_lock);
> hu->proto->recv(hu, (void *) data, count);
>
> The crash occurs in the dereference hu->proto->recv. Presumably no recv
> function has been set and a NULL pointer dereference occurs.
>
> Note that the flag HCI_UART_PROTO_SET being clear should prevent the failure
> but in fact fails. This indicates that HCI_UART_PROTO_SET is in the set state.
>
> The HCI_UART_PROTO_SET locking fails because HCI_UART_PROTO_SET is put into
> the set state in hci_uart_tty_ioctl() BEFORE calling hci_uart_set_proto()
> to set the recv function pointer. Therefore, there is a race condition.
>
> The solution is to set the flag HCI_UART_PROTO_SET after successfully
> calling hci_uart_set_proto() in the HCIUARTSETPROTO handling in
> hci_uart_tty_ioctl(). This will prevent hci_uart_tty_receive() from
> performing the recv function pointer dereference until hci_uart_set_proto()
> has set the recv function pointer.
>
> Signed-off-by: Vignesh Raman <Vignesh_Raman@mentor.com>
> Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
> Signed-off-by: Mark Craske <Mark_Craske@mentor.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index dda9739..1e338d5 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -695,12 +695,12 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
>
> switch (cmd) {
> case HCIUARTSETPROTO:
> - if (!test_and_set_bit(HCI_UART_PROTO_SET, &hu->flags)) {
> + if (!test_bit(HCI_UART_PROTO_SET, &hu->flags)) {
> err = hci_uart_set_proto(hu, arg);
> - if (err) {
> - clear_bit(HCI_UART_PROTO_SET, &hu->flags);
> + if (!err)
> + set_bit(HCI_UART_PROTO_SET, &hu->flags);
> + else
> return err;
> - }
can we flip this around.
if (err)
return err;
/* Comment why this is correct thing to do */
set_bit(HCI_UART_PROTO_SET, &hu->flags);
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 2/4] Bluetooth: prevent a race condition on hci_uart_tty_ioctl() call
2016-09-07 14:15 [PATCH v1 0/4] Bluetooth HCI LDISC and BCSP fixes Dean Jenkins
2016-09-07 14:15 ` [PATCH v1 1/4] Bluetooth: Fix HCI UART HCI_UART_PROTO_SET locking Dean Jenkins
@ 2016-09-07 14:15 ` Dean Jenkins
2016-09-07 14:20 ` Marcel Holtmann
2016-09-07 14:15 ` [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer Dean Jenkins
2016-09-07 14:15 ` [PATCH v1 4/4] Bluetooth: Prevent scheduling of work after hci_uart_tty_close() Dean Jenkins
3 siblings, 1 reply; 10+ messages in thread
From: Dean Jenkins @ 2016-09-07 14:15 UTC (permalink / raw)
To: Dean Jenkins, Marcel Holtmann, Gustavo Padovan, Johan Hedberg
Cc: linux-bluetooth
From: Vignesh Raman <Vignesh_Raman@mentor.com>
Add global mutex lock to prevent reentrancy in the hci_uart_tty_ioctl()
ioctl handling. This prevents concurrency of all function calls within
hci_uart_tty_ioctl handling including hci_uart_set_proto().
Also removed multiple return statements in hci_uart_tty_ioctl call and
added a single return statement since the mutex needs to be unlocked
before exiting the function.
Signed-off-by: Vignesh Raman <Vignesh_Raman@mentor.com>
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 43 ++++++++++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index 1e338d5..cf1883d 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -51,6 +51,8 @@
#define VERSION "2.3"
+static DEFINE_MUTEX(ioctl_mutex);
+
static const struct hci_uart_proto *hup[HCI_UART_MAX_PROTO];
int hci_uart_register_proto(const struct hci_uart_proto *p)
@@ -685,7 +687,7 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
unsigned int cmd, unsigned long arg)
{
struct hci_uart *hu = tty->disc_data;
- int err = 0;
+ int result = 0;
BT_DBG("");
@@ -693,45 +695,52 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
if (!hu)
return -EBADF;
+ mutex_lock(&ioctl_mutex);
switch (cmd) {
case HCIUARTSETPROTO:
if (!test_bit(HCI_UART_PROTO_SET, &hu->flags)) {
- err = hci_uart_set_proto(hu, arg);
- if (!err)
+ result = hci_uart_set_proto(hu, arg);
+ if (!result)
set_bit(HCI_UART_PROTO_SET, &hu->flags);
- else
- return err;
} else
- return -EBUSY;
+ result = -EBUSY;
break;
case HCIUARTGETPROTO:
if (test_bit(HCI_UART_PROTO_SET, &hu->flags))
- return hu->proto->id;
- return -EUNATCH;
+ result = hu->proto->id;
+ else
+ result = -EUNATCH;
+ break;
case HCIUARTGETDEVICE:
if (test_bit(HCI_UART_REGISTERED, &hu->flags))
- return hu->hdev->id;
- return -EUNATCH;
+ result = hu->hdev->id;
+ else
+ result = -EUNATCH;
+ break;
case HCIUARTSETFLAGS:
if (test_bit(HCI_UART_PROTO_SET, &hu->flags))
- return -EBUSY;
- err = hci_uart_set_flags(hu, arg);
- if (err)
- return err;
+ result = -EBUSY;
+ else {
+ result = hci_uart_set_flags(hu, arg);
+ if (result)
+ return result;
+ }
break;
case HCIUARTGETFLAGS:
- return hu->hdev_flags;
+ result = hu->hdev_flags;
+ break;
default:
- err = n_tty_ioctl_helper(tty, file, cmd, arg);
+ result = n_tty_ioctl_helper(tty, file, cmd, arg);
break;
}
+ mutex_unlock(&ioctl_mutex);
- return err;
+ return result;
}
/*
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v1 2/4] Bluetooth: prevent a race condition on hci_uart_tty_ioctl() call
2016-09-07 14:15 ` [PATCH v1 2/4] Bluetooth: prevent a race condition on hci_uart_tty_ioctl() call Dean Jenkins
@ 2016-09-07 14:20 ` Marcel Holtmann
0 siblings, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2016-09-07 14:20 UTC (permalink / raw)
To: Dean Jenkins; +Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth
Hi Dean,
> Add global mutex lock to prevent reentrancy in the hci_uart_tty_ioctl()
> ioctl handling. This prevents concurrency of all function calls within
> hci_uart_tty_ioctl handling including hci_uart_set_proto().
>
> Also removed multiple return statements in hci_uart_tty_ioctl call and
> added a single return statement since the mutex needs to be unlocked
> before exiting the function.
can we split this into two please. First change the error handling path and then introduce the mutex.
>
> Signed-off-by: Vignesh Raman <Vignesh_Raman@mentor.com>
> Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
> Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
> ---
> drivers/bluetooth/hci_ldisc.c | 43 ++++++++++++++++++++++++++-----------------
> 1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index 1e338d5..cf1883d 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -51,6 +51,8 @@
>
> #define VERSION "2.3"
>
> +static DEFINE_MUTEX(ioctl_mutex);
> +
> static const struct hci_uart_proto *hup[HCI_UART_MAX_PROTO];
>
> int hci_uart_register_proto(const struct hci_uart_proto *p)
> @@ -685,7 +687,7 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
> unsigned int cmd, unsigned long arg)
> {
> struct hci_uart *hu = tty->disc_data;
> - int err = 0;
> + int result = 0;
I am fine keeping this err. I think renaming it to result does not help.
>
> BT_DBG("");
>
> @@ -693,45 +695,52 @@ static int hci_uart_tty_ioctl(struct tty_struct *tty, struct file *file,
> if (!hu)
> return -EBADF;
>
> + mutex_lock(&ioctl_mutex);
> switch (cmd) {
> case HCIUARTSETPROTO:
> if (!test_bit(HCI_UART_PROTO_SET, &hu->flags)) {
> - err = hci_uart_set_proto(hu, arg);
> - if (!err)
> + result = hci_uart_set_proto(hu, arg);
> + if (!result)
> set_bit(HCI_UART_PROTO_SET, &hu->flags);
> - else
> - return err;
> } else
> - return -EBUSY;
> + result = -EBUSY;
> break;
>
> case HCIUARTGETPROTO:
> if (test_bit(HCI_UART_PROTO_SET, &hu->flags))
> - return hu->proto->id;
> - return -EUNATCH;
> + result = hu->proto->id;
> + else
> + result = -EUNATCH;
> + break;
>
> case HCIUARTGETDEVICE:
> if (test_bit(HCI_UART_REGISTERED, &hu->flags))
> - return hu->hdev->id;
> - return -EUNATCH;
> + result = hu->hdev->id;
> + else
> + result = -EUNATCH;
> + break;
>
> case HCIUARTSETFLAGS:
> if (test_bit(HCI_UART_PROTO_SET, &hu->flags))
> - return -EBUSY;
> - err = hci_uart_set_flags(hu, arg);
> - if (err)
> - return err;
> + result = -EBUSY;
> + else {
> + result = hci_uart_set_flags(hu, arg);
> + if (result)
> + return result;
> + }
> break;
>
> case HCIUARTGETFLAGS:
> - return hu->hdev_flags;
> + result = hu->hdev_flags;
> + break;
>
> default:
> - err = n_tty_ioctl_helper(tty, file, cmd, arg);
> + result = n_tty_ioctl_helper(tty, file, cmd, arg);
> break;
> }
> + mutex_unlock(&ioctl_mutex);
>
> - return err;
> + return result;
> }
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer
2016-09-07 14:15 [PATCH v1 0/4] Bluetooth HCI LDISC and BCSP fixes Dean Jenkins
2016-09-07 14:15 ` [PATCH v1 1/4] Bluetooth: Fix HCI UART HCI_UART_PROTO_SET locking Dean Jenkins
2016-09-07 14:15 ` [PATCH v1 2/4] Bluetooth: prevent a race condition on hci_uart_tty_ioctl() call Dean Jenkins
@ 2016-09-07 14:15 ` Dean Jenkins
2016-09-07 14:22 ` Marcel Holtmann
2016-09-07 14:15 ` [PATCH v1 4/4] Bluetooth: Prevent scheduling of work after hci_uart_tty_close() Dean Jenkins
3 siblings, 1 reply; 10+ messages in thread
From: Dean Jenkins @ 2016-09-07 14:15 UTC (permalink / raw)
To: Dean Jenkins, Marcel Holtmann, Gustavo Padovan, Johan Hedberg
Cc: linux-bluetooth
Send an ACK frame with the current txack value in response to
every received reliable frame unless a TX reliable frame is being
sent. This modification allows re-transmitted frames from the remote
peer to be acknowledged rather than ignored. It means that the remote
peer knows which frame number to start re-transmitting from.
Without this modification, the recovery time to a missing frame
from the remote peer was unnecessarily being extended because the
headers of the out of order reliable frames were being discarded rather
than being processed. The frame headers of received frames will
indicate whether the local peer's transmissions have been
acknowledged by the remote peer. Therefore, the local peer may
unnecessarily re-transmit despite the remote peer already indicating
that the frame had been acknowledged in out of order reliable frame.
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
---
drivers/bluetooth/hci_bcsp.c | 82 ++++++++++++++++++++++++++------------------
1 file changed, 48 insertions(+), 34 deletions(-)
diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
index d7d23ce..739ae59 100644
--- a/drivers/bluetooth/hci_bcsp.c
+++ b/drivers/bluetooth/hci_bcsp.c
@@ -485,13 +485,27 @@ static inline void bcsp_unslip_one_byte(struct bcsp_struct *bcsp, unsigned char
static void bcsp_complete_rx_pkt(struct hci_uart *hu)
{
struct bcsp_struct *bcsp = hu->priv;
- int pass_up;
+ int pass_up = 0;
if (bcsp->rx_skb->data[0] & 0x80) { /* reliable pkt */
BT_DBG("Received seqno %u from card", bcsp->rxseq_txack);
- bcsp->rxseq_txack++;
- bcsp->rxseq_txack %= 0x8;
- bcsp->txack_req = 1;
+
+ /* check the rx sequence number is as expected */
+ if ((bcsp->rx_skb->data[0] & 0x07) == bcsp->rxseq_txack) {
+ bcsp->rxseq_txack++;
+ bcsp->rxseq_txack %= 0x8;
+ } else {
+ /* handle re-transmitted packet or
+ when packet was missed */
+ BT_ERR("Out-of-order packet arrived, got %u expected %u",
+ bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
+
+ /* do not process out-of-order packet payload */
+ pass_up = 2;
+ }
+
+ /* send current txack value to all received reliable packets */
+ bcsp->txack_req = 1;
/* If needed, transmit an ack pkt */
hci_uart_tx_wakeup(hu);
@@ -500,26 +514,32 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
bcsp->rxack = (bcsp->rx_skb->data[0] >> 3) & 0x07;
BT_DBG("Request for pkt %u from card", bcsp->rxack);
+ /* handle received ACK indications,
+ including those from out-of-order packets */
bcsp_pkt_cull(bcsp);
- if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
- bcsp->rx_skb->data[0] & 0x80) {
- hci_skb_pkt_type(bcsp->rx_skb) = HCI_ACLDATA_PKT;
- pass_up = 1;
- } else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
- bcsp->rx_skb->data[0] & 0x80) {
- hci_skb_pkt_type(bcsp->rx_skb) = HCI_EVENT_PKT;
- pass_up = 1;
- } else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
- hci_skb_pkt_type(bcsp->rx_skb) = HCI_SCODATA_PKT;
- pass_up = 1;
- } else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
- !(bcsp->rx_skb->data[0] & 0x80)) {
- bcsp_handle_le_pkt(hu);
- pass_up = 0;
- } else
- pass_up = 0;
-
- if (!pass_up) {
+
+ if (pass_up != 2) {
+ if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
+ (bcsp->rx_skb->data[0] & 0x80)) {
+ bt_cb(bcsp->rx_skb)->pkt_type = HCI_ACLDATA_PKT;
+ pass_up = 1;
+ } else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
+ (bcsp->rx_skb->data[0] & 0x80)) {
+ bt_cb(bcsp->rx_skb)->pkt_type = HCI_EVENT_PKT;
+ pass_up = 1;
+ } else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
+ bt_cb(bcsp->rx_skb)->pkt_type = HCI_SCODATA_PKT;
+ pass_up = 1;
+ } else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
+ !(bcsp->rx_skb->data[0] & 0x80)) {
+ bcsp_handle_le_pkt(hu);
+ pass_up = 0;
+ } else {
+ pass_up = 0;
+ }
+ }
+
+ if (pass_up == 0) {
struct hci_event_hdr hdr;
u8 desc = (bcsp->rx_skb->data[1] & 0x0f);
@@ -544,11 +564,15 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
}
} else
kfree_skb(bcsp->rx_skb);
- } else {
+ } else if (pass_up == 1) {
/* Pull out BCSP hdr */
skb_pull(bcsp->rx_skb, 4);
hci_recv_frame(hu->hdev, bcsp->rx_skb);
+ } else {
+ /* ignore packet payload of already ACKed re-transmitted
+ packets or when a packet was missed in the BCSP window */
+ kfree_skb(bcsp->rx_skb);
}
bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
@@ -594,16 +618,6 @@ static int bcsp_recv(struct hci_uart *hu, const void *data, int count)
bcsp->rx_count = 0;
continue;
}
- if (bcsp->rx_skb->data[0] & 0x80 /* reliable pkt */
- && (bcsp->rx_skb->data[0] & 0x07) != bcsp->rxseq_txack) {
- BT_ERR("Out-of-order packet arrived, got %u expected %u",
- bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
-
- kfree_skb(bcsp->rx_skb);
- bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
- bcsp->rx_count = 0;
- continue;
- }
bcsp->rx_state = BCSP_W4_DATA;
bcsp->rx_count = (bcsp->rx_skb->data[1] >> 4) +
(bcsp->rx_skb->data[2] << 4); /* May be 0 */
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer
2016-09-07 14:15 ` [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer Dean Jenkins
@ 2016-09-07 14:22 ` Marcel Holtmann
2016-09-08 9:41 ` Dean Jenkins
0 siblings, 1 reply; 10+ messages in thread
From: Marcel Holtmann @ 2016-09-07 14:22 UTC (permalink / raw)
To: Dean Jenkins; +Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth
Hi Dean,
> Send an ACK frame with the current txack value in response to
> every received reliable frame unless a TX reliable frame is being
> sent. This modification allows re-transmitted frames from the remote
> peer to be acknowledged rather than ignored. It means that the remote
> peer knows which frame number to start re-transmitting from.
>
> Without this modification, the recovery time to a missing frame
> from the remote peer was unnecessarily being extended because the
> headers of the out of order reliable frames were being discarded rather
> than being processed. The frame headers of received frames will
> indicate whether the local peer's transmissions have been
> acknowledged by the remote peer. Therefore, the local peer may
> unnecessarily re-transmit despite the remote peer already indicating
> that the frame had been acknowledged in out of order reliable frame.
>
> Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
> ---
> drivers/bluetooth/hci_bcsp.c | 82 ++++++++++++++++++++++++++------------------
> 1 file changed, 48 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
> index d7d23ce..739ae59 100644
> --- a/drivers/bluetooth/hci_bcsp.c
> +++ b/drivers/bluetooth/hci_bcsp.c
> @@ -485,13 +485,27 @@ static inline void bcsp_unslip_one_byte(struct bcsp_struct *bcsp, unsigned char
> static void bcsp_complete_rx_pkt(struct hci_uart *hu)
> {
> struct bcsp_struct *bcsp = hu->priv;
> - int pass_up;
> + int pass_up = 0;
>
> if (bcsp->rx_skb->data[0] & 0x80) { /* reliable pkt */
> BT_DBG("Received seqno %u from card", bcsp->rxseq_txack);
> - bcsp->rxseq_txack++;
> - bcsp->rxseq_txack %= 0x8;
> - bcsp->txack_req = 1;
> +
> + /* check the rx sequence number is as expected */
> + if ((bcsp->rx_skb->data[0] & 0x07) == bcsp->rxseq_txack) {
> + bcsp->rxseq_txack++;
> + bcsp->rxseq_txack %= 0x8;
> + } else {
> + /* handle re-transmitted packet or
> + when packet was missed */
> + BT_ERR("Out-of-order packet arrived, got %u expected %u",
> + bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
> +
> + /* do not process out-of-order packet payload */
> + pass_up = 2;
> + }
> +
> + /* send current txack value to all received reliable packets */
> + bcsp->txack_req = 1;
>
> /* If needed, transmit an ack pkt */
> hci_uart_tx_wakeup(hu);
> @@ -500,26 +514,32 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
> bcsp->rxack = (bcsp->rx_skb->data[0] >> 3) & 0x07;
> BT_DBG("Request for pkt %u from card", bcsp->rxack);
>
> + /* handle received ACK indications,
> + including those from out-of-order packets */
> bcsp_pkt_cull(bcsp);
> - if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
> - bcsp->rx_skb->data[0] & 0x80) {
> - hci_skb_pkt_type(bcsp->rx_skb) = HCI_ACLDATA_PKT;
> - pass_up = 1;
> - } else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
> - bcsp->rx_skb->data[0] & 0x80) {
> - hci_skb_pkt_type(bcsp->rx_skb) = HCI_EVENT_PKT;
> - pass_up = 1;
> - } else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
> - hci_skb_pkt_type(bcsp->rx_skb) = HCI_SCODATA_PKT;
> - pass_up = 1;
> - } else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
> - !(bcsp->rx_skb->data[0] & 0x80)) {
> - bcsp_handle_le_pkt(hu);
> - pass_up = 0;
> - } else
> - pass_up = 0;
> -
> - if (!pass_up) {
> +
> + if (pass_up != 2) {
> + if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
> + (bcsp->rx_skb->data[0] & 0x80)) {
lets start using proper network subsystem coding style now.
Regards
Marcel
> + bt_cb(bcsp->rx_skb)->pkt_type = HCI_ACLDATA_PKT;
> + pass_up = 1;
> + } else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
> + (bcsp->rx_skb->data[0] & 0x80)) {
> + bt_cb(bcsp->rx_skb)->pkt_type = HCI_EVENT_PKT;
> + pass_up = 1;
> + } else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
> + bt_cb(bcsp->rx_skb)->pkt_type = HCI_SCODATA_PKT;
> + pass_up = 1;
> + } else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
> + !(bcsp->rx_skb->data[0] & 0x80)) {
> + bcsp_handle_le_pkt(hu);
> + pass_up = 0;
> + } else {
> + pass_up = 0;
> + }
> + }
> +
> + if (pass_up == 0) {
> struct hci_event_hdr hdr;
> u8 desc = (bcsp->rx_skb->data[1] & 0x0f);
>
> @@ -544,11 +564,15 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
> }
> } else
> kfree_skb(bcsp->rx_skb);
> - } else {
> + } else if (pass_up == 1) {
> /* Pull out BCSP hdr */
> skb_pull(bcsp->rx_skb, 4);
>
> hci_recv_frame(hu->hdev, bcsp->rx_skb);
> + } else {
> + /* ignore packet payload of already ACKed re-transmitted
> + packets or when a packet was missed in the BCSP window */
Also use network subsystem comment style.
> + kfree_skb(bcsp->rx_skb);
> }
>
> bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
> @@ -594,16 +618,6 @@ static int bcsp_recv(struct hci_uart *hu, const void *data, int count)
> bcsp->rx_count = 0;
> continue;
> }
> - if (bcsp->rx_skb->data[0] & 0x80 /* reliable pkt */
> - && (bcsp->rx_skb->data[0] & 0x07) != bcsp->rxseq_txack) {
> - BT_ERR("Out-of-order packet arrived, got %u expected %u",
> - bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
> -
> - kfree_skb(bcsp->rx_skb);
> - bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
> - bcsp->rx_count = 0;
> - continue;
> - }
> bcsp->rx_state = BCSP_W4_DATA;
> bcsp->rx_count = (bcsp->rx_skb->data[1] >> 4) +
> (bcsp->rx_skb->data[2] << 4); /* May be 0 */
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer
2016-09-07 14:22 ` Marcel Holtmann
@ 2016-09-08 9:41 ` Dean Jenkins
2016-09-08 10:28 ` Marcel Holtmann
0 siblings, 1 reply; 10+ messages in thread
From: Dean Jenkins @ 2016-09-08 9:41 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth
Hi Marcel,
On 07/09/16 15:22, Marcel Holtmann wrote:
> Hi Dean,
>
>> Send an ACK frame with the current txack value in response to
>> every received reliable frame unless a TX reliable frame is being
>> sent. This modification allows re-transmitted frames from the remote
>> peer to be acknowledged rather than ignored. It means that the remote
>> peer knows which frame number to start re-transmitting from.
>>
>> Without this modification, the recovery time to a missing frame
>> from the remote peer was unnecessarily being extended because the
>> headers of the out of order reliable frames were being discarded rather
>> than being processed. The frame headers of received frames will
>> indicate whether the local peer's transmissions have been
>> acknowledged by the remote peer. Therefore, the local peer may
>> unnecessarily re-transmit despite the remote peer already indicating
>> that the frame had been acknowledged in out of order reliable frame.
>>
>> Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>> Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
>> ---
>> drivers/bluetooth/hci_bcsp.c | 82 ++++++++++++++++++++++++++------------------
>> 1 file changed, 48 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
>> index d7d23ce..739ae59 100644
>> --- a/drivers/bluetooth/hci_bcsp.c
>> +++ b/drivers/bluetooth/hci_bcsp.c
>> @@ -485,13 +485,27 @@ static inline void bcsp_unslip_one_byte(struct bcsp_struct *bcsp, unsigned char
>> static void bcsp_complete_rx_pkt(struct hci_uart *hu)
>> {
>> struct bcsp_struct *bcsp = hu->priv;
>> - int pass_up;
>> + int pass_up = 0;
>>
>> if (bcsp->rx_skb->data[0] & 0x80) { /* reliable pkt */
>> BT_DBG("Received seqno %u from card", bcsp->rxseq_txack);
>> - bcsp->rxseq_txack++;
>> - bcsp->rxseq_txack %= 0x8;
>> - bcsp->txack_req = 1;
>> +
>> + /* check the rx sequence number is as expected */
>> + if ((bcsp->rx_skb->data[0] & 0x07) == bcsp->rxseq_txack) {
>> + bcsp->rxseq_txack++;
>> + bcsp->rxseq_txack %= 0x8;
>> + } else {
>> + /* handle re-transmitted packet or
>> + when packet was missed */
>> + BT_ERR("Out-of-order packet arrived, got %u expected %u",
>> + bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
>> +
>> + /* do not process out-of-order packet payload */
>> + pass_up = 2;
>> + }
>> +
>> + /* send current txack value to all received reliable packets */
>> + bcsp->txack_req = 1;
>>
>> /* If needed, transmit an ack pkt */
>> hci_uart_tx_wakeup(hu);
>> @@ -500,26 +514,32 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
>> bcsp->rxack = (bcsp->rx_skb->data[0] >> 3) & 0x07;
>> BT_DBG("Request for pkt %u from card", bcsp->rxack);
>>
>> + /* handle received ACK indications,
>> + including those from out-of-order packets */
>> bcsp_pkt_cull(bcsp);
>> - if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
>> - bcsp->rx_skb->data[0] & 0x80) {
>> - hci_skb_pkt_type(bcsp->rx_skb) = HCI_ACLDATA_PKT;
>> - pass_up = 1;
>> - } else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
>> - bcsp->rx_skb->data[0] & 0x80) {
>> - hci_skb_pkt_type(bcsp->rx_skb) = HCI_EVENT_PKT;
>> - pass_up = 1;
>> - } else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
>> - hci_skb_pkt_type(bcsp->rx_skb) = HCI_SCODATA_PKT;
>> - pass_up = 1;
>> - } else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
>> - !(bcsp->rx_skb->data[0] & 0x80)) {
>> - bcsp_handle_le_pkt(hu);
>> - pass_up = 0;
>> - } else
>> - pass_up = 0;
>> -
>> - if (!pass_up) {
>> +
>> + if (pass_up != 2) {
>> + if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
>> + (bcsp->rx_skb->data[0] & 0x80)) {
> lets start using proper network subsystem coding style now.
I think you are referring to the indentation style, right ?
I am aware of the checkpatch.pl --strict warning of
CHECK: Alignment should match open parenthesis
Would this style be acceptable so the 2nd line of arguments is one character
position to the right of the opening parenthesis of the 1st line by adding
spaces after tabs to do the alignment ?
+ if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
+ (bcsp->rx_skb->data[0] & 0x80)) {
>
> Regards
>
> Marcel
>
>> + bt_cb(bcsp->rx_skb)->pkt_type = HCI_ACLDATA_PKT;
>> + pass_up = 1;
>> + } else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
>> + (bcsp->rx_skb->data[0] & 0x80)) {
>> + bt_cb(bcsp->rx_skb)->pkt_type = HCI_EVENT_PKT;
>> + pass_up = 1;
>> + } else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
>> + bt_cb(bcsp->rx_skb)->pkt_type = HCI_SCODATA_PKT;
>> + pass_up = 1;
>> + } else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
>> + !(bcsp->rx_skb->data[0] & 0x80)) {
>> + bcsp_handle_le_pkt(hu);
>> + pass_up = 0;
>> + } else {
>> + pass_up = 0;
>> + }
>> + }
>> +
>> + if (pass_up == 0) {
>> struct hci_event_hdr hdr;
>> u8 desc = (bcsp->rx_skb->data[1] & 0x0f);
>>
>> @@ -544,11 +564,15 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
>> }
>> } else
>> kfree_skb(bcsp->rx_skb);
>> - } else {
>> + } else if (pass_up == 1) {
>> /* Pull out BCSP hdr */
>> skb_pull(bcsp->rx_skb, 4);
>>
>> hci_recv_frame(hu->hdev, bcsp->rx_skb);
>> + } else {
>> + /* ignore packet payload of already ACKed re-transmitted
>> + packets or when a packet was missed in the BCSP window */
> Also use network subsystem comment style.
Please can confirm which comment style is your preference because I have
doubts.
Below is a list of possible comment styles (I think), please tell me
which ones
are acceptable:
a)
/*
* comment line #1
* comment line #2
*/
b) I think you prefer this style aka "network subsystem style"
/* comment line #1
* comment line #2
*/
c)
/* comment line #1 */
/* comment line #2 */
d) this style was used in our patch to match the style in the file
/* comment line #1
comment line #2 */
drivers/bluetooth/hci_bcsp.c already contains styles b) and d)
If you agree, I can create some style clean-up commits for
drivers/bluetooth/hci_bcsp.c before applying our Bluetooth patch.
>
>> + kfree_skb(bcsp->rx_skb);
>> }
>>
>> bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
>> @@ -594,16 +618,6 @@ static int bcsp_recv(struct hci_uart *hu, const void *data, int count)
>> bcsp->rx_count = 0;
>> continue;
>> }
>> - if (bcsp->rx_skb->data[0] & 0x80 /* reliable pkt */
>> - && (bcsp->rx_skb->data[0] & 0x07) != bcsp->rxseq_txack) {
>> - BT_ERR("Out-of-order packet arrived, got %u expected %u",
>> - bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
>> -
>> - kfree_skb(bcsp->rx_skb);
>> - bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
>> - bcsp->rx_count = 0;
>> - continue;
>> - }
>> bcsp->rx_state = BCSP_W4_DATA;
>> bcsp->rx_count = (bcsp->rx_skb->data[1] >> 4) +
>> (bcsp->rx_skb->data[2] << 4); /* May be 0 */
Regards,
Dean
--
Dean Jenkins
Embedded Software Engineer
Linux Transportation Solutions
Mentor Embedded Software Division
Mentor Graphics (UK) Ltd.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer
2016-09-08 9:41 ` Dean Jenkins
@ 2016-09-08 10:28 ` Marcel Holtmann
0 siblings, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2016-09-08 10:28 UTC (permalink / raw)
To: Dean Jenkins; +Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth
Hi Dean,
>>> Send an ACK frame with the current txack value in response to
>>> every received reliable frame unless a TX reliable frame is being
>>> sent. This modification allows re-transmitted frames from the remote
>>> peer to be acknowledged rather than ignored. It means that the remote
>>> peer knows which frame number to start re-transmitting from.
>>>
>>> Without this modification, the recovery time to a missing frame
>>> from the remote peer was unnecessarily being extended because the
>>> headers of the out of order reliable frames were being discarded rather
>>> than being processed. The frame headers of received frames will
>>> indicate whether the local peer's transmissions have been
>>> acknowledged by the remote peer. Therefore, the local peer may
>>> unnecessarily re-transmit despite the remote peer already indicating
>>> that the frame had been acknowledged in out of order reliable frame.
>>>
>>> Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
>>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>>> Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
>>> ---
>>> drivers/bluetooth/hci_bcsp.c | 82 ++++++++++++++++++++++++++------------------
>>> 1 file changed, 48 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
>>> index d7d23ce..739ae59 100644
>>> --- a/drivers/bluetooth/hci_bcsp.c
>>> +++ b/drivers/bluetooth/hci_bcsp.c
>>> @@ -485,13 +485,27 @@ static inline void bcsp_unslip_one_byte(struct bcsp_struct *bcsp, unsigned char
>>> static void bcsp_complete_rx_pkt(struct hci_uart *hu)
>>> {
>>> struct bcsp_struct *bcsp = hu->priv;
>>> - int pass_up;
>>> + int pass_up = 0;
>>>
>>> if (bcsp->rx_skb->data[0] & 0x80) { /* reliable pkt */
>>> BT_DBG("Received seqno %u from card", bcsp->rxseq_txack);
>>> - bcsp->rxseq_txack++;
>>> - bcsp->rxseq_txack %= 0x8;
>>> - bcsp->txack_req = 1;
>>> +
>>> + /* check the rx sequence number is as expected */
>>> + if ((bcsp->rx_skb->data[0] & 0x07) == bcsp->rxseq_txack) {
>>> + bcsp->rxseq_txack++;
>>> + bcsp->rxseq_txack %= 0x8;
>>> + } else {
>>> + /* handle re-transmitted packet or
>>> + when packet was missed */
>>> + BT_ERR("Out-of-order packet arrived, got %u expected %u",
>>> + bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
>>> +
>>> + /* do not process out-of-order packet payload */
>>> + pass_up = 2;
>>> + }
>>> +
>>> + /* send current txack value to all received reliable packets */
>>> + bcsp->txack_req = 1;
>>>
>>> /* If needed, transmit an ack pkt */
>>> hci_uart_tx_wakeup(hu);
>>> @@ -500,26 +514,32 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
>>> bcsp->rxack = (bcsp->rx_skb->data[0] >> 3) & 0x07;
>>> BT_DBG("Request for pkt %u from card", bcsp->rxack);
>>>
>>> + /* handle received ACK indications,
>>> + including those from out-of-order packets */
>>> bcsp_pkt_cull(bcsp);
>>> - if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
>>> - bcsp->rx_skb->data[0] & 0x80) {
>>> - hci_skb_pkt_type(bcsp->rx_skb) = HCI_ACLDATA_PKT;
>>> - pass_up = 1;
>>> - } else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
>>> - bcsp->rx_skb->data[0] & 0x80) {
>>> - hci_skb_pkt_type(bcsp->rx_skb) = HCI_EVENT_PKT;
>>> - pass_up = 1;
>>> - } else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
>>> - hci_skb_pkt_type(bcsp->rx_skb) = HCI_SCODATA_PKT;
>>> - pass_up = 1;
>>> - } else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
>>> - !(bcsp->rx_skb->data[0] & 0x80)) {
>>> - bcsp_handle_le_pkt(hu);
>>> - pass_up = 0;
>>> - } else
>>> - pass_up = 0;
>>> -
>>> - if (!pass_up) {
>>> +
>>> + if (pass_up != 2) {
>>> + if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
>>> + (bcsp->rx_skb->data[0] & 0x80)) {
>> lets start using proper network subsystem coding style now.
>
> I think you are referring to the indentation style, right ?
>
> I am aware of the checkpatch.pl --strict warning of
> CHECK: Alignment should match open parenthesis
>
> Would this style be acceptable so the 2nd line of arguments is one character
> position to the right of the opening parenthesis of the 1st line by adding
> spaces after tabs to do the alignment ?
>
> + if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
> + (bcsp->rx_skb->data[0] & 0x80)) {
yes, this is how it should look like.
>>
>>> + bt_cb(bcsp->rx_skb)->pkt_type = HCI_ACLDATA_PKT;
>>> + pass_up = 1;
>>> + } else if ((bcsp->rx_skb->data[1] & 0x0f) == 5 &&
>>> + (bcsp->rx_skb->data[0] & 0x80)) {
>>> + bt_cb(bcsp->rx_skb)->pkt_type = HCI_EVENT_PKT;
>>> + pass_up = 1;
>>> + } else if ((bcsp->rx_skb->data[1] & 0x0f) == 7) {
>>> + bt_cb(bcsp->rx_skb)->pkt_type = HCI_SCODATA_PKT;
>>> + pass_up = 1;
>>> + } else if ((bcsp->rx_skb->data[1] & 0x0f) == 1 &&
>>> + !(bcsp->rx_skb->data[0] & 0x80)) {
>>> + bcsp_handle_le_pkt(hu);
>>> + pass_up = 0;
>>> + } else {
>>> + pass_up = 0;
>>> + }
>>> + }
>>> +
>>> + if (pass_up == 0) {
>>> struct hci_event_hdr hdr;
>>> u8 desc = (bcsp->rx_skb->data[1] & 0x0f);
>>>
>>> @@ -544,11 +564,15 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
>>> }
>>> } else
>>> kfree_skb(bcsp->rx_skb);
>>> - } else {
>>> + } else if (pass_up == 1) {
>>> /* Pull out BCSP hdr */
>>> skb_pull(bcsp->rx_skb, 4);
>>>
>>> hci_recv_frame(hu->hdev, bcsp->rx_skb);
>>> + } else {
>>> + /* ignore packet payload of already ACKed re-transmitted
>>> + packets or when a packet was missed in the BCSP window */
>> Also use network subsystem comment style.
>
> Please can confirm which comment style is your preference because I have doubts.
>
> Below is a list of possible comment styles (I think), please tell me which ones
> are acceptable:
>
> a)
> /*
> * comment line #1
> * comment line #2
> */
>
> b) I think you prefer this style aka "network subsystem style"
> /* comment line #1
> * comment line #2
> */
Yes. That is what we are suppose to be using.
>
> c)
> /* comment line #1 */
> /* comment line #2 */
>
> d) this style was used in our patch to match the style in the file
> /* comment line #1
> comment line #2 */
>
> drivers/bluetooth/hci_bcsp.c already contains styles b) and d)
>
> If you agree, I can create some style clean-up commits for
> drivers/bluetooth/hci_bcsp.c before applying our Bluetooth patch.
Go ahead if you have time for it.
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v1 4/4] Bluetooth: Prevent scheduling of work after hci_uart_tty_close()
2016-09-07 14:15 [PATCH v1 0/4] Bluetooth HCI LDISC and BCSP fixes Dean Jenkins
` (2 preceding siblings ...)
2016-09-07 14:15 ` [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer Dean Jenkins
@ 2016-09-07 14:15 ` Dean Jenkins
3 siblings, 0 replies; 10+ messages in thread
From: Dean Jenkins @ 2016-09-07 14:15 UTC (permalink / raw)
To: Dean Jenkins, Marcel Holtmann, Gustavo Padovan, Johan Hedberg
Cc: linux-bluetooth
From: Deepak Das <Deepak_Das@mentor.com>
There is a work queue hu->write_work that can be scheduled to handle
a transmission work item in BCSP scenarios that are described below.
In these scenarios, the payload of a BCSP frame can contain a HCI
message as follows:
a) Normal sending of a HCI message within a BCSP frame from upper
layers via hci_uart_send_frame().
b) BCSP retransmission due to a failure to get a BCSP ACK indication.
This resends an old HCI message triggered by the BCSP timer timeout event
handled by bcsp_timed_event().
c) Sending of a BCSP ACK frame in response to a received reliable BCSP
frame is handled in bcsp_complete_rx_pkt() which allows an empty payload
(no HCI message) to be sent.
A good reproduction scenario of the crash is for a bad UART driver to
corrupt or cause a total loss of incoming BCSP frames. This leads to a link
failure so the BCSP timer is periodically triggered in an attempt to
retransmit unacknowledged frames. Then upper layers request the HCI UART
to close immediately before the BCSP timer event is handled. This causes
a race condition between closing the HCI UART and retransmitting a BCSP
frame.
The current solution is using cancel_work_sync(&hu->write_work); within
hci_uart_tty_close() in an attempt to cleanse the work queue of the
transmission work item.
It should be noted that a transmission work item is scheduled into the work
queue by hci_uart_tx_wakeup() which can be running in an atomic context
such as a UART driver interrupt thread or in a process context. Therefore,
it is possible for hci_uart_tx_wakeup() to interrupt or run concurrently
with hci_uart_tty_close().
In addition, hu->proto->close(hu) calls bcsp_close() which frees the
various queues of messages and deletes the BCSP timer.
The current solution has a flaw because hci_uart_tty_close() fails to
inhibit the asynchronous b) and c) transmission events from adding a work
item to the work queue AFTER cancel_work_sync(&hu->write_work); has been
executed. A crash occurs after bcsp_close() has freed the message queues
which are subsequently accessed when the work queue work item runs
hci_uart_write_work(). Therefore, there is a race condition.
This flaw was masked as typically retransmissions should be rare and
Bluetooth has a low sub-system rate of being actively shutdown.
This fix introduces a new HCI_UART proto flag bit called
HCI_UART_UNREGISTERING to indicate the unregistering state of the HCI UART.
This flag is used to prevent the addition of a transmission work item after
hci_uart_tty_close() starts to run.
Note that a spinlock is necessary because it is not possible to check
the state of the atomic flag and to schedule the work queue in an atomic
sequence of operations in hci_uart_tx_wakeup(). This means that the flag
must be prevented from changing state after the flag is checked and before
the work queue is scheduled in hci_uart_tx_wakeup().
Signed-off-by: Deepak Das <Deepak_Das@mentor.com>
Signed-off-by: Rajeev Kumar <rajeev_kumar@mentor.com>
Signed-off-by: Dean_Jenkins@mentor.com
Signed-off-by: Dean Jenkins <Dean_Jenkins@mentor.com>
---
drivers/bluetooth/hci_ldisc.c | 16 +++++++++++++++-
drivers/bluetooth/hci_uart.h | 3 +++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index cf1883d..ddf4cda 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -125,15 +125,22 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
int hci_uart_tx_wakeup(struct hci_uart *hu)
{
+ unsigned long flags;
+
+ spin_lock_irqsave(&hu->schedule_lock, flags);
+ if (test_bit(HCI_UART_UNREGISTERING, &hu->flags))
+ goto no_schedule;
if (test_and_set_bit(HCI_UART_SENDING, &hu->tx_state)) {
set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
- return 0;
+ goto no_schedule;
}
BT_DBG("");
schedule_work(&hu->write_work);
+no_schedule:
+ spin_unlock_irqrestore(&hu->schedule_lock, flags);
return 0;
}
@@ -464,6 +471,8 @@ static int hci_uart_tty_open(struct tty_struct *tty)
INIT_WORK(&hu->init_ready, hci_uart_init_work);
INIT_WORK(&hu->write_work, hci_uart_write_work);
+ spin_lock_init(&hu->schedule_lock);
+
/* Flush any pending characters in the driver */
tty_driver_flush_buffer(tty);
@@ -479,6 +488,7 @@ static void hci_uart_tty_close(struct tty_struct *tty)
{
struct hci_uart *hu = tty->disc_data;
struct hci_dev *hdev;
+ unsigned long flags;
BT_DBG("tty %p", tty);
@@ -488,6 +498,10 @@ static void hci_uart_tty_close(struct tty_struct *tty)
if (!hu)
return;
+ spin_lock_irqsave(&hu->schedule_lock, flags);
+ set_bit(HCI_UART_UNREGISTERING, &hu->flags);
+ spin_unlock_irqrestore(&hu->schedule_lock, flags);
+
hdev = hu->hdev;
if (hdev)
hci_uart_close(hdev);
diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
index 839bad1..27a7708 100644
--- a/drivers/bluetooth/hci_uart.h
+++ b/drivers/bluetooth/hci_uart.h
@@ -88,6 +88,8 @@ struct hci_uart {
struct sk_buff *tx_skb;
unsigned long tx_state;
+ /* prevent scheduling work during close */
+ spinlock_t schedule_lock;
unsigned int init_speed;
unsigned int oper_speed;
};
@@ -96,6 +98,7 @@ struct hci_uart {
#define HCI_UART_PROTO_SET 0
#define HCI_UART_REGISTERED 1
#define HCI_UART_PROTO_READY 2
+#define HCI_UART_UNREGISTERING 3
/* TX states */
#define HCI_UART_SENDING 1
--
2.7.4
^ permalink raw reply related [flat|nested] 10+ messages in thread