[RFC 1/2] Bluetooth: General HCI callback implementation

[RFC 1/2] Bluetooth: General HCI callback implementation

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Add general HCI callback implementation. Can be used for executing
HCI commands from A2MP protocol.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/hci_core.h |   18 ++++++++++++
 net/bluetooth/hci_core.c         |   57 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index d12e353..2ef515e 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -131,6 +131,17 @@ struct le_scan_params {
 
 #define HCI_MAX_SHORT_NAME_LENGTH	10
 
+struct hci_dev;
+
+struct cb_cmd {
+	struct list_head list;
+	u16 opcode;
+	u8 status;
+	void *opt;
+	void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd);
+	void (*destructor)(struct cb_cmd *cmd);
+};
+
 #define NUM_REASSEMBLY 4
 struct hci_dev {
 	struct list_head list;
@@ -237,6 +248,7 @@ struct hci_dev {
 	__u16			init_last_cmd;
 
 	struct list_head	mgmt_pending;
+	struct list_head	cb_list;
 
 	struct discovery_state	discovery;
 	struct hci_conn_hash	conn_hash;
@@ -1086,4 +1098,10 @@ int hci_cancel_inquiry(struct hci_dev *hdev);
 int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
 								int timeout);
 
+struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode);
+int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
+	void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd), void *opt,
+				void (*destructor)(struct cb_cmd *cmd));
+void hci_remove_cb(struct cb_cmd *cmd);
+
 #endif /* __HCI_CORE_H */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5b1ddab..cdc0220 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1826,6 +1826,8 @@ int hci_register_dev(struct hci_dev *hdev)
 
 	INIT_LIST_HEAD(&hdev->mgmt_pending);
 
+	INIT_LIST_HEAD(&hdev->cb_list);
+
 	INIT_LIST_HEAD(&hdev->blacklist);
 
 	INIT_LIST_HEAD(&hdev->uuids);
@@ -2239,6 +2241,61 @@ int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
 	return 0;
 }
 
+void hci_add_cb(struct hci_dev *hdev, __u16 opcode,
+			void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd),
+			void *opt, void (*destructor)(struct cb_cmd *cmd))
+{
+	struct cb_cmd *cmd;
+
+	cmd = kmalloc(sizeof(*cmd), GFP_ATOMIC);
+	if (!cmd)
+		return;
+
+	cmd->cb = cb;
+	cmd->opcode = opcode;
+	cmd->opt = opt;
+	cmd->status = 0;
+
+	if (destructor)
+		cmd->destructor = destructor;
+
+	list_add(&cmd->list, &hdev->cb_list);
+}
+
+struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode)
+{
+	struct cb_cmd *cmd;
+
+	list_for_each_entry(cmd, &hdev->cb_list, list)
+		if (cmd->opcode == opcode)
+			return cmd;
+
+	return NULL;
+}
+
+void hci_remove_cb(struct cb_cmd *cmd)
+{
+	list_del(&cmd->list);
+
+	if (cmd->destructor) {
+		cmd->destructor(cmd);
+	} else {
+		kfree(cmd->opt);
+		kfree(cmd);
+	}
+}
+
+/* Send HCI command with callback */
+int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
+			void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd),
+			void *opt, void (*destructor)(struct cb_cmd *cmd))
+{
+	if (cb)
+		hci_add_cb(hdev, opcode, cb, opt, destructor);
+
+	return hci_send_cmd(hdev, opcode, plen, param);
+}
+
 /* Get data from the previously sent command */
 void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode)
 {
-- 
1.7.9

[RFC 2/2] Bluetooth: Process HCI callbacks in a workqueue

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>


Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/hci_core.h |    2 +
 net/bluetooth/hci_core.c         |   42 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 2ef515e..47f1631 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1103,5 +1103,7 @@ int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
 	void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd), void *opt,
 				void (*destructor)(struct cb_cmd *cmd));
 void hci_remove_cb(struct cb_cmd *cmd);
+void hci_queue_cb(struct hci_dev *hdev, struct cb_cmd *cmd,
+					struct workqueue_struct *workqueue);
 
 #endif /* __HCI_CORE_H */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index cdc0220..2bd97b4 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2273,6 +2273,48 @@ struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode)
 	return NULL;
 }
 
