On 16.12.2024 17:52, Luiz Augusto von Dentz wrote:
> Hi Arseniy,
>
> On Wed, Nov 27, 2024 at 7:38 AM Arseniy Krasnov
> <avkrasnov@xxxxxxxxxxxxxxxxx> 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 adds one more bit in addition to 'HCI_UART_PROTO_READY' which
>> allows to perform power up logic without original bit set.
>>
>> Signed-off-by: Arseniy Krasnov <avkrasnov@xxxxxxxxxxxxxxxxx>
>> ---
>> drivers/bluetooth/hci_ldisc.c | 19 ++++++++++++++-----
>> drivers/bluetooth/hci_uart.h | 1 +
>> 2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
>> index 30192bb083549..2e20f00649bd7 100644
>> --- a/drivers/bluetooth/hci_ldisc.c
>> +++ b/drivers/bluetooth/hci_ldisc.c
>> @@ -102,7 +102,8 @@ static inline struct sk_buff *hci_uart_dequeue(struct hci_uart *hu)
>> if (!skb) {
>> percpu_down_read(&hu->proto_lock);
>>
>> - if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
>> + if (test_bit(HCI_UART_PROTO_READY, &hu->flags) ||
>> + test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>> skb = hu->proto->dequeue(hu);
>>
>> percpu_up_read(&hu->proto_lock);
>> @@ -124,7 +125,8 @@ int hci_uart_tx_wakeup(struct hci_uart *hu)
>> if (!percpu_down_read_trylock(&hu->proto_lock))
>> return 0;
>>
>> - if (!test_bit(HCI_UART_PROTO_READY, &hu->flags))
>> + if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
>> + !test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>> goto no_schedule;
>>
>> set_bit(HCI_UART_TX_WAKEUP, &hu->tx_state);
>> @@ -278,7 +280,8 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
>>
>> percpu_down_read(&hu->proto_lock);
>>
>> - if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
>> + if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
>> + !test_bit(HCI_UART_PROTO_INIT, &hu->flags)) {
>> percpu_up_read(&hu->proto_lock);
>> return -EUNATCH;
>> }
>> @@ -582,7 +585,8 @@ static void hci_uart_tty_wakeup(struct tty_struct *tty)
>> if (tty != hu->tty)
>> return;
>>
>> - if (test_bit(HCI_UART_PROTO_READY, &hu->flags))
>> + if (test_bit(HCI_UART_PROTO_READY, &hu->flags) ||
>> + test_bit(HCI_UART_PROTO_INIT, &hu->flags))
>> hci_uart_tx_wakeup(hu);
>> }
>>
>> @@ -608,7 +612,8 @@ static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data,
>>
>> percpu_down_read(&hu->proto_lock);
>>
>> - if (!test_bit(HCI_UART_PROTO_READY, &hu->flags)) {
>> + if (!test_bit(HCI_UART_PROTO_READY, &hu->flags) &&
>> + !test_bit(HCI_UART_PROTO_INIT, &hu->flags)) {
>> percpu_up_read(&hu->proto_lock);
>> return;
>> }
>> @@ -704,12 +709,16 @@ static int hci_uart_set_proto(struct hci_uart *hu, int id)
>>
>> hu->proto = p;
>>
>> + set_bit(HCI_UART_PROTO_INIT, &hu->flags);
>> +
>> err = hci_uart_register_dev(hu);
>> if (err) {
>> return err;
>> }
>>
>> set_bit(HCI_UART_PROTO_READY, &hu->flags);
>> + clear_bit(HCI_UART_PROTO_INIT, &hu->flags);
>
> This should be quite obvious, why don't you just move the
> HCI_UART_PROTO_READY above hci_uart_register_dev?
>
Hi, this is simplest way, but I had doubts that this may be so obvious because
I don't have enough experience in bt logic.
I'll send v2 with it.
Thanks
>> return 0;
>> }
>>
>> diff --git a/drivers/bluetooth/hci_uart.h b/drivers/bluetooth/hci_uart.h
>> index 00bf7ae82c5b7..39f39704be41f 100644
>> --- a/drivers/bluetooth/hci_uart.h
>> +++ b/drivers/bluetooth/hci_uart.h
>> @@ -89,6 +89,7 @@ struct hci_uart {
>> #define HCI_UART_REGISTERED 1
>> #define HCI_UART_PROTO_READY 2
>> #define HCI_UART_NO_SUSPEND_NOTIFIER 3
>> +#define HCI_UART_PROTO_INIT 4
>>
>> /* TX states */
>> #define HCI_UART_SENDING 1
>> --
>> 2.30.1
>
>
>
