Re: [PATCH 3/3] Bluetooth: Use advertising cache thread-safe functions

[Ulisses Furquim <ulisses@profusion.mobi>]

Hi Andre,

On Tue, Feb 7, 2012 at 10:20 AM, Andre Guedes
<andre.guedes@openbossa.org> wrote:
> Hi Lizardo,
>
> On Tue, Feb 7, 2012 at 7:48 AM, Anderson Lizardo
> <anderson.lizardo@openbossa.org> wrote:
>> Hi Andre,
>>
>> On Tue, Feb 7, 2012 at 12:29 AM, Andre Guedes <aguedespe@gmail.com> wrote:
>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>> index 6808069..3933ccd 100644
>>> --- a/net/bluetooth/hci_event.c
>>> +++ b/net/bluetooth/hci_event.c
>>> @@ -3255,12 +3255,10 @@ static inline void hci_le_adv_report_evt(struct hci_dev *hdev,
>>>        void *ptr = &skb->data[1];
>>>        s8 rssi;
>>>
>>> -       hci_dev_lock(hdev);
>>> -
>>
>> So there is no need to lock hdev between the hci_add_adv_entry() and
>> mgmt_device_found() calls? This looks different from what is done for
>> BR/EDR for the inquiry cache.
>
> Yes, mgmt_device_found() does not require locking hdev->lock.

We could then move the lock and unlock calls to inside the loop. But
as we might have more than one call to hci_add_adv_entry() it'd be
good to lock and unlock only once, no? Any problems I don't see?

>>>        while (num_reports--) {
>>>                struct hci_ev_le_advertising_info *ev = ptr;
>>>
>>> -               __hci_add_adv_entry(hdev, ev);
>>> +               hci_add_adv_entry(hdev, ev);
>>>
>>>                rssi = ev->data[ev->length];
>>>                mgmt_device_found(hdev, &ev->bdaddr, LE_LINK, ev->bdaddr_type,
>>> @@ -3268,8 +3266,6 @@ static inline void hci_le_adv_report_evt(struct hci_dev *hdev,
>>>
>>>                ptr += sizeof(*ev) + ev->length + 1;
>>>        }
>>> -
>>> -       hci_dev_unlock(hdev);
>>>  }
>>>
>>>  static inline void hci_le_ltk_request_evt(struct hci_dev *hdev,
>>> --
>>> 1.7.9

While I don't see anything wrong with your changes I don't think we
really need it. All the other functions that need to be called with
hdev->lock held don't have "__" prefix so it'll be different than the
others. And you added 3 new locked functions but your last patch only
uses 2 of them and only in 2 places. Unless I'm missing something here
we don't really need this refactoring at all. Do you have any other
reason to do that? Are you gonna use those functions in other
patchset?

Thanks,
Regards,

-- 
Ulisses Furquim
ProFUSION embedded systems
http://profusion.mobi
Mobile: +55 19 9250 0942
Skype: ulissesffs