diff mbox series

[v1,2/2] Bluetooth: btintel: surface Intel telemetry events through mgmt

Message ID 20220121192152.v1.2.I63681490281b2392aa1ac05dff91a126394ab649@changeid (mailing list archive)
State Superseded
Headers show
Series [v1,1/2] Bluetooth: aosp: surface AOSP quality report through mgmt | expand

Checks

Context Check Description
tedd_an/checkpatch success Checkpatch PASS
tedd_an/gitlint success Gitlint PASS
tedd_an/subjectprefix success PASS

Commit Message

Joseph Hwang Jan. 21, 2022, 11:22 a.m. UTC 
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>
---

 drivers/bluetooth/btintel.c      | 43 +++++++++++++++++++++++++++++++-
 drivers/bluetooth/btintel.h      | 12 +++++++++
 include/net/bluetooth/hci_core.h |  2 ++
 net/bluetooth/hci_event.c        | 12 ++++++---
 4 files changed, 65 insertions(+), 4 deletions(-)

Comments

kernel test robot Jan. 21, 2022, 6:30 p.m. UTC | #1 
Hi Joseph,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on next-20220121]
[cannot apply to net-next/master net/master bluetooth/master v5.16]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Joseph-Hwang/Bluetooth-aosp-surface-AOSP-quality-report-through-mgmt/20220121-192436
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: ia64-randconfig-r035-20220121 (https://download.01.org/0day-ci/archive/20220122/202201220238.tZ1BfUpc-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/9484eb7f87f7c8759b6fd7eec9d431c375b97432
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Joseph-Hwang/Bluetooth-aosp-surface-AOSP-quality-report-through-mgmt/20220121-192436
        git checkout 9484eb7f87f7c8759b6fd7eec9d431c375b97432
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/bluetooth/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from arch/ia64/include/asm/pgtable.h:153,
                    from include/linux/pgtable.h:6,
                    from arch/ia64/include/asm/uaccess.h:40,
                    from include/linux/uaccess.h:11,
                    from include/linux/sched/task.h:11,
                    from include/linux/sched/signal.h:9,
                    from include/linux/rcuwait.h:6,
                    from include/linux/percpu-rwsem.h:7,
                    from include/linux/fs.h:33,
                    from include/linux/poll.h:10,
                    from include/net/bluetooth/bluetooth.h:28,
                    from drivers/bluetooth/btintel.c:14:
   arch/ia64/include/asm/mmu_context.h: In function 'reload_context':
   arch/ia64/include/asm/mmu_context.h:127:48: warning: variable 'old_rr4' set but not used [-Wunused-but-set-variable]
     127 |         unsigned long rr0, rr1, rr2, rr3, rr4, old_rr4;
         |                                                ^~~~~~~
   In file included from drivers/bluetooth/btintel.c:14:
   drivers/bluetooth/btintel.c: In function 'btintel_pull_quality_report_data':
>> drivers/bluetooth/btintel.c:2680:24: warning: format '%u' expects argument of type 'unsigned int', but argument 3 has type 'long unsigned int' [-Wformat=]
    2680 |                 BT_ERR("Telemetry event length %d too short (at least %u)",
         |                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    2681 |                        skb->len, sizeof(struct intel_tlv));
         |                                  ~~~~~~~~~~~~~~~~~~~~~~~~
         |                                  |
         |                                  long unsigned int
   include/net/bluetooth/bluetooth.h:199:40: note: in definition of macro 'BT_ERR'
     199 | #define BT_ERR(fmt, ...)        bt_err(fmt "\n", ##__VA_ARGS__)
         |                                        ^~~
   drivers/bluetooth/btintel.c:2680:72: note: format string is defined here
    2680 |                 BT_ERR("Telemetry event length %d too short (at least %u)",
         |                                                                       ~^
         |                                                                        |
         |                                                                        unsigned int
         |                                                                       %lu


vim +2680 drivers/bluetooth/btintel.c

  2673	
  2674	bool btintel_pull_quality_report_data(struct sk_buff *skb)
  2675	{
  2676		skb_pull(skb, sizeof(struct intel_prefix_evt_data));
  2677	
  2678		/* A telemetry event contains at least one intel_tlv subevent. */
  2679		if (skb->len < sizeof(struct intel_tlv)) {
> 2680			BT_ERR("Telemetry event length %d too short (at least %u)",
  2681			       skb->len, sizeof(struct intel_tlv));
  2682			return false;
  2683		}
  2684	
  2685		return true;
  2686	}
  2687	EXPORT_SYMBOL_GPL(btintel_pull_quality_report_data);
  2688	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Marcel Holtmann Jan. 21, 2022, 8:21 p.m. UTC | #2 
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>
> ---
> 
> drivers/bluetooth/btintel.c      | 43 +++++++++++++++++++++++++++++++-
> drivers/bluetooth/btintel.h      | 12 +++++++++
> include/net/bluetooth/hci_core.h |  2 ++
> net/bluetooth/hci_event.c        | 12 ++++++---
> 4 files changed, 65 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index 1a4f8b227eac..d3b7a796cb91 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -2401,8 +2401,10 @@ 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;
> +	hdev->is_quality_report_evt = btintel_is_quality_report_evt;
> +	hdev->pull_quality_report_data = btintel_pull_quality_report_data;

we are not doing this. This is all internal handling. Don’t bother the core hci_dev with it.

> 
> 	/* For Legacy device, check the HW platform value and size */
> 	if (skb->len == sizeof(ver) && skb->data[1] == 0x37) {
> @@ -2645,6 +2647,45 @@ 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[0];   /* a number of struct intel_tlv subevents */
> +} __packed;
> +
> +bool btintel_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;
> +}
> +EXPORT_SYMBOL_GPL(btintel_is_quality_report_evt);
> +
> +bool btintel_pull_quality_report_data(struct sk_buff *skb)
> +{
> +	skb_pull(skb, sizeof(struct intel_prefix_evt_data));
> +
> +	/* A telemetry event contains at least one intel_tlv subevent. */
> +	if (skb->len < sizeof(struct intel_tlv)) {
> +		BT_ERR("Telemetry event length %d too short (at least %u)",
> +		       skb->len, sizeof(struct intel_tlv));
> +		return false;
> +	}
> +
> +	return true;
> +}
> +EXPORT_SYMBOL_GPL(btintel_pull_quality_report_data);
> +
> 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 c9b24e9299e2..841aef3dbd4c 100644
> --- a/drivers/bluetooth/btintel.h
> +++ b/drivers/bluetooth/btintel.h
> @@ -210,6 +210,8 @@ 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);
> +bool btintel_is_quality_report_evt(struct sk_buff *skb);
> +bool btintel_pull_quality_report_data(struct sk_buff *skb);
> #else
> 
> static inline int btintel_check_bdaddr(struct hci_dev *hdev)
> @@ -305,4 +307,14 @@ static inline int btintel_set_quality_report(struct hci_dev *hdev, bool enable)
> {
> 	return -ENODEV;
> }
> +
> +static inline bool btintel_is_quality_report_evt(struct sk_buff *skb)
> +{
> +	return false;
> +}
> +
> +static inline bool btintel_pull_quality_report_data(struct sk_buff *skb);
> +{
> +	return false;
> +}
> #endif
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 727cb9c056b2..b74ba1585df9 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -632,6 +632,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);
> +	bool (*is_quality_report_evt)(struct sk_buff *skb);
> +	bool (*pull_quality_report_data)(struct sk_buff *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 bccb659a9454..5f9cc7b942a1 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -4237,11 +4237,17 @@ static bool quality_report_evt(struct hci_dev *hdev,  void *data,
> 		if (aosp_has_quality_report(hdev) &&
> 		    aosp_pull_quality_report_data(skb))
> 			mgmt_quality_report(hdev, skb, QUALITY_SPEC_AOSP_BQR);
> -
> -		return true;
> +	} else if (hdev->is_quality_report_evt &&
> +		   hdev->is_quality_report_evt(skb)) {
> +		if (hdev->set_quality_report &&
> +		    hdev->pull_quality_report_data(skb))
> +			mgmt_quality_report(hdev, skb,
> +					    QUALITY_SPEC_INTEL_TELEMETRY);
> +	} else {
> +		return false;
> 	}

No. You now have Intel internal details bleeding into the core.

Regards

Marcel
kernel test robot Jan. 22, 2022, 7:31 a.m. UTC | #3 
Hi Joseph,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bluetooth-next/master]
[also build test ERROR on next-20220121]
[cannot apply to net-next/master net/master bluetooth/master v5.16]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Joseph-Hwang/Bluetooth-aosp-surface-AOSP-quality-report-through-mgmt/20220121-192436
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20220122/202201221555.iq4k2aDR-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/9484eb7f87f7c8759b6fd7eec9d431c375b97432
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Joseph-Hwang/Bluetooth-aosp-surface-AOSP-quality-report-through-mgmt/20220121-192436
        git checkout 9484eb7f87f7c8759b6fd7eec9d431c375b97432
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/bluetooth/hci_ldisc.c:34:
>> drivers/bluetooth/btintel.h:317:1: error: expected identifier or '(' before '{' token
     317 | {
         | ^
   drivers/bluetooth/btintel.h:316:20: warning: 'btintel_pull_quality_report_data' declared 'static' but never defined [-Wunused-function]
     316 | static inline bool btintel_pull_quality_report_data(struct sk_buff *skb);
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +317 drivers/bluetooth/btintel.h

   315	
   316	static inline bool btintel_pull_quality_report_data(struct sk_buff *skb);
 > 317	{

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
index 1a4f8b227eac..d3b7a796cb91 100644
--- a/drivers/bluetooth/btintel.c
+++ b/drivers/bluetooth/btintel.c
@@ -2401,8 +2401,10 @@  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;
+	hdev->is_quality_report_evt = btintel_is_quality_report_evt;
+	hdev->pull_quality_report_data = btintel_pull_quality_report_data;
 
 	/* For Legacy device, check the HW platform value and size */
 	if (skb->len == sizeof(ver) && skb->data[1] == 0x37) {
@@ -2645,6 +2647,45 @@  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[0];   /* a number of struct intel_tlv subevents */
+} __packed;
+
+bool btintel_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;
+}
+EXPORT_SYMBOL_GPL(btintel_is_quality_report_evt);
+
+bool btintel_pull_quality_report_data(struct sk_buff *skb)
+{
+	skb_pull(skb, sizeof(struct intel_prefix_evt_data));
+
+	/* A telemetry event contains at least one intel_tlv subevent. */
+	if (skb->len < sizeof(struct intel_tlv)) {
+		BT_ERR("Telemetry event length %d too short (at least %u)",
+		       skb->len, sizeof(struct intel_tlv));
+		return false;
+	}
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(btintel_pull_quality_report_data);
+
 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 c9b24e9299e2..841aef3dbd4c 100644
--- a/drivers/bluetooth/btintel.h
+++ b/drivers/bluetooth/btintel.h
@@ -210,6 +210,8 @@  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);
+bool btintel_is_quality_report_evt(struct sk_buff *skb);
+bool btintel_pull_quality_report_data(struct sk_buff *skb);
 #else
 
 static inline int btintel_check_bdaddr(struct hci_dev *hdev)
@@ -305,4 +307,14 @@  static inline int btintel_set_quality_report(struct hci_dev *hdev, bool enable)
 {
 	return -ENODEV;
 }
+
+static inline bool btintel_is_quality_report_evt(struct sk_buff *skb)
+{
+	return false;
+}
+
+static inline bool btintel_pull_quality_report_data(struct sk_buff *skb);
+{
+	return false;
+}
 #endif
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 727cb9c056b2..b74ba1585df9 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -632,6 +632,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);
+	bool (*is_quality_report_evt)(struct sk_buff *skb);
+	bool (*pull_quality_report_data)(struct sk_buff *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 bccb659a9454..5f9cc7b942a1 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -4237,11 +4237,17 @@  static bool quality_report_evt(struct hci_dev *hdev,  void *data,
 		if (aosp_has_quality_report(hdev) &&
 		    aosp_pull_quality_report_data(skb))
 			mgmt_quality_report(hdev, skb, QUALITY_SPEC_AOSP_BQR);
-
-		return true;
+	} else if (hdev->is_quality_report_evt &&
+		   hdev->is_quality_report_evt(skb)) {
+		if (hdev->set_quality_report &&
+		    hdev->pull_quality_report_data(skb))
+			mgmt_quality_report(hdev, skb,
+					    QUALITY_SPEC_INTEL_TELEMETRY);
+	} else {
+		return false;
 	}
 
-	return false;
+	return true;
 }
 
 static void hci_vendor_evt(struct hci_dev *hdev, void *data,