| Message ID | 20201216123317.v3.4.I215b0904cb68d68ac780a0c75c06f7d12e6147b7@changeid |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/5] Bluetooth: advmon offload MSFT add rssi support | expand |
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 <apusaka@chromium.org> > Reviewed-by: Miao-chen Chou <mcchou@chromium.org> > Reviewed-by: Yun-Hao Chung <howardchung@google.com> > > --- > > (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
Hi Marcel, On Mon, 21 Dec 2020 at 17:12, Marcel Holtmann <marcel@holtmann.org> 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 <apusaka@chromium.org> > > Reviewed-by: Miao-chen Chou <mcchou@chromium.org> > > Reviewed-by: Yun-Hao Chung <howardchung@google.com> > > > > --- > > > > (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
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 <apusaka@chromium.org> >>> Reviewed-by: Miao-chen Chou <mcchou@chromium.org> >>> Reviewed-by: Yun-Hao Chung <howardchung@google.com> >>> >>> --- >>> >>> (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
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 <marcel@holtmann.org> 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 <apusaka@chromium.org> > >>> Reviewed-by: Miao-chen Chou <mcchou@chromium.org> > >>> Reviewed-by: Yun-Hao Chung <howardchung@google.com> > >>> > >>> --- > >>> > >>> (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 >
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(&hdev->adv_monitors_idr, &handle); + 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(&msft->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, &msft->handle_map, list) { + monitor = idr_find(&hdev->adv_monitors_idr, + handle_data->mgmt_handle); + + if (monitor && monitor->state == ADV_MONITOR_STATE_OFFLOADED) + monitor->state = ADV_MONITOR_STATE_REGISTERED; + list_del(&handle_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 caller holds hdev->lock */ +int msft_add_monitor_pattern(struct hci_dev *hdev, struct adv_monitor *monitor) +{ + struct msft_data *msft = hdev->msft_data; + + if (!msft) + return -EOPNOTSUPP; + + if (msft->flags.reregistering) + return -EBUSY; + + return __msft_add_monitor_pattern(hdev, monitor); +} + /* This function requires the caller holds hdev->lock */ int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor, u16 handle) @@ -447,6 +513,9 @@ int msft_remove_monitor(struct hci_dev *hdev, struct adv_monitor *monitor, if (!msft) return -EOPNOTSUPP; + if (msft->flags.reregistering) + return -EBUSY; + handle_data = msft_find_handle_data(hdev, monitor->handle, true); /* If no matched handle, just remove without telling controller */