Re: [PATCH v4 02/17] Bluetooth: hci_core: Introduce multi-adv inst list

[Florian Grandel <fgrandel@gmail.com>]

Hi Marcel,

thanks for reviewing my patches!

On 05/23/2015 11:25 PM, Marcel Holtmann wrote:
> Hi Florian,
>
>> The current hci dev structure only supports a single advertising
>> instance. To support multi-instance advertising it is necessary to
>> introduce a linked list of advertising instances so that multiple
>> advertising instances can be dynamically added and/or removed.
>>
>> In a first step, the existing adv_instance member of the hci_dev
>> struct is supplemented by a linked list of advertising instances.
>> This patch introduces the list and supporting list management
>> infrastructure. The list is not being used yet.
>>
>> Signed-off-by: Florian Grandel <fgrandel@gmail.com>
>> ---
>> include/net/bluetooth/hci_core.h |  16 ++++++
>> net/bluetooth/hci_core.c         | 112 +++++++++++++++++++++++++++++++++++++++
>> net/bluetooth/mgmt.c             |  10 ++--
>> 3 files changed, 133 insertions(+), 5 deletions(-)
>
> I dislike you intermixing userspace with kernel space patches. That makes it so hard to review. Keep them separate.

Ok, I'll provide two separate patch sets instead.

