Hi Luiz,
>>> 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 <luiz.von.dentz@xxxxxxxxx>
>>> ---
>>> 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
