Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1328588997-25029-1-git-send-email-aguedespe@gmail.com> <1328588997-25029-4-git-send-email-aguedespe@gmail.com> Date: Tue, 7 Feb 2012 11:26:34 -0200 Message-ID: Subject: Re: [PATCH 3/3] Bluetooth: Use advertising cache thread-safe functions From: Ulisses Furquim To: Andre Guedes Cc: Anderson Lizardo , Andre Guedes , linux-bluetooth@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-bluetooth-owner@vger.kernel.org List-ID: Hi Andre, On Tue, Feb 7, 2012 at 10:20 AM, Andre Guedes wrote: > Hi Lizardo, > > On Tue, Feb 7, 2012 at 7:48 AM, Anderson Lizardo > wrote: >> Hi Andre, >> >> On Tue, Feb 7, 2012 at 12:29 AM, Andre Guedes 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