Hi Hans,
On Mon, Aug 8, 2022 at 12:58 PM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Hans,
>
> On Sun, Aug 7, 2022 at 1:57 PM 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>
> > ---
> > net/bluetooth/hci_event.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index af17dfb20e01..fda31d558ded 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -3996,6 +3996,13 @@ 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.
> > + */
> > + *status = skb->data[0];
> > + }
>
> The format of return parameters in command is not defined by the spec:
>
> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E
> page 2189:
>
> Return_Parameters:
> Size: Depends on command
>
> This is the return parameter(s) for the command specified in the
> Command_Opcode event parameter. See each command’s definition for
> the list of return parameters associated with that command.
>
> So assuming the status is the first by is not quite right, although
> for the standard ones that seems to be valid, I think the best way to
> resolve this would have been to check if it a vendor command and then
> have the driver handle it or perhaps have some means for the driver to
> register it vendor_cc_table, we can perhaps have this as a workaround
> for now and only really change how we parse the cc for vendor commands
> if a vendor decide not to have a status as first parameter but Id
> probably leave a comment that quoting the spec that reminds us this
> code may need changing.
Are you still planning to send updates for this, I consider this quite
urgent given that it can break support with some vendors.
> > handle_cmd_cnt_and_timer(hdev, ev->ncmd);
> >
> > --
> > 2.37.1
> >
>
>
> --
> Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
