This patch moves whole packet filering logic of service discovery
into single place. It's done in preparation to simplify this logic.
Signed-off-by: Jakub Pawlowski <[email protected]>
---
net/bluetooth/mgmt.c | 183 +++++++++++++++++++++++++++------------------------
1 file changed, 96 insertions(+), 87 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 1e4635a..0a7f23e 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -7300,21 +7300,100 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
return;
}
- /* When using service discovery with a RSSI threshold, then check
- * if such a RSSI threshold is specified. If a RSSI threshold has
- * been specified, and HCI_QUIRK_STRICT_DUPLICATE_FILTER is not set,
- * then all results with a RSSI smaller than the RSSI threshold will be
- * dropped. If the quirk is set, let it through for further processing,
- * as we might need to restart the scan.
- *
- * For BR/EDR devices (pre 1.2) providing no RSSI during inquiry,
- * the results are also dropped.
- */
- if (hdev->discovery.rssi != HCI_RSSI_INVALID &&
- (rssi == HCI_RSSI_INVALID ||
- (rssi < hdev->discovery.rssi &&
- !test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks))))
- return;
+ if (hdev->discovery.rssi != HCI_RSSI_INVALID ||
+ hdev->discovery.uuid_count > 0) {
+ /* We are using service discovery */
+
+ /* If a RSSI threshold has been specified, and
+ * HCI_QUIRK_STRICT_DUPLICATE_FILTER is not set, then all
+ * results with a RSSI smaller than the RSSI threshold will be
+ * dropped. If the quirk is set, let it through for further
+ * processing, as we might need to restart the scan.
+ *
+ * For BR/EDR devices (pre 1.2) providing no RSSI during
+ * inquiry, the results are also dropped.
+ */
+ if (hdev->discovery.rssi != HCI_RSSI_INVALID &&
+ (rssi == HCI_RSSI_INVALID ||
+ (rssi < hdev->discovery.rssi &&
+ !test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER,
+ &hdev->quirks))))
+ return;
+
+ if (eir_len > 0) {
+ /* When using service discovery and a list of UUID is
+ * provided, results with no matching UUID should be
+ * dropped. In case there is a match the result is
+ * kept and checking possible scan response data
+ * will be skipped.
+ */
+ if (hdev->discovery.uuid_count > 0) {
+ match = eir_has_uuids(eir, eir_len,
+ hdev->discovery.uuid_count,
+ hdev->discovery.uuids);
+ /* If duplicate filtering does not report RSSI
+ * changes, then restart scanning to ensure
+ * updated result with updated RSSI values.
+ */
+ if (match && test_bit(
+ HCI_QUIRK_STRICT_DUPLICATE_FILTER,
+ &hdev->quirks))
+ restart_le_scan(hdev);
+ } else {
+ match = true;
+ }
+
+ if (!match && !scan_rsp_len)
+ return;
+ } else {
+ /* When using service discovery and a list of UUID is
+ * provided, results with empty EIR or advertising data
+ * should be dropped since they do not match any UUID.
+ */
+ if (hdev->discovery.uuid_count > 0 && !scan_rsp_len)
+ return;
+
+ match = false;
+ }
+
+ if (scan_rsp_len > 0) {
+ /* When using service discovery and a list of UUID is
+ * provided, results with no matching UUID should be
+ * dropped if there is no previous match from the
+ * advertising data.
+ */
+ if (hdev->discovery.uuid_count > 0) {
+ if (!match &&
+ !eir_has_uuids(scan_rsp, scan_rsp_len,
+ hdev->discovery.uuid_count,
+ hdev->discovery.uuids))
+ return;
+
+ /* If duplicate filtering does not report RSSI
+ * changes, then restart scanning to ensure
+ * updated result with updated RSSI values.
+ */
+ if (test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER,
+ &hdev->quirks))
+ restart_le_scan(hdev);
+ }
+ } else {
+ /* When using service discovery and a list of UUID is
+ * provided, results with empty scan response and no
+ * previous matched advertising data should be dropped.
+ */
+ if (hdev->discovery.uuid_count > 0 && !match)
+ return;
+ }
+
+ /* Validate the reported RSSI value against the RSSI threshold
+ * once more incase HCI_QUIRK_STRICT_DUPLICATE_FILTER forced a
+ * restart of LE scanning.
+ */
+ if (hdev->discovery.rssi != HCI_RSSI_INVALID &&
+ rssi < hdev->discovery.rssi)
+ return;
+ }
/* Make sure that the buffer is big enough. The 5 extra bytes
* are for the potential CoD field.
@@ -7341,87 +7420,17 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
ev->rssi = rssi;
ev->flags = cpu_to_le32(flags);
- if (eir_len > 0) {
- /* When using service discovery and a list of UUID is
- * provided, results with no matching UUID should be
- * dropped. In case there is a match the result is
- * kept and checking possible scan response data
- * will be skipped.
- */
- if (hdev->discovery.uuid_count > 0) {
- match = eir_has_uuids(eir, eir_len,
- hdev->discovery.uuid_count,
- hdev->discovery.uuids);
- /* If duplicate filtering does not report RSSI changes,
- * then restart scanning to ensure updated result with
- * updated RSSI values.
- */
- if (match && test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER,
- &hdev->quirks))
- restart_le_scan(hdev);
- } else {
- match = true;
- }
-
- if (!match && !scan_rsp_len)
- return;
-
+ if (eir_len > 0)
/* Copy EIR or advertising data into event */
memcpy(ev->eir, eir, eir_len);
- } else {
- /* When using service discovery and a list of UUID is
- * provided, results with empty EIR or advertising data
- * should be dropped since they do not match any UUID.
- */
- if (hdev->discovery.uuid_count > 0 && !scan_rsp_len)
- return;
-
- match = false;
- }
if (dev_class && !eir_has_data_type(ev->eir, eir_len, EIR_CLASS_OF_DEV))
eir_len = eir_append_data(ev->eir, eir_len, EIR_CLASS_OF_DEV,
dev_class, 3);
- if (scan_rsp_len > 0) {
- /* When using service discovery and a list of UUID is
- * provided, results with no matching UUID should be
- * dropped if there is no previous match from the
- * advertising data.
- */
- if (hdev->discovery.uuid_count > 0) {
- if (!match && !eir_has_uuids(scan_rsp, scan_rsp_len,
- hdev->discovery.uuid_count,
- hdev->discovery.uuids))
- return;
-
- /* If duplicate filtering does not report RSSI changes,
- * then restart scanning to ensure updated result with
- * updated RSSI values.
- */
- if (test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER,
- &hdev->quirks))
- restart_le_scan(hdev);
- }
-
+ if (scan_rsp_len > 0)
/* Append scan response data to event */
memcpy(ev->eir + eir_len, scan_rsp, scan_rsp_len);
- } else {
- /* When using service discovery and a list of UUID is
- * provided, results with empty scan response and no
- * previous matched advertising data should be dropped.
- */
- if (hdev->discovery.uuid_count > 0 && !match)
- return;
- }
-
- /* Validate the reported RSSI value against the RSSI threshold once more
- * incase HCI_QUIRK_STRICT_DUPLICATE_FILTER forced a restart of LE
- * scanning.
- */
- if (hdev->discovery.rssi != HCI_RSSI_INVALID &&
- rssi < hdev->discovery.rssi)
- return;
ev->eir_len = cpu_to_le16(eir_len + scan_rsp_len);
ev_size = sizeof(*ev) + eir_len + scan_rsp_len;
--
2.2.0.rc0.207.ga3a616c
On Wed, Mar 4, 2015 at 11:58 AM, Johan Hedberg <[email protected]> wrote:
> Hi Jakub,
>
> On Tue, Mar 03, 2015, Jakub Pawlowski wrote:
>> + if (hdev->discovery.uuid_count != 0 &&
>> + (eir_len == 0 || !eir_has_uuids(eir, eir_len,
>> + hdev->discovery.uuid_count,
>> + hdev->discovery.uuids)) &&
>> + (scan_rsp_len == 0 || !eir_has_uuids(scan_rsp, scan_rsp_len,
>> + hdev->discovery.uuid_count,
>> + hdev->discovery.uuids)))
>> + return;
>
> This if-statement needs to be simplified somehow, or split up, since
> right now it looks just too complex. You should at least be able to
> remove the '== 0' comparisons since eir_has_uuids() will directly return
> false then.
Good catch!
>
> Other than this I haven't found any other major issues with this set.
>
> One thing you might wanna consider is to do a bit more refactoring of
> this new big if-branch (i.e. move the content of it to separate
> function). This is something that can be done as an additional patch on
> top of this set. I realize the mgmt_device_found() is suffering of being
> quite complex already before your patches, but having the filtered_scan
> case now in a single branch opens the door to factoring it out into a
> separate function. As a bonus you'll also get a bit better readability
> due to having one less tab for indentation, i.e. not as frequent forced
> line breaks.
So I moved all filtering logic into separate funciton in first patch.
>
> Johan
Hi Jakub,
On Tue, Mar 03, 2015, Jakub Pawlowski wrote:
> This patch refactor code responsible for filtering when service
> discovery method is used.
>
> Signed-off-by: Jakub Pawlowski <[email protected]>
> ---
> net/bluetooth/mgmt.c | 95 ++++++++++++++--------------------------------------
> 1 file changed, 26 insertions(+), 69 deletions(-)
One thing I forgot to mention: please add some more meat to the bones
for this commit message. It's particularly important that you describe
*why* the patch is good. Anyone can with a bit of reading see that the
patch does some refactoring, but the reasoning for doing it it not quite
as clear (and will be even less so if we or anyone else looks at the
commit history 1+ years in the future).
The same applies for any other commit message where you feel you could
improve with these details.
Johan
Hi Jakub,
On Tue, Mar 03, 2015, Jakub Pawlowski wrote:
> + if (hdev->discovery.uuid_count != 0 &&
> + (eir_len == 0 || !eir_has_uuids(eir, eir_len,
> + hdev->discovery.uuid_count,
> + hdev->discovery.uuids)) &&
> + (scan_rsp_len == 0 || !eir_has_uuids(scan_rsp, scan_rsp_len,
> + hdev->discovery.uuid_count,
> + hdev->discovery.uuids)))
> + return;
This if-statement needs to be simplified somehow, or split up, since
right now it looks just too complex. You should at least be able to
remove the '== 0' comparisons since eir_has_uuids() will directly return
false then.
Other than this I haven't found any other major issues with this set.
One thing you might wanna consider is to do a bit more refactoring of
this new big if-branch (i.e. move the content of it to separate
function). This is something that can be done as an additional patch on
top of this set. I realize the mgmt_device_found() is suffering of being
quite complex already before your patches, but having the filtered_scan
case now in a single branch opens the door to factoring it out into a
separate function. As a bonus you'll also get a bit better readability
due to having one less tab for indentation, i.e. not as frequent forced
line breaks.
Johan
This patch fixes service discovery behaviour, when provided uuid filter
is empty and HCI_QUIRK_STRICT_DUPLICATE_FILTER is set. Before this
patch, empty uuid filter was unable to trigger scan restart, and that
caused inconsistent behaviour in applications.
Example: two DBus clients call BlueZ, one to find all devices with
service abcd, second to find all devices with rssi smaller than -90.
Sum of those filters, that is passed to mgmt_service_scan is empty
filter, with no rssi or uuids set.
That caused kernel not to restart scan when quirk was set.
That was inconsistent with what happen when there's only one of those
two filters set (scan is restarted and reports devices).
To fix that, new variable hdev->discovery.result_filtering was
introduced. It can indicate that filtered scan is running, no matter
what uuid or rssi filter is set.
Signed-off-by: Jakub Pawlowski <[email protected]>
---
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/mgmt.c | 7 +++----
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index acec914..15c761c 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -76,6 +76,7 @@ struct discovery_state {
u8 last_adv_data[HCI_MAX_AD_LENGTH];
u8 last_adv_data_len;
bool report_invalid_rssi;
+ bool result_filtering;
s8 rssi;
u16 uuid_count;
u8 (*uuids)[16];
@@ -525,6 +526,7 @@ static inline void discovery_init(struct hci_dev *hdev)
static inline void hci_discovery_filter_clear(struct hci_dev *hdev)
{
+ hdev->discovery.result_filtering = false;
hdev->discovery.report_invalid_rssi = true;
hdev->discovery.rssi = HCI_RSSI_INVALID;
hdev->discovery.uuid_count = 0;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 896ace7..0590bfe 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3933,8 +3933,7 @@ static void start_discovery_complete(struct hci_dev *hdev, u8 status,
*/
if (test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER,
&hdev->quirks) &&
- (hdev->discovery.uuid_count > 0 ||
- hdev->discovery.rssi != HCI_RSSI_INVALID)) {
+ hdev->discovery.result_filtering) {
hdev->discovery.scan_start = jiffies;
hdev->discovery.scan_duration = timeout;
}
@@ -4087,6 +4086,7 @@ static int start_service_discovery(struct sock *sk, struct hci_dev *hdev,
*/
hci_discovery_filter_clear(hdev);
+ hdev->discovery.result_filtering = true;
hdev->discovery.type = cp->type;
hdev->discovery.rssi = cp->rssi;
hdev->discovery.uuid_count = uuid_count;
@@ -7299,8 +7299,7 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
return;
}
- if (hdev->discovery.rssi != HCI_RSSI_INVALID ||
- hdev->discovery.uuid_count > 0) {
+ if (hdev->discovery.result_filtering) {
/* We are using service discovery */
/* If a RSSI threshold has been specified, and
--
2.2.0.rc0.207.ga3a616c
This patch refactor code responsible for filtering when service
discovery method is used.
Signed-off-by: Jakub Pawlowski <[email protected]>
---
net/bluetooth/mgmt.c | 95 ++++++++++++++--------------------------------------
1 file changed, 26 insertions(+), 69 deletions(-)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 0a7f23e..896ace7 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -7287,7 +7287,6 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
char buf[512];
struct mgmt_ev_device_found *ev = (void *) buf;
size_t ev_size;
- bool match;
/* Don't send events for a non-kernel initiated discovery. With
* LE one exception is if we have pend_le_reports > 0 in which
@@ -7320,79 +7319,37 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
&hdev->quirks))))
return;
- if (eir_len > 0) {
- /* When using service discovery and a list of UUID is
- * provided, results with no matching UUID should be
- * dropped. In case there is a match the result is
- * kept and checking possible scan response data
- * will be skipped.
- */
- if (hdev->discovery.uuid_count > 0) {
- match = eir_has_uuids(eir, eir_len,
- hdev->discovery.uuid_count,
- hdev->discovery.uuids);
- /* If duplicate filtering does not report RSSI
- * changes, then restart scanning to ensure
- * updated result with updated RSSI values.
- */
- if (match && test_bit(
- HCI_QUIRK_STRICT_DUPLICATE_FILTER,
- &hdev->quirks))
- restart_le_scan(hdev);
- } else {
- match = true;
- }
-
- if (!match && !scan_rsp_len)
- return;
- } else {
- /* When using service discovery and a list of UUID is
- * provided, results with empty EIR or advertising data
- * should be dropped since they do not match any UUID.
- */
- if (hdev->discovery.uuid_count > 0 && !scan_rsp_len)
- return;
+ /* If a list of UUID is provided, results with no matching UUID
+ * should be dropped. If list of UUID is not provided, treat
+ * all devices as matches.
+ * Empty UUID filter might be a result of merging filters
+ * somewhere in highter layer, and should behave same as when
+ * we have UUID match.
+ */
+ if (hdev->discovery.uuid_count != 0 &&
+ (eir_len == 0 || !eir_has_uuids(eir, eir_len,
+ hdev->discovery.uuid_count,
+ hdev->discovery.uuids)) &&
+ (scan_rsp_len == 0 || !eir_has_uuids(scan_rsp, scan_rsp_len,
+ hdev->discovery.uuid_count,
+ hdev->discovery.uuids)))
+ return;
- match = false;
- }
+ /* If duplicate filtering does not report RSSI changes,
+ * then restart scanning to ensure updated result with
+ * updated RSSI values.
+ */
+ if (test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER,
+ &hdev->quirks)) {
+ restart_le_scan(hdev);
- if (scan_rsp_len > 0) {
- /* When using service discovery and a list of UUID is
- * provided, results with no matching UUID should be
- * dropped if there is no previous match from the
- * advertising data.
- */
- if (hdev->discovery.uuid_count > 0) {
- if (!match &&
- !eir_has_uuids(scan_rsp, scan_rsp_len,
- hdev->discovery.uuid_count,
- hdev->discovery.uuids))
- return;
-
- /* If duplicate filtering does not report RSSI
- * changes, then restart scanning to ensure
- * updated result with updated RSSI values.
- */
- if (test_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER,
- &hdev->quirks))
- restart_le_scan(hdev);
- }
- } else {
- /* When using service discovery and a list of UUID is
- * provided, results with empty scan response and no
- * previous matched advertising data should be dropped.
+ /* Validate the reported RSSI value against the
+ * RSSI threshold once more.
*/
- if (hdev->discovery.uuid_count > 0 && !match)
+ if (hdev->discovery.rssi != HCI_RSSI_INVALID &&
+ rssi < hdev->discovery.rssi)
return;
}
-
- /* Validate the reported RSSI value against the RSSI threshold
- * once more incase HCI_QUIRK_STRICT_DUPLICATE_FILTER forced a
- * restart of LE scanning.
- */
- if (hdev->discovery.rssi != HCI_RSSI_INVALID &&
- rssi < hdev->discovery.rssi)
- return;
}
/* Make sure that the buffer is big enough. The 5 extra bytes
--
2.2.0.rc0.207.ga3a616c