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
--
Luiz Augusto von Dentz