+struct cb_work {
+	struct work_struct work;
+	struct hci_dev *hdev;
+	struct cb_cmd *cmd;
+};
+
+static void hci_cb_work(struct work_struct *w)
+{
+	struct cb_work *work = (struct cb_work *) w;
+	struct cb_cmd *cmd = work->cmd;
+	struct hci_dev *hdev = work->hdev;
+
+	cmd->cb(hdev, cmd);
+
+	hci_dev_put(hdev);
+
+	hci_remove_cb(cmd);
+	kfree(w);
+}
+
+void hci_queue_cb(struct hci_dev *hdev, struct cb_cmd *cmd,
+					struct workqueue_struct *workqueue)
+{
+	struct cb_work *work;
+
+	BT_ERR("Queue cmd %p opt %p", cmd, cmd->opt);
+
+	work = kmalloc(sizeof(*work), GFP_ATOMIC);
+	if (!work)
+		return;
+
+	INIT_WORK(&work->work, hci_cb_work);
+	work->hdev = hdev;
+	work->cmd = cmd;
+	hci_dev_hold(hdev);
+
+	if (!queue_work(workqueue, &work->work)) {
+		kfree(work);
+		hci_dev_put(hdev);
+	}
+}
+
 void hci_remove_cb(struct cb_cmd *cmd)
 {
 	list_del(&cmd->list);
-- 
1.7.9

Re: [RFC 1/2] Bluetooth: General HCI callback implementation

[Ulisses Furquim]

Hi Andrei,

On Tue, Mar 6, 2012 at 10:16 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
> Add general HCI callback implementation. Can be used for executing
> HCI commands from A2MP protocol.
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/hci_core.h |   18 ++++++++++++
>  net/bluetooth/hci_core.c         |   57 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 75 insertions(+), 0 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index d12e353..2ef515e 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -131,6 +131,17 @@ struct le_scan_params {
>
>  #define HCI_MAX_SHORT_NAME_LENGTH      10
>
> +struct hci_dev;
> +
> +struct cb_cmd {

Maybe a better name here with a prefix? hci_cb_cmd?

> +       struct list_head list;
> +       u16 opcode;
> +       u8 status;
> +       void *opt;
> +       void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd);
> +       void (*destructor)(struct cb_cmd *cmd);
> +};
> +
>  #define NUM_REASSEMBLY 4
>  struct hci_dev {
>        struct list_head list;
> @@ -237,6 +248,7 @@ struct hci_dev {
>        __u16                   init_last_cmd;
>
>        struct list_head        mgmt_pending;
> +       struct list_head        cb_list;
>
>        struct discovery_state  discovery;
>        struct hci_conn_hash    conn_hash;
> @@ -1086,4 +1098,10 @@ int hci_cancel_inquiry(struct hci_dev *hdev);
>  int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
>                                                                int timeout);
>
> +struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode);
> +int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
> +       void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd), void *opt,
> +                               void (*destructor)(struct cb_cmd *cmd));

This line is very ugly and hard to read. Maybe have a longer line like
other parts of the kernel seem to be accepting now?

