Hi,
This serie of patch will add the link establishment protocol for BCSP into
the kernel rather than having to handle in userland before switching discipline
and make the TX window size configurable.
These patches might not be ready for direct submission to the kernel mainline, but this is a first request for comment/review.
Thanks,
Regards
Tristan Lelong (2):
hci_bcsp: Add LE protocol on kernel side
bcsp: Change tx window size in configuration
drivers/bluetooth/Kconfig | 7 ++
drivers/bluetooth/hci_bcsp.c | 203 +++++++++++++++++++++++++++++++--------
drivers/bluetooth/hci_ldisc.c | 5 +
include/net/bluetooth/hci_core.h | 2 +
net/bluetooth/hci_core.c | 6 +-
5 files changed, 182 insertions(+), 41 deletions(-)
--
2.1.1
Hi,
I took care of all the comments and started to test the patch.
I probably need to modify some code on the userland side for better testing.
I have a bug to fix already, probably caused by the cleanup of the patch.
I'll do this and resubmit the patch once fully tested and working.
Best regards
On Fri, Dec 12, 2014 at 02:58:30PM +0100, Marcel Holtmann wrote:
Hi Marcel,
> >
> > +config BT_HCIUART_BCSP_WINSIZE
> > + int "BCSP reliable packet TX window size"
> > + default 4
> > + depends on BT_HCIUART_BCSP
> > + help
> > + Defines the number of packets that can be sent before receiving a ACK.
> > +
>
> I would prefer if we get a bit more help text here.
>
Yes, I can make it a bit more explicit and descriptive.
> >
> > - if (bcsp->unack.qlen < BCSP_TXWINSIZE) {
> > + if (bcsp->unack.qlen < CONFIG_BT_HCIUART_BCSP_WINSIZE) {
> > skb = skb_dequeue(&bcsp->rel);
> > if (skb != NULL) {
> > struct sk_buff *nskb = bcsp_prepare_pkt(bcsp, skb->data, skb->len,
>
> This is fine with me, but we might also want to add a module parameter to allow changing it. If people want to play with it then they do not need to rebuild their module.
>
> Another alternative is to provide an ioctl for changing it.
That makes sense, I think a parameter is a good solution, and if nobody shows any preference, I'll implement it this way.
Thanks
On Fri, Dec 12, 2014 at 02:58:33PM +0100, Marcel Holtmann wrote:
> > ---
> > drivers/bluetooth/hci_bcsp.c | 202 +++++++++++++++++++++++++++++++--------
> > include/net/bluetooth/hci_core.h | 2 +
> > net/bluetooth/hci_core.c | 6 +-
> > 3 files changed, 171 insertions(+), 39 deletions(-)
>
> patch for drivers and net should always be separate. You need to split this.
Ok, I didn't know this one, I'll fix this.
>
> >
> > diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
> > index 21cc45b..142b42b 100644
> > --- a/drivers/bluetooth/hci_bcsp.c
> > +++ b/drivers/bluetooth/hci_bcsp.c
> > @@ -1,6 +1,5 @@
> > /*
> > *
> > - * Bluetooth HCI UART driver
>
> No idea why you are removing this line.
I'll fix this one.
> >
> > +static u8 conf_pkt[4] = { 0xad, 0xef, 0xac, 0xed };
> > +static u8 conf_rsp_pkt[4] = { 0xde, 0xad, 0xd0, 0xd0 };
> > +static u8 sync_pkt[4] = { 0xda, 0xdc, 0xed, 0xed };
> > +static u8 sync_rsp_pkt[4] = { 0xac, 0xaf, 0xef, 0xee };
>
> This should be const actually.
Agreed, I'll change this.
>
> > +
> > +
>
> And we do not do double empty lines. If that is in the code, then it is a leftover.
I missed this one, I'll fix it.
> > +
> > + /* tell upper layer about the reset */
> > + hci_dev_reset_stat(hu->hdev->id);
> > +
>
> We introduced hci_reset_dev for the H:5 protocol. And I think that is the same that should be used here. The core only injects are hardware error event at the moment, but that is a core bug and we should fix that to handle proper stack reset when that happens. The driver should not hack around this.
>
I initially wrote the code for a 3.10 kernel, I must have missed this in the new version. I'll change this.
> > +
> > + if (BCSP_PKT_LEN(bcsp->rx_skb->data) == BCSP_LE_PKT_LEN) {
> > + /* spot "conf" pkts and reply with a "conf rsp" pkt */
> > + if (!memcmp(&bcsp->rx_skb->data[4], conf_pkt, 4)) {
> > + if (bcsp->le_state == BCSP_SHY)
> > + return;
> > +
> > + BT_DBG("Found a LE conf pkt");
> > + nskb = alloc_skb(4, GFP_ATOMIC);
> > + if (!nskb)
> > + return;
> > + memcpy(skb_put(nskb, 4), conf_rsp_pkt, 4);
> > + bt_cb(nskb)->pkt_type = BCSP_LE_PKT;
> > +
> > + skb_queue_head(&bcsp->unrel, nskb);
> > + hci_uart_tx_wakeup(hu);
> > + }
> > + /* Spot "sync" pkts and reply with a "sync rsp" pkt */
> > + else if (!memcmp(&bcsp->rx_skb->data[4], sync_pkt, 4)) {
> > + if (bcsp->le_state == BCSP_GARULOUS)
> > + /* we force the connection state to reset */
> > + bcsp_internal_reset(hu);
>
> These needs { }. If you end up having a comment include it in { } even if the compiler does not care. That is for our reading sake.
>
Just tried to follow how it was done initially, but I did not really like it either, I'll fix this.
> > +
> > + BT_INFO("BCSP: hci%d LE sync pkt, performing reset",
> > + hu->hdev->id);
>
> Align the second line properly according to the coding style. Even if previous code might actually violate it.
>
Will do, as well as all the following coding style issues.
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index f001856..2c5130a 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -65,10 +65,12 @@ static DEFINE_IDA(hci_index_ida);
> >
> > /* ---- HCI notifications ---- */
> >
> > -static void hci_notify(struct hci_dev *hdev, int event)
> > +void hci_notify(struct hci_dev *hdev, int event)
> > {
> > hci_sock_dev_event(hdev, event);
> > }
> > +EXPORT_SYMBOL(hci_notify);
> > +
>
> We will never export this internal detail to drivers. Use hci_reset_dev and lets get a proper stack reset handling implemented.
Understood, I'll ignore the notification to upper layer for now.
>
> >
> > /* ---- HCI debugfs entries ---- */
> >
> > @@ -2783,6 +2785,8 @@ done:
> > hci_dev_put(hdev);
> > return ret;
> > }
> > +EXPORT_SYMBOL(hci_dev_reset_stat);
> > +
>
> I have no idea why I would ever allow a driver to reset these. Even in case of errors it is fine to keep counting.
Ok, I can see the point here.
I will fix all this and do some testing, then resubmit early next week.
Thank you very much for your comments.
Regards
Hi Tristan,
> Add the link establishment protocol handling in the kernel:
> - le_state enum to handle state machine
> - bcsp_timed_event to send LE messages
> - bcsp_handle_le_pkt to answer endpoint LE messages
> - various tests to muzzle BCSP link before GARULOUS
>
> The patch exports some HCI functions that are required whitin BCSP to properly handle the connection reset.
> - hci_dev_reset_stat
> - hci_notify
>
> Signed-off-by: Tristan Lelong <[email protected]>
> ---
> drivers/bluetooth/hci_bcsp.c | 202 +++++++++++++++++++++++++++++++--------
> include/net/bluetooth/hci_core.h | 2 +
> net/bluetooth/hci_core.c | 6 +-
> 3 files changed, 171 insertions(+), 39 deletions(-)
patch for drivers and net should always be separate. You need to split this.
>
> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
> index 21cc45b..142b42b 100644
> --- a/drivers/bluetooth/hci_bcsp.c
> +++ b/drivers/bluetooth/hci_bcsp.c
> @@ -1,6 +1,5 @@
> /*
> *
> - * Bluetooth HCI UART driver
No idea why you are removing this line.
> *
> * Copyright (C) 2002-2003 Fabrizio Gennari <[email protected]>
> * Copyright (C) 2004-2005 Marcel Holtmann <[email protected]>
> @@ -47,13 +46,25 @@
>
> #include "hci_uart.h"
>
> -#define VERSION "0.3"
> +#define VERSION "0.4"
> +
> +#define LE_POLLING (HZ/10)
> +#define TX_POLLING (HZ/4)
>
> static bool txcrc = 1;
> static bool hciextn = 1;
>
> +static u8 conf_pkt[4] = { 0xad, 0xef, 0xac, 0xed };
> +static u8 conf_rsp_pkt[4] = { 0xde, 0xad, 0xd0, 0xd0 };
> +static u8 sync_pkt[4] = { 0xda, 0xdc, 0xed, 0xed };
> +static u8 sync_rsp_pkt[4] = { 0xac, 0xaf, 0xef, 0xee };
This should be const actually.
> +
> +
And we do not do double empty lines. If that is in the code, then it is a leftover.
> #define BCSP_TXWINSIZE 4
>
> +#define BCSP_PKT_LEN(data) ((data[1] >> 4) + (data[2]))
> +#define BCSP_LE_PKT_LEN 0x04
> +
> #define BCSP_ACK_PKT 0x05
> #define BCSP_LE_PKT 0x06
>
> @@ -69,6 +80,12 @@ struct bcsp_struct {
> struct timer_list tbcsp;
>
> enum {
> + BCSP_SHY = 0,
> + BCSP_CURIOUS,
> + BCSP_GARULOUS
> + } le_state;
> +
> + enum {
> BCSP_W4_PKT_DELIMITER,
> BCSP_W4_PKT_START,
> BCSP_W4_BCSP_HDR,
> @@ -184,6 +201,9 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
> u16 BCSP_CRC_INIT(bcsp_txmsg_crc);
> int rel, i;
>
> + if (bcsp->le_state != BCSP_GARULOUS && pkt_type != BCSP_LE_PKT)
> + return NULL;
> +
> switch (pkt_type) {
> case HCI_ACLDATA_PKT:
> chan = 6; /* BCSP ACL channel */
> @@ -299,6 +319,8 @@ static struct sk_buff *bcsp_dequeue(struct hci_uart *hu)
> return nskb;
> } else {
> skb_queue_head(&bcsp->unrel, skb);
> + if (bcsp->le_state == BCSP_GARULOUS)
> + return NULL;
> BT_ERR("Could not dequeue pkt because alloc_skb failed");
> }
> }
> @@ -316,11 +338,18 @@ static struct sk_buff *bcsp_dequeue(struct hci_uart *hu)
> bt_cb(skb)->pkt_type);
> if (nskb) {
> __skb_queue_tail(&bcsp->unack, skb);
> - mod_timer(&bcsp->tbcsp, jiffies + HZ / 4);
> +
> + if (bcsp->le_state == BCSP_GARULOUS)
> + /* only re-arm retransmit timer
> + * if the BCSP link is connected */
> + mod_timer(&bcsp->tbcsp,
> + jiffies + HZ / 4);
> spin_unlock_irqrestore(&bcsp->unack.lock, flags);
> return nskb;
> } else {
> skb_queue_head(&bcsp->rel, skb);
> + if (bcsp->le_state != BCSP_GARULOUS)
> + return NULL;
> BT_ERR("Could not dequeue pkt because alloc_skb failed");
> }
> }
> @@ -386,7 +415,7 @@ static void bcsp_pkt_cull(struct bcsp_struct *bcsp)
> kfree_skb(skb);
> }
>
> - if (skb_queue_empty(&bcsp->unack))
> + if (skb_queue_empty(&bcsp->unack) && bcsp->le_state == BCSP_GARULOUS)
> del_timer(&bcsp->tbcsp);
>
> spin_unlock_irqrestore(&bcsp->unack.lock, flags);
> @@ -395,34 +424,100 @@ static void bcsp_pkt_cull(struct bcsp_struct *bcsp)
> BT_ERR("Removed only %u out of %u pkts", i, pkts_to_be_removed);
> }
>
> -/* Handle BCSP link-establishment packets. When we
> - detect a "sync" packet, symptom that the BT module has reset,
> - we do nothing :) (yet) */
> +static void bcsp_internal_reset(struct hci_uart *hu)
> +{
> + struct bcsp_struct *bcsp = hu->priv;
> +
> + /* reset the bcsp stack counters */
> + bcsp->rxseq_txack = 0;
> + bcsp->rxack = 0;
> + bcsp->message_crc = 0;
> + bcsp->txack_req = 0;
> + bcsp->msgq_txseq = 0;
> +
> + /* reset state machine */
> + bcsp->le_state = BCSP_SHY;
> + bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
> + bcsp->rx_esc_state = BCSP_ESCSTATE_NOESC;
> +
> + /* empty all the queues */
> + skb_queue_purge(&bcsp->unack);
> + skb_queue_purge(&bcsp->rel);
> + skb_queue_purge(&bcsp->unrel);
> +
> + /* tell upper layer about the reset */
> + hci_dev_reset_stat(hu->hdev->id);
> +
> + hci_notify(hu->hdev, HCI_DEV_UNREG);
We introduced hci_reset_dev for the H:5 protocol. And I think that is the same that should be used here. The core only injects are hardware error event at the moment, but that is a core bug and we should fix that to handle proper stack reset when that happens. The driver should not hack around this.
> +
> + bcsp->tbcsp.expires = jiffies + LE_POLLING;
> + add_timer(&bcsp->tbcsp);
> +}
> +
> +/* Handle BCSP link-establishment packets.
> + */
> static void bcsp_handle_le_pkt(struct hci_uart *hu)
> {
> + struct sk_buff *nskb;
> struct bcsp_struct *bcsp = hu->priv;
> - u8 conf_pkt[4] = { 0xad, 0xef, 0xac, 0xed };
> - u8 conf_rsp_pkt[4] = { 0xde, 0xad, 0xd0, 0xd0 };
> - u8 sync_pkt[4] = { 0xda, 0xdc, 0xed, 0xed };
> -
> - /* spot "conf" pkts and reply with a "conf rsp" pkt */
> - if (bcsp->rx_skb->data[1] >> 4 == 4 && bcsp->rx_skb->data[2] == 0 &&
> - !memcmp(&bcsp->rx_skb->data[4], conf_pkt, 4)) {
> - struct sk_buff *nskb = alloc_skb(4, GFP_ATOMIC);
> -
> - BT_DBG("Found a LE conf pkt");
> - if (!nskb)
> - return;
> - memcpy(skb_put(nskb, 4), conf_rsp_pkt, 4);
> - bt_cb(nskb)->pkt_type = BCSP_LE_PKT;
> -
> - skb_queue_head(&bcsp->unrel, nskb);
> - hci_uart_tx_wakeup(hu);
> - }
> - /* Spot "sync" pkts. If we find one...disaster! */
> - else if (bcsp->rx_skb->data[1] >> 4 == 4 && bcsp->rx_skb->data[2] == 0 &&
> - !memcmp(&bcsp->rx_skb->data[4], sync_pkt, 4)) {
> - BT_ERR("Found a LE sync pkt, card has reset");
> +
> + if (BCSP_PKT_LEN(bcsp->rx_skb->data) == BCSP_LE_PKT_LEN) {
> + /* spot "conf" pkts and reply with a "conf rsp" pkt */
> + if (!memcmp(&bcsp->rx_skb->data[4], conf_pkt, 4)) {
> + if (bcsp->le_state == BCSP_SHY)
> + return;
> +
> + BT_DBG("Found a LE conf pkt");
> + nskb = alloc_skb(4, GFP_ATOMIC);
> + if (!nskb)
> + return;
> + memcpy(skb_put(nskb, 4), conf_rsp_pkt, 4);
> + bt_cb(nskb)->pkt_type = BCSP_LE_PKT;
> +
> + skb_queue_head(&bcsp->unrel, nskb);
> + hci_uart_tx_wakeup(hu);
> + }
> + /* Spot "sync" pkts and reply with a "sync rsp" pkt */
> + else if (!memcmp(&bcsp->rx_skb->data[4], sync_pkt, 4)) {
> + if (bcsp->le_state == BCSP_GARULOUS)
> + /* we force the connection state to reset */
> + bcsp_internal_reset(hu);
These needs { }. If you end up having a comment include it in { } even if the compiler does not care. That is for our reading sake.
> +
> + BT_INFO("BCSP: hci%d LE sync pkt, performing reset",
> + hu->hdev->id);
Align the second line properly according to the coding style. Even if previous code might actually violate it.
> +
> + nskb = alloc_skb(4, GFP_ATOMIC);
> + if (!nskb)
> + return;
> + memcpy(skb_put(nskb, 4), sync_rsp_pkt, 4);
> + bt_cb(nskb)->pkt_type = BCSP_LE_PKT;
> +
> + skb_queue_head(&bcsp->unrel, nskb);
> + hci_uart_tx_wakeup(hu);
> + }
> + /* Spot "conf rsp" pkts and update state machine */
> + else if (!memcmp(&bcsp->rx_skb->data[4], conf_rsp_pkt, 4)) {
It should be } else if
> + if (bcsp->le_state != BCSP_CURIOUS) {
> + BT_DBG("BCSP: hci%d ingoring conf_resp packet",
> + hu->hdev->id);
Same here. Proper alignment.
> + return;
> + }
> +
> + BT_INFO("BCSP: hci%d connected", hu->hdev->id);
> + bcsp->le_state = BCSP_GARULOUS;
> + }
> + /* Spot "sync rsp" pkts and update state machine */
Same here. } else if
> + else if (!memcmp(&bcsp->rx_skb->data[4], sync_rsp_pkt, 4)) {
> + if (bcsp->le_state != BCSP_SHY) {
> + BT_DBG("BCSP: hci%d ignoring sync_resp packet",
> + hu->hdev->id);
And alignment adjustment here.
> + return;
> + }
> +
> + BT_DBG("Found a LE sync rsp pkt");
> + bcsp->le_state = BCSP_CURIOUS;
> + }
> +
> }
> }
>
> @@ -537,12 +632,13 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
> }
> } else
> kfree_skb(bcsp->rx_skb);
> - } else {
> + } else if (bcsp->le_state == BCSP_GARULOUS) {
> /* Pull out BCSP hdr */
> skb_pull(bcsp->rx_skb, 4);
>
> hci_recv_frame(hu->hdev, bcsp->rx_skb);
> - }
> + } else
> + kfree_skb(bcsp->rx_skb);
If one part uses { }, then all of them have to use it.
>
> bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
> bcsp->rx_skb = NULL;
> @@ -676,16 +772,42 @@ static void bcsp_timed_event(unsigned long arg)
> struct sk_buff *skb;
> unsigned long flags;
>
> - BT_DBG("hu %p retransmitting %u pkts", hu, bcsp->unack.qlen);
> + switch (bcsp->le_state) {
> + case BCSP_GARULOUS:
> + BT_DBG("hu %p retransmitting %u pkts", hu, bcsp->unack.qlen);
>
> - spin_lock_irqsave_nested(&bcsp->unack.lock, flags, SINGLE_DEPTH_NESTING);
> + spin_lock_irqsave_nested(&bcsp->unack.lock, flags,
> + SINGLE_DEPTH_NESTING);
Alignment with &bcsp-> please.
>
> - while ((skb = __skb_dequeue_tail(&bcsp->unack)) != NULL) {
> - bcsp->msgq_txseq = (bcsp->msgq_txseq - 1) & 0x07;
> - skb_queue_head(&bcsp->rel, skb);
> - }
> + while ((skb = __skb_dequeue_tail(&bcsp->unack)) != NULL) {
> + bcsp->msgq_txseq = (bcsp->msgq_txseq - 1) & 0x07;
> + skb_queue_head(&bcsp->rel, skb);
> + }
>
> - spin_unlock_irqrestore(&bcsp->unack.lock, flags);
> + spin_unlock_irqrestore(&bcsp->unack.lock, flags);
> + break;
> + case BCSP_CURIOUS:
> + skb = alloc_skb(4, GFP_ATOMIC);
> + memcpy(skb_put(skb, 4), conf_pkt, 4);
> + bt_cb(skb)->pkt_type = BCSP_LE_PKT;
> + skb_queue_head(&bcsp->unrel, skb);
> +
> + bcsp->tbcsp.expires += LE_POLLING;
> + add_timer(&bcsp->tbcsp);
> + break;
> + default:
> + BT_INFO("Internal state unknown. Assuming not connected.");
> + bcsp->le_state = BCSP_SHY;
> + case BCSP_SHY:
> + skb = alloc_skb(4, GFP_ATOMIC);
> + memcpy(skb_put(skb, 4), sync_pkt, 4);
> + bt_cb(skb)->pkt_type = BCSP_LE_PKT;
> + skb_queue_head(&bcsp->unrel, skb);
> +
> + bcsp->tbcsp.expires += LE_POLLING;
> + add_timer(&bcsp->tbcsp);
> + break;
> + }
>
> hci_uart_tx_wakeup(hu);
> }
> @@ -708,12 +830,16 @@ static int bcsp_open(struct hci_uart *hu)
> init_timer(&bcsp->tbcsp);
> bcsp->tbcsp.function = bcsp_timed_event;
> bcsp->tbcsp.data = (u_long) hu;
> + bcsp->tbcsp.expires = jiffies + LE_POLLING;
>
> bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
> + bcsp->le_state = BCSP_SHY;
>
> if (txcrc)
> bcsp->use_crc = 1;
>
> + add_timer(&bcsp->tbcsp);
> +
> return 0;
> }
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 1dae700..6fd8356 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -735,6 +735,8 @@ int hci_disconnect(struct hci_conn *conn, __u8 reason);
> bool hci_setup_sync(struct hci_conn *conn, __u16 handle);
> void hci_sco_setup(struct hci_conn *conn, __u8 status);
>
> +void hci_notify(struct hci_dev *hdev, int event);
> +
> struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst,
> u8 role);
> int hci_conn_del(struct hci_conn *conn);
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index f001856..2c5130a 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -65,10 +65,12 @@ static DEFINE_IDA(hci_index_ida);
>
> /* ---- HCI notifications ---- */
>
> -static void hci_notify(struct hci_dev *hdev, int event)
> +void hci_notify(struct hci_dev *hdev, int event)
> {
> hci_sock_dev_event(hdev, event);
> }
> +EXPORT_SYMBOL(hci_notify);
> +
We will never export this internal detail to drivers. Use hci_reset_dev and lets get a proper stack reset handling implemented.
>
> /* ---- HCI debugfs entries ---- */
>
> @@ -2783,6 +2785,8 @@ done:
> hci_dev_put(hdev);
> return ret;
> }
> +EXPORT_SYMBOL(hci_dev_reset_stat);
> +
I have no idea why I would ever allow a driver to reset these. Even in case of errors it is fine to keep counting.
Regards
Marcel
Hi Tristan,
> This patch make the TX window size configurable using the kernel configuration interface.
> The default value used is the previous value: 4
>
> Signed-off-by: Tristan Lelong <[email protected]>
> ---
> drivers/bluetooth/Kconfig | 7 +++++++
> drivers/bluetooth/hci_bcsp.c | 5 +----
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 364f080..9f634cb 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -59,6 +59,13 @@ config BT_HCIUART_BCSP
>
> Say Y here to compile support for HCI BCSP protocol.
>
> +config BT_HCIUART_BCSP_WINSIZE
> + int "BCSP reliable packet TX window size"
> + default 4
> + depends on BT_HCIUART_BCSP
> + help
> + Defines the number of packets that can be sent before receiving a ACK.
> +
I would prefer if we get a bit more help text here.
> config BT_HCIUART_ATH3K
> bool "Atheros AR300x serial support"
> depends on BT_HCIUART
> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
> index 142b42b..bb18f2a 100644
> --- a/drivers/bluetooth/hci_bcsp.c
> +++ b/drivers/bluetooth/hci_bcsp.c
> @@ -59,9 +59,6 @@ static u8 conf_rsp_pkt[4] = { 0xde, 0xad, 0xd0, 0xd0 };
> static u8 sync_pkt[4] = { 0xda, 0xdc, 0xed, 0xed };
> static u8 sync_rsp_pkt[4] = { 0xac, 0xaf, 0xef, 0xee };
>
> -
> -#define BCSP_TXWINSIZE 4
> -
> #define BCSP_PKT_LEN(data) ((data[1] >> 4) + (data[2]))
> #define BCSP_LE_PKT_LEN 0x04
>
> @@ -331,7 +328,7 @@ static struct sk_buff *bcsp_dequeue(struct hci_uart *hu)
>
> spin_lock_irqsave_nested(&bcsp->unack.lock, flags, SINGLE_DEPTH_NESTING);
>
> - if (bcsp->unack.qlen < BCSP_TXWINSIZE) {
> + if (bcsp->unack.qlen < CONFIG_BT_HCIUART_BCSP_WINSIZE) {
> skb = skb_dequeue(&bcsp->rel);
> if (skb != NULL) {
> struct sk_buff *nskb = bcsp_prepare_pkt(bcsp, skb->data, skb->len,
This is fine with me, but we might also want to add a module parameter to allow changing it. If people want to play with it then they do not need to rebuild their module.
Another alternative is to provide an ioctl for changing it.
Regards
Marcel
Add the link establishment protocol handling in the kernel:
- le_state enum to handle state machine
- bcsp_timed_event to send LE messages
- bcsp_handle_le_pkt to answer endpoint LE messages
- various tests to muzzle BCSP link before GARULOUS
The patch exports some HCI functions that are required whitin BCSP to properly handle the connection reset.
- hci_dev_reset_stat
- hci_notify
Signed-off-by: Tristan Lelong <[email protected]>
---
drivers/bluetooth/hci_bcsp.c | 202 +++++++++++++++++++++++++++++++--------
include/net/bluetooth/hci_core.h | 2 +
net/bluetooth/hci_core.c | 6 +-
3 files changed, 171 insertions(+), 39 deletions(-)
diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
index 21cc45b..142b42b 100644
--- a/drivers/bluetooth/hci_bcsp.c
+++ b/drivers/bluetooth/hci_bcsp.c
@@ -1,6 +1,5 @@
/*
*
- * Bluetooth HCI UART driver
*
* Copyright (C) 2002-2003 Fabrizio Gennari <[email protected]>
* Copyright (C) 2004-2005 Marcel Holtmann <[email protected]>
@@ -47,13 +46,25 @@
#include "hci_uart.h"
-#define VERSION "0.3"
+#define VERSION "0.4"
+
+#define LE_POLLING (HZ/10)
+#define TX_POLLING (HZ/4)
static bool txcrc = 1;
static bool hciextn = 1;
+static u8 conf_pkt[4] = { 0xad, 0xef, 0xac, 0xed };
+static u8 conf_rsp_pkt[4] = { 0xde, 0xad, 0xd0, 0xd0 };
+static u8 sync_pkt[4] = { 0xda, 0xdc, 0xed, 0xed };
+static u8 sync_rsp_pkt[4] = { 0xac, 0xaf, 0xef, 0xee };
+
+
#define BCSP_TXWINSIZE 4
+#define BCSP_PKT_LEN(data) ((data[1] >> 4) + (data[2]))
+#define BCSP_LE_PKT_LEN 0x04
+
#define BCSP_ACK_PKT 0x05
#define BCSP_LE_PKT 0x06
@@ -69,6 +80,12 @@ struct bcsp_struct {
struct timer_list tbcsp;
enum {
+ BCSP_SHY = 0,
+ BCSP_CURIOUS,
+ BCSP_GARULOUS
+ } le_state;
+
+ enum {
BCSP_W4_PKT_DELIMITER,
BCSP_W4_PKT_START,
BCSP_W4_BCSP_HDR,
@@ -184,6 +201,9 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
u16 BCSP_CRC_INIT(bcsp_txmsg_crc);
int rel, i;
+ if (bcsp->le_state != BCSP_GARULOUS && pkt_type != BCSP_LE_PKT)
+ return NULL;
+
switch (pkt_type) {
case HCI_ACLDATA_PKT:
chan = 6; /* BCSP ACL channel */
@@ -299,6 +319,8 @@ static struct sk_buff *bcsp_dequeue(struct hci_uart *hu)
return nskb;
} else {
skb_queue_head(&bcsp->unrel, skb);
+ if (bcsp->le_state == BCSP_GARULOUS)
+ return NULL;
BT_ERR("Could not dequeue pkt because alloc_skb failed");
}
}
@@ -316,11 +338,18 @@ static struct sk_buff *bcsp_dequeue(struct hci_uart *hu)
bt_cb(skb)->pkt_type);
if (nskb) {
__skb_queue_tail(&bcsp->unack, skb);
- mod_timer(&bcsp->tbcsp, jiffies + HZ / 4);
+
+ if (bcsp->le_state == BCSP_GARULOUS)
+ /* only re-arm retransmit timer
+ * if the BCSP link is connected */
+ mod_timer(&bcsp->tbcsp,
+ jiffies + HZ / 4);
spin_unlock_irqrestore(&bcsp->unack.lock, flags);
return nskb;
} else {
skb_queue_head(&bcsp->rel, skb);
+ if (bcsp->le_state != BCSP_GARULOUS)
+ return NULL;
BT_ERR("Could not dequeue pkt because alloc_skb failed");
}
}
@@ -386,7 +415,7 @@ static void bcsp_pkt_cull(struct bcsp_struct *bcsp)
kfree_skb(skb);
}
- if (skb_queue_empty(&bcsp->unack))
+ if (skb_queue_empty(&bcsp->unack) && bcsp->le_state == BCSP_GARULOUS)
del_timer(&bcsp->tbcsp);
spin_unlock_irqrestore(&bcsp->unack.lock, flags);
@@ -395,34 +424,100 @@ static void bcsp_pkt_cull(struct bcsp_struct *bcsp)
BT_ERR("Removed only %u out of %u pkts", i, pkts_to_be_removed);
}
-/* Handle BCSP link-establishment packets. When we
- detect a "sync" packet, symptom that the BT module has reset,
- we do nothing :) (yet) */
+static void bcsp_internal_reset(struct hci_uart *hu)
+{
+ struct bcsp_struct *bcsp = hu->priv;
+
+ /* reset the bcsp stack counters */
+ bcsp->rxseq_txack = 0;
+ bcsp->rxack = 0;
+ bcsp->message_crc = 0;
+ bcsp->txack_req = 0;
+ bcsp->msgq_txseq = 0;
+
+ /* reset state machine */
+ bcsp->le_state = BCSP_SHY;
+ bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
+ bcsp->rx_esc_state = BCSP_ESCSTATE_NOESC;
+
+ /* empty all the queues */
+ skb_queue_purge(&bcsp->unack);
+ skb_queue_purge(&bcsp->rel);
+ skb_queue_purge(&bcsp->unrel);
+
+ /* tell upper layer about the reset */
+ hci_dev_reset_stat(hu->hdev->id);
+
+ hci_notify(hu->hdev, HCI_DEV_UNREG);
+
+ bcsp->tbcsp.expires = jiffies + LE_POLLING;
+ add_timer(&bcsp->tbcsp);
+}
+
+/* Handle BCSP link-establishment packets.
+ */
static void bcsp_handle_le_pkt(struct hci_uart *hu)
{
+ struct sk_buff *nskb;
struct bcsp_struct *bcsp = hu->priv;
- u8 conf_pkt[4] = { 0xad, 0xef, 0xac, 0xed };
- u8 conf_rsp_pkt[4] = { 0xde, 0xad, 0xd0, 0xd0 };
- u8 sync_pkt[4] = { 0xda, 0xdc, 0xed, 0xed };
-
- /* spot "conf" pkts and reply with a "conf rsp" pkt */
- if (bcsp->rx_skb->data[1] >> 4 == 4 && bcsp->rx_skb->data[2] == 0 &&
- !memcmp(&bcsp->rx_skb->data[4], conf_pkt, 4)) {
- struct sk_buff *nskb = alloc_skb(4, GFP_ATOMIC);
-
- BT_DBG("Found a LE conf pkt");
- if (!nskb)
- return;
- memcpy(skb_put(nskb, 4), conf_rsp_pkt, 4);
- bt_cb(nskb)->pkt_type = BCSP_LE_PKT;
-
- skb_queue_head(&bcsp->unrel, nskb);
- hci_uart_tx_wakeup(hu);
- }
- /* Spot "sync" pkts. If we find one...disaster! */
- else if (bcsp->rx_skb->data[1] >> 4 == 4 && bcsp->rx_skb->data[2] == 0 &&
- !memcmp(&bcsp->rx_skb->data[4], sync_pkt, 4)) {
- BT_ERR("Found a LE sync pkt, card has reset");
+
+ if (BCSP_PKT_LEN(bcsp->rx_skb->data) == BCSP_LE_PKT_LEN) {
+ /* spot "conf" pkts and reply with a "conf rsp" pkt */
+ if (!memcmp(&bcsp->rx_skb->data[4], conf_pkt, 4)) {
+ if (bcsp->le_state == BCSP_SHY)
+ return;
+
+ BT_DBG("Found a LE conf pkt");
+ nskb = alloc_skb(4, GFP_ATOMIC);
+ if (!nskb)
+ return;
+ memcpy(skb_put(nskb, 4), conf_rsp_pkt, 4);
+ bt_cb(nskb)->pkt_type = BCSP_LE_PKT;
+
+ skb_queue_head(&bcsp->unrel, nskb);
+ hci_uart_tx_wakeup(hu);
+ }
+ /* Spot "sync" pkts and reply with a "sync rsp" pkt */
+ else if (!memcmp(&bcsp->rx_skb->data[4], sync_pkt, 4)) {
+ if (bcsp->le_state == BCSP_GARULOUS)
+ /* we force the connection state to reset */
+ bcsp_internal_reset(hu);
+
+ BT_INFO("BCSP: hci%d LE sync pkt, performing reset",
+ hu->hdev->id);
+
+ nskb = alloc_skb(4, GFP_ATOMIC);
+ if (!nskb)
+ return;
+ memcpy(skb_put(nskb, 4), sync_rsp_pkt, 4);
+ bt_cb(nskb)->pkt_type = BCSP_LE_PKT;
+
+ skb_queue_head(&bcsp->unrel, nskb);
+ hci_uart_tx_wakeup(hu);
+ }
+ /* Spot "conf rsp" pkts and update state machine */
+ else if (!memcmp(&bcsp->rx_skb->data[4], conf_rsp_pkt, 4)) {
+ if (bcsp->le_state != BCSP_CURIOUS) {
+ BT_DBG("BCSP: hci%d ingoring conf_resp packet",
+ hu->hdev->id);
+ return;
+ }
+
+ BT_INFO("BCSP: hci%d connected", hu->hdev->id);
+ bcsp->le_state = BCSP_GARULOUS;
+ }
+ /* Spot "sync rsp" pkts and update state machine */
+ else if (!memcmp(&bcsp->rx_skb->data[4], sync_rsp_pkt, 4)) {
+ if (bcsp->le_state != BCSP_SHY) {
+ BT_DBG("BCSP: hci%d ignoring sync_resp packet",
+ hu->hdev->id);
+ return;
+ }
+
+ BT_DBG("Found a LE sync rsp pkt");
+ bcsp->le_state = BCSP_CURIOUS;
+ }
+
}
}
@@ -537,12 +632,13 @@ static void bcsp_complete_rx_pkt(struct hci_uart *hu)
}
} else
kfree_skb(bcsp->rx_skb);
- } else {
+ } else if (bcsp->le_state == BCSP_GARULOUS) {
/* Pull out BCSP hdr */
skb_pull(bcsp->rx_skb, 4);
hci_recv_frame(hu->hdev, bcsp->rx_skb);
- }
+ } else
+ kfree_skb(bcsp->rx_skb);
bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
bcsp->rx_skb = NULL;
@@ -676,16 +772,42 @@ static void bcsp_timed_event(unsigned long arg)
struct sk_buff *skb;
unsigned long flags;
- BT_DBG("hu %p retransmitting %u pkts", hu, bcsp->unack.qlen);
+ switch (bcsp->le_state) {
+ case BCSP_GARULOUS:
+ BT_DBG("hu %p retransmitting %u pkts", hu, bcsp->unack.qlen);
- spin_lock_irqsave_nested(&bcsp->unack.lock, flags, SINGLE_DEPTH_NESTING);
+ spin_lock_irqsave_nested(&bcsp->unack.lock, flags,
+ SINGLE_DEPTH_NESTING);
- while ((skb = __skb_dequeue_tail(&bcsp->unack)) != NULL) {
- bcsp->msgq_txseq = (bcsp->msgq_txseq - 1) & 0x07;
- skb_queue_head(&bcsp->rel, skb);
- }
+ while ((skb = __skb_dequeue_tail(&bcsp->unack)) != NULL) {
+ bcsp->msgq_txseq = (bcsp->msgq_txseq - 1) & 0x07;
+ skb_queue_head(&bcsp->rel, skb);
+ }
- spin_unlock_irqrestore(&bcsp->unack.lock, flags);
+ spin_unlock_irqrestore(&bcsp->unack.lock, flags);
+ break;
+ case BCSP_CURIOUS:
+ skb = alloc_skb(4, GFP_ATOMIC);
+ memcpy(skb_put(skb, 4), conf_pkt, 4);
+ bt_cb(skb)->pkt_type = BCSP_LE_PKT;
+ skb_queue_head(&bcsp->unrel, skb);
+
+ bcsp->tbcsp.expires += LE_POLLING;
+ add_timer(&bcsp->tbcsp);
+ break;
+ default:
+ BT_INFO("Internal state unknown. Assuming not connected.");
+ bcsp->le_state = BCSP_SHY;
+ case BCSP_SHY:
+ skb = alloc_skb(4, GFP_ATOMIC);
+ memcpy(skb_put(skb, 4), sync_pkt, 4);
+ bt_cb(skb)->pkt_type = BCSP_LE_PKT;
+ skb_queue_head(&bcsp->unrel, skb);
+
+ bcsp->tbcsp.expires += LE_POLLING;
+ add_timer(&bcsp->tbcsp);
+ break;
+ }
hci_uart_tx_wakeup(hu);
}
@@ -708,12 +830,16 @@ static int bcsp_open(struct hci_uart *hu)
init_timer(&bcsp->tbcsp);
bcsp->tbcsp.function = bcsp_timed_event;
bcsp->tbcsp.data = (u_long) hu;
+ bcsp->tbcsp.expires = jiffies + LE_POLLING;
bcsp->rx_state = BCSP_W4_PKT_DELIMITER;
+ bcsp->le_state = BCSP_SHY;
if (txcrc)
bcsp->use_crc = 1;
+ add_timer(&bcsp->tbcsp);
+
return 0;
}
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 1dae700..6fd8356 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -735,6 +735,8 @@ int hci_disconnect(struct hci_conn *conn, __u8 reason);
bool hci_setup_sync(struct hci_conn *conn, __u16 handle);
void hci_sco_setup(struct hci_conn *conn, __u8 status);
+void hci_notify(struct hci_dev *hdev, int event);
+
struct hci_conn *hci_conn_add(struct hci_dev *hdev, int type, bdaddr_t *dst,
u8 role);
int hci_conn_del(struct hci_conn *conn);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index f001856..2c5130a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -65,10 +65,12 @@ static DEFINE_IDA(hci_index_ida);
/* ---- HCI notifications ---- */
-static void hci_notify(struct hci_dev *hdev, int event)
+void hci_notify(struct hci_dev *hdev, int event)
{
hci_sock_dev_event(hdev, event);
}
+EXPORT_SYMBOL(hci_notify);
+
/* ---- HCI debugfs entries ---- */
@@ -2783,6 +2785,8 @@ done:
hci_dev_put(hdev);
return ret;
}
+EXPORT_SYMBOL(hci_dev_reset_stat);
+
static void hci_update_scan_state(struct hci_dev *hdev, u8 scan)
{
--
2.1.1
From: Tristan Lelong <[email protected]>
This patch make the TX window size configurable using the kernel configuration interface.
The default value used is the previous value: 4
Signed-off-by: Tristan Lelong <[email protected]>
---
drivers/bluetooth/Kconfig | 7 +++++++
drivers/bluetooth/hci_bcsp.c | 5 +----
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 364f080..9f634cb 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -59,6 +59,13 @@ config BT_HCIUART_BCSP
Say Y here to compile support for HCI BCSP protocol.
+config BT_HCIUART_BCSP_WINSIZE
+ int "BCSP reliable packet TX window size"
+ default 4
+ depends on BT_HCIUART_BCSP
+ help
+ Defines the number of packets that can be sent before receiving a ACK.
+
config BT_HCIUART_ATH3K
bool "Atheros AR300x serial support"
depends on BT_HCIUART
diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
index 142b42b..bb18f2a 100644
--- a/drivers/bluetooth/hci_bcsp.c
+++ b/drivers/bluetooth/hci_bcsp.c
@@ -59,9 +59,6 @@ static u8 conf_rsp_pkt[4] = { 0xde, 0xad, 0xd0, 0xd0 };
static u8 sync_pkt[4] = { 0xda, 0xdc, 0xed, 0xed };
static u8 sync_rsp_pkt[4] = { 0xac, 0xaf, 0xef, 0xee };
-
-#define BCSP_TXWINSIZE 4
-
#define BCSP_PKT_LEN(data) ((data[1] >> 4) + (data[2]))
#define BCSP_LE_PKT_LEN 0x04
@@ -331,7 +328,7 @@ static struct sk_buff *bcsp_dequeue(struct hci_uart *hu)
spin_lock_irqsave_nested(&bcsp->unack.lock, flags, SINGLE_DEPTH_NESTING);
- if (bcsp->unack.qlen < BCSP_TXWINSIZE) {
+ if (bcsp->unack.qlen < CONFIG_BT_HCIUART_BCSP_WINSIZE) {
skb = skb_dequeue(&bcsp->rel);
if (skb != NULL) {
struct sk_buff *nskb = bcsp_prepare_pkt(bcsp, skb->data, skb->len,
--
2.1.1