Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220811120103.47669-1-hdegoede@redhat.com>
In-Reply-To: <20220811120103.47669-1-hdegoede@redhat.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Thu, 11 Aug 2022 13:27:40 -0700
Message-ID: <CABBYNZL2TjDFCx3EFnYRhe2JCSh3YAgBPiNAHpk1Ya+e3sDtxQ@mail.gmail.com>
Subject: Re: [PATCH v2] Bluetooth: hci_event: Fix vendor (unknown) opcode
 status handling
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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

-- 
Luiz Augusto von Dentz