Return-Path: Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\)) Subject: Re: [PATCH v1 3/4] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer From: Marcel Holtmann In-Reply-To: <1473257710-2073-4-git-send-email-Dean_Jenkins@mentor.com> Date: Wed, 7 Sep 2016 15:22:17 +0100 Cc: "Gustavo F. Padovan" , Johan Hedberg , linux-bluetooth@vger.kernel.org Message-Id: <09CFA74F-BCE6-4627-A318-E18B3BCEBE07@holtmann.org> References: <1473257710-2073-1-git-send-email-Dean_Jenkins@mentor.com> <1473257710-2073-4-git-send-email-Dean_Jenkins@mentor.com> To: Dean Jenkins Sender: linux-bluetooth-owner@vger.kernel.org List-ID: 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. 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