[PATCH v2] Bluetooth: hci_uart: fix race during initialization

[PATCH v2] Bluetooth: hci_uart: fix race during initialization

[Arseniy Krasnov]

'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;
 }
 
-- 
2.30.1

RE: [v2] Bluetooth: hci_uart: fix race during initialization

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2109 bytes --]

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

Re: [PATCH v2] Bluetooth: hci_uart: fix race during initialization

[Arseniy Krasnov]

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;
>  }
>  

Re: [PATCH v2] Bluetooth: hci_uart: fix race during initialization

[Luiz Augusto von Dentz]

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;
> >  }
> >



-- 
Luiz Augusto von Dentz

Re: [PATCH v2] Bluetooth: hci_uart: fix race during initialization

[Arseniy Krasnov]

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;
>>>  }
>>>
> 
> 
> 

Re: [PATCH v2] Bluetooth: hci_uart: fix race during initialization

[Luiz Augusto von Dentz]

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



-- 
Luiz Augusto von Dentz

Re: [PATCH v2] Bluetooth: hci_uart: fix race during initialization

[Arseniy Krasnov]

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

Re: [PATCH v2] Bluetooth: hci_uart: fix race during initialization

[Arseniy Krasnov]

On 19.12.2024 23:07, Arseniy Krasnov wrote:
> 
> 
> 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

Hi, sorry, but is this enough info about my test env ?

Thanks

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

Re: [PATCH v2] Bluetooth: hci_uart: fix race during initialization

[Arseniy Krasnov]

Hi, sorry ping

Thanks

On 08.01.2025 15:39, Arseniy Krasnov wrote:
> 
> 
> On 19.12.2024 23:07, Arseniy Krasnov wrote:
>>
>>
>> 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
> 
> Hi, sorry, but is this enough info about my test env ?
> 
> Thanks
> 
>>
>> 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
>>>
>>>
>>>

Re: [PATCH v2] Bluetooth: hci_uart: fix race during initialization

[Luiz Augusto von Dentz]

Hi Arseniy,

On Thu, Jan 30, 2025 at 1:34 PM Arseniy Krasnov
<avkrasnov@salutedevices.com> wrote:
>
> Hi, sorry ping
>
> Thanks
>
> On 08.01.2025 15:39, Arseniy Krasnov wrote:
> >
> >
> > On 19.12.2024 23:07, Arseniy Krasnov wrote:
> >>
> >>
> >> 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
> >
> > Hi, sorry, but is this enough info about my test env ?
> >
> > Thanks
> >
> >>
> >> 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
> >>>
> >>>
> >>>

Please resend it.

-- 
Luiz Augusto von Dentz

Re: [PATCH v2] Bluetooth: hci_uart: fix race during initialization

[Arseniy Krasnov]

Hi, sorry ping

Thanks

On 08.01.2025 15:39, Arseniy Krasnov wrote:
> 
> 
> On 19.12.2024 23:07, Arseniy Krasnov wrote:
>>
>>
>> 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
> 
> Hi, sorry, but is this enough info about my test env ?
> 
> Thanks
> 
>>
>> 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
>>>
>>>
>>>

[RESEND PATCH v2] Bluetooth: hci_uart: fix race during initialization

[Arseniy Krasnov]

'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;
 }
 
-- 
2.30.1

RE: [RESEND,v2] Bluetooth: hci_uart: fix race during initialization

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

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=929318

---Test result---

Test Summary:
CheckPatch                    PENDING   0.24 seconds
GitLint                       PENDING   0.19 seconds
SubjectPrefix                 PASS      0.11 seconds
BuildKernel                   PASS      24.52 seconds
CheckAllWarning               PASS      26.96 seconds
CheckSparse                   PASS      30.12 seconds
BuildKernel32                 PASS      24.45 seconds
TestRunnerSetup               PASS      422.74 seconds
TestRunner_l2cap-tester       PASS      20.28 seconds
TestRunner_iso-tester         PASS      29.18 seconds
TestRunner_bnep-tester        PASS      4.83 seconds
TestRunner_mgmt-tester        FAIL      119.73 seconds
TestRunner_rfcomm-tester      PASS      7.42 seconds
TestRunner_sco-tester         PASS      9.31 seconds
TestRunner_ioctl-tester       PASS      8.02 seconds
TestRunner_mesh-tester        PASS      5.96 seconds
TestRunner_smp-tester         PASS      7.02 seconds
TestRunner_userchan-tester    PASS      4.99 seconds
IncrementalBuild              PENDING   0.43 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: 485 (99.0%), Failed: 1, Not Run: 4

Failed Test Cases
LL Privacy - Set Flags 3 (2 Devices to RL)           Failed       0.198 seconds
##############################
Test: IncrementalBuild - PENDING
Desc: Incremental build with the patches in the series
Output:



---
Regards,
Linux Bluetooth

Re: [RESEND PATCH v2] Bluetooth: hci_uart: fix race during initialization

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Thu, 30 Jan 2025 21:43:26 +0300 you 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.
> 
> [...]

Here is the summary with links:
  - [RESEND,v2] Bluetooth: hci_uart: fix race during initialization
    https://git.kernel.org/bluetooth/bluetooth-next/c/c411c62cc133

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html