diff mbox series

[RFC] Bluetooth: core: Allow bind HCI socket user channel when HCI is UP.

Message ID 20220412120945.28862-1-vasyl.vavrychuk@opensynergy.com (mailing list archive)
State New, archived
Headers show
Series [RFC] Bluetooth: core: Allow bind HCI socket user channel when HCI is UP. | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/checkpatch fail [RFC] Bluetooth: core: Allow bind HCI socket user channel when HCI is UP.\WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Vasyl Vavrychuk <vvavrychuk@gmail.com>' != 'Signed-off-by: Vasyl Vavrychuk <vasyl.vavrychuk@opensynergy.com>' total: 0 errors, 1 warnings, 0 checks, 10 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/12810739.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.
tedd_an/gitlint fail [RFC] Bluetooth: core: Allow bind HCI socket user channel when HCI is UP. 1: T3 Title has trailing punctuation (.): "[RFC] Bluetooth: core: Allow bind HCI socket user channel when HCI is UP."
tedd_an/subjectprefix success PASS
tedd_an/buildkernel success Build Kernel PASS
tedd_an/buildkernel32 success Build Kernel32 PASS
tedd_an/incremental_build success Pass
tedd_an/testrunnersetup success Test Runner Setup PASS
tedd_an/testrunnerl2cap-tester success Total: 40, Passed: 40 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnerbnep-tester success Total: 1, Passed: 1 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnermgmt-tester success Total: 493, Passed: 493 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnerrfcomm-tester success Total: 10, Passed: 10 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersco-tester success Total: 12, Passed: 12 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunnersmp-tester success Total: 8, Passed: 8 (100.0%), Failed: 0, Not Run: 0
tedd_an/testrunneruserchan-tester fail Total: 4, Passed: 3 (75.0%), Failed: 1, Not Run: 0

Commit Message

Vasyl Vavrychuk April 12, 2022, 12:09 p.m. UTC 
This is needed for user-space to ensure that HCI init scheduled from
hci_register_dev is completed.

Function hci_register_dev queues power_on workqueue which will run
hci_power_on > hci_dev_do_open. Function hci_dev_do_open sets HCI_INIT
for some time.

It is not allowed to bind to HCI socket user channel when HCI_INIT is
set. As result, bind might fail when user-space program is run early
enough during boot.

Now, user-space program can first issue HCIDEVUP ioctl to ensure HCI
init scheduled at hci_register_dev was completed.

Signed-off-by: Vasyl Vavrychuk <vasyl.vavrychuk@opensynergy.com>
---
 net/bluetooth/hci_sock.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Comments

bluez.test.bot@gmail.com April 12, 2022, 1:36 p.m. UTC | #1 
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=631489

---Test result---

Test Summary:
CheckPatch                    FAIL      1.67 seconds
GitLint                       FAIL      1.02 seconds
SubjectPrefix                 PASS      0.82 seconds
BuildKernel                   PASS      30.11 seconds
BuildKernel32                 PASS      27.80 seconds
Incremental Build with patchesPASS      39.08 seconds
TestRunner: Setup             PASS      471.62 seconds
TestRunner: l2cap-tester      PASS      16.22 seconds
TestRunner: bnep-tester       PASS      6.26 seconds
TestRunner: mgmt-tester       PASS      106.06 seconds
TestRunner: rfcomm-tester     PASS      8.22 seconds
TestRunner: sco-tester        PASS      7.99 seconds
TestRunner: smp-tester        PASS      7.98 seconds
TestRunner: userchan-tester   FAIL      6.66 seconds

Details
##############################
Test: CheckPatch - FAIL - 1.67 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
[RFC] Bluetooth: core: Allow bind HCI socket user channel when HCI is UP.\WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Vasyl Vavrychuk <vvavrychuk@gmail.com>' != 'Signed-off-by: Vasyl Vavrychuk <vasyl.vavrychuk@opensynergy.com>'

total: 0 errors, 1 warnings, 0 checks, 10 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/12810739.patch has style problems, please review.

NOTE: Ignored message types: UNKNOWN_COMMIT_ID

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


##############################
Test: GitLint - FAIL - 1.02 seconds
Run gitlint with rule in .gitlint
[RFC] Bluetooth: core: Allow bind HCI socket user channel when HCI is UP.
1: T3 Title has trailing punctuation (.): "[RFC] Bluetooth: core: Allow bind HCI socket user channel when HCI is UP."


