Hi Bastien, On Thu, Mar 30, 2023 at 3:08 PM Bastien Nocera <hadess@xxxxxxxxxx> wrote: > > Top posting to comment on the subject. It should have "[BlueZ PATCH]" > as the prefix so that the user-space CI runs on it. It should be fine in this case CI seem to have figured it out without it: https://patchwork.kernel.org/project/bluetooth/patch/20230330211855.102798-1-hdegoede@xxxxxxxxxx/ > On Thu, 2023-03-30 at 23:12 +0200, Hans de Goede wrote: > > discovery_filter_to_mgmt_cp() does not add > > discovery_filter.discoverable > > to the created mgmt_cp_start_service_discovery struct. > > > > Instead update_discovery_filter() seprately checks > > client->discovery_filter->discoverable for all clients. > > "separately". > > > This means that for discovery-filters which only have the > > discoverable > > flag set, to put the adapter in discoverable mode while discovering, > > the created mgmt_cp_start_service_discovery struct is empty. > > > > This empty mgmt_cp_start_service_discovery struct then gets send > > "sent" > > > to the kernel as part of a MGMT_OP_START_SERVICE_DISCOVERY msg > > by start_discovery_timeout(). > > > > This use of an empty filter with MGMT_OP_START_SERVICE_DISCOVERY > > causes some bluetooth devices to not get seen with some (most?) > > Broadcom bluetooth adapters. This problem has been observed with > > the following Broadcom models: BCM4343A0, BCM43430A1, BCM43341B0 . > > > > On these models the following 2 devices were not being discovered > > when starting a scan with a filter with just discoverable set > > in the filter (as gnome-bluetooth does): > > > > Device 09:02:01:03:0F:87 (public) > > Name: Bluetooth 3.0 Keyboard > > Alias: Bluetooth 3.0 Keyboard > > Class: 0x00000540 > > Icon: input-keyboard > > Paired: yes > > Bonded: yes > > Trusted: yes > > Blocked: no > > Connected: yes > > WakeAllowed: yes > > LegacyPairing: yes > > UUID: Service Discovery Serve.. (00001000-0000-1000-8000- > > 00805f9b34fb) > > UUID: Human Interface Device... (00001124-0000-1000-8000- > > 00805f9b34fb) > > UUID: PnP Information (00001200-0000-1000-8000- > > 00805f9b34fb) > > Modalias: bluetooth:v05ACp022Cd011B > > > > Device 00:60:D1:00:00:34 (public) > > Name: Bluetooth Mouse > > Alias: Bluetooth Mouse > > Class: 0x00002580 > > Icon: input-mouse > > Paired: yes > > Bonded: yes > > Trusted: yes > > Blocked: no > > Connected: yes > > WakeAllowed: yes > > LegacyPairing: no > > UUID: Human Interface Device... (00001124-0000-1000-8000- > > 00805f9b34fb) > > UUID: PnP Information (00001200-0000-1000-8000- > > 00805f9b34fb) > > Modalias: usb:v0103p0204d001E > > > > Since setting the discoverable flag on a filter only is a way to > > automatically put the adapter in discoverable mode itself while > > it is discovering; and since this does not any device filtering > > at all; modify merge_discovery_filters() to treat discovery with > > such filters as regular unfiltered discovery. > > > > This results in start_discovery_timeout() starting regular > > discovery through a MGMT_OP_START_DISCOVERY message and this > > fixes these 2 example devices not getting discovered by the > > mentioned Broadcom BT adapter models. > > > > Link: > > https://gitlab.gnome.org/GNOME/gnome-bluetooth/-/merge_requests/163 > > --- > > Note the same argument can be made for the pattern and duplicate part > > of > > the filters which also get handled outside of the kernel filter. > > But I prefer to keep the first patch small and targetted at solving > > things > > not working with the gnome-bluetooth filter settings. > > > > Also I'm not familiar enough with the code to say with certainty that > > filters with just a pattern or the duplicate flag set (or a > > combination) > > should also be treated as unfiltered discovery. > > --- > > src/adapter.c | 17 ++++++++++++++++- > > 1 file changed, 16 insertions(+), 1 deletion(-) > > > > diff --git a/src/adapter.c b/src/adapter.c > > index 7947160a6..cc7f891d9 100644 > > --- a/src/adapter.c > > +++ b/src/adapter.c > > @@ -2192,6 +2192,7 @@ static int merge_discovery_filters(struct > > btd_adapter *adapter, int *rssi, > > bool empty_uuid = false; > > bool has_regular_discovery = false; > > bool has_filtered_discovery = false; > > + uint8_t adapter_scan_type = get_scan_type(adapter); > > > > for (l = adapter->discovery_list; l != NULL; l = > > g_slist_next(l)) { > > struct discovery_client *client = l->data; > > @@ -2202,6 +2203,20 @@ static int merge_discovery_filters(struct > > btd_adapter *adapter, int *rssi, > > continue; > > } > > > > + /* > > + * Detect empty filter with only discoverable > > + * (which does not require a kernel filter) set. > > + */ > > + if (item->uuids == NULL && > > + item->pathloss == DISTANCE_VAL_INVALID && > > + item->rssi == DISTANCE_VAL_INVALID && > > + item->type == adapter_scan_type && > > + item->duplicate == false && > > + item->pattern == NULL) { > > I would have split this chunky "if" into a separate function, but > otherwise the logic looks good. > > Reviewed-by: Bastien Nocera <hadess@xxxxxxxxxx> > > > + has_regular_discovery = true; > > + continue; > > + } > > + > > has_filtered_discovery = true; > > > > *transport |= item->type; > > @@ -2251,7 +2266,7 @@ static int merge_discovery_filters(struct > > btd_adapter *adapter, int *rssi, > > * It there is both regular and filtered scan > > running, then > > * clear whole fitler to report all devices. > > */ > > - *transport = get_scan_type(adapter); > > + *transport = adapter_scan_type; > > *rssi = HCI_RSSI_INVALID; > > g_slist_free(*uuids); > > *uuids = NULL; > -- Luiz Augusto von Dentz
