* [RFC 2/2] Bluetooth: Process HCI callbacks in a workqueue
2012-03-06 13:16 [RFC 1/2] Bluetooth: General HCI callback implementation Andrei Emeltchenko
@ 2012-03-06 13:16 ` Andrei Emeltchenko
2012-03-06 15:02 ` Ulisses Furquim
2012-03-06 15:04 ` Szymon Janc
2012-03-06 14:57 ` [RFC 1/2] Bluetooth: General HCI callback implementation Ulisses Furquim
` (2 subsequent siblings)
3 siblings, 2 replies; 14+ messages in thread
From: Andrei Emeltchenko @ 2012-03-06 13:16 UTC (permalink / raw)
To: linux-bluetooth
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [RFC 2/2] Bluetooth: Process HCI callbacks in a workqueue
2012-03-06 13:16 ` [RFC 2/2] Bluetooth: Process HCI callbacks in a workqueue Andrei Emeltchenko
@ 2012-03-06 15:02 ` Ulisses Furquim
2012-03-07 9:22 ` Andrei Emeltchenko
2012-03-06 15:04 ` Szymon Janc
1 sibling, 1 reply; 14+ messages in thread
From: Ulisses Furquim @ 2012-03-06 15:02 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [RFC 2/2] Bluetooth: Process HCI callbacks in a workqueue
2012-03-06 15:02 ` Ulisses Furquim
@ 2012-03-07 9:22 ` Andrei Emeltchenko
0 siblings, 0 replies; 14+ messages in thread
From: Andrei Emeltchenko @ 2012-03-07 9:22 UTC (permalink / raw)
To: Ulisses Furquim; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 2/2] Bluetooth: Process HCI callbacks in a workqueue
2012-03-06 13:16 ` [RFC 2/2] Bluetooth: Process HCI callbacks in a workqueue Andrei Emeltchenko
2012-03-06 15:02 ` Ulisses Furquim
@ 2012-03-06 15:04 ` Szymon Janc
2012-03-07 9:23 ` Andrei Emeltchenko
1 sibling, 1 reply; 14+ messages in thread
From: Szymon Janc @ 2012-03-06 15:04 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth@vger.kernel.org
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 1/2] Bluetooth: General HCI callback implementation
2012-03-06 13:16 [RFC 1/2] Bluetooth: General HCI callback implementation Andrei Emeltchenko
2012-03-06 13:16 ` [RFC 2/2] Bluetooth: Process HCI callbacks in a workqueue Andrei Emeltchenko
@ 2012-03-06 14:57 ` Ulisses Furquim
2012-03-06 14:59 ` Ulisses Furquim
2012-03-07 9:13 ` Andrei Emeltchenko
2012-03-06 15:02 ` Szymon Janc
2012-03-08 5:53 ` Gustavo Padovan
3 siblings, 2 replies; 14+ messages in thread
From: Ulisses Furquim @ 2012-03-06 14:57 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [RFC 1/2] Bluetooth: General HCI callback implementation
2012-03-06 14:57 ` [RFC 1/2] Bluetooth: General HCI callback implementation Ulisses Furquim
@ 2012-03-06 14:59 ` Ulisses Furquim
2012-03-07 9:13 ` Andrei Emeltchenko
1 sibling, 0 replies; 14+ messages in thread
From: Ulisses Furquim @ 2012-03-06 14:59 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [RFC 1/2] Bluetooth: General HCI callback implementation
2012-03-06 14:57 ` [RFC 1/2] Bluetooth: General HCI callback implementation Ulisses Furquim
2012-03-06 14:59 ` Ulisses Furquim
@ 2012-03-07 9:13 ` Andrei Emeltchenko
1 sibling, 0 replies; 14+ messages in thread
From: Andrei Emeltchenko @ 2012-03-07 9:13 UTC (permalink / raw)
To: Ulisses Furquim; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 1/2] Bluetooth: General HCI callback implementation
2012-03-06 13:16 [RFC 1/2] Bluetooth: General HCI callback implementation Andrei Emeltchenko
2012-03-06 13:16 ` [RFC 2/2] Bluetooth: Process HCI callbacks in a workqueue Andrei Emeltchenko
2012-03-06 14:57 ` [RFC 1/2] Bluetooth: General HCI callback implementation Ulisses Furquim
@ 2012-03-06 15:02 ` Szymon Janc
2012-03-07 9:19 ` Andrei Emeltchenko
2012-03-07 15:50 ` Andrei Emeltchenko
2012-03-08 5:53 ` Gustavo Padovan
3 siblings, 2 replies; 14+ messages in thread
From: Szymon Janc @ 2012-03-06 15:02 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth@vger.kernel.org
> 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)
> {
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [RFC 1/2] Bluetooth: General HCI callback implementation
2012-03-06 15:02 ` Szymon Janc
@ 2012-03-07 9:19 ` Andrei Emeltchenko
2012-03-07 15:50 ` Andrei Emeltchenko
1 sibling, 0 replies; 14+ messages in thread
From: Andrei Emeltchenko @ 2012-03-07 9:19 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [RFC 1/2] Bluetooth: General HCI callback implementation
2012-03-06 15:02 ` Szymon Janc
2012-03-07 9:19 ` Andrei Emeltchenko
@ 2012-03-07 15:50 ` Andrei Emeltchenko
1 sibling, 0 replies; 14+ messages in thread
From: Andrei Emeltchenko @ 2012-03-07 15:50 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 1/2] Bluetooth: General HCI callback implementation
2012-03-06 13:16 [RFC 1/2] Bluetooth: General HCI callback implementation Andrei Emeltchenko
` (2 preceding siblings ...)
2012-03-06 15:02 ` Szymon Janc
@ 2012-03-08 5:53 ` Gustavo Padovan
2012-03-08 7:58 ` Andrei Emeltchenko
3 siblings, 1 reply; 14+ messages in thread
From: Gustavo Padovan @ 2012-03-08 5:53 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
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
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [RFC 1/2] Bluetooth: General HCI callback implementation
2012-03-08 5:53 ` Gustavo Padovan
@ 2012-03-08 7:58 ` Andrei Emeltchenko
0 siblings, 0 replies; 14+ messages in thread
From: Andrei Emeltchenko @ 2012-03-08 7:58 UTC (permalink / raw)
To: linux-bluetooth
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
^ permalink raw reply [flat|nested] 14+ messages in thread