##############################
Test: TestRunner: userchan-tester - FAIL - 6.66 seconds
Run test-runner with userchan-tester
Total: 4, Passed: 3 (75.0%), Failed: 1, Not Run: 0

Failed Test Cases
User channel open - Failed                           Failed       0.082 seconds



---
Regards,
Linux Bluetooth
Vasyl Vavrychuk April 12, 2022, 6:26 p.m. UTC | #2 
CI reported userchan-tester failure since it is expected old
behaviour: that bind to user channel should fail when device is
powered up.

I can update https://github.com/bluez/bluez/blob/master/tools/userchan-tester.c
by removing test cases:
* User channel open - Failed
* User channel open - Power Toggle Success

On Tue, Apr 12, 2022 at 4:36 PM <bluez.test.bot@gmail.com> 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=631489
>
> ---Test result---
>
> Test Summary:
> CheckPatch                    FAIL      1.67 seconds
> GitLint                       FAIL      1.02 seconds
> SubjectPrefix                 PASS      0.82 seconds
> BuildKernel                   PASS      30.11 seconds
> BuildKernel32                 PASS      27.80 seconds
> Incremental Build with patchesPASS      39.08 seconds
> TestRunner: Setup             PASS      471.62 seconds
> TestRunner: l2cap-tester      PASS      16.22 seconds
> TestRunner: bnep-tester       PASS      6.26 seconds
> TestRunner: mgmt-tester       PASS      106.06 seconds
> TestRunner: rfcomm-tester     PASS      8.22 seconds
> TestRunner: sco-tester        PASS      7.99 seconds
> TestRunner: smp-tester        PASS      7.98 seconds
> TestRunner: userchan-tester   FAIL      6.66 seconds
>
> Details
> ##############################
> Test: CheckPatch - FAIL - 1.67 seconds
> Run checkpatch.pl script with rule in .checkpatch.conf
> [RFC] Bluetooth: core: Allow bind HCI socket user channel when HCI is UP.\WARNING:FROM_SIGN_OFF_MISMATCH: From:/Signed-off-by: email address mismatch: 'From: Vasyl Vavrychuk <vvavrychuk@gmail.com>' != 'Signed-off-by: Vasyl Vavrychuk <vasyl.vavrychuk@opensynergy.com>'
>
> total: 0 errors, 1 warnings, 0 checks, 10 lines checked
>
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.
>
> /github/workspace/src/12810739.patch has style problems, please review.
>
> NOTE: Ignored message types: UNKNOWN_COMMIT_ID
>
> NOTE: If any of the errors are false positives, please report
>       them to the maintainer, see CHECKPATCH in MAINTAINERS.
>
>
> ##############################
> Test: GitLint - FAIL - 1.02 seconds
> Run gitlint with rule in .gitlint
> [RFC] Bluetooth: core: Allow bind HCI socket user channel when HCI is UP.
> 1: T3 Title has trailing punctuation (.): "[RFC] Bluetooth: core: Allow bind HCI socket user channel when HCI is UP."
>
>
> ##############################
> Test: TestRunner: userchan-tester - FAIL - 6.66 seconds
> Run test-runner with userchan-tester
> Total: 4, Passed: 3 (75.0%), Failed: 1, Not Run: 0
>
> Failed Test Cases
> User channel open - Failed                           Failed       0.082 seconds
>
>
>
> ---
> Regards,
> Linux Bluetooth
>
Marcel Holtmann April 22, 2022, 9:20 a.m. UTC | #3 
Hi Vasyl,

