[PATCH] Bluetooth: HCI H5 peer reset detection

[PATCH] Bluetooth: HCI H5 peer reset detection

[Loic Poulain]

H5 Specification says:
If a SYNC message is received while in the Active State, it is
assumed that the peer device has reset. The local device should
therefore perform a full reset of the upper stack, and start Link
Establishment again at the Uninitialized State. Upon entering the
Active State, the first packet sent shall have its SEQ and ACK
numbers set to zero.

This patch resets the HCI H5 driver data/state to unitialized and
reports an HCI hardware error event to notify the upper stack that
HCI synchronization has been lost. H5 will be re-synchronized and
upper stack should generate an HCI Reset command.

Signed-off-by: Loic Poulain <loic.poulain@intel.com>
---
 drivers/bluetooth/hci_h5.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index caacb42..5c5e9ac 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -168,6 +168,36 @@ wakeup:
 	hci_uart_tx_wakeup(hu);
 }
 
+static void h5_peer_reset(struct hci_uart *hu)
+{
+	struct h5 *h5 = hu->priv;
+	struct sk_buff *skb;
+	const unsigned char hard_err[] = { 0x10, 0x01, 0x00 };
+
+	BT_ERR("Peer device has reset");
+
+	h5->state = H5_UNINITIALIZED;
+
+	del_timer(&h5->timer);
+
+	skb_queue_purge(&h5->rel);
+	skb_queue_purge(&h5->unrel);
+	skb_queue_purge(&h5->unack);
+
+	h5->tx_seq = 0;
+	h5->tx_ack = 0;
+
+	skb = bt_skb_alloc(3, GFP_ATOMIC);
+	if (!skb)
+		return;
+
+	bt_cb(skb)->pkt_type = HCI_EVENT_PKT;
+	memcpy(skb_put(skb, 3), hard_err, 3);
+
+	/* Send Hardware Error to upper stack */
+	hci_recv_frame(hu->hdev, skb);
+}
+
 static int h5_open(struct hci_uart *hu)
 {
 	struct h5 *h5;
@@ -283,8 +313,12 @@ static void h5_handle_internal_rx(struct hci_uart *hu)
 	conf_req[2] = h5_cfg_field(h5);
 
 	if (memcmp(data, sync_req, 2) == 0) {
+		if (h5->state == H5_ACTIVE)
+			h5_peer_reset(hu);
 		h5_link_control(hu, sync_rsp, 2);
 	} else if (memcmp(data, sync_rsp, 2) == 0) {
+		if (h5->state == H5_ACTIVE)
+			h5_peer_reset(hu);
 		h5->state = H5_INITIALIZED;
 		h5_link_control(hu, conf_req, 3);
 	} else if (memcmp(data, conf_req, 2) == 0) {
-- 
1.8.3.2

Re: [PATCH] Bluetooth: HCI H5 peer reset detection

[Johan Hedberg]

Hi Loic,

On Wed, Oct 08, 2014, Loic Poulain wrote:
> This patch resets the HCI H5 driver data/state to unitialized and
> reports an HCI hardware error event to notify the upper stack that
> HCI synchronization has been lost. H5 will be re-synchronized and
> upper stack should generate an HCI Reset command.

I couldn't find anywhere where we'd currently do this "upper stack"
behavior that you describe (there . Did you have some follow-up patches to
implement that?

Johan

Re: [PATCH] Bluetooth: HCI H5 peer reset detection

[Loic Poulain]

Hi Johan,

This is just the expected behavior as described int the specification:

UART Spec says:
If the UART synchronization is lost in the communication from Host to Host
Controller, then the Host Controller shall send a Hardware Error Event 
to tell
the Host about the synchronization error. The Host Controller will then 
expect to
receive an HCI_Reset command from the Host in order to perform a reset. The
Host Controller will also use the HCI_Reset command in the byte stream from
Host to Host Controller to re-synchronize.

H5 part says:
If a SYNC message is received while in the Active State, it is assumed 
that the
peer device has reset. The local device should therefore perform a full 
reset of
the upper stack, and start Link Establishment again at the Uninitialized 
State.

So, hardware error seems the best way to warn host stack about the H5 
controller
reset.

Regarding the "upper stack", I know that Bluedroid restarts on hardware 
error, but
I ignore the Bluez behavior.

Regards,
Loic

On 31/10/2014 09:39, Johan Hedberg wrote:
> Hi Loic,
>
> On Wed, Oct 08, 2014, Loic Poulain wrote:
>> This patch resets the HCI H5 driver data/state to unitialized and
>> reports an HCI hardware error event to notify the upper stack that
>> HCI synchronization has been lost. H5 will be re-synchronized and
>> upper stack should generate an HCI Reset command.
> I couldn't find anywhere where we'd currently do this "upper stack"
> behavior that you describe (there . Did you have some follow-up patches to
> implement that?
>
> Johan

-- 
Intel Open Source Technology Center
http://oss.intel.com/

Re: [PATCH] Bluetooth: HCI H5 peer reset detection

[Marcel Holtmann]

Hi Loic,

please do not top-post on this mailing list. It breaks the flow of things since everybody is doing inline quoting.

> This is just the expected behavior as described int the specification:
> 
> UART Spec says:
> If the UART synchronization is lost in the communication from Host to Host
> Controller, then the Host Controller shall send a Hardware Error Event to tell
> the Host about the synchronization error. The Host Controller will then expect to
> receive an HCI_Reset command from the Host in order to perform a reset. The
> Host Controller will also use the HCI_Reset command in the byte stream from
> Host to Host Controller to re-synchronize.
> 
> H5 part says:
> If a SYNC message is received while in the Active State, it is assumed that the
> peer device has reset. The local device should therefore perform a full reset of
> the upper stack, and start Link Establishment again at the Uninitialized State.
> 
> So, hardware error seems the best way to warn host stack about the H5 controller
> reset.

I am actually fine with doing it this way.

> Regarding the "upper stack", I know that Bluedroid restarts on hardware error, but
> I ignore the Bluez behavior.

This is something we need to fix then. First and foremost this has to work with the Bluetooth subsystem in the kernel as well and not just a stack on top of HCI User Channel.

Regards

Marcel

Re: [PATCH] Bluetooth: HCI H5 peer reset detection

[Johan Hedberg]

Hi Marcel,

On Fri, Oct 31, 2014, Marcel Holtmann wrote:
> > So, hardware error seems the best way to warn host stack about the H5 controller
> > reset.
> 
> I am actually fine with doing it this way.
> 
> > Regarding the "upper stack", I know that Bluedroid restarts on hardware error, but
> > I ignore the Bluez behavior.
> 
> This is something we need to fix then. First and foremost this has to
> work with the Bluetooth subsystem in the kernel as well and not just a
> stack on top of HCI User Channel.

Agreed. I've now applied this patch to bluetooth-next so we can continue
towards making the higher layers do the right thing as well.

Johan