Return-Path: Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 11.1 \(3445.4.7\)) Subject: Re: [RFC 5/9] Bluetooth: Process Adv-Set Terminate event From: Marcel Holtmann In-Reply-To: <1512374873-1956-6-git-send-email-jaganathx.kanakkassery@intel.com> Date: Fri, 8 Dec 2017 09:51:15 +0100 Cc: linux-bluetooth@vger.kernel.org, Jaganath Kanakkassery Message-Id: <7C08A17A-0E74-4AB8-9830-68F6C3EC0E64@holtmann.org> References: <1512374873-1956-1-git-send-email-jaganathx.kanakkassery@intel.com> <1512374873-1956-6-git-send-email-jaganathx.kanakkassery@intel.com> To: Jaganath Kanakkassery Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Jaganath, > This patch enables and process Advertising Set Terminate event. > This event can come mainly in two scenarios - Connection and > timeout. In case of connection - adv instance state will be > changed to disabled and in timeout - adv instance will be removed. > > Signed-off-by: Jaganath Kanakkassery > --- > include/net/bluetooth/hci.h | 8 ++++++ > net/bluetooth/hci_core.c | 6 +++++ > net/bluetooth/hci_event.c | 60 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 74 insertions(+) > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index 65d2124..2ee16f7 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -2115,6 +2115,14 @@ struct hci_ev_le_enh_conn_complete { > __u8 clk_accurancy; > } __packed; > > +#define HCI_EV_LE_ADV_SET_TERM 0x12 > +struct hci_ev_le_adv_set_term { > + __u8 status; > + __u8 handle; > + __le16 conn_handle; > + __u8 num_evts; > +} __packed; please get the formatting right here. > + > /* Internal events generated by Bluetooth stack */ > #define HCI_EV_STACK_INTERNAL 0xfd > struct hci_ev_stack_internal { > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index 6c22ed6..7f17325 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -689,6 +689,12 @@ static int hci_init3_req(struct hci_request *req, unsigned long opt) > if (hdev->commands[37] & 0x80) > events[1] |= 0x02; /* LE Enhanced conn complete */ > > + /* If the controller supports the LE Extended advertising > + * enable the corresponding event. Here we also have extra whitespaces. > + */ > + if (ext_adv_capable(hdev)) > + events[2] |= 0x02; /* LE Adv Set Terminated */ > + > hci_req_add(req, HCI_OP_LE_SET_EVENT_MASK, sizeof(events), > events); > > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index 64873dd..8a118c1 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -4879,6 +4879,62 @@ static void hci_le_enh_conn_complete_evt(struct hci_dev *hdev, > le16_to_cpu(ev->supervision_timeout)); > } > > +static void hci_le_adv_set_terminated_evt(struct hci_dev *hdev, > + struct sk_buff *skb) > +{ > + struct hci_ev_le_adv_set_term *ev = (void *) skb->data; > + struct adv_info *adv_instance = NULL; > + int err; > + > + hci_dev_lock(hdev); > + > + if (ev->handle) { > + adv_instance = hci_find_adv_instance(hdev, ev->handle); > + if (!adv_instance) > + goto unlock; > + } > + > + /* If this is because of connection */ > + if (!ev->status) { > + if (!ev->handle) > + hci_dev_clear_flag(hdev, HCI_LE_ADV); > + else > + clear_bit(ADV_INST_ENABLED, &adv_instance->state); This needs to be done all super carefully since we now will have multiple active advertising sets and connections can happen on more than one. So just clearing LE_ADV flag is dangerous. > + } else if (ev->handle) { > + /* Remove the instance in all other cases */ > + err = hci_remove_adv_instance(hdev, ev->handle); > + if (!err) > + mgmt_advertising_removed(NULL, hdev, ev->handle); > + } > + > + /* If instance 0 was advertiing then others would be already disabled */ > + if (!ev->handle) > + goto unlock; > + > + /* Check state of other instances and modify flags accordingly */ > + list_for_each_entry(adv_instance, &hdev->adv_instances, list) { > + if (test_bit(ADV_INST_ENABLED, &adv_instance->state)) { > + if (!ev->status) { > + struct hci_request req; > + > + /* If it is a connection and another instance > + * is enabled then disable all, to be > + * alligned with the existing design > + */ > + hci_req_init(&req, hdev); > + __hci_req_stop_ext_adv(&req, 0, true); > + hci_req_run(&req, NULL); > + } > + goto unlock; > + } > + } > + > + hci_dev_clear_flag(hdev, HCI_LE_ADV); > + > +unlock: > + hci_dev_unlock(hdev); > +} > + > static void hci_le_conn_update_complete_evt(struct hci_dev *hdev, > struct sk_buff *skb) > { > @@ -5492,6 +5548,10 @@ static void hci_le_meta_evt(struct hci_dev *hdev, struct sk_buff *skb) > hci_le_enh_conn_complete_evt(hdev, skb); > break; > > + case HCI_EV_LE_ADV_SET_TERM: > + hci_le_adv_set_terminated_evt(hdev, skb); > + break; > + > default: > break; > } I have the feeling we really need to store the connection handle in the instance data. So that when we get the connection complete event that we can clearly identify where things belong to. So dies the connection complete or set terminated comes first. I think that btmon traces in the commit messages would be good here. Regards Marcel