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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
