Re: [PATCH v4 2/3] Bluetooth: btintel: surface Intel telemetry events through mgmt

[Marcel Holtmann <marcel@holtmann.org>]

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@chromium.org>
> Reviewed-by: Archie Pusaka <apusaka@chromium.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.

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.

> +}
> +EXPORT_SYMBOL_GPL(btintel_vendor_evt);
> +
> MODULE_AUTHOR("Marcel Holtmann <marcel@holtmann.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.

> 
> /* 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