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

Commit Message

Zhengping Jiang Jan. 25, 2023, 9:12 p.m. UTC

From: Archie Pusaka <[e-mail]>

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 <[e-mail]>
Signed-off-by: Zhengping Jiang <[e-mail]>

---

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(-)

Comments

Luiz Augusto von Dentz Jan. 25, 2023, 9:54 p.m. UTC | #1

Hi Zhengping,

On Wed, Jan 25, 2023 at 1:12 PM Zhengping Jiang <[e-mail]> wrote:
>
> From: Archie Pusaka <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Zhengping Jiang <[e-mail]>
>
> ---
>
> 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
>

[e-mail] Jan. 25, 2023, 9:57 p.m. UTC | #2

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

Luiz Augusto von Dentz Jan. 25, 2023, 11:46 p.m. UTC | #3

Hi,

On Wed, Jan 25, 2023 at 2:06 PM <[e-mail]> 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
>

Zhengping Jiang Jan. 26, 2023, 4:33 a.m. UTC | #4

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
<[e-mail]> wrote:
>
> Hi,
>
> On Wed, Jan 25, 2023 at 2:06 PM <[e-mail]> 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 Jan. 26, 2023, 7:02 a.m. UTC | #5

Hi Tedd,

On Wed, Jan 25, 2023 at 8:34 PM Zhengping Jiang <[e-mail]> 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
> <[e-mail]> wrote:
> >
> > Hi,
> >
> > On Wed, Jan 25, 2023 at 2:06 PM <[e-mail]> 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

Zhengping Jiang Jan. 26, 2023, 7:16 a.m. UTC | #6

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
<[e-mail]> wrote:
>
> Hi Tedd,
>
> On Wed, Jan 25, 2023 at 8:34 PM Zhengping Jiang <[e-mail]> 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
> > <[e-mail]> wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Jan 25, 2023 at 2:06 PM <[e-mail]> 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 Jan. 26, 2023, 10:20 p.m. UTC | #7

Hi Zhengping,

On Wed, Jan 25, 2023 at 11:16 PM Zhengping Jiang <[e-mail]> 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
> <[e-mail]> wrote:
> >
> > Hi Tedd,
> >
> > On Wed, Jan 25, 2023 at 8:34 PM Zhengping Jiang <[e-mail]> 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
> > > <[e-mail]> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Wed, Jan 25, 2023 at 2:06 PM <[e-mail]> 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

Patch

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)