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