[kernel PATCH v1 0/1] Reason to disable adv during power off without clearing

[kernel PATCH v1 0/1] Reason to disable adv during power off without clearing

[Zhengping Jiang]

This change was made to keep the same behavior as before the hci_sync
rework. When the adapter is powered off, the advertisement will be
disabled, instead of being cleared. As a result, when the adapter is powered
on, the advertisement can be re-enabled. There is no need to re-register
the same advertisement.

However, if the adv is not cleared and the user requested to remove the
adv when the adapter is off, it will trigger GPIO reset, so we will not
send the HCI command in this case.

Changes in v1:
- Mark the advertisement as disabled instead of clearing it.
- Remove the advertisement without sending HCI command if the adapter is off.

Archie Pusaka (1):
  Bluetooth: Don't send HCI commands to remove adv if adapter is off

 net/bluetooth/hci_sync.c | 57 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 53 insertions(+), 4 deletions(-)

-- 
2.39.1.456.gfc5497dd1b-goog

[kernel PATCH v1 1/1] Bluetooth: Don't send HCI commands to remove adv if adapter is off

[Zhengping Jiang]

From: Archie Pusaka <apusaka@chromium.org>

Mark the advertisement as disabled when powering off the adapter
without removing the advertisement, so they can be correctly
re-enabled when adapter is powered on again.

When adapter is off and user requested to remove advertisement,
a HCI command will be issued. This causes the command to timeout
and trigger GPIO reset.

Therefore, immediately remove the advertisement without sending
any HCI commands.

Note that the above scenario only happens with extended advertisement
(i.e. not using software rotation), because on the SW rotation
scenario, we just wait until the rotation timer runs out before
sending the HCI command. Since the timer is inactive when adapter is
off, no HCI commands are sent.

Signed-off-by: Archie Pusaka <apusaka@chromium.org>
Signed-off-by: Zhengping Jiang <jiangzp@google.com>

---

Changes in v1:
- Mark the advertisement as disabled instead of clearing it.
- Remove the advertisement without sending HCI command if the adapter is off.

 net/bluetooth/hci_sync.c | 57 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 53 insertions(+), 4 deletions(-)

diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
index 117eedb6f709..08da68a30acc 100644
--- a/net/bluetooth/hci_sync.c
+++ b/net/bluetooth/hci_sync.c
@@ -1591,6 +1591,16 @@ int hci_remove_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance,
 	if (!ext_adv_capable(hdev))
 		return 0;
 
+	/* When adapter is off, remove adv without sending HCI commands */
+	if (!hdev_is_powered(hdev)) {
+		hci_dev_lock(hdev);
+		err = hci_remove_adv_instance(hdev, instance);
+		if (!err)
+			mgmt_advertising_removed(sk, hdev, instance);
+		hci_dev_unlock(hdev);
+		return err;
+	}
+
 	err = hci_disable_ext_adv_instance_sync(hdev, instance);
 	if (err)
 		return err;
@@ -1772,6 +1782,23 @@ int hci_schedule_adv_instance_sync(struct hci_dev *hdev, u8 instance,
 	return hci_start_adv_sync(hdev, instance);
 }
 
+static void hci_clear_ext_adv_ins_during_power_off(struct hci_dev *hdev,
+						   struct sock *sk)
+{
+	struct adv_info *adv, *n;
+	int err;
+
+	hci_dev_lock(hdev);
+	list_for_each_entry_safe(adv, n, &hdev->adv_instances, list) {
+		u8 instance = adv->instance;
+
+		err = hci_remove_adv_instance(hdev, instance);
+		if (!err)
+			mgmt_advertising_removed(sk, hdev, instance);
+	}
+	hci_dev_unlock(hdev);
+}
+
 static int hci_clear_adv_sets_sync(struct hci_dev *hdev, struct sock *sk)
 {
 	int err;
@@ -1779,6 +1806,12 @@ static int hci_clear_adv_sets_sync(struct hci_dev *hdev, struct sock *sk)
 	if (!ext_adv_capable(hdev))
 		return 0;
 
+	/* When adapter is off, remove adv without sending HCI commands */
+	if (!hdev_is_powered(hdev)) {
+		hci_clear_ext_adv_ins_during_power_off(hdev, sk);
+		return 0;
+	}
+
 	/* Disable instance 0x00 to disable all instances */
 	err = hci_disable_ext_adv_instance_sync(hdev, 0x00);
 	if (err)
@@ -5177,9 +5210,27 @@ static int hci_disconnect_all_sync(struct hci_dev *hdev, u8 reason)
 	return 0;
 }
 
