| Message ID | 20201001230403.2445035-1-danielwinkler@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Bluetooth: Add new MGMT interface for advertising add | expand |
Hello Maintainers, Just a friendly reminder to review this kernel patch series. I may have accidentally named this series the same as the userspace series, so I apologize if it has caused the set to be hidden in anybody's inbox. I'll be sure not to do this in the future. Thanks in advance for your time! Best regards, Daniel Winkler On Thu, Oct 1, 2020 at 4:04 PM Daniel Winkler <danielwinkler@google.com> wrote: > > Hi Maintainers, > > This patch series defines the new two-call MGMT interface for adding > new advertising instances. Similarly to the hci advertising commands, a > mgmt call to set parameters is expected to be first, followed by a mgmt > call to set advertising data/scan response. The members of the > parameters request are optional; the caller defines a "params" bitfield > in the structure that indicates which parameters were intentionally set, > and others are set to defaults. > > The main feature here is the introduction of min/max parameters and tx > power that can be requested by the client. Min/max parameters will be > used both with and without extended advertising support, and tx power > will be used with extended advertising support. After a call for hci > advertising parameters, a new TX_POWER_SELECTED event will be emitted to > alert userspace to the actual chosen tx power. > > Additionally, to inform userspace of the controller LE Tx power > capabilities for the client's benefit, this series also changes the > security info MGMT command to more flexibly contain other capabilities, > such as LE min and max tx power. > > All changes have been tested on hatch (extended advertising) and kukui > (no extended advertising) chromebooks with manual testing verifying > correctness of parameters/data in btmon traces, and our automated test > suite of 25 single- and multi-advertising usage scenarios. > > A separate patch series will add support in bluetoothd. Thanks in > advance for your feedback! > > Daniel Winkler > > > Changes in v4: > - Add remaining data and scan response length to MGMT params response > - Moving optional params into 'flags' field of MGMT command > - Combine LE tx range into a single EIR field for MGMT capabilities cmd > > Changes in v3: > - Adding selected tx power to adv params mgmt response, removing event > - Re-using security info MGMT command to carry controller capabilities > > Changes in v2: > - Fixed sparse error in Capabilities MGMT command > > Daniel Winkler (5): > Bluetooth: Add helper to set adv data > Bluetooth: Break add adv into two mgmt commands > Bluetooth: Use intervals and tx power from mgmt cmds > Bluetooth: Query LE tx power on startup > Bluetooth: Change MGMT security info CMD to be more generic > > include/net/bluetooth/hci.h | 7 + > include/net/bluetooth/hci_core.h | 12 +- > include/net/bluetooth/mgmt.h | 49 +++- > net/bluetooth/hci_core.c | 47 +++- > net/bluetooth/hci_event.c | 19 ++ > net/bluetooth/hci_request.c | 29 ++- > net/bluetooth/mgmt.c | 424 +++++++++++++++++++++++++++++-- > 7 files changed, 542 insertions(+), 45 deletions(-) > > -- > 2.28.0.709.gb0816b6eb0-goog >
Hi Daniel, On Thu, Oct 29, 2020 at 2:35 PM Daniel Winkler <danielwinkler@google.com> wrote: > > Hello Maintainers, > > Just a friendly reminder to review this kernel patch series. I may > have accidentally named this series the same as the userspace series, > so I apologize if it has caused the set to be hidden in anybody's > inbox. I'll be sure not to do this in the future. I will review them coming next, one of the things that seems to be missing these days is to update mgmt-tester when a new command is introduced, this should actually be added along side the kernel changes since we do plan to have the CI verify the kernel patches as well, also there is a way to test the kernel changes directly in the host with use of tools/test-runner you just need insure the options mentioned in doc/test-runner are set so you can run the kernel with the changes directly. > Thanks in advance for your time! > > Best regards, > Daniel Winkler > > On Thu, Oct 1, 2020 at 4:04 PM Daniel Winkler <danielwinkler@google.com> wrote: > > > > Hi Maintainers, > > > > This patch series defines the new two-call MGMT interface for adding > > new advertising instances. Similarly to the hci advertising commands, a > > mgmt call to set parameters is expected to be first, followed by a mgmt > > call to set advertising data/scan response. The members of the > > parameters request are optional; the caller defines a "params" bitfield > > in the structure that indicates which parameters were intentionally set, > > and others are set to defaults. > > > > The main feature here is the introduction of min/max parameters and tx > > power that can be requested by the client. Min/max parameters will be > > used both with and without extended advertising support, and tx power > > will be used with extended advertising support. After a call for hci > > advertising parameters, a new TX_POWER_SELECTED event will be emitted to > > alert userspace to the actual chosen tx power. > > > > Additionally, to inform userspace of the controller LE Tx power > > capabilities for the client's benefit, this series also changes the > > security info MGMT command to more flexibly contain other capabilities, > > such as LE min and max tx power. > > > > All changes have been tested on hatch (extended advertising) and kukui > > (no extended advertising) chromebooks with manual testing verifying > > correctness of parameters/data in btmon traces, and our automated test > > suite of 25 single- and multi-advertising usage scenarios. > > > > A separate patch series will add support in bluetoothd. Thanks in > > advance for your feedback! > > > > Daniel Winkler > > > > > > Changes in v4: > > - Add remaining data and scan response length to MGMT params response > > - Moving optional params into 'flags' field of MGMT command > > - Combine LE tx range into a single EIR field for MGMT capabilities cmd > > > > Changes in v3: > > - Adding selected tx power to adv params mgmt response, removing event > > - Re-using security info MGMT command to carry controller capabilities > > > > Changes in v2: > > - Fixed sparse error in Capabilities MGMT command > > > > Daniel Winkler (5): > > Bluetooth: Add helper to set adv data > > Bluetooth: Break add adv into two mgmt commands > > Bluetooth: Use intervals and tx power from mgmt cmds > > Bluetooth: Query LE tx power on startup > > Bluetooth: Change MGMT security info CMD to be more generic > > > > include/net/bluetooth/hci.h | 7 + > > include/net/bluetooth/hci_core.h | 12 +- > > include/net/bluetooth/mgmt.h | 49 +++- > > net/bluetooth/hci_core.c | 47 +++- > > net/bluetooth/hci_event.c | 19 ++ > > net/bluetooth/hci_request.c | 29 ++- > > net/bluetooth/mgmt.c | 424 +++++++++++++++++++++++++++++-- > > 7 files changed, 542 insertions(+), 45 deletions(-) > > > > -- > > 2.28.0.709.gb0816b6eb0-goog > >
Hi Luiz, Thank you for the feedback regarding mgmt-tester. I intended to use the tool, but found that it had a very high rate of test failure even before I started adding new tests. If you have a strong preference for its use, I can look into it again but it may take some time. These changes were tested with manual and automated functional testing on our end. Please let me know your thoughts. Thanks, Daniel On Thu, Oct 29, 2020 at 2:45 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Daniel, > > On Thu, Oct 29, 2020 at 2:35 PM Daniel Winkler <danielwinkler@google.com> wrote: > > > > Hello Maintainers, > > > > Just a friendly reminder to review this kernel patch series. I may > > have accidentally named this series the same as the userspace series, > > so I apologize if it has caused the set to be hidden in anybody's > > inbox. I'll be sure not to do this in the future. > > I will review them coming next, one of the things that seems to be > missing these days is to update mgmt-tester when a new command is > introduced, this should actually be added along side the kernel > changes since we do plan to have the CI verify the kernel patches as > well, also there is a way to test the kernel changes directly in the > host with use of tools/test-runner you just need insure the options > mentioned in doc/test-runner are set so you can run the kernel with > the changes directly. > > > Thanks in advance for your time! > > > > Best regards, > > Daniel Winkler > > > > On Thu, Oct 1, 2020 at 4:04 PM Daniel Winkler <danielwinkler@google.com> wrote: > > > > > > Hi Maintainers, > > > > > > This patch series defines the new two-call MGMT interface for adding > > > new advertising instances. Similarly to the hci advertising commands, a > > > mgmt call to set parameters is expected to be first, followed by a mgmt > > > call to set advertising data/scan response. The members of the > > > parameters request are optional; the caller defines a "params" bitfield > > > in the structure that indicates which parameters were intentionally set, > > > and others are set to defaults. > > > > > > The main feature here is the introduction of min/max parameters and tx > > > power that can be requested by the client. Min/max parameters will be > > > used both with and without extended advertising support, and tx power > > > will be used with extended advertising support. After a call for hci > > > advertising parameters, a new TX_POWER_SELECTED event will be emitted to > > > alert userspace to the actual chosen tx power. > > > > > > Additionally, to inform userspace of the controller LE Tx power > > > capabilities for the client's benefit, this series also changes the > > > security info MGMT command to more flexibly contain other capabilities, > > > such as LE min and max tx power. > > > > > > All changes have been tested on hatch (extended advertising) and kukui > > > (no extended advertising) chromebooks with manual testing verifying > > > correctness of parameters/data in btmon traces, and our automated test > > > suite of 25 single- and multi-advertising usage scenarios. > > > > > > A separate patch series will add support in bluetoothd. Thanks in > > > advance for your feedback! > > > > > > Daniel Winkler > > > > > > > > > Changes in v4: > > > - Add remaining data and scan response length to MGMT params response > > > - Moving optional params into 'flags' field of MGMT command > > > - Combine LE tx range into a single EIR field for MGMT capabilities cmd > > > > > > Changes in v3: > > > - Adding selected tx power to adv params mgmt response, removing event > > > - Re-using security info MGMT command to carry controller capabilities > > > > > > Changes in v2: > > > - Fixed sparse error in Capabilities MGMT command > > > > > > Daniel Winkler (5): > > > Bluetooth: Add helper to set adv data > > > Bluetooth: Break add adv into two mgmt commands > > > Bluetooth: Use intervals and tx power from mgmt cmds > > > Bluetooth: Query LE tx power on startup > > > Bluetooth: Change MGMT security info CMD to be more generic > > > > > > include/net/bluetooth/hci.h | 7 + > > > include/net/bluetooth/hci_core.h | 12 +- > > > include/net/bluetooth/mgmt.h | 49 +++- > > > net/bluetooth/hci_core.c | 47 +++- > > > net/bluetooth/hci_event.c | 19 ++ > > > net/bluetooth/hci_request.c | 29 ++- > > > net/bluetooth/mgmt.c | 424 +++++++++++++++++++++++++++++-- > > > 7 files changed, 542 insertions(+), 45 deletions(-) > > > > > > -- > > > 2.28.0.709.gb0816b6eb0-goog > > > > > > > -- > Luiz Augusto von Dentz
Hi Daniel, On Thu, Oct 29, 2020 at 3:25 PM Daniel Winkler <danielwinkler@google.com> wrote: > > Hi Luiz, > > Thank you for the feedback regarding mgmt-tester. I intended to use > the tool, but found that it had a very high rate of test failure even > before I started adding new tests. If you have a strong preference for > its use, I can look into it again but it may take some time. These > changes were tested with manual and automated functional testing on > our end. > > Please let me know your thoughts. Total: 406, Passed: 358 (88.2%), Failed: 43, Not Run: 5 Looks like there are some 43 tests failing, we will need to fix these but it should prevent us to add new ones as well, you can use -p to filter what tests to run if you want to avoid these for now.
Hello Luiz, Thank you for the information. It is good to know that this tool is actively used and that there is a way to skip existing flaky tests. Just for clarification, is this a requirement to land the kernel changes, i.e. should I prioritize adding these tests immediately to move the process forward? Or can we land the changes based on the testing I have already done and I'll work on these tests in parallel? Thanks, Daniel On Thu, Oct 29, 2020 at 5:04 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Daniel, > > On Thu, Oct 29, 2020 at 3:25 PM Daniel Winkler <danielwinkler@google.com> wrote: > > > > Hi Luiz, > > > > Thank you for the feedback regarding mgmt-tester. I intended to use > > the tool, but found that it had a very high rate of test failure even > > before I started adding new tests. If you have a strong preference for > > its use, I can look into it again but it may take some time. These > > changes were tested with manual and automated functional testing on > > our end. > > > > Please let me know your thoughts. > > Total: 406, Passed: 358 (88.2%), Failed: 43, Not Run: 5 > > Looks like there are some 43 tests failing, we will need to fix these > but it should prevent us to add new ones as well, you can use -p to > filter what tests to run if you want to avoid these for now.
Hi Daniel, On Tue, Nov 3, 2020 at 9:42 AM Daniel Winkler <danielwinkler@google.com> wrote: > > Hello Luiz, > > Thank you for the information. It is good to know that this tool is > actively used and that there is a way to skip existing flaky tests. > Just for clarification, is this a requirement to land the kernel > changes, i.e. should I prioritize adding these tests immediately to > move the process forward? Or can we land the changes based on the > testing I have already done and I'll work on these tests in parallel? We used to require updates to mgmt-tester but it seems some of recent command did not have a test yet, but if we intend to have the CI to tests the kernel changes properly I think we should start to requiring it some basic testing, obviously it will be hard to cover everything that is affected by a new command but the basic formatting, etc, we should be able to test, also tester supports the concept of 'not run' which we can probably use for experimental commands. > Thanks, > Daniel > > On Thu, Oct 29, 2020 at 5:04 PM Luiz Augusto von Dentz > <luiz.dentz@gmail.com> wrote: > > > > Hi Daniel, > > > > On Thu, Oct 29, 2020 at 3:25 PM Daniel Winkler <danielwinkler@google.com> wrote: > > > > > > Hi Luiz, > > > > > > Thank you for the feedback regarding mgmt-tester. I intended to use > > > the tool, but found that it had a very high rate of test failure even > > > before I started adding new tests. If you have a strong preference for > > > its use, I can look into it again but it may take some time. These > > > changes were tested with manual and automated functional testing on > > > our end. > > > > > > Please let me know your thoughts. > > > > Total: 406, Passed: 358 (88.2%), Failed: 43, Not Run: 5 > > > > Looks like there are some 43 tests failing, we will need to fix these > > but it should prevent us to add new ones as well, you can use -p to > > filter what tests to run if you want to avoid these for now.
Hi Luiz, Thank you again for the support on this issue. I have just provided a patch series here: https://patchwork.kernel.org/project/bluetooth/list/?series=390411 to include test coverage for the new APIs via mgmt-tester. In addition, as this coverage helped me find a minor bug in returning remaining adv data size in the MGMT response, I've submitted a fix in the kernel patch series. Please let me know if there is anything further I can provide. Thanks! Daniel On Tue, Nov 3, 2020 at 1:25 PM Luiz Augusto von Dentz <luiz.dentz@gmail.com> wrote: > > Hi Daniel, > > On Tue, Nov 3, 2020 at 9:42 AM Daniel Winkler <danielwinkler@google.com> wrote: > > > > Hello Luiz, > > > > Thank you for the information. It is good to know that this tool is > > actively used and that there is a way to skip existing flaky tests. > > Just for clarification, is this a requirement to land the kernel > > changes, i.e. should I prioritize adding these tests immediately to > > move the process forward? Or can we land the changes based on the > > testing I have already done and I'll work on these tests in parallel? > > We used to require updates to mgmt-tester but it seems some of recent > command did not have a test yet, but if we intend to have the CI to > tests the kernel changes properly I think we should start to requiring > it some basic testing, obviously it will be hard to cover everything > that is affected by a new command but the basic formatting, etc, we > should be able to test, also tester supports the concept of 'not run' > which we can probably use for experimental commands. > > > Thanks, > > Daniel > > > > On Thu, Oct 29, 2020 at 5:04 PM Luiz Augusto von Dentz > > <luiz.dentz@gmail.com> wrote: > > > > > > Hi Daniel, > > > > > > On Thu, Oct 29, 2020 at 3:25 PM Daniel Winkler <danielwinkler@google.com> wrote: > > > > > > > > Hi Luiz, > > > > > > > > Thank you for the feedback regarding mgmt-tester. I intended to use > > > > the tool, but found that it had a very high rate of test failure even > > > > before I started adding new tests. If you have a strong preference for > > > > its use, I can look into it again but it may take some time. These > > > > changes were tested with manual and automated functional testing on > > > > our end. > > > > > > > > Please let me know your thoughts. > > > > > > Total: 406, Passed: 358 (88.2%), Failed: 43, Not Run: 5 > > > > > > Looks like there are some 43 tests failing, we will need to fix these > > > but it should prevent us to add new ones as well, you can use -p to > > > filter what tests to run if you want to avoid these for now. > > > > -- > Luiz Augusto von Dentz