2012-03-06 13:16:28

by Andrei Emeltchenko

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

From: Andrei Emeltchenko <[email protected]>

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

Signed-off-by: Andrei Emeltchenko <[email protected]>
---
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



2012-03-08 07:58:52

by Andrei Emeltchenko

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

Hi Gustavo,

On Thu, Mar 08, 2012 at 02:53:06AM -0300, Gustavo Padovan wrote:
> Hi Andrei,
>
> * Andrei Emeltchenko <[email protected]> [2012-03-06 15:16:28 +0200]:
>
> > From: Andrei Emeltchenko <[email protected]>
> >
> > 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


2012-03-08 05:53:06

by Gustavo Padovan

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

Hi Andrei,

* Andrei Emeltchenko <[email protected]> [2012-03-06 15:16:28 +0200]:

> From: Andrei Emeltchenko <[email protected]>
>
> 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

2012-03-07 15:50:24

by Andrei Emeltchenko

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

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


2012-03-07 09:23:36

by Andrei Emeltchenko

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

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


2012-03-07 09:22:15

by Andrei Emeltchenko

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

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


2012-03-07 09:19:31

by Andrei Emeltchenko

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

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

2012-03-07 09:13:27

by Andrei Emeltchenko

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

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


2012-03-06 15:04:02

by Szymon Janc

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

Hi Andrei,

> Signed-off-by: Andrei Emeltchenko <[email protected]>
> ---
> 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

2012-03-06 15:02:17

by Szymon Janc

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

> From: Andrei Emeltchenko <[email protected]>

Hi Andrei,

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

> Signed-off-by: Andrei Emeltchenko <[email protected]>
> ---
> 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)
> {
>

2012-03-06 15:02:11

by Ulisses Furquim

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

Hi Andrei,

On Tue, Mar 6, 2012 at 10:16 AM, Andrei Emeltchenko
<[email protected]> wrote:
> From: Andrei Emeltchenko <[email protected]>
>
>
> Signed-off-by: Andrei Emeltchenko <[email protected]>
> ---
> ?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

2012-03-06 14:59:47

by Ulisses Furquim

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

Hi Andrei,

I forgot one comment.

On Tue, Mar 6, 2012 at 11:57 AM, Ulisses Furquim <[email protected]> wrote:
> Hi Andrei,
>
> On Tue, Mar 6, 2012 at 10:16 AM, Andrei Emeltchenko
> <[email protected]> wrote:
>> From: Andrei Emeltchenko <[email protected]>
>>
>> Add general HCI callback implementation. Can be used for executing
>> HCI commands from A2MP protocol.
>>
>> Signed-off-by: Andrei Emeltchenko <[email protected]>
>> ---
>> ?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

2012-03-06 14:57:59

by Ulisses Furquim

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

Hi Andrei,

On Tue, Mar 6, 2012 at 10:16 AM, Andrei Emeltchenko
<[email protected]> wrote:
> From: Andrei Emeltchenko <[email protected]>
>
> Add general HCI callback implementation. Can be used for executing
> HCI commands from A2MP protocol.
>
> Signed-off-by: Andrei Emeltchenko <[email protected]>
> ---
> ?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

2012-03-06 13:16:29

by Andrei Emeltchenko

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

From: Andrei Emeltchenko <[email protected]>


Signed-off-by: Andrei Emeltchenko <[email protected]>
---
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