Hi Johan,
> To avoid too many events being sent to user space and to help parsing of
> all available remote device data it makes sense for us to wait for the
> scan response and send a single merged Device Found event to user space.
>
> This patch adds a new hdev->pending_adv cache which gets allocated
> whenever active scanning gets started and freed when scanning stops. The
> cache is able to hold a single advertising report and is operated upon
> each time we get an advertising report event.
>
> Normally we simply merge scan responses into data in the cache that was
> gotten from a previous adv_ind or adv_direct_ind report. Exceptions are
> if we don't get a matching scan response (or no scan response at all) or
> if there is not enough space in the cache, in which case we force
> sending of any data that there is before continuing further.
>
> Signed-off-by: Johan Hedberg <johan.hedberg@xxxxxxxxx>
> ---
> include/net/bluetooth/hci_core.h | 10 ++++++
> net/bluetooth/hci_core.c | 3 ++
> net/bluetooth/hci_event.c | 74 ++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 85 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 5f8bc05694ac..323f1e64e299 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -148,9 +148,18 @@ struct amp_assoc {
> __u8 data[HCI_MAX_AMP_ASSOC_SIZE];
> };
>
> +struct hci_adv_ind {
> + bdaddr_t bdaddr;
> + u8 bdaddr_type;
> + s8 rssi;
> + u8 len;
> + u8 data[256];
I think this should be HCI_MAX_AD_LENGTH instead of 256.
> +};
> +
> #define HCI_MAX_PAGES 3
>
> #define NUM_REASSEMBLY 4
> +
> struct hci_dev {
> struct list_head list;
> struct mutex lock;
> @@ -282,6 +291,7 @@ struct hci_dev {
>
> struct crypto_blkcipher *tfm_aes;
>
> + struct hci_adv_ind *pending_adv;
> struct discovery_state discovery;
so my idea would have been to include this into the discovery_state structure.
@@ -68,6 +68,10 @@ struct discovery_state {
struct list_head unknown; /* Name state not known */
struct list_head resolve; /* Name needs to be resolved */
__u32 timestamp;
+ bdaddr_t last_adv_addr;
+ u8 last_adv_addr_type;
+ u8 last_adv_data[HCI_MAX_AD_LENGTH];
+ u8 last_adv_data_len;
};
That is what I was thinking about. I left the RSSI value out since in case there is no scan response we might better send it without RSSI value since the report would be delayed. However we can also store it and use it.
> struct hci_conn_hash conn_hash;
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 1c6ffaa8902f..a8e225899e6a 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2437,6 +2437,9 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> kfree_skb(hdev->recv_evt);
> hdev->recv_evt = NULL;
>
> + kfree(hdev->pending_adv);
> + hdev->pending_adv = NULL;
> +
> /* After this point our queues are empty
> * and no tasks are scheduled. */
> hdev->close(hdev);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 403c1d96331a..f35a398e80a9 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1018,6 +1018,12 @@ static void hci_cc_le_set_scan_param(struct hci_dev *hdev, struct sk_buff *skb)
> hci_dev_unlock(hdev);
> }
>
> +#define ADV_CACHE_EMPTY(p) (!bacmp(&(p)->bdaddr, BDADDR_ANY))
> +#define ADV_CACHE_CLEAR(p) do { \
> + bacpy(&(p)->bdaddr, BDADDR_ANY); \
> + (p)->len = 0; \
> + } while (0)
> +
> static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
> struct sk_buff *skb)
> {
> @@ -1036,9 +1042,31 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
> switch (cp->enable) {
> case LE_SCAN_ENABLE:
> set_bit(HCI_LE_SCAN, &hdev->dev_flags);
> +
> + if (hdev->le_scan_type != LE_SCAN_ACTIVE)
> + break;
> +
> + /* Allocate buffer for merging ADV_IND and SCAN_RSP */
> + if (!hdev->pending_adv)
> + hdev->pending_adv = kzalloc(sizeof(*hdev->pending_adv),
> + GFP_KERNEL);
> +
So instead of allocating it, sticking it into discovery_state seems better and in the end a lot simpler.
> break;
>
> case LE_SCAN_DISABLE:
> + if (hdev->pending_adv) {
> + struct hci_adv_ind *p = hdev->pending_adv;
> +
> + if (!ADV_CACHE_EMPTY(p))
> + mgmt_device_found(hdev, &p->bdaddr, LE_LINK,
> + p->bdaddr_type, NULL,
> + p->rssi, 0, 1, p->data,
> + p->len);
> +
> + hdev->pending_adv = NULL;
> + kfree(p);
> + }
> +
I wonder if this should not all be centralized in the discovery set_state function.
> /* Cancel this timer so that we don't try to disable scanning
> * when it's already disabled.
> */
> @@ -3944,6 +3972,8 @@ static void check_pending_le_conn(struct hci_dev *hdev, bdaddr_t *addr,
> static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
> u8 bdaddr_type, s8 rssi, u8 *data, u8 len)
> {
> + struct hci_adv_ind *p = hdev->pending_adv;
> +
> /* Passive scanning shouldn't trigger any device found events */
> if (hdev->le_scan_type == LE_SCAN_PASSIVE) {
> if (type == LE_ADV_IND || type == LE_ADV_DIRECT_IND)
> @@ -3951,8 +3981,48 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr,
> return;
> }
>
> - mgmt_device_found(hdev, bdaddr, LE_LINK, bdaddr_type, NULL, rssi, 0, 1,
> - data, len);
> + /* If we don't have a cache, or we don't have data in the cache
> + * and we got a scan response just send the mgmt event without
> + * doing anything else.
> + */
> + if (!p || (ADV_CACHE_EMPTY(p) && type == LE_ADV_SCAN_RSP)) {
> + mgmt_device_found(hdev, bdaddr, LE_LINK, bdaddr_type, NULL,
> + rssi, 0, 1, data, len);
> + return;
> + }
> +
> + /* If there's nothing cached proceed to updating the cache */
> + if (ADV_CACHE_EMPTY(p))
> + goto update_cache;
> +
> + /* If the cached data doesn't match this report or there is not
> + * enough space in the cache (unlikely but we have to check for
> + * it to avoid buffer overflow) then force sending of the cached
> + * data.
> + */
> + if (bacmp(bdaddr, &p->bdaddr) || bdaddr_type != p->bdaddr_type ||
> + len > (sizeof(p->data) - p->len)) {
> + mgmt_device_found(hdev, &p->bdaddr, LE_LINK, p->bdaddr_type,
> + NULL, p->rssi, 0, 1, p->data, p->len);
> + ADV_CACHE_CLEAR(p);
> + }
> +
> +update_cache:
> + /* Update the cache with data from this report */
> + bacpy(&p->bdaddr, bdaddr);
> + p->bdaddr_type = bdaddr_type;
> + p->rssi = rssi;
> + memcpy(p->data + p->len, data, len);
> + p->len += len;
> +
> + /* If this report will not trigger a scan request just send the
> + * device found event without doing anything else.
> + */
> + if (type != LE_ADV_IND && type != LE_ADV_DIRECT_IND) {
s/LE_ADV_DIRECT_IND/LE_ADV_SCAN_IND/
> + mgmt_device_found(hdev, bdaddr, LE_LINK, bdaddr_type, NULL,
> + rssi, 0, 1, p->data, p->len);
> + ADV_CACHE_CLEAR(p);
> + }
> }
Regards
Marcel
--
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