> +void hci_remove_cb(struct cb_cmd *cmd);
> +
>  #endif /* __HCI_CORE_H */
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 5b1ddab..cdc0220 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1826,6 +1826,8 @@ int hci_register_dev(struct hci_dev *hdev)
>
>        INIT_LIST_HEAD(&hdev->mgmt_pending);
>
> +       INIT_LIST_HEAD(&hdev->cb_list);
> +
>        INIT_LIST_HEAD(&hdev->blacklist);
>
>        INIT_LIST_HEAD(&hdev->uuids);
> @@ -2239,6 +2241,61 @@ int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
>        return 0;
>  }
>
> +void hci_add_cb(struct hci_dev *hdev, __u16 opcode,
> +                       void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd),
> +                       void *opt, void (*destructor)(struct cb_cmd *cmd))
> +{
> +       struct cb_cmd *cmd;
> +
> +       cmd = kmalloc(sizeof(*cmd), GFP_ATOMIC);
> +       if (!cmd)
> +               return;
> +
> +       cmd->cb = cb;
> +       cmd->opcode = opcode;
> +       cmd->opt = opt;
> +       cmd->status = 0;
> +
> +       if (destructor)
> +               cmd->destructor = destructor;
> +
> +       list_add(&cmd->list, &hdev->cb_list);

Don't we need some protection here? We probably can use hdev lock.

> +}
> +
> +struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode)
> +{
> +       struct cb_cmd *cmd;
> +
> +       list_for_each_entry(cmd, &hdev->cb_list, list)
> +               if (cmd->opcode == opcode)
> +                       return cmd;

Here as well, we might need to protect the critical section.

> +
> +       return NULL;
> +}
> +
> +void hci_remove_cb(struct cb_cmd *cmd)
> +{
> +       list_del(&cmd->list);
> +
> +       if (cmd->destructor) {
> +               cmd->destructor(cmd);
> +       } else {
> +               kfree(cmd->opt);
> +               kfree(cmd);
> +       }

And here too. Right?

> +}
> +
> +/* Send HCI command with callback */
> +int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
> +                       void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd),
> +                       void *opt, void (*destructor)(struct cb_cmd *cmd))
> +{
> +       if (cb)
> +               hci_add_cb(hdev, opcode, cb, opt, destructor);
> +
> +       return hci_send_cmd(hdev, opcode, plen, param);
> +}
> +
>  /* Get data from the previously sent command */
>  void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode)
>  {

We don't have any callers of find and remove functions?

Best regards,

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

Re: [RFC 1/2] Bluetooth: General HCI callback implementation

[Ulisses Furquim]

Hi Andrei,

I forgot one comment.

On Tue, Mar 6, 2012 at 11:57 AM, Ulisses Furquim <ulisses@profusion.mobi> wrote:
> Hi Andrei,
>
> On Tue, Mar 6, 2012 at 10:16 AM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
>> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>>
>> Add general HCI callback implementation. Can be used for executing
>> HCI commands from A2MP protocol.
>>
>> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>> ---
>>  include/net/bluetooth/hci_core.h |   18 ++++++++++++
>>  net/bluetooth/hci_core.c         |   57 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 75 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index d12e353..2ef515e 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -131,6 +131,17 @@ struct le_scan_params {
>>
>>  #define HCI_MAX_SHORT_NAME_LENGTH      10
>>
>> +struct hci_dev;
>> +
>> +struct cb_cmd {
>
> Maybe a better name here with a prefix? hci_cb_cmd?
>
>> +       struct list_head list;
>> +       u16 opcode;
>> +       u8 status;
>> +       void *opt;
>> +       void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd);
>> +       void (*destructor)(struct cb_cmd *cmd);
>> +};
>> +
>>  #define NUM_REASSEMBLY 4
>>  struct hci_dev {
>>        struct list_head list;
>> @@ -237,6 +248,7 @@ struct hci_dev {
>>        __u16                   init_last_cmd;
>>
>>        struct list_head        mgmt_pending;
>> +       struct list_head        cb_list;
>>
>>        struct discovery_state  discovery;
>>        struct hci_conn_hash    conn_hash;
>> @@ -1086,4 +1098,10 @@ int hci_cancel_inquiry(struct hci_dev *hdev);
>>  int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
>>                                                                int timeout);
>>
>> +struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode);
>> +int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
>> +       void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd), void *opt,
>> +                               void (*destructor)(struct cb_cmd *cmd));
>
> This line is very ugly and hard to read. Maybe have a longer line like
> other parts of the kernel seem to be accepting now?
>
>> +void hci_remove_cb(struct cb_cmd *cmd);
>> +
>>  #endif /* __HCI_CORE_H */
>> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
>> index 5b1ddab..cdc0220 100644
>> --- a/net/bluetooth/hci_core.c
>> +++ b/net/bluetooth/hci_core.c
>> @@ -1826,6 +1826,8 @@ int hci_register_dev(struct hci_dev *hdev)
>>
>>        INIT_LIST_HEAD(&hdev->mgmt_pending);
>>
>> +       INIT_LIST_HEAD(&hdev->cb_list);
>> +
>>        INIT_LIST_HEAD(&hdev->blacklist);
>>
>>        INIT_LIST_HEAD(&hdev->uuids);
>> @@ -2239,6 +2241,61 @@ int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
>>        return 0;
>>  }
>>
>> +void hci_add_cb(struct hci_dev *hdev, __u16 opcode,
>> +                       void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd),
>> +                       void *opt, void (*destructor)(struct cb_cmd *cmd))
>> +{
>> +       struct cb_cmd *cmd;
>> +
>> +       cmd = kmalloc(sizeof(*cmd), GFP_ATOMIC);

Can't we use GFP_KERNEL here?

>> +       if (!cmd)
>> +               return;
>> +
>> +       cmd->cb = cb;
>> +       cmd->opcode = opcode;
>> +       cmd->opt = opt;
>> +       cmd->status = 0;
>> +
>> +       if (destructor)
>> +               cmd->destructor = destructor;
>> +
>> +       list_add(&cmd->list, &hdev->cb_list);
>
> Don't we need some protection here? We probably can use hdev lock.
>
>> +}
>> +
>> +struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode)
>> +{
>> +       struct cb_cmd *cmd;
>> +
>> +       list_for_each_entry(cmd, &hdev->cb_list, list)
>> +               if (cmd->opcode == opcode)
>> +                       return cmd;
>
> Here as well, we might need to protect the critical section.
>
>> +
>> +       return NULL;
>> +}
>> +
>> +void hci_remove_cb(struct cb_cmd *cmd)
>> +{
>> +       list_del(&cmd->list);
>> +
>> +       if (cmd->destructor) {
>> +               cmd->destructor(cmd);
>> +       } else {
>> +               kfree(cmd->opt);
>> +               kfree(cmd);
>> +       }
>
> And here too. Right?
>
>> +}
>> +
>> +/* Send HCI command with callback */
>> +int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
>> +                       void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd),
>> +                       void *opt, void (*destructor)(struct cb_cmd *cmd))
>> +{
>> +       if (cb)
>> +               hci_add_cb(hdev, opcode, cb, opt, destructor);
>> +
>> +       return hci_send_cmd(hdev, opcode, plen, param);
>> +}
>> +
>>  /* Get data from the previously sent command */
>>  void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode)
>>  {
>
> We don't have any callers of find and remove functions?
>
> Best regards,
>
> --
> Ulisses Furquim
> ProFUSION embedded systems
> http://profusion.mobi
> Mobile: +55 19 9250 0942
> Skype: ulissesffs

Regards,

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

Re: [RFC 2/2] Bluetooth: Process HCI callbacks in a workqueue

[Ulisses Furquim]

Hi Andrei,

On Tue, Mar 6, 2012 at 10:16 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@gmail.com> wrote:
> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
>
>
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/hci_core.h |    2 +
>  net/bluetooth/hci_core.c         |   42 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 0 deletions(-)
>
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 2ef515e..47f1631 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1103,5 +1103,7 @@ int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
>        void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd), void *opt,
>                                void (*destructor)(struct cb_cmd *cmd));
>  void hci_remove_cb(struct cb_cmd *cmd);
> +void hci_queue_cb(struct hci_dev *hdev, struct cb_cmd *cmd,
> +                                       struct workqueue_struct *workqueue);
>
>  #endif /* __HCI_CORE_H */
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index cdc0220..2bd97b4 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2273,6 +2273,48 @@ struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode)
>        return NULL;
>  }
>
> +struct cb_work {

Again, maybe hci_cb_work?

> +       struct work_struct work;
> +       struct hci_dev *hdev;
> +       struct cb_cmd *cmd;
> +};
> +
> +static void hci_cb_work(struct work_struct *w)

