2017-06-09 16:43:56

by Marcel Holtmann

[permalink] [raw]
Subject: [PATCH] Bluetooth: Send HCI Set Event Mask Page 2 command only when needed

The Broadcom BCM20702 Bluetooth controller in ThinkPad-T530 devices
report support for the Set Event Mask Page 2 command, but actually do
return an error when trying to use it.

< HCI Command: Read Local Supported Commands (0x04|0x0002) plen 0
> HCI Event: Command Complete (0x0e) plen 68
Read Local Supported Commands (0x04|0x0002) ncmd 1
Status: Success (0x00)
Commands: 162 entries
...
Set Event Mask Page 2 (Octet 22 - Bit 2)
...

< HCI Command: Set Event Mask Page 2 (0x03|0x0063) plen 8
Mask: 0x0000000000000000
> HCI Event: Command Complete (0x0e) plen 4
Set Event Mask Page 2 (0x03|0x0063) ncmd 1
Status: Unknown HCI Command (0x01)

Since these controllers do not support any feature that would require
the event mask page 2 to be modified, it is safe to not send this
command at all. The default value is all bits set to zero.

T: Bus=01 Lev=02 Prnt=02 Port=03 Cnt=03 Dev#= 9 Spd=12 MxCh= 0
D: Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs= 1
P: Vendor=0a5c ProdID=21e6 Rev= 1.12
S: Manufacturer=Broadcom Corp
S: Product=BCM20702A0
S: SerialNumber=F82FA8E8CFC0
C:* #Ifs= 4 Cfg#= 1 Atr=e0 MxPwr= 0mA
I:* If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
E: Ad=81(I) Atr=03(Int.) MxPS= 16 Ivl=1ms
E: Ad=82(I) Atr=02(Bulk) MxPS= 64 Ivl=0ms
E: Ad=02(O) Atr=02(Bulk) MxPS= 64 Ivl=0ms
I:* If#= 1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
E: Ad=83(I) Atr=01(Isoc) MxPS= 0 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 0 Ivl=1ms
I: If#= 1 Alt= 1 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
E: Ad=83(I) Atr=01(Isoc) MxPS= 9 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 9 Ivl=1ms
I: If#= 1 Alt= 2 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
E: Ad=83(I) Atr=01(Isoc) MxPS= 17 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 17 Ivl=1ms
I: If#= 1 Alt= 3 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
E: Ad=83(I) Atr=01(Isoc) MxPS= 25 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 25 Ivl=1ms
I: If#= 1 Alt= 4 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
E: Ad=83(I) Atr=01(Isoc) MxPS= 33 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 33 Ivl=1ms
I: If#= 1 Alt= 5 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
E: Ad=83(I) Atr=01(Isoc) MxPS= 49 Ivl=1ms
E: Ad=03(O) Atr=01(Isoc) MxPS= 49 Ivl=1ms
I:* If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=btusb
E: Ad=84(I) Atr=02(Bulk) MxPS= 32 Ivl=0ms
E: Ad=04(O) Atr=02(Bulk) MxPS= 32 Ivl=0ms
I:* If#= 3 Alt= 0 #EPs= 0 Cls=fe(app. ) Sub=01 Prot=01 Driver=(none)

Signed-off-by: Marcel Holtmann <[email protected]>
---
net/bluetooth/hci_core.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7655b4005dfb..93806b959039 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -548,6 +548,7 @@ static void hci_set_event_mask_page_2(struct hci_request *req)
{
struct hci_dev *hdev = req->hdev;
u8 events[8] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
+ bool changed = false;

/* If Connectionless Slave Broadcast master role is supported
* enable all necessary events for it.
@@ -557,6 +558,7 @@ static void hci_set_event_mask_page_2(struct hci_request *req)
events[1] |= 0x80; /* Synchronization Train Complete */
events[2] |= 0x10; /* Slave Page Response Timeout */
events[2] |= 0x20; /* CSB Channel Map Change */
+ changed = true;
}

