* [PATCH v4 2/3] Bluetooth: btintel: surface Intel telemetry events through mgmt
2022-02-15 13:35 [PATCH v4 1/3] Bluetooth: aosp: surface AOSP quality report through mgmt Joseph Hwang
@ 2022-02-15 13:35 ` Joseph Hwang
2022-02-17 12:53 ` Marcel Holtmann
2022-02-15 13:35 ` [PATCH v4 3/3] Bluetooth: mgmt: add set_quality_report for MGMT_OP_SET_QUALITY_REPORT Joseph Hwang
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Joseph Hwang @ 2022-02-15 13:35 UTC (permalink / raw)
To: linux-bluetooth, marcel, luiz.dentz, pali
Cc: chromeos-bluetooth-upstreaming, josephsih, Joseph Hwang,
Archie Pusaka, David S. Miller, Jakub Kicinski, Johan Hedberg,
linux-kernel, netdev
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(-)
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);
+}
+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,
+ 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);
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 };
/* 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),
--
2.35.1.265.g69c8d7142f-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/3] Bluetooth: btintel: surface Intel telemetry events through mgmt
2022-02-15 13:35 ` [PATCH v4 2/3] Bluetooth: btintel: surface Intel telemetry events " Joseph Hwang
@ 2022-02-17 12:53 ` Marcel Holtmann
2022-03-05 7:46 ` Joseph Hwang
0 siblings, 1 reply; 15+ messages in thread
From: Marcel Holtmann @ 2022-02-17 12:53 UTC (permalink / raw)
To: Joseph Hwang
Cc: BlueZ, Luiz Augusto von Dentz, Pali Rohár,
CrosBT Upstreaming, Joseph Hwang, Archie Pusaka, David S. Miller,
Jakub Kicinski, Johan Hedberg, LKML, netdev
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 2/3] Bluetooth: btintel: surface Intel telemetry events through mgmt
2022-02-17 12:53 ` Marcel Holtmann
@ 2022-03-05 7:46 ` Joseph Hwang
0 siblings, 0 replies; 15+ messages in thread
From: Joseph Hwang @ 2022-03-05 7:46 UTC (permalink / raw)
To: Marcel Holtmann
Cc: BlueZ, Luiz Augusto von Dentz, Pali Rohár,
CrosBT Upstreaming, Archie Pusaka, David S. Miller,
Jakub Kicinski, Johan Hedberg, LKML, netdev
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@holtmann.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@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.
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@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.
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@google.com
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4 3/3] Bluetooth: mgmt: add set_quality_report for MGMT_OP_SET_QUALITY_REPORT
2022-02-15 13:35 [PATCH v4 1/3] Bluetooth: aosp: surface AOSP quality report through mgmt Joseph Hwang
2022-02-15 13:35 ` [PATCH v4 2/3] Bluetooth: btintel: surface Intel telemetry events " Joseph Hwang
@ 2022-02-15 13:35 ` Joseph Hwang
2022-02-17 13:04 ` Marcel Holtmann
2022-02-15 14:08 ` [v4,1/3] Bluetooth: aosp: surface AOSP quality report through mgmt bluez.test.bot
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Joseph Hwang @ 2022-02-15 13:35 UTC (permalink / raw)
To: linux-bluetooth, marcel, luiz.dentz, pali
Cc: chromeos-bluetooth-upstreaming, josephsih, Joseph Hwang,
David S. Miller, Jakub Kicinski, Johan Hedberg, linux-kernel,
netdev
This patch adds a new set_quality_report mgmt handler to set
the quality report feature. The feature is removed from the
experimental features at the same time.
Signed-off-by: Joseph Hwang <josephsih@chromium.org>
---
Changes in v4:
- return current settings for set_quality_report.
Changes in v3:
- This is a new patch to enable the quality report feature.
The reading and setting of the quality report feature are
removed from the experimental features.
include/net/bluetooth/mgmt.h | 7 ++
net/bluetooth/mgmt.c | 168 +++++++++++++++--------------------
2 files changed, 81 insertions(+), 94 deletions(-)
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 83b602636262..74d253ff617a 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -109,6 +109,7 @@ struct mgmt_rp_read_index_list {
#define MGMT_SETTING_STATIC_ADDRESS 0x00008000
#define MGMT_SETTING_PHY_CONFIGURATION 0x00010000
#define MGMT_SETTING_WIDEBAND_SPEECH 0x00020000
+#define MGMT_SETTING_QUALITY_REPORT 0x00040000
#define MGMT_OP_READ_INFO 0x0004
#define MGMT_READ_INFO_SIZE 0
@@ -838,6 +839,12 @@ struct mgmt_cp_add_adv_patterns_monitor_rssi {
} __packed;
#define MGMT_ADD_ADV_PATTERNS_MONITOR_RSSI_SIZE 8
+#define MGMT_OP_SET_QUALITY_REPORT 0x0057
+struct mgmt_cp_set_quality_report {
+ __u8 action;
+} __packed;
+#define MGMT_SET_QUALITY_REPORT_SIZE 1
+
#define MGMT_EV_CMD_COMPLETE 0x0001
struct mgmt_ev_cmd_complete {
__le16 opcode;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 5e48576041fb..ccd77b94905b 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -857,6 +857,10 @@ static u32 get_supported_settings(struct hci_dev *hdev)
settings |= MGMT_SETTING_PHY_CONFIGURATION;
+ if (hdev && (aosp_has_quality_report(hdev) ||
+ hdev->set_quality_report))
+ settings |= MGMT_SETTING_QUALITY_REPORT;
+
return settings;
}
@@ -928,6 +932,9 @@ static u32 get_current_settings(struct hci_dev *hdev)
if (hci_dev_test_flag(hdev, HCI_WIDEBAND_SPEECH_ENABLED))
settings |= MGMT_SETTING_WIDEBAND_SPEECH;
+ if (hci_dev_test_flag(hdev, HCI_QUALITY_REPORT))
+ settings |= MGMT_SETTING_QUALITY_REPORT;
+
return settings;
}
@@ -3871,12 +3878,6 @@ static const u8 debug_uuid[16] = {
};
#endif
-/* 330859bc-7506-492d-9370-9a6f0614037f */
-static const u8 quality_report_uuid[16] = {
- 0x7f, 0x03, 0x14, 0x06, 0x6f, 0x9a, 0x70, 0x93,
- 0x2d, 0x49, 0x06, 0x75, 0xbc, 0x59, 0x08, 0x33,
-};
-
/* a6695ace-ee7f-4fb9-881a-5fac66c629af */
static const u8 offload_codecs_uuid[16] = {
0xaf, 0x29, 0xc6, 0x66, 0xac, 0x5f, 0x1a, 0x88,
@@ -3898,7 +3899,7 @@ static const u8 rpa_resolution_uuid[16] = {
static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
void *data, u16 data_len)
{
- char buf[102]; /* Enough space for 5 features: 2 + 20 * 5 */
+ char buf[82]; /* Enough space for 4 features: 2 + 20 * 4 */
struct mgmt_rp_read_exp_features_info *rp = (void *)buf;
u16 idx = 0;
u32 flags;
@@ -3939,18 +3940,6 @@ static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
idx++;
}
- if (hdev && (aosp_has_quality_report(hdev) ||
- hdev->set_quality_report)) {
- if (hci_dev_test_flag(hdev, HCI_QUALITY_REPORT))
- flags = BIT(0);
- else
- flags = 0;
-
- memcpy(rp->features[idx].uuid, quality_report_uuid, 16);
- rp->features[idx].flags = cpu_to_le32(flags);
- idx++;
- }
-
if (hdev && hdev->get_data_path_id) {
if (hci_dev_test_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED))
flags = BIT(0);
@@ -4163,80 +4152,6 @@ static int set_rpa_resolution_func(struct sock *sk, struct hci_dev *hdev,
return err;
}
-static int set_quality_report_func(struct sock *sk, struct hci_dev *hdev,
- struct mgmt_cp_set_exp_feature *cp,
- u16 data_len)
-{
- struct mgmt_rp_set_exp_feature rp;
- bool val, changed;
- int err;
-
- /* Command requires to use a valid controller index */
- if (!hdev)
- return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
- MGMT_OP_SET_EXP_FEATURE,
- MGMT_STATUS_INVALID_INDEX);
-
- /* Parameters are limited to a single octet */
- if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1)
- return mgmt_cmd_status(sk, hdev->id,
- MGMT_OP_SET_EXP_FEATURE,
- MGMT_STATUS_INVALID_PARAMS);
-
- /* Only boolean on/off is supported */
- if (cp->param[0] != 0x00 && cp->param[0] != 0x01)
- return mgmt_cmd_status(sk, hdev->id,
- MGMT_OP_SET_EXP_FEATURE,
- MGMT_STATUS_INVALID_PARAMS);
-
- hci_req_sync_lock(hdev);
-
- val = !!cp->param[0];
- changed = (val != hci_dev_test_flag(hdev, HCI_QUALITY_REPORT));
-
- if (!aosp_has_quality_report(hdev) && !hdev->set_quality_report) {
- err = mgmt_cmd_status(sk, hdev->id,
- MGMT_OP_SET_EXP_FEATURE,
- MGMT_STATUS_NOT_SUPPORTED);
- goto unlock_quality_report;
- }
-
- if (changed) {
- if (hdev->set_quality_report)
- err = hdev->set_quality_report(hdev, val);
- else
- err = aosp_set_quality_report(hdev, val);
-
- if (err) {
- err = mgmt_cmd_status(sk, hdev->id,
- MGMT_OP_SET_EXP_FEATURE,
- MGMT_STATUS_FAILED);
- goto unlock_quality_report;
- }
-
- if (val)
- hci_dev_set_flag(hdev, HCI_QUALITY_REPORT);
- else
- hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT);
- }
-
- bt_dev_dbg(hdev, "quality report enable %d changed %d", val, changed);
-
- memcpy(rp.uuid, quality_report_uuid, 16);
- rp.flags = cpu_to_le32(val ? BIT(0) : 0);
- hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
-
- err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_SET_EXP_FEATURE, 0,
- &rp, sizeof(rp));
-
- if (changed)
- exp_feature_changed(hdev, quality_report_uuid, val, sk);
-
-unlock_quality_report:
- hci_req_sync_unlock(hdev);
- return err;
-}
-
static int set_offload_codec_func(struct sock *sk, struct hci_dev *hdev,
struct mgmt_cp_set_exp_feature *cp,
u16 data_len)
@@ -4363,7 +4278,6 @@ static const struct mgmt_exp_feature {
EXP_FEAT(debug_uuid, set_debug_func),
#endif
EXP_FEAT(rpa_resolution_uuid, set_rpa_resolution_func),
- EXP_FEAT(quality_report_uuid, set_quality_report_func),
EXP_FEAT(offload_codecs_uuid, set_offload_codec_func),
EXP_FEAT(le_simultaneous_roles_uuid, set_le_simultaneous_roles_func),
@@ -8653,6 +8567,71 @@ static int get_adv_size_info(struct sock *sk, struct hci_dev *hdev,
MGMT_STATUS_SUCCESS, &rp, sizeof(rp));
}
+static int set_quality_report(struct sock *sk, struct hci_dev *hdev,
+ void *data, u16 data_len)
+{
+ struct mgmt_cp_set_quality_report *cp = data;
+ bool enable, changed;
+ int err;
+
+ /* Command requires to use a valid controller index */
+ if (!hdev)
+ return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
+ MGMT_OP_SET_QUALITY_REPORT,
+ MGMT_STATUS_INVALID_INDEX);
+
+ /* Only 0 (off) and 1 (on) is supported */
+ if (cp->action != 0x00 && cp->action != 0x01)
+ return mgmt_cmd_status(sk, hdev->id,
+ MGMT_OP_SET_QUALITY_REPORT,
+ MGMT_STATUS_INVALID_PARAMS);
+
+ hci_req_sync_lock(hdev);
+
+ enable = !!cp->action;
+ changed = (enable != hci_dev_test_flag(hdev, HCI_QUALITY_REPORT));
+
+ if (!aosp_has_quality_report(hdev) && !hdev->set_quality_report) {
+ err = mgmt_cmd_status(sk, hdev->id,
+ MGMT_OP_SET_QUALITY_REPORT,
+ MGMT_STATUS_NOT_SUPPORTED);
+ goto unlock_quality_report;
+ }
+
+ if (changed) {
+ if (hdev->set_quality_report)
+ err = hdev->set_quality_report(hdev, enable);
+ else
+ err = aosp_set_quality_report(hdev, enable);
+
+ if (err) {
+ err = mgmt_cmd_status(sk, hdev->id,
+ MGMT_OP_SET_QUALITY_REPORT,
+ MGMT_STATUS_FAILED);
+ goto unlock_quality_report;
+ }
+
+ if (enable)
+ hci_dev_set_flag(hdev, HCI_QUALITY_REPORT);
+ else
+ hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT);
+ }
+
+ bt_dev_dbg(hdev, "quality report enable %d changed %d",
+ enable, changed);
+
+ err = send_settings_rsp(sk, MGMT_OP_SET_QUALITY_REPORT, hdev);
+ if (err < 0)
+ goto unlock_quality_report;
+
+ if (changed)
+ err = new_settings(hdev, sk);
+
+unlock_quality_report:
+ hci_req_sync_unlock(hdev);
+ return err;
+}
+
static const struct hci_mgmt_handler mgmt_handlers[] = {
{ NULL }, /* 0x0000 (no command) */
{ read_version, MGMT_READ_VERSION_SIZE,
@@ -8779,6 +8758,7 @@ static const struct hci_mgmt_handler mgmt_handlers[] = {
{ add_adv_patterns_monitor_rssi,
MGMT_ADD_ADV_PATTERNS_MONITOR_RSSI_SIZE,
HCI_MGMT_VAR_LEN },
+ { set_quality_report, MGMT_SET_QUALITY_REPORT_SIZE },
};
void mgmt_index_added(struct hci_dev *hdev)
--
2.35.1.265.g69c8d7142f-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/3] Bluetooth: mgmt: add set_quality_report for MGMT_OP_SET_QUALITY_REPORT
2022-02-15 13:35 ` [PATCH v4 3/3] Bluetooth: mgmt: add set_quality_report for MGMT_OP_SET_QUALITY_REPORT Joseph Hwang
@ 2022-02-17 13:04 ` Marcel Holtmann
2022-03-05 7:51 ` Joseph Hwang
0 siblings, 1 reply; 15+ messages in thread
From: Marcel Holtmann @ 2022-02-17 13:04 UTC (permalink / raw)
To: Joseph Hwang
Cc: BlueZ, Luiz Augusto von Dentz, pali,
chromeos-bluetooth-upstreaming, josephsih, David S. Miller,
Jakub Kicinski, Johan Hedberg, linux-kernel, netdev
Hi Joseph,
> This patch adds a new set_quality_report mgmt handler to set
> the quality report feature. The feature is removed from the
> experimental features at the same time.
>
> Signed-off-by: Joseph Hwang <josephsih@chromium.org>
> ---
>
> Changes in v4:
> - return current settings for set_quality_report.
>
> Changes in v3:
> - This is a new patch to enable the quality report feature.
> The reading and setting of the quality report feature are
> removed from the experimental features.
>
> include/net/bluetooth/mgmt.h | 7 ++
> net/bluetooth/mgmt.c | 168 +++++++++++++++--------------------
> 2 files changed, 81 insertions(+), 94 deletions(-)
>
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 83b602636262..74d253ff617a 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -109,6 +109,7 @@ struct mgmt_rp_read_index_list {
> #define MGMT_SETTING_STATIC_ADDRESS 0x00008000
> #define MGMT_SETTING_PHY_CONFIGURATION 0x00010000
> #define MGMT_SETTING_WIDEBAND_SPEECH 0x00020000
> +#define MGMT_SETTING_QUALITY_REPORT 0x00040000
>
> #define MGMT_OP_READ_INFO 0x0004
> #define MGMT_READ_INFO_SIZE 0
> @@ -838,6 +839,12 @@ struct mgmt_cp_add_adv_patterns_monitor_rssi {
> } __packed;
> #define MGMT_ADD_ADV_PATTERNS_MONITOR_RSSI_SIZE 8
>
> +#define MGMT_OP_SET_QUALITY_REPORT 0x0057
> +struct mgmt_cp_set_quality_report {
> + __u8 action;
> +} __packed;
> +#define MGMT_SET_QUALITY_REPORT_SIZE 1
> +
> #define MGMT_EV_CMD_COMPLETE 0x0001
> struct mgmt_ev_cmd_complete {
> __le16 opcode;
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 5e48576041fb..ccd77b94905b 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -857,6 +857,10 @@ static u32 get_supported_settings(struct hci_dev *hdev)
>
> settings |= MGMT_SETTING_PHY_CONFIGURATION;
>
> + if (hdev && (aosp_has_quality_report(hdev) ||
> + hdev->set_quality_report))
> + settings |= MGMT_SETTING_QUALITY_REPORT;
> +
the hdev check here is useless. The hdev structure is always valid.
> return settings;
> }
>
> @@ -928,6 +932,9 @@ static u32 get_current_settings(struct hci_dev *hdev)
> if (hci_dev_test_flag(hdev, HCI_WIDEBAND_SPEECH_ENABLED))
> settings |= MGMT_SETTING_WIDEBAND_SPEECH;
>
> + if (hci_dev_test_flag(hdev, HCI_QUALITY_REPORT))
> + settings |= MGMT_SETTING_QUALITY_REPORT;
> +
> return settings;
> }
>
> @@ -3871,12 +3878,6 @@ static const u8 debug_uuid[16] = {
> };
> #endif
>
> -/* 330859bc-7506-492d-9370-9a6f0614037f */
> -static const u8 quality_report_uuid[16] = {
> - 0x7f, 0x03, 0x14, 0x06, 0x6f, 0x9a, 0x70, 0x93,
> - 0x2d, 0x49, 0x06, 0x75, 0xbc, 0x59, 0x08, 0x33,
> -};
> -
> /* a6695ace-ee7f-4fb9-881a-5fac66c629af */
> static const u8 offload_codecs_uuid[16] = {
> 0xaf, 0x29, 0xc6, 0x66, 0xac, 0x5f, 0x1a, 0x88,
> @@ -3898,7 +3899,7 @@ static const u8 rpa_resolution_uuid[16] = {
> static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
> void *data, u16 data_len)
> {
> - char buf[102]; /* Enough space for 5 features: 2 + 20 * 5 */
> + char buf[82]; /* Enough space for 4 features: 2 + 20 * 4 */
> struct mgmt_rp_read_exp_features_info *rp = (void *)buf;
> u16 idx = 0;
> u32 flags;
> @@ -3939,18 +3940,6 @@ static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
> idx++;
> }
>
> - if (hdev && (aosp_has_quality_report(hdev) ||
> - hdev->set_quality_report)) {
> - if (hci_dev_test_flag(hdev, HCI_QUALITY_REPORT))
> - flags = BIT(0);
> - else
> - flags = 0;
> -
> - memcpy(rp->features[idx].uuid, quality_report_uuid, 16);
> - rp->features[idx].flags = cpu_to_le32(flags);
> - idx++;
> - }
> -
(I see, you copied it from here. Yes, here you need to check for hdev!)
> if (hdev && hdev->get_data_path_id) {
> if (hci_dev_test_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED))
> flags = BIT(0);
> @@ -4163,80 +4152,6 @@ static int set_rpa_resolution_func(struct sock *sk, struct hci_dev *hdev,
> return err;
> }
>
> -static int set_quality_report_func(struct sock *sk, struct hci_dev *hdev,
> - struct mgmt_cp_set_exp_feature *cp,
> - u16 data_len)
> -{
> - struct mgmt_rp_set_exp_feature rp;
> - bool val, changed;
> - int err;
> -
> - /* Command requires to use a valid controller index */
> - if (!hdev)
> - return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
> - MGMT_OP_SET_EXP_FEATURE,
> - MGMT_STATUS_INVALID_INDEX);
> -
> - /* Parameters are limited to a single octet */
> - if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1)
> - return mgmt_cmd_status(sk, hdev->id,
> - MGMT_OP_SET_EXP_FEATURE,
> - MGMT_STATUS_INVALID_PARAMS);
> -
> - /* Only boolean on/off is supported */
> - if (cp->param[0] != 0x00 && cp->param[0] != 0x01)
> - return mgmt_cmd_status(sk, hdev->id,
> - MGMT_OP_SET_EXP_FEATURE,
> - MGMT_STATUS_INVALID_PARAMS);
> -
> - hci_req_sync_lock(hdev);
> -
> - val = !!cp->param[0];
> - changed = (val != hci_dev_test_flag(hdev, HCI_QUALITY_REPORT));
> -
> - if (!aosp_has_quality_report(hdev) && !hdev->set_quality_report) {
> - err = mgmt_cmd_status(sk, hdev->id,
> - MGMT_OP_SET_EXP_FEATURE,
> - MGMT_STATUS_NOT_SUPPORTED);
> - goto unlock_quality_report;
> - }
> -
> - if (changed) {
> - if (hdev->set_quality_report)
> - err = hdev->set_quality_report(hdev, val);
> - else
> - err = aosp_set_quality_report(hdev, val);
> -
> - if (err) {
> - err = mgmt_cmd_status(sk, hdev->id,
> - MGMT_OP_SET_EXP_FEATURE,
> - MGMT_STATUS_FAILED);
> - goto unlock_quality_report;
> - }
> -
> - if (val)
> - hci_dev_set_flag(hdev, HCI_QUALITY_REPORT);
> - else
> - hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT);
> - }
> -
> - bt_dev_dbg(hdev, "quality report enable %d changed %d", val, changed);
> -
> - memcpy(rp.uuid, quality_report_uuid, 16);
> - rp.flags = cpu_to_le32(val ? BIT(0) : 0);
> - hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
> -
> - err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_SET_EXP_FEATURE, 0,
> - &rp, sizeof(rp));
> -
> - if (changed)
> - exp_feature_changed(hdev, quality_report_uuid, val, sk);
> -
> -unlock_quality_report:
> - hci_req_sync_unlock(hdev);
> - return err;
> -}
> -
> static int set_offload_codec_func(struct sock *sk, struct hci_dev *hdev,
> struct mgmt_cp_set_exp_feature *cp,
> u16 data_len)
> @@ -4363,7 +4278,6 @@ static const struct mgmt_exp_feature {
> EXP_FEAT(debug_uuid, set_debug_func),
> #endif
> EXP_FEAT(rpa_resolution_uuid, set_rpa_resolution_func),
> - EXP_FEAT(quality_report_uuid, set_quality_report_func),
> EXP_FEAT(offload_codecs_uuid, set_offload_codec_func),
> EXP_FEAT(le_simultaneous_roles_uuid, set_le_simultaneous_roles_func),
>
> @@ -8653,6 +8567,71 @@ static int get_adv_size_info(struct sock *sk, struct hci_dev *hdev,
> MGMT_STATUS_SUCCESS, &rp, sizeof(rp));
> }
>
> +static int set_quality_report(struct sock *sk, struct hci_dev *hdev,
> + void *data, u16 data_len)
> +{
> + struct mgmt_cp_set_quality_report *cp = data;
> + bool enable, changed;
> + int err;
> +
> + /* Command requires to use a valid controller index */
> + if (!hdev)
> + return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
> + MGMT_OP_SET_QUALITY_REPORT,
> + MGMT_STATUS_INVALID_INDEX);
> +
> + /* Only 0 (off) and 1 (on) is supported */
> + if (cp->action != 0x00 && cp->action != 0x01)
> + return mgmt_cmd_status(sk, hdev->id,
> + MGMT_OP_SET_QUALITY_REPORT,
> + MGMT_STATUS_INVALID_PARAMS);
> +
> + hci_req_sync_lock(hdev);
> +
> + enable = !!cp->action;
> + changed = (enable != hci_dev_test_flag(hdev, HCI_QUALITY_REPORT));
> +
> + if (!aosp_has_quality_report(hdev) && !hdev->set_quality_report) {
> + err = mgmt_cmd_status(sk, hdev->id,
> + MGMT_OP_SET_QUALITY_REPORT,
> + MGMT_STATUS_NOT_SUPPORTED);
> + goto unlock_quality_report;
> + }
> +
> + if (changed) {
> + if (hdev->set_quality_report)
> + err = hdev->set_quality_report(hdev, enable);
> + else
> + err = aosp_set_quality_report(hdev, enable);
> +
> + if (err) {
> + err = mgmt_cmd_status(sk, hdev->id,
> + MGMT_OP_SET_QUALITY_REPORT,
> + MGMT_STATUS_FAILED);
> + goto unlock_quality_report;
> + }
> +
> + if (enable)
> + hci_dev_set_flag(hdev, HCI_QUALITY_REPORT);
> + else
> + hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT);
> + }
> +
> + bt_dev_dbg(hdev, "quality report enable %d changed %d",
> + enable, changed);
> +
> + err = send_settings_rsp(sk, MGMT_OP_SET_QUALITY_REPORT, hdev);
> + if (err < 0)
> + goto unlock_quality_report;
> +
> + if (changed)
> + err = new_settings(hdev, sk);
> +
> +unlock_quality_report:
> + hci_req_sync_unlock(hdev);
> + return err;
> +}
> +
> static const struct hci_mgmt_handler mgmt_handlers[] = {
> { NULL }, /* 0x0000 (no command) */
> { read_version, MGMT_READ_VERSION_SIZE,
> @@ -8779,6 +8758,7 @@ static const struct hci_mgmt_handler mgmt_handlers[] = {
> { add_adv_patterns_monitor_rssi,
> MGMT_ADD_ADV_PATTERNS_MONITOR_RSSI_SIZE,
> HCI_MGMT_VAR_LEN },
> + { set_quality_report, MGMT_SET_QUALITY_REPORT_SIZE },
> };
>
> void mgmt_index_added(struct hci_dev *hdev)
So this patch I actually get merged first.
However we still need to figure out if this setting is suppose to survive power off/on cycles. Right now as far as I can tell it is added to hci_dev_clear_volatile_flags and thus needs to be set again after power on.
Is this the expected behavior? I don’t think we want that. Since normally all other settings are restored after power on. And it is explicitly mentioned in doc/mgmt-api.txt as well.
Regards
Marcel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 3/3] Bluetooth: mgmt: add set_quality_report for MGMT_OP_SET_QUALITY_REPORT
2022-02-17 13:04 ` Marcel Holtmann
@ 2022-03-05 7:51 ` Joseph Hwang
0 siblings, 0 replies; 15+ messages in thread
From: Joseph Hwang @ 2022-03-05 7:51 UTC (permalink / raw)
To: Marcel Holtmann
Cc: BlueZ, Luiz Augusto von Dentz, pali,
chromeos-bluetooth-upstreaming, David S. Miller, Jakub Kicinski,
Johan Hedberg, linux-kernel, netdev
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 9:04 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Joseph,
>
> > This patch adds a new set_quality_report mgmt handler to set
> > the quality report feature. The feature is removed from the
> > experimental features at the same time.
> >
> > Signed-off-by: Joseph Hwang <josephsih@chromium.org>
> > ---
> >
> > Changes in v4:
> > - return current settings for set_quality_report.
> >
> > Changes in v3:
> > - This is a new patch to enable the quality report feature.
> > The reading and setting of the quality report feature are
> > removed from the experimental features.
> >
> > include/net/bluetooth/mgmt.h | 7 ++
> > net/bluetooth/mgmt.c | 168 +++++++++++++++--------------------
> > 2 files changed, 81 insertions(+), 94 deletions(-)
> >
> > diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> > index 83b602636262..74d253ff617a 100644
> > --- a/include/net/bluetooth/mgmt.h
> > +++ b/include/net/bluetooth/mgmt.h
> > @@ -109,6 +109,7 @@ struct mgmt_rp_read_index_list {
> > #define MGMT_SETTING_STATIC_ADDRESS 0x00008000
> > #define MGMT_SETTING_PHY_CONFIGURATION 0x00010000
> > #define MGMT_SETTING_WIDEBAND_SPEECH 0x00020000
> > +#define MGMT_SETTING_QUALITY_REPORT 0x00040000
> >
> > #define MGMT_OP_READ_INFO 0x0004
> > #define MGMT_READ_INFO_SIZE 0
> > @@ -838,6 +839,12 @@ struct mgmt_cp_add_adv_patterns_monitor_rssi {
> > } __packed;
> > #define MGMT_ADD_ADV_PATTERNS_MONITOR_RSSI_SIZE 8
> >
> > +#define MGMT_OP_SET_QUALITY_REPORT 0x0057
> > +struct mgmt_cp_set_quality_report {
> > + __u8 action;
> > +} __packed;
> > +#define MGMT_SET_QUALITY_REPORT_SIZE 1
> > +
> > #define MGMT_EV_CMD_COMPLETE 0x0001
> > struct mgmt_ev_cmd_complete {
> > __le16 opcode;
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index 5e48576041fb..ccd77b94905b 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -857,6 +857,10 @@ static u32 get_supported_settings(struct hci_dev *hdev)
> >
> > settings |= MGMT_SETTING_PHY_CONFIGURATION;
> >
> > + if (hdev && (aosp_has_quality_report(hdev) ||
> > + hdev->set_quality_report))
> > + settings |= MGMT_SETTING_QUALITY_REPORT;
> > +
>
> the hdev check here is useless. The hdev structure is always valid.
>
> > return settings;
> > }
> >
> > @@ -928,6 +932,9 @@ static u32 get_current_settings(struct hci_dev *hdev)
> > if (hci_dev_test_flag(hdev, HCI_WIDEBAND_SPEECH_ENABLED))
> > settings |= MGMT_SETTING_WIDEBAND_SPEECH;
> >
> > + if (hci_dev_test_flag(hdev, HCI_QUALITY_REPORT))
> > + settings |= MGMT_SETTING_QUALITY_REPORT;
> > +
> > return settings;
> > }
> >
> > @@ -3871,12 +3878,6 @@ static const u8 debug_uuid[16] = {
> > };
> > #endif
> >
> > -/* 330859bc-7506-492d-9370-9a6f0614037f */
> > -static const u8 quality_report_uuid[16] = {
> > - 0x7f, 0x03, 0x14, 0x06, 0x6f, 0x9a, 0x70, 0x93,
> > - 0x2d, 0x49, 0x06, 0x75, 0xbc, 0x59, 0x08, 0x33,
> > -};
> > -
> > /* a6695ace-ee7f-4fb9-881a-5fac66c629af */
> > static const u8 offload_codecs_uuid[16] = {
> > 0xaf, 0x29, 0xc6, 0x66, 0xac, 0x5f, 0x1a, 0x88,
> > @@ -3898,7 +3899,7 @@ static const u8 rpa_resolution_uuid[16] = {
> > static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
> > void *data, u16 data_len)
> > {
> > - char buf[102]; /* Enough space for 5 features: 2 + 20 * 5 */
> > + char buf[82]; /* Enough space for 4 features: 2 + 20 * 4 */
> > struct mgmt_rp_read_exp_features_info *rp = (void *)buf;
> > u16 idx = 0;
> > u32 flags;
> > @@ -3939,18 +3940,6 @@ static int read_exp_features_info(struct sock *sk, struct hci_dev *hdev,
> > idx++;
> > }
> >
> > - if (hdev && (aosp_has_quality_report(hdev) ||
> > - hdev->set_quality_report)) {
> > - if (hci_dev_test_flag(hdev, HCI_QUALITY_REPORT))
> > - flags = BIT(0);
> > - else
> > - flags = 0;
> > -
> > - memcpy(rp->features[idx].uuid, quality_report_uuid, 16);
> > - rp->features[idx].flags = cpu_to_le32(flags);
> > - idx++;
> > - }
> > -
>
> (I see, you copied it from here. Yes, here you need to check for hdev!)
>
> > if (hdev && hdev->get_data_path_id) {
> > if (hci_dev_test_flag(hdev, HCI_OFFLOAD_CODECS_ENABLED))
> > flags = BIT(0);
> > @@ -4163,80 +4152,6 @@ static int set_rpa_resolution_func(struct sock *sk, struct hci_dev *hdev,
> > return err;
> > }
> >
> > -static int set_quality_report_func(struct sock *sk, struct hci_dev *hdev,
> > - struct mgmt_cp_set_exp_feature *cp,
> > - u16 data_len)
> > -{
> > - struct mgmt_rp_set_exp_feature rp;
> > - bool val, changed;
> > - int err;
> > -
> > - /* Command requires to use a valid controller index */
> > - if (!hdev)
> > - return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
> > - MGMT_OP_SET_EXP_FEATURE,
> > - MGMT_STATUS_INVALID_INDEX);
> > -
> > - /* Parameters are limited to a single octet */
> > - if (data_len != MGMT_SET_EXP_FEATURE_SIZE + 1)
> > - return mgmt_cmd_status(sk, hdev->id,
> > - MGMT_OP_SET_EXP_FEATURE,
> > - MGMT_STATUS_INVALID_PARAMS);
> > -
> > - /* Only boolean on/off is supported */
> > - if (cp->param[0] != 0x00 && cp->param[0] != 0x01)
> > - return mgmt_cmd_status(sk, hdev->id,
> > - MGMT_OP_SET_EXP_FEATURE,
> > - MGMT_STATUS_INVALID_PARAMS);
> > -
> > - hci_req_sync_lock(hdev);
> > -
> > - val = !!cp->param[0];
> > - changed = (val != hci_dev_test_flag(hdev, HCI_QUALITY_REPORT));
> > -
> > - if (!aosp_has_quality_report(hdev) && !hdev->set_quality_report) {
> > - err = mgmt_cmd_status(sk, hdev->id,
> > - MGMT_OP_SET_EXP_FEATURE,
> > - MGMT_STATUS_NOT_SUPPORTED);
> > - goto unlock_quality_report;
> > - }
> > -
> > - if (changed) {
> > - if (hdev->set_quality_report)
> > - err = hdev->set_quality_report(hdev, val);
> > - else
> > - err = aosp_set_quality_report(hdev, val);
> > -
> > - if (err) {
> > - err = mgmt_cmd_status(sk, hdev->id,
> > - MGMT_OP_SET_EXP_FEATURE,
> > - MGMT_STATUS_FAILED);
> > - goto unlock_quality_report;
> > - }
> > -
> > - if (val)
> > - hci_dev_set_flag(hdev, HCI_QUALITY_REPORT);
> > - else
> > - hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT);
> > - }
> > -
> > - bt_dev_dbg(hdev, "quality report enable %d changed %d", val, changed);
> > -
> > - memcpy(rp.uuid, quality_report_uuid, 16);
> > - rp.flags = cpu_to_le32(val ? BIT(0) : 0);
> > - hci_sock_set_flag(sk, HCI_MGMT_EXP_FEATURE_EVENTS);
> > -
> > - err = mgmt_cmd_complete(sk, hdev->id, MGMT_OP_SET_EXP_FEATURE, 0,
> > - &rp, sizeof(rp));
> > -
> > - if (changed)
> > - exp_feature_changed(hdev, quality_report_uuid, val, sk);
> > -
> > -unlock_quality_report:
> > - hci_req_sync_unlock(hdev);
> > - return err;
> > -}
> > -
> > static int set_offload_codec_func(struct sock *sk, struct hci_dev *hdev,
> > struct mgmt_cp_set_exp_feature *cp,
> > u16 data_len)
> > @@ -4363,7 +4278,6 @@ static const struct mgmt_exp_feature {
> > EXP_FEAT(debug_uuid, set_debug_func),
> > #endif
> > EXP_FEAT(rpa_resolution_uuid, set_rpa_resolution_func),
> > - EXP_FEAT(quality_report_uuid, set_quality_report_func),
> > EXP_FEAT(offload_codecs_uuid, set_offload_codec_func),
> > EXP_FEAT(le_simultaneous_roles_uuid, set_le_simultaneous_roles_func),
> >
> > @@ -8653,6 +8567,71 @@ static int get_adv_size_info(struct sock *sk, struct hci_dev *hdev,
> > MGMT_STATUS_SUCCESS, &rp, sizeof(rp));
> > }
> >
> > +static int set_quality_report(struct sock *sk, struct hci_dev *hdev,
> > + void *data, u16 data_len)
> > +{
> > + struct mgmt_cp_set_quality_report *cp = data;
> > + bool enable, changed;
> > + int err;
> > +
> > + /* Command requires to use a valid controller index */
> > + if (!hdev)
> > + return mgmt_cmd_status(sk, MGMT_INDEX_NONE,
> > + MGMT_OP_SET_QUALITY_REPORT,
> > + MGMT_STATUS_INVALID_INDEX);
> > +
> > + /* Only 0 (off) and 1 (on) is supported */
> > + if (cp->action != 0x00 && cp->action != 0x01)
> > + return mgmt_cmd_status(sk, hdev->id,
> > + MGMT_OP_SET_QUALITY_REPORT,
> > + MGMT_STATUS_INVALID_PARAMS);
> > +
> > + hci_req_sync_lock(hdev);
> > +
> > + enable = !!cp->action;
> > + changed = (enable != hci_dev_test_flag(hdev, HCI_QUALITY_REPORT));
> > +
> > + if (!aosp_has_quality_report(hdev) && !hdev->set_quality_report) {
> > + err = mgmt_cmd_status(sk, hdev->id,
> > + MGMT_OP_SET_QUALITY_REPORT,
> > + MGMT_STATUS_NOT_SUPPORTED);
> > + goto unlock_quality_report;
> > + }
> > +
> > + if (changed) {
> > + if (hdev->set_quality_report)
> > + err = hdev->set_quality_report(hdev, enable);
> > + else
> > + err = aosp_set_quality_report(hdev, enable);
> > +
> > + if (err) {
> > + err = mgmt_cmd_status(sk, hdev->id,
> > + MGMT_OP_SET_QUALITY_REPORT,
> > + MGMT_STATUS_FAILED);
> > + goto unlock_quality_report;
> > + }
> > +
> > + if (enable)
> > + hci_dev_set_flag(hdev, HCI_QUALITY_REPORT);
> > + else
> > + hci_dev_clear_flag(hdev, HCI_QUALITY_REPORT);
> > + }
> > +
> > + bt_dev_dbg(hdev, "quality report enable %d changed %d",
> > + enable, changed);
> > +
> > + err = send_settings_rsp(sk, MGMT_OP_SET_QUALITY_REPORT, hdev);
> > + if (err < 0)
> > + goto unlock_quality_report;
> > +
> > + if (changed)
> > + err = new_settings(hdev, sk);
> > +
> > +unlock_quality_report:
> > + hci_req_sync_unlock(hdev);
> > + return err;
> > +}
> > +
> > static const struct hci_mgmt_handler mgmt_handlers[] = {
> > { NULL }, /* 0x0000 (no command) */
> > { read_version, MGMT_READ_VERSION_SIZE,
> > @@ -8779,6 +8758,7 @@ static const struct hci_mgmt_handler mgmt_handlers[] = {
> > { add_adv_patterns_monitor_rssi,
> > MGMT_ADD_ADV_PATTERNS_MONITOR_RSSI_SIZE,
> > HCI_MGMT_VAR_LEN },
> > + { set_quality_report, MGMT_SET_QUALITY_REPORT_SIZE },
> > };
> >
> > void mgmt_index_added(struct hci_dev *hdev)
>
> So this patch I actually get merged first.
I do not see this patch getting merged yet. I guess I still need to
remove the “hdev” that you mentioned above and re-submit this patch?
>
> However we still need to figure out if this setting is suppose to survive power off/on cycles. Right now as far as I can tell it is added to hci_dev_clear_volatile_flags and thus needs to be set again after power on.
Thank you for pointing this out. Whether the setting should survive
power off/on cycles is not mentioned in the AOSP BQR and Intel
telemetry specifications. I did a quick test on an Intel platform, the
setting does NOT survive over power cycles probably due to the HCI
Reset command during power off. Hence, I will need to address this
issue by restoring it explicitly. Please let me send separate patches
later to address this issue for both Intel and AOSP specs.
>
> Is this the expected behavior? I don’t think we want that. Since normally all other settings are restored after power on. And it is explicitly mentioned in doc/mgmt-api.txt as well.
I will mention this in the Set Powered Command in doc/mgmt-api.txt in
the separate patches later.
>
> Regards
>
> Marcel
>
>
--
Joseph Shyh-In Hwang
Email: josephsih@google.com
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [v4,1/3] Bluetooth: aosp: surface AOSP quality report through mgmt
2022-02-15 13:35 [PATCH v4 1/3] Bluetooth: aosp: surface AOSP quality report through mgmt Joseph Hwang
2022-02-15 13:35 ` [PATCH v4 2/3] Bluetooth: btintel: surface Intel telemetry events " Joseph Hwang
2022-02-15 13:35 ` [PATCH v4 3/3] Bluetooth: mgmt: add set_quality_report for MGMT_OP_SET_QUALITY_REPORT Joseph Hwang
@ 2022-02-15 14:08 ` bluez.test.bot
2022-02-15 17:52 ` kernel test robot
2022-02-17 12:41 ` Marcel Holtmann
4 siblings, 0 replies; 15+ messages in thread
From: bluez.test.bot @ 2022-02-15 14:08 UTC (permalink / raw)
To: linux-bluetooth, josephsih
[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=614537
---Test result---
Test Summary:
CheckPatch PASS 4.35 seconds
GitLint PASS 1.15 seconds
SubjectPrefix PASS 0.69 seconds
BuildKernel PASS 35.09 seconds
BuildKernel32 PASS 30.56 seconds
Incremental Build with patchesPASS 82.91 seconds
TestRunner: Setup PASS 556.96 seconds
TestRunner: l2cap-tester PASS 14.92 seconds
TestRunner: bnep-tester PASS 6.70 seconds
TestRunner: mgmt-tester PASS 119.24 seconds
TestRunner: rfcomm-tester PASS 8.50 seconds
TestRunner: sco-tester PASS 8.70 seconds
TestRunner: smp-tester PASS 8.49 seconds
TestRunner: userchan-tester PASS 7.03 seconds
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/3] Bluetooth: aosp: surface AOSP quality report through mgmt
2022-02-15 13:35 [PATCH v4 1/3] Bluetooth: aosp: surface AOSP quality report through mgmt Joseph Hwang
@ 2022-02-15 17:52 ` kernel test robot
2022-02-15 13:35 ` [PATCH v4 3/3] Bluetooth: mgmt: add set_quality_report for MGMT_OP_SET_QUALITY_REPORT Joseph Hwang
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2022-02-15 17:52 UTC (permalink / raw)
To: Joseph Hwang, linux-bluetooth, marcel, luiz.dentz, pali
Cc: kbuild-all, chromeos-bluetooth-upstreaming, josephsih,
Joseph Hwang, Archie Pusaka, Jakub Kicinski, Johan Hedberg,
linux-kernel, netdev
Hi Joseph,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on net-next/master next-20220215]
[cannot apply to net/master v5.17-rc4]
[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/20220215-213800
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: h8300-randconfig-s032-20220214 (https://download.01.org/0day-ci/archive/20220216/202202160117.jjnGwidL-lkp@intel.com/config)
compiler: h8300-linux-gcc (GCC) 11.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-dirty
# https://github.com/0day-ci/linux/commit/8c2212761e41006d67f3fad819b5bde57bc17773
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/20220215-213800
git checkout 8c2212761e41006d67f3fad819b5bde57bc17773
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=h8300 SHELL=/bin/bash net/bluetooth/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
net/bluetooth/hci_event.c:338:15: sparse: sparse: restricted __le16 degrades to integer
>> net/bluetooth/hci_event.c:4288:3: sparse: sparse: symbol 'evt_prefixes' was not declared. Should it be static?
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
include/net/bluetooth/hci.h:2473:47: sparse: sparse: array of flexible structures
include/net/bluetooth/hci.h:2559:43: sparse: sparse: array of flexible structures
vim +/evt_prefixes +4288 net/bluetooth/hci_event.c
4275
4276 /* Every distinct vendor specification must have a well-defined vendor
4277 * event prefix to determine if a vendor event meets the specification.
4278 * If an event prefix is fixed, it should be delcared with FIXED_EVT_PREFIX.
4279 * Otherwise, DYNAMIC_EVT_PREFIX should be used for variable prefixes.
4280 */
4281 struct vendor_event_prefix {
4282 __u8 *prefix;
4283 __u8 prefix_len;
4284 void (*vendor_func)(struct hci_dev *hdev, void *data,
4285 struct sk_buff *skb);
4286 __u8 *(*get_prefix)(struct hci_dev *hdev);
4287 __u8 (*get_prefix_len)(struct hci_dev *hdev);
> 4288 } evt_prefixes[] = {
4289 FIXED_EVT_PREFIX(AOSP_BQR_PREFIX, aosp_quality_report_evt),
4290 DYNAMIC_EVT_PREFIX(get_msft_evt_prefix, get_msft_evt_prefix_len,
4291 msft_vendor_evt),
4292
4293 /* end with a null entry */
4294 {},
4295 };
4296
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/3] Bluetooth: aosp: surface AOSP quality report through mgmt
@ 2022-02-15 17:52 ` kernel test robot
0 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2022-02-15 17:52 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3383 bytes --]
Hi Joseph,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on bluetooth-next/master]
[also build test WARNING on net-next/master next-20220215]
[cannot apply to net/master v5.17-rc4]
[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/20220215-213800
base: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: h8300-randconfig-s032-20220214 (https://download.01.org/0day-ci/archive/20220216/202202160117.jjnGwidL-lkp(a)intel.com/config)
compiler: h8300-linux-gcc (GCC) 11.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-dirty
# https://github.com/0day-ci/linux/commit/8c2212761e41006d67f3fad819b5bde57bc17773
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/20220215-213800
git checkout 8c2212761e41006d67f3fad819b5bde57bc17773
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=h8300 SHELL=/bin/bash net/bluetooth/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
net/bluetooth/hci_event.c:338:15: sparse: sparse: restricted __le16 degrades to integer
>> net/bluetooth/hci_event.c:4288:3: sparse: sparse: symbol 'evt_prefixes' was not declared. Should it be static?
net/bluetooth/hci_event.c: note: in included file (through include/net/bluetooth/hci_core.h):
include/net/bluetooth/hci.h:2473:47: sparse: sparse: array of flexible structures
include/net/bluetooth/hci.h:2559:43: sparse: sparse: array of flexible structures
vim +/evt_prefixes +4288 net/bluetooth/hci_event.c
4275
4276 /* Every distinct vendor specification must have a well-defined vendor
4277 * event prefix to determine if a vendor event meets the specification.
4278 * If an event prefix is fixed, it should be delcared with FIXED_EVT_PREFIX.
4279 * Otherwise, DYNAMIC_EVT_PREFIX should be used for variable prefixes.
4280 */
4281 struct vendor_event_prefix {
4282 __u8 *prefix;
4283 __u8 prefix_len;
4284 void (*vendor_func)(struct hci_dev *hdev, void *data,
4285 struct sk_buff *skb);
4286 __u8 *(*get_prefix)(struct hci_dev *hdev);
4287 __u8 (*get_prefix_len)(struct hci_dev *hdev);
> 4288 } evt_prefixes[] = {
4289 FIXED_EVT_PREFIX(AOSP_BQR_PREFIX, aosp_quality_report_evt),
4290 DYNAMIC_EVT_PREFIX(get_msft_evt_prefix, get_msft_evt_prefix_len,
4291 msft_vendor_evt),
4292
4293 /* end with a null entry */
4294 {},
4295 };
4296
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v4 1/3] Bluetooth: aosp: surface AOSP quality report through mgmt
2022-02-15 13:35 [PATCH v4 1/3] Bluetooth: aosp: surface AOSP quality report through mgmt Joseph Hwang
` (3 preceding siblings ...)
2022-02-15 17:52 ` kernel test robot
@ 2022-02-17 12:41 ` Marcel Holtmann
2022-03-05 7:42 ` Joseph Hwang
4 siblings, 1 reply; 15+ messages in thread
From: Marcel Holtmann @ 2022-02-17 12:41 UTC (permalink / raw)
To: Joseph Hwang
Cc: BlueZ, Luiz Augusto von Dentz, Pali Rohár,
CrosBT Upstreaming, Joseph Hwang, Archie Pusaka, David S. Miller,
Jakub Kicinski, Johan Hedberg, LKML, netdev
Hi Joseph,
> When receiving a HCI vendor event, the kernel checks if it is an
> AOSP bluetooth quality report. 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:
> - Rebase to resolve the code conflict.
> - Move aosp_quality_report_evt() from hci_event.c to aosp.c.
> - A new patch (3/3) is added to enable the quality report feature.
>
> Changes in v2:
> - Scrap the two structures defined in aosp.c and use constants for
> size check.
> - Do a basic size check about the quality report event. Do not pull
> data from the event in which the kernel has no interest.
> - Define vendor event prefixes with which vendor events of distinct
> vendor specifications can be clearly differentiated.
> - Use mgmt helpers to add the header and data to a mgmt skb.
this unsolicited vendor event business is giving me a headache. I assume it would be a lot simpler, but it doesn’t look like this. I need to spent some rounds of thinking to give you good advice. Unfortunately since we also want to support Intel vendor specific pieces, this is getting super complicated.
> include/net/bluetooth/hci_core.h | 5 ++
> include/net/bluetooth/mgmt.h | 7 +++
> net/bluetooth/aosp.c | 27 ++++++++++
> net/bluetooth/aosp.h | 13 +++++
> net/bluetooth/hci_event.c | 84 +++++++++++++++++++++++++++++++-
> net/bluetooth/mgmt.c | 20 ++++++++
> net/bluetooth/msft.c | 14 ++++++
> net/bluetooth/msft.h | 12 +++++
> 8 files changed, 181 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index f5caff1ddb29..ea83619ac4de 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1864,6 +1864,8 @@ int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
> int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
> void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
> bdaddr_t *bdaddr, u8 addr_type);
> +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> + u8 quality_spec);
>
> u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
> u16 to_multiplier);
> @@ -1882,4 +1884,7 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
>
> #define TRANSPORT_TYPE_MAX 0x04
>
> +#define QUALITY_SPEC_AOSP_BQR 0x0
> +#define QUALITY_SPEC_INTEL_TELEMETRY 0x1
> +
> #endif /* __HCI_CORE_H */
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 3d26e6a3478b..83b602636262 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -1120,3 +1120,10 @@ struct mgmt_ev_adv_monitor_device_lost {
> __le16 monitor_handle;
> struct mgmt_addr_info addr;
> } __packed;
> +
> +#define MGMT_EV_QUALITY_REPORT 0x0031
> +struct mgmt_ev_quality_report {
> + __u8 quality_spec;
> + __u32 data_len;
> + __u8 data[];
> +} __packed;
> diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c
> index 432ae3aac9e3..4a336433180d 100644
> --- a/net/bluetooth/aosp.c
> +++ b/net/bluetooth/aosp.c
> @@ -199,3 +199,30 @@ int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> else
> return disable_quality_report(hdev);
> }
> +
> +#define BLUETOOTH_QUALITY_REPORT_EV 0x58
> +
> +/* The following LEN = 1-byte Sub-event code + 48-byte Sub-event Parameters */
> +#define BLUETOOTH_QUALITY_REPORT_LEN 49
> +
> +bool aosp_check_quality_report_len(struct sk_buff *skb)
> +{
> + /* skb->len is allowed to be larger than BLUETOOTH_QUALITY_REPORT_LEN
> + * to accommodate an additional Vendor Specific Parameter (vsp) field.
> + */
> + if (skb->len < BLUETOOTH_QUALITY_REPORT_LEN) {
> + BT_ERR("AOSP evt data len %d too short (%u expected)",
> + skb->len, BLUETOOTH_QUALITY_REPORT_LEN);
> + return false;
> + }
> +
> + return true;
> +}
> +
> +void aosp_quality_report_evt(struct hci_dev *hdev, void *data,
> + struct sk_buff *skb)
> +{
> + if (aosp_has_quality_report(hdev) && aosp_check_quality_report_len(skb))
> + mgmt_quality_report(hdev, skb->data, skb->len,
> + QUALITY_SPEC_AOSP_BQR);
> +}
> diff --git a/net/bluetooth/aosp.h b/net/bluetooth/aosp.h
> index 2fd8886d51b2..b21751e012de 100644
> --- a/net/bluetooth/aosp.h
> +++ b/net/bluetooth/aosp.h
> @@ -10,6 +10,9 @@ void aosp_do_close(struct hci_dev *hdev);
>
> bool aosp_has_quality_report(struct hci_dev *hdev);
> int aosp_set_quality_report(struct hci_dev *hdev, bool enable);
> +bool aosp_check_quality_report_len(struct sk_buff *skb);
> +void aosp_quality_report_evt(struct hci_dev *hdev, void *data,
> + struct sk_buff *skb);
>
> #else
>
> @@ -26,4 +29,14 @@ static inline int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> return -EOPNOTSUPP;
> }
>
> +static inline bool aosp_check_quality_report_len(struct sk_buff *skb)
> +{
> + return false;
> +}
> +
> +static inline void aosp_quality_report_evt(struct hci_dev *hdev, void *data,
> + struct sk_buff *skb)
> +{
> +}
> +
> #endif
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 63b925921c87..6468ea0f71bd 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -37,6 +37,7 @@
> #include "smp.h"
> #include "msft.h"
> #include "eir.h"
> +#include "aosp.h"
>
> #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
> "\x00\x00\x00\x00\x00\x00\x00\x00"
> @@ -4241,6 +4242,87 @@ static void hci_num_comp_blocks_evt(struct hci_dev *hdev, void *data,
> queue_work(hdev->workqueue, &hdev->tx_work);
> }
>
> +/* Define the fixed vendor event prefixes below.
> + * Note: AOSP HCI Requirements use 0x54 and up as sub-event codes without
> + * actually defining a vendor prefix. Refer to
> + * https://source.android.com/devices/bluetooth/hci_requirements
> + * Hence, the other vendor event prefixes should not use the same
> + * space to avoid collision.
> + */
> +static unsigned char AOSP_BQR_PREFIX[] = { 0x58 };
> +
> +/* Some vendor prefixes are fixed values and lengths. */
> +#define FIXED_EVT_PREFIX(_prefix, _vendor_func) \
> +{ \
> + .prefix = _prefix, \
> + .prefix_len = sizeof(_prefix), \
> + .vendor_func = _vendor_func, \
> + .get_prefix = NULL, \
> + .get_prefix_len = NULL, \
> +}
> +
> +/* Some vendor prefixes are only available at run time. The
> + * values and lengths are variable.
> + */
> +#define DYNAMIC_EVT_PREFIX(_prefix_func, _prefix_len_func, _vendor_func)\
> +{ \
> + .prefix = NULL, \
> + .prefix_len = 0, \
> + .vendor_func = _vendor_func, \
> + .get_prefix = _prefix_func, \
> + .get_prefix_len = _prefix_len_func, \
> +}
> +
> +/* 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.
> + * Otherwise, DYNAMIC_EVT_PREFIX should be used for variable prefixes.
> + */
> +struct vendor_event_prefix {
> + __u8 *prefix;
> + __u8 prefix_len;
> + void (*vendor_func)(struct hci_dev *hdev, void *data,
> + struct sk_buff *skb);
> + __u8 *(*get_prefix)(struct hci_dev *hdev);
> + __u8 (*get_prefix_len)(struct hci_dev *hdev);
> +} evt_prefixes[] = {
> + FIXED_EVT_PREFIX(AOSP_BQR_PREFIX, aosp_quality_report_evt),
> + DYNAMIC_EVT_PREFIX(get_msft_evt_prefix, get_msft_evt_prefix_len,
> + msft_vendor_evt),
> +
> + /* end with a null entry */
> + {},
> +};
> +
> +static void hci_vendor_evt(struct hci_dev *hdev, void *data,
> + struct sk_buff *skb)
> +{
> + int i;
> + __u8 *prefix;
> + __u8 prefix_len;
> +
> + for (i = 0; evt_prefixes[i].vendor_func; i++) {
> + if (evt_prefixes[i].get_prefix)
> + prefix = evt_prefixes[i].get_prefix(hdev);
> + else
> + prefix = evt_prefixes[i].prefix;
> +
> + if (evt_prefixes[i].get_prefix_len)
> + prefix_len = evt_prefixes[i].get_prefix_len(hdev);
> + else
> + prefix_len = evt_prefixes[i].prefix_len;
> +
> + if (!prefix || prefix_len == 0)
> + continue;
> +
> + /* Compare the raw prefix data directly. */
> + if (!memcmp(prefix, skb->data, prefix_len)) {
> + evt_prefixes[i].vendor_func(hdev, data, skb);
> + break;
> + }
> + }
> +}
> +
> static void hci_mode_change_evt(struct hci_dev *hdev, void *data,
> struct sk_buff *skb)
> {
> @@ -6844,7 +6926,7 @@ static const struct hci_ev {
> HCI_EV(HCI_EV_NUM_COMP_BLOCKS, hci_num_comp_blocks_evt,
> sizeof(struct hci_ev_num_comp_blocks)),
> /* [0xff = HCI_EV_VENDOR] */
> - HCI_EV_VL(HCI_EV_VENDOR, msft_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> + HCI_EV_VL(HCI_EV_VENDOR, hci_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> };
I was thinking along the lines like this:
HCI_EV_VND(evt_prefix, evt_prefix_len, callback),
So that we in the end can do things like this:
HCI_EV_VL({ 0x58 }, 1, aosp_quality_report_evt),
>
> static void hci_event_func(struct hci_dev *hdev, u8 event, struct sk_buff *skb,
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 914e2f2d3586..5e48576041fb 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -4389,6 +4389,26 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> MGMT_STATUS_NOT_SUPPORTED);
> }
>
> +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> + u8 quality_spec)
> +{
> + struct mgmt_ev_quality_report *ev;
> + struct sk_buff *skb;
> +
> + skb = mgmt_alloc_skb(hdev, MGMT_EV_QUALITY_REPORT,
> + sizeof(*ev) + data_len);
> + if (!skb)
> + return -ENOMEM;
> +
> + ev = skb_put(skb, sizeof(*ev));
> + ev->quality_spec = quality_spec;
> + ev->data_len = data_len;
> + skb_put_data(skb, data, data_len);
> +
> + return mgmt_event_skb(skb, NULL);
> +}
> +EXPORT_SYMBOL(mgmt_quality_report);
> +
I know what you want to do, but I can not let you call mgmt_ function from a driver. We need to make this cleaner and abstract so that the driver has a proper path to report it to hci_core and that decides then to send the report or not.
> static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> u16 data_len)
> {
> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> index 9a3d77d3ca86..3edf64baf479 100644
> --- a/net/bluetooth/msft.c
> +++ b/net/bluetooth/msft.c
> @@ -731,6 +731,20 @@ static void msft_monitor_device_evt(struct hci_dev *hdev, struct sk_buff *skb)
> handle_data->mgmt_handle);
> }
>
> +__u8 *get_msft_evt_prefix(struct hci_dev *hdev)
> +{
> + struct msft_data *msft = hdev->msft_data;
> +
> + return msft->evt_prefix;
> +}
> +
> +__u8 get_msft_evt_prefix_len(struct hci_dev *hdev)
> +{
> + struct msft_data *msft = hdev->msft_data;
> +
> + return msft->evt_prefix_len;
> +}
> +
So I wonder if this should be moved directly into hci_dev under CONFIG_BT_MSFTEXT check. Luiz also needs to have a look at this. This is unfortunately getting a bit nasty now. We need to find a clean solution, otherwise the next vendor thing is blowing up in our face.
> void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
> {
> struct msft_data *msft = hdev->msft_data;
> diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
> index afcaf7d3b1cb..a354ebf61fed 100644
> --- a/net/bluetooth/msft.h
> +++ b/net/bluetooth/msft.h
> @@ -27,6 +27,8 @@ int msft_set_filter_enable(struct hci_dev *hdev, bool enable);
> int msft_suspend_sync(struct hci_dev *hdev);
> int msft_resume_sync(struct hci_dev *hdev);
> bool msft_curve_validity(struct hci_dev *hdev);
> +__u8 *get_msft_evt_prefix(struct hci_dev *hdev);
> +__u8 get_msft_evt_prefix_len(struct hci_dev *hdev);
>
> #else
>
> @@ -77,4 +79,14 @@ static inline bool msft_curve_validity(struct hci_dev *hdev)
> return false;
> }
>
> +static inline __u8 *get_msft_evt_prefix(struct hci_dev *hdev)
> +{
> + return NULL;
> +}
> +
> +static inline __u8 get_msft_evt_prefix_len(struct hci_dev *hdev)
> +{
> + return 0;
> +}
> +
> #endif
Regards
Marcel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v4 1/3] Bluetooth: aosp: surface AOSP quality report through mgmt
2022-02-17 12:41 ` Marcel Holtmann
@ 2022-03-05 7:42 ` Joseph Hwang
0 siblings, 0 replies; 15+ messages in thread
From: Joseph Hwang @ 2022-03-05 7:42 UTC (permalink / raw)
To: Marcel Holtmann
Cc: BlueZ, Luiz Augusto von Dentz, Pali Rohár,
CrosBT Upstreaming, Archie Pusaka, David S. Miller,
Jakub Kicinski, Johan Hedberg, LKML, netdev
Hi Marcel, thank you for reviewing the patches! I have some questions
and would like to confirm with you. Please read my replies in the
lines below. Thanks!
On Thu, Feb 17, 2022 at 8:41 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Joseph,
>
> > When receiving a HCI vendor event, the kernel checks if it is an
> > AOSP bluetooth quality report. 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:
> > - Rebase to resolve the code conflict.
> > - Move aosp_quality_report_evt() from hci_event.c to aosp.c.
> > - A new patch (3/3) is added to enable the quality report feature.
> >
> > Changes in v2:
> > - Scrap the two structures defined in aosp.c and use constants for
> > size check.
> > - Do a basic size check about the quality report event. Do not pull
> > data from the event in which the kernel has no interest.
> > - Define vendor event prefixes with which vendor events of distinct
> > vendor specifications can be clearly differentiated.
> > - Use mgmt helpers to add the header and data to a mgmt skb.
>
> this unsolicited vendor event business is giving me a headache. I assume it would be a lot simpler, but it doesn’t look like this. I need to spent some rounds of thinking to give you good advice. Unfortunately since we also want to support Intel vendor specific pieces, this is getting super complicated.
You have mentioned using hdev->hci_recv_quality_report to send the
unsolicited events from the Intel driver in your comments in Patch
2/3. Just would like to confirm with you. Thanks.
>
> > include/net/bluetooth/hci_core.h | 5 ++
> > include/net/bluetooth/mgmt.h | 7 +++
> > net/bluetooth/aosp.c | 27 ++++++++++
> > net/bluetooth/aosp.h | 13 +++++
> > net/bluetooth/hci_event.c | 84 +++++++++++++++++++++++++++++++-
> > net/bluetooth/mgmt.c | 20 ++++++++
> > net/bluetooth/msft.c | 14 ++++++
> > net/bluetooth/msft.h | 12 +++++
> > 8 files changed, 181 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index f5caff1ddb29..ea83619ac4de 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -1864,6 +1864,8 @@ int mgmt_add_adv_patterns_monitor_complete(struct hci_dev *hdev, u8 status);
> > int mgmt_remove_adv_monitor_complete(struct hci_dev *hdev, u8 status);
> > void mgmt_adv_monitor_device_lost(struct hci_dev *hdev, u16 handle,
> > bdaddr_t *bdaddr, u8 addr_type);
> > +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> > + u8 quality_spec);
> >
> > u8 hci_le_conn_update(struct hci_conn *conn, u16 min, u16 max, u16 latency,
> > u16 to_multiplier);
> > @@ -1882,4 +1884,7 @@ void hci_copy_identity_address(struct hci_dev *hdev, bdaddr_t *bdaddr,
> >
> > #define TRANSPORT_TYPE_MAX 0x04
> >
> > +#define QUALITY_SPEC_AOSP_BQR 0x0
> > +#define QUALITY_SPEC_INTEL_TELEMETRY 0x1
> > +
> > #endif /* __HCI_CORE_H */
> > diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> > index 3d26e6a3478b..83b602636262 100644
> > --- a/include/net/bluetooth/mgmt.h
> > +++ b/include/net/bluetooth/mgmt.h
> > @@ -1120,3 +1120,10 @@ struct mgmt_ev_adv_monitor_device_lost {
> > __le16 monitor_handle;
> > struct mgmt_addr_info addr;
> > } __packed;
> > +
> > +#define MGMT_EV_QUALITY_REPORT 0x0031
> > +struct mgmt_ev_quality_report {
> > + __u8 quality_spec;
> > + __u32 data_len;
> > + __u8 data[];
> > +} __packed;
> > diff --git a/net/bluetooth/aosp.c b/net/bluetooth/aosp.c
> > index 432ae3aac9e3..4a336433180d 100644
> > --- a/net/bluetooth/aosp.c
> > +++ b/net/bluetooth/aosp.c
> > @@ -199,3 +199,30 @@ int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> > else
> > return disable_quality_report(hdev);
> > }
> > +
> > +#define BLUETOOTH_QUALITY_REPORT_EV 0x58
> > +
> > +/* The following LEN = 1-byte Sub-event code + 48-byte Sub-event Parameters */
> > +#define BLUETOOTH_QUALITY_REPORT_LEN 49
> > +
> > +bool aosp_check_quality_report_len(struct sk_buff *skb)
> > +{
> > + /* skb->len is allowed to be larger than BLUETOOTH_QUALITY_REPORT_LEN
> > + * to accommodate an additional Vendor Specific Parameter (vsp) field.
> > + */
> > + if (skb->len < BLUETOOTH_QUALITY_REPORT_LEN) {
> > + BT_ERR("AOSP evt data len %d too short (%u expected)",
> > + skb->len, BLUETOOTH_QUALITY_REPORT_LEN);
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +void aosp_quality_report_evt(struct hci_dev *hdev, void *data,
> > + struct sk_buff *skb)
> > +{
> > + if (aosp_has_quality_report(hdev) && aosp_check_quality_report_len(skb))
> > + mgmt_quality_report(hdev, skb->data, skb->len,
> > + QUALITY_SPEC_AOSP_BQR);
> > +}
> > diff --git a/net/bluetooth/aosp.h b/net/bluetooth/aosp.h
> > index 2fd8886d51b2..b21751e012de 100644
> > --- a/net/bluetooth/aosp.h
> > +++ b/net/bluetooth/aosp.h
> > @@ -10,6 +10,9 @@ void aosp_do_close(struct hci_dev *hdev);
> >
> > bool aosp_has_quality_report(struct hci_dev *hdev);
> > int aosp_set_quality_report(struct hci_dev *hdev, bool enable);
> > +bool aosp_check_quality_report_len(struct sk_buff *skb);
> > +void aosp_quality_report_evt(struct hci_dev *hdev, void *data,
> > + struct sk_buff *skb);
> >
> > #else
> >
> > @@ -26,4 +29,14 @@ static inline int aosp_set_quality_report(struct hci_dev *hdev, bool enable)
> > return -EOPNOTSUPP;
> > }
> >
> > +static inline bool aosp_check_quality_report_len(struct sk_buff *skb)
> > +{
> > + return false;
> > +}
> > +
> > +static inline void aosp_quality_report_evt(struct hci_dev *hdev, void *data,
> > + struct sk_buff *skb)
> > +{
> > +}
> > +
> > #endif
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index 63b925921c87..6468ea0f71bd 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -37,6 +37,7 @@
> > #include "smp.h"
> > #include "msft.h"
> > #include "eir.h"
> > +#include "aosp.h"
> >
> > #define ZERO_KEY "\x00\x00\x00\x00\x00\x00\x00\x00" \
> > "\x00\x00\x00\x00\x00\x00\x00\x00"
> > @@ -4241,6 +4242,87 @@ static void hci_num_comp_blocks_evt(struct hci_dev *hdev, void *data,
> > queue_work(hdev->workqueue, &hdev->tx_work);
> > }
> >
> > +/* Define the fixed vendor event prefixes below.
> > + * Note: AOSP HCI Requirements use 0x54 and up as sub-event codes without
> > + * actually defining a vendor prefix. Refer to
> > + * https://source.android.com/devices/bluetooth/hci_requirements
> > + * Hence, the other vendor event prefixes should not use the same
> > + * space to avoid collision.
> > + */
> > +static unsigned char AOSP_BQR_PREFIX[] = { 0x58 };
> > +
> > +/* Some vendor prefixes are fixed values and lengths. */
> > +#define FIXED_EVT_PREFIX(_prefix, _vendor_func) \
> > +{ \
> > + .prefix = _prefix, \
> > + .prefix_len = sizeof(_prefix), \
> > + .vendor_func = _vendor_func, \
> > + .get_prefix = NULL, \
> > + .get_prefix_len = NULL, \
> > +}
> > +
> > +/* Some vendor prefixes are only available at run time. The
> > + * values and lengths are variable.
> > + */
> > +#define DYNAMIC_EVT_PREFIX(_prefix_func, _prefix_len_func, _vendor_func)\
> > +{ \
> > + .prefix = NULL, \
> > + .prefix_len = 0, \
> > + .vendor_func = _vendor_func, \
> > + .get_prefix = _prefix_func, \
> > + .get_prefix_len = _prefix_len_func, \
> > +}
> > +
> > +/* 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.
> > + * Otherwise, DYNAMIC_EVT_PREFIX should be used for variable prefixes.
> > + */
> > +struct vendor_event_prefix {
> > + __u8 *prefix;
> > + __u8 prefix_len;
> > + void (*vendor_func)(struct hci_dev *hdev, void *data,
> > + struct sk_buff *skb);
> > + __u8 *(*get_prefix)(struct hci_dev *hdev);
> > + __u8 (*get_prefix_len)(struct hci_dev *hdev);
> > +} evt_prefixes[] = {
> > + FIXED_EVT_PREFIX(AOSP_BQR_PREFIX, aosp_quality_report_evt),
> > + DYNAMIC_EVT_PREFIX(get_msft_evt_prefix, get_msft_evt_prefix_len,
> > + msft_vendor_evt),
> > +
> > + /* end with a null entry */
> > + {},
> > +};
> > +
> > +static void hci_vendor_evt(struct hci_dev *hdev, void *data,
> > + struct sk_buff *skb)
> > +{
> > + int i;
> > + __u8 *prefix;
> > + __u8 prefix_len;
> > +
> > + for (i = 0; evt_prefixes[i].vendor_func; i++) {
> > + if (evt_prefixes[i].get_prefix)
> > + prefix = evt_prefixes[i].get_prefix(hdev);
> > + else
> > + prefix = evt_prefixes[i].prefix;
> > +
> > + if (evt_prefixes[i].get_prefix_len)
> > + prefix_len = evt_prefixes[i].get_prefix_len(hdev);
> > + else
> > + prefix_len = evt_prefixes[i].prefix_len;
> > +
> > + if (!prefix || prefix_len == 0)
> > + continue;
> > +
> > + /* Compare the raw prefix data directly. */
> > + if (!memcmp(prefix, skb->data, prefix_len)) {
> > + evt_prefixes[i].vendor_func(hdev, data, skb);
> > + break;
> > + }
> > + }
> > +}
> > +
> > static void hci_mode_change_evt(struct hci_dev *hdev, void *data,
> > struct sk_buff *skb)
> > {
> > @@ -6844,7 +6926,7 @@ static const struct hci_ev {
> > HCI_EV(HCI_EV_NUM_COMP_BLOCKS, hci_num_comp_blocks_evt,
> > sizeof(struct hci_ev_num_comp_blocks)),
> > /* [0xff = HCI_EV_VENDOR] */
> > - HCI_EV_VL(HCI_EV_VENDOR, msft_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> > + HCI_EV_VL(HCI_EV_VENDOR, hci_vendor_evt, 0, HCI_MAX_EVENT_SIZE),
> > };
>
> I was thinking along the lines like this:
>
> HCI_EV_VND(evt_prefix, evt_prefix_len, callback),
>
> So that we in the end can do things like this:
>
> HCI_EV_VL({ 0x58 }, 1, aosp_quality_report_evt),
>
I wish I could use a macro as simple as HCI_EV_VND(evt_prefix,
evt_prefix_len, callback). However, I do not have a clean method to
handle the dynamic msft vendor prefix of which the value and length
are not known until runtime. Do you have any suggestions here?
>
>
> >
> > static void hci_event_func(struct hci_dev *hdev, u8 event, struct sk_buff *skb,
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index 914e2f2d3586..5e48576041fb 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -4389,6 +4389,26 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
> > MGMT_STATUS_NOT_SUPPORTED);
> > }
> >
> > +int mgmt_quality_report(struct hci_dev *hdev, void *data, u32 data_len,
> > + u8 quality_spec)
> > +{
> > + struct mgmt_ev_quality_report *ev;
> > + struct sk_buff *skb;
> > +
> > + skb = mgmt_alloc_skb(hdev, MGMT_EV_QUALITY_REPORT,
> > + sizeof(*ev) + data_len);
> > + if (!skb)
> > + return -ENOMEM;
> > +
> > + ev = skb_put(skb, sizeof(*ev));
> > + ev->quality_spec = quality_spec;
> > + ev->data_len = data_len;
> > + skb_put_data(skb, data, data_len);
> > +
> > + return mgmt_event_skb(skb, NULL);
> > +}
> > +EXPORT_SYMBOL(mgmt_quality_report);
> > +
>
> I know what you want to do, but I can not let you call mgmt_ function from a driver. We need to make this cleaner and abstract so that the driver has a proper path to report it to hci_core and that decides then to send the report or not.
Will set up hdev->hci_recv_quality_report as you mentioned in comments
in Patch 2/3.
>
>
> > static int get_device_flags(struct sock *sk, struct hci_dev *hdev, void *data,
> > u16 data_len)
> > {
> > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> > index 9a3d77d3ca86..3edf64baf479 100644
> > --- a/net/bluetooth/msft.c
> > +++ b/net/bluetooth/msft.c
> > @@ -731,6 +731,20 @@ static void msft_monitor_device_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > handle_data->mgmt_handle);
> > }
> >
> > +__u8 *get_msft_evt_prefix(struct hci_dev *hdev)
> > +{
> > + struct msft_data *msft = hdev->msft_data;
> > +
> > + return msft->evt_prefix;
> > +}
> > +
> > +__u8 get_msft_evt_prefix_len(struct hci_dev *hdev)
> > +{
> > + struct msft_data *msft = hdev->msft_data;
> > +
> > + return msft->evt_prefix_len;
> > +}
> > +
>
> So I wonder if this should be moved directly into hci_dev under CONFIG_BT_MSFTEXT check. Luiz also needs to have a look at this. This is unfortunately getting a bit nasty now. We need to find a clean solution, otherwise the next vendor thing is blowing up in our face.
My understanding is that static inline hci_set_msft_opcode() under
CONFIG_BT_MSFTEXT is placed in hci_core.h because drivers need to call
it. Here, get_msft_evt_prefix() and get_msft_evt_prefix_len() are not
to be called by drivers. Do we still need to move them into
hci_core.h? Please let me know if I have any misunderstanding. Thanks.
>
>
> > void msft_vendor_evt(struct hci_dev *hdev, void *data, struct sk_buff *skb)
> > {
> > struct msft_data *msft = hdev->msft_data;
> > diff --git a/net/bluetooth/msft.h b/net/bluetooth/msft.h
> > index afcaf7d3b1cb..a354ebf61fed 100644
> > --- a/net/bluetooth/msft.h
> > +++ b/net/bluetooth/msft.h
> > @@ -27,6 +27,8 @@ int msft_set_filter_enable(struct hci_dev *hdev, bool enable);
> > int msft_suspend_sync(struct hci_dev *hdev);
> > int msft_resume_sync(struct hci_dev *hdev);
> > bool msft_curve_validity(struct hci_dev *hdev);
> > +__u8 *get_msft_evt_prefix(struct hci_dev *hdev);
> > +__u8 get_msft_evt_prefix_len(struct hci_dev *hdev);
> >
> > #else
> >
> > @@ -77,4 +79,14 @@ static inline bool msft_curve_validity(struct hci_dev *hdev)
> > return false;
> > }
> >
> > +static inline __u8 *get_msft_evt_prefix(struct hci_dev *hdev)
> > +{
> > + return NULL;
> > +}
> > +
> > +static inline __u8 get_msft_evt_prefix_len(struct hci_dev *hdev)
> > +{
> > + return 0;
> > +}
> > +
> > #endif
>
> Regards
>
> Marcel
>
Regards,
--
Joseph Shyh-In Hwang
Email: josephsih@google.com
^ permalink raw reply [flat|nested] 15+ messages in thread