Messages in this thread | | | Date | Tue, 15 Jul 2014 05:57:37 -0700 | From | Jiada Wang <> | 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@mvista.com> >> Signed-off-by: Jiada Wang <jiada_wang@mentor.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
| |