/* If Connectionless Slave Broadcast slave role is supported
@@ -567,13 +569,24 @@ static void hci_set_event_mask_page_2(struct hci_request *req)
events[2] |= 0x02; /* CSB Receive */
events[2] |= 0x04; /* CSB Timeout */
events[2] |= 0x08; /* Truncated Page Complete */
+ changed = true;
}

/* Enable Authenticated Payload Timeout Expired event if supported */
- if (lmp_ping_capable(hdev) || hdev->le_features[0] & HCI_LE_PING)
+ if (lmp_ping_capable(hdev) || hdev->le_features[0] & HCI_LE_PING) {
events[2] |= 0x80;
+ changed = true;
+ }

- hci_req_add(req, HCI_OP_SET_EVENT_MASK_PAGE_2, sizeof(events), events);
+ /* Some Broadcom based controllers indicate support for Set Event
+ * Mask Page 2 command, but then actually do not support it. Since
+ * the default value is all bits set to zero, the command is only
+ * required if the event mask has to be changed. In case no change
+ * to the event mask is needed, skip this command.
+ */
+ if (changed)
+ hci_req_add(req, HCI_OP_SET_EVENT_MASK_PAGE_2,
+ sizeof(events), events);
}

static int hci_init3_req(struct hci_request *req, unsigned long opt)
--
2.9.4



2017-06-12 09:49:42

by Szymon Janc

[permalink] [raw]
Subject: Re: [PATCH] Bluetooth: Send HCI Set Event Mask Page 2 command only when needed

Hi Marcel,