And here hci_cb_worker?

> +{
> +       struct cb_work *work = (struct cb_work *) w;
> +       struct cb_cmd *cmd = work->cmd;
> +       struct hci_dev *hdev = work->hdev;
> +
> +       cmd->cb(hdev, cmd);
> +
> +       hci_dev_put(hdev);
> +
> +       hci_remove_cb(cmd);
> +       kfree(w);
> +}
> +
> +void hci_queue_cb(struct hci_dev *hdev, struct cb_cmd *cmd,
> +                                       struct workqueue_struct *workqueue)
> +{
> +       struct cb_work *work;
> +
> +       BT_ERR("Queue cmd %p opt %p", cmd, cmd->opt);
> +
> +       work = kmalloc(sizeof(*work), GFP_ATOMIC);

Why not GFP_KERNEL?

> +       if (!work)
> +               return;
> +
> +       INIT_WORK(&work->work, hci_cb_work);
> +       work->hdev = hdev;
> +       work->cmd = cmd;
> +       hci_dev_hold(hdev);
> +
> +       if (!queue_work(workqueue, &work->work)) {
> +               kfree(work);
> +               hci_dev_put(hdev);
> +       }
> +}
> +
>  void hci_remove_cb(struct cb_cmd *cmd)
>  {
>        list_del(&cmd->list);

And again, no callers of hci_queue_cb so I'm assuming you'll only use
in for AMP, is that it?

Regards,

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

Re: [RFC 1/2] Bluetooth: General HCI callback implementation

[Szymon Janc]

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Hi Andrei,

> Add general HCI callback implementation. Can be used for executing
> HCI commands from A2MP protocol.

> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/hci_core.h |   18 ++++++++++++
>  net/bluetooth/hci_core.c         |   57 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 75 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index d12e353..2ef515e 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -131,6 +131,17 @@ struct le_scan_params {
>  
>  #define HCI_MAX_SHORT_NAME_LENGTH	10
>  
> +struct hci_dev;
> +
> +struct cb_cmd {

This name seems a bit too more generic to me, maybe prefix it with hci_?

> +	struct list_head list;
> +	u16 opcode;
> +	u8 status;
> +	void *opt;
> +	void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd);
> +	void (*destructor)(struct cb_cmd *cmd);
> +};
> +
>  #define NUM_REASSEMBLY 4
>  struct hci_dev {
>  	struct list_head list;
> @@ -237,6 +248,7 @@ struct hci_dev {
>  	__u16			init_last_cmd;
>  
>  	struct list_head	mgmt_pending;
> +	struct list_head	cb_list;
>  
>  	struct discovery_state	discovery;
>  	struct hci_conn_hash	conn_hash;
> @@ -1086,4 +1098,10 @@ int hci_cancel_inquiry(struct hci_dev *hdev);
>  int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
>  								int timeout);
>  
> +struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode);
> +int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
> +	void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd), void *opt,
> +				void (*destructor)(struct cb_cmd *cmd));
> +void hci_remove_cb(struct cb_cmd *cmd);
> +
>  #endif /* __HCI_CORE_H */
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 5b1ddab..cdc0220 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1826,6 +1826,8 @@ int hci_register_dev(struct hci_dev *hdev)
>  
>  	INIT_LIST_HEAD(&hdev->mgmt_pending);
>  
> +	INIT_LIST_HEAD(&hdev->cb_list);
> +
>  	INIT_LIST_HEAD(&hdev->blacklist);
>  
>  	INIT_LIST_HEAD(&hdev->uuids);
> @@ -2239,6 +2241,61 @@ int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
>  	return 0;
>  }
>  

