Re: [PATCH v2] Bluetooth: hci_event: Fix vendor (unknown) opcode status handling

[Hans de Goede <hdegoede@redhat.com>]

Hi Luiz,

On 8/11/22 22:27, Luiz Augusto von Dentz wrote:
> Hi Hans,
> 
> On Thu, Aug 11, 2022 at 5:01 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Commit c8992cffbe74 ("Bluetooth: hci_event: Use of a function table to
>> handle Command Complete") was (presumably) meant to only refactor things
>> without any functional changes.
>>
>> But it does have one undesirable side-effect, before *status would always
>> be set to skb->data[0] and it might be overridden by some of the opcode
>> specific handling. While now it always set by the opcode specific handlers.
>> This means that if the opcode is not known *status does not get set any
>> more at all!
>>
>> This behavior change has broken bluetooth support for BCM4343A0 HCIs,
>> the hci_bcm.c code tries to configure UART attached HCIs at a higher
>> baudraute using vendor specific opcodes. The BCM4343A0 does not
>> support this and this used to simply fail:
>>
>> [   25.646442] Bluetooth: hci0: BCM: failed to write clock (-56)
>> [   25.646481] Bluetooth: hci0: Failed to set baudrate
>>
>> After which things would continue with the initial baudraute. But now
>> that hci_cmd_complete_evt() no longer sets status for unknown opcodes
>> *status is left at 0. This causes the hci_bcm.c code to think the baudraute
>> has been changed on the HCI side and to also adjust the UART baudrate,
>> after which communication with the HCI is broken, leading to:
>>
>> [   28.579042] Bluetooth: hci0: command 0x0c03 tx timeout
>> [   36.961601] Bluetooth: hci0: BCM: Reset failed (-110)
>>
>> And non working bluetooth. Fix this by restoring the previous
>> default "*status = skb->data[0]" handling for unknown opcodes.
>>
>> Fixes: c8992cffbe74 ("Bluetooth: hci_event: Use of a function table to handle Command Complete")
>> Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> - Add a comment that byte 0 containing the status is not guaranteed
>>   by the Bluetooth specification
>> ---
>>  net/bluetooth/hci_event.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index af17dfb20e01..eb43afd5549a 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -3996,6 +3996,26 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, void *data,
>>                         break;
>>                 }
>>         }
>> +       if (i == ARRAY_SIZE(hci_cc_table)) {
>> +               /* Unknown opcode, assume byte 0 contains the status, so
>> +                * that e.g. __hci_cmd_sync() properly returns errors
>> +                * for vendor specific commands send by HCI drivers.
>> +                *
>> +                * Note that the specification does not specify that
>> +                * byte 0 is the status:
>> +                *
>> +                * BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E
>> +                * page 2189:
>> +                *
>> +                * Return_Parameters:
>> +                * Size: Depends on command
>> +                *
>> +                * For now using byte 0 seems to work fine, but in the future
>> +                * this may need to be updated so that drivers using vendor
>> +                * commands can specify their own completion handler.
>> +                */
>> +               *status = skb->data[0];
>> +       }
>>
>>         handle_cmd_cnt_and_timer(hdev, ev->ncmd);
>>
>> --
>> 2.37.1
> 
> Not sure why CI bot didn't respond to the list but this patch was
> already applied yesterday:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=84a0a27ea39a9caed74d80a78666a91a9ea5e12b

Great, thank you.

Regards,

Hans