>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index a056c2b..36cc941 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -156,6 +156,9 @@ struct oob_data {
>> };
>>
>> struct adv_info {
>> +	struct list_head list;
>> +	struct hci_dev *hdev;
>
> Why is the a copy of hci_dev needed here?

We need this copy of hdev in adv_timeout_expired() - see further down in 
this patch.

Arman and I had a discussion about this on the list - and this was the 
solution we came up with. I'll forward you the thread so you don't have 
to search through the archives.

>> +	bool pending;
>
> What is this pending for. I do not see it being used in this patch.

It's required to make sure that we do not leak advertising instances 
when they have just been created but for some reason the 
add_advertising_complete() command encounters an error status. I'll move 
the introduction of the variable to the patch where it's actually being 
used to make it easier for you to review that part. In any case the 
function to look at is add_advertising_complete().

>> 	struct delayed_work timeout_exp;
>> 	__u8	instance;
>> 	__u32	flags;
>> @@ -166,6 +169,8 @@ struct adv_info {
>> 	__u8	scan_rsp_data[HCI_MAX_AD_LENGTH];
>> };
>>
>> +#define HCI_MAX_ADV_INSTANCES		1
>> +
>> #define HCI_MAX_SHORT_NAME_LENGTH	10
>>
>> /* Default LE RPA expiry time, 15 minutes */
>> @@ -374,6 +379,9 @@ struct hci_dev {
>> 	__u8			scan_rsp_data_len;
>>
>> 	struct adv_info		adv_instance;
>> +	struct list_head	adv_instances;
>> +	unsigned int		adv_instance_cnt;
>> +	__u8			cur_adv_instance;
>>
>> 	__u8			irk[16];
>> 	__u32			rpa_timeout;
>> @@ -1007,6 +1015,14 @@ int hci_add_remote_oob_data(struct hci_dev *hdev, bdaddr_t *bdaddr,
>> int hci_remove_remote_oob_data(struct hci_dev *hdev, bdaddr_t *bdaddr,
>> 			       u8 bdaddr_type);
>>
>> +void hci_adv_instances_clear(struct hci_dev *hdev);
>> +struct adv_info *hci_find_adv_instance(struct hci_dev *hdev, u8 instance);
>> +int hci_add_adv_instance(struct hci_dev *hdev, u8 instance, u32 flags,
>> +			 u16 adv_data_len, u8 *adv_data,
>> +			 u16 scan_rsp_len, u8 *scan_rsp_data,
>> +			 work_func_t timeout_work, u16 timeout);
>> +int hci_remove_adv_instance(struct hci_dev *hdev, u8 instance);
>> +
>> void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb);
>>
>> int hci_recv_frame(struct hci_dev *hdev, struct sk_buff *skb);
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 476709b..de00e21 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -2613,6 +2613,114 @@ int hci_add_remote_oob_data(struct hci_dev *hdev, bdaddr_t *bdaddr,
>> 	return 0;
>> }
>>
>> +/* This function requires the caller holds hdev->lock */
>> +struct adv_info *hci_find_adv_instance(struct hci_dev *hdev, u8 instance)
>> +{
>> +	struct adv_info *adv_instance;
>> +
>> +	list_for_each_entry(adv_instance, &hdev->adv_instances, list) {
>> +		if (adv_instance->instance == instance)
>> +			return adv_instance;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/* This function requires the caller holds hdev->lock */
>> +int hci_remove_adv_instance(struct hci_dev *hdev, u8 instance)
>> +{
>> +	struct adv_info *adv_instance;
>> +
>> +	adv_instance = hci_find_adv_instance(hdev, instance);
>> +	if (!adv_instance)
>> +		return -ENOENT;
>> +
>> +	BT_DBG("%s removing %dMR", hdev->name, instance);
>> +
>> +	if (adv_instance->timeout)
>> +		cancel_delayed_work(&adv_instance->timeout_exp);
>> +
>> +	list_del(&adv_instance->list);
>> +	kfree(adv_instance);
>> +
>> +	hdev->adv_instance_cnt--;
>> +
>> +	return 0;
>> +}
>> +
>> +/* This function requires the caller holds hdev->lock */
>> +void hci_adv_instances_clear(struct hci_dev *hdev)
>> +{
>> +	struct adv_info *adv_instance, *n;
>> +
>> +	list_for_each_entry_safe(adv_instance, n, &hdev->adv_instances, list) {
>> +		if (adv_instance->timeout)
>> +			cancel_delayed_work(&adv_instance->timeout_exp);
>> +
>> +		list_del(&adv_instance->list);
>> +		kfree(adv_instance);
>> +	}
>> +
>> +	hdev->adv_instance_cnt = 0;
>> +}
>> +
>> +/* This function requires the caller holds hdev->lock */
>> +int hci_add_adv_instance(struct hci_dev *hdev, u8 instance, u32 flags,
>> +			 u16 adv_data_len, u8 *adv_data,
>> +			 u16 scan_rsp_len, u8 *scan_rsp_data,
>> +			 work_func_t timeout_work, u16 timeout)
>> +{
>> +	struct adv_info *adv_instance;
>> +
>> +	adv_instance = hci_find_adv_instance(hdev, instance);
>> +	if (adv_instance) {
>> +		if (adv_instance->timeout)
>> +			cancel_delayed_work(&adv_instance->timeout_exp);
>> +
>> +		memset(adv_instance->adv_data, 0,
>> +		       sizeof(adv_instance->adv_data));
>> +		memset(adv_instance->scan_rsp_data, 0,
>> +		       sizeof(adv_instance->scan_rsp_data));
>> +	} else {
>> +		if (hdev->adv_instance_cnt >= HCI_MAX_ADV_INSTANCES)
>> +			return -EOVERFLOW;
>> +
>> +		adv_instance = kmalloc(sizeof(*adv_instance), GFP_KERNEL);
>> +		if (!adv_instance)
>> +			return -ENOMEM;
>> +
>> +		memset(adv_instance, 0, sizeof(*adv_instance));
>> +		adv_instance->hdev = hdev;
>> +		adv_instance->pending = true;
>> +		INIT_DELAYED_WORK(&adv_instance->timeout_exp, timeout_work);
>> +		adv_instance->instance = instance;
>> +		list_add(&adv_instance->list, &hdev->adv_instances);
>> +		hdev->adv_instance_cnt++;
>> +	}
>> +
>> +	adv_instance->flags = flags;
>> +	adv_instance->adv_data_len = adv_data_len;
>> +	adv_instance->scan_rsp_len = scan_rsp_len;
>> +
>> +	if (adv_data_len)
>> +		memcpy(adv_instance->adv_data, adv_data, adv_data_len);
>> +
>> +	if (scan_rsp_len)
>> +		memcpy(adv_instance->scan_rsp_data,
>> +		       scan_rsp_data, scan_rsp_len);
>> +
>> +	adv_instance->timeout = timeout;
>> +
>> +	if (timeout)
>> +		queue_delayed_work(hdev->workqueue,
>> +				   &adv_instance->timeout_exp,
>> +				   msecs_to_jiffies(timeout * 1000));
>> +
>> +	BT_DBG("%s for %dMR", hdev->name, instance);
>> +
>> +	return 0;
>> +}
>> +
>> struct bdaddr_list *hci_bdaddr_list_lookup(struct list_head *bdaddr_list,
>> 					 bdaddr_t *bdaddr, u8 type)
>> {
>> @@ -3016,6 +3124,8 @@ struct hci_dev *hci_alloc_dev(void)
>> 	hdev->manufacturer = 0xffff;	/* Default to internal use */
>> 	hdev->inq_tx_power = HCI_TX_POWER_INVALID;
>> 	hdev->adv_tx_power = HCI_TX_POWER_INVALID;
>> +	hdev->adv_instance_cnt = 0;
>> +	hdev->cur_adv_instance = 0x00;
>>
>> 	hdev->sniff_max_interval = 800;
>> 	hdev->sniff_min_interval = 80;
>> @@ -3057,6 +3167,7 @@ struct hci_dev *hci_alloc_dev(void)
>> 	INIT_LIST_HEAD(&hdev->pend_le_conns);
>> 	INIT_LIST_HEAD(&hdev->pend_le_reports);
>> 	INIT_LIST_HEAD(&hdev->conn_hash.list);
>> +	INIT_LIST_HEAD(&hdev->adv_instances);
>>
>> 	INIT_WORK(&hdev->rx_work, hci_rx_work);
>> 	INIT_WORK(&hdev->cmd_work, hci_cmd_work);
>> @@ -3250,6 +3361,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
>> 	hci_smp_ltks_clear(hdev);
>> 	hci_smp_irks_clear(hdev);
>> 	hci_remote_oob_data_clear(hdev);
>> +	hci_adv_instances_clear(hdev);
>> 	hci_bdaddr_list_clear(&hdev->le_white_list);
>> 	hci_conn_params_clear_all(hdev);
>> 	hci_discovery_filter_clear(hdev);
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index 7fd87e7..ce25b6d 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -6813,7 +6813,7 @@ static int read_adv_features(struct sock *sk, struct hci_dev *hdev,
>> 	rp->supported_flags = cpu_to_le32(supported_flags);
>> 	rp->max_adv_data_len = HCI_MAX_AD_LENGTH;
>> 	rp->max_scan_rsp_len = HCI_MAX_AD_LENGTH;
>> -	rp->max_instances = 1;
>> +	rp->max_instances = HCI_MAX_ADV_INSTANCES;
>>
>> 	/* Currently only one instance is supported, so simply return the
>> 	 * current instance number.
>> @@ -6916,11 +6916,11 @@ unlock:
>>
>> static void adv_timeout_expired(struct work_struct *work)
>> {
>> -	struct hci_dev *hdev = container_of(work, struct hci_dev,
>> -					    adv_instance.timeout_exp.work);
>> -
>> -	hdev->adv_instance.timeout = 0;
>> +	struct adv_info *adv_instance = container_of(work, struct adv_info,
>> +						     timeout_exp.work);
>> +	struct hci_dev *hdev = adv_instance->hdev;
>>
>> +	adv_instance->timeout = 0;
>> 	hci_dev_lock(hdev);
>> 	clear_adv_instance(hdev);
>> 	hci_dev_unlock(hdev);
>
> Regards
>
> Marcel
>
>

Cheers!

Florian