hci_add_cb could return error on failure and that could be used in hci_cmd_cb().

> +void hci_add_cb(struct hci_dev *hdev, __u16 opcode,
> +			void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd),
> +			void *opt, void (*destructor)(struct cb_cmd *cmd))
> +{
> +	struct cb_cmd *cmd;
> +
> +	cmd = kmalloc(sizeof(*cmd), GFP_ATOMIC);

Why atomic? Maybe allow to pass custom gfp mask?

> +	if (!cmd)
> +		return;
> +
> +	cmd->cb = cb;
> +	cmd->opcode = opcode;
> +	cmd->opt = opt;
> +	cmd->status = 0;
> +
> +	if (destructor)
> +		cmd->destructor = destructor;

That could leave cmd->destructor uninitialized (cmd is allocated with kmalloc).
I would assign destructor unconditionally.

> +
> +	list_add(&cmd->list, &hdev->cb_list);
> +}
> +
> +struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode)
> +{
> +	struct cb_cmd *cmd;
> +
> +	list_for_each_entry(cmd, &hdev->cb_list, list)
> +		if (cmd->opcode == opcode)
> +			return cmd;
> +
> +	return NULL;
> +}
> +
> +void hci_remove_cb(struct cb_cmd *cmd)
> +{
> +	list_del(&cmd->list);
> +
> +	if (cmd->destructor) {
> +		cmd->destructor(cmd);
> +	} else {
> +		kfree(cmd->opt);
> +		kfree(cmd);
> +	}
> +}
> +
> +/* Send HCI command with callback */
> +int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
> +			void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd),
> +			void *opt, void (*destructor)(struct cb_cmd *cmd))
> +{
> +	if (cb)
> +		hci_add_cb(hdev, opcode, cb, opt, destructor);

Is this if really needed? If one would like to send cmd without cb he can just
call hci_send_cmd directly. And I would also check if hci_add_cb failed before
sending command.

> +
> +	return hci_send_cmd(hdev, opcode, plen, param);
> +}
> +
>  /* Get data from the previously sent command */
>  void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode)
>  {
> 

Re: [RFC 2/2] Bluetooth: Process HCI callbacks in a workqueue

[Szymon Janc]

Hi Andrei,

> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  include/net/bluetooth/hci_core.h |    2 +
>  net/bluetooth/hci_core.c         |   42 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+), 0 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 2ef515e..47f1631 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1103,5 +1103,7 @@ int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
>  	void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd), void *opt,
>  				void (*destructor)(struct cb_cmd *cmd));
>  void hci_remove_cb(struct cb_cmd *cmd);
> +void hci_queue_cb(struct hci_dev *hdev, struct cb_cmd *cmd,
> +					struct workqueue_struct *workqueue);
>  
>  #endif /* __HCI_CORE_H */
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index cdc0220..2bd97b4 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -2273,6 +2273,48 @@ struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode)
>  	return NULL;
>  }
>  
> +struct cb_work {
> +	struct work_struct work;
> +	struct hci_dev *hdev;
> +	struct cb_cmd *cmd;
> +};
> +
> +static void hci_cb_work(struct work_struct *w)
> +{
> +	struct cb_work *work = (struct cb_work *) w;
> +	struct cb_cmd *cmd = work->cmd;
> +	struct hci_dev *hdev = work->hdev;
> +
> +	cmd->cb(hdev, cmd);
> +
> +	hci_dev_put(hdev);
> +
> +	hci_remove_cb(cmd);
> +	kfree(w);
> +}
> +
> +void hci_queue_cb(struct hci_dev *hdev, struct cb_cmd *cmd,
> +					struct workqueue_struct *workqueue)
> +{
> +	struct cb_work *work;
> +
> +	BT_ERR("Queue cmd %p opt %p", cmd, cmd->opt);

BT_DBG I guess?

> +
> +	work = kmalloc(sizeof(*work), GFP_ATOMIC);
> +	if (!work)
> +		return;
> +
> +	INIT_WORK(&work->work, hci_cb_work);
> +	work->hdev = hdev;
> +	work->cmd = cmd;
> +	hci_dev_hold(hdev);
> +
> +	if (!queue_work(workqueue, &work->work)) {
> +		kfree(work);
> +		hci_dev_put(hdev);
> +	}
> +}
> +
>  void hci_remove_cb(struct cb_cmd *cmd)
>  {
>  	list_del(&cmd->list);
> 

-- 
BR
Szymon Janc

Re: [RFC 1/2] Bluetooth: General HCI callback implementation

[Andrei Emeltchenko]

Hi Ulisses,

On Tue, Mar 06, 2012 at 11:57:59AM -0300, Ulisses Furquim wrote:
> > diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> > index d12e353..2ef515e 100644
> > --- a/include/net/bluetooth/hci_core.h
> > +++ b/include/net/bluetooth/hci_core.h
> > @@ -131,6 +131,17 @@ struct le_scan_params {
> >
> >  #define HCI_MAX_SHORT_NAME_LENGTH      10
> >
> > +struct hci_dev;
> > +
> > +struct cb_cmd {
> 
> Maybe a better name here with a prefix? hci_cb_cmd?

Yes agree, I will change it to hci_cb_cmd.

> 
> > +       struct list_head list;
> > +       u16 opcode;
> > +       u8 status;
> > +       void *opt;
> > +       void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd);
> > +       void (*destructor)(struct cb_cmd *cmd);
> > +};
> > +
> >  #define NUM_REASSEMBLY 4
> >  struct hci_dev {
> >        struct list_head list;
> > @@ -237,6 +248,7 @@ struct hci_dev {
> >        __u16                   init_last_cmd;
> >
> >        struct list_head        mgmt_pending;
> > +       struct list_head        cb_list;
> >
> >        struct discovery_state  discovery;
> >        struct hci_conn_hash    conn_hash;
> > @@ -1086,4 +1098,10 @@ int hci_cancel_inquiry(struct hci_dev *hdev);
> >  int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
> >                                                                int timeout);
> >
> > +struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode);
> > +int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
> > +       void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd), void *opt,
> > +                               void (*destructor)(struct cb_cmd *cmd));
> 
> This line is very ugly and hard to read. Maybe have a longer line like
> other parts of the kernel seem to be accepting now?