+static void hci_disable_ext_advertising_temporarily(struct hci_dev *hdev)
+{
+	struct adv_info *adv, *n;
+
+	if (!ext_adv_capable(hdev))
+		return;
+
+	hci_dev_lock(hdev);
+
+	list_for_each_entry_safe(adv, n, &hdev->adv_instances, list)
+		adv->enabled = false;
+
+	hci_dev_clear_flag(hdev, HCI_LE_ADV);
+
+	hci_dev_unlock(hdev);
+}
+
 /* This function perform power off HCI command sequence as follows:
  *
- * Clear Advertising
+ * Disable Advertising Instances. Do not clear adv instances so advertising
+ * can be re-enabled on power on.
  * Stop Discovery
  * Disconnect all connections
  * hci_dev_close_sync
@@ -5199,9 +5250,7 @@ static int hci_power_off_sync(struct hci_dev *hdev)
 			return err;
 	}
 
-	err = hci_clear_adv_sync(hdev, NULL, false);
-	if (err)
-		return err;
+	hci_disable_ext_advertising_temporarily(hdev);
 
 	err = hci_stop_discovery_sync(hdev);
 	if (err)
-- 
2.39.1.456.gfc5497dd1b-goog

Re: [kernel PATCH v1 1/1] Bluetooth: Don't send HCI commands to remove adv if adapter is off

[Luiz Augusto von Dentz]

Hi Zhengping,

On Wed, Jan 25, 2023 at 1:12 PM Zhengping Jiang <jiangzp@google.com> wrote:
>
> From: Archie Pusaka <apusaka@chromium.org>
>
> Mark the advertisement as disabled when powering off the adapter
> without removing the advertisement, so they can be correctly
> re-enabled when adapter is powered on again.
>
> When adapter is off and user requested to remove advertisement,
> a HCI command will be issued. This causes the command to timeout
> and trigger GPIO reset.

Please include the btmon portion when the issue occurs.

> Therefore, immediately remove the advertisement without sending
> any HCI commands.
>
> Note that the above scenario only happens with extended advertisement
> (i.e. not using software rotation), because on the SW rotation
> scenario, we just wait until the rotation timer runs out before
> sending the HCI command. Since the timer is inactive when adapter is
> off, no HCI commands are sent.
>
> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> Signed-off-by: Zhengping Jiang <jiangzp@google.com>
>
> ---
>
> Changes in v1:
> - Mark the advertisement as disabled instead of clearing it.
> - Remove the advertisement without sending HCI command if the adapter is off.

Perhaps we should split these into 2 separated changes then, on top of
it it perhaps would be a good idea to implement a test in mgmt-tester
to check the correct behavior.

>  net/bluetooth/hci_sync.c | 57 +++++++++++++++++++++++++++++++++++++---
>  1 file changed, 53 insertions(+), 4 deletions(-)
>
> diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c
> index 117eedb6f709..08da68a30acc 100644
> --- a/net/bluetooth/hci_sync.c
> +++ b/net/bluetooth/hci_sync.c
> @@ -1591,6 +1591,16 @@ int hci_remove_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance,
>         if (!ext_adv_capable(hdev))
>                 return 0;
>
> +       /* When adapter is off, remove adv without sending HCI commands */
> +       if (!hdev_is_powered(hdev)) {
> +               hci_dev_lock(hdev);
> +               err = hci_remove_adv_instance(hdev, instance);
> +               if (!err)
> +                       mgmt_advertising_removed(sk, hdev, instance);

This code above is duplicated in a few places so we might as well have
it as a separate function e.g. hci_remove_adv

> +               hci_dev_unlock(hdev);
> +               return err;
> +       }
> +
>         err = hci_disable_ext_adv_instance_sync(hdev, instance);
>         if (err)
>                 return err;
> @@ -1772,6 +1782,23 @@ int hci_schedule_adv_instance_sync(struct hci_dev *hdev, u8 instance,
>         return hci_start_adv_sync(hdev, instance);
>  }
>
> +static void hci_clear_ext_adv_ins_during_power_off(struct hci_dev *hdev,
> +                                                  struct sock *sk)
> +{
> +       struct adv_info *adv, *n;
> +       int err;
> +
> +       hci_dev_lock(hdev);
> +       list_for_each_entry_safe(adv, n, &hdev->adv_instances, list) {
> +               u8 instance = adv->instance;
> +
> +               err = hci_remove_adv_instance(hdev, instance);
> +               if (!err)
> +                       mgmt_advertising_removed(sk, hdev, instance);

Then just call hci_remove_adv.

> +       }
> +       hci_dev_unlock(hdev);
> +}
> +
>  static int hci_clear_adv_sets_sync(struct hci_dev *hdev, struct sock *sk)
>  {
>         int err;
> @@ -1779,6 +1806,12 @@ static int hci_clear_adv_sets_sync(struct hci_dev *hdev, struct sock *sk)
>         if (!ext_adv_capable(hdev))
>                 return 0;
>
> +       /* When adapter is off, remove adv without sending HCI commands */
> +       if (!hdev_is_powered(hdev)) {
> +               hci_clear_ext_adv_ins_during_power_off(hdev, sk);

Use the remove to describe the action e.g. hci_remove_all_adv.

> +               return 0;
> +       }
> +
>         /* Disable instance 0x00 to disable all instances */
>         err = hci_disable_ext_adv_instance_sync(hdev, 0x00);
>         if (err)
> @@ -5177,9 +5210,27 @@ static int hci_disconnect_all_sync(struct hci_dev *hdev, u8 reason)
>         return 0;
>  }
>
> +static void hci_disable_ext_advertising_temporarily(struct hci_dev *hdev)
> +{
> +       struct adv_info *adv, *n;
> +
> +       if (!ext_adv_capable(hdev))
> +               return;
> +
> +       hci_dev_lock(hdev);
> +
> +       list_for_each_entry_safe(adv, n, &hdev->adv_instances, list)
> +               adv->enabled = false;
> +
> +       hci_dev_clear_flag(hdev, HCI_LE_ADV);
> +
> +       hci_dev_unlock(hdev);
> +}
> +
>  /* This function perform power off HCI command sequence as follows:
>   *
> - * Clear Advertising
> + * Disable Advertising Instances. Do not clear adv instances so advertising
> + * can be re-enabled on power on.
>   * Stop Discovery
>   * Disconnect all connections
>   * hci_dev_close_sync
> @@ -5199,9 +5250,7 @@ static int hci_power_off_sync(struct hci_dev *hdev)
>                         return err;
>         }
>
> -       err = hci_clear_adv_sync(hdev, NULL, false);
> -       if (err)
> -               return err;
> +       hci_disable_ext_advertising_temporarily(hdev);

Something like hci_disable_all_adv sounds better here.

>
>         err = hci_stop_discovery_sync(hdev);
>         if (err)
> --
> 2.39.1.456.gfc5497dd1b-goog
>


-- 
Luiz Augusto von Dentz