From: Andrei Emeltchenko <[email protected]>
The code adds AMP HCI commands from A2MP protocol. HCI events are handled
similar way as mgmt interface. amp_pending is a kind of copy of mgmt_pending.
Andrei Emeltchenko (3):
Bluetooth: AMP: HCI infrastructure
Bluetooth: AMP: Initialize and free amp_pending
Bluetooth: AMP: Read Local AMP Info
include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/a2mp.c | 4 +
net/bluetooth/amp.c | 141 ++++++++++++++++++++++++++++++++++++++
net/bluetooth/hci_core.c | 7 ++
net/bluetooth/hci_event.c | 4 +
5 files changed, 157 insertions(+), 0 deletions(-)
create mode 100644 net/bluetooth/amp.c
--
1.7.4.1
Hi Andrei, Marcel
> Hi Marcel,
> > Hi Andrei,
> > > The code adds AMP HCI commands from A2MP protocol. HCI events are handled
> > > similar way as mgmt interface. amp_pending is a kind of copy of mgmt_pending.
> >
> > this is all kernel internal code with no interface to user space. I do
> > not see the need for such a complex infrastructure. Can not just have
> > proper callbacks or event callback table like with L2CAP. Or just
> > something similar.
>
> I see this as a simple callback. amp_pending is just keeping context of HCI
> command we need to handle. I also included reference counting since we had
> bad experience with l2cap and rfcomm.
There does have to be some way to carry the A2MP message context while performing
local HCI operations to service the message. A2MP Get Remote Assoc requires multiple
HCI commands with data accumulated between the commands before the response can be
sent.
> > As far as I see it, we get an A2MP command over L2CAP fixed channel, we
> > have to issue a HCI command or do some other task based on this and then
> > respond to it. We only have one user of this first of all. And second of
> > all, I think we can not really have pending A2MP commands anyway. This
> > is pretty much one command at a time (per ACL link).
A2MP commands are serialized by the sender, so A2MP message context could be
associated with the hci_conn for BR-EDR.
--Peter Krystad
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
> The picture "Figure 7.1: Overview MSC for physical link create/accept"
> BLUETOOTH SPECIFICATION Version 4.0 [Vol 5] page 49 of 60
> shows quite a lot of commands between A2MP messages, I feel that if we
> have just callback from HCI event to handle A2MP responses it might be
> difficult to sync them.
>
> Best regards
> Andrei Emeltchenko
>
> > If I am mistaken here, please correct here. It has been a while since I
> > read that specification.
> >
> > Regards
> >
> > Marcel
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marcel,
On Wed, Nov 30, 2011 at 03:31:24PM +0100, Marcel Holtmann wrote:
> Hi Andrei,
>
> > The code adds AMP HCI commands from A2MP protocol. HCI events are handled
> > similar way as mgmt interface. amp_pending is a kind of copy of mgmt_pending.
>
> this is all kernel internal code with no interface to user space. I do
> not see the need for such a complex infrastructure. Can not just have
> proper callbacks or event callback table like with L2CAP. Or just
> something similar.
I see this as a simple callback. amp_pending is just keeping context of HCI
command we need to handle. I also included reference counting since we had
bad experience with l2cap and rfcomm.
> As far as I see it, we get an A2MP command over L2CAP fixed channel, we
> have to issue a HCI command or do some other task based on this and then
> respond to it. We only have one user of this first of all. And second of
> all, I think we can not really have pending A2MP commands anyway. This
> is pretty much one command at a time (per ACL link).
The picture "Figure 7.1: Overview MSC for physical link create/accept"
BLUETOOTH SPECIFICATION Version 4.0 [Vol 5] page 49 of 60
shows quite a lot of commands between A2MP messages, I feel that if we
have just callback from HCI event to handle A2MP responses it might be
difficult to sync them.
Best regards
Andrei Emeltchenko
> If I am mistaken here, please correct here. It has been a while since I
> read that specification.
>
> Regards
>
> Marcel
>
>
Hi Andrei,
> The code adds AMP HCI commands from A2MP protocol. HCI events are handled
> similar way as mgmt interface. amp_pending is a kind of copy of mgmt_pending.
this is all kernel internal code with no interface to user space. I do
not see the need for such a complex infrastructure. Can not just have
proper callbacks or event callback table like with L2CAP. Or just
something similar.
As far as I see it, we get an A2MP command over L2CAP fixed channel, we
have to issue a HCI command or do some other task based on this and then
respond to it. We only have one user of this first of all. And second of
all, I think we can not really have pending A2MP commands anyway. This
is pretty much one command at a time (per ACL link).
If I am mistaken here, please correct here. It has been a while since I
read that specification.
Regards
Marcel
From: Andrei Emeltchenko <[email protected]>
Implementing amp pending HCI function to deal with executing AMP
HCI commands from A2MP protocol.
Code copied from mgmt interface.
---
include/net/bluetooth/hci_core.h | 1 +
net/bluetooth/amp.c | 79 ++++++++++++++++++++++++++++++++++++++
2 files changed, 80 insertions(+), 0 deletions(-)
create mode 100644 net/bluetooth/amp.c
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 6f2397e..1e53444 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -225,6 +225,7 @@ struct hci_dev {
__u16 init_last_cmd;
struct list_head mgmt_pending;
+ struct list_head amp_pending;
struct inquiry_cache inq_cache;
struct hci_conn_hash conn_hash;
diff --git a/net/bluetooth/amp.c b/net/bluetooth/amp.c
new file mode 100644
index 0000000..d5c48b6
--- /dev/null
+++ b/net/bluetooth/amp.c
@@ -0,0 +1,79 @@
+#include <net/bluetooth/bluetooth.h>
+#include <net/bluetooth/hci.h>
+#include <net/bluetooth/hci_core.h>
+#include <net/bluetooth/a2mp.h>
+#include <linux/workqueue.h>
+
+struct pending_amp {
+ struct list_head list;
+ u16 opcode;
+ int index;
+ void *param;
+ struct sock *sk;
+ void *user_data;
+ struct amp_mgr *mgr;
+};
+
+static struct pending_amp *amp_pending_find(u16 opcode, struct hci_dev *hdev)
+{
+ struct pending_amp *cmd;
+
+ list_for_each_entry(cmd, &hdev->amp_pending, list)
+ if (cmd->opcode == opcode)
+ return cmd;
+
+ return NULL;
+}
+
+static struct pending_amp *amp_pending_add(struct amp_mgr *mgr, u16 opcode,
+ struct hci_dev *hdev, void *data, u16 len)
+{
+ struct pending_amp *cmd;
+
+ cmd = kmalloc(sizeof(*cmd), GFP_ATOMIC);
+ if (!cmd)
+ return NULL;
+
+ cmd->opcode = opcode;
+ cmd->index = hdev->id;
+
+ cmd->param = kmalloc(len, GFP_ATOMIC);
+ if (!cmd->param) {
+ kfree(cmd);
+ return NULL;
+ }
+
+ if (data)
+ memcpy(cmd->param, data, len);
+
+ amp_mgr_get(mgr);
+ cmd->mgr = mgr;
+ cmd->sk = mgr->a2mp_sock->sk;
+ sock_hold(cmd->sk);
+
+ list_add(&cmd->list, &hdev->amp_pending);
+
+ return cmd;
+}
+
+static void amp_pending_free(struct pending_amp *cmd)
+{
+ sock_put(cmd->sk);
+ amp_mgr_put(cmd->mgr);
+ kfree(cmd->param);
+ kfree(cmd);
+}
+
+static void amp_pending_remove(struct pending_amp *cmd)
+{
+ list_del(&cmd->list);
+ amp_pending_free(cmd);
+}
+
+void amp_pending_remove_all(struct hci_dev *hdev)
+{
+ struct pending_amp *cmd;
+
+ list_for_each_entry(cmd, &hdev->amp_pending, list)
+ amp_pending_remove(cmd);
+}
--
1.7.4.1
From: Andrei Emeltchenko <[email protected]>
Execute HCI command Read Local AMP Info upon receiving A2MP
Discover Request
---
net/bluetooth/a2mp.c | 4 +++
net/bluetooth/amp.c | 62 +++++++++++++++++++++++++++++++++++++++++++++
net/bluetooth/hci_event.c | 4 +++
3 files changed, 70 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/a2mp.c b/net/bluetooth/a2mp.c
index b6b9cde..49f9978 100644
--- a/net/bluetooth/a2mp.c
+++ b/net/bluetooth/a2mp.c
@@ -24,6 +24,8 @@ struct workqueue_struct *a2mp_workqueue;
static struct amp_mgr *get_amp_mgr_sk(struct sock *sk);
+int amp_read_local_amp_info(struct hci_dev *hdev, struct amp_mgr *mgr);
+
LIST_HEAD(amp_mgr_list);
DEFINE_RWLOCK(amp_mgr_list_lock);
@@ -99,6 +101,8 @@ static void __a2mp_add_cl(struct amp_mgr *mgr, struct a2mp_cl *cl, u8 num_ctrl)
cl[i].id = hdev->id;
cl[i].type = hdev->amp_type;
cl[i].status = hdev->amp_status;
+
+ amp_read_local_amp_info(hdev, mgr);
}
out:
read_unlock_bh(&hci_dev_list_lock);
diff --git a/net/bluetooth/amp.c b/net/bluetooth/amp.c
index d5c48b6..218ae29 100644
--- a/net/bluetooth/amp.c
+++ b/net/bluetooth/amp.c
@@ -77,3 +77,65 @@ void amp_pending_remove_all(struct hci_dev *hdev)
list_for_each_entry(cmd, &hdev->amp_pending, list)
amp_pending_remove(cmd);
}
+
+/* Invoked with hci_dev locked */
+int amp_read_local_amp_info(struct hci_dev *hdev, struct amp_mgr *mgr)
+{
+ int err = 0;
+ struct pending_amp *cmd;
+
+ cmd = amp_pending_add(mgr, HCI_OP_READ_LOCAL_AMP_INFO, hdev, NULL, 0);
+ if (!cmd) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ BT_DBG("Read Local AMP Info for %s cmd %p", hdev->name, cmd);
+
+ err = hci_send_cmd(hdev, HCI_OP_READ_LOCAL_AMP_INFO, 0, NULL);
+ if (err < 0)
+ amp_pending_remove(cmd);
+out:
+ return err;
+}
+
+struct amp_work {
+ struct work_struct work;
+ struct hci_dev *hdev;
+};
+
+static void amp_local_info_wrk(struct work_struct *w)
+{
+ struct amp_work *work = (struct amp_work *) w;
+ struct pending_amp *cmd;
+ struct hci_dev *hdev;
+
+ hdev = work->hdev;
+
+ cmd = amp_pending_find(HCI_OP_READ_LOCAL_AMP_INFO, hdev);
+ if (cmd) {
+ BT_DBG("Found pending amp cmd %p", cmd);
+
+ /* TODO correct AMP information if needed */
+
+ amp_pending_remove(cmd);
+ }
+
+ hci_dev_put(hdev);
+}
+
+void amp_read_local_amp_info_complete(struct hci_dev *hdev)
+{
+ struct amp_work *work;
+
+ work = kmalloc(sizeof(*work), GFP_ATOMIC);
+ if (work) {
+ INIT_WORK(&work->work, amp_local_info_wrk);
+ work->hdev = hdev;
+ hci_dev_hold(hdev);
+ if (!queue_work(a2mp_workqueue, &work->work)) {
+ kfree(work);
+ hci_dev_put(hdev);
+ }
+ }
+}
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 2ada168..fa66d88 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -45,6 +45,8 @@
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/hci_core.h>
+int amp_read_local_amp_info_complete(struct hci_dev *hdev);
+
static int enable_le;
/* Handle HCI Event packets */
@@ -835,6 +837,8 @@ static void hci_cc_read_local_amp_info(struct hci_dev *hdev,
hdev->amp_max_flush_to = __le32_to_cpu(rp->max_flush_to);
hci_req_complete(hdev, HCI_OP_READ_LOCAL_AMP_INFO, rp->status);
+
+ amp_read_local_amp_info_complete(hdev);
}
static void hci_cc_delete_stored_link_key(struct hci_dev *hdev,
--
1.7.4.1
From: Andrei Emeltchenko <[email protected]>
amp_pending is used as a callback when receiving HCI response event.
Idea is taken from mgmt interface (mgmt_pending).
---
net/bluetooth/hci_core.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index fa33b27..331061f 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -60,6 +60,8 @@ static void hci_cmd_task(unsigned long arg);
static void hci_rx_task(unsigned long arg);
static void hci_tx_task(unsigned long arg);
+void amp_pending_remove_all(struct hci_dev *hdev);
+
static DEFINE_RWLOCK(hci_task_lock);
/* HCI device list */
@@ -1559,6 +1561,8 @@ int hci_register_dev(struct hci_dev *hdev)
INIT_LIST_HEAD(&hdev->mgmt_pending);
+ INIT_LIST_HEAD(&hdev->amp_pending);
+
INIT_LIST_HEAD(&hdev->blacklist);
INIT_LIST_HEAD(&hdev->uuids);
@@ -1640,6 +1644,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
!test_bit(HCI_SETUP, &hdev->flags)) {
hci_dev_lock_bh(hdev);
mgmt_index_removed(hdev);
+ amp_pending_remove_all(hdev);
hci_dev_unlock_bh(hdev);
}
@@ -1647,6 +1652,8 @@ void hci_unregister_dev(struct hci_dev *hdev)
* pending list */
BUG_ON(!list_empty(&hdev->mgmt_pending));
+ BUG_ON(!list_empty(&hdev->amp_pending));
+
hci_notify(hdev, HCI_DEV_UNREG);
if (hdev->rfkill) {
--
1.7.4.1
Hi Peter,
On Thu, Dec 01, 2011 at 10:39:03PM -0800, Peter Krystad wrote:
> > > > > > The code adds AMP HCI commands from A2MP protocol. HCI events are handled
> > > > > > similar way as mgmt interface. amp_pending is a kind of copy of mgmt_pending.
> > > > >
> > > > > this is all kernel internal code with no interface to user space. I do
> > > > > not see the need for such a complex infrastructure. Can not just have
> > > > > proper callbacks or event callback table like with L2CAP. Or just
> > > > > something similar.
> > > >
> > > > I see this as a simple callback. amp_pending is just keeping context of HCI
> > > > command we need to handle. I also included reference counting since we had
> > > > bad experience with l2cap and rfcomm.
> > >
> > > There does have to be some way to carry the A2MP message context while performing
> > > local HCI operations to service the message. A2MP Get Remote Assoc requires multiple
> > > HCI commands with data accumulated between the commands before the response can be
> > > sent.
> >
> > I agree, but this amp_pending seems to be wrong. I talked about changing
> > the init handling and at the same time we might can apply this to the
> > A2MP handling.
>
> I don't think this amp_pending is the right approach either.
>
> > Why not just set a bit in hdev->dev_flags or maybe even hcon->dev_flags
> > or something and based on that we do a nice async state machine that
> > sends the commands and results in calling back into A2MP core when its
> > finished.
>
> I agree we can build a nice async state machine. But it needs to be carry more data
> than a bit in the hci_dev->flags. 1) For every A2MP request received we have
> to hang onto the request identifier byte to be returned in the response.
> 2) For processing the A2MP Get AMP Assoc the partial assoc data has to be accumulated
> over multiple HCI calls before it can be returned in the response. 3) In the
> create physical link sequence the remote assoc has to be stored while it is fed
> to HCI in multiple commands, and after the channel select event the local assoc
> has to be accumulated before it can be sent back to the remote.
>
> > That seems a bit simpler to me and more aligned in a way I wanna redo
> > the whole HCI command/event handling anyway. In addition we can just
> > expose these flags via debugfs as text and have a nice way of knowing
> > current states if something goes wrong.
>
> In the CAF implementation there is a separate set of data structures that track all
> this context. I (and I think Andrei) think that that isn't the way to go. Storing
> the context in the hci_conn seems the best without adding more complexity. It needs
You are referring to HCI connection between BR/EDR controllers? But
context is needed when you get HCI event from AMP controller, then you
have only HCI id of AMP controller.
> to be on hci_conn not hci_dev because it possible for more than one remote AMP to send
> an A2MP command at one time. There will still need to be a flag in hci_dev->flags
This is easy solvable with amp_pending list.
> that serializes create physical link attempts, as PALs may require these to be serialized.
>
> There is a union in the structure amp_ctx in the CAF code that summarizes the data
> that needs to kept around. I've included it at the end of this e-mail. I'm not suggesting
> use this as is, just that it shows what state has to be kept while processing
> the A2MP requests.
I feel that some data might be stored in some phy_link structure
especially state of the connection, etc.
Best regards
Andrei Emeltchenko
>
> Regards,
>
> Peter.
>
> > > > > As far as I see it, we get an A2MP command over L2CAP fixed channel, we
> > > > > have to issue a HCI command or do some other task based on this and then
> > > > > respond to it. We only have one user of this first of all. And second of
> > > > > all, I think we can not really have pending A2MP commands anyway. This
> > > > > is pretty much one command at a time (per ACL link).
> > >
> > > A2MP commands are serialized by the sender, so A2MP message context could be
> > > associated with the hci_conn for BR-EDR.
> >
> > That is what I thought. Thanks for confirming. So using hcon->dev_flags
> > seems like a possible way to make this a lot simpler.
> >
> > And if the sender misbehaves, we just reject that commands that way.
> > Testing a flag is always cheaper then iterating a list.
> >
> > Regards
> >
> > Marcel
> >
>
> /* Get AMP Assoc sequence */
> struct amp_gaa_state {
> __u8 req_ident;
> __u16 len_so_far;
> __u8 *assoc;
> };
>
> /* Create Physical Link sequence */
> struct amp_cpl_state {
> __u8 remote_id;
> __u16 max_len;
> __u8 *remote_assoc;
> __u8 *local_assoc;
> __u16 len_so_far;
> __u16 rem_len;
> __u8 phy_handle;
> };
>
> /* Accept Physical Link sequence */
> struct amp_apl_state {
> __u8 remote_id;
> __u8 req_ident;
> __u8 *remote_assoc;
> __u16 len_so_far;
> __u16 rem_len;
> __u8 phy_handle;
> };
>
> struct amp_ctx {
> struct list_head list;
> struct amp_mgr *mgr;
> struct hci_dev *hdev;
> __u8 type;
> __u8 state;
> union {
> struct amp_gaa_state gaa;
> struct amp_cpl_state cpl;
> struct amp_apl_state apl;
> } d;
> __u8 evt_type;
> __u8 evt_code;
> __u16 opcode;
> __u8 id;
> __u8 rsp_ident;
>
> struct sock *sk;
> struct amp_ctx *deferred;
> struct timer_list timer;
> };
>
> --Peter Krystad
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
>
>
>
> -----Original Message-----
> From: Marcel Holtmann [mailto:[email protected]]
>
> Hi Peter,
>
> > > > > The code adds AMP HCI commands from A2MP protocol. HCI events are handled
> > > > > similar way as mgmt interface. amp_pending is a kind of copy of mgmt_pending.
> > > >
> > > > this is all kernel internal code with no interface to user space. I do
> > > > not see the need for such a complex infrastructure. Can not just have
> > > > proper callbacks or event callback table like with L2CAP. Or just
> > > > something similar.
> > >
> > > I see this as a simple callback. amp_pending is just keeping context of HCI
> > > command we need to handle. I also included reference counting since we had
> > > bad experience with l2cap and rfcomm.
> >
> > There does have to be some way to carry the A2MP message context while performing
> > local HCI operations to service the message. A2MP Get Remote Assoc requires multiple
> > HCI commands with data accumulated between the commands before the response can be
> > sent.
>
> I agree, but this amp_pending seems to be wrong. I talked about changing
> the init handling and at the same time we might can apply this to the
> A2MP handling.
I don't think this amp_pending is the right approach either.
> Why not just set a bit in hdev->dev_flags or maybe even hcon->dev_flags
> or something and based on that we do a nice async state machine that
> sends the commands and results in calling back into A2MP core when its
> finished.
I agree we can build a nice async state machine. But it needs to be carry more data
than a bit in the hci_dev->flags. 1) For every A2MP request received we have
to hang onto the request identifier byte to be returned in the response.
2) For processing the A2MP Get AMP Assoc the partial assoc data has to be accumulated
over multiple HCI calls before it can be returned in the response. 3) In the
create physical link sequence the remote assoc has to be stored while it is fed
to HCI in multiple commands, and after the channel select event the local assoc
has to be accumulated before it can be sent back to the remote.
> That seems a bit simpler to me and more aligned in a way I wanna redo
> the whole HCI command/event handling anyway. In addition we can just
> expose these flags via debugfs as text and have a nice way of knowing
> current states if something goes wrong.
In the CAF implementation there is a separate set of data structures that track all
this context. I (and I think Andrei) think that that isn't the way to go. Storing
the context in the hci_conn seems the best without adding more complexity. It needs
to be on hci_conn not hci_dev because it possible for more than one remote AMP to send
an A2MP command at one time. There will still need to be a flag in hci_dev->flags
that serializes create physical link attempts, as PALs may require these to be serialized.
There is a union in the structure amp_ctx in the CAF code that summarizes the data
that needs to kept around. I've included it at the end of this e-mail. I'm not suggesting
use this as is, just that it shows what state has to be kept while processing
the A2MP requests.
Regards,
Peter.
> > > > As far as I see it, we get an A2MP command over L2CAP fixed channel, we
> > > > have to issue a HCI command or do some other task based on this and then
> > > > respond to it. We only have one user of this first of all. And second of
> > > > all, I think we can not really have pending A2MP commands anyway. This
> > > > is pretty much one command at a time (per ACL link).
> >
> > A2MP commands are serialized by the sender, so A2MP message context could be
> > associated with the hci_conn for BR-EDR.
>
> That is what I thought. Thanks for confirming. So using hcon->dev_flags
> seems like a possible way to make this a lot simpler.
>
> And if the sender misbehaves, we just reject that commands that way.
> Testing a flag is always cheaper then iterating a list.
>
> Regards
>
> Marcel
>
/* Get AMP Assoc sequence */
struct amp_gaa_state {
__u8 req_ident;
__u16 len_so_far;
__u8 *assoc;
};
/* Create Physical Link sequence */
struct amp_cpl_state {
__u8 remote_id;
__u16 max_len;
__u8 *remote_assoc;
__u8 *local_assoc;
__u16 len_so_far;
__u16 rem_len;
__u8 phy_handle;
};
/* Accept Physical Link sequence */
struct amp_apl_state {
__u8 remote_id;
__u8 req_ident;
__u8 *remote_assoc;
__u16 len_so_far;
__u16 rem_len;
__u8 phy_handle;
};
struct amp_ctx {
struct list_head list;
struct amp_mgr *mgr;
struct hci_dev *hdev;
__u8 type;
__u8 state;
union {
struct amp_gaa_state gaa;
struct amp_cpl_state cpl;
struct amp_apl_state apl;
} d;
__u8 evt_type;
__u8 evt_code;
__u16 opcode;
__u8 id;
__u8 rsp_ident;
struct sock *sk;
struct amp_ctx *deferred;
struct timer_list timer;
};
--Peter Krystad
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum
Hi Peter,
> > > > The code adds AMP HCI commands from A2MP protocol. HCI events are handled
> > > > similar way as mgmt interface. amp_pending is a kind of copy of mgmt_pending.
> > >
> > > this is all kernel internal code with no interface to user space. I do
> > > not see the need for such a complex infrastructure. Can not just have
> > > proper callbacks or event callback table like with L2CAP. Or just
> > > something similar.
> >
> > I see this as a simple callback. amp_pending is just keeping context of HCI
> > command we need to handle. I also included reference counting since we had
> > bad experience with l2cap and rfcomm.
>
> There does have to be some way to carry the A2MP message context while performing
> local HCI operations to service the message. A2MP Get Remote Assoc requires multiple
> HCI commands with data accumulated between the commands before the response can be
> sent.
I agree, but this amp_pending seems to be wrong. I talked about changing
the init handling and at the same time we might can apply this to the
A2MP handling.
Why not just set a bit in hdev->dev_flags or maybe even hcon->dev_flags
or something and based on that we do a nice async state machine that
sends the commands and results in calling back into A2MP core when its
finished.
That seems a bit simpler to me and more aligned in a way I wanna redo
the whole HCI command/event handling anyway. In addition we can just
expose these flags via debugfs as text and have a nice way of knowing
current states if something goes wrong.
> > > As far as I see it, we get an A2MP command over L2CAP fixed channel, we
> > > have to issue a HCI command or do some other task based on this and then
> > > respond to it. We only have one user of this first of all. And second of
> > > all, I think we can not really have pending A2MP commands anyway. This
> > > is pretty much one command at a time (per ACL link).
>
> A2MP commands are serialized by the sender, so A2MP message context could be
> associated with the hci_conn for BR-EDR.
That is what I thought. Thanks for confirming. So using hcon->dev_flags
seems like a possible way to make this a lot simpler.
And if the sender misbehaves, we just reject that commands that way.
Testing a flag is always cheaper then iterating a list.
Regards
Marcel