> This is needed for user-space to ensure that HCI init scheduled from
> hci_register_dev is completed.
> 
> Function hci_register_dev queues power_on workqueue which will run
> hci_power_on > hci_dev_do_open. Function hci_dev_do_open sets HCI_INIT
> for some time.
> 
> It is not allowed to bind to HCI socket user channel when HCI_INIT is
> set. As result, bind might fail when user-space program is run early
> enough during boot.
> 
> Now, user-space program can first issue HCIDEVUP ioctl to ensure HCI
> init scheduled at hci_register_dev was completed.
> 
> Signed-off-by: Vasyl Vavrychuk <vasyl.vavrychuk@opensynergy.com>
> ---
> net/bluetooth/hci_sock.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index 33b3c0ffc339..c98de809f856 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -1194,9 +1194,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
> 
> 		if (test_bit(HCI_INIT, &hdev->flags) ||
> 		    hci_dev_test_flag(hdev, HCI_SETUP) ||
> -		    hci_dev_test_flag(hdev, HCI_CONFIG) ||
> -		    (!hci_dev_test_flag(hdev, HCI_AUTO_OFF) &&
> -		     test_bit(HCI_UP, &hdev->flags))) {
> +		    hci_dev_test_flag(hdev, HCI_CONFIG)) {
> 			err = -EBUSY;
> 			hci_dev_put(hdev);
> 			goto done;

I am not following the reasoning here. It is true that the device has to run init before you can do something with it. From mgmt interface your device will only be announced when it is really ready.

Regards

Marcel
Vasyl Vavrychuk April 26, 2022, 9:24 a.m. UTC | #4 
Hi, Marcel,

On 4/22/2022 12:20 PM, Marcel Holtmann wrote:
> Hi Vasyl,
> 
>> This is needed for user-space to ensure that HCI init scheduled from
>> hci_register_dev is completed.
>>
>> Function hci_register_dev queues power_on workqueue which will run
>> hci_power_on > hci_dev_do_open. Function hci_dev_do_open sets HCI_INIT
>> for some time.
>>
>> It is not allowed to bind to HCI socket user channel when HCI_INIT is
>> set. As result, bind might fail when user-space program is run early
>> enough during boot.
>>
>> Now, user-space program can first issue HCIDEVUP ioctl to ensure HCI
>> init scheduled at hci_register_dev was completed.
>>
>> Signed-off-by: Vasyl Vavrychuk <vasyl.vavrychuk@opensynergy.com>
>> ---
>> net/bluetooth/hci_sock.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
>> index 33b3c0ffc339..c98de809f856 100644
>> --- a/net/bluetooth/hci_sock.c
>> +++ b/net/bluetooth/hci_sock.c
>> @@ -1194,9 +1194,7 @@ static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
>>
>> 		if (test_bit(HCI_INIT, &hdev->flags) ||
>> 		    hci_dev_test_flag(hdev, HCI_SETUP) ||
>> -		    hci_dev_test_flag(hdev, HCI_CONFIG) ||
>> -		    (!hci_dev_test_flag(hdev, HCI_AUTO_OFF) &&
>> -		     test_bit(HCI_UP, &hdev->flags))) {
>> +		    hci_dev_test_flag(hdev, HCI_CONFIG)) {
>> 			err = -EBUSY;
>> 			hci_dev_put(hdev);
>> 			goto done;
> 
> I am not following the reasoning here. It is true that the device has to run init before you can do something with it. From mgmt interface your device will only be announced when it is really ready.

Sorry, I am not familiar with mgmt interface. I obtain device using 
HCIGETDEVLIST.

BTW. I have pushed related patch [1]. Comparing to this patch, [1] is 
less intrusive since it does not effect user-space semantics.

Patch [1] allows to ensure that device is not in HCI_INIT state by running

     hciconfig hci0 down

This will either wait for HCI_INIT complete and then powers HCI down, or 
cancels pending power_on.

If we apply [1], we can still consider an optimization to allow binding 
during HCI_INIT since this optimization will allow me to ommit extra

     hciconfig hci0 down

[1]: 
https://lore.kernel.org/linux-bluetooth/20220426081823.21557-1-vasyl.vavrychuk@opensynergy.com/T/#u

Kind regards,
Vasyl
diff mbox series

Patch

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 33b3c0ffc339..c98de809f856 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -1194,9 +1194,7 @@  static int hci_sock_bind(struct socket *sock, struct sockaddr *addr,
 
 		if (test_bit(HCI_INIT, &hdev->flags) ||
 		    hci_dev_test_flag(hdev, HCI_SETUP) ||
-		    hci_dev_test_flag(hdev, HCI_CONFIG) ||
-		    (!hci_dev_test_flag(hdev, HCI_AUTO_OFF) &&
-		     test_bit(HCI_UP, &hdev->flags))) {
+		    hci_dev_test_flag(hdev, HCI_CONFIG)) {
 			err = -EBUSY;
 			hci_dev_put(hdev);
 			goto done;