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
From: Archie Pusaka <[email protected]>
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 <[email protected]>
Signed-off-by: Zhengping Jiang <[email protected]>
---
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
Hi Zhengping,
On Wed, Jan 25, 2023 at 1:12 PM Zhengping Jiang <[email protected]> wrote:
>
> From: Archie Pusaka <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Zhengping Jiang <[email protected]>
>
> ---
>
> 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
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=715641
---Test result---
Test Summary:
CheckPatch PASS 0.65 seconds
GitLint FAIL 0.85 seconds
SubjectPrefix PASS 0.09 seconds
BuildKernel PASS 31.31 seconds
CheckAllWarning PASS 33.97 seconds
CheckSparse PASS 38.25 seconds
CheckSmatch PASS 107.12 seconds
BuildKernel32 PASS 29.96 seconds
TestRunnerSetup PASS 430.17 seconds
TestRunner_l2cap-tester PASS 16.16 seconds
TestRunner_iso-tester PASS 16.36 seconds
TestRunner_bnep-tester PASS 5.43 seconds
TestRunner_mgmt-tester FAIL 110.68 seconds
TestRunner_rfcomm-tester PASS 8.62 seconds
TestRunner_sco-tester PASS 7.99 seconds
TestRunner_ioctl-tester PASS 9.42 seconds
TestRunner_mesh-tester PASS 6.79 seconds
TestRunner_smp-tester PASS 7.81 seconds
TestRunner_userchan-tester PASS 5.73 seconds
IncrementalBuild PASS 27.75 seconds
Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint
Output:
[kernel,v1,1/1] Bluetooth: Don't send HCI commands to remove adv if adapter is off
WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
1: T1 Title exceeds max length (82>80): "[kernel,v1,1/1] Bluetooth: Don't send HCI commands to remove adv if adapter is off"
##############################
Test: TestRunner_mgmt-tester - FAIL
Desc: Run mgmt-tester with test-runner
Output:
Total: 494, Passed: 493 (99.8%), Failed: 1, Not Run: 0
Failed Test Cases
Add Advertising - Success 18 (Power -> off, Remove) Timed out 2.314 seconds
---
Regards,
Linux Bluetooth
Hi,
On Wed, Jan 25, 2023 at 2:06 PM <[email protected]> wrote:
>
> This is automated email and please do not reply to this email!
>
> Dear submitter,
>
> Thank you for submitting the patches to the linux bluetooth mailing list.
> This is a CI test results with your patch series:
> PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=715641
>
> ---Test result---
>
> Test Summary:
> CheckPatch PASS 0.65 seconds
> GitLint FAIL 0.85 seconds
> SubjectPrefix PASS 0.09 seconds
> BuildKernel PASS 31.31 seconds
> CheckAllWarning PASS 33.97 seconds
> CheckSparse PASS 38.25 seconds
> CheckSmatch PASS 107.12 seconds
> BuildKernel32 PASS 29.96 seconds
> TestRunnerSetup PASS 430.17 seconds
> TestRunner_l2cap-tester PASS 16.16 seconds
> TestRunner_iso-tester PASS 16.36 seconds
> TestRunner_bnep-tester PASS 5.43 seconds
> TestRunner_mgmt-tester FAIL 110.68 seconds
> TestRunner_rfcomm-tester PASS 8.62 seconds
> TestRunner_sco-tester PASS 7.99 seconds
> TestRunner_ioctl-tester PASS 9.42 seconds
> TestRunner_mesh-tester PASS 6.79 seconds
> TestRunner_smp-tester PASS 7.81 seconds
> TestRunner_userchan-tester PASS 5.73 seconds
> IncrementalBuild PASS 27.75 seconds
>
> Details
> ##############################
> Test: GitLint - FAIL
> Desc: Run gitlint
> Output:
> [kernel,v1,1/1] Bluetooth: Don't send HCI commands to remove adv if adapter is off
>
> WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
> 1: T1 Title exceeds max length (82>80): "[kernel,v1,1/1] Bluetooth: Don't send HCI commands to remove adv if adapter is off"
> ##############################
> Test: TestRunner_mgmt-tester - FAIL
> Desc: Run mgmt-tester with test-runner
> Output:
> Total: 494, Passed: 493 (99.8%), Failed: 1, Not Run: 0
>
> Failed Test Cases
> Add Advertising - Success 18 (Power -> off, Remove) Timed out 2.314 seconds
Looks like there is something not quite right wrt assumption that
power off don't remove clear the adv, at least this test says
otherwise and this test is actually quite old so I don't know if it
has always been like that or we did change this behavior and forgot to
change the test, anyway this explains why we have done the clearing
with cmd_sync work since that is what this test expects.
>
> ---
> Regards,
> Linux Bluetooth
>
--
Luiz Augusto von Dentz
Hi Luiz,
Thanks for the comment. Is there a way to get more information about
the failed test? After we cherry picked the hci_sync rework to our
branch, some existing tests failed. The failed test will register a
few advertisements, then run a power cycle. After the power cycle, the
advertisements should be re-enabled automatically. I believe this used
to be the behavior before the hci_sync rework? Now calling
"hci_clear_adv_sync" during hci_power_off_sync will remove all
advertisement instances, so the client code needs to "manually"
re-register the advertisement after powering on the adapter. Please
let me know if I understood correctly.
Thanks,
Zhengping
On Wed, Jan 25, 2023 at 3:47 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi,
>
> On Wed, Jan 25, 2023 at 2:06 PM <[email protected]> wrote:
> >
> > This is automated email and please do not reply to this email!
> >
> > Dear submitter,
> >
> > Thank you for submitting the patches to the linux bluetooth mailing list.
> > This is a CI test results with your patch series:
> > PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=715641
> >
> > ---Test result---
> >
> > Test Summary:
> > CheckPatch PASS 0.65 seconds
> > GitLint FAIL 0.85 seconds
> > SubjectPrefix PASS 0.09 seconds
> > BuildKernel PASS 31.31 seconds
> > CheckAllWarning PASS 33.97 seconds
> > CheckSparse PASS 38.25 seconds
> > CheckSmatch PASS 107.12 seconds
> > BuildKernel32 PASS 29.96 seconds
> > TestRunnerSetup PASS 430.17 seconds
> > TestRunner_l2cap-tester PASS 16.16 seconds
> > TestRunner_iso-tester PASS 16.36 seconds
> > TestRunner_bnep-tester PASS 5.43 seconds
> > TestRunner_mgmt-tester FAIL 110.68 seconds
> > TestRunner_rfcomm-tester PASS 8.62 seconds
> > TestRunner_sco-tester PASS 7.99 seconds
> > TestRunner_ioctl-tester PASS 9.42 seconds
> > TestRunner_mesh-tester PASS 6.79 seconds
> > TestRunner_smp-tester PASS 7.81 seconds
> > TestRunner_userchan-tester PASS 5.73 seconds
> > IncrementalBuild PASS 27.75 seconds
> >
> > Details
> > ##############################
> > Test: GitLint - FAIL
> > Desc: Run gitlint
> > Output:
> > [kernel,v1,1/1] Bluetooth: Don't send HCI commands to remove adv if adapter is off
> >
> > WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
> > 1: T1 Title exceeds max length (82>80): "[kernel,v1,1/1] Bluetooth: Don't send HCI commands to remove adv if adapter is off"
> > ##############################
> > Test: TestRunner_mgmt-tester - FAIL
> > Desc: Run mgmt-tester with test-runner
> > Output:
> > Total: 494, Passed: 493 (99.8%), Failed: 1, Not Run: 0
> >
> > Failed Test Cases
> > Add Advertising - Success 18 (Power -> off, Remove) Timed out 2.314 seconds
>
> Looks like there is something not quite right wrt assumption that
> power off don't remove clear the adv, at least this test says
> otherwise and this test is actually quite old so I don't know if it
> has always been like that or we did change this behavior and forgot to
> change the test, anyway this explains why we have done the clearing
> with cmd_sync work since that is what this test expects.
>
> >
> > ---
> > Regards,
> > Linux Bluetooth
> >
>
>
> --
> Luiz Augusto von Dentz
Hi Tedd,
On Wed, Jan 25, 2023 at 8:34 PM Zhengping Jiang <[email protected]> wrote:
>
> Hi Luiz,
>
> Thanks for the comment. Is there a way to get more information about
> the failed test? After we cherry picked the hci_sync rework to our
> branch, some existing tests failed. The failed test will register a
> few advertisements, then run a power cycle. After the power cycle, the
> advertisements should be re-enabled automatically. I believe this used
> to be the behavior before the hci_sync rework? Now calling
> "hci_clear_adv_sync" during hci_power_off_sync will remove all
> advertisement instances, so the client code needs to "manually"
> re-register the advertisement after powering on the adapter. Please
> let me know if I understood correctly.
What test are you talking about? The one in mgmt-tester which the CI
runs or your own tests?
@Tedd Ho-Jeong An Perhaps we could add the github PR link to pw that
way people can inspect the errors.
> Thanks,
> Zhengping
>
> On Wed, Jan 25, 2023 at 3:47 PM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi,
> >
> > On Wed, Jan 25, 2023 at 2:06 PM <[email protected]> wrote:
> > >
> > > This is automated email and please do not reply to this email!
> > >
> > > Dear submitter,
> > >
> > > Thank you for submitting the patches to the linux bluetooth mailing list.
> > > This is a CI test results with your patch series:
> > > PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=715641
> > >
> > > ---Test result---
> > >
> > > Test Summary:
> > > CheckPatch PASS 0.65 seconds
> > > GitLint FAIL 0.85 seconds
> > > SubjectPrefix PASS 0.09 seconds
> > > BuildKernel PASS 31.31 seconds
> > > CheckAllWarning PASS 33.97 seconds
> > > CheckSparse PASS 38.25 seconds
> > > CheckSmatch PASS 107.12 seconds
> > > BuildKernel32 PASS 29.96 seconds
> > > TestRunnerSetup PASS 430.17 seconds
> > > TestRunner_l2cap-tester PASS 16.16 seconds
> > > TestRunner_iso-tester PASS 16.36 seconds
> > > TestRunner_bnep-tester PASS 5.43 seconds
> > > TestRunner_mgmt-tester FAIL 110.68 seconds
> > > TestRunner_rfcomm-tester PASS 8.62 seconds
> > > TestRunner_sco-tester PASS 7.99 seconds
> > > TestRunner_ioctl-tester PASS 9.42 seconds
> > > TestRunner_mesh-tester PASS 6.79 seconds
> > > TestRunner_smp-tester PASS 7.81 seconds
> > > TestRunner_userchan-tester PASS 5.73 seconds
> > > IncrementalBuild PASS 27.75 seconds
> > >
> > > Details
> > > ##############################
> > > Test: GitLint - FAIL
> > > Desc: Run gitlint
> > > Output:
> > > [kernel,v1,1/1] Bluetooth: Don't send HCI commands to remove adv if adapter is off
> > >
> > > WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
> > > 1: T1 Title exceeds max length (82>80): "[kernel,v1,1/1] Bluetooth: Don't send HCI commands to remove adv if adapter is off"
> > > ##############################
> > > Test: TestRunner_mgmt-tester - FAIL
> > > Desc: Run mgmt-tester with test-runner
> > > Output:
> > > Total: 494, Passed: 493 (99.8%), Failed: 1, Not Run: 0
> > >
> > > Failed Test Cases
> > > Add Advertising - Success 18 (Power -> off, Remove) Timed out 2.314 seconds
> >
> > Looks like there is something not quite right wrt assumption that
> > power off don't remove clear the adv, at least this test says
> > otherwise and this test is actually quite old so I don't know if it
> > has always been like that or we did change this behavior and forgot to
> > change the test, anyway this explains why we have done the clearing
> > with cmd_sync work since that is what this test expects.
> >
> > >
> > > ---
> > > Regards,
> > > Linux Bluetooth
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
Hi Luiz,
> What test are you talking about? The one in mgmt-tester which the CI
runs or your own tests?
Sorry for the confusion, I mean our own tests. Are the advertisement
instances fully cleared before the hci_sync rework? I looked at the
old hci_power_off before commit cf75ad8b41d2aa0 and did not find the
code to do that.
Thanks
Zhengping
On Wed, Jan 25, 2023 at 11:02 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Tedd,
>
> On Wed, Jan 25, 2023 at 8:34 PM Zhengping Jiang <[email protected]> wrote:
> >
> > Hi Luiz,
> >
> > Thanks for the comment. Is there a way to get more information about
> > the failed test? After we cherry picked the hci_sync rework to our
> > branch, some existing tests failed. The failed test will register a
> > few advertisements, then run a power cycle. After the power cycle, the
> > advertisements should be re-enabled automatically. I believe this used
> > to be the behavior before the hci_sync rework? Now calling
> > "hci_clear_adv_sync" during hci_power_off_sync will remove all
> > advertisement instances, so the client code needs to "manually"
> > re-register the advertisement after powering on the adapter. Please
> > let me know if I understood correctly.
>
> What test are you talking about? The one in mgmt-tester which the CI
> runs or your own tests?
>
> @Tedd Ho-Jeong An Perhaps we could add the github PR link to pw that
> way people can inspect the errors.
>
> > Thanks,
> > Zhengping
> >
> > On Wed, Jan 25, 2023 at 3:47 PM Luiz Augusto von Dentz
> > <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Jan 25, 2023 at 2:06 PM <[email protected]> wrote:
> > > >
> > > > This is automated email and please do not reply to this email!
> > > >
> > > > Dear submitter,
> > > >
> > > > Thank you for submitting the patches to the linux bluetooth mailing list.
> > > > This is a CI test results with your patch series:
> > > > PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=715641
> > > >
> > > > ---Test result---
> > > >
> > > > Test Summary:
> > > > CheckPatch PASS 0.65 seconds
> > > > GitLint FAIL 0.85 seconds
> > > > SubjectPrefix PASS 0.09 seconds
> > > > BuildKernel PASS 31.31 seconds
> > > > CheckAllWarning PASS 33.97 seconds
> > > > CheckSparse PASS 38.25 seconds
> > > > CheckSmatch PASS 107.12 seconds
> > > > BuildKernel32 PASS 29.96 seconds
> > > > TestRunnerSetup PASS 430.17 seconds
> > > > TestRunner_l2cap-tester PASS 16.16 seconds
> > > > TestRunner_iso-tester PASS 16.36 seconds
> > > > TestRunner_bnep-tester PASS 5.43 seconds
> > > > TestRunner_mgmt-tester FAIL 110.68 seconds
> > > > TestRunner_rfcomm-tester PASS 8.62 seconds
> > > > TestRunner_sco-tester PASS 7.99 seconds
> > > > TestRunner_ioctl-tester PASS 9.42 seconds
> > > > TestRunner_mesh-tester PASS 6.79 seconds
> > > > TestRunner_smp-tester PASS 7.81 seconds
> > > > TestRunner_userchan-tester PASS 5.73 seconds
> > > > IncrementalBuild PASS 27.75 seconds
> > > >
> > > > Details
> > > > ##############################
> > > > Test: GitLint - FAIL
> > > > Desc: Run gitlint
> > > > Output:
> > > > [kernel,v1,1/1] Bluetooth: Don't send HCI commands to remove adv if adapter is off
> > > >
> > > > WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
> > > > 1: T1 Title exceeds max length (82>80): "[kernel,v1,1/1] Bluetooth: Don't send HCI commands to remove adv if adapter is off"
> > > > ##############################
> > > > Test: TestRunner_mgmt-tester - FAIL
> > > > Desc: Run mgmt-tester with test-runner
> > > > Output:
> > > > Total: 494, Passed: 493 (99.8%), Failed: 1, Not Run: 0
> > > >
> > > > Failed Test Cases
> > > > Add Advertising - Success 18 (Power -> off, Remove) Timed out 2.314 seconds
> > >
> > > Looks like there is something not quite right wrt assumption that
> > > power off don't remove clear the adv, at least this test says
> > > otherwise and this test is actually quite old so I don't know if it
> > > has always been like that or we did change this behavior and forgot to
> > > change the test, anyway this explains why we have done the clearing
> > > with cmd_sync work since that is what this test expects.
> > >
> > > >
> > > > ---
> > > > Regards,
> > > > Linux Bluetooth
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz
Hi Zhengping,
On Wed, Jan 25, 2023 at 11:16 PM Zhengping Jiang <[email protected]> wrote:
>
> Hi Luiz,
>
> > What test are you talking about? The one in mgmt-tester which the CI
> runs or your own tests?
>
> Sorry for the confusion, I mean our own tests. Are the advertisement
> instances fully cleared before the hci_sync rework? I looked at the
> old hci_power_off before commit cf75ad8b41d2aa0 and did not find the
> code to do that.
The test expects Advertising Removed (0x0024) to pass:
https://gist.github.com/Vudentz/82938a4b7d501d5fea43a3b71e265e12
Like I said I suspect that test was broken, perhaps we change the
behavior but did not change the tests which then caused us to revert
to the old behavior in order to pass the test, anyway Add Advertising
does allow instances to be programmed while powered off but Add
Extended Advertising Data doesn't:
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/mgmt-api.txt#n3745
So I think it is better to maintain the current behavior and handle
this in the daemon, meaning we shall handle if the Advertising got
removed we just re-add it on power on, so at D-Bus level we allow
application to register advertising instance regardless of the power
state but while at the kernel level the instances are cleared on power
off. We can actually introduce a property to tell if the instance is
active or not at the kernel level so the daemon is able to notify the
application when they are currently programmed in the controller or
not.
>
> Thanks
> Zhengping
>
> On Wed, Jan 25, 2023 at 11:02 PM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Tedd,
> >
> > On Wed, Jan 25, 2023 at 8:34 PM Zhengping Jiang <[email protected]> wrote:
> > >
> > > Hi Luiz,
> > >
> > > Thanks for the comment. Is there a way to get more information about
> > > the failed test? After we cherry picked the hci_sync rework to our
> > > branch, some existing tests failed. The failed test will register a
> > > few advertisements, then run a power cycle. After the power cycle, the
> > > advertisements should be re-enabled automatically. I believe this used
> > > to be the behavior before the hci_sync rework? Now calling
> > > "hci_clear_adv_sync" during hci_power_off_sync will remove all
> > > advertisement instances, so the client code needs to "manually"
> > > re-register the advertisement after powering on the adapter. Please
> > > let me know if I understood correctly.
> >
> > What test are you talking about? The one in mgmt-tester which the CI
> > runs or your own tests?
> >
> > @Tedd Ho-Jeong An Perhaps we could add the github PR link to pw that
> > way people can inspect the errors.
> >
> > > Thanks,
> > > Zhengping
> > >
> > > On Wed, Jan 25, 2023 at 3:47 PM Luiz Augusto von Dentz
> > > <[email protected]> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Wed, Jan 25, 2023 at 2:06 PM <[email protected]> wrote:
> > > > >
> > > > > This is automated email and please do not reply to this email!
> > > > >
> > > > > Dear submitter,
> > > > >
> > > > > Thank you for submitting the patches to the linux bluetooth mailing list.
> > > > > This is a CI test results with your patch series:
> > > > > PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=715641
> > > > >
> > > > > ---Test result---
> > > > >
> > > > > Test Summary:
> > > > > CheckPatch PASS 0.65 seconds
> > > > > GitLint FAIL 0.85 seconds
> > > > > SubjectPrefix PASS 0.09 seconds
> > > > > BuildKernel PASS 31.31 seconds
> > > > > CheckAllWarning PASS 33.97 seconds
> > > > > CheckSparse PASS 38.25 seconds
> > > > > CheckSmatch PASS 107.12 seconds
> > > > > BuildKernel32 PASS 29.96 seconds
> > > > > TestRunnerSetup PASS 430.17 seconds
> > > > > TestRunner_l2cap-tester PASS 16.16 seconds
> > > > > TestRunner_iso-tester PASS 16.36 seconds
> > > > > TestRunner_bnep-tester PASS 5.43 seconds
> > > > > TestRunner_mgmt-tester FAIL 110.68 seconds
> > > > > TestRunner_rfcomm-tester PASS 8.62 seconds
> > > > > TestRunner_sco-tester PASS 7.99 seconds
> > > > > TestRunner_ioctl-tester PASS 9.42 seconds
> > > > > TestRunner_mesh-tester PASS 6.79 seconds
> > > > > TestRunner_smp-tester PASS 7.81 seconds
> > > > > TestRunner_userchan-tester PASS 5.73 seconds
> > > > > IncrementalBuild PASS 27.75 seconds
> > > > >
> > > > > Details
> > > > > ##############################
> > > > > Test: GitLint - FAIL
> > > > > Desc: Run gitlint
> > > > > Output:
> > > > > [kernel,v1,1/1] Bluetooth: Don't send HCI commands to remove adv if adapter is off
> > > > >
> > > > > WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search
> > > > > 1: T1 Title exceeds max length (82>80): "[kernel,v1,1/1] Bluetooth: Don't send HCI commands to remove adv if adapter is off"
> > > > > ##############################
> > > > > Test: TestRunner_mgmt-tester - FAIL
> > > > > Desc: Run mgmt-tester with test-runner
> > > > > Output:
> > > > > Total: 494, Passed: 493 (99.8%), Failed: 1, Not Run: 0
> > > > >
> > > > > Failed Test Cases
> > > > > Add Advertising - Success 18 (Power -> off, Remove) Timed out 2.314 seconds
> > > >
> > > > Looks like there is something not quite right wrt assumption that
> > > > power off don't remove clear the adv, at least this test says
> > > > otherwise and this test is actually quite old so I don't know if it
> > > > has always been like that or we did change this behavior and forgot to
> > > > change the test, anyway this explains why we have done the clearing
> > > > with cmd_sync work since that is what this test expects.
> > > >
> > > > >
> > > > > ---
> > > > > Regards,
> > > > > Linux Bluetooth
> > > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
--
Luiz Augusto von Dentz