Hi Marcel,
On Sunday, 18 September 2016 16:28:11 CEST Marcel Holtmann wrote:
> Hi Szymon,
>
> > This patch enables appending local name to scan response data. If
> > currently advertised instance has name flag set it is expired
> > immediately.
> >
> > Signed-off-by: Michał Narajowski <michal.narajowski@xxxxxxxxxxx>
> > Signed-off-by: Szymon Janc <szymon.janc@xxxxxxxxxxx>
> > ---
> > net/bluetooth/hci_request.c | 28 +++++++++++++++++++--------
> > net/bluetooth/mgmt.c | 46
> > +++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 64
> > insertions(+), 10 deletions(-)
> >
> > diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> > index 9566ff8..0ce6cdd 100644
> > --- a/net/bluetooth/hci_request.c
> > +++ b/net/bluetooth/hci_request.c
> > @@ -971,14 +971,14 @@ void __hci_req_enable_advertising(struct hci_request
> > *req)>
> > hci_req_add(req, HCI_OP_LE_SET_ADV_ENABLE, sizeof(enable), &enable);
> >
> > }
> >
> > -static u8 create_default_scan_rsp_data(struct hci_dev *hdev, u8 *ptr)
> > +static u8 append_local_name(struct hci_dev *hdev, u8 *ptr, u8 ad_len)
> > {
> > - u8 ad_len = 0;
> >
> > size_t name_len;
> >
> > + int max_len;
> >
> > + max_len = HCI_MAX_AD_LENGTH - ad_len - 2;
> >
> > name_len = strlen(hdev->dev_name);
> >
> > - if (name_len > 0) {
> > - size_t max_len = HCI_MAX_AD_LENGTH - ad_len - 2;
> > + if (name_len > 0 && max_len > 0) {
> >
> > if (name_len > max_len) {
> >
> > name_len = max_len;
> >
> > @@ -997,22 +997,34 @@ static u8 create_default_scan_rsp_data(struct
> > hci_dev *hdev, u8 *ptr)>
> > return ad_len;
> >
> > }
> >
> > +static u8 create_default_scan_rsp_data(struct hci_dev *hdev, u8 *ptr)
> > +{
> > + return append_local_name(hdev, ptr, 0);
> > +}
> > +
> > static u8 create_instance_scan_rsp_data(struct hci_dev *hdev, u8 instance,
> >
> > u8 *ptr)
> >
> > {
> >
> > struct adv_info *adv_instance;
> >
> > + u32 instance_flags;
> > + u8 scan_rsp_len = 0;
> >
> > adv_instance = hci_find_adv_instance(hdev, instance);
> > if (!adv_instance)
> >
> > return 0;
> >
> > - /* TODO: Set the appropriate entries based on advertising instance
flags
> > - * here once flags other than 0 are supported.
> > - */
> > + instance_flags = adv_instance->flags;
> > +
> >
> > memcpy(ptr, adv_instance->scan_rsp_data,
> >
> > adv_instance->scan_rsp_len);
> >
> > - return adv_instance->scan_rsp_len;
> > + scan_rsp_len += adv_instance->scan_rsp_len;
> > + ptr += adv_instance->scan_rsp_len;
> > +
> > + if (instance_flags & MGMT_ADV_FLAG_LOCAL_NAME)
> > + scan_rsp_len = append_local_name(hdev, ptr, scan_rsp_len);
> > +
> > + return scan_rsp_len;
> > }
> >
> > void __hci_req_update_scan_rsp_data(struct hci_request *req, u8 instance)
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index 74179b9..fa1f1c7 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -3014,6 +3014,35 @@ static int user_passkey_neg_reply(struct sock *sk,
> > struct hci_dev *hdev,>
> > HCI_OP_USER_PASSKEY_NEG_REPLY, 0);
> >
> > }
> >
> > +static void adv_expire(struct hci_dev *hdev, u32 flags)
> > +{
> > + struct adv_info *adv_instance;
> > + struct hci_request req;
> > + int err;
> > +
> > + adv_instance = hci_find_adv_instance(hdev, hdev->cur_adv_instance);
> > + if (!adv_instance)
> > + return;
> > +
> > + /* stop if current instance doesn't need to be changed */
> > + if (!(adv_instance->flags & flags))
> > + return;
>
> are you sure this is correct? Isn't this !(current_flags ^ new_flags) what
> you are looking for.
I want to expire current advertising only if it has specific flag(s) set. Eg
if name is changed, only expire if current advertising has name flag, if
apperance is change, only expire if apprearance flag is set etc.
>
> Also we should really have mgmt-tester test cases for this. We want to make
> sure that this works for active instances and also the default instance 0
> (aka Set Advertising).
Yeap, will send updated mgmt-tester patches next week.
> > +
> > + cancel_adv_timeout(hdev);
> > +
> > + adv_instance = hci_get_next_instance(hdev, adv_instance->instance);
> > + if (!adv_instance)
> > + return;
> > +
> > + hci_req_init(&req, hdev);
> > + err = __hci_req_schedule_adv_instance(&req, adv_instance->instance,
> > + true);
> > + if (err)
> > + return;
> > +
> > + hci_req_run(&req, NULL);
> > +}
> > +
> > static void set_name_complete(struct hci_dev *hdev, u8 status, u16 opcode)
> > {
> >
> > struct mgmt_cp_set_local_name *cp;
> >
> > @@ -3029,13 +3058,17 @@ static void set_name_complete(struct hci_dev
> > *hdev, u8 status, u16 opcode)>
> > cp = cmd->param;
> >
> > - if (status)
> > + if (status) {
> >
> > mgmt_cmd_status(cmd->sk, hdev->id, MGMT_OP_SET_LOCAL_NAME,
> >
> > mgmt_status(status));
> >
> > - else
> > + } else {
> >
> > mgmt_cmd_complete(cmd->sk, hdev->id, MGMT_OP_SET_LOCAL_NAME, 0,
> >
> > cp, sizeof(*cp));
> >
> > + if (hci_dev_test_flag(hdev, HCI_LE_ADV))
> > + adv_expire(hdev, MGMT_ADV_FLAG_LOCAL_NAME);
> > + }
> > +
> >
> > mgmt_pending_remove(cmd);
> >
> > unlock:
> > @@ -5887,6 +5920,7 @@ static u32 get_supported_adv_flags(struct hci_dev
> > *hdev)>
> > flags |= MGMT_ADV_FLAG_DISCOV;
> > flags |= MGMT_ADV_FLAG_LIMITED_DISCOV;
> > flags |= MGMT_ADV_FLAG_MANAGED_FLAGS;
> >
> > + flags |= MGMT_ADV_FLAG_LOCAL_NAME;
> >
> > if (hdev->adv_tx_power != HCI_TX_POWER_INVALID)
> >
> > flags |= MGMT_ADV_FLAG_TX_POWER;
> >
> > @@ -5963,6 +5997,10 @@ static bool tlv_data_is_valid(struct hci_dev *hdev,
> > u32 adv_flags, u8 *data,>
> > tx_power_managed = true;
> > max_len -= 3;
> >
> > }
> >
> > + } else {
> > + /* at least 1 byte of name should fit in */
> > + if (adv_flags & MGMT_ADV_FLAG_LOCAL_NAME)
> > + max_len -= 3;
> >
> > }
> >
> > if (len > max_len)
> >
> > @@ -6295,6 +6333,10 @@ static u8 tlv_data_max_len(u32 adv_flags, bool
> > is_adv_data)>
> > if (adv_flags & MGMT_ADV_FLAG_TX_POWER)
> >
> > max_len -= 3;
> >
> > + } else {
> > + /* at least 1 byte of name should fit in */
> > + if (adv_flags & MGMT_ADV_FLAG_LOCAL_NAME)
> > + max_len -= 3;
> >
> > }
>
> So I do not get the math here. Either the short name will fit or the long
> name. Especially the Get Advertising Size Information command should return
> the accurate leftover size. I mean if the name utilizes all the space, then
> this should return 0.
>
> Also the short name is maximal 11 + 2 octets. Meaning it will always fit
> together with appearance no matter what. So if long name is too large, then
> we pick the short name and return the leftover space.
That was my thinking as well initially but...shortened name is never used for
HCI (only copied to mgmt events so I assumed this is legacy cruft).
What is done is that name is shortened if it doesn't fit (which make sense
since shortened name should always be full name substring). But seems like max
shortened name length is not respected eg for EIR shortened name is used if
name length is >48 chars....
I can work on that but I think this can be seprate patchset that would clean
all of above issues.
>
> Regards
>
> Marcel
--
pozdrawiam
Szymon Janc
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
