diff mbox series

[v1,1/5] Bluetooth: advmon offload MSFT add rssi support

Message ID 20201203182903.v1.1.I92d2e2a87419730d60136680cbe27636baf94b15@changeid (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series MSFT offloading support for advertisement monitor | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 21 this patch: 21
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 49 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 21 this patch: 21
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Archie Pusaka Dec. 3, 2020, 10:29 a.m. UTC
From: Archie Pusaka <apusaka@chromium.org>

MSFT needs rssi parameter for monitoring advertisement packet,
therefore we should supply them from mgmt.

Signed-off-by: Archie Pusaka <apusaka@chromium.org>
Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
Reviewed-by: Yun-Hao Chung <howardchung@google.com>

---

 include/net/bluetooth/hci_core.h | 9 +++++++++
 include/net/bluetooth/mgmt.h     | 9 +++++++++
 net/bluetooth/mgmt.c             | 8 ++++++++
 3 files changed, 26 insertions(+)

Comments

Marcel Holtmann Dec. 3, 2020, 2:03 p.m. UTC | #1
Hi Archie,

> MSFT needs rssi parameter for monitoring advertisement packet,
> therefore we should supply them from mgmt.
> 
> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> Reviewed-by: Yun-Hao Chung <howardchung@google.com>

I don’t need any Reviewed-by if they are not catching an obvious user API breakage.

> ---
> 
> include/net/bluetooth/hci_core.h | 9 +++++++++
> include/net/bluetooth/mgmt.h     | 9 +++++++++
> net/bluetooth/mgmt.c             | 8 ++++++++
> 3 files changed, 26 insertions(+)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 9873e1c8cd16..42d446417817 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -246,8 +246,17 @@ struct adv_pattern {
> 	__u8 value[HCI_MAX_AD_LENGTH];
> };
> 
> +struct adv_rssi_thresholds {
> +	__s8 low_threshold;
> +	__s8 high_threshold;
> +	__u16 low_threshold_timeout;
> +	__u16 high_threshold_timeout;
> +	__u8 sampling_period;
> +};
> +
> struct adv_monitor {
> 	struct list_head patterns;
> +	struct adv_rssi_thresholds rssi;
> 	bool		active;
> 	__u16		handle;
> };
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index d8367850e8cd..dc534837be0e 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -763,9 +763,18 @@ struct mgmt_adv_pattern {
> 	__u8 value[31];
> } __packed;
> 
> +struct mgmt_adv_rssi_thresholds {
> +	__s8 high_threshold;
> +	__le16 high_threshold_timeout;
> +	__s8 low_threshold;
> +	__le16 low_threshold_timeout;
> +	__u8 sampling_period;
> +} __packed;
> +
> #define MGMT_OP_ADD_ADV_PATTERNS_MONITOR	0x0052
> struct mgmt_cp_add_adv_patterns_monitor {
> 	__u8 pattern_count;
> +	struct mgmt_adv_rssi_thresholds rssi;
> 	struct mgmt_adv_pattern patterns[];
> } __packed;

This is something we can not do. It breaks an userspace facing API. Is the mgmt opcode 0x0052 in an already released kernel?

Regards

Marcel
Archie Pusaka Dec. 4, 2020, 3:25 a.m. UTC | #2
Hi Marcel,

On Thu, 3 Dec 2020 at 22:03, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Archie,
>
> > MSFT needs rssi parameter for monitoring advertisement packet,
> > therefore we should supply them from mgmt.
> >
> > Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> > Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> > Reviewed-by: Yun-Hao Chung <howardchung@google.com>
>
> I don’t need any Reviewed-by if they are not catching an obvious user API breakage.
>
> > ---
> >
> > include/net/bluetooth/hci_core.h | 9 +++++++++
> > include/net/bluetooth/mgmt.h     | 9 +++++++++
> > net/bluetooth/mgmt.c             | 8 ++++++++
> > 3 files changed, 26 insertions(+)
> >
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index 9873e1c8cd16..42d446417817 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -246,8 +246,17 @@ struct adv_pattern {
> >       __u8 value[HCI_MAX_AD_LENGTH];
> > };
> >
> > +struct adv_rssi_thresholds {
> > +     __s8 low_threshold;
> > +     __s8 high_threshold;
> > +     __u16 low_threshold_timeout;
> > +     __u16 high_threshold_timeout;
> > +     __u8 sampling_period;
> > +};
> > +
> > struct adv_monitor {
> >       struct list_head patterns;
> > +     struct adv_rssi_thresholds rssi;
> >       bool            active;
> >       __u16           handle;
> > };
> > diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> > index d8367850e8cd..dc534837be0e 100644
> > --- a/include/net/bluetooth/mgmt.h
> > +++ b/include/net/bluetooth/mgmt.h
> > @@ -763,9 +763,18 @@ struct mgmt_adv_pattern {
> >       __u8 value[31];
> > } __packed;
> >
> > +struct mgmt_adv_rssi_thresholds {
> > +     __s8 high_threshold;
> > +     __le16 high_threshold_timeout;
> > +     __s8 low_threshold;
> > +     __le16 low_threshold_timeout;
> > +     __u8 sampling_period;
> > +} __packed;
> > +
> > #define MGMT_OP_ADD_ADV_PATTERNS_MONITOR      0x0052
> > struct mgmt_cp_add_adv_patterns_monitor {
> >       __u8 pattern_count;
> > +     struct mgmt_adv_rssi_thresholds rssi;
> >       struct mgmt_adv_pattern patterns[];
> > } __packed;
>
> This is something we can not do. It breaks an userspace facing API. Is the mgmt opcode 0x0052 in an already released kernel?

Yes, the opcode does exist in an already released kernel.

The DBus method which accesses this API is put behind the experimental
flag, therefore we expect they are flexible enough to support changes.
Previously, we already had a discussion in an email thread with the
title "Offload RSSI tracking to controller", and the outcome supports
this change.

Here is an excerpt of the discussion.
On Thu, 1 Oct 2020 at 05:58, Miao-chen Chou <mcchou@google.com> wrote:
>
> Hi Luiz,
>
> Yes, the RSSI is included as a part of the Adv monitor API, and the RSSI tracking is currently implemented (the patch series is still under review) in bluetoothd and used by bluetoothctl (submenu advmon). As mentioned, we are planning to offload the RSSI tracking to the controller as well, so there will be changes to the corresponding MGMT commands.
> Thanks for your quick feedback!
>
> Regards,
> Miao
>
> On Wed, Sep 30, 2020 at 2:00 PM Von Dentz, Luiz <luiz.von.dentz@intel.com> wrote:
>>
>> Hi Miao,
>>
>> I do recall seeing these at D-Bus level, or perhaps it was in use by bluetoothctl commands? Anyway since these are still experimental it should be fine to change them.
>> ________________________________
>> From: Miao-chen Chou <mcchou@google.com>
>> Sent: Wednesday, September 30, 2020 12:51 PM
>> To: Holtmann, Marcel <marcel.holtmann@intel.com>; Von Dentz, Luiz <luiz.von.dentz@intel.com>
>> Cc: Alain Michaud <alainmichaud@google.com>; Yun-hao Chung <howardchung@google.com>; Manish Mandlik <mmandlik@google.com>; Archie Pusaka <apusaka@google.com>
>> Subject: Offload RSSI tracking to controller.
>>
>> Hi Luiz and Marcel,
>>
>> Going forward to 2020 Q4, we will be working on offloading the content filtering to the controllers based on controll's support of MSFT HCI extension. In the meantime, we are planning to change the existing MGMT commands of Adv monitoring to allow the offloading of RSSI tracking shortly. Here is a snippet of potential changes.
>>
>> +struct mgmt_adv_rssi_thresholds {
>> +       __s8 high_rssi_threshold;
>> +       u16 high_rssi_threshold_timeout;
>> +       __s8 low_rssi_threshold;
>> +       u16 high_rssi_threshold_timeout;
>> +} __packed;
>>
>> struct mgmt_cp_add_adv_patterns_monitor {
>>         u8 pattern_count;
>> +        struct mgmt_adv_rssi_thresholds rssi_thresholds;
>>         struct mgmt_adv_pattern patterns[];
>> } __packed;
>>
>> Note that as suggested by you, the D-Bus Adv monitor API which accesses these MGMT commands is currently hidden behind the experimental flag, so they are still mutable. We'd like to hear your early feedback on changing the corresponding MGMT commands.
>>
>> Thanks,
>> Miao

Thanks,
Archie
Marcel Holtmann Dec. 4, 2020, 9:51 a.m. UTC | #3
Hi Archie,

>>> MSFT needs rssi parameter for monitoring advertisement packet,
>>> therefore we should supply them from mgmt.
>>> 
>>> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
>>> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
>>> Reviewed-by: Yun-Hao Chung <howardchung@google.com>
>> 
>> I don’t need any Reviewed-by if they are not catching an obvious user API breakage.
>> 
>>> ---
>>> 
>>> include/net/bluetooth/hci_core.h | 9 +++++++++
>>> include/net/bluetooth/mgmt.h     | 9 +++++++++
>>> net/bluetooth/mgmt.c             | 8 ++++++++
>>> 3 files changed, 26 insertions(+)
>>> 
>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>> index 9873e1c8cd16..42d446417817 100644
>>> --- a/include/net/bluetooth/hci_core.h
>>> +++ b/include/net/bluetooth/hci_core.h
>>> @@ -246,8 +246,17 @@ struct adv_pattern {
>>>      __u8 value[HCI_MAX_AD_LENGTH];
>>> };
>>> 
>>> +struct adv_rssi_thresholds {
>>> +     __s8 low_threshold;
>>> +     __s8 high_threshold;
>>> +     __u16 low_threshold_timeout;
>>> +     __u16 high_threshold_timeout;
>>> +     __u8 sampling_period;
>>> +};
>>> +
>>> struct adv_monitor {
>>>      struct list_head patterns;
>>> +     struct adv_rssi_thresholds rssi;
>>>      bool            active;
>>>      __u16           handle;
>>> };
>>> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
>>> index d8367850e8cd..dc534837be0e 100644
>>> --- a/include/net/bluetooth/mgmt.h
>>> +++ b/include/net/bluetooth/mgmt.h
>>> @@ -763,9 +763,18 @@ struct mgmt_adv_pattern {
>>>      __u8 value[31];
>>> } __packed;
>>> 
>>> +struct mgmt_adv_rssi_thresholds {
>>> +     __s8 high_threshold;
>>> +     __le16 high_threshold_timeout;
>>> +     __s8 low_threshold;
>>> +     __le16 low_threshold_timeout;
>>> +     __u8 sampling_period;
>>> +} __packed;
>>> +
>>> #define MGMT_OP_ADD_ADV_PATTERNS_MONITOR      0x0052
>>> struct mgmt_cp_add_adv_patterns_monitor {
>>>      __u8 pattern_count;
>>> +     struct mgmt_adv_rssi_thresholds rssi;
>>>      struct mgmt_adv_pattern patterns[];
>>> } __packed;
>> 
>> This is something we can not do. It breaks an userspace facing API. Is the mgmt opcode 0x0052 in an already released kernel?
> 
> Yes, the opcode does exist in an already released kernel.
> 
> The DBus method which accesses this API is put behind the experimental
> flag, therefore we expect they are flexible enough to support changes.
> Previously, we already had a discussion in an email thread with the
> title "Offload RSSI tracking to controller", and the outcome supports
> this change.
> 
> Here is an excerpt of the discussion.

it doesn’t matter. This is fixed API now and so we can not just change it. The argument above is void. What matters if it is in already released kernel.

Regards

Marcel
Archie Pusaka Dec. 4, 2020, 4:34 p.m. UTC | #4
Hi Marcel,

On Fri, 4 Dec 2020 at 17:51, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Archie,
>
> >>> MSFT needs rssi parameter for monitoring advertisement packet,
> >>> therefore we should supply them from mgmt.
> >>>
> >>> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> >>> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> >>> Reviewed-by: Yun-Hao Chung <howardchung@google.com>
> >>
> >> I don’t need any Reviewed-by if they are not catching an obvious user API breakage.
> >>
> >>> ---
> >>>
> >>> include/net/bluetooth/hci_core.h | 9 +++++++++
> >>> include/net/bluetooth/mgmt.h     | 9 +++++++++
> >>> net/bluetooth/mgmt.c             | 8 ++++++++
> >>> 3 files changed, 26 insertions(+)
> >>>
> >>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >>> index 9873e1c8cd16..42d446417817 100644
> >>> --- a/include/net/bluetooth/hci_core.h
> >>> +++ b/include/net/bluetooth/hci_core.h
> >>> @@ -246,8 +246,17 @@ struct adv_pattern {
> >>>      __u8 value[HCI_MAX_AD_LENGTH];
> >>> };
> >>>
> >>> +struct adv_rssi_thresholds {
> >>> +     __s8 low_threshold;
> >>> +     __s8 high_threshold;
> >>> +     __u16 low_threshold_timeout;
> >>> +     __u16 high_threshold_timeout;
> >>> +     __u8 sampling_period;
> >>> +};
> >>> +
> >>> struct adv_monitor {
> >>>      struct list_head patterns;
> >>> +     struct adv_rssi_thresholds rssi;
> >>>      bool            active;
> >>>      __u16           handle;
> >>> };
> >>> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> >>> index d8367850e8cd..dc534837be0e 100644
> >>> --- a/include/net/bluetooth/mgmt.h
> >>> +++ b/include/net/bluetooth/mgmt.h
> >>> @@ -763,9 +763,18 @@ struct mgmt_adv_pattern {
> >>>      __u8 value[31];
> >>> } __packed;
> >>>
> >>> +struct mgmt_adv_rssi_thresholds {
> >>> +     __s8 high_threshold;
> >>> +     __le16 high_threshold_timeout;
> >>> +     __s8 low_threshold;
> >>> +     __le16 low_threshold_timeout;
> >>> +     __u8 sampling_period;
> >>> +} __packed;
> >>> +
> >>> #define MGMT_OP_ADD_ADV_PATTERNS_MONITOR      0x0052
> >>> struct mgmt_cp_add_adv_patterns_monitor {
> >>>      __u8 pattern_count;
> >>> +     struct mgmt_adv_rssi_thresholds rssi;
> >>>      struct mgmt_adv_pattern patterns[];
> >>> } __packed;
> >>
> >> This is something we can not do. It breaks an userspace facing API. Is the mgmt opcode 0x0052 in an already released kernel?
> >
> > Yes, the opcode does exist in an already released kernel.
> >
> > The DBus method which accesses this API is put behind the experimental
> > flag, therefore we expect they are flexible enough to support changes.
> > Previously, we already had a discussion in an email thread with the
> > title "Offload RSSI tracking to controller", and the outcome supports
> > this change.
> >
> > Here is an excerpt of the discussion.
>
> it doesn’t matter. This is fixed API now and so we can not just change it. The argument above is void. What matters if it is in already released kernel.

If that is the case, do you have a suggestion to allow RSSI to be
considered when monitoring advertisement? Would a new MGMT opcode with
these parameters suffice?
#define MGMT_OP_ADD_ADV_PATTERNS_MONITOR_RSSI 0x005?
struct mgmt_cp_add_adv_patterns_monitor {
    __u8 pattern_count;
    struct mgmt_adv_rssi_thresholds rssi;
    struct mgmt_adv_pattern patterns[];
} __packed;

Thanks,
Archie
Marcel Holtmann Dec. 7, 2020, 9:56 a.m. UTC | #5
Hi Archie,

>>>>> MSFT needs rssi parameter for monitoring advertisement packet,
>>>>> therefore we should supply them from mgmt.
>>>>> 
>>>>> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
>>>>> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
>>>>> Reviewed-by: Yun-Hao Chung <howardchung@google.com>
>>>> 
>>>> I don’t need any Reviewed-by if they are not catching an obvious user API breakage.
>>>> 
>>>>> ---
>>>>> 
>>>>> include/net/bluetooth/hci_core.h | 9 +++++++++
>>>>> include/net/bluetooth/mgmt.h     | 9 +++++++++
>>>>> net/bluetooth/mgmt.c             | 8 ++++++++
>>>>> 3 files changed, 26 insertions(+)
>>>>> 
>>>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>>>>> index 9873e1c8cd16..42d446417817 100644
>>>>> --- a/include/net/bluetooth/hci_core.h
>>>>> +++ b/include/net/bluetooth/hci_core.h
>>>>> @@ -246,8 +246,17 @@ struct adv_pattern {
>>>>>     __u8 value[HCI_MAX_AD_LENGTH];
>>>>> };
>>>>> 
>>>>> +struct adv_rssi_thresholds {
>>>>> +     __s8 low_threshold;
>>>>> +     __s8 high_threshold;
>>>>> +     __u16 low_threshold_timeout;
>>>>> +     __u16 high_threshold_timeout;
>>>>> +     __u8 sampling_period;
>>>>> +};
>>>>> +
>>>>> struct adv_monitor {
>>>>>     struct list_head patterns;
>>>>> +     struct adv_rssi_thresholds rssi;
>>>>>     bool            active;
>>>>>     __u16           handle;
>>>>> };
>>>>> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
>>>>> index d8367850e8cd..dc534837be0e 100644
>>>>> --- a/include/net/bluetooth/mgmt.h
>>>>> +++ b/include/net/bluetooth/mgmt.h
>>>>> @@ -763,9 +763,18 @@ struct mgmt_adv_pattern {
>>>>>     __u8 value[31];
>>>>> } __packed;
>>>>> 
>>>>> +struct mgmt_adv_rssi_thresholds {
>>>>> +     __s8 high_threshold;
>>>>> +     __le16 high_threshold_timeout;
>>>>> +     __s8 low_threshold;
>>>>> +     __le16 low_threshold_timeout;
>>>>> +     __u8 sampling_period;
>>>>> +} __packed;
>>>>> +
>>>>> #define MGMT_OP_ADD_ADV_PATTERNS_MONITOR      0x0052
>>>>> struct mgmt_cp_add_adv_patterns_monitor {
>>>>>     __u8 pattern_count;
>>>>> +     struct mgmt_adv_rssi_thresholds rssi;
>>>>>     struct mgmt_adv_pattern patterns[];
>>>>> } __packed;
>>>> 
>>>> This is something we can not do. It breaks an userspace facing API. Is the mgmt opcode 0x0052 in an already released kernel?
>>> 
>>> Yes, the opcode does exist in an already released kernel.
>>> 
>>> The DBus method which accesses this API is put behind the experimental
>>> flag, therefore we expect they are flexible enough to support changes.
>>> Previously, we already had a discussion in an email thread with the
>>> title "Offload RSSI tracking to controller", and the outcome supports
>>> this change.
>>> 
>>> Here is an excerpt of the discussion.
>> 
>> it doesn’t matter. This is fixed API now and so we can not just change it. The argument above is void. What matters if it is in already released kernel.
> 
> If that is the case, do you have a suggestion to allow RSSI to be
> considered when monitoring advertisement? Would a new MGMT opcode with
> these parameters suffice?

its the only way.

Regards

Marcel
Archie Pusaka Dec. 7, 2020, 10:48 a.m. UTC | #6
Hi Marcel,

On Mon, 7 Dec 2020 at 17:57, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Archie,
>
> >>>>> MSFT needs rssi parameter for monitoring advertisement packet,
> >>>>> therefore we should supply them from mgmt.
> >>>>>
> >>>>> Signed-off-by: Archie Pusaka <apusaka@chromium.org>
> >>>>> Reviewed-by: Miao-chen Chou <mcchou@chromium.org>
> >>>>> Reviewed-by: Yun-Hao Chung <howardchung@google.com>
> >>>>
> >>>> I don’t need any Reviewed-by if they are not catching an obvious user API breakage.
> >>>>
> >>>>> ---
> >>>>>
> >>>>> include/net/bluetooth/hci_core.h | 9 +++++++++
> >>>>> include/net/bluetooth/mgmt.h     | 9 +++++++++
> >>>>> net/bluetooth/mgmt.c             | 8 ++++++++
> >>>>> 3 files changed, 26 insertions(+)
> >>>>>
> >>>>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >>>>> index 9873e1c8cd16..42d446417817 100644
> >>>>> --- a/include/net/bluetooth/hci_core.h
> >>>>> +++ b/include/net/bluetooth/hci_core.h
> >>>>> @@ -246,8 +246,17 @@ struct adv_pattern {
> >>>>>     __u8 value[HCI_MAX_AD_LENGTH];
> >>>>> };
> >>>>>
> >>>>> +struct adv_rssi_thresholds {
> >>>>> +     __s8 low_threshold;
> >>>>> +     __s8 high_threshold;
> >>>>> +     __u16 low_threshold_timeout;
> >>>>> +     __u16 high_threshold_timeout;
> >>>>> +     __u8 sampling_period;
> >>>>> +};
> >>>>> +
> >>>>> struct adv_monitor {
> >>>>>     struct list_head patterns;
> >>>>> +     struct adv_rssi_thresholds rssi;
> >>>>>     bool            active;
> >>>>>     __u16           handle;
> >>>>> };
> >>>>> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> >>>>> index d8367850e8cd..dc534837be0e 100644
> >>>>> --- a/include/net/bluetooth/mgmt.h
> >>>>> +++ b/include/net/bluetooth/mgmt.h
> >>>>> @@ -763,9 +763,18 @@ struct mgmt_adv_pattern {
> >>>>>     __u8 value[31];
> >>>>> } __packed;
> >>>>>
> >>>>> +struct mgmt_adv_rssi_thresholds {
> >>>>> +     __s8 high_threshold;
> >>>>> +     __le16 high_threshold_timeout;
> >>>>> +     __s8 low_threshold;
> >>>>> +     __le16 low_threshold_timeout;
> >>>>> +     __u8 sampling_period;
> >>>>> +} __packed;
> >>>>> +
> >>>>> #define MGMT_OP_ADD_ADV_PATTERNS_MONITOR      0x0052
> >>>>> struct mgmt_cp_add_adv_patterns_monitor {
> >>>>>     __u8 pattern_count;
> >>>>> +     struct mgmt_adv_rssi_thresholds rssi;
> >>>>>     struct mgmt_adv_pattern patterns[];
> >>>>> } __packed;
> >>>>
> >>>> This is something we can not do. It breaks an userspace facing API. Is the mgmt opcode 0x0052 in an already released kernel?
> >>>
> >>> Yes, the opcode does exist in an already released kernel.
> >>>
> >>> The DBus method which accesses this API is put behind the experimental
> >>> flag, therefore we expect they are flexible enough to support changes.
> >>> Previously, we already had a discussion in an email thread with the
> >>> title "Offload RSSI tracking to controller", and the outcome supports
> >>> this change.
> >>>
> >>> Here is an excerpt of the discussion.
> >>
> >> it doesn’t matter. This is fixed API now and so we can not just change it. The argument above is void. What matters if it is in already released kernel.
> >
> > If that is the case, do you have a suggestion to allow RSSI to be
> > considered when monitoring advertisement? Would a new MGMT opcode with
> > these parameters suffice?
>
> its the only way.

I will make the necessary changes. Thanks for the confirmation.

Regards,
Archie

>
> Regards
>
> Marcel
>
diff mbox series

Patch

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9873e1c8cd16..42d446417817 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -246,8 +246,17 @@  struct adv_pattern {
 	__u8 value[HCI_MAX_AD_LENGTH];
 };
 
+struct adv_rssi_thresholds {
+	__s8 low_threshold;
+	__s8 high_threshold;
+	__u16 low_threshold_timeout;
+	__u16 high_threshold_timeout;
+	__u8 sampling_period;
+};
+
 struct adv_monitor {
 	struct list_head patterns;
+	struct adv_rssi_thresholds rssi;
 	bool		active;
 	__u16		handle;
 };
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index d8367850e8cd..dc534837be0e 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -763,9 +763,18 @@  struct mgmt_adv_pattern {
 	__u8 value[31];
 } __packed;
 
+struct mgmt_adv_rssi_thresholds {
+	__s8 high_threshold;
+	__le16 high_threshold_timeout;
+	__s8 low_threshold;
+	__le16 low_threshold_timeout;
+	__u8 sampling_period;
+} __packed;
+
 #define MGMT_OP_ADD_ADV_PATTERNS_MONITOR	0x0052
 struct mgmt_cp_add_adv_patterns_monitor {
 	__u8 pattern_count;
+	struct mgmt_adv_rssi_thresholds rssi;
 	struct mgmt_adv_pattern patterns[];
 } __packed;
 #define MGMT_ADD_ADV_PATTERNS_MONITOR_SIZE	1
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 3dfed4efa078..5f0f03dcd81d 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4237,6 +4237,14 @@  static int add_adv_patterns_monitor(struct sock *sk, struct hci_dev *hdev,
 	INIT_LIST_HEAD(&m->patterns);
 	m->active = false;
 
+	m->rssi.low_threshold = cp->rssi.low_threshold;
+	m->rssi.low_threshold_timeout =
+	    __le16_to_cpu(cp->rssi.low_threshold_timeout);
+	m->rssi.high_threshold = cp->rssi.high_threshold;
+	m->rssi.high_threshold_timeout =
+	    __le16_to_cpu(cp->rssi.high_threshold_timeout);
+	m->rssi.sampling_period = cp->rssi.sampling_period;
+
 	for (i = 0; i < cp->pattern_count; i++) {
 		if (++mp_cnt > HCI_MAX_ADV_MONITOR_NUM_PATTERNS) {
 			err = mgmt_cmd_status(sk, hdev->id,