Date:   Sat, 5 Mar 2022 15:46:39 +0800
From:   Joseph Hwang <josephsih@...gle.com>
To:     Marcel Holtmann <marcel@...tmann.org>
Cc:     BlueZ <linux-bluetooth@...r.kernel.org>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        Pali Rohár <pali@...nel.org>,
        CrosBT Upstreaming <chromeos-bluetooth-upstreaming@...omium.org>,
        Archie Pusaka <apusaka@...omium.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        LKML <linux-kernel@...r.kernel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH v4 2/3] Bluetooth: btintel: surface Intel telemetry events
 through mgmt

Hi Marcel, thank you for reviewing the patches! I have some questions.
Please read my replies in the lines below. Thanks!

On Thu, Feb 17, 2022 at 8:53 PM Marcel Holtmann <marcel@...tmann.org> wrote:
>
> Hi Jospeh,
>
> > When receiving a HCI vendor event, the kernel checks if it is an
> > Intel telemetry event. If yes, the event is sent to bluez user
> > space through the mgmt socket.
> >
> > Signed-off-by: Joseph Hwang <josephsih@...omium.org>
> > Reviewed-by: Archie Pusaka <apusaka@...omium.org>
> > ---
> >
> > (no changes since v3)
> >
> > Changes in v3:
> > - Move intel_vendor_evt() from hci_event.c to the btintel driver.
> >
> > Changes in v2:
> > - Drop the pull_quality_report_data function from hci_dev.
> >  Do not bother hci_dev with it. Do not bleed the details
> >  into the core.
> >
> > drivers/bluetooth/btintel.c      | 37 +++++++++++++++++++++++++++++++-
> > drivers/bluetooth/btintel.h      |  7 ++++++
> > include/net/bluetooth/hci_core.h |  2 ++
> > net/bluetooth/hci_event.c        | 12 +++++++++++
> > 4 files changed, 57 insertions(+), 1 deletion(-)
>
> I don’t like intermixing core additions with driver implementations of it. I thought that I have mentioned this a few times, but maybe I missed that in the last review round. So first introduce the callbacks and the handling in hci_core etc. and then provide a patch for the driver using it.
>
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index 06514ed66022..c7732da2752f 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -2401,9 +2401,12 @@ static int btintel_setup_combined(struct hci_dev *hdev)
> >       set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> >       set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
> >
> > -     /* Set up the quality report callback for Intel devices */
> > +     /* Set up the quality report callbacks for Intel devices */
> >       hdev->set_quality_report = btintel_set_quality_report;
> >
> > +     /* Set up the vendor specific callback for Intel devices */
> > +     hdev->vendor_evt = btintel_vendor_evt;
> > +
> >       /* For Legacy device, check the HW platform value and size */
> >       if (skb->len == sizeof(ver) && skb->data[1] == 0x37) {
> >               bt_dev_dbg(hdev, "Read the legacy Intel version information");
> > @@ -2650,6 +2653,38 @@ void btintel_secure_send_result(struct hci_dev *hdev,
> > }
> > EXPORT_SYMBOL_GPL(btintel_secure_send_result);
> >
> > +#define INTEL_PREFIX         0x8087
> > +#define TELEMETRY_CODE               0x03
> > +
> > +struct intel_prefix_evt_data {
> > +     __le16 vendor_prefix;
> > +     __u8 code;
> > +     __u8 data[];   /* a number of struct intel_tlv subevents */
> > +} __packed;
> > +
> > +static bool is_quality_report_evt(struct sk_buff *skb)
> > +{
> > +     struct intel_prefix_evt_data *ev;
> > +     u16 vendor_prefix;
> > +
> > +     if (skb->len < sizeof(struct intel_prefix_evt_data))
> > +             return false;
> > +
> > +     ev = (struct intel_prefix_evt_data *)skb->data;
> > +     vendor_prefix = __le16_to_cpu(ev->vendor_prefix);
> > +
> > +     return vendor_prefix == INTEL_PREFIX && ev->code == TELEMETRY_CODE;
> > +}
> > +
> > +void btintel_vendor_evt(struct hci_dev *hdev,  void *data, struct sk_buff *skb)
> > +{
> > +     /* Only interested in the telemetry event for now. */
> > +     if (hdev->set_quality_report && is_quality_report_evt(skb))
> > +             mgmt_quality_report(hdev, skb->data, skb->len,
> > +                                 QUALITY_SPEC_INTEL_TELEMETRY);
>
> You can not do that. Keep the interaction with hci_dev as limited as possible. I think it would be best to introduce a hci_recv_quality_report function that drivers can call.

Do you mean to set a callback hdev->hci_recv_quality_report in the
kernel (which will invoke mgmt_quality_report) so that the driver can
call it to send the quality reports?  There would be very few things
done (maybe just to strip off the prefix header) in the driver before
passing the skb data back to the kernel via
hdev->hci_recv_quality_report. Does this sound good with you?

>
> And really don’t bother with all these check. Dissect the vendor event, if it is a quality report, then just report it via that callback. And you should be stripping off the prefix etc. Just report the plain data.

I will remove those checks. As to stripping off the prefix, that was
what I did in Series-version: 1. Your comment about my AOSP function
in pulling off the prefix header from the skb was “just do a basic
length check and then move on. The kernel has no interest in this
data.” So that is why the whole skb->data is sent to the user space
for further handling. Please let me know if it is better to strip off
the prefix header for both Intel and AOSP.

>
> > +}
> > +EXPORT_SYMBOL_GPL(btintel_vendor_evt);
> > +
> > MODULE_AUTHOR("Marcel Holtmann <marcel@...tmann.org>");
> > MODULE_DESCRIPTION("Bluetooth support for Intel devices ver " VERSION);
> > MODULE_VERSION(VERSION);
> > diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
> > index e0060e58573c..82dc278b09eb 100644
> > --- a/drivers/bluetooth/btintel.h
> > +++ b/drivers/bluetooth/btintel.h
> > @@ -211,6 +211,7 @@ void btintel_bootup(struct hci_dev *hdev, const void *ptr, unsigned int len);
> > void btintel_secure_send_result(struct hci_dev *hdev,
> >                               const void *ptr, unsigned int len);
> > int btintel_set_quality_report(struct hci_dev *hdev, bool enable);
> > +void btintel_vendor_evt(struct hci_dev *hdev,  void *data, struct sk_buff *skb);
> > #else
> >
> > static inline int btintel_check_bdaddr(struct hci_dev *hdev)
> > @@ -306,4 +307,10 @@ static inline int btintel_set_quality_report(struct hci_dev *hdev, bool enable)
> > {
> >       return -ENODEV;
> > }
> > +
> > +static inline void btintel_vendor_evt(struct hci_dev *hdev,  void *data,
>
> Double space here.
>
> > +                                   struct sk_buff *skb)
> > +{
> > +}
> > +
> > #endif
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index ea83619ac4de..3505ffe20779 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -635,6 +635,8 @@ struct hci_dev {
> >       void (*cmd_timeout)(struct hci_dev *hdev);
> >       bool (*wakeup)(struct hci_dev *hdev);
> >       int (*set_quality_report)(struct hci_dev *hdev, bool enable);
> > +     void (*vendor_evt)(struct hci_dev *hdev, void *data,
> > +                        struct sk_buff *skb);
>
> So I do not understand the void *data portion. Just hand down the skb.
>
> >       int (*get_data_path_id)(struct hci_dev *hdev, __u8 *data_path);
> >       int (*get_codec_config_data)(struct hci_dev *hdev, __u8 type,
> >                                    struct bt_codec *codec, __u8 *vnd_len,
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 6468ea0f71bd..e34dea0f0c2e 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -4250,6 +4250,7 @@ static void hci_num_comp_blocks_evt(struct hci_dev *hdev, void *data,
> >  *       space to avoid collision.
> >  */
> > static unsigned char AOSP_BQR_PREFIX[] = { 0x58 };
> > +static unsigned char INTEL_PREFIX[] = { 0x87, 0x80 };
>
> This really bugs me. Intel specifics can not be here. I think we really have to push all vendor events down to the driver.

Can we have new hdev callbacks like hdev->get_vendor_prefix() and
hdev->get_vendor_prefix_len() so that the vendor drivers such as Intel
can set up their driver functions to derive the prefix and its length?

>
> >
> > /* Some vendor prefixes are fixed values and lengths. */
> > #define FIXED_EVT_PREFIX(_prefix, _vendor_func)                               \
> > @@ -4273,6 +4274,16 @@ static unsigned char AOSP_BQR_PREFIX[] = { 0x58 };
> >       .get_prefix_len = _prefix_len_func,                             \
> > }
> >
> > +/* Every vendor that handles particular vendor events in its driver should
> > + * 1. set up the vendor_evt callback in its driver and
> > + * 2. add an entry in struct vendor_event_prefix.
> > + */
> > +static void vendor_evt(struct hci_dev *hdev,  void *data, struct sk_buff *skb)
> > +{
> > +     if (hdev->vendor_evt)
> > +             hdev->vendor_evt(hdev, data, skb);
> > +}
> > +
> > /* Every distinct vendor specification must have a well-defined vendor
> >  * event prefix to determine if a vendor event meets the specification.
> >  * If an event prefix is fixed, it should be delcared with FIXED_EVT_PREFIX.
> > @@ -4287,6 +4298,7 @@ struct vendor_event_prefix {
> >       __u8 (*get_prefix_len)(struct hci_dev *hdev);
> > } evt_prefixes[] = {
> >       FIXED_EVT_PREFIX(AOSP_BQR_PREFIX, aosp_quality_report_evt),
> > +     FIXED_EVT_PREFIX(INTEL_PREFIX, vendor_evt),
> >       DYNAMIC_EVT_PREFIX(get_msft_evt_prefix, get_msft_evt_prefix_len,
> >                          msft_vendor_evt),
>
> Regards
>
> Marcel
>


-- 

Joseph Shyh-In Hwang
Email: josephsih@...gle.com

Powered by blists - more mailing lists