[PATCH BlueZ 1/3 v3] AVRCP: Fix not handling commands while browsing is connecting

[PATCH BlueZ 1/3 v3] AVRCP: Fix not handling commands while browsing is connecting

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

With introdution of browsing channel the .init callback is called when
browsing channel connection completes, but in the meantime the remote
device can send commands over control channel.

To fix this the init callaback of control and browsing channel are now
separated into .init_control and .init_browsing so the handler can be
register as soon the respective channel connection completes.
---
v2: Separate control and browsing inits and use handler id to track if channel
has been initialized or not.
v3: Adds patches 2 and 3

 profiles/audio/avrcp.c | 73 +++++++++++++++++++++++++++++---------------------
 1 file changed, 42 insertions(+), 31 deletions(-)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 4965b0c..6b61664 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -190,9 +190,9 @@ struct avrcp {
 	gboolean target;
 	uint16_t version;
 	int features;
-	bool initialized;
 
-	void (*init) (struct avrcp *session);
+	void (*init_control) (struct avrcp *session);
+	void (*init_browsing) (struct avrcp *session);
 	void (*destroy) (struct avrcp *sesion);
 
 	const struct control_pdu_handler *control_handlers;
@@ -2165,14 +2165,23 @@ static struct avrcp *find_session(GSList *list, struct audio_device *dev)
 	return NULL;
 }
 
-static void session_tg_init(struct avrcp *session)
+static void session_tg_init_browsing(struct avrcp *session)
+{
+	session->browsing_id = avctp_register_browsing_pdu_handler(
+							session->conn,
+							handle_browsing_pdu,
+							session);
+}
+
+static void session_tg_init_control(struct avrcp *session)
 {
 	struct avrcp_server *server = session->server;
 	struct avrcp_player *player;
 
-	DBG("%p version 0x%04x", session, session->version);
+	if (session->version < 0x0103)
+		return;
 
-	session->initialized = true;
+	DBG("%p version 0x%04x", session, session->version);
 
 	player = g_slist_nth_data(server->players, 0);
 	if (player != NULL) {
@@ -2180,6 +2189,10 @@ static void session_tg_init(struct avrcp *session)
 		player->sessions = g_slist_prepend(player->sessions, session);
 	}
 
+	session->control_id = avctp_register_pdu_handler(session->conn,
+							AVC_OP_VENDORDEP,
+							handle_vendordep_pdu,
+							session);
 	session->control_handlers = tg_control_handlers;
 	session->supported_events = (1 << AVRCP_EVENT_STATUS_CHANGED) |
 				(1 << AVRCP_EVENT_TRACK_CHANGED) |
@@ -2190,36 +2203,31 @@ static void session_tg_init(struct avrcp *session)
 	if (session->version >= 0x0104)
 		avrcp_register_notification(session,
 						AVRCP_EVENT_VOLUME_CHANGED);
+}
 
-	session->control_id = avctp_register_pdu_handler(session->conn,
-							AVC_OP_VENDORDEP,
-							handle_vendordep_pdu,
-							session);
+static void session_ct_init_browsing(struct avrcp *session)
+{
 	session->browsing_id = avctp_register_browsing_pdu_handler(
 							session->conn,
 							handle_browsing_pdu,
 							session);
 }
 
