* Re: [PATCHv2 10/19] Bluetooth: Add logical link confirm
From: Marcel Holtmann @ 2012-10-17 17:22 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, sunnyk
In-Reply-To: <1350430593-23565-11-git-send-email-mathewm@codeaurora.org>
Hi Mat,
> The logical link confirm callback is executed when the AMP controller
> completes its logical link setup. During a channel move, a newly
> formed logical link allows a move responder to send a move channel
> response. A move initiator will send a move channel confirm. A
> failed logical link will end the channel move and send an appropriate
> response or confirm command indicating a failure.
>
> If the channel is being created on an AMP controller, L2CAP
> configuration is started after the logical link is set up.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/l2cap_core.c | 114 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 112 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 5e4796c..ddd8c72 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -3799,6 +3799,7 @@ static inline int l2cap_config_req(struct l2cap_conn *conn,
> goto unlock;
> }
>
> + chan->ident = cmd->ident;
> l2cap_send_cmd(conn, cmd->ident, L2CAP_CONF_RSP, len, rsp);
> chan->num_conf_rsp++;
>
> @@ -4241,11 +4242,120 @@ static void l2cap_send_move_chan_cfm_rsp(struct l2cap_conn *conn, u8 ident,
> l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_CFM_RSP, sizeof(rsp), &rsp);
> }
>
> +/* Call with chan locked */
> static void l2cap_logical_cfm(struct l2cap_chan *chan, struct hci_chan *hchan,
> u8 status)
> {
> - /* Placeholder */
> - return;
> + BT_DBG("chan %p, hchan %p, status %d", chan, hchan, status);
> +
> + if (status) {
> + /* Logical link setup failed */
> + if (chan->state != BT_CONNECTED) {
> + /* Create channel failure, disconnect */
> + l2cap_send_disconn_req(chan->conn, chan, ECONNRESET);
> + } else if (chan->move_role == L2CAP_MOVE_ROLE_RESPONDER) {
> + l2cap_move_revert(chan);
> + chan->move_role = L2CAP_MOVE_ROLE_NONE;
> + chan->move_state = L2CAP_MOVE_STABLE;
> + l2cap_send_move_chan_rsp(chan->conn, chan->ident,
> + chan->dcid, L2CAP_MR_NOT_SUPP);
> + } else if (chan->move_role == L2CAP_MOVE_ROLE_INITIATOR) {
> + if (chan->move_state == L2CAP_MOVE_WAIT_LOGICAL_COMP ||
> + chan->move_state == L2CAP_MOVE_WAIT_LOGICAL_CFM) {
> + /* Remote has only sent pending or
> + * success responses, clean up
> + */
> + l2cap_move_revert(chan);
> + chan->move_role = L2CAP_MOVE_ROLE_NONE;
> + chan->move_state = L2CAP_MOVE_STABLE;
> + }
> +
> + /* Other amp move states imply that the move
> + * has already aborted
> + */
> + l2cap_send_move_chan_cfm(chan->conn, chan, chan->scid,
> + L2CAP_MC_UNCONFIRMED);
> + __set_chan_timer(chan, L2CAP_MOVE_TIMEOUT);
> + }
> +
> + chan->hs_hchan = NULL;
> + chan->hs_hcon = NULL;
> +
> + /* Placeholder - free logical link */
> +
> + } else if (chan->state != BT_CONNECTED) {
> + struct l2cap_conf_rsp rsp;
> + u8 code;
> +
> + /* Create channel complete */
> +
> + /* Ignore logical link if channel is on BR/EDR */
> + if (!chan->local_amp_id)
> + return;
> +
> + chan->hs_hcon = hchan->conn;
> + chan->hs_hcon->l2cap_data = chan->conn;
> +
> + code = l2cap_build_conf_rsp(chan, &rsp,
> + L2CAP_CONF_SUCCESS, 0);
> + l2cap_send_cmd(chan->conn, chan->ident, L2CAP_CONF_RSP, code,
> + &rsp);
> + set_bit(CONF_OUTPUT_DONE, &chan->conf_state);
> +
> + if (test_bit(CONF_INPUT_DONE, &chan->conf_state)) {
> + int err = 0;
> +
> + set_default_fcs(chan);
> +
> + err = l2cap_ertm_init(chan);
> + if (err < 0)
> + l2cap_send_disconn_req(chan->conn, chan, -err);
> + else
> + l2cap_chan_ready(chan);
> + }
> + } else {
> + /* Channel move */
> + chan->hs_hcon = hchan->conn;
> + chan->hs_hcon->l2cap_data = chan->conn;
> +
> + BT_DBG("move_state %d", chan->move_state);
> +
> + switch (chan->move_state) {
> + case L2CAP_MOVE_WAIT_LOGICAL_COMP:
> + /* Move confirm will be sent after a success
> + * response is received
> + */
> + chan->move_state = L2CAP_MOVE_WAIT_RSP_SUCCESS;
> + break;
> + case L2CAP_MOVE_WAIT_LOGICAL_CFM:
> + if (test_bit(CONN_LOCAL_BUSY, &chan->conn_state)) {
> + chan->move_state = L2CAP_MOVE_WAIT_LOCAL_BUSY;
> + } else if (chan->move_role ==
> + L2CAP_MOVE_ROLE_INITIATOR) {
> + chan->move_state = L2CAP_MOVE_WAIT_CONFIRM_RSP;
> + l2cap_send_move_chan_cfm(chan->conn, chan,
> + chan->scid,
> + L2CAP_MR_SUCCESS);
> + __set_chan_timer(chan, L2CAP_MOVE_TIMEOUT);
> + } else if (chan->move_role ==
> + L2CAP_MOVE_ROLE_RESPONDER) {
> + chan->move_state = L2CAP_MOVE_WAIT_CONFIRM;
> + l2cap_send_move_chan_rsp(chan->conn,
> + chan->ident,
> + chan->dcid,
> + L2CAP_MR_SUCCESS);
> + }
> + break;
> + default:
> + /* Move was not in expected state, free the channel */
> + chan->hs_hchan = NULL;
> + chan->hs_hcon = NULL;
> +
> + /* Placeholder - free the logical link */
> +
> + chan->move_state = L2CAP_MOVE_STABLE;
> + }
> + }
> }
>
I find this this function still a little bit too complex. Any chance we
can split into more logical pieces. It is fine if not, but we should at
least give it another try.
Regards
Marcel
^ permalink raw reply
* Re: [PATCHv2 09/19] Bluetooth: Move channel response
From: Marcel Holtmann @ 2012-10-17 17:20 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, sunnyk
In-Reply-To: <1350430593-23565-10-git-send-email-mathewm@codeaurora.org>
Hi Mat,
> The move response command includes a result code indicationg
> "pending", "success", or "failure" status. A pending result is
> received when the remote address is still setting up a physical link,
> and will be followed by success or failure. On success, logical link
> setup will proceed. On failure, the move is stopped. The receiver of
> a move channel response must always follow up by sending a move
> channel confirm command.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> include/net/bluetooth/l2cap.h | 2 +
> net/bluetooth/l2cap_core.c | 161 ++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 158 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 6d3615e..b4c3c65 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -52,6 +52,8 @@
> #define L2CAP_ENC_TIMEOUT msecs_to_jiffies(5000)
> #define L2CAP_CONN_TIMEOUT msecs_to_jiffies(40000)
> #define L2CAP_INFO_TIMEOUT msecs_to_jiffies(4000)
> +#define L2CAP_MOVE_TIMEOUT msecs_to_jiffies(4000)
> +#define L2CAP_MOVE_ERTX_TIMEOUT msecs_to_jiffies(60000)
>
> #define L2CAP_A2MP_DEFAULT_MTU 670
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index ed2c23f..5e4796c 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -128,6 +128,20 @@ static struct l2cap_chan *__l2cap_get_chan_by_ident(struct l2cap_conn *conn,
> return NULL;
> }
>
> +static struct l2cap_chan *l2cap_get_chan_by_ident(struct l2cap_conn *conn,
> + u8 ident)
> +{
> + struct l2cap_chan *c;
> +
> + mutex_lock(&conn->chan_lock);
> + c = __l2cap_get_chan_by_ident(conn, ident);
> + if (c)
> + l2cap_chan_lock(c);
> + mutex_unlock(&conn->chan_lock);
> +
> + return c;
> +}
> +
> static struct l2cap_chan *__l2cap_global_chan_by_addr(__le16 psm, bdaddr_t *src)
> {
> struct l2cap_chan *c;
> @@ -4227,6 +4241,13 @@ static void l2cap_send_move_chan_cfm_rsp(struct l2cap_conn *conn, u8 ident,
> l2cap_send_cmd(conn, ident, L2CAP_MOVE_CHAN_CFM_RSP, sizeof(rsp), &rsp);
> }
>
> +static void l2cap_logical_cfm(struct l2cap_chan *chan, struct hci_chan *hchan,
> + u8 status)
> +{
> + /* Placeholder */
> + return;
> +}
> +
> static inline int l2cap_move_channel_req(struct l2cap_conn *conn,
> struct l2cap_cmd_hdr *cmd,
> u16 cmd_len, void *data)
> @@ -4317,9 +4338,137 @@ send_move_response:
> return 0;
> }
>
> -static inline int l2cap_move_channel_rsp(struct l2cap_conn *conn,
> - struct l2cap_cmd_hdr *cmd,
> - u16 cmd_len, void *data)
> +static void l2cap_move_continue(struct l2cap_conn *conn, u16 icid, u16 result)
> +{
> + struct l2cap_chan *chan;
> +
> + chan = l2cap_get_chan_by_scid(conn, icid);
> + if (!chan) {
> + l2cap_send_move_chan_cfm(conn, NULL, icid,
> + L2CAP_MC_UNCONFIRMED);
> + return;
> + }
> +
> + __clear_chan_timer(chan);
> + if (result == L2CAP_MR_PEND)
> + __set_chan_timer(chan, L2CAP_MOVE_ERTX_TIMEOUT);
> +
> + if (chan->move_state == L2CAP_MOVE_WAIT_LOGICAL_COMP) {
> + /* Move confirm will be sent when logical link
> + * is complete.
> + */
> + chan->move_state = L2CAP_MOVE_WAIT_LOGICAL_CFM;
> + } else if (chan->move_state == L2CAP_MOVE_WAIT_RSP_SUCCESS) {
> + if (result == L2CAP_MR_PEND) {
> + goto done;
> + } else if (test_bit(CONN_LOCAL_BUSY,
> + &chan->conn_state)) {
> + chan->move_state = L2CAP_MOVE_WAIT_LOCAL_BUSY;
> + } else {
> + /* Logical link is up or moving to BR/EDR,
> + * proceed with move */
> + chan->move_state = L2CAP_MOVE_WAIT_CONFIRM_RSP;
> + l2cap_send_move_chan_cfm(conn, chan, chan->scid,
> + L2CAP_MC_CONFIRMED);
> + __set_chan_timer(chan, L2CAP_MOVE_TIMEOUT);
> + }
> + } else if (chan->move_state == L2CAP_MOVE_WAIT_RSP) {
> + struct hci_chan *hchan = NULL;
> + /* Moving to AMP */
> + if (result == L2CAP_MR_SUCCESS) {
> + /* Remote is ready, send confirm immediately
> + * after logical link is ready
> + */
> + chan->move_state = L2CAP_MOVE_WAIT_LOGICAL_CFM;
> + } else {
> + /* Both logical link and move success
> + * are required to confirm
> + */
> + chan->move_state = L2CAP_MOVE_WAIT_LOGICAL_COMP;
> + }
> +
> + /* Placeholder - get hci_chan for logical link */
> + if (!hchan) {
> + /* Logical link not available */
> + l2cap_send_move_chan_cfm(conn, chan, chan->scid,
> + L2CAP_MC_UNCONFIRMED);
> + goto done;
> + }
> +
> + /* If the logical link is not yet connected, do not
> + * send confirmation.
> + */
> + if (hchan->state != BT_CONNECTED)
> + goto done;
> +
> + /* Logical link is already ready to go */
> +
> + chan->hs_hcon = hchan->conn;
> + chan->hs_hcon->l2cap_data = chan->conn;
> +
> + if (result == L2CAP_MR_SUCCESS) {
> + /* Can confirm now */
> + l2cap_send_move_chan_cfm(conn, chan, chan->scid,
> + L2CAP_MC_CONFIRMED);
> + } else {
> + /* Now only need move success
> + * to confirm
> + */
> + chan->move_state = L2CAP_MOVE_WAIT_RSP_SUCCESS;
> + }
> +
> + l2cap_logical_cfm(chan, hchan, L2CAP_MR_SUCCESS);
> + } else {
> + /* Any other amp move state means the move failed. */
> + chan->move_id = chan->local_amp_id;
> + chan->move_state = L2CAP_MOVE_STABLE;
> + l2cap_move_revert(chan);
> + chan->move_role = L2CAP_MOVE_ROLE_NONE;
> + l2cap_send_move_chan_cfm(conn, chan, chan->scid,
> + L2CAP_MC_UNCONFIRMED);
> + __set_chan_timer(chan, L2CAP_MOVE_TIMEOUT);
> + }
can you just use a switch statement here.
> +
> +done:
> + l2cap_chan_unlock(chan);
> +}
> +
> +static void l2cap_move_fail(struct l2cap_conn *conn, u8 ident, u16 icid,
> + u16 result)
> +{
> + struct l2cap_chan *chan;
> +
> + chan = l2cap_get_chan_by_ident(conn, ident);
> + if (!chan) {
> + /* Could not locate channel, icid is best guess */
> + l2cap_send_move_chan_cfm(conn, NULL, icid,
> + L2CAP_MC_UNCONFIRMED);
> + return;
> + }
> +
> + __clear_chan_timer(chan);
> +
> + if (chan->move_role == L2CAP_MOVE_ROLE_INITIATOR) {
> + if (result == L2CAP_MR_COLLISION) {
> + chan->move_role = L2CAP_MOVE_ROLE_RESPONDER;
> + } else {
> + /* Cleanup - cancel move */
> + chan->move_id = chan->local_amp_id;
> + chan->move_state = L2CAP_MOVE_STABLE;
> + l2cap_move_revert(chan);
> + chan->move_role = L2CAP_MOVE_ROLE_NONE;
> + }
> + }
> +
> + l2cap_send_move_chan_cfm(conn, chan, chan->scid, L2CAP_MC_UNCONFIRMED);
> + __set_chan_timer(chan, L2CAP_MOVE_TIMEOUT);
> +
> + l2cap_chan_unlock(chan);
> +}
> +
> +static int l2cap_move_channel_rsp(struct l2cap_conn *conn,
> + struct l2cap_cmd_hdr *cmd,
> + u16 cmd_len, void *data)
> {
> struct l2cap_move_chan_rsp *rsp = data;
> u16 icid, result;
> @@ -4332,8 +4481,10 @@ static inline int l2cap_move_channel_rsp(struct l2cap_conn *conn,
>
> BT_DBG("icid 0x%4.4x, result 0x%4.4x", icid, result);
>
> - /* Placeholder: Always unconfirmed */
> - l2cap_send_move_chan_cfm(conn, NULL, icid, L2CAP_MC_UNCONFIRMED);
> + if (result == L2CAP_MR_SUCCESS || result == L2CAP_MR_PEND)
> + l2cap_move_continue(conn, icid, result);
> + else
> + l2cap_move_fail(conn, cmd->ident, icid, result);
>
> return 0;
> }
Regards
Marcel
^ permalink raw reply
* Re: [PATCHv2 07/19] Bluetooth: Add move channel confirm handling
From: Marcel Holtmann @ 2012-10-17 17:16 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, sunnyk
In-Reply-To: <1350430593-23565-8-git-send-email-mathewm@codeaurora.org>
Hi Mat,
> After sending a move channel response, a move responder waits for a
> move channel confirm command. If the received command has a
> "confirmed" result the move is proceeding, and "unconfirmed" means the
> move has failed and the channel will not change controllers.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/l2cap_core.c | 70 ++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 67 insertions(+), 3 deletions(-)
>
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 9663292..ed2c23f 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -1036,6 +1036,42 @@ static void l2cap_move_setup(struct l2cap_chan *chan)
> set_bit(CONN_REMOTE_BUSY, &chan->conn_state);
> }
>
> +static void l2cap_move_success(struct l2cap_chan *chan)
> +{
> + BT_DBG("chan %p", chan);
> +
> + if (chan->mode != L2CAP_MODE_ERTM)
> + return;
> +
> + switch (chan->move_role) {
> + case L2CAP_MOVE_ROLE_INITIATOR:
> + l2cap_tx(chan, NULL, NULL, L2CAP_EV_EXPLICIT_POLL);
> + chan->rx_state = L2CAP_RX_STATE_WAIT_F;
> + break;
> + case L2CAP_MOVE_ROLE_RESPONDER:
> + chan->rx_state = L2CAP_RX_STATE_WAIT_P;
> + break;
> + }
> +}
> +
> +static void l2cap_move_revert(struct l2cap_chan *chan)
> +{
> + BT_DBG("chan %p", chan);
> +
> + if (chan->mode != L2CAP_MODE_ERTM)
> + return;
> +
> + switch (chan->move_role) {
> + case L2CAP_MOVE_ROLE_INITIATOR:
> + l2cap_tx(chan, NULL, NULL, L2CAP_EV_EXPLICIT_POLL);
> + chan->rx_state = L2CAP_RX_STATE_WAIT_F;
> + break;
> + case L2CAP_MOVE_ROLE_RESPONDER:
> + chan->rx_state = L2CAP_RX_STATE_WAIT_P;
> + break;
> + }
> +}
> +
> static void l2cap_chan_ready(struct l2cap_chan *chan)
> {
> /* This clears all conf flags, including CONF_NOT_COMPLETE */
> @@ -4302,11 +4338,12 @@ static inline int l2cap_move_channel_rsp(struct l2cap_conn *conn,
> return 0;
> }
>
> -static inline int l2cap_move_channel_confirm(struct l2cap_conn *conn,
> - struct l2cap_cmd_hdr *cmd,
> - u16 cmd_len, void *data)
> +static int l2cap_move_channel_confirm(struct l2cap_conn *conn,
> + struct l2cap_cmd_hdr *cmd,
> + u16 cmd_len, void *data)
> {
> struct l2cap_move_chan_cfm *cfm = data;
> + struct l2cap_chan *chan;
> u16 icid, result;
>
> if (cmd_len != sizeof(*cfm))
> @@ -4317,8 +4354,35 @@ static inline int l2cap_move_channel_confirm(struct l2cap_conn *conn,
>
> BT_DBG("icid 0x%4.4x, result 0x%4.4x", icid, result);
>
> + chan = l2cap_get_chan_by_dcid(conn, icid);
> + if (!chan)
> + goto send_move_confirm_response;
> +
> + if (chan->move_state == L2CAP_MOVE_WAIT_CONFIRM) {
> + chan->move_state = L2CAP_MOVE_STABLE;
> + if (result == L2CAP_MC_CONFIRMED) {
> + chan->local_amp_id = chan->move_id;
> + if (!chan->local_amp_id) {
> + /* Have moved off of AMP, free the channel */
> + chan->hs_hchan = NULL;
> + chan->hs_hcon = NULL;
> +
> + /* Placeholder - free the logical link */
> + }
> + l2cap_move_success(chan);
> + } else {
> + chan->move_id = chan->local_amp_id;
> + l2cap_move_revert(chan);
> + }
> + chan->move_role = L2CAP_MOVE_ROLE_NONE;
> + }
> +
> +send_move_confirm_response:
> l2cap_send_move_chan_cfm_rsp(conn, cmd->ident, icid);
>
> + if (chan)
> + l2cap_chan_unlock(chan);
> +
still not a big fan of the if (chan) check before the unlock. This way
of dealing with locks makes my brain hurt ;)
Regards
Marcel
^ permalink raw reply
* Re: [PATCHv2 06/19] Bluetooth: Add new ERTM receive states for channel move
From: Marcel Holtmann @ 2012-10-17 17:13 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, sunnyk
In-Reply-To: <1350430593-23565-7-git-send-email-mathewm@codeaurora.org>
Hi Mat,
> Two new states are required to implement channel moves with the ERTM
> receive state machine.
>
> The "WAIT_P" state is used by a move responder to wait for a "poll"
> flag after a move is completed (success or failure). "WAIT_F" is
> similarly used by a move initiator to wait for a "final" flag when the
> move is completing. In either state, the reqseq value in the
> poll/final frame tells the state machine exactly which frame should be
> expected next.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/l2cap_core.c | 104 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 104 insertions(+)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCHv2 04/19] Bluetooth: Lookup channel structure based on DCID
From: Marcel Holtmann @ 2012-10-17 17:12 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, sunnyk
In-Reply-To: <1350430593-23565-5-git-send-email-mathewm@codeaurora.org>
Hi Mat,
> Processing a move channel request involves getting the channel
> structure using the destination channel ID. Previous code could only
> look up using the source channel ID.
>
> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> ---
> net/bluetooth/l2cap_core.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 07/19] Bluetooth: Add move channel confirm handling
From: Marcel Holtmann @ 2012-10-17 17:10 UTC (permalink / raw)
To: Mat Martineau; +Cc: linux-bluetooth, gustavo, sunnyk
In-Reply-To: <alpine.DEB.2.02.1210161020250.13129@mathewm-linux>
Hi Mat,
> >> After sending a move channel response, a move responder waits for a
> >> move channel confirm command. If the received command has a
> >> "confirmed" result the move is proceeding, and "unconfirmed" means the
> >> move has failed and the channel will not change controllers.
> >>
> >> Signed-off-by: Mat Martineau <mathewm@codeaurora.org>
> >> ---
> >> net/bluetooth/l2cap_core.c | 70 ++++++++++++++++++++++++++++++++++++++++++++--
> >> 1 file changed, 67 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> >> index aab7f79..ef744a9 100644
> >> --- a/net/bluetooth/l2cap_core.c
> >> +++ b/net/bluetooth/l2cap_core.c
> >> @@ -1030,6 +1030,42 @@ static void l2cap_move_setup(struct l2cap_chan *chan)
> >> set_bit(CONN_REMOTE_BUSY, &chan->conn_state);
> >> }
> >>
> >> +static void l2cap_move_success(struct l2cap_chan *chan)
> >> +{
> >> + BT_DBG("chan %p", chan);
> >> +
> >> + if (chan->mode != L2CAP_MODE_ERTM)
> >> + return;
> >> +
> >> + switch (chan->move_role) {
> >> + case L2CAP_MOVE_ROLE_INITIATOR:
> >> + l2cap_tx(chan, NULL, NULL, L2CAP_EV_EXPLICIT_POLL);
> >> + chan->rx_state = L2CAP_RX_STATE_WAIT_F;
> >> + break;
> >> + case L2CAP_MOVE_ROLE_RESPONDER:
> >> + chan->rx_state = L2CAP_RX_STATE_WAIT_P;
> >> + break;
> >> + }
> >> +}
> >> +
> >> +static void l2cap_move_revert(struct l2cap_chan *chan)
> >> +{
> >> + BT_DBG("chan %p", chan);
> >> +
> >> + if (chan->mode != L2CAP_MODE_ERTM)
> >> + return;
> >> +
> >> + switch (chan->move_role) {
> >> + case L2CAP_MOVE_ROLE_INITIATOR:
> >> + l2cap_tx(chan, NULL, NULL, L2CAP_EV_EXPLICIT_POLL);
> >> + chan->rx_state = L2CAP_RX_STATE_WAIT_F;
> >> + break;
> >> + case L2CAP_MOVE_ROLE_RESPONDER:
> >> + chan->rx_state = L2CAP_RX_STATE_WAIT_P;
> >> + break;
> >> + }
> >> +}
> >> +
> >> static void l2cap_chan_ready(struct l2cap_chan *chan)
> >> {
> >> /* This clears all conf flags, including CONF_NOT_COMPLETE */
> >> @@ -4297,11 +4333,12 @@ static inline int l2cap_move_channel_rsp(struct l2cap_conn *conn,
> >> return 0;
> >> }
> >>
> >> -static inline int l2cap_move_channel_confirm(struct l2cap_conn *conn,
> >> - struct l2cap_cmd_hdr *cmd,
> >> - u16 cmd_len, void *data)
> >> +static int l2cap_move_channel_confirm(struct l2cap_conn *conn,
> >> + struct l2cap_cmd_hdr *cmd,
> >> + u16 cmd_len, void *data)
> >> {
> >> struct l2cap_move_chan_cfm *cfm = data;
> >> + struct l2cap_chan *chan;
> >> u16 icid, result;
> >>
> >> if (cmd_len != sizeof(*cfm))
> >> @@ -4312,8 +4349,35 @@ static inline int l2cap_move_channel_confirm(struct l2cap_conn *conn,
> >>
> >> BT_DBG("icid 0x%4.4x, result 0x%4.4x", icid, result);
> >>
> >> + chan = l2cap_get_chan_by_dcid(conn, icid);
> >> + if (!chan)
> >> + goto send_move_confirm_response;
> >> +
> >> + if (chan->move_state == L2CAP_MOVE_WAIT_CONFIRM) {
> >> + chan->move_state = L2CAP_MOVE_STABLE;
> >> + if (result == L2CAP_MC_CONFIRMED) {
> >> + chan->local_amp_id = chan->move_id;
> >> + if (!chan->local_amp_id) {
> >> + /* Have moved off of AMP, free the channel */
> >> + chan->hs_hchan = NULL;
> >> + chan->hs_hcon = NULL;
> >> +
> >> + /* Placeholder - free the logical link */
> >> + }
> >> + l2cap_move_success(chan);
> >> + } else {
> >> + chan->move_id = chan->local_amp_id;
> >> + l2cap_move_revert(chan);
> >> + }
> >> + chan->move_role = L2CAP_MOVE_ROLE_NONE;
> >> + }
> >> +
> >> +send_move_confirm_response:
> >> l2cap_send_move_chan_cfm_rsp(conn, cmd->ident, icid);
> >>
> >> + if (chan)
> >> + l2cap_chan_unlock(chan);
> >> +
> >> return 0;
> >> }
> >>
> >
> > the more I read into this one, can we not have a clean exit when (!chan)
> > for this one. Or am I missing something.
>
> Are you thinking this should send a COMMAND_REJ if it can't find the
> matching channel?
>
> I think sending the confirm response is the appropriate thing to do.
> The spec says "When a device receives a Move Channel Confirmation
> packet it shall send a Move Channel Confirmation response packet".
> Sending this response doesn't indicate that the move was successful.
I am fine with sending a response packet. However we might have to make
it explicit a response with success and response with failure. My
concern is that the code becomes complex and hard to follow.
Regards
Marcel
^ permalink raw reply
* Re: [RFCv0 0/8] Handling physical and logical link
From: Marcel Holtmann @ 2012-10-17 17:07 UTC (permalink / raw)
To: Andrei Emeltchenko; +Cc: linux-bluetooth
In-Reply-To: <1350399670-12418-1-git-send-email-Andrei.Emeltchenko.news@gmail.com>
Hi Andrei,
> Set of patches adding handling for physical and logical link
> complete events.
>
> Andrei Emeltchenko (8):
> Bluetooth: AMP: Process Physical Link Complete evt
> Bluetooth: AMP: Process Logical Link complete evt
> Bluetooth: Add logical link create function
> Bluetooth: AMP: Process Disc Logical Link
> Bluetooth: AMP: Process Disc Physical Link complete evt
> Bluetooth: AMP: Remove hci_conn receiving error command status
> Bluetooth: Disconnect logical link when deleteing chan
> Bluetooth: Add put(hcon) when deleting hchan
I looked over the patches and the look fine to me.
Acked-by: Marcel Holtmann <marcel@holtmann.org>
Regards
Marcel
^ permalink raw reply
* Re: [PATCH BlueZ 2/2] core: Update gdbus function calls
From: Anderson Lizardo @ 2012-10-17 16:25 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: linux-bluetooth
In-Reply-To: <CAJdJm_OoM=Ku7pESgqvXEOJO3jB21bdGHfA368uiTnfoxb=bJw@mail.gmail.com>
Hi again,
On Wed, Oct 17, 2012 at 1:20 PM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> Hi Lucas,
>
> On Wed, Oct 17, 2012 at 12:14 PM, Lucas De Marchi
> <lucas.demarchi@profusion.mobi> wrote:
>> @@ -827,24 +824,21 @@ static void set_name(struct btd_adapter *adapter, const char *name,
>> int ret;
>>
>> if (adapter->allow_name_changes == FALSE)
>> - return g_dbus_pending_property_error(btd_get_dbus_connection(),
>> - id, ERROR_INTERFACE ".Failed",
>> + return g_dbus_pending_property_error(id,
>> + ERROR_INTERFACE ".Failed",
>> strerror(EPERM));
>>
>> ret = adapter_set_name(adapter, name);
>> - if (ret >= 0) {
>> - g_dbus_pending_property_success(btd_get_dbus_connection(), id);
>> - return;
>> - }
>> + if (ret >= 0)
>> + return g_dbus_pending_property_success(id);
>
> Out of curiosity, is this style of returning "void" (instead of just
> "return;") common in gdbus?
Just realized that this is BlueZ specific code. Anyway, just found it
odd (but answering my question, seems to be common).
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH BlueZ 2/2] core: Update gdbus function calls
From: Anderson Lizardo @ 2012-10-17 16:20 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: linux-bluetooth
In-Reply-To: <1350486869-5020-2-git-send-email-lucas.demarchi@profusion.mobi>
Hi Lucas,
On Wed, Oct 17, 2012 at 12:14 PM, Lucas De Marchi
<lucas.demarchi@profusion.mobi> wrote:
> @@ -827,24 +824,21 @@ static void set_name(struct btd_adapter *adapter, const char *name,
> int ret;
>
> if (adapter->allow_name_changes == FALSE)
> - return g_dbus_pending_property_error(btd_get_dbus_connection(),
> - id, ERROR_INTERFACE ".Failed",
> + return g_dbus_pending_property_error(id,
> + ERROR_INTERFACE ".Failed",
> strerror(EPERM));
>
> ret = adapter_set_name(adapter, name);
> - if (ret >= 0) {
> - g_dbus_pending_property_success(btd_get_dbus_connection(), id);
> - return;
> - }
> + if (ret >= 0)
> + return g_dbus_pending_property_success(id);
Out of curiosity, is this style of returning "void" (instead of just
"return;") common in gdbus?
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* [PATCH BlueZ 2/2] core: Update gdbus function calls
From: Lucas De Marchi @ 2012-10-17 15:14 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Lucas De Marchi
In-Reply-To: <1350486869-5020-1-git-send-email-lucas.demarchi@profusion.mobi>
Done by the following semantic patch, with manual tweaks afterwards due
to changes in 80-chars line breaks:
// <smpl>
@r1 @
expression E1;
@@
g_dbus_pending_property_success(
- E1,
...)
@r2 @
expression E1;
@@
g_dbus_pending_property_error(
- E1,
...)
@r3 @
expression E1;
@@
g_dbus_pending_property_error_valist(
- E1,
...)
// </smpl>
---
src/adapter.c | 91 +++++++++++++++++++++++++++--------------------------------
src/device.c | 72 ++++++++++++++++++----------------------------
2 files changed, 69 insertions(+), 94 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 7c2aec0..e5d4cf7 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -346,7 +346,6 @@ static void set_session_pending_mode(struct btd_adapter *adapter,
static void set_discoverable(struct btd_adapter *adapter,
gboolean discoverable, GDBusPendingPropertySet id)
{
- DBusConnection *conn = btd_get_dbus_connection();
uint8_t mode;
int err;
@@ -354,12 +353,12 @@ static void set_discoverable(struct btd_adapter *adapter,
if (mode == adapter->mode) {
adapter->global_mode = mode;
- return g_dbus_pending_property_success(conn, id);
+ return g_dbus_pending_property_success(id);
}
err = set_mode(adapter, mode);
if (err < 0)
- return g_dbus_pending_property_error(conn, id,
+ return g_dbus_pending_property_error(id,
ERROR_INTERFACE ".Failed",
strerror(-err));
@@ -371,7 +370,6 @@ static void set_discoverable(struct btd_adapter *adapter,
static void set_powered(struct btd_adapter *adapter, gboolean powered,
GDBusPendingPropertySet id)
{
- DBusConnection *conn = btd_get_dbus_connection();
uint8_t mode;
int err;
@@ -385,12 +383,12 @@ static void set_powered(struct btd_adapter *adapter, gboolean powered,
if (mode == adapter->mode) {
adapter->global_mode = mode;
- return g_dbus_pending_property_success(conn, id);
+ return g_dbus_pending_property_success(id);
}
err = set_mode(adapter, mode);
if (err < 0)
- return g_dbus_pending_property_error(conn, id,
+ return g_dbus_pending_property_error(id,
ERROR_INTERFACE ".Failed",
strerror(-err));
@@ -402,11 +400,10 @@ static void set_powered(struct btd_adapter *adapter, gboolean powered,
static void set_pairable(struct btd_adapter *adapter, gboolean pairable,
bool reply, GDBusPendingPropertySet id)
{
- DBusConnection *conn = btd_get_dbus_connection();
int err;
if (adapter->scan_mode == SCAN_DISABLED)
- return g_dbus_pending_property_error(conn, id,
+ return g_dbus_pending_property_error(id,
ERROR_INTERFACE ".NotReady",
"Resource Not Ready");
@@ -419,7 +416,7 @@ static void set_pairable(struct btd_adapter *adapter, gboolean pairable,
err = set_mode(adapter, MODE_DISCOVERABLE);
if (err < 0) {
if (reply)
- g_dbus_pending_property_error(conn, id,
+ g_dbus_pending_property_error(id,
ERROR_INTERFACE ".Failed",
strerror(-err));
return;
@@ -430,7 +427,7 @@ store:
done:
if (reply)
- g_dbus_pending_property_success(conn, id);
+ g_dbus_pending_property_success(id);
}
static gboolean pairable_timeout_handler(void *data)
@@ -716,7 +713,7 @@ static void set_discoverable_timeout(struct btd_adapter *adapter,
DBusConnection *conn = btd_get_dbus_connection();
if (adapter->discov_timeout == timeout && timeout == 0)
- return g_dbus_pending_property_success(conn, id);
+ return g_dbus_pending_property_success(id);
if (adapter->scan_mode & SCAN_INQUIRY)
mgmt_set_discoverable(adapter->dev_id, TRUE, timeout);
@@ -727,7 +724,7 @@ static void set_discoverable_timeout(struct btd_adapter *adapter,
g_dbus_emit_property_changed(conn, adapter->path, ADAPTER_INTERFACE,
"DiscoverableTimeout");
- g_dbus_pending_property_success(conn, id);
+ g_dbus_pending_property_success(id);
}
static void set_pairable_timeout(struct btd_adapter *adapter,
@@ -736,7 +733,7 @@ static void set_pairable_timeout(struct btd_adapter *adapter,
DBusConnection *conn = btd_get_dbus_connection();
if (adapter->pairable_timeout == timeout && timeout == 0)
- return g_dbus_pending_property_success(conn, id);
+ return g_dbus_pending_property_success(id);
if (adapter->pairable)
adapter_set_pairable_timeout(adapter, timeout);
@@ -747,7 +744,7 @@ static void set_pairable_timeout(struct btd_adapter *adapter,
g_dbus_emit_property_changed(conn, adapter->path, ADAPTER_INTERFACE,
"PairableTimeout");
- g_dbus_pending_property_success(conn, id);
+ g_dbus_pending_property_success(id);
}
void btd_adapter_class_changed(struct btd_adapter *adapter, uint8_t *new_class)
@@ -827,24 +824,21 @@ static void set_name(struct btd_adapter *adapter, const char *name,
int ret;
if (adapter->allow_name_changes == FALSE)
- return g_dbus_pending_property_error(btd_get_dbus_connection(),
- id, ERROR_INTERFACE ".Failed",
+ return g_dbus_pending_property_error(id,
+ ERROR_INTERFACE ".Failed",
strerror(EPERM));
ret = adapter_set_name(adapter, name);
- if (ret >= 0) {
- g_dbus_pending_property_success(btd_get_dbus_connection(), id);
- return;
- }
+ if (ret >= 0)
+ return g_dbus_pending_property_success(id);
if (ret == -EINVAL)
- g_dbus_pending_property_error(btd_get_dbus_connection(),
- id, ERROR_INTERFACE ".InvalidArguments",
- "Invalid arguments in method call");
+ g_dbus_pending_property_error(id,
+ ERROR_INTERFACE ".InvalidArguments",
+ "Invalid arguments in method call");
else
- g_dbus_pending_property_error(btd_get_dbus_connection(),
- id, ERROR_INTERFACE ".Failed",
- strerror(-ret));
+ g_dbus_pending_property_error(id, ERROR_INTERFACE ".Failed",
+ strerror(-ret));
}
struct btd_device *adapter_find_device(struct btd_adapter *adapter,
@@ -1173,12 +1167,10 @@ static void adapter_property_set_name(const GDBusPropertyTable *property,
{
const char *name;
- if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_STRING) {
- g_dbus_pending_property_error(btd_get_dbus_connection(),
- id, ERROR_INTERFACE ".InvalidArguments",
- "Invalid arguments in method call");
- return;
- }
+ if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_STRING)
+ return g_dbus_pending_property_error(id,
+ ERROR_INTERFACE ".InvalidArguments",
+ "Invalid arguments in method call");
dbus_message_iter_get_basic(value, &name);
@@ -1217,9 +1209,9 @@ static void adapter_property_set_powered(
dbus_bool_t powered;
if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_BOOLEAN)
- return g_dbus_pending_property_error(btd_get_dbus_connection(),
- id, ERROR_INTERFACE ".InvalidArguments",
- "Invalid arguments in method call");
+ return g_dbus_pending_property_error(id,
+ ERROR_INTERFACE ".InvalidArguments",
+ "Invalid arguments in method call");
dbus_message_iter_get_basic(value, &powered);
@@ -1247,9 +1239,9 @@ static void adapter_property_set_discoverable(
dbus_bool_t discoverable;
if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_BOOLEAN)
- return g_dbus_pending_property_error(btd_get_dbus_connection(),
- id, ERROR_INTERFACE ".InvalidArguments",
- "Invalid arguments in method call");
+ return g_dbus_pending_property_error(id,
+ ERROR_INTERFACE ".InvalidArguments",
+ "Invalid arguments in method call");
dbus_message_iter_get_basic(value, &discoverable);
@@ -1274,9 +1266,9 @@ static void adapter_property_set_pairable(const GDBusPropertyTable *property,
dbus_bool_t pairable;
if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_BOOLEAN)
- return g_dbus_pending_property_error(btd_get_dbus_connection(),
- id, ERROR_INTERFACE ".InvalidArguments",
- "Invalid arguments in method call");
+ return g_dbus_pending_property_error(id,
+ ERROR_INTERFACE ".InvalidArguments",
+ "Invalid arguments in method call");
dbus_message_iter_get_basic(value, &pairable);
@@ -1303,9 +1295,9 @@ static void adapter_property_set_discoverable_timeout(
uint32_t timeout;
if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_UINT32)
- return g_dbus_pending_property_error(btd_get_dbus_connection(),
- id, ERROR_INTERFACE ".InvalidArguments",
- "Invalid arguments in method call");
+ return g_dbus_pending_property_error(id,
+ ERROR_INTERFACE ".InvalidArguments",
+ "Invalid arguments in method call");
dbus_message_iter_get_basic(value, &timeout);
set_discoverable_timeout(data, timeout, id);
@@ -1330,9 +1322,9 @@ static void adapter_property_set_pairable_timeout(
uint32_t timeout;
if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_UINT32)
- return g_dbus_pending_property_error(btd_get_dbus_connection(),
- id, ERROR_INTERFACE ".InvalidArguments",
- "Invalid arguments in method call");
+ return g_dbus_pending_property_error(id,
+ ERROR_INTERFACE ".InvalidArguments",
+ "Invalid arguments in method call");
dbus_message_iter_get_basic(value, &timeout);
set_pairable_timeout(data, timeout, id);
@@ -2361,13 +2353,12 @@ static void set_mode_complete(struct btd_adapter *adapter)
if (pending->type == SESSION_TYPE_MODE_GLOBAL) {
if (err < 0)
- g_dbus_pending_property_error(conn, pending->prop_id,
+ g_dbus_pending_property_error(pending->prop_id,
ERROR_INTERFACE ".Failed",
strerror(-err));
else {
adapter->global_mode = adapter->mode;
- g_dbus_pending_property_success(conn,
- pending->prop_id);
+ g_dbus_pending_property_success(pending->prop_id);
}
} else if (pending->msg != NULL) {
DBusMessage *msg = pending->msg;
diff --git a/src/device.c b/src/device.c
index bb0f890..bc7f8dd 100644
--- a/src/device.c
+++ b/src/device.c
@@ -394,10 +394,8 @@ static void set_alias(GDBusPendingPropertySet id, const char *alias,
/* No change */
if ((device->alias == NULL && g_str_equal(alias, "")) ||
- g_strcmp0(device->alias, alias) == 0) {
- g_dbus_pending_property_success(btd_get_dbus_connection(), id);
- return;
- }
+ g_strcmp0(device->alias, alias) == 0)
+ return g_dbus_pending_property_success(id);
ba2str(adapter_get_address(adapter), srcaddr);
ba2str(&device->bdaddr, dstaddr);
@@ -405,11 +403,9 @@ static void set_alias(GDBusPendingPropertySet id, const char *alias,
/* Remove alias if empty string */
err = write_device_alias(srcaddr, dstaddr, device->bdaddr_type,
g_str_equal(alias, "") ? NULL : alias);
- if (err < 0) {
- g_dbus_pending_property_error(btd_get_dbus_connection(),
- id, ERROR_INTERFACE ".Failed", strerror(-err));
- return;
- }
+ if (err < 0)
+ return g_dbus_pending_property_error(id,
+ ERROR_INTERFACE ".Failed", strerror(-err));
g_free(device->alias);
device->alias = g_str_equal(alias, "") ? NULL : g_strdup(alias);
@@ -417,7 +413,7 @@ static void set_alias(GDBusPendingPropertySet id, const char *alias,
g_dbus_emit_property_changed(btd_get_dbus_connection(),
device->path, DEVICE_INTERFACE, "Alias");
- g_dbus_pending_property_success(btd_get_dbus_connection(), id);
+ g_dbus_pending_property_success(id);
}
static void dev_property_set_alias(const GDBusPropertyTable *property,
@@ -426,12 +422,10 @@ static void dev_property_set_alias(const GDBusPropertyTable *property,
{
const char *alias;
- if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_STRING) {
- g_dbus_pending_property_error(btd_get_dbus_connection(),
- id, ERROR_INTERFACE ".InvalidArguments",
- "Invalid arguments in method call");
- return;
- }
+ if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_STRING)
+ return g_dbus_pending_property_error(id,
+ ERROR_INTERFACE ".InvalidArguments",
+ "Invalid arguments in method call");
dbus_message_iter_get_basic(value, &alias);
@@ -686,27 +680,23 @@ static void set_trust(GDBusPendingPropertySet id, gboolean value, void *data)
char srcaddr[18], dstaddr[18];
int err;
- if (device->trusted == value) {
- g_dbus_pending_property_success(btd_get_dbus_connection(), id);
- return;
- }
+ if (device->trusted == value)
+ return g_dbus_pending_property_success(id);
ba2str(adapter_get_address(adapter), srcaddr);
ba2str(&device->bdaddr, dstaddr);
err = write_trust(srcaddr, dstaddr, device->bdaddr_type, value);
- if (err < 0) {
- g_dbus_pending_property_error(btd_get_dbus_connection(),
- id, ERROR_INTERFACE ".Failed", strerror(-err));
- return;
- }
+ if (err < 0)
+ return g_dbus_pending_property_error(id,
+ ERROR_INTERFACE ".Failed", strerror(-err));
device->trusted = value;
g_dbus_emit_property_changed(btd_get_dbus_connection(),
device->path, DEVICE_INTERFACE, "Trusted");
- g_dbus_pending_property_success(btd_get_dbus_connection(), id);
+ g_dbus_pending_property_success(id);
}
static void dev_property_set_trusted(const GDBusPropertyTable *property,
@@ -715,12 +705,10 @@ static void dev_property_set_trusted(const GDBusPropertyTable *property,
{
dbus_bool_t b;
- if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_BOOLEAN) {
- g_dbus_pending_property_error(btd_get_dbus_connection(),
- id, ERROR_INTERFACE ".InvalidArguments",
- "Invalid arguments in method call");
- return;
- }
+ if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_BOOLEAN)
+ return g_dbus_pending_property_error(id,
+ ERROR_INTERFACE ".InvalidArguments",
+ "Invalid arguments in method call");
dbus_message_iter_get_basic(value, &b);
@@ -750,17 +738,15 @@ static void set_blocked(GDBusPendingPropertySet id, gboolean value, void *data)
switch (-err) {
case 0:
- g_dbus_pending_property_success(btd_get_dbus_connection(), id);
+ g_dbus_pending_property_success(id);
break;
case EINVAL:
- g_dbus_pending_property_error(btd_get_dbus_connection(), id,
- ERROR_INTERFACE ".Failed",
+ g_dbus_pending_property_error(id, ERROR_INTERFACE ".Failed",
"Kernel lacks blacklist support");
break;
default:
- g_dbus_pending_property_error(btd_get_dbus_connection(), id,
- ERROR_INTERFACE ".Failed",
- strerror(-err));
+ g_dbus_pending_property_error(id, ERROR_INTERFACE ".Failed",
+ strerror(-err));
break;
}
}
@@ -772,12 +758,10 @@ static void dev_property_set_blocked(const GDBusPropertyTable *property,
{
dbus_bool_t b;
- if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_BOOLEAN) {
- g_dbus_pending_property_error(btd_get_dbus_connection(),
- id, ERROR_INTERFACE ".InvalidArguments",
- "Invalid arguments in method call");
- return;
- }
+ if (dbus_message_iter_get_arg_type(value) != DBUS_TYPE_BOOLEAN)
+ return g_dbus_pending_property_error(id,
+ ERROR_INTERFACE ".InvalidArguments",
+ "Invalid arguments in method call");
dbus_message_iter_get_basic(value, &b);
--
1.7.12.3
^ permalink raw reply related
* [PATCH BlueZ 1/2] gdbus: Remove connection from pending_property functions
From: Lucas De Marchi @ 2012-10-17 15:14 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Lucas De Marchi
The reply to a DBus.Properties.Set() method call should go through the
same D-Bus connection. Thus remove the DBusConnection parameter from the
following functions:
- g_dbus_pending_property_success()
- g_dbus_pending_property_error_valist()
- g_dbus_pending_property_error()
---
gdbus/gdbus.h | 13 +++++--------
gdbus/object.c | 24 ++++++++++++------------
2 files changed, 17 insertions(+), 20 deletions(-)
diff --git a/gdbus/gdbus.h b/gdbus/gdbus.h
index 8251947..ba49621 100644
--- a/gdbus/gdbus.h
+++ b/gdbus/gdbus.h
@@ -246,14 +246,11 @@ guint g_dbus_add_signal_watch(DBusConnection *connection,
gboolean g_dbus_remove_watch(DBusConnection *connection, guint tag);
void g_dbus_remove_all_watches(DBusConnection *connection);
-void g_dbus_pending_property_success(DBusConnection *connection,
- GDBusPendingPropertySet id);
-void g_dbus_pending_property_error_valist(DBusConnection *connection,
- GDBusPendingReply id, const char *name,
- const char *format, va_list args);
-void g_dbus_pending_property_error(DBusConnection *connection,
- GDBusPendingReply id, const char *name,
- const char *format, ...);
+void g_dbus_pending_property_success(GDBusPendingPropertySet id);
+void g_dbus_pending_property_error_valist(GDBusPendingReply id,
+ const char *name, const char *format, va_list args);
+void g_dbus_pending_property_error(GDBusPendingReply id, const char *name,
+ const char *format, ...);
void g_dbus_emit_property_changed(DBusConnection *connection,
const char *path, const char *interface,
const char *name);
diff --git a/gdbus/object.c b/gdbus/object.c
index 4147361..ec98c2c 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -71,6 +71,7 @@ struct security_data {
};
struct property_data {
+ DBusConnection *conn;
GDBusPendingPropertySet id;
DBusMessage *message;
};
@@ -445,8 +446,7 @@ static struct property_data *remove_pending_property_data(
return propdata;
}
-void g_dbus_pending_property_success(DBusConnection *connection,
- GDBusPendingPropertySet id)
+void g_dbus_pending_property_success(GDBusPendingPropertySet id)
{
struct property_data *propdata;
@@ -454,14 +454,15 @@ void g_dbus_pending_property_success(DBusConnection *connection,
if (propdata == NULL)
return;
- g_dbus_send_reply(connection, propdata->message, DBUS_TYPE_INVALID);
+ g_dbus_send_reply(propdata->conn, propdata->message,
+ DBUS_TYPE_INVALID);
dbus_message_unref(propdata->message);
g_free(propdata);
}
-void g_dbus_pending_property_error_valist(DBusConnection *connection,
- GDBusPendingReply id, const char *name,
- const char *format, va_list args)
+void g_dbus_pending_property_error_valist(GDBusPendingReply id,
+ const char *name, const char *format,
+ va_list args)
{
struct property_data *propdata;
DBusMessage *reply;
@@ -473,7 +474,7 @@ void g_dbus_pending_property_error_valist(DBusConnection *connection,
reply = g_dbus_create_error_valist(propdata->message, name, format,
args);
if (reply != NULL) {
- dbus_connection_send(connection, reply, NULL);
+ dbus_connection_send(propdata->conn, reply, NULL);
dbus_message_unref(reply);
}
@@ -481,16 +482,14 @@ void g_dbus_pending_property_error_valist(DBusConnection *connection,
g_free(propdata);
}
-void g_dbus_pending_property_error(DBusConnection *connection,
- GDBusPendingReply id, const char *name,
- const char *format, ...)
+void g_dbus_pending_property_error(GDBusPendingReply id, const char *name,
+ const char *format, ...)
{
va_list args;
va_start(args, format);
- g_dbus_pending_property_error_valist(connection, id, name, format,
- args);
+ g_dbus_pending_property_error_valist(id, name, format, args);
va_end(args);
}
@@ -891,6 +890,7 @@ static DBusMessage *properties_set(DBusConnection *connection,
propdata = g_new(struct property_data, 1);
propdata->id = next_pending_property++;
propdata->message = dbus_message_ref(message);
+ propdata->conn = connection;
pending_property_set = g_slist_prepend(pending_property_set, propdata);
property->set(property, &sub, propdata->id, iface->user_data);
--
1.7.12.3
^ permalink raw reply related
* Re: REMINDER: Linux wireless networking summit, Barcelona 8-9 November
From: John W. Linville @ 2012-10-17 14:46 UTC (permalink / raw)
To: Daniel Wagner; +Cc: linux-wireless, linux-bluetooth
In-Reply-To: <507EB186.40506@monom.org>
On Wed, Oct 17, 2012 at 03:24:22PM +0200, Daniel Wagner wrote:
> Hi John,
>
> On 10/15/2012 09:36 PM, John W. Linville wrote:
> >Just a reminder...there will be a Linux wireless networking mini-summit
> >event coming soon. It will be on 8-9 November in Barcelona, just
> >after LinuxCon Europe. If you haven't already made plans, you had
> >better get moving!
> >
> >Attendees of the wireless mini-summit must also register for LinuxCon
> >Europe. More information is available here:
> >
> > http://wireless.kernel.org/en/developers/Summits/Barcelona-2012
> >
> >Please notice that there is a wiki there with schedules for discussion
> >topics. If you have suggestions or want other changes, please make
> >them and/or let us know here.
>
> I changed the "a short overview what we do in ConnMan" to "ConnMan
> and wpa_supplicant" which is probably a more attractive title.
> Unfortunately, Patrik Flykt wont be there.
Awesome, thanks!
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply
* Re: Memory leak introduced in commit f8619bef3406a2134082dc41c208105fe028c09f
From: Anderson Lizardo @ 2012-10-17 14:08 UTC (permalink / raw)
To: Ludek Finstrle; +Cc: linux-bluetooth
In-Reply-To: <20121017124931.GA32194@pzkagis.cz>
Hi Ludek,
On Wed, Oct 17, 2012 at 9:49 AM, Ludek Finstrle <luf@pzkagis.cz> wrote:
> Hello,
>
> I see memory leak which was introduced in commit f8619bef3406a2134082dc41c208105fe028c09f:
> attrib: Fix not checking if att_data_list_alloc fails
>
> It returns (in src/attrib-server.c) from functions when adl local variable is NULL but
> it doesn't free local variables (read_by_group: group; read_by_type: type; find_info: info).
>
> The patch is very easy but I can't test it with head so I don't want to send possible
> crapy patch.
Please feel free to send it marked as RFC on the subject (e.g. "[RFC]
bla bla") and rebased against master, so we can review the fix.
Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* Re: Memory leak introduced in commit f8619bef3406a2134082dc41c208105fe028c09f
From: Vinicius Costa Gomes @ 2012-10-17 14:03 UTC (permalink / raw)
To: Ludek Finstrle; +Cc: linux-bluetooth
In-Reply-To: <20121017124931.GA32194@pzkagis.cz>
Hi Luf,
On 14:49 Wed 17 Oct, Ludek Finstrle wrote:
> Hello,
>
> I see memory leak which was introduced in commit f8619bef3406a2134082dc41c208105fe028c09f:
> attrib: Fix not checking if att_data_list_alloc fails
>
> It returns (in src/attrib-server.c) from functions when adl local variable is NULL but
> it doesn't free local variables (read_by_group: group; read_by_type: type; find_info: info).
Thanks a lot for the report.
Ugh, I will fix it.
>
> The patch is very easy but I can't test it with head so I don't want to send possible
> crapy patch.
>
> Luf
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Cheers,
--
Vinicius
^ permalink raw reply
* Re: REMINDER: Linux wireless networking summit, Barcelona 8-9 November
From: Daniel Wagner @ 2012-10-17 13:24 UTC (permalink / raw)
To: John W. Linville; +Cc: linux-wireless, linux-bluetooth
In-Reply-To: <20121015193648.GF16817@tuxdriver.com>
Hi John,
On 10/15/2012 09:36 PM, John W. Linville wrote:
> Just a reminder...there will be a Linux wireless networking mini-summit
> event coming soon. It will be on 8-9 November in Barcelona, just
> after LinuxCon Europe. If you haven't already made plans, you had
> better get moving!
>
> Attendees of the wireless mini-summit must also register for LinuxCon
> Europe. More information is available here:
>
> http://wireless.kernel.org/en/developers/Summits/Barcelona-2012
>
> Please notice that there is a wiki there with schedules for discussion
> topics. If you have suggestions or want other changes, please make
> them and/or let us know here.
I changed the "a short overview what we do in ConnMan" to "ConnMan and
wpa_supplicant" which is probably a more attractive title.
Unfortunately, Patrik Flykt wont be there.
cheers,
daniel
^ permalink raw reply
* Re: gatttool and multiple devices connected
From: Anderson Lizardo @ 2012-10-17 13:02 UTC (permalink / raw)
To: Anderson Lizardo, Kim Schulz, linux-bluetooth
In-Reply-To: <20121017113515.GA16879@x220.ger.corp.intel.com>
Hi Johan,
On Wed, Oct 17, 2012 at 8:35 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> This kernel is actually not that old (as long as it is based on the
>> official 3.4.0 one). Did you have hcidump logs for the attempt to
>> discover primary services on device B while connected to device A?
>
> Actually I'd say that it is too old for LE usage. E.g. the address type
> field to the L2CAP socket address was introduced in 3.5 which is quite
> important from an LE standpoint.
Ok, my mistake. I thought it was introduced still on 3.4.0. In this
case, Better use the latest mainline (3.6.*).
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* Memory leak introduced in commit f8619bef3406a2134082dc41c208105fe028c09f
From: Ludek Finstrle @ 2012-10-17 12:49 UTC (permalink / raw)
To: linux-bluetooth
Hello,
I see memory leak which was introduced in commit f8619bef3406a2134082dc41c208105fe028c09f:
attrib: Fix not checking if att_data_list_alloc fails
It returns (in src/attrib-server.c) from functions when adl local variable is NULL but
it doesn't free local variables (read_by_group: group; read_by_type: type; find_info: info).
The patch is very easy but I can't test it with head so I don't want to send possible
crapy patch.
Luf
^ permalink raw reply
* [PATCH BlueZ] AVDTP: Do not keep a internal reference
From: Luiz Augusto von Dentz @ 2012-10-17 11:49 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Don't initialize reference with 1, instead always start disconnect timer
when reference drops to 0, so in case nobody reclaims the session it
automatically disconnect after 1 second and frees the memory.
---
audio/avdtp.c | 220 ++++++++++++++++++++++++++--------------------------------
1 file changed, 97 insertions(+), 123 deletions(-)
diff --git a/audio/avdtp.c b/audio/avdtp.c
index bd91cb6..6bfeb9b 100644
--- a/audio/avdtp.c
+++ b/audio/avdtp.c
@@ -387,8 +387,7 @@ struct avdtp_stream {
/* Structure describing an AVDTP connection between two devices */
struct avdtp {
- int ref;
- int free_lock;
+ unsigned int ref;
uint16_t version;
@@ -657,50 +656,6 @@ static gboolean stream_open_timeout(gpointer user_data)
return FALSE;
}
-static gboolean disconnect_timeout(gpointer user_data)
-{
- struct avdtp *session = user_data;
- struct audio_device *dev;
- gboolean stream_setup;
-
- session->dc_timer = 0;
- stream_setup = session->stream_setup;
- session->stream_setup = FALSE;
-
- dev = manager_get_device(&session->server->src, &session->dst, FALSE);
-
- if (dev && dev->sink && stream_setup)
- sink_setup_stream(dev->sink, session);
- else if (dev && dev->source && stream_setup)
- source_setup_stream(dev->source, session);
- else
- connection_lost(session, ETIMEDOUT);
-
- return FALSE;
-}
-
-static void remove_disconnect_timer(struct avdtp *session)
-{
- g_source_remove(session->dc_timer);
- session->dc_timer = 0;
- session->stream_setup = FALSE;
-}
-
-static void set_disconnect_timer(struct avdtp *session)
-{
- if (session->dc_timer)
- remove_disconnect_timer(session);
-
- if (session->device_disconnect) {
- session->dc_timer = g_idle_add(disconnect_timeout, session);
- return;
- }
-
- session->dc_timer = g_timeout_add_seconds(DISCONNECT_TIMEOUT,
- disconnect_timeout,
- session);
-}
-
void avdtp_error_init(struct avdtp_error *err, uint8_t category, int id)
{
err->category = category;
@@ -780,8 +735,9 @@ static void avdtp_set_state(struct avdtp *session,
}
}
-static void stream_free(struct avdtp_stream *stream)
+static void stream_free(void *data)
{
+ struct avdtp_stream *stream = data;
struct avdtp_remote_sep *rsep;
stream->lsep->info.inuse = 0;
@@ -1144,37 +1100,42 @@ static int avdtp_cancel_authorization(struct avdtp *session)
return err;
session->auth_id = 0;
+ avdtp_unref(session);
return 0;
}
-static void connection_lost(struct avdtp *session, int err)
+static void sep_free(gpointer data)
{
- char address[18];
+ struct avdtp_remote_sep *sep = data;
- ba2str(&session->dst, address);
- DBG("Disconnected from %s", address);
+ g_slist_free_full(sep->caps, g_free);
+ g_free(sep);
+}
- if (err != EACCES)
- avdtp_cancel_authorization(session);
+static void remove_disconnect_timer(struct avdtp *session)
+{
+ g_source_remove(session->dc_timer);
+ session->dc_timer = 0;
+ session->stream_setup = FALSE;
+}
- session->free_lock = 1;
+static void avdtp_free(void *data)
+{
+ struct avdtp *session = data;
- finalize_discovery(session, err);
+ DBG("%p", session);
- g_slist_foreach(session->streams, (GFunc) release_stream, session);
- session->streams = NULL;
+ g_slist_free_full(session->streams, stream_free);
- session->free_lock = 0;
+ if (session->state != AVDTP_SESSION_STATE_DISCONNECTED)
+ avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);
if (session->io) {
g_io_channel_shutdown(session->io, FALSE, NULL);
g_io_channel_unref(session->io);
- session->io = NULL;
}
- avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);
-
if (session->io_id) {
g_source_remove(session->io_id);
session->io_id = 0;
@@ -1183,69 +1144,95 @@ static void connection_lost(struct avdtp *session, int err)
if (session->dc_timer)
remove_disconnect_timer(session);
- if (session->ref != 1)
- error("connection_lost: ref count not 1 after all callbacks");
- else
- avdtp_unref(session);
+ avdtp_cancel_authorization(session);
+
+ if (session->req)
+ pending_req_free(session->req);
+
+ g_slist_free_full(session->seps, sep_free);
+
+ g_free(session->buf);
+
+ g_free(session);
}
-static void sep_free(gpointer data)
+static gboolean disconnect_timeout(gpointer user_data)
{
- struct avdtp_remote_sep *sep = data;
+ struct avdtp *session = user_data;
+ struct audio_device *dev;
+ gboolean stream_setup;
- g_slist_free_full(sep->caps, g_free);
- g_free(sep);
+ session->dc_timer = 0;
+
+ if (session->ref == 0) {
+ struct avdtp_server *server = session->server;
+
+ server->sessions = g_slist_remove(server->sessions, session);
+ avdtp_free(session);
+ return FALSE;
+ }
+
+ stream_setup = session->stream_setup;
+ session->stream_setup = FALSE;
+
+ dev = manager_get_device(&session->server->src, &session->dst, FALSE);
+
+ if (dev && dev->sink && stream_setup)
+ sink_setup_stream(dev->sink, session);
+ else if (dev && dev->source && stream_setup)
+ source_setup_stream(dev->source, session);
+ else
+ connection_lost(session, ETIMEDOUT);
+
+ return FALSE;
}
-void avdtp_unref(struct avdtp *session)
+static void set_disconnect_timer(struct avdtp *session)
{
- struct avdtp_server *server;
+ if (session->dc_timer)
+ remove_disconnect_timer(session);
- if (!session)
+ if (session->device_disconnect) {
+ session->dc_timer = g_idle_add(disconnect_timeout, session);
return;
+ }
- session->ref--;
-
- DBG("%p: ref=%d", session, session->ref);
+ session->dc_timer = g_timeout_add_seconds(DISCONNECT_TIMEOUT,
+ disconnect_timeout,
+ session);
+}
- if (session->ref == 1) {
- if (session->state == AVDTP_SESSION_STATE_CONNECTING &&
- session->io) {
- avdtp_cancel_authorization(session);
- g_io_channel_shutdown(session->io, TRUE, NULL);
- g_io_channel_unref(session->io);
- session->io = NULL;
- avdtp_set_state(session,
- AVDTP_SESSION_STATE_DISCONNECTED);
- }
+static void connection_lost(struct avdtp *session, int err)
+{
+ char address[18];
- if (session->io)
- set_disconnect_timer(session);
- else if (!session->free_lock) /* Drop the local ref if we
- aren't connected */
- session->ref--;
- }
+ ba2str(&session->dst, address);
+ DBG("Disconnected from %s", address);
- if (session->ref > 0)
- return;
+ if (err != EACCES)
+ avdtp_cancel_authorization(session);
- server = session->server;
+ g_slist_foreach(session->streams, (GFunc) release_stream, session);
+ session->streams = NULL;
- DBG("%p: freeing session and removing from list", session);
+ finalize_discovery(session, err);
- if (session->dc_timer)
- remove_disconnect_timer(session);
+ avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);
+}
- server->sessions = g_slist_remove(server->sessions, session);
+void avdtp_unref(struct avdtp *session)
+{
+ if (!session)
+ return;
- if (session->req)
- pending_req_free(session->req);
+ session->ref--;
- g_slist_free_full(session->seps, sep_free);
+ DBG("%p: ref=%d", session, session->ref);
- g_free(session->buf);
+ if (session->ref > 0)
+ return;
- g_free(session);
+ set_disconnect_timer(session);
}
struct avdtp *avdtp_ref(struct avdtp *session)
@@ -2231,12 +2218,6 @@ static gboolean session_cb(GIOChannel *chan, GIOCondition cond,
goto failed;
}
- if (session->ref == 1 && !session->streams && !session->req)
- set_disconnect_timer(session);
-
- if (session->streams && session->dc_timer)
- remove_disconnect_timer(session);
-
if (session->req && session->req->collided) {
DBG("Collision detected");
goto next;
@@ -2383,7 +2364,7 @@ static struct avdtp *avdtp_get_internal(const bdaddr_t *src, const bdaddr_t *dst
session->server = server;
bacpy(&session->dst, dst);
- session->ref = 1;
+ set_disconnect_timer(session);
/* We don't use avdtp_set_state() here since this isn't a state change
* but just setting of the initial state */
session->state = AVDTP_SESSION_STATE_DISCONNECTED;
@@ -2490,6 +2471,8 @@ static void auth_cb(DBusError *derr, void *user_data)
struct avdtp *session = user_data;
GError *err = NULL;
+ avdtp_unref(session);
+
if (derr && dbus_error_is_set(derr)) {
error("Access denied: %s", derr->message);
connection_lost(session, EACCES);
@@ -2578,10 +2561,12 @@ static void avdtp_confirm_cb(GIOChannel *chan, gpointer data)
auth_cb, session);
if (session->auth_id == 0) {
avdtp_set_state(session, AVDTP_SESSION_STATE_DISCONNECTED);
- avdtp_unref(session);
goto drop;
}
+ /* Disable disconnect timer while authorizing */
+ avdtp_ref(session);
+
dev->auto_connect = auto_connect;
return;
@@ -3949,23 +3934,12 @@ proceed:
void avdtp_exit(const bdaddr_t *src)
{
struct avdtp_server *server;
- GSList *l;
server = find_server(servers, src);
if (!server)
return;
- l = server->sessions;
- while (l) {
- struct avdtp *session = l->data;
-
- l = l->next;
- /* value of l pointer should be updated before invoking
- * connection_lost since it internally uses avdtp_unref
- * which operates on server->session list as well
- */
- connection_lost(session, -ECONNABORTED);
- }
+ g_slist_free_full(server->sessions, avdtp_free);
servers = g_slist_remove(servers, server);
--
1.7.11.4
^ permalink raw reply related
* Re: gatttool and multiple devices connected
From: Johan Hedberg @ 2012-10-17 11:35 UTC (permalink / raw)
To: Anderson Lizardo; +Cc: Kim Schulz, linux-bluetooth
In-Reply-To: <CAJdJm_P+udqJta3K-FhzYEO4bOm+W=u9koV953etN8TmO=eo4w@mail.gmail.com>
Hi Lizardo,
On Wed, Oct 17, 2012, Anderson Lizardo wrote:
> On Wed, Oct 17, 2012 at 6:00 AM, Kim Schulz <kim@schulz.dk> wrote:
> > Turns out that the guy compiling our kernel choose a version
> > 3.4.0.030400 so not really the
> > bleeding edge version.
> > Do you have any recommendation on what version of the kernel we should
> > aim for?
>
> This kernel is actually not that old (as long as it is based on the
> official 3.4.0 one). Did you have hcidump logs for the attempt to
> discover primary services on device B while connected to device A?
Actually I'd say that it is too old for LE usage. E.g. the address type
field to the L2CAP socket address was introduced in 3.5 which is quite
important from an LE standpoint.
Johan
^ permalink raw reply
* Re: gatttool and multiple devices connected
From: Anderson Lizardo @ 2012-10-17 11:21 UTC (permalink / raw)
To: Kim Schulz; +Cc: linux-bluetooth
In-Reply-To: <005601cdac45$e1ee8ac0$a5cba040$@dk>
Hi Kim,
On Wed, Oct 17, 2012 at 6:00 AM, Kim Schulz <kim@schulz.dk> wrote:
> Turns out that the guy compiling our kernel choose a version
> 3.4.0.030400 so not really the
> bleeding edge version.
> Do you have any recommendation on what version of the kernel we should
> aim for?
This kernel is actually not that old (as long as it is based on the
official 3.4.0 one). Did you have hcidump logs for the attempt to
discover primary services on device B while connected to device A?
Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil
^ permalink raw reply
* Re: [PATCH] device: Fix modifying list while iterating
From: Johan Hedberg @ 2012-10-17 10:42 UTC (permalink / raw)
To: Mikel Astiz; +Cc: linux-bluetooth, Mikel Astiz
In-Reply-To: <1350468429-30882-1-git-send-email-mikel.astiz.oss@gmail.com>
Hi Mikel,
On Wed, Oct 17, 2012, Mikel Astiz wrote:
> A list should not be modified while iterating on it, and in this case
> the solution is trivial: the code is just trying to free the whole list
> with a previous call to profile->device_remove() per list item.
> ---
> src/device.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Applied. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH] avdtp: Fix crash on connection lost
From: Luiz Augusto von Dentz @ 2012-10-17 10:32 UTC (permalink / raw)
To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <5925869.iVRXt7vIOW@uw000953>
Hi Szymon,
On Wed, Oct 17, 2012 at 12:17 PM, Szymon Janc <szymon.janc@tieto.com> wrote:
> On Wednesday 17 of October 2012 11:48:36 Luiz Augusto von Dentz wrote:
>> Hi Szymon,
>
> Hi Luiz,
>
>> >> We should probably get rid of the free_lock as well.
>> >
>> > Well, this is what this patch does, doesn't it?:)
>>
>> What about the attached patch, it should fix the references for good
>> by using the dc_timer when the reference drops to 0, I just need to
>> check if 1 second is enough to cover all cases including the ones
>> where the remote device connects but doesn't setup any stream, with
>> the devices I have 1 second seems enough.
>
> Yeap, this fix crash I've observed.
>
> However, avdtp_get_internal() now returns newly created session with ref=0
> and in avdtp_confirm_cb() there is unref called when btd_request_authorization
> failed. Due to ref being signed int (why?) this could lead to not-nice-to-debug
> bug if that session is reclaimed before dc timer fires (ref would go from -1 to
> 0).
Good catch, I should be able to fix this, perhaps we can free
immediately or let the timeout handle this as well.
> Also I'm having trouble with tracking lifetime of session object.. I think
> only avdtp_ref/unref function should really care about ref count. That would
> require other function to only use ref/unref to get/free session object.
> e.g. avdtp_free should be only called from disconnect_timeout() and
> connection_lost() should only trigger other session users to drop their
> references. Same goes to set_disconnect_timer() which should only be called
> from avdtp_unref. There should be no need to call connection_lost from
> disconnect_timeout as this timer should only run when there is no references
> held to that session, right?
Well that involves changing some other logic behind the timeout, such as this:
if (session->ref == 0 && !session->streams && !session->req)
set_disconnect_timer(session);
if (session->streams && session->dc_timer)
remove_disconnect_timer(session);
IMO those should be removed in favor of the ref/unref handling if the
disconnect timer should be started/removed, if there is a request the
user should be holding a reference and the same for the stream, this
should actually simplify even further the code. Also the reason I did
avdtp_free in the connection_lost was not to much too much code around
as avdtp_free call a lot of other functions, but you are probably
right and this should actually be called from disconnect_timeout.
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH] avdtp: Fix crash on connection lost
From: Szymon Janc @ 2012-10-17 10:17 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <CABBYNZKQWy2CmM2FLZyw24UXEo202SsHymK-jzGF+k2dmg6brw@mail.gmail.com>
On Wednesday 17 of October 2012 11:48:36 Luiz Augusto von Dentz wrote:
> Hi Szymon,
Hi Luiz,
> >> We should probably get rid of the free_lock as well.
> >
> > Well, this is what this patch does, doesn't it?:)
>
> What about the attached patch, it should fix the references for good
> by using the dc_timer when the reference drops to 0, I just need to
> check if 1 second is enough to cover all cases including the ones
> where the remote device connects but doesn't setup any stream, with
> the devices I have 1 second seems enough.
Yeap, this fix crash I've observed.
However, avdtp_get_internal() now returns newly created session with ref=0
and in avdtp_confirm_cb() there is unref called when btd_request_authorization
failed. Due to ref being signed int (why?) this could lead to not-nice-to-debug
bug if that session is reclaimed before dc timer fires (ref would go from -1 to
0).
Also I'm having trouble with tracking lifetime of session object.. I think
only avdtp_ref/unref function should really care about ref count. That would
require other function to only use ref/unref to get/free session object.
e.g. avdtp_free should be only called from disconnect_timeout() and
connection_lost() should only trigger other session users to drop their
references. Same goes to set_disconnect_timer() which should only be called
from avdtp_unref. There should be no need to call connection_lost from
disconnect_timeout as this timer should only run when there is no references
held to that session, right?
Or do I miss something?:)
--
BR
Szymon Janc
^ permalink raw reply
* [PATCH] device: Fix modifying list while iterating
From: Mikel Astiz @ 2012-10-17 10:07 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Mikel Astiz
From: Mikel Astiz <mikel.astiz@bmw-carit.de>
A list should not be modified while iterating on it, and in this case
the solution is trivial: the code is just trying to free the whole list
with a previous call to profile->device_remove() per list item.
---
src/device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/device.c b/src/device.c
index f48faba..bb0f890 100644
--- a/src/device.c
+++ b/src/device.c
@@ -872,8 +872,6 @@ static void profile_remove(struct btd_profile *profile,
struct btd_device *device)
{
profile->device_remove(profile, device);
-
- device->profiles = g_slist_remove(device->profiles, profile);
}
static gboolean do_disconnect(gpointer user_data)
@@ -899,6 +897,8 @@ int device_block(struct btd_device *device, gboolean update_only)
do_disconnect(device);
g_slist_foreach(device->profiles, (GFunc) profile_remove, device);
+ g_slist_free(device->profiles);
+ device->profiles = NULL;
if (!update_only)
err = btd_adapter_block_address(device->adapter,
--
1.7.11.7
^ permalink raw reply related
* Bluez/Obexd upstream test result_20121017(bluez-4.101.681+ obexd-0.47.66)
From: Li, XiaX @ 2012-10-17 9:58 UTC (permalink / raw)
To: linux-bluetooth@vger.kernel.org; +Cc: zhouzhy@neusoft.com
[-- Attachment #1: Type: text/plain, Size: 1303 bytes --]
Hi all,
QA finished upstream testing. The test is for bluez-4.101.681 and obexd-0.47.66. Against last,
BlueZ is changed from 4.101.319 to 4.101.681, Obexd is changed from 0.47.37 to 0.47.66.
In this test, totally ran 100 case: 72 Pass, 10 Fail. Other 18 are blocked or unavailable (some
cases validation method is still in investigating).
The pass rate is 88% (pass / <pass + fail>). Ofono has not caught up with the BlueZ API changing in HFP profile.
New bugs:
===============================================
25748 - [HFP]Ofono list-modems shows no modem after paired with phone.
https://bugs.meego.com/show_bug.cgi?id=25748
Re-open bugs:
===============================================
No
Verified bugs:
===============================================
No
Testing Environment
==============================================
Hardware: netbook Eeepc 1005HA | Acer AspireOne NAV50
Image: netbook-ia32-pinetrail-tizen_20120424.2
Linux Kernel: v3.4-rc7
bluez-4.101.681.g6f4d4e0-1.1.i586
obexd-0.47.66.g2281d4f-2.1.i586
obexd-server-0.47.66.g2281d4f-2.1.i586
connman-1.8.6.g9f0fc79-4.1.i586
ofono-1.11.114.g73c69ee-2.1.i586
Pulseaudio: 67602d8743e8e529919bb9fbb956aa77724a8e50 (oct, 6th)
For detailed test results, please see attached file.
Thanks
Li Xia
[-- Attachment #2: Bluetooth_upstream_quality_20121017.txt --]
[-- Type: text/plain, Size: 7554 bytes --]
== Detail Result ==
[PASS]: case passed <br>
[FAIL]: case failed <br>
[Block]: case is blocked by one bug <br>
[N/A]: case not available to be tested
=== Audio-A2DP ===
[PASS] A2DP_001: audio sink connect/re-connect
[PASS] A2DP_002: stereo headset playback
[PASS] A2DP_003: stereo headset voice record
[PASS] A2DP_004: Codec support: SBC
[PASS] A2DP_005: Playback music on line
[PASS] A2DP_006: SINK role, as speaker for other device music playing
With some BT headset, we ever met issue [https://bugs.meego.com/show_bug.cgi?id=25480 Bug#25480 A2DP-source: pulseaudio fails to create bluez_sink and bluez_source]
=== Audio-AVRCP ===
[PASS] AVRCP_001: Volume Up/Down
[Block] AVRCP_002: Play/Pause (by unavailable media-player)
[Block] AVRCP_003: Next/Privious (by unavailable media-player)
[PASS] AVRCP_004: Connection Establish/Release
[N/A] AVRCP_005: Audio metadata get.
We still try to find avaliable media-player to support AVRCP-1.4
=== Audio-HFP ===
Ofono needs to update its API, since BlueZ already took the changes. Here is bug [https://bugs.meego.com/show_bug.cgi?id=25748 Bug#25748 HFP: Ofono List-modems shows no modem after paired with phone]
[FAIL] HFP_001: RFCOMM connection on AG
Need install sbc for latest pulseaudio
[FAIL] HFP_002: list (ofono) hfp modem
[FAIL] HFP_003: HFP modem establishment (Ofono)
[FAIL] HFP_004: Audio SNK and SRC establishment
[FAIL] HFP_005: voicecall, audio creates BT SNK/SRC
[FAIL] HFP_006: redirect AG SNK/SRC to local SRC/SNK
BlueZ interface has some regression, bug is reported as [https://bugs.meego.com/show_bug.cgi?id=25473 Bug#25473 HFP: latest bluez cannot support pulseaudio to create bluez_sink and bluez_source]
Pulseaudio has new crash bug: [https://bugs.meego.com/show_bug.cgi?id=25593 Bug#25593 Pulseaudio crashes after connect BT sink to alsa source]
[FAIL] HFP_007: Check AT-commands from HF part
[FAIL] HFP_008: Connect BT Headset to HFP phone
=== Audio-HSP ===
[PASS] HSP_001: Use mono headset to play music
*** DUT is a Netbook, unable to take phone ***
[N/A] HSP_002: Take incoming call by button-press
[N/A] HSP_003: Audio transfer between AG and HS
[PASS] HSP_004: Adjust Volume Up/Down
=== OBEX-FTP ===
[PASS] FTP_001: pull/push files from/to server.
[PASS] FTP_002: browse server files
[PASS] FTP_003: Client "delete file", "rename file"
[PASS] FTP_004: Server enables FTP parameter
[PASS] FTP_005: Server sets sharing root path
[PASS] FTP_006: Server handles all requirements
[PASS] FTP_007: Server has ability to set permission for FTP
[PASS] FTP_008: FTP data-rate about 30k/s~230k/s
[PASS] FTP_009: Big size file transferred stable
=== OBEX-OPP ===
[PASS] OPP_001: pull un-patterned object from server.
[PASS] OPP_002: Server enables OPP parameter
[PASS] OPP_003: During object transferring, the progress is clear
=== OBEX-PBAP ===
[PASS] PBAP_001: Client gets phone book entries from server
[PASS] PBAP_002: Client gets ICH, OCH, MCH and CCH from server
[FAIL] PBAP_003: PSE can provide PBAP daemon by enabling corresponding parameter
Case failed due to [https://bugs.meego.com/show_bug.cgi?id=25598 Bug#25598, PBAP server fails to open /telecom/pb.vcf file]
[PASS] PBAP_004: Both sides support vCard2.1/vCard3.0
=== OBEX-SYNC ===
[PASS] SYNC_001: Server enables SYNC daemon
[PASS] SYNC_002: During sync, Server can show "syncing..."
[PASS] SYNC_003: client can set PIM fetching from INT
[PASS] SYNC_004: Server get/put entire phonebook from/to client.
[PASS] SYNC_005: Client can support vCard2.1, vCard3.0
=== OBEX-MAP MCE ===
[FAIL] MAP_001: MCE can browse message/folder list on MSE
Regression happened on client side, bug is reported as [https://bugs.meego.com/show_bug.cgi?id=25595 Bug#25595 MAP client fail to get message list from Samsung GT-i9100]
[BLOCK] MAP_002: MCE can upload local message to MSE
[BLOCK] MAP_003: MCE can delete the message on MSE side
[BLOCK] MAP_004: MCE can take use of MSE to send message
=== Network-PAN ===
[PASS] PAN_001: PANU can init nap0 device connect to NAP
[PASS] PAN_002: PANU can get ip address or assigned static ip
[PASS] PAN_003: PANU can logon internet website
[N/A] PAN_004: NAP can init bridge bnep
[N/A] PAN_005: NAP can support one or multiple PANU connection
[N/A] PAN_006: NAP can have DHCP responding to each PANU
Current connman does not support DHCP functions.
=== Network-BNEP ===
[PASS] BNEP_001: Check BNEP support on DUT
=== Network-DUN ===
[PASS] DUN_001: GW (DUT) can parse a series of AT commands from the data terminal.
[PASS] DUN_002: DT can build up rfcomm device by bluetooth and DUN modem by Ofono.
[PASS] DUN_003: DT can use Ofono to dial up special service number ("*99#").
[PASS] DUN_004: When network connected, DT can log on website in internet.
=== SIM-SAP ===
The SAP test method is still in investigation.
[N/A] SAP_001: Server can enable a module to register Client
[N/A] SAP_002: Server can power on/off SIM Card or reset it
[N/A] SAP_003: Server can disconnect Client
[N/A] SAP_004: Server can disconnect SIM
[N/A] SAP_005: Client can connect to Server SIM
[N/A] SAP_006: Client can power on Server SIM
[N/A] SAP_007: Client can disconnect Server
=== Generic-GAP ===
[PASS] GAP_001: PSCAN/ISCAN mode setting.
[PASS] GAP_002: Active pairing to another bluetooth device.
[PASS] GAP_003: Passive pairing, accepte pair master requirement
[PASS] GAP_004: SSP supports
[PASS] GAP_005: If lost link-key, it needs re-pair
[PASS] GAP_006: reboot DUT with link-key restored, no need to re-pair
[PASS] GAP_007: Pair can be released
=== Generic-SDP ===
[PASS] SDP_001: browse available service list in local
[PASS] SDP_002: browse available service list from remote
[PASS] SDP_003: browse service by RecHandle for detail info
[PASS] SDP_004: Server can answer SDP searching request
[PASS] SDP_005: Server can add/del/update service records
=== Others-HID ===
[PASS] HID_001: Host can search nearby HID device.
[PASS] HID_002: [BAT] Host can connect to HID device.
[PASS] HID_003: Host can handle multiple human input/output devices.
=== HCI ===
[PASS] HCI_001: Build hci connection between Dev_A and Dev_B.
[PASS] HCI_002: Receive ACL data with HCI.
[PASS] HCI_003: Send ACL data with HCI.
[PASS] HCI_004: Change name of remote Dev by HCI.
[PASS] HCI_005: Change class of remote Dev by HCI.
[PASS] HCI_006: Read local HCI controller information.
[PASS] HCI_007: Read remote HCI controller information.
=== L2CAP ===
[PASS] L2CAP_001: Build l2cap connection between Dev_A and Dev_B.
[PASS] L2CAP_002: Dev_A and Dev_B take l2cap protocol to do pingpong.
=== RFCOMM ===
[PASS] RFCOMM_001: Build rfcomm connection between Dev_A and Dev_B.
[PASS] RFCOMM_002: Dev_A and Dev_B take rfcomm protocol to do pingpong.
=== Settings ===
[PASS] SET_001: [BAT] No error output during bluetoothd startup process.
=== Bluetooth Utils ===
Some basic checking for hciconfig, hcitool, sdptool commands.
[PASS] BTCMD_001: hciconfig -a
[PASS] BTCMD_002: hciconfig <adapter> piscan
[PASS] BTCMD_003: hciconfig <adapter> up/down
[PASS] BTCMD_004: hcitool scan
[PASS] BTCMD_005: sdptool browse local
[PASS] BTCMD_006: sdptool browse <ermote MAC>
[PASS] BTCMD_007: sdptool add/del
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox