Re: [PATCH v2 1/2] Bluetooth: Introduce HCI_CONN_FLAG_DEVICE_PRIVACY device flag

Hi Luiz,

quoted

quoted

This introduces HCI_CONN_FLAG_DEVICE_PRIVACY which can be used by
userspace to indicate to the controller to use Device Privacy Mode to a
specific device.

Signed-off-by: Luiz Augusto von Dentz <redacted>
---
v2: Fix marking Device Privacy Flag even when adapter is not capable of
handling Set Privacy Mode.

include/net/bluetooth/hci_core.h |  4 ++++
net/bluetooth/mgmt.c             | 24 ++++++++++++++++++++----
2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index b5f061882c10..07d2d099dc2a 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h

@@ -160,6 +160,7 @@ struct bdaddr_list_with_flags {

enum hci_conn_flags {
     HCI_CONN_FLAG_REMOTE_WAKEUP,
+     HCI_CONN_FLAG_DEVICE_PRIVACY,
     HCI_CONN_FLAG_MAX
};

@@ -1468,6 +1469,9 @@ void hci_conn_del_sysfs(struct hci_conn *conn);

#define use_ll_privacy(dev) (ll_privacy_capable(dev) && \
                          hci_dev_test_flag(dev, HCI_ENABLE_LL_PRIVACY))

+#define privacy_mode_capable(dev) (use_ll_privacy(dev) && \
+                                (hdev->commands[39] & 0x04))
+
/* Use enhanced synchronous connection if command is supported */
#define enhanced_sco_capable(dev) ((dev)->commands[29] & 0x08)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 06384d761928..8a8376d32be3 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c

@@ -4350,7 +4350,16 @@ static int set_exp_feature(struct sock *sk, struct hci_dev *hdev,
                            MGMT_STATUS_NOT_SUPPORTED);

}

-#define SUPPORTED_DEVICE_FLAGS() ((1U << HCI_CONN_FLAG_MAX) - 1)
+static u32 supported_device_flags(struct hci_dev *hdev)
+{
+     u32 flags = BIT(HCI_CONN_FLAG_MAX) - 1;
+
+     /* Check if Privacy Mode can be set */
+     if (!privacy_mode_capable(hdev))
+             flags &= ~BIT(HCI_CONN_FLAG_DEVICE_PRIVACY);
+
+     return flags;
+}

I am lost on what we are doing, I know that SUPPORTED_DEVICE_FLAGS was introduced by 4c54bf2b093bb from Abhishek, but I fail to reason now why it is correct.

But we don't set the HCI_CONN_FLAG_DEVICE_PRIVACY is hdev is not
capable of setting it, anyway the general idea of the supported_flags
is to indicate to userspace what flags the kernel is capable of
settings, so yeah I would expected it to be capable of setting every
flag except for those the controller don't have proper support for.
Maybe you got confused by the logic of first enabling everything and
then toggle back the bits that are not supported by the hdev.

and I think that is wrong. We should only set the bits that the hardware and/or kernel supports. I have no idea on how I missed this in the review back then. I am trying to figure it out, but I am failing to follow this logic.

Regards

Marcel