Re: [PATCH v2] Bluetooth: hci_core/mgmt: Change adv inst to list.

[Johan Hedberg <johan.hedberg@gmail.com>]

Hi Florian,

On Sun, Apr 05, 2015, Florian Grandel wrote:
> As a preparatory step towards multi-instance advertising it is
> necessary to introduce a data structure that supports storing
> multiple advertising info data structures for a bluetooth device.
> 
> This is introduced by refactoring the existing adv_instance member
> of the hci_dev struct into a linked list and making the necessary
> changes in the code to support this list.
> 
> Signed-off-by: Florian Grandel <fgrandel@gmail.com>
> ---
> 
> v1 -> v2: add missing braces in read_adv_features()
> 
>  include/net/bluetooth/hci_core.h |  21 ++-
>  net/bluetooth/hci_core.c         | 118 ++++++++++++-
>  net/bluetooth/mgmt.c             | 345 +++++++++++++++++++++------------------
>  3 files changed, 316 insertions(+), 168 deletions(-)

It'd be really good if you could try to split this patch up a bit. It
really is going over the limits of individual patch sizes that we're
comfortable with. See if you can split this up logically into at least
two, preferably more (3-4) patches while still keeping something that
compiles during each step.

> +			if (err == 0)
> +				advertising_removed(sk, hdev,
> +						    adv_instance->instance);
> +		}
> +	} else {
> +		err = hci_remove_adv_instance(hdev, instance);
> +		if (err == 0)
> +			advertising_removed(sk, hdev, instance);

I think if (!err) is the more consistent convension.

> +	err = hci_add_adv_instance(hdev, cp->instance, flags,
> +					 cp->adv_data_len, cp->data,
> +					 cp->scan_rsp_len,
> +					 cp->data + cp->adv_data_len,
> +					 adv_timeout_expired, timeout);

The split lines don't look like they're correctly lined up. They should
start from one column after the opening '(' on the first line.

> +	// TODO: Trigger an advertising added event even when instance
> +	// advertising is already switched on?

Please stick to /* ... */ for comments.

Johan