Re: [PATCH RFC v2 3/3] Bluetooth: Add hdev->recv_bt_vendor() to handle BT vendor frames

[Zijun Hu]

On 7/23/2026 9:56 PM, Luiz Augusto von Dentz wrote:
>> | Vendor | Coredump | Other                                   |
>> |--------|----------|-----------------------------------------|
>> | QCOM   | 0xEDD    | Enhanced Logging (0xEDC)                |
>> | MTK    | 0xFC6F   | Firmware debug logging (0x05FF, 0x05FE) |
>> | NXP    | 0xFFF    | —                                       |
>>
>> So this hook may need to cover both. Any suggestion for a good name?
> Derr, connection traffic is not handled as events and you have the
> likes of  hdev->classify_pkt_type(hdev, skb); to reclassify them to
> HCI_DIAG_PKT. Don't tell me the firmware it generating Connection
> Complete, NOCP, or any other event, for these because that is totally
> against the spec since the handle wouldn't match any connection.
> 

okay, got your point.

so, what about changing recv_vendor_ev() to recv_ev_vendor() to map HCI_EV_VENDOR ?


>>>>         void (*recv_vendor_pkt)(struct hci_dev *hdev, struct sk_buff *skb);
>>>>         void (*notify)(struct hci_dev *hdev, unsigned int evt);
>>>>         void (*hw_error)(struct hci_dev *hdev, u8 code);
>>>> @@ -2329,6 +2331,7 @@ static inline int hci_check_conn_params(u16 min, u16 max, u16 latency,
>>>>  int hci_register_cb(struct hci_cb *hcb);
>>>>  int hci_unregister_cb(struct hci_cb *hcb);
>>>>
>>>> +bool hci_recv_bt_vendor(struct hci_dev *hdev, struct sk_buff *skb);
>>>>  int hci_send_vendor_frame(struct hci_dev *hdev, struct iov_iter *iter);
>>>>
>> [...]
>>
>>>>  static bool hci_req_is_complete(struct hci_dev *hdev)
>>>>  {
>>>>         struct sk_buff *skb;
>>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>>> index ea858391c789..94c02b66bdfa 100644
>>>> --- a/net/bluetooth/hci_event.c
>>>> +++ b/net/bluetooth/hci_event.c
>>>> @@ -7801,6 +7801,11 @@ void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb)
>>>>                 goto done;
>>>>         }
>>>>
>>>> +       if (hci_recv_bt_vendor(hdev, skb)) {
>>>> +               hdev->stat.evt_rx++;
>>>> +               return;
>>>> +       }
>>> This seems too early actually, I would have expected it to run after
>>> hci_event_func or within it if ev->func is NULL, that said we
>> I don't think running it after hci_event_func() works, because the VSE has already been
>> corrupted by then, as explained below:
>>
>> msft_vendor_evt() is the handler for all vendor events called by hci_event_func(), but many
>> VSEs are not MSFT ones. Once the MSFT extension is enabled, it always calls skb_pull_data()
>> and corrupts the non-MSFT VSEs.
>>
>> Besides, running it there is asymmetric with the ACL path: the event header is already pulled,
>> but the ACL header isn't.
>>
>> For ev->func is NULL:
>> it may mean we don't need to care about that event, so the current behavior of ignoring it
>> is reasonable.
>>
>>> currently assume HCI_EV_VENDOR=msft_vendor_evt which is probably not
>>> valid, or maybe it is which then screw the whole idea that 0xff is
>>> only used with 1 vendor specific domain, now if we try to squeesh both
>>> a vendor event and a msft event handler their opcodes shall not
>>> collide otherwise the whole thing doesn't work.
>>>
>> let me explain more for recv_bt_vendor() for BT as below:
>>
>> Design idea:
>> let the transport driver consume whatever frames it is interested in first; for
>> the rest, don't care how the stack handles them — it doesn't matter even if they are
>> corrupted.
> Well I already told you that by doing preposing we may expose driver
> to the same type of bugs that existed in core in the past, so we maybe
> reintroducing parsing errors and since this requires to preserve the
> skb it will not be able to use skb_data_pulll (which internally checks
> skb->len).
> 


Sorry, my description was misleading.
The callback registered by the transport driver is meant to avoid pre-processing:
the stack invokes it for the driver's VSEs of interest, and the callback then uses skb_pull_data()
to handle the event just as the stack would.
>> Motivation:
>> For the VSEs and vendor-reserved-handle ACLs that transport drivers are interested in, move
>> their handling from the transport driver's RX path (IRQ-disabled context) to the stack's
>> process context — i.e. avoid pre-processing them.
>>
>> Advantages:
>> -) Avoid skb_clone() in IRQ-disabled context of RX-path to improve performance
>>    take INTEL diagnostic events as an example, btintel_diagnostics() skb_clone() them
>> -) Avoid re-processing VSEs that the driver has already handled.
>>    Intel diagnostic events still arrive at hci_event_packet() in hci_rx_work(), even though
>>    they have already been handled by btintel_diagnostics().
> What does btintel have to do with this? Are you saying there are bugs
> in it and you are trying to report them here?


No, I'm not reporting bugs in btintel. Almost all transport drivers handle these vendor frames the same way

show what this callback may improve, using Intel as an example since it came up earlier as a reference.


Any further comments ?