Re: [PATCH v2] Bluetooth: BCSP fails to ACK re-transmitted frames from the peer

[Marcel Holtmann]

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@xxxxxxxxxx>
> Signed-off-by: Jiada Wang <jiada_wang@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/