| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <B8E404C6-6A75-41E8-9506-0B05AA4C56D6@holtmann.org>
Date: Tue, 15 Jul 2014 13:42:42 +0200
From: Marcel Holtmann <marcel@...tmann.org>
To: Jiada Wang <jiada_wang@...tor.com>
Cc: "Gustavo F. Padovan" <gustavo@...ovan.org>,
Johan Hedberg <johan.hedberg@...il.com>,
Linux Bluetooth mailing list
<linux-bluetooth@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>, Dean_Jenkins@...tor.com
Subject: Re: [PATCH v2] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer
Hi Jiada,
> 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 <djenkins@...sta.com>
> Signed-off-by: Jiada Wang <jiada_wang@...tor.com>
> ---
> drivers/bluetooth/hci_bcsp.c | 94 ++++++++++++++++++++++++++++----------------
> 1 file changed, 60 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
> index 21cc45b..0f4664d 100644
> --- a/drivers/bluetooth/hci_bcsp.c
> +++ b/drivers/bluetooth/hci_bcsp.c
> @@ -478,13 +478,29 @@ 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
> + */
Comment style is wrong.
/* aaa
* bbb
*/
> + BT_ERR ("Out-of-order packet arrived, got %u expected %u",
> + bcsp->rx_skb->data[0] & 0x07, bcsp->rxseq_txack);
It is BT_ERR(" and not BT_ERR (".
> +
> + /* do not process out-of-order packet payload */
> + pass_up = 2;
> + }
> +
> + /* send current txack value to all recieved reliable packets */
> + bcsp->txack_req = 1;
>
> /* If needed, transmit an ack pkt */
> hci_uart_tx_wakeup(hu);
> @@ -493,26 +509,36 @@ 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 recieved ACK indications,
> + * including those from out-of-order packets
> + */
Same here. Please fix comment style.
> bcsp_pkt_cull(bcsp);
> - 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) {
> +
> + if (pass_up != 2) {
> + if ((bcsp->rx_skb->data[1] & 0x0f) == 6 &&
> + bcsp->rx_skb->data[0] & 0x80) {
Fix indentation here.
> + 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) {
And here.
> + 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)) {
Same here.
> + bcsp_handle_le_pkt(hu);
> + pass_up = 0;
> + } else {
> + pass_up = 0;
> + }
> + }
> +
> + switch (pass_up) {
> + case 0:
> + {
> struct hci_event_hdr hdr;
> u8 desc = (bcsp->rx_skb->data[1] & 0x0f);
In general I do not prefer using { } in case statements. Please declare the variables where they are needed or use if else.
>
> @@ -537,11 +563,21 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
> }
> } else
> kfree_skb(bcsp->rx_skb);
> - } else {
> + break;
> + }
> + case 1:
> /* Pull out BCSP hdr */
> skb_pull(bcsp->rx_skb, 4);
>
> hci_recv_frame(hu->hdev, bcsp->rx_skb);
> + break;
> + default:
> + /*
> + * ignore packet payload of already ACKed re-transmitted
> + * packets or when a packet was missed in the BCSP window
> + */
Fix up comment style.
> + kfree_skb(bcsp->rx_skb);
> + break;
> }
>
> bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
> @@ -587,16 +623,6 @@ static int bcsp_recv(struct hci_uart *hu, 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists