Skip to content

Commit

Permalink
Revert "FROMLIST: Bluetooth: Always call advertising disable before s…
Browse files Browse the repository at this point in the history 
…etting params"

This reverts commit 398beab.

Reason for revert: Regresses advertising registration on ThP and WP2

Original change's description:
> FROMLIST: Bluetooth: Always call advertising disable before setting params
>
> In __hci_req_enable_advertising, the HCI_LE_ADV hdev flag is temporarily
> cleared to allow the random address to be set, which exposes a race
> condition when an advertisement is configured immediately (<10ms) after
> software rotation starts to refresh an advertisement.
>
> In normal operation, the HCI_LE_ADV flag is updated as follows:
>
> 1. adv_timeout_expire is called, HCI_LE_ADV gets cleared in
>    __hci_req_enable_advertising, but hci_req configures an enable
>    request
> 2. hci_req is run, enable callback re-sets HCI_LE_ADV flag
>
> However, in this race condition, the following occurs:
>
> 1. adv_timeout_expire is called, HCI_LE_ADV gets cleared in
>    __hci_req_enable_advertising, but hci_req configures an enable
>    request
> 2. add_advertising is called, which also calls
>    __hci_req_enable_advertising. Because HCI_LE_ADV was cleared in Step
>    1, no "disable" command is queued.
> 3. hci_req for adv_timeout_expire is run, which enables advertising and
>    re-sets HCI_LE_ADV
> 4. hci_req for add_advertising is run, but because no "disable" command
>    was queued, we try to set advertising parameters while advertising is
>    active, causing a Command Disallowed error, failing the registration.
>
> To resolve the issue, this patch removes the check for the HCI_LE_ADV
> flag, and always queues the "disable" request, since HCI_LE_ADV could be
> very temporarily out-of-sync. According to the spec, there is no harm in
> calling "disable" when advertising is not active.
>
> An example trace showing the HCI error in setting advertising parameters
> is included below, with some notes annotating the states I mentioned
> above:
>
> @ MGMT Command: Add Ext Adv.. (0x0055) plen 35  {0x0001} [hci0]04:05.884
>         Instance: 3
>         Advertising data length: 24
>         16-bit Service UUIDs (complete): 2 entries
>           Location and Navigation (0x1819)
>           Phone Alert Status Service (0x180e)
>         Company: not assigned (65283)
>           Data: 3a3b3c3d3e
>         Service Data (UUID 0x9993): 3132333435
>         Scan response length: 0
> @ MGMT Event: Advertising Ad.. (0x0023) plen 1  {0x0005} [hci0]04:05.885
>         Instance: 3
>
> === adv_timeout_expire request starts running. This request was created
> before our add advertising request
> > HCI Event: Command Complete (0x0e) plen 4         torvalds#220 [hci0]04:05.993
>       LE Set Advertising Data (0x08|0x0008) ncmd 1
>         Status: Success (0x00)
> < HCI Command: LE Set Scan.. (0x08|0x0009) plen 32  torvalds#221 [hci0]04:05.993
>         Length: 24
>         Service Data (UUID 0xabcd): 161718191a1b1c1d1e1f2021222324252627
> > HCI Event: Command Complete (0x0e) plen 4         torvalds#222 [hci0]04:05.995
>       LE Set Scan Response Data (0x08|0x0009) ncmd 1
>         Status: Success (0x00)
> < HCI Command: LE Set Adver.. (0x08|0x000a) plen 1  torvalds#223 [hci0]04:05.995
>         Advertising: Disabled (0x00)
> > HCI Event: Command Complete (0x0e) plen 4         torvalds#224 [hci0]04:05.997
>       LE Set Advertise Enable (0x08|0x000a) ncmd 1
>         Status: Success (0x00)
> < HCI Command: LE Set Adve.. (0x08|0x0006) plen 15  torvalds#225 [hci0]04:05.997
>         Min advertising interval: 200.000 msec (0x0140)
>         Max advertising interval: 200.000 msec (0x0140)
>         Type: Connectable undirected - ADV_IND (0x00)
>         Own address type: Public (0x00)
>         Direct address type: Public (0x00)
>         Direct address: 00:00:00:00:00:00 (OUI 00-00-00)
>         Channel map: 37, 38, 39 (0x07)
>         Filter policy: Allow Scan Request, Connect from Any (0x00)
> > HCI Event: Command Complete (0x0e) plen 4         torvalds#226 [hci0]04:05.998
>       LE Set Advertising Parameters (0x08|0x0006) ncmd 1
>         Status: Success (0x00)
> < HCI Command: LE Set Adver.. (0x08|0x000a) plen 1  torvalds#227 [hci0]04:05.999
>         Advertising: Enabled (0x01)
> > HCI Event: Command Complete (0x0e) plen 4         torvalds#228 [hci0]04:06.000
>       LE Set Advertise Enable (0x08|0x000a) ncmd 1
>         Status: Success (0x00)
>
> === Our new add_advertising request starts running
> < HCI Command: Read Local N.. (0x03|0x0014) plen 0  torvalds#229 [hci0]04:06.001
> > HCI Event: Command Complete (0x0e) plen 252       torvalds#230 [hci0]04:06.005
>       Read Local Name (0x03|0x0014) ncmd 1
>         Status: Success (0x00)
>         Name: Chromebook_FB3D
>
> === Although the controller is advertising, no disable command is sent
> < HCI Command: LE Set Adve.. (0x08|0x0006) plen 15  torvalds#231 [hci0]04:06.005
>         Min advertising interval: 200.000 msec (0x0140)
>         Max advertising interval: 200.000 msec (0x0140)
>         Type: Connectable undirected - ADV_IND (0x00)
>         Own address type: Public (0x00)
>         Direct address type: Public (0x00)
>         Direct address: 00:00:00:00:00:00 (OUI 00-00-00)
>         Channel map: 37, 38, 39 (0x07)
>         Filter policy: Allow Scan Request, Connect from Any (0x00)
> > HCI Event: Command Complete (0x0e) plen 4         torvalds#232 [hci0]04:06.005
>       LE Set Advertising Parameters (0x08|0x0006) ncmd 1
>         Status: Command Disallowed (0x0c)
>
> Reviewed-by: Miao-chen Chou <[email protected]>
> Signed-off-by: Daniel Winkler <[email protected]>
> (am from https://patchwork.kernel.org/patch/12162043/)
> (also found at https://lore.kernel.org/r/20210324114645.v2.1.I53e6be1f7df0be198b7e55ae9fc45c7f5760132d@changeid)
>
> BUG=b:182382092
> TEST=AdvHealth on kefka chromebook
>
> Change-Id: I53e6be1f7df0be198b7e55ae9fc45c7f5760132d
> Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2775994
> Reviewed-by: Sean Paul <[email protected]>
> Reviewed-by: Yu Liu <[email protected]>
> Reviewed-by: Alain Michaud <[email protected]>
> Reviewed-by: Miao-chen Chou <[email protected]>
> Commit-Queue: Daniel Winkler <[email protected]>
> Tested-by: Daniel Winkler <[email protected]>

BUG=b:182382092
Change-Id: I5be2649e7887bc3a8139ea18037afd28e929d3b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2792003
Bot-Commit: Rubber Stamper <[email protected]>
Commit-Queue: Daniel Winkler <[email protected]>
  • Loading branch information
@dwinkler2
dwinkler2 authored and Commit Bot committed Mar 30, 2021
1 parent 0b00207 commit 49be17e
Showing 1 changed file with 2 additions and 4 deletions.
6 changes: 2 additions & 4 deletions net/bluetooth/hci_request.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1629,10 +1629,8 @@ void __hci_req_enable_advertising(struct hci_request *req)
if (!is_advertising_allowed(hdev, connectable))
return;

/* Request that the controller stop advertising. This can be called
* whether or not there is an active advertisement.
*/
__hci_req_disable_advertising(req);
if (hci_dev_test_flag(hdev, HCI_LE_ADV))
__hci_req_disable_advertising(req);

/* Clear the HCI_LE_ADV bit temporarily so that the
* hci_update_random_address knows that it's safe to go ahead
Expand Down

0 comments on commit 49be17e

Please sign in to comment.