Re: [PATCH v3 4/5] Bluetooth: advmon offload MSFT handle controller reset
Hi Marcel,
I've sent a new v5 patch to address this issue.
Thanks,
Archie
On Tue, 22 Dec 2020 at 18:03, Marcel Holtmann wrote:
>
> Hi Archie,
>
> >>> When the controller is powered off, the registered advertising monitor
> >>> is removed from the controller. This patch handles the re-registration
> >>> of those monitors when the power is on.
> >>>
> >>> Signed-off-by: Archie Pusaka
> >>> Reviewed-by: Miao-chen Chou
> >>> Reviewed-by: Yun-Hao Chung
> >>>
> >>> ---
> >>>
> >>> (no changes since v1)
> >>>
> >>> net/bluetooth/msft.c | 79 +---
> >>> 1 file changed, 74 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> >>> index f5aa0e3b1b9b..7e33a85c3f1c 100644
> >>> --- a/net/bluetooth/msft.c
> >>> +++ b/net/bluetooth/msft.c
> >>> @@ -82,8 +82,15 @@ struct msft_data {
> >>> struct list_head handle_map;
> >>> __u16 pending_add_handle;
> >>> __u16 pending_remove_handle;
> >>> +
> >>> + struct {
> >>> + u8 reregistering:1;
> >>> + } flags;
> >>> };
> >>
> >> hmmm. Do you have bigger plans with this struct? I would just skip it.
> >>
> > This struct is also used in patch 5/5 to store the "enabled" status of
> > the filter.
> > Suspend/resume would need to enable/disable the filter, but it is not
> > yet implemented in this patch series.
>
> just do it without the nested structs. I think you are overdoing it here.
>
> Regards
>
> Marcel
>
Re: [PATCH v3 4/5] Bluetooth: advmon offload MSFT handle controller reset
Hi Archie,
>>> When the controller is powered off, the registered advertising monitor
>>> is removed from the controller. This patch handles the re-registration
>>> of those monitors when the power is on.
>>>
>>> Signed-off-by: Archie Pusaka
>>> Reviewed-by: Miao-chen Chou
>>> Reviewed-by: Yun-Hao Chung
>>>
>>> ---
>>>
>>> (no changes since v1)
>>>
>>> net/bluetooth/msft.c | 79 +---
>>> 1 file changed, 74 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
>>> index f5aa0e3b1b9b..7e33a85c3f1c 100644
>>> --- a/net/bluetooth/msft.c
>>> +++ b/net/bluetooth/msft.c
>>> @@ -82,8 +82,15 @@ struct msft_data {
>>> struct list_head handle_map;
>>> __u16 pending_add_handle;
>>> __u16 pending_remove_handle;
>>> +
>>> + struct {
>>> + u8 reregistering:1;
>>> + } flags;
>>> };
>>
>> hmmm. Do you have bigger plans with this struct? I would just skip it.
>>
> This struct is also used in patch 5/5 to store the "enabled" status of
> the filter.
> Suspend/resume would need to enable/disable the filter, but it is not
> yet implemented in this patch series.
just do it without the nested structs. I think you are overdoing it here.
Regards
Marcel
Re: [PATCH v3 4/5] Bluetooth: advmon offload MSFT handle controller reset
Hi Marcel,
On Mon, 21 Dec 2020 at 17:12, Marcel Holtmann wrote:
>
> Hi Archie,
>
> > When the controller is powered off, the registered advertising monitor
> > is removed from the controller. This patch handles the re-registration
> > of those monitors when the power is on.
> >
> > Signed-off-by: Archie Pusaka
> > Reviewed-by: Miao-chen Chou
> > Reviewed-by: Yun-Hao Chung
> >
> > ---
> >
> > (no changes since v1)
> >
> > net/bluetooth/msft.c | 79 +---
> > 1 file changed, 74 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> > index f5aa0e3b1b9b..7e33a85c3f1c 100644
> > --- a/net/bluetooth/msft.c
> > +++ b/net/bluetooth/msft.c
> > @@ -82,8 +82,15 @@ struct msft_data {
> > struct list_head handle_map;
> > __u16 pending_add_handle;
> > __u16 pending_remove_handle;
> > +
> > + struct {
> > + u8 reregistering:1;
> > + } flags;
> > };
>
> hmmm. Do you have bigger plans with this struct? I would just skip it.
>
This struct is also used in patch 5/5 to store the "enabled" status of
the filter.
Suspend/resume would need to enable/disable the filter, but it is not
yet implemented in this patch series.
Thanks,
Archie
Re: [PATCH v3 4/5] Bluetooth: advmon offload MSFT handle controller reset
Hi Archie,
> When the controller is powered off, the registered advertising monitor
> is removed from the controller. This patch handles the re-registration
> of those monitors when the power is on.
>
> Signed-off-by: Archie Pusaka
> Reviewed-by: Miao-chen Chou
> Reviewed-by: Yun-Hao Chung
>
> ---
>
> (no changes since v1)
>
> net/bluetooth/msft.c | 79 +---
> 1 file changed, 74 insertions(+), 5 deletions(-)
>
> diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
> index f5aa0e3b1b9b..7e33a85c3f1c 100644
> --- a/net/bluetooth/msft.c
> +++ b/net/bluetooth/msft.c
> @@ -82,8 +82,15 @@ struct msft_data {
> struct list_head handle_map;
> __u16 pending_add_handle;
> __u16 pending_remove_handle;
> +
> + struct {
> + u8 reregistering:1;
> + } flags;
> };
hmmm. Do you have bigger plans with this struct? I would just skip it.
Regards
Marcel
[PATCH v3 4/5] Bluetooth: advmon offload MSFT handle controller reset
From: Archie Pusaka
When the controller is powered off, the registered advertising monitor
is removed from the controller. This patch handles the re-registration
of those monitors when the power is on.
Signed-off-by: Archie Pusaka
Reviewed-by: Miao-chen Chou
Reviewed-by: Yun-Hao Chung
---
(no changes since v1)
net/bluetooth/msft.c | 79 +---
1 file changed, 74 insertions(+), 5 deletions(-)
diff --git a/net/bluetooth/msft.c b/net/bluetooth/msft.c
index f5aa0e3b1b9b..7e33a85c3f1c 100644
--- a/net/bluetooth/msft.c
+++ b/net/bluetooth/msft.c
@@ -82,8 +82,15 @@ struct msft_data {
struct list_head handle_map;
__u16 pending_add_handle;
__u16 pending_remove_handle;
+
+ struct {
+ u8 reregistering:1;
+ } flags;
};
+static int __msft_add_monitor_pattern(struct hci_dev *hdev,
+ struct adv_monitor *monitor);
+
bool msft_monitor_supported(struct hci_dev *hdev)
{
return !!(msft_get_features(hdev) & MSFT_FEATURE_MASK_LE_ADV_MONITOR);
@@ -134,6 +141,35 @@ static bool read_supported_features(struct hci_dev *hdev,
return false;
}
+/* This function requires the caller holds hdev->lock */
+static void reregister_monitor_on_restart(struct hci_dev *hdev, int handle)
+{
+ struct adv_monitor *monitor;
+ struct msft_data *msft = hdev->msft_data;
+ int err;
+
+ while (1) {
+ monitor = idr_get_next(>adv_monitors_idr, );
+ if (!monitor) {
+ /* All monitors have been reregistered */
+ msft->flags.reregistering = false;
+ hci_update_background_scan(hdev);
+ return;
+ }
+
+ msft->pending_add_handle = (u16)handle;
+ err = __msft_add_monitor_pattern(hdev, monitor);
+
+ /* If success, we return and wait for monitor added callback */
+ if (!err)
+ return;
+
+ /* Otherwise remove the monitor and keep registering */
+ hci_free_adv_monitor(hdev, monitor);
+ handle++;
+ }
+}
+
void msft_do_open(struct hci_dev *hdev)
{
struct msft_data *msft;
@@ -154,12 +190,18 @@ void msft_do_open(struct hci_dev *hdev)
INIT_LIST_HEAD(>handle_map);
hdev->msft_data = msft;
+
+ if (msft_monitor_supported(hdev)) {
+ msft->flags.reregistering = true;
+ reregister_monitor_on_restart(hdev, 0);
+ }
}
void msft_do_close(struct hci_dev *hdev)
{
struct msft_data *msft = hdev->msft_data;
struct msft_monitor_advertisement_handle_data *handle_data, *tmp;
+ struct adv_monitor *monitor;
if (!msft)
return;
@@ -169,6 +211,12 @@ void msft_do_close(struct hci_dev *hdev)
hdev->msft_data = NULL;
list_for_each_entry_safe(handle_data, tmp, >handle_map, list) {
+ monitor = idr_find(>adv_monitors_idr,
+ handle_data->mgmt_handle);
+
+ if (monitor && monitor->state == ADV_MONITOR_STATE_OFFLOADED)
+ monitor->state = ADV_MONITOR_STATE_REGISTERED;
+
list_del(_data->list);
kfree(handle_data);
}
@@ -282,9 +330,15 @@ static void msft_le_monitor_advertisement_cb(struct
hci_dev *hdev,
if (status && monitor)
hci_free_adv_monitor(hdev, monitor);
+ /* If in restart/reregister sequence, keep registering. */
+ if (msft->flags.reregistering)
+ reregister_monitor_on_restart(hdev,
+ msft->pending_add_handle + 1);
+
hci_dev_unlock(hdev);
- hci_add_adv_patterns_monitor_complete(hdev, status);
+ if (!msft->flags.reregistering)
+ hci_add_adv_patterns_monitor_complete(hdev, status);
}
static void msft_le_cancel_monitor_advertisement_cb(struct hci_dev *hdev,
@@ -374,7 +428,8 @@ static bool msft_monitor_pattern_valid(struct adv_monitor
*monitor)
}
/* This function requires the caller holds hdev->lock */
-int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor)
+static int __msft_add_monitor_pattern(struct hci_dev *hdev,
+ struct adv_monitor *monitor)
{
struct msft_cp_le_monitor_advertisement *cp;
struct msft_le_monitor_advertisement_pattern_data *pattern_data;
@@ -387,9 +442,6 @@ int msft_add_monitor_pattern(struct hci_dev *hdev, struct
adv_monitor *monitor)
u8 pattern_count = 0;
int err = 0;
- if (!msft)
- return -EOPNOTSUPP;
-
if (!msft_monitor_pattern_valid(monitor))
return -EINVAL;
@@ -434,6 +486,20 @@ int msft_add_monitor_pattern(struct hci_dev *hdev, struct
adv_monitor *monitor)
return err;
}
+/* This function requires the
