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

[Luiz Augusto von Dentz]

Hi Zijun,

On Thu, Jul 23, 2026 at 7:18 AM Zijun Hu <zijun.hu@xxxxxxxxxxxxxxxx> wrote:
>
> On 7/23/2026 12:46 AM, Luiz Augusto von Dentz wrote:
> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >> index 428261591288..12f3720f6cc8 100644
> >> --- a/include/net/bluetooth/hci_core.h
> >> +++ b/include/net/bluetooth/hci_core.h
> >> @@ -646,6 +646,8 @@ struct hci_dev {
> >>         int (*setup)(struct hci_dev *hdev);
> >>         int (*shutdown)(struct hci_dev *hdev);
> >>         int (*send)(struct hci_dev *hdev, struct sk_buff *skb);
> >> +       /* Return true if @skb was consumed, false otherwise */
> >> +       bool (*recv_bt_vendor)(struct hci_dev *hdev, struct sk_buff *skb);
> > This should be probably called recv_vendor_ev or something like that.
> >
>
> For BT, besides VSEs, some vendors also reserve ACL handles for special usages, e.g.:
>
> | 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.

> >>         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).

> 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?

> -) Log them via btmon to help debugging
>     Several existing transport drivers consume these frames directly without btmon logging
>
> I'd welcome your thoughts — especially on the hook name (point 1) and whether the
> placement/design here is acceptable. Happy to rework it based on your guidance.
>
> > Anyway, I had the impression you would be using a vendor packet not a
> > vendor event, or you use both?
> >
>
> My PERI-HCI requirement is already addressed by recv_vendor_pkt() in [PATCH 2/3]
>
> This hook, recv_bt_vendor(), addresses concerns raised during the BT-HCI discussion. The same
> problem exists in many transport drivers, so it tries to address them for BT along the way
>
> I'd like to use it in the upcoming QCC2072 support as well, once these concerns are resolved.
>
> >>         hci_dev_lock(hdev);
> >>         kfree_skb(hdev->recv_event);
> >>         hdev->recv_event = skb_clone(skb, GFP_KERNEL);
>


-- 
Luiz Augusto von Dentz