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@xxxxxxxxxx> 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@xxxxxxxxx>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> 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
