[PATCH 0/3] Advertising cache locking code refactoring

[PATCH 0/3] Advertising cache locking code refactoring

[Andre Guedes]

Hi all,

This patch series does some code refactoring in advertising cache locking
related code. This work is a small effort to improve locking usage in
Bluetooth subsystem.

BR,

Andre

Andre Guedes (3):
  Bluetooth: Add prefix "__" to advertising cache functions
  Bluetooth: Create thread-safe advertising cache functions
  Bluetooth: Use advertising cache thread-safe functions

 include/net/bluetooth/hci_core.h |    4 +++
 net/bluetooth/hci_conn.c         |    2 +-
 net/bluetooth/hci_core.c         |   51 +++++++++++++++++++++++++++++++------
 net/bluetooth/hci_event.c        |    6 +---
 4 files changed, 48 insertions(+), 15 deletions(-)

-- 
1.7.9

[PATCH 1/3] Bluetooth: Add prefix "__" to advertising cache functions

[Andre Guedes]

This patch does a code refactoring in advertising cache functions.
It adds the prefix "__" to these functions and adds a comment to
explicitly indicate they must be called with hdev->lock held.

Signed-off-by: Andre Guedes <aguedespe@gmail.com>
---
 include/net/bluetooth/hci_core.h |    6 +++---
 net/bluetooth/hci_conn.c         |    2 +-
 net/bluetooth/hci_core.c         |   15 +++++++++------
 net/bluetooth/hci_event.c        |    4 ++--
 4 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 7107790..a6fccca 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -673,9 +673,9 @@ int hci_add_remote_oob_data(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 *hash,
 int hci_remove_remote_oob_data(struct hci_dev *hdev, bdaddr_t *bdaddr);
 
 #define ADV_CLEAR_TIMEOUT (3*60*HZ) /* Three minutes */
-int hci_adv_entries_clear(struct hci_dev *hdev);
-struct adv_entry *hci_find_adv_entry(struct hci_dev *hdev, bdaddr_t *bdaddr);
-int hci_add_adv_entry(struct hci_dev *hdev,
+int __hci_adv_entries_clear(struct hci_dev *hdev);
+struct adv_entry *__hci_find_adv_entry(struct hci_dev *hdev, bdaddr_t *bdaddr);
+int __hci_add_adv_entry(struct hci_dev *hdev,
 					struct hci_ev_le_advertising_info *ev);
 
 void hci_del_off_timer(struct hci_dev *hdev);
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index b4ecdde..2f6ca8e 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -530,7 +530,7 @@ struct hci_conn *hci_connect(struct hci_dev *hdev, int type, bdaddr_t *dst, __u8
 		if (le)
 			return ERR_PTR(-EBUSY);
 
-		entry = hci_find_adv_entry(hdev, dst);
+		entry = __hci_find_adv_entry(hdev, dst);
 		if (!entry)
 			return ERR_PTR(-EHOSTUNREACH);
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 3d09f4b4..362b1ab 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1534,12 +1534,13 @@ static void hci_clear_adv_cache(struct work_struct *work)
 
 	hci_dev_lock(hdev);
 
-	hci_adv_entries_clear(hdev);
+	__hci_adv_entries_clear(hdev);
 
 	hci_dev_unlock(hdev);
 }
 
-int hci_adv_entries_clear(struct hci_dev *hdev)
+/* Must be holding hdev->lock */
+int __hci_adv_entries_clear(struct hci_dev *hdev)
 {
 	struct adv_entry *entry, *tmp;
 
@@ -1553,7 +1554,8 @@ int hci_adv_entries_clear(struct hci_dev *hdev)
 	return 0;
 }
 
-struct adv_entry *hci_find_adv_entry(struct hci_dev *hdev, bdaddr_t *bdaddr)
+/* Must be holding hdev->lock */
+struct adv_entry *__hci_find_adv_entry(struct hci_dev *hdev, bdaddr_t *bdaddr)
 {
 	struct adv_entry *entry;
 
@@ -1572,7 +1574,8 @@ static inline int is_connectable_adv(u8 evt_type)
 	return 0;
 }
 
-int hci_add_adv_entry(struct hci_dev *hdev,
+/* Must be holding hdev->lock */
+int __hci_add_adv_entry(struct hci_dev *hdev,
 					struct hci_ev_le_advertising_info *ev)
 {
 	struct adv_entry *entry;
@@ -1582,7 +1585,7 @@ int hci_add_adv_entry(struct hci_dev *hdev,
 
 	/* Only new entries should be added to adv_entries. So, if
 	 * bdaddr was found, don't add it. */
-	if (hci_find_adv_entry(hdev, &ev->bdaddr))
+	if (__hci_find_adv_entry(hdev, &ev->bdaddr))
 		return 0;
 
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
@@ -1879,7 +1882,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
 	hci_link_keys_clear(hdev);
 	hci_smp_ltks_clear(hdev);
 	hci_remote_oob_data_clear(hdev);
-	hci_adv_entries_clear(hdev);
+	__hci_adv_entries_clear(hdev);
 	hci_dev_unlock(hdev);
 
 	hci_dev_put(hdev);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index ad5f37b..6808069 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -1070,7 +1070,7 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
 		cancel_delayed_work_sync(&hdev->adv_work);
 
 		hci_dev_lock(hdev);
-		hci_adv_entries_clear(hdev);
+		__hci_adv_entries_clear(hdev);
 		hci_discovery_set_state(hdev, DISCOVERY_LE_SCAN);
 		hci_dev_unlock(hdev);
 		break;
@@ -3260,7 +3260,7 @@ static inline void hci_le_adv_report_evt(struct hci_dev *hdev,
 	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,
-- 
1.7.9

[PATCH 2/3] Bluetooth: Create thread-safe advertising cache functions

[Andre Guedes]

This patch creates thread-safe versions of advertising cache add/
find/clear functions.

Signed-off-by: Andre Guedes <aguedespe@gmail.com>
---
 include/net/bluetooth/hci_core.h |    4 ++++
 net/bluetooth/hci_core.c         |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index a6fccca..ca52a88 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -677,6 +677,10 @@ int __hci_adv_entries_clear(struct hci_dev *hdev);
 struct adv_entry *__hci_find_adv_entry(struct hci_dev *hdev, bdaddr_t *bdaddr);
 int __hci_add_adv_entry(struct hci_dev *hdev,
 					struct hci_ev_le_advertising_info *ev);
+int hci_adv_entries_clear(struct hci_dev *hdev);
+struct adv_entry *hci_find_adv_entry(struct hci_dev *hdev, bdaddr_t *bdaddr);
+int hci_add_adv_entry(struct hci_dev *hdev,
+					struct hci_ev_le_advertising_info *ev);
 
 void hci_del_off_timer(struct hci_dev *hdev);
 
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 362b1ab..536248d 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1603,6 +1603,40 @@ int __hci_add_adv_entry(struct hci_dev *hdev,
 	return 0;
 }
 
+struct adv_entry *hci_find_adv_entry(struct hci_dev *hdev, bdaddr_t *bdaddr)
+{
+	struct adv_entry *entry;
+
+	hci_dev_lock(hdev);
+	entry = __hci_find_adv_entry(hdev, bdaddr);
+	hci_dev_unlock(hdev);
+
+	return entry;
+}
+
+int hci_adv_entries_clear(struct hci_dev *hdev)
+{
+	int res;
+
+	hci_dev_lock(hdev);
+	res = __hci_adv_entries_clear(hdev);
+	hci_dev_unlock(hdev);
+
+	return res;
+}
+
+int hci_add_adv_entry(struct hci_dev *hdev,
+					struct hci_ev_le_advertising_info *ev)
+{
+	int res;
+
+	hci_dev_lock(hdev);
+	res = __hci_add_adv_entry(hdev, ev);
+	hci_dev_unlock(hdev);
+
+	return res;
+}
+
 static void le_scan_param_req(struct hci_dev *hdev, unsigned long opt)
 {
 	struct le_scan_params *param =  (struct le_scan_params *) opt;
-- 
1.7.9

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

[Andre Guedes]

This patch replaces some advertising cache unsafe function calls
by its thread-safe versions where applicable.

Signed-off-by: Andre Guedes <aguedespe@gmail.com>
---
 net/bluetooth/hci_core.c  |    6 +-----
 net/bluetooth/hci_event.c |    6 +-----
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 536248d..24b7621 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1532,11 +1532,7 @@ static void hci_clear_adv_cache(struct work_struct *work)
 	struct hci_dev *hdev = container_of(work, struct hci_dev,
 							adv_work.work);
 
-	hci_dev_lock(hdev);
-
-	__hci_adv_entries_clear(hdev);
-
-	hci_dev_unlock(hdev);
+	hci_adv_entries_clear(hdev);
 }
 
 /* Must be holding hdev->lock */
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);
-
 	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

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

[Anderson Lizardo]

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.

>        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


Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

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

[Andre Guedes]

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.

>>        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

BR,

Andre

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

[Ulisses Furquim]

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

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

[Andre Guedes]

Hi Ulisses,

On Tue, Feb 7, 2012 at 10:26 AM, Ulisses Furquim <ulisses@profusion.mobi> wrote:
> 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?

Yes, that's right. For this particular case, it may be better to lock
hdev outside while() and call the thread-unsafe version here.

This way, it may be better we just drop patches 02/03 and 03/03.

>>>>        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?

Yes, some other functions don't have the prefix "__" and that fact
makes a bit painful and error-prone since we always have to dig in
the "call chain" to know if we need to hold hdev->lock or not.
Prefixing a function with "__" is just a standard way to indicate
that.

BR,

Andre

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

[Ulisses Furquim]

Hi Andre,

On Tue, Feb 7, 2012 at 12:46 PM, Andre Guedes
<andre.guedes@openbossa.org> wrote:
> Hi Ulisses,
>
> On Tue, Feb 7, 2012 at 10:26 AM, Ulisses Furquim <ulisses@profusion.mobi> wrote:
>> 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?
>
> Yes, that's right. For this particular case, it may be better to lock
> hdev outside while() and call the thread-unsafe version here.
>
> This way, it may be better we just drop patches 02/03 and 03/03.
>
>>>>>        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?
>
> Yes, some other functions don't have the prefix "__" and that fact
> makes a bit painful and error-prone since we always have to dig in
> the "call chain" to know if we need to hold hdev->lock or not.
> Prefixing a function with "__" is just a standard way to indicate
> that.

I understand that. I just don't know if Marcel will want to change
them all to have "__" prefixes, though. Having only one subset with
this prefix can make things even more confusing, don't you agree?

And have you been working with these functions or is this just a
cleanup you thought it'd be good to do?

Regards,

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

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

[Andre Guedes]

Hi Ulisses,

On Tue, Feb 7, 2012 at 12:20 PM, Ulisses Furquim <ulisses@profusion.mobi> wrote:
> Hi Andre,
>
> On Tue, Feb 7, 2012 at 12:46 PM, Andre Guedes
> <andre.guedes@openbossa.org> wrote:
>> Hi Ulisses,
>>
>> On Tue, Feb 7, 2012 at 10:26 AM, Ulisses Furquim <ulisses@profusion.mobi> wrote:
>>> 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?
>>
>> Yes, that's right. For this particular case, it may be better to lock
>> hdev outside while() and call the thread-unsafe version here.
>>
>> This way, it may be better we just drop patches 02/03 and 03/03.
>>
>>>>>>        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?
>>
>> Yes, some other functions don't have the prefix "__" and that fact
>> makes a bit painful and error-prone since we always have to dig in
>> the "call chain" to know if we need to hold hdev->lock or not.
>> Prefixing a function with "__" is just a standard way to indicate
>> that.
>
> I understand that. I just don't know if Marcel will want to change
> them all to have "__" prefixes, though. Having only one subset with
> this prefix can make things even more confusing, don't you agree?

Yes, this is pretty much an RFC series, I just realized I missed
changing the --subject-prefix.

> And have you been working with these functions or is this just a
> cleanup you thought it'd be good to do?

Just a cleanup.

Andre

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

[Ulisses Furquim]

Hi Andre,

On Tue, Feb 7, 2012 at 3:41 PM, Andre Guedes <andre.guedes@openbossa.org> wrote:
> Hi Ulisses,
>
> On Tue, Feb 7, 2012 at 12:20 PM, Ulisses Furquim <ulisses@profusion.mobi> wrote:
>> Hi Andre,
>>
>> On Tue, Feb 7, 2012 at 12:46 PM, Andre Guedes
>> <andre.guedes@openbossa.org> wrote:
>>> Hi Ulisses,
>>>
>>> On Tue, Feb 7, 2012 at 10:26 AM, Ulisses Furquim <ulisses@profusion.mobi> wrote:
>>>> 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?
>>>
>>> Yes, that's right. For this particular case, it may be better to lock
>>> hdev outside while() and call the thread-unsafe version here.
>>>
>>> This way, it may be better we just drop patches 02/03 and 03/03.
>>>
>>>>>>>        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?
>>>
>>> Yes, some other functions don't have the prefix "__" and that fact
>>> makes a bit painful and error-prone since we always have to dig in
>>> the "call chain" to know if we need to hold hdev->lock or not.
>>> Prefixing a function with "__" is just a standard way to indicate
>>> that.
>>
>> I understand that. I just don't know if Marcel will want to change
>> them all to have "__" prefixes, though. Having only one subset with
>> this prefix can make things even more confusing, don't you agree?
>
> Yes, this is pretty much an RFC series, I just realized I missed
> changing the --subject-prefix.
>
>> And have you been working with these functions or is this just a
>> cleanup you thought it'd be good to do?
>
> Just a cleanup.

Ok, then it's really up to Marcel to accept the addition of prefixes
or not. I'd leave it without the prefix for consistency.

Regards,

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

Re: [PATCH 0/3] Advertising cache locking code refactoring

[Marcel Holtmann]

Hi Andre,

> This patch series does some code refactoring in advertising cache locking
> related code. This work is a small effort to improve locking usage in
> Bluetooth subsystem.
> 
> BR,
> 
> Andre
> 
> Andre Guedes (3):
>   Bluetooth: Add prefix "__" to advertising cache functions
>   Bluetooth: Create thread-safe advertising cache functions
>   Bluetooth: Use advertising cache thread-safe functions
> 
>  include/net/bluetooth/hci_core.h |    4 +++
>  net/bluetooth/hci_conn.c         |    2 +-
>  net/bluetooth/hci_core.c         |   51 +++++++++++++++++++++++++++++++------
>  net/bluetooth/hci_event.c        |    6 +---
>  4 files changed, 48 insertions(+), 15 deletions(-)

so I looked through this patch series and the only useful patch is 1/3
and even that one is kinda questionable. However if people in general
find this a bit clearer that we prefix unlocked hdev functions with __,
then I would be fine is it. Opinions anybody?

Regards

Marcel

Re: [PATCH 0/3] Advertising cache locking code refactoring

[Ulisses Furquim]

Hi Marcel,

On Thu, Feb 9, 2012 at 11:49 AM, Marcel Holtmann <marcel@holtmann.org> wrot=
e:
> Hi Andre,
>
>> This patch series does some code refactoring in advertising cache lockin=
g
>> related code. This work is a small effort to improve locking usage in
>> Bluetooth subsystem.
>>
>> BR,
>>
>> Andre
>>
>> Andre Guedes (3):
>> =A0 Bluetooth: Add prefix "__" to advertising cache functions
>> =A0 Bluetooth: Create thread-safe advertising cache functions
>> =A0 Bluetooth: Use advertising cache thread-safe functions
>>
>> =A0include/net/bluetooth/hci_core.h | =A0 =A04 +++
>> =A0net/bluetooth/hci_conn.c =A0 =A0 =A0 =A0 | =A0 =A02 +-
>> =A0net/bluetooth/hci_core.c =A0 =A0 =A0 =A0 | =A0 51 +++++++++++++++++++=
++++++++++++------
>> =A0net/bluetooth/hci_event.c =A0 =A0 =A0 =A0| =A0 =A06 +---
>> =A04 files changed, 48 insertions(+), 15 deletions(-)
>
> so I looked through this patch series and the only useful patch is 1/3
> and even that one is kinda questionable. However if people in general
> find this a bit clearer that we prefix unlocked hdev functions with __,
> then I would be fine is it. Opinions anybody?

Not sure if you saw my replies. I said this series is not needed at
all IMO. And if we want to prefix unlocked hdev functions with __ then
we better change all of them to have everything consistent. And I'm
really against adding locked versions if we're not really using them.

Best regards,

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

Re: [PATCH 0/3] Advertising cache locking code refactoring

[Marcel Holtmann]

Hi Ulisses,

> >> This patch series does some code refactoring in advertising cache locking
> >> related code. This work is a small effort to improve locking usage in
> >> Bluetooth subsystem.
> >>
> >> BR,
> >>
> >> Andre
> >>
> >> Andre Guedes (3):
> >>   Bluetooth: Add prefix "__" to advertising cache functions
> >>   Bluetooth: Create thread-safe advertising cache functions
> >>   Bluetooth: Use advertising cache thread-safe functions
> >>
> >>  include/net/bluetooth/hci_core.h |    4 +++
> >>  net/bluetooth/hci_conn.c         |    2 +-
> >>  net/bluetooth/hci_core.c         |   51 +++++++++++++++++++++++++++++++------
> >>  net/bluetooth/hci_event.c        |    6 +---
> >>  4 files changed, 48 insertions(+), 15 deletions(-)
> >
> > so I looked through this patch series and the only useful patch is 1/3
> > and even that one is kinda questionable. However if people in general
> > find this a bit clearer that we prefix unlocked hdev functions with __,
> > then I would be fine is it. Opinions anybody?
> 
> Not sure if you saw my replies. I said this series is not needed at
> all IMO. And if we want to prefix unlocked hdev functions with __ then
> we better change all of them to have everything consistent. And I'm
> really against adding locked versions if we're not really using them.

I agree, no locked versions if we don't need them. We can add them
always later when we do.

And I am fine with __ prefixes for all unlocked functions, but yes, we
should be 100% consistent then.

Regards

Marcel