Return-Path: Subject: Re: [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer To: Marcel Holtmann References: <1473257710-2073-1-git-send-email-Dean_Jenkins@mentor.com> <1473257710-2073-4-git-send-email-Dean_Jenkins@mentor.com> <09CFA74F-BCE6-4627-A318-E18B3BCEBE07@holtmann.org> CC: "Gustavo F. Padovan" , Johan Hedberg , From: Dean Jenkins Message-ID: <501e6875-ee82-1297-47f4-eab4696c9187@mentor.com> Date: Thu, 8 Sep 2016 10:41:53 +0100 MIME-Version: 1.0 In-Reply-To: <09CFA74F-BCE6-4627-A318-E18B3BCEBE07@holtmann.org> Content-Type: text/plain; charset="windows-1252"; format=flowed List-ID: 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 >> Signed-off-by: Jiada Wang >> Signed-off-by: Rajeev Kumar >> --- >> 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.