| Message ID | d159c57f-8490-4c26-79da-6ad3612c4a14@salutedevices.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Bluetooth: hci_uart: fix race during initialization | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/BuildKernel | success | BuildKernel PASS |
| tedd_an/CheckAllWarning | success | CheckAllWarning PASS |
| tedd_an/CheckSparse | success | CheckSparse PASS |
| tedd_an/BuildKernel32 | success | BuildKernel32 PASS |
| tedd_an/TestRunnerSetup | success | TestRunnerSetup PASS |
| tedd_an/TestRunner_l2cap-tester | success | TestRunner PASS |
| tedd_an/TestRunner_iso-tester | success | TestRunner PASS |
| tedd_an/TestRunner_bnep-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mgmt-tester | fail | TestRunner_mgmt-tester: Total: 490, Passed: 483 (98.6%), Failed: 3, Not Run: 4 |
| tedd_an/TestRunner_rfcomm-tester | success | TestRunner PASS |
| tedd_an/TestRunner_sco-tester | success | TestRunner PASS |
| tedd_an/TestRunner_ioctl-tester | success | TestRunner PASS |
| tedd_an/TestRunner_mesh-tester | success | TestRunner PASS |
| tedd_an/TestRunner_smp-tester | success | TestRunner PASS |
| tedd_an/TestRunner_userchan-tester | success | TestRunner PASS |
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=918554 ---Test result--- Test Summary: CheckPatch PENDING 0.30 seconds GitLint PENDING 0.21 seconds SubjectPrefix PASS 0.12 seconds BuildKernel PASS 25.26 seconds CheckAllWarning PASS 27.58 seconds CheckSparse PASS 30.88 seconds BuildKernel32 PASS 24.67 seconds TestRunnerSetup PASS 440.93 seconds TestRunner_l2cap-tester PASS 20.61 seconds TestRunner_iso-tester PASS 32.16 seconds TestRunner_bnep-tester PASS 4.91 seconds TestRunner_mgmt-tester FAIL 120.79 seconds TestRunner_rfcomm-tester PASS 7.57 seconds TestRunner_sco-tester PASS 10.46 seconds TestRunner_ioctl-tester PASS 8.09 seconds TestRunner_mesh-tester PASS 6.04 seconds TestRunner_smp-tester PASS 7.10 seconds TestRunner_userchan-tester PASS 5.04 seconds IncrementalBuild PENDING 0.41 seconds Details ############################## Test: CheckPatch - PENDING Desc: Run checkpatch.pl script Output: ############################## Test: GitLint - PENDING Desc: Run gitlint Output: ############################## Test: TestRunner_mgmt-tester - FAIL Desc: Run mgmt-tester with test-runner Output: Total: 490, Passed: 483 (98.6%), Failed: 3, Not Run: 4 Failed Test Cases LL Privacy - Add Device 3 (AL is full) Failed 0.206 seconds LL Privacy - Set Flags 3 (2 Devices to RL) Failed 0.206 seconds LL Privacy - Set Device Flag 1 (Device Privacy) Failed 0.162 seconds ############################## Test: IncrementalBuild - PENDING Desc: Incremental build with the patches in the series Output: --- Regards, Linux Bluetooth
Hi, sorry i'm new in bluetooth subsystem. I get the following message from CI: https://patchwork.kernel.org/project/bluetooth/list/?series=918554 Where one of tests was failed. Where I can get more information about failure: tedd_an/TestRunner_mgmt-tester fail TestRunner_mgmt-tester: Total: 490, Passed: 483 (98.6%), Failed: 3, Not Run: 4 ? Thanks On 17.12.2024 11:12, Arseniy Krasnov wrote: > 'hci_register_dev()' calls power up function, which is executed by > kworker - 'hci_power_on()'. This function does access to bluetooth chip > using callbacks from 'hci_ldisc.c', for example 'hci_uart_send_frame()'. > Now 'hci_uart_send_frame()' checks 'HCI_UART_PROTO_READY' bit set, and > if not - it fails. Problem is that 'HCI_UART_PROTO_READY' is set after > 'hci_register_dev()', and there is tiny chance that 'hci_power_on()' will > be executed before setting this bit. In that case HCI init logic fails. > > Patch moves setting of 'HCI_UART_PROTO_READY' before calling function > 'hci_uart_register_dev()'. > > Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> > --- > Changelog v1->v2: > * Move 'set_bit()' before 'hci_uart_register_dev()' instead of > adding new bit 'HCI_UART_PROTO_INIT'. > > drivers/bluetooth/hci_ldisc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c > index 30192bb083549..07b9aa09bbe2e 100644 > --- a/drivers/bluetooth/hci_ldisc.c > +++ b/drivers/bluetooth/hci_ldisc.c > @@ -704,12 +704,13 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id) > > hu->proto = p; > > + set_bit(HCI_UART_PROTO_READY, &hu->flags); > + > err = hci_uart_register_dev(hu); > if (err) { > return err; > } > > - set_bit(HCI_UART_PROTO_READY, &hu->flags); > return 0; > } >
Hi Arseniy, On Thu, Dec 19, 2024 at 1:42 PM Arseniy Krasnov <avkrasnov@salutedevices.com> wrote: > > Hi, sorry i'm new in bluetooth subsystem. I get the following > message from CI: > https://patchwork.kernel.org/project/bluetooth/list/?series=918554 > Where one of tests was failed. Where I can get more information > about failure: > > tedd_an/TestRunner_mgmt-tester fail TestRunner_mgmt-tester: Total: 490, Passed: 483 (98.6%), Failed: 3, Not Run: 4 These looks like a false positives, your changes should affect the tests since that run with hci_vhci driver, not the uart ones. > ? > > Thanks > > On 17.12.2024 11:12, Arseniy Krasnov wrote: > > 'hci_register_dev()' calls power up function, which is executed by > > kworker - 'hci_power_on()'. This function does access to bluetooth chip > > using callbacks from 'hci_ldisc.c', for example 'hci_uart_send_frame()'. > > Now 'hci_uart_send_frame()' checks 'HCI_UART_PROTO_READY' bit set, and > > if not - it fails. Problem is that 'HCI_UART_PROTO_READY' is set after > > 'hci_register_dev()', and there is tiny chance that 'hci_power_on()' will > > be executed before setting this bit. In that case HCI init logic fails. > > > > Patch moves setting of 'HCI_UART_PROTO_READY' before calling function > > 'hci_uart_register_dev()'. > > > > Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> > > --- > > Changelog v1->v2: > > * Move 'set_bit()' before 'hci_uart_register_dev()' instead of > > adding new bit 'HCI_UART_PROTO_INIT'. > > > > drivers/bluetooth/hci_ldisc.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c > > index 30192bb083549..07b9aa09bbe2e 100644 > > --- a/drivers/bluetooth/hci_ldisc.c > > +++ b/drivers/bluetooth/hci_ldisc.c > > @@ -704,12 +704,13 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id) > > > > hu->proto = p; > > > > + set_bit(HCI_UART_PROTO_READY, &hu->flags); > > + > > err = hci_uart_register_dev(hu); > > if (err) { > > return err; > > } > > > > - set_bit(HCI_UART_PROTO_READY, &hu->flags); > > return 0; > > } > >
Hi Arseniy, On Tue, Dec 17, 2024 at 3:12 AM Arseniy Krasnov <avkrasnov@salutedevices.com> wrote: > > 'hci_register_dev()' calls power up function, which is executed by > kworker - 'hci_power_on()'. This function does access to bluetooth chip > using callbacks from 'hci_ldisc.c', for example 'hci_uart_send_frame()'. > Now 'hci_uart_send_frame()' checks 'HCI_UART_PROTO_READY' bit set, and > if not - it fails. Problem is that 'HCI_UART_PROTO_READY' is set after > 'hci_register_dev()', and there is tiny chance that 'hci_power_on()' will > be executed before setting this bit. In that case HCI init logic fails. > > Patch moves setting of 'HCI_UART_PROTO_READY' before calling function > 'hci_uart_register_dev()'. > > Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> > --- > Changelog v1->v2: > * Move 'set_bit()' before 'hci_uart_register_dev()' instead of > adding new bit 'HCI_UART_PROTO_INIT'. What drivers/controllers this was tested with? I want to make sure this doesn't cause regressions to other drivers if there are perhaps some drivers assuming HCI_UART_PROTO_READY was set in a certain order. > drivers/bluetooth/hci_ldisc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c > index 30192bb083549..07b9aa09bbe2e 100644 > --- a/drivers/bluetooth/hci_ldisc.c > +++ b/drivers/bluetooth/hci_ldisc.c > @@ -704,12 +704,13 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id) > > hu->proto = p; > > + set_bit(HCI_UART_PROTO_READY, &hu->flags); > + > err = hci_uart_register_dev(hu); > if (err) { > return err; > } > > - set_bit(HCI_UART_PROTO_READY, &hu->flags); > return 0; > } > > -- > 2.30.1
On 19.12.2024 22:18, Luiz Augusto von Dentz wrote: > Hi Arseniy, > > On Tue, Dec 17, 2024 at 3:12 AM Arseniy Krasnov > <avkrasnov@salutedevices.com> wrote: >> >> 'hci_register_dev()' calls power up function, which is executed by >> kworker - 'hci_power_on()'. This function does access to bluetooth chip >> using callbacks from 'hci_ldisc.c', for example 'hci_uart_send_frame()'. >> Now 'hci_uart_send_frame()' checks 'HCI_UART_PROTO_READY' bit set, and >> if not - it fails. Problem is that 'HCI_UART_PROTO_READY' is set after >> 'hci_register_dev()', and there is tiny chance that 'hci_power_on()' will >> be executed before setting this bit. In that case HCI init logic fails. >> >> Patch moves setting of 'HCI_UART_PROTO_READY' before calling function >> 'hci_uart_register_dev()'. >> >> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> >> --- >> Changelog v1->v2: >> * Move 'set_bit()' before 'hci_uart_register_dev()' instead of >> adding new bit 'HCI_UART_PROTO_INIT'. > > What drivers/controllers this was tested with? I want to make sure > this doesn't cause regressions to other drivers if there are perhaps > some drivers assuming HCI_UART_PROTO_READY was set in a certain order. Hi, I tested this on: CONFIG_BT=y CONFIG_BT_HCIUART=y CONFIG_BT_HCIUART_H4=y Yes, my v1 patchset with extra INIT bit was targeted to keep original behaviour - e.g. PROTO_READY bit usage still the same, just adding extra bit to handle this specific case. Thanks > >> drivers/bluetooth/hci_ldisc.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c >> index 30192bb083549..07b9aa09bbe2e 100644 >> --- a/drivers/bluetooth/hci_ldisc.c >> +++ b/drivers/bluetooth/hci_ldisc.c >> @@ -704,12 +704,13 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id) >> >> hu->proto = p; >> >> + set_bit(HCI_UART_PROTO_READY, &hu->flags); >> + >> err = hci_uart_register_dev(hu); >> if (err) { >> return err; >> } >> >> - set_bit(HCI_UART_PROTO_READY, &hu->flags); >> return 0; >> } >> >> -- >> 2.30.1 > > >
On 19.12.2024 22:01, Luiz Augusto von Dentz wrote: > Hi Arseniy, > > On Thu, Dec 19, 2024 at 1:42 PM Arseniy Krasnov > <avkrasnov@salutedevices.com> wrote: >> >> Hi, sorry i'm new in bluetooth subsystem. I get the following >> message from CI: >> https://patchwork.kernel.org/project/bluetooth/list/?series=918554 >> Where one of tests was failed. Where I can get more information >> about failure: >> >> tedd_an/TestRunner_mgmt-tester fail TestRunner_mgmt-tester: Total: 490, Passed: 483 (98.6%), Failed: 3, Not Run: 4 > > These looks like a false positives, your changes should affect the > tests since that run with hci_vhci driver, not the uart ones. Got it, Thanks > >> ? >> >> Thanks >> >> On 17.12.2024 11:12, Arseniy Krasnov wrote: >>> 'hci_register_dev()' calls power up function, which is executed by >>> kworker - 'hci_power_on()'. This function does access to bluetooth chip >>> using callbacks from 'hci_ldisc.c', for example 'hci_uart_send_frame()'. >>> Now 'hci_uart_send_frame()' checks 'HCI_UART_PROTO_READY' bit set, and >>> if not - it fails. Problem is that 'HCI_UART_PROTO_READY' is set after >>> 'hci_register_dev()', and there is tiny chance that 'hci_power_on()' will >>> be executed before setting this bit. In that case HCI init logic fails. >>> >>> Patch moves setting of 'HCI_UART_PROTO_READY' before calling function >>> 'hci_uart_register_dev()'. >>> >>> Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> >>> --- >>> Changelog v1->v2: >>> * Move 'set_bit()' before 'hci_uart_register_dev()' instead of >>> adding new bit 'HCI_UART_PROTO_INIT'. >>> >>> drivers/bluetooth/hci_ldisc.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c >>> index 30192bb083549..07b9aa09bbe2e 100644 >>> --- a/drivers/bluetooth/hci_ldisc.c >>> +++ b/drivers/bluetooth/hci_ldisc.c >>> @@ -704,12 +704,13 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id) >>> >>> hu->proto = p; >>> >>> + set_bit(HCI_UART_PROTO_READY, &hu->flags); >>> + >>> err = hci_uart_register_dev(hu); >>> if (err) { >>> return err; >>> } >>> >>> - set_bit(HCI_UART_PROTO_READY, &hu->flags); >>> return 0; >>> } >>> > > >
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c index 30192bb083549..07b9aa09bbe2e 100644 --- a/drivers/bluetooth/hci_ldisc.c +++ b/drivers/bluetooth/hci_ldisc.c @@ -704,12 +704,13 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id) hu->proto = p; + set_bit(HCI_UART_PROTO_READY, &hu->flags); + err = hci_uart_register_dev(hu); if (err) { return err; } - set_bit(HCI_UART_PROTO_READY, &hu->flags); return 0; }
'hci_register_dev()' calls power up function, which is executed by kworker - 'hci_power_on()'. This function does access to bluetooth chip using callbacks from 'hci_ldisc.c', for example 'hci_uart_send_frame()'. Now 'hci_uart_send_frame()' checks 'HCI_UART_PROTO_READY' bit set, and if not - it fails. Problem is that 'HCI_UART_PROTO_READY' is set after 'hci_register_dev()', and there is tiny chance that 'hci_power_on()' will be executed before setting this bit. In that case HCI init logic fails. Patch moves setting of 'HCI_UART_PROTO_READY' before calling function 'hci_uart_register_dev()'. Signed-off-by: Arseniy Krasnov <avkrasnov@salutedevices.com> --- Changelog v1->v2: * Move 'set_bit()' before 'hci_uart_register_dev()' instead of adding new bit 'HCI_UART_PROTO_INIT'. drivers/bluetooth/hci_ldisc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)