Do you mean line over 80 characters? I think checkpatch do not like that.

> 
> > +void hci_remove_cb(struct cb_cmd *cmd);
> > +
> >  #endif /* __HCI_CORE_H */
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 5b1ddab..cdc0220 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -1826,6 +1826,8 @@ int hci_register_dev(struct hci_dev *hdev)
> >
> >        INIT_LIST_HEAD(&hdev->mgmt_pending);
> >
> > +       INIT_LIST_HEAD(&hdev->cb_list);
> > +
> >        INIT_LIST_HEAD(&hdev->blacklist);
> >
> >        INIT_LIST_HEAD(&hdev->uuids);
> > @@ -2239,6 +2241,61 @@ int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
> >        return 0;
> >  }
> >
> > +void hci_add_cb(struct hci_dev *hdev, __u16 opcode,
> > +                       void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd),
> > +                       void *opt, void (*destructor)(struct cb_cmd *cmd))
> > +{
> > +       struct cb_cmd *cmd;
> > +
> > +       cmd = kmalloc(sizeof(*cmd), GFP_ATOMIC);
> > +       if (!cmd)
> > +               return;
> > +
> > +       cmd->cb = cb;
> > +       cmd->opcode = opcode;
> > +       cmd->opt = opt;
> > +       cmd->status = 0;
> > +
> > +       if (destructor)
> > +               cmd->destructor = destructor;
> > +
> > +       list_add(&cmd->list, &hdev->cb_list);
> 
> Don't we need some protection here? We probably can use hdev lock.

This function is used currently locked, maybe I name it as __hci_add_cb.
> 
> > +}
> > +
> > +struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode)
> > +{
> > +       struct cb_cmd *cmd;
> > +
> > +       list_for_each_entry(cmd, &hdev->cb_list, list)
> > +               if (cmd->opcode == opcode)
> > +                       return cmd;
> 
> Here as well, we might need to protect the critical section.

Here looks like we do not need unlocked version so I will add locks.

> > +
> > +       return NULL;
> > +}
> > +
> > +void hci_remove_cb(struct cb_cmd *cmd)
> > +{
> > +       list_del(&cmd->list);
> > +
> > +       if (cmd->destructor) {
> > +               cmd->destructor(cmd);
> > +       } else {
> > +               kfree(cmd->opt);
> > +               kfree(cmd);
> > +       }
> 
> And here too. Right?

Same as last comment.

> 
> > +}
> > +
> > +/* Send HCI command with callback */
> > +int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
> > +                       void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd),
> > +                       void *opt, void (*destructor)(struct cb_cmd *cmd))
> > +{
> > +       if (cb)
> > +               hci_add_cb(hdev, opcode, cb, opt, destructor);
> > +
> > +       return hci_send_cmd(hdev, opcode, plen, param);
> > +}
> > +
> >  /* Get data from the previously sent command */
> >  void *hci_sent_cmd_data(struct hci_dev *hdev, __u16 opcode)
> >  {
> 
> We don't have any callers of find and remove functions?

Those will be used in functions handling HCI complete event handlers like:

cmd = hci_find_cb(hdev, HCI_OP_READ_LOCAL_AMP_INFO);
if (cmd) {
	cmd->status = rp->status;
	hci_queue_cb(hdev, cmd, hdev->workqueue);
}

Best regards 
Andrei Emeltchenko 

Re: [RFC 1/2] Bluetooth: General HCI callback implementation

[Andrei Emeltchenko]

Hi Janc,

On Tue, Mar 06, 2012 at 04:02:17PM +0100, Szymon Janc wrote:
> > +
> > +struct cb_cmd {
> 
> This name seems a bit too more generic to me, maybe prefix it with hci_?

OK.

> 
> > +	struct list_head list;
> > +	u16 opcode;
> > +	u8 status;
> > +	void *opt;
> > +	void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd);
> > +	void (*destructor)(struct cb_cmd *cmd);
> > +};
> > +
> >  #define NUM_REASSEMBLY 4
> >  struct hci_dev {
> >  	struct list_head list;
> > @@ -237,6 +248,7 @@ struct hci_dev {
> >  	__u16			init_last_cmd;
> >  
> >  	struct list_head	mgmt_pending;
> > +	struct list_head	cb_list;
> >  
> >  	struct discovery_state	discovery;
> >  	struct hci_conn_hash	conn_hash;
> > @@ -1086,4 +1098,10 @@ int hci_cancel_inquiry(struct hci_dev *hdev);
> >  int hci_le_scan(struct hci_dev *hdev, u8 type, u16 interval, u16 window,
> >  								int timeout);
> >  
> > +struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode);
> > +int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
> > +	void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd), void *opt,
> > +				void (*destructor)(struct cb_cmd *cmd));
> > +void hci_remove_cb(struct cb_cmd *cmd);
> > +
> >  #endif /* __HCI_CORE_H */
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 5b1ddab..cdc0220 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -1826,6 +1826,8 @@ int hci_register_dev(struct hci_dev *hdev)
> >  
> >  	INIT_LIST_HEAD(&hdev->mgmt_pending);
> >  
> > +	INIT_LIST_HEAD(&hdev->cb_list);
> > +
> >  	INIT_LIST_HEAD(&hdev->blacklist);
> >  
> >  	INIT_LIST_HEAD(&hdev->uuids);
> > @@ -2239,6 +2241,61 @@ int hci_send_cmd(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param)
> >  	return 0;
> >  }
> >  
> 
> hci_add_cb could return error on failure and that could be used in hci_cmd_cb().
> 
> > +void hci_add_cb(struct hci_dev *hdev, __u16 opcode,
> > +			void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd),
> > +			void *opt, void (*destructor)(struct cb_cmd *cmd))
> > +{
> > +	struct cb_cmd *cmd;
> > +
> > +	cmd = kmalloc(sizeof(*cmd), GFP_ATOMIC);
> 
> Why atomic? Maybe allow to pass custom gfp mask?

I think I will use GFP_KERNEL as Ulisses suggested. I have used the code
when we had tasklets.

> 
> > +	if (!cmd)
> > +		return;
> > +
> > +	cmd->cb = cb;
> > +	cmd->opcode = opcode;
> > +	cmd->opt = opt;
> > +	cmd->status = 0;
> > +
> > +	if (destructor)
> > +		cmd->destructor = destructor;
> 
> That could leave cmd->destructor uninitialized (cmd is allocated with kmalloc).
> I would assign destructor unconditionally.

This makes sense.

> > +
> > +	list_add(&cmd->list, &hdev->cb_list);
> > +}
> > +
> > +struct cb_cmd *hci_find_cb(struct hci_dev *hdev, __u16 opcode)
> > +{
> > +	struct cb_cmd *cmd;
> > +
> > +	list_for_each_entry(cmd, &hdev->cb_list, list)
> > +		if (cmd->opcode == opcode)
> > +			return cmd;
> > +
> > +	return NULL;
> > +}
> > +
> > +void hci_remove_cb(struct cb_cmd *cmd)
> > +{
> > +	list_del(&cmd->list);
> > +
> > +	if (cmd->destructor) {
> > +		cmd->destructor(cmd);
> > +	} else {
> > +		kfree(cmd->opt);
> > +		kfree(cmd);
> > +	}
> > +}
> > +
> > +/* Send HCI command with callback */
> > +int hci_cmd_cb(struct hci_dev *hdev, __u16 opcode, __u32 plen, void *param,
> > +			void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd),
> > +			void *opt, void (*destructor)(struct cb_cmd *cmd))
> > +{
> > +	if (cb)
> > +		hci_add_cb(hdev, opcode, cb, opt, destructor);
> 
> Is this if really needed? If one would like to send cmd without cb he can just
> call hci_send_cmd directly. And I would also check if hci_add_cb failed before
> sending command.

OK I will change the code this way.

Best regards 
Andrei Emeltchenko 

Re: [RFC 2/2] Bluetooth: Process HCI callbacks in a workqueue

[Andrei Emeltchenko]

Hi Ulisses,

On Tue, Mar 06, 2012 at 12:02:11PM -0300, Ulisses Furquim wrote:
> >
> > +struct cb_work {
> 
> Again, maybe hci_cb_work?

OK.

> > +       struct work_struct work;
> > +       struct hci_dev *hdev;
> > +       struct cb_cmd *cmd;
> > +};
> > +
> > +static void hci_cb_work(struct work_struct *w)
> 
> And here hci_cb_worker?

Sounds good.

> > +{
> > +       struct cb_work *work = (struct cb_work *) w;
> > +       struct cb_cmd *cmd = work->cmd;
> > +       struct hci_dev *hdev = work->hdev;
> > +
> > +       cmd->cb(hdev, cmd);
> > +
> > +       hci_dev_put(hdev);
> > +
> > +       hci_remove_cb(cmd);
> > +       kfree(w);
> > +}
> > +
> > +void hci_queue_cb(struct hci_dev *hdev, struct cb_cmd *cmd,
> > +                                       struct workqueue_struct *workqueue)
> > +{
> > +       struct cb_work *work;
> > +
> > +       BT_ERR("Queue cmd %p opt %p", cmd, cmd->opt);
> > +
> > +       work = kmalloc(sizeof(*work), GFP_ATOMIC);
> 
> Why not GFP_KERNEL?

Sure.

> 
> > +       if (!work)
> > +               return;
> > +
> > +       INIT_WORK(&work->work, hci_cb_work);
> > +       work->hdev = hdev;
> > +       work->cmd = cmd;
> > +       hci_dev_hold(hdev);
> > +
> > +       if (!queue_work(workqueue, &work->work)) {
> > +               kfree(work);
> > +               hci_dev_put(hdev);
> > +       }
> > +}
> > +
> >  void hci_remove_cb(struct cb_cmd *cmd)
> >  {
> >        list_del(&cmd->list);
> 
> And again, no callers of hci_queue_cb so I'm assuming you'll only use
> in for AMP, is that it?

Yes, so far.

Thanks for the review.

Best regards 
Andrei Emeltchenko 

Re: [RFC 2/2] Bluetooth: Process HCI callbacks in a workqueue

[Andrei Emeltchenko]

Hi Janc,

On Tue, Mar 06, 2012 at 04:04:02PM +0100, Szymon Janc wrote:
> > +void hci_queue_cb(struct hci_dev *hdev, struct cb_cmd *cmd,
> > +					struct workqueue_struct *workqueue)
> > +{
> > +	struct cb_work *work;
> > +
> > +	BT_ERR("Queue cmd %p opt %p", cmd, cmd->opt);
> 
> BT_DBG I guess?

Sure.

Thanks for the review.

Best regards 
Andrei Emeltchenko 

Re: [RFC 1/2] Bluetooth: General HCI callback implementation

[Andrei Emeltchenko]

Hi Janc,

On Tue, Mar 06, 2012 at 04:02:17PM +0100, Szymon Janc wrote:
> hci_add_cb could return error on failure and that could be used in hci_cmd_cb().
> 
> > +void hci_add_cb(struct hci_dev *hdev, __u16 opcode,
> > +			void (*cb)(struct hci_dev *hdev, struct cb_cmd *cmd),
> > +			void *opt, void (*destructor)(struct cb_cmd *cmd))
> > +{
> > +	struct cb_cmd *cmd;
> > +
> > +	cmd = kmalloc(sizeof(*cmd), GFP_ATOMIC);
> 
> Why atomic? Maybe allow to pass custom gfp mask?

I think I will use gfp mask since I use it also with spinlocks held.

Best regards 
Andrei Emeltchenko 

Re: [RFC 1/2] Bluetooth: General HCI callback implementation

[Gustavo Padovan]

Hi Andrei,

* Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-03-06 15:16:28 +0200]:

> From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> 
> Add general HCI callback implementation. Can be used for executing
> HCI commands from A2MP protocol.

I'm not sure why we need this, can you add a better explanation here?

	Gustavo

Re: [RFC 1/2] Bluetooth: General HCI callback implementation

[Andrei Emeltchenko]

Hi Gustavo,

On Thu, Mar 08, 2012 at 02:53:06AM -0300, Gustavo Padovan wrote:
> Hi Andrei,
> 
> * Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> [2012-03-06 15:16:28 +0200]:
> 
> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > 
> > Add general HCI callback implementation. Can be used for executing
> > HCI commands from A2MP protocol.
> 
> I'm not sure why we need this, can you add a better explanation here?

We need callbacks from HCI events for A2MP protocol.

Best regards 
Andrei Emeltchenko