Re: [PATCH v3 2/2] Bluetooth: mgmt: Start using multi-adv inst list

[Florian Grandel <fgrandel@gmail.com>]

Hi Arman,

to answer your remaining review questions...

>> @@ -1044,11 +1047,13 @@ static u8 create_instance_adv_data(struct hci_dev *hdev, u8 instance, u8 *ptr)
>>          }
>>
>>          if (instance) {
>
> Maybe we should do this check earlier and just return 0 if instance
> doesn't exist. This code worked before since the only valid instances
> were 0x00 and 0x01. Now you may need to do a look up earlier and
> return 0 if instance is non-zero and hci_find_adv_instance returns
> NULL.
>

Agreed. I'm checking for this condition early on now and return 0 when 
the current instance identifier is invalid for some reason.

>> -static void clear_adv_instance(struct hci_dev *hdev)
>> +static void clear_adv_instance(struct sock *sk, struct hci_dev *hdev,
>> +                              u8 instance)
>>   {
>> -       struct hci_request req;
>> -
>> -       if (!hci_dev_test_flag(hdev, HCI_ADVERTISING_INSTANCE))
>> -               return;
>> -
>> -       if (hdev->adv_instance.timeout)
>> -               cancel_delayed_work(&hdev->adv_instance.timeout_exp);
>> -
>> -       memset(&hdev->adv_instance, 0, sizeof(hdev->adv_instance));
>> -       advertising_removed(NULL, hdev, 1);
>> -       hci_dev_clear_flag(hdev, HCI_ADVERTISING_INSTANCE);
>> -
>> -       if (!hdev_is_powered(hdev) ||
>> -           hci_dev_test_flag(hdev, HCI_ADVERTISING))
>> -               return;
>> +       struct adv_info *adv_instance, *n;
>> +       int err;
>>
>> -       hci_req_init(&req, hdev);
>> -       disable_advertising(&req);
>> -       hci_req_run(&req, NULL);
>
> Why did you remove this logic? Advertising needs to be disabled if
> HCI_ADVERTISING_INSTANCE is set and HCI_ADVERTISING wasn't. Hence most
> of the logic above (within this function) is still needed.

This was a serious oversight on my side. My intention was to deduplicate 
code in remove_advertising(). While this is probably a good idea, I just 
didn't implement it correctly. For the moment being I removed the 
refactoring. I'll propose an improved patch later on. Just not now - my 
change set is large enough without that change. ;-)

>
>> +       /* A value of 0 indicates that all instances should be cleared. */
>> +       if (instance == 0x00) {
>> +               list_for_each_entry_safe(adv_instance, n,
>> +                                        &hdev->adv_instances, list) {
>
> Didn't you add a hci_adv_instances_clear for this purpose? Now you
> have nested loops for iterating through the list and doing the lookup.
> If you've added this loop just to send the removed event, then perhaps
> it makes more sense to make advertising_removed public (like
> mgmt_advertising_removed) and to call it from hci.c. Or, just do a
> loop first to send the removed events and then call
> hci_adv_instances_clear.

I implemented the latter proposal.

>> @@ -4697,11 +4694,14 @@ static void set_advertising_complete(struct hci_dev *hdev, u8 status,
>>           * set up earlier, then enable the advertising instance.
>>           */
>>          if (hci_dev_test_flag(hdev, HCI_ADVERTISING) ||
>> -           !hci_dev_test_flag(hdev, HCI_ADVERTISING_INSTANCE))
>> +           !hci_dev_test_flag(hdev, HCI_ADVERTISING_INSTANCE) ||
>> +           list_empty(&hdev->adv_instances))
>
> We should make sure that the HCI_ADVERTISING_INSTANCE setting is set
> as long as hdev->adv_instances is non-empty and it's not set if the
> list is empty.

I introduced a TODO for that as there are several other similar cases in 
the code. The code currently still relies on HCI_MAX_ADV_INSTANCES being 
exactly one and should work correctly in that case. All places in the 
code that need tweaking for real multi-adv support are marked with TODOs 
now.

>
>> @@ -6882,24 +6883,37 @@ static void add_advertising_complete(struct hci_dev *hdev, u8 status,
>>          if (status) {
>> +               /* TODO: Start advertising another adv instance if any? */
>>                  hci_dev_clear_flag(hdev, HCI_ADVERTISING_INSTANCE);
>> -               memset(&hdev->adv_instance, 0, sizeof(hdev->adv_instance));
>> -               advertising_removed(cmd ? cmd->sk : NULL, hdev, 1);
>> +
>> +               if (cmd) {
>
> If "cmd" is NULL for whatever reason, then we'll end up leaking the
> instance. Maybe there is a better way to propagate the pending
> instance ID?

I fixed that by introducing a "pending" flag in the adv_info struct. 
This also catches other cases, e.g. when no new instance has been 
introduced.


>> @@ -6981,38 +7015,31 @@ static int add_advertising(struct sock *sk, struct hci_dev *hdev,
>>                  goto unlock;
>>          }
>>
>> +       /* TODO: Trigger an advertising added event even when instance
>> +        * advertising is already switched on?
>> +        */
>
> With a single instance, what this prevents is sending an "added" event
> for an instance that was previously added. So the TODO doesn't make
> sense in that context but the new code needs to correctly abide by
> that logic. What you actually need to pay attention to is to not send
> any HCI commands to update advertising data every time you add a new
> instance. So maybe add a TODO for that.

I obviously didn't understand the purpose of this code. I refactored the 
code to make it work in the way you propose at least for one instance 
and introduced a TODO for the real multi-adv case.

> The patch is still a bit too large
> for linux-bluetooth standards and it's a bit difficult to get through,
> so if you can break it down into smaller logical pieces it will be
> easier for the others to review it.

I now split up the code in much more manageable pieces. This comes at 
the cost, though, that e2e-tests (mgmt-tester) won't run any more if you 
take away parts of the change set. I asked the question on the list, 
whether that was ok, but this was probably overlooked. I assume, though, 
that such a thing is valid as otherwise there would be no means to 
introduce such a strongly coherent patch in multiple pieces. I'm making 
sure though (git rebase -i --exec make) that the build will not break 
for any of the pieces.

Thanks again for your great in-depth review!

Florian