| 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: <53C52541.50300@mentor.com>
Date: Tue, 15 Jul 2014 05:57:37 -0700
From: Jiada Wang <jiada_wang@...tor.com>
To: Marcel Holtmann <marcel@...tmann.org>
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 Marcel
On 07/15/2014 04:42 AM, Marcel Holtmann wrote:
> 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.
Can you tell me what should be the correct indentation.
>
>> + 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
>
Thanks,
Jiada
--
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