-static void session_ct_init(struct avrcp *session)
+static void session_ct_init_control(struct avrcp *session)
 {
 	struct avrcp_player *player;
 	struct media_player *mp;
 	const char *path;
 
-	session->control_handlers = ct_control_handlers;
-
-	if (session->version >= 0x0104)
-		session->supported_events = (1 << AVRCP_EVENT_VOLUME_CHANGED);
-
 	DBG("%p version 0x%04x", session, session->version);
 
-	session->initialized = true;
-
 	session->control_id = avctp_register_pdu_handler(session->conn,
 							AVC_OP_VENDORDEP,
 							handle_vendordep_pdu,
 							session);
+	session->control_handlers = ct_control_handlers;
+	if (session->version >= 0x0104)
+		session->supported_events = (1 << AVRCP_EVENT_VOLUME_CHANGED);
 
 	player = g_new0(struct avrcp_player, 1);
 	player->sessions = g_slist_prepend(player->sessions, session);
@@ -2316,11 +2324,13 @@ static struct avrcp *session_create(struct avrcp_server *server,
 		session->target = FALSE;
 
 	if (session->target) {
-		session->init = session_tg_init;
+		session->init_control = session_tg_init_control;
+		session->init_browsing = session_tg_init_browsing;
 		session->destroy = session_tg_destroy;
 		rec = btd_device_get_record(dev->btd_dev, AVRCP_REMOTE_UUID);
 	} else {
-		session->init = session_ct_init;
+		session->init_control = session_ct_init_control;
+		session->init_browsing = session_ct_init_browsing;
 		session->destroy = session_ct_destroy;
 		rec = btd_device_get_record(dev->btd_dev, AVRCP_TARGET_UUID);
 	}
@@ -2368,26 +2378,27 @@ static void state_changed(struct audio_device *dev, avctp_state_t old_state,
 
 		break;
 	case AVCTP_STATE_CONNECTED:
-		if (session == NULL || session->initialized)
+		if (session == NULL)
 			break;
 
-		/* Initialize session if browsing cannot be used */
-		if (session->version <= 0x0103 ||
-				old_state == AVCTP_STATE_BROWSING_CONNECTING ||
-				!(session->features & AVRCP_FEATURE_BROWSING)) {
-			session->init(session);
-			break;
-		}
+		if (session->browsing_id > 0)
+			session->browsing_id = 0;
+
+		if (session->control_id > 0)
+			return;
+
+		session->init_control(session);
 
-		if (avctp_connect_browsing(session->conn) != 0)
-			session->init(session);
+		if (session->version >= 0x0104 &&
+				session->features & AVRCP_FEATURE_BROWSING)
+			avctp_connect_browsing(session->conn);
 
 		break;
 	case AVCTP_STATE_BROWSING_CONNECTED:
-		if (session == NULL || session->initialized)
+		if (session == NULL || session->browsing_id > 0)
 			break;
 
-		session->init(session);
+		session->init_browsing(session);
 		break;
 	default:
 		return;
-- 
1.8.1

[PATCH BlueZ 2/3 v3] AVCTP: Fix not destroying browsing channel if disconnected

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

If the browsing channel is disconnected it should be destroyed
immediatelly and set to NULL otherwise it will point to invalid channel.
---
 profiles/audio/avctp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
index 6ad2b66..6cac718 100644
--- a/profiles/audio/avctp.c
+++ b/profiles/audio/avctp.c
@@ -485,6 +485,10 @@ static void avctp_set_state(struct avctp *session, avctp_state_t new_state)
 		break;
 	case AVCTP_STATE_CONNECTED:
 		DBG("AVCTP Connected");
+		if (session->browsing) {
+			avctp_channel_destroy(session->browsing);
+			session->browsing = NULL;
+		}
 		break;
 	case AVCTP_STATE_BROWSING_CONNECTING:
 		DBG("AVCTP Browsing Connecting");
-- 
1.8.1

[PATCH BlueZ 3/3 v3] AVCTP: Add destroy callback to avctp_register_browsing_pdu_handler

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

This adds a destroy callback which is called when the PDU handler is
destroyed.
---
 profiles/audio/avctp.c | 44 +++++++++++++++++++++++++++++++++++++-------
 profiles/audio/avctp.h |  3 ++-
 profiles/audio/avrcp.c | 21 ++++++++++++---------
 3 files changed, 51 insertions(+), 17 deletions(-)

diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
index 6cac718..41e2f46 100644
--- a/profiles/audio/avctp.c
+++ b/profiles/audio/avctp.c
@@ -168,6 +168,7 @@ struct avctp_channel {
 	GQueue *queue;
 	GSList *processed;
 	guint process_id;
+	GDestroyNotify destroy;
 };
 
 struct key_pressed {
@@ -206,6 +207,7 @@ struct avctp_browsing_pdu_handler {
 	avctp_browsing_pdu_cb cb;
 	void *user_data;
 	unsigned int id;
+	GDestroyNotify destroy;
 };
 
 static struct {
@@ -406,6 +408,9 @@ static void avctp_channel_destroy(struct avctp_channel *chan)
 	if (chan->process_id > 0)
 		g_source_remove(chan->process_id);
 
+	if (chan->destroy)
+		chan->destroy(chan);
+
 	g_free(chan->buffer);
 	g_queue_foreach(chan->queue, pending_destroy, NULL);
 	g_queue_free(chan->queue);
@@ -985,7 +990,8 @@ static void init_uinput(struct avctp *session)
 }
 
 static struct avctp_channel *avctp_channel_create(struct avctp *session,
-							GIOChannel *io)
+							GIOChannel *io,
+							GDestroyNotify destroy)
 {
 	struct avctp_channel *chan;
 
@@ -993,10 +999,30 @@ static struct avctp_channel *avctp_channel_create(struct avctp *session,
 	chan->session = session;
 	chan->io = g_io_channel_ref(io);
 	chan->queue = g_queue_new();
+	chan->destroy = destroy;
 
 	return chan;
 }
 
+static void handler_free(void *data)
+{
+	struct avctp_browsing_pdu_handler *handler = data;
+
+	if (handler->destroy)
+		handler->destroy(handler->user_data);
+
+	g_free(data);
+}
+
+static void avctp_destroy_browsing(void *data)
+{
+	struct avctp_channel *chan = data;
+
+	g_slist_free_full(chan->handlers, handler_free);
+
+	chan->handlers = NULL;
+}
+
 static void avctp_connect_browsing_cb(GIOChannel *chan, GError *err,
 							gpointer data)
 {
@@ -1026,7 +1052,8 @@ static void avctp_connect_browsing_cb(GIOChannel *chan, GError *err,
 	DBG("AVCTP Browsing: connected to %s", address);
 
 	if (session->browsing == NULL)
-		session->browsing = avctp_channel_create(session, chan);
+		session->browsing = avctp_channel_create(session, chan,
+						avctp_destroy_browsing);
 
 	session->browsing->imtu = imtu;
 	session->browsing->omtu = omtu;
@@ -1075,7 +1102,7 @@ static void avctp_connect_cb(GIOChannel *chan, GError *err, gpointer data)
 	DBG("AVCTP: connected to %s", address);
 
 	if (session->control == NULL)
-		session->control = avctp_channel_create(session, chan);
+		session->control = avctp_channel_create(session, chan, NULL);
 
 	session->control->imtu = imtu;
 	session->control->omtu = omtu;
@@ -1189,7 +1216,7 @@ static void avctp_control_confirm(struct avctp *session, GIOChannel *chan,
 	}
 
 	avctp_set_state(session, AVCTP_STATE_CONNECTING);
-	session->control = avctp_channel_create(session, chan);
+	session->control = avctp_channel_create(session, chan, NULL);
 
 	src = adapter_get_address(device_get_adapter(dev->btd_dev));
 	dst = device_get_address(dev->btd_dev);
@@ -1666,7 +1693,8 @@ unsigned int avctp_register_pdu_handler(struct avctp *session, uint8_t opcode,
 
 unsigned int avctp_register_browsing_pdu_handler(struct avctp *session,
 						avctp_browsing_pdu_cb cb,
-						void *user_data)
+						void *user_data,
+						GDestroyNotify destroy)
 {
 	struct avctp_channel *browsing = session->browsing;
 	struct avctp_browsing_pdu_handler *handler;
@@ -1682,6 +1710,7 @@ unsigned int avctp_register_browsing_pdu_handler(struct avctp *session,
 	handler->cb = cb;
 	handler->user_data = user_data;
 	handler->id = ++id;
+	handler->destroy = destroy;
 
 	browsing->handlers = g_slist_append(browsing->handlers, handler);
 
@@ -1787,7 +1816,7 @@ struct avctp *avctp_connect(struct audio_device *device)
 		return NULL;
 	}
 
-	session->control = avctp_channel_create(session, io);
+	session->control = avctp_channel_create(session, io, NULL);
 	g_io_channel_unref(io);
 
 	return session;
@@ -1821,7 +1850,8 @@ int avctp_connect_browsing(struct avctp *session)
 		return -EIO;
 	}
 
-	session->browsing = avctp_channel_create(session, io);
+	session->browsing = avctp_channel_create(session, io,
+						avctp_destroy_browsing);
 	g_io_channel_unref(io);
 
 	return 0;
diff --git a/profiles/audio/avctp.h b/profiles/audio/avctp.h
index 6cbda92..411b093 100644
--- a/profiles/audio/avctp.h
+++ b/profiles/audio/avctp.h
@@ -110,7 +110,8 @@ gboolean avctp_unregister_pdu_handler(unsigned int id);
 
 unsigned int avctp_register_browsing_pdu_handler(struct avctp *session,
 						avctp_browsing_pdu_cb cb,
-						void *user_data);
+						void *user_data,
+						GDestroyNotify destroy);
 gboolean avctp_unregister_browsing_pdu_handler(unsigned int id);
 
 int avctp_send_passthrough(struct avctp *session, uint8_t op);
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 6b61664..f83c3db 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -2165,12 +2165,20 @@ static struct avrcp *find_session(GSList *list, struct audio_device *dev)
 	return NULL;
 }
 
+static void destroy_browsing(void *data)
+{
+	struct avrcp *session = data;
+
+	session->browsing_id = 0;
+}
+
 static void session_tg_init_browsing(struct avrcp *session)
 {
 	session->browsing_id = avctp_register_browsing_pdu_handler(
 							session->conn,
 							handle_browsing_pdu,
-							session);
+							session,
+							destroy_browsing);
 }
 
 static void session_tg_init_control(struct avrcp *session)
@@ -2210,7 +2218,8 @@ static void session_ct_init_browsing(struct avrcp *session)
 	session->browsing_id = avctp_register_browsing_pdu_handler(
 							session->conn,
 							handle_browsing_pdu,
-							session);
+							session,
+							destroy_browsing);
 }
 
 static void session_ct_init_control(struct avrcp *session)
@@ -2378,15 +2387,9 @@ static void state_changed(struct audio_device *dev, avctp_state_t old_state,
 
 		break;
 	case AVCTP_STATE_CONNECTED:
-		if (session == NULL)
+		if (session == NULL || session->control_id > 0)
 			break;
 
-		if (session->browsing_id > 0)
-			session->browsing_id = 0;
-
-		if (session->control_id > 0)
-			return;
-
 		session->init_control(session);
 
 		if (session->version >= 0x0104 &&
-- 
1.8.1

Re: [PATCH BlueZ 1/3 v3] AVRCP: Fix not handling commands while browsing is connecting

[Johan Hedberg]

Hi Luiz,

On Sun, Jan 27, 2013, Luiz Augusto von Dentz wrote:
> With introdution of browsing channel the .init callback is called when
> browsing channel connection completes, but in the meantime the remote
> device can send commands over control channel.
> 
> To fix this the init callaback of control and browsing channel are now
> separated into .init_control and .init_browsing so the handler can be
> register as soon the respective channel connection completes.
> ---
> v2: Separate control and browsing inits and use handler id to track if channel
> has been initialized or not.
> v3: Adds patches 2 and 3
> 
>  profiles/audio/avrcp.c | 73 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 42 insertions(+), 31 deletions(-)

All three patches have been applied. Thanks.

Johan