On Friday, 9 June 2017 18:43:56 CEST Marcel Holtmann wrote:
> The Broadcom BCM20702 Bluetooth controller in ThinkPad-T530 devices
> report support for the Set Event Mask Page 2 command, but actually do
> return an error when trying to use it.
>
> < HCI Command: Read Local Supported Commands (0x04|0x0002) plen 0
>
> > HCI Event: Command Complete (0x0e) plen 68
>
> Read Local Supported Commands (0x04|0x0002) ncmd 1
> Status: Success (0x00)
> Commands: 162 entries
> ...
> Set Event Mask Page 2 (Octet 22 - Bit 2)
> ...
>
> < HCI Command: Set Event Mask Page 2 (0x03|0x0063) plen 8
> Mask: 0x0000000000000000
>
> > HCI Event: Command Complete (0x0e) plen 4
>
> Set Event Mask Page 2 (0x03|0x0063) ncmd 1
> Status: Unknown HCI Command (0x01)
>
> Since these controllers do not support any feature that would require
> the event mask page 2 to be modified, it is safe to not send this
> command at all. The default value is all bits set to zero.
>
> T: Bus=01 Lev=02 Prnt=02 Port=03 Cnt=03 Dev#= 9 Spd=12 MxCh= 0
> D: Ver= 2.00 Cls=ff(vend.) Sub=01 Prot=01 MxPS=64 #Cfgs= 1
> P: Vendor=0a5c ProdID=21e6 Rev= 1.12
> S: Manufacturer=Broadcom Corp
> S: Product=BCM20702A0
> S: SerialNumber=F82FA8E8CFC0
> C:* #Ifs= 4 Cfg#= 1 Atr=e0 MxPwr= 0mA
> I:* If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> E: Ad=81(I) Atr=03(Int.) MxPS= 16 Ivl=1ms
> E: Ad=82(I) Atr=02(Bulk) MxPS= 64 Ivl=0ms
> E: Ad=02(O) Atr=02(Bulk) MxPS= 64 Ivl=0ms
> I:* If#= 1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> E: Ad=83(I) Atr=01(Isoc) MxPS= 0 Ivl=1ms
> E: Ad=03(O) Atr=01(Isoc) MxPS= 0 Ivl=1ms
> I: If#= 1 Alt= 1 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> E: Ad=83(I) Atr=01(Isoc) MxPS= 9 Ivl=1ms
> E: Ad=03(O) Atr=01(Isoc) MxPS= 9 Ivl=1ms
> I: If#= 1 Alt= 2 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> E: Ad=83(I) Atr=01(Isoc) MxPS= 17 Ivl=1ms
> E: Ad=03(O) Atr=01(Isoc) MxPS= 17 Ivl=1ms
> I: If#= 1 Alt= 3 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> E: Ad=83(I) Atr=01(Isoc) MxPS= 25 Ivl=1ms
> E: Ad=03(O) Atr=01(Isoc) MxPS= 25 Ivl=1ms
> I: If#= 1 Alt= 4 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> E: Ad=83(I) Atr=01(Isoc) MxPS= 33 Ivl=1ms
> E: Ad=03(O) Atr=01(Isoc) MxPS= 33 Ivl=1ms
> I: If#= 1 Alt= 5 #EPs= 2 Cls=ff(vend.) Sub=01 Prot=01 Driver=btusb
> E: Ad=83(I) Atr=01(Isoc) MxPS= 49 Ivl=1ms
> E: Ad=03(O) Atr=01(Isoc) MxPS= 49 Ivl=1ms
> I:* If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=btusb
> E: Ad=84(I) Atr=02(Bulk) MxPS= 32 Ivl=0ms
> E: Ad=04(O) Atr=02(Bulk) MxPS= 32 Ivl=0ms
> I:* If#= 3 Alt= 0 #EPs= 0 Cls=fe(app. ) Sub=01 Prot=01 Driver=(none)
>
> Signed-off-by: Marcel Holtmann <[email protected]>
> ---
> net/bluetooth/hci_core.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 7655b4005dfb..93806b959039 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -548,6 +548,7 @@ static void hci_set_event_mask_page_2(struct hci_request
> *req) {
> struct hci_dev *hdev = req->hdev;
> u8 events[8] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 };
> + bool changed = false;
>
> /* If Connectionless Slave Broadcast master role is supported
> * enable all necessary events for it.
> @@ -557,6 +558,7 @@ static void hci_set_event_mask_page_2(struct hci_request
> *req) events[1] |= 0x80; /* Synchronization Train Complete */
> events[2] |= 0x10; /* Slave Page Response Timeout */
> events[2] |= 0x20; /* CSB Channel Map Change */
> + changed = true;
> }
>
> /* If Connectionless Slave Broadcast slave role is supported
> @@ -567,13 +569,24 @@ static void hci_set_event_mask_page_2(struct
> hci_request *req) events[2] |= 0x02; /* CSB Receive */
> events[2] |= 0x04; /* CSB Timeout */
> events[2] |= 0x08; /* Truncated Page Complete */
> + changed = true;
> }
>
> /* Enable Authenticated Payload Timeout Expired event if supported */
> - if (lmp_ping_capable(hdev) || hdev->le_features[0] & HCI_LE_PING)
> + if (lmp_ping_capable(hdev) || hdev->le_features[0] & HCI_LE_PING) {
> events[2] |= 0x80;
> + changed = true;
> + }
>
> - hci_req_add(req, HCI_OP_SET_EVENT_MASK_PAGE_2, sizeof(events), events);
> + /* Some Broadcom based controllers indicate support for Set Event
> + * Mask Page 2 command, but then actually do not support it. Since
> + * the default value is all bits set to zero, the command is only
> + * required if the event mask has to be changed. In case no change
> + * to the event mask is needed, skip this command.
> + */
> + if (changed)
> + hci_req_add(req, HCI_OP_SET_EVENT_MASK_PAGE_2,
> + sizeof(events), events);
> }
>
> static int hci_init3_req(struct hci_request *req, unsigned long opt)

Applied to bluetooth-next tree. Thanks.

--
pozdrawiam
Szymon Janc