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

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

Hi Luiz,

On 8/11/22 00:26, Luiz Augusto von Dentz wrote:
> Hi Hans,
> 
> On Mon, Aug 8, 2022 at 12:58 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>>
>> Hi Hans,
>>
>> On Sun, Aug 7, 2022 at 1:57 PM 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>
>>> ---
>>>  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.

Right, sorry for being a bit slow. I will prepare + email a version 2
adding a comment that byte 0 being the status is not guaranteed with
vendor commands right away.

Regards,

Hans



> 
>>>         handle_cmd_cnt_and_timer(hdev, ev->ncmd);
>>>
>>> --
>>> 2.37.1
>>>
>>
>>
>> --
>> Luiz Augusto von Dentz
> 
> 
>