[PATCH 0/5] avrcp: fixes and pdu-continuation

[PATCH 0/5] avrcp: fixes and pdu-continuation

[Lucas De Marchi]

Patch 0001 and 0003 are nice small refactor, whereas 0002 fixes the meaning of
one metadata field (spec is not so clear, but this makes much more sense). I
put David Stockwell as the author since he wrote them; I split, rebased and
sent to ML again.

Patch 0004 fixes an issue when metadata fields sum more than ~500 bytes. It
just refuses to write more data to the PDU if this condition happens. The last
patch goes a step further and implements proper pdu-continuation (really
annoying since fields might split in the middle, but essential for passing
AVRCP 1.3 tests).



*** BLURB HERE ***

David Stockwell (3):
  avrcp: use LAST element on media_info_id enum
  avrcp: fix handling of metadata item 0x7
  avrcp: get/set three-byte company-id

Lucas De Marchi (2):
  avrcp: limit avrcp packet size to the MTU of AV/C
  avrcp: implement pdu continuation request and abort

 audio/control.c |  350 ++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 269 insertions(+), 81 deletions(-)

-- 
1.7.6.1

[PATCH 1/5] avrcp: use LAST element on media_info_id enum

[Lucas De Marchi]

From: David Stockwell <dstockwell@frequency-one.com>

---
 audio/control.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/audio/control.c b/audio/control.c
index 9990b06..8857737 100644
--- a/audio/control.c
+++ b/audio/control.c
@@ -195,6 +195,7 @@ enum media_info_id {
 	MEDIA_INFO_N_TRACKS =		5,
 	MEDIA_INFO_GENRE =		6,
 	MEDIA_INFO_CURRENT_POSITION =	7,
+	MEDIA_INFO_LAST
 };
 
 static DBusConnection *connection = NULL;
@@ -1109,7 +1110,7 @@ static int avrcp_handle_get_element_attributes(struct control *control,
 		 * Return all available information, at least
 		 * title must be returned.
 		 */
-		for (i = 1; i <= MEDIA_INFO_CURRENT_POSITION; i++) {
+		for (i = 1; i < MEDIA_INFO_LAST; i++) {
 			size = mp_get_media_attribute(control->mp, i,
 							&pdu->params[pos]);
 
-- 
1.7.6.1

[PATCH 2/5] avrcp: fix handling of metadata item 0x7

[Lucas De Marchi]

From: David Stockwell <dstockwell@frequency-one.com>

Metadata field number 0x7 should be the total playing time of the track
(TrackDuration) in msec, not current position within track.
---
 audio/control.c |   18 ++++++------------
 1 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/audio/control.c b/audio/control.c
index 8857737..2f240f0 100644
--- a/audio/control.c
+++ b/audio/control.c
@@ -194,7 +194,7 @@ enum media_info_id {
 	MEDIA_INFO_TRACK =		4,
 	MEDIA_INFO_N_TRACKS =		5,
 	MEDIA_INFO_GENRE =		6,
-	MEDIA_INFO_CURRENT_POSITION =	7,
+	MEDIA_INFO_PLAYING_TIME =	7,
 	MEDIA_INFO_LAST
 };
 
@@ -910,19 +910,13 @@ static int mp_get_media_attribute(struct media_player *mp,
 		len = strlen(valstr);
 		memcpy(elem->val, valstr, len);
 		break;
-	case MEDIA_INFO_CURRENT_POSITION:
-		if (mi->elapsed != 0xFFFFFFFF) {
-			uint32_t elapsed;
-
-			mp_get_playback_status(mp, NULL, &elapsed, NULL);
-
-			snprintf(valstr, 20, "%u", elapsed);
-			len = strlen(valstr);
-			memcpy(elem->val, valstr, len);
-		} else {
+	case MEDIA_INFO_PLAYING_TIME:
+		if (mi->track_len == 0xFFFFFFFF)
 			return -ENOENT;
-		}
 
+		snprintf(valstr, 20, "%u", mi->track_len);
+		len = strlen(valstr);
+		memcpy(elem->val, valstr, len);
 		break;
 	default:
 		return -EINVAL;
-- 
1.7.6.1

[PATCH 3/5] avrcp: get/set three-byte company-id

[Lucas De Marchi]

From: David Stockwell <dstockwell@frequency-one.com>

---
 audio/control.c |   36 +++++++++++++++++++++++++++---------
 1 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/audio/control.c b/audio/control.c
index 2f240f0..0a333db 100644
--- a/audio/control.c
+++ b/audio/control.c
@@ -471,6 +471,28 @@ static sdp_record_t *avrcp_tg_record(void)
 	return record;
 }
 
+/**
+ *	get_company_id:
+ *
+ *	Get three-byte Company_ID from incoming AVRCP message
+ */
+static uint32_t get_company_id(const uint8_t cid[3])
+{
+	return cid[0] << 16 | cid[1] << 8 | cid[2];
+}
+
+/**
+ *	set_company_id:
+ *
+ *	Set three-byte Company_ID into outgoing AVRCP message
+ */
+static void set_company_id(uint8_t cid[3], const uint32_t cid_in)
+{
+	cid[0] = cid_in >> 16;
+	cid[1] = cid_in >> 8;
+	cid[2] = cid_in;
+}
+
 static int send_event(int fd, uint16_t type, uint16_t code, int32_t value)
 {
 	struct uinput_event event;
@@ -748,9 +770,7 @@ static int avctp_send_event(struct control *control, uint8_t id, void *data)
 	avrcp->subunit_type = SUBUNIT_PANEL;
 	avrcp->opcode = OP_VENDORDEP;
 
-	pdu->company_id[0] = IEEEID_BTSIG >> 16;
-	pdu->company_id[1] = (IEEEID_BTSIG >> 8) & 0xFF;
-	pdu->company_id[2] = IEEEID_BTSIG & 0xFF;
+	set_company_id(pdu->company_id, IEEEID_BTSIG);
 
 	pdu->pdu_id = AVRCP_REGISTER_NOTIFICATION;
 	pdu->params[0] = id;
@@ -995,9 +1015,8 @@ static int avrcp_handle_get_capabilities(struct control *control,
 	switch (pdu->params[0]) {
 	case CAP_COMPANY_ID:
 		for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
-			pdu->params[2 + i * 3] = company_ids[i] >> 16;
-			pdu->params[3 + i * 3] = (company_ids[i] >> 8) & 0xFF;
-			pdu->params[4 + i * 3] = company_ids[i] & 0xFF;
+			set_company_id(&pdu->params[2 + i * 3],
+							company_ids[i]);
 		}
 
 		pdu->params_len = htons(2 + (3 * G_N_ELEMENTS(company_ids)));
@@ -1378,9 +1397,8 @@ static int handle_vendordep_pdu(struct control *control,
 					int operand_count)
 {
 	struct avrcp_spec_avc_pdu *pdu = (void *) avrcp + AVRCP_HEADER_LENGTH;
-	uint32_t company_id = (pdu->company_id[0] << 16) |
-				(pdu->company_id[1] << 8) |
-				(pdu->company_id[2]);
+	uint32_t company_id = get_company_id(pdu->company_id);
+
 	int len;
 
 	if (company_id != IEEEID_BTSIG ||
-- 
1.7.6.1

[PATCH 4/5] avrcp: limit avrcp packet size to the MTU of AV/C

[Lucas De Marchi]

AVRCP is an extension of AV/C spec which has a limit of 512 bytes. The
only place where it can exceed this value is in the response to
GetElementAttributes command.

Now we simply don't add the attributes that would overflow the available
buffer space.
---
 audio/control.c |   51 +++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/audio/control.c b/audio/control.c
index 0a333db..c162a62 100644
--- a/audio/control.c
+++ b/audio/control.c
@@ -266,6 +266,10 @@ struct avrcp_spec_avc_pdu {
 #error "Unknown byte order"
 #endif
 
+#define AVC_MTU			512
+#define AVRCP_PDU_MTU		(AVC_MTU - AVRCP_HEADER_LENGTH \
+					- AVRCP_SPECAVCPDU_HEADER_LENGTH)
+
 struct avctp_state_callback {
 	avctp_state_cb cb;
 	void *user_data;
@@ -869,7 +873,8 @@ static void mp_set_playback_status(struct control *control, uint8_t status,
  * attribute, -ENOENT is returned.
  */
 static int mp_get_media_attribute(struct media_player *mp,
-						uint32_t id, uint8_t *buf)
+						uint32_t id, uint8_t *buf,
+						uint16_t maxlen)
 {
 	struct media_info_elem {
 		uint32_t id;
@@ -882,21 +887,33 @@ static int mp_get_media_attribute(struct media_player *mp,
 	uint16_t len;
 	char valstr[20];
 
+	if (maxlen < sizeof(struct media_info_elem))
+		return -ENOBUFS;
+
+	/* Subtract the size of elem header from the available space */
+	maxlen -= sizeof(struct media_info_elem);
+
 	switch (id) {
 	case MEDIA_INFO_TITLE:
-		if (mi->title) {
-			len = strlen(mi->title);
-			memcpy(elem->val, mi->title, len);
-		} else {
+		if (mi->title == NULL) {
 			len = 0;
+			break;
 		}
 
+		len = strlen(mi->title);
+		if (len > maxlen)
+			return -ENOBUFS;
+
+		memcpy(elem->val, mi->title, len);
 		break;
 	case MEDIA_INFO_ARTIST:
 		if (mi->artist == NULL)
 			return -ENOENT;
 
 		len = strlen(mi->artist);
+		if (len > maxlen)
+			return -ENOBUFS;
+
 		memcpy(elem->val, mi->artist, len);
 		break;
 	case MEDIA_INFO_ALBUM:
@@ -904,6 +921,9 @@ static int mp_get_media_attribute(struct media_player *mp,
 			return -ENOENT;
 
 		len = strlen(mi->album);
+		if (len > maxlen)
+			return -ENOBUFS;
+
 		memcpy(elem->val, mi->album, len);
 		break;
 	case MEDIA_INFO_GENRE:
@@ -911,6 +931,9 @@ static int mp_get_media_attribute(struct media_player *mp,
 			return -ENOENT;
 
 		len = strlen(mi->genre);
+		if (len > maxlen)
+			return -ENOBUFS;
+
 		memcpy(elem->val, mi->genre, len);
 		break;
 
@@ -920,6 +943,9 @@ static int mp_get_media_attribute(struct media_player *mp,
 
 		snprintf(valstr, 20, "%u", mi->track);
 		len = strlen(valstr);
+		if (len > maxlen)
+			return -ENOBUFS;
+
 		memcpy(elem->val, valstr, len);
 		break;
 	case MEDIA_INFO_N_TRACKS:
@@ -928,6 +954,9 @@ static int mp_get_media_attribute(struct media_player *mp,
 
 		snprintf(valstr, 20, "%u", mi->ntracks);
 		len = strlen(valstr);
+		if (len > maxlen)
+			return -ENOBUFS;
+
 		memcpy(elem->val, valstr, len);
 		break;
 	case MEDIA_INFO_PLAYING_TIME:
@@ -936,6 +965,9 @@ static int mp_get_media_attribute(struct media_player *mp,
 
 		snprintf(valstr, 20, "%u", mi->track_len);
 		len = strlen(valstr);
+		if (len > maxlen)
+			return -ENOBUFS;
+
 		memcpy(elem->val, valstr, len);
 		break;
 	default:
@@ -1115,8 +1147,6 @@ static int avrcp_handle_get_element_attributes(struct control *control,
 	pos = 1; /* Keep track of current position in reponse */
 	nattr = pdu->params[8];
 
-	if (!control->mp)
-		goto done;
 
 	if (!nattr) {
 		/*
@@ -1125,7 +1155,8 @@ static int avrcp_handle_get_element_attributes(struct control *control,
 		 */
 		for (i = 1; i < MEDIA_INFO_LAST; i++) {
 			size = mp_get_media_attribute(control->mp, i,
-							&pdu->params[pos]);
+					&pdu->params[pos], AVRCP_PDU_MTU - pos);
+
 
 			if (size > 0) {
 				len++;
@@ -1141,7 +1172,8 @@ static int avrcp_handle_get_element_attributes(struct control *control,
 			uint32_t attr = ntohl(attr_ids[i]);
 
 			size = mp_get_media_attribute(control->mp, attr,
-							&pdu->params[pos]);
+							&pdu->params[pos],
+							AVRCP_PDU_MTU - pos);
 
 			if (size > 0) {
 				len++;
@@ -1155,7 +1187,6 @@ static int avrcp_handle_get_element_attributes(struct control *control,
 			goto err;
 	}
 
-done:
 	pdu->params[0] = len;
 	pdu->params_len = htons(pos);
 
-- 
1.7.6.1

[PATCH 5/5] avrcp: implement pdu continuation request and abort

[Lucas De Marchi]

GetElementAttributes is the only one that can possibly overflow the MTU
of AVC. When this command is received, if the response is larger than
the MTU split the messages and queue them for later processing.
---
 audio/control.c |  318 ++++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 231 insertions(+), 87 deletions(-)

diff --git a/audio/control.c b/audio/control.c
index c162a62..adb5113 100644
--- a/audio/control.c
+++ b/audio/control.c
@@ -128,6 +128,8 @@
 #define AVRCP_GET_ELEMENT_ATTRIBUTES	0x20
 #define AVRCP_GET_PLAY_STATUS		0x30
 #define AVRCP_REGISTER_NOTIFICATION	0x31
+#define AVRCP_REQUEST_CONTINUING	0x40
+#define AVRCP_ABORT_CONTINUING		0x41
 
 /* Notification events */
 #define AVRCP_EVENT_PLAYBACK_STATUS_CHANGED		0x01
@@ -300,6 +302,7 @@ struct media_player {
 
 	struct media_info mi;
 	GTimer *timer;
+	GQueue *pending_pdus;
 };
 
 struct control {
@@ -866,15 +869,17 @@ static void mp_set_playback_status(struct control *control, uint8_t status,
  * Copy media_info field to a buffer, intended to be used in a response to
  * GetElementAttributes message.
  *
- * It assumes there's enough space in the buffer and on success it returns the
- * size written.
+ * If there isn't enough space in the buffer, the available space is filled,
+ * the remainder (both header and value) is allocated in @param remainder and
+ * its size is written to remainder_len.
  *
- * If @param id is not valid, -EINVAL is returned. If there's no such media
- * attribute, -ENOENT is returned.
+ * On success, it returns the size written. Otherwise it returns -EINVAL if
+ * @param id is not valid or -ENOENT if there's no such media attribute.
  */
 static int mp_get_media_attribute(struct media_player *mp,
-						uint32_t id, uint8_t *buf,
-						uint16_t maxlen)
+					uint32_t id, uint8_t *buf,
+					uint16_t maxlen, uint8_t **remainder,
+					int *remainder_len)
 {
 	struct media_info_elem {
 		uint32_t id;
@@ -883,102 +888,105 @@ static int mp_get_media_attribute(struct media_player *mp,
 		uint8_t val[];
 	};
 	const struct media_info *mi = &mp->mi;
-	struct media_info_elem *elem = (void *)buf;
+	struct media_info_elem *elem;
 	uint16_t len;
 	char valstr[20];
+	char *valp;
+	int ret;
 
-	if (maxlen < sizeof(struct media_info_elem))
-		return -ENOBUFS;
+	assert(remainder != NULL);
+	assert(remainder_len != NULL);
 
-	/* Subtract the size of elem header from the available space */
-	maxlen -= sizeof(struct media_info_elem);
+	DBG("id=%u buf=%p maxlen=%u", id, buf, maxlen);
 
 	switch (id) {
 	case MEDIA_INFO_TITLE:
-		if (mi->title == NULL) {
-			len = 0;
-			break;
-		}
+		valp = mi->title;
 
-		len = strlen(mi->title);
-		if (len > maxlen)
-			return -ENOBUFS;
+		if (mi->title == NULL)
+			len = 0;
 
-		memcpy(elem->val, mi->title, len);
 		break;
 	case MEDIA_INFO_ARTIST:
 		if (mi->artist == NULL)
 			return -ENOENT;
 
-		len = strlen(mi->artist);
-		if (len > maxlen)
-			return -ENOBUFS;
-
-		memcpy(elem->val, mi->artist, len);
+		valp = mi->artist;
 		break;
 	case MEDIA_INFO_ALBUM:
 		if (mi->album == NULL)
 			return -ENOENT;
 
-		len = strlen(mi->album);
-		if (len > maxlen)
-			return -ENOBUFS;
-
-		memcpy(elem->val, mi->album, len);
+		valp = mi->album;
 		break;
 	case MEDIA_INFO_GENRE:
 		if (mi->genre == NULL)
 			return -ENOENT;
 
-		len = strlen(mi->genre);
-		if (len > maxlen)
-			return -ENOBUFS;
-
-		memcpy(elem->val, mi->genre, len);
+		valp = mi->genre;
 		break;
-
 	case MEDIA_INFO_TRACK:
 		if (!mi->track)
 			return -ENOENT;
 
 		snprintf(valstr, 20, "%u", mi->track);
-		len = strlen(valstr);
-		if (len > maxlen)
-			return -ENOBUFS;
-
-		memcpy(elem->val, valstr, len);
+		valp = valstr;
 		break;
 	case MEDIA_INFO_N_TRACKS:
 		if (!mi->ntracks)
 			return -ENOENT;
 
 		snprintf(valstr, 20, "%u", mi->ntracks);
-		len = strlen(valstr);
-		if (len > maxlen)
-			return -ENOBUFS;
-
-		memcpy(elem->val, valstr, len);
+		valp = valstr;
 		break;
 	case MEDIA_INFO_PLAYING_TIME:
 		if (mi->track_len == 0xFFFFFFFF)
 			return -ENOENT;
 
 		snprintf(valstr, 20, "%u", mi->track_len);
-		len = strlen(valstr);
-		if (len > maxlen)
-			return -ENOBUFS;
-
-		memcpy(elem->val, valstr, len);
+		valp = valstr;
 		break;
 	default:
 		return -EINVAL;
 	}
 
+	if (valp)
+		len = strlen(valp);
+
+	if (maxlen < sizeof(struct media_info_elem)) {
+		/*
+		 * There isn't space even for the header: put both header and
+		 * value in remainder.
+		 */
+		*remainder_len = sizeof(struct media_info_elem) + len;
+		elem = g_malloc(*remainder_len);
+		*remainder = (uint8_t *) elem;
+		memcpy(elem->val, valp, len);
+		ret = 0;
+	} else {
+		/* Put at least header on current PDU */
+		elem = (struct media_info_elem *) buf;
+		maxlen -= sizeof(struct media_info_elem);
+
+		if (maxlen < len) {
+			/* Split value between current PDU and remainder. */
+			memcpy(elem->val, valp, maxlen);
+			*remainder_len = len - maxlen;
+			*remainder = g_memdup(valp + maxlen, *remainder_len);
+			ret = sizeof(struct media_info_elem) + maxlen;
+		} else {
+			/* Header and value fit in current PDU */
+			memcpy(elem->val, valp, len);
+			*remainder = NULL;
+			ret = sizeof(struct media_info_elem) + len;
+		}
+	}
+
 	elem->id = htonl(id);
 	elem->charset = htons(0x6A); /* Always use UTF-8 */
 	elem->len = htons(len);
 
-	return sizeof(struct media_info_elem) + len;
+	return ret;
 }
 
 static void mp_set_attribute(struct media_player *mp,
@@ -1130,69 +1138,122 @@ err:
 	return -EINVAL;
 }
 
+static struct avrcp_spec_avc_pdu *avrcp_split_pdu(struct media_player *mp,
+					struct avrcp_spec_avc_pdu *last_pdu,
+					uint8_t *remainder, int remainder_len,
+					uint16_t *pos)
+{
+	struct avrcp_spec_avc_pdu *pdu = last_pdu;
+	uint16_t len;
+
+	assert(remainder != NULL);
+	assert(pos != NULL);
+
+	if (!mp->pending_pdus)
+		mp->pending_pdus = g_queue_new();
+
+	for (len = *pos; remainder_len; remainder_len -= len) {
+		/* Close last pdu - keep host endiannes */
+		pdu->params_len = len;
+
+		/* Create a new PDU */
+		pdu = g_malloc(AVRCP_PDU_MTU + AVRCP_SPECAVCPDU_HEADER_LENGTH);
+
+		memcpy(pdu, last_pdu, sizeof(struct avrcp_spec_avc_pdu));
+		g_queue_push_tail(mp->pending_pdus, pdu);
+
+		if (remainder_len > AVRCP_PDU_MTU)
+			len = AVRCP_PDU_MTU;
+		else
+			len = remainder_len;
+
+		memcpy(&pdu->params[0], remainder, len);
+	}
+
+	*pos = len;
+
+	return pdu;
+}
+
 static int avrcp_handle_get_element_attributes(struct control *control,
-						struct avrcp_spec_avc_pdu *pdu)
+					struct avrcp_spec_avc_pdu *first_pdu)
 {
-	uint16_t len = ntohs(pdu->params_len);
+	struct avrcp_spec_avc_pdu *pdu = first_pdu;
+	struct media_player *mp = control->mp;
 	uint64_t *identifier = (void *) &pdu->params[0];
+	uint32_t attr_ids[MEDIA_INFO_LAST];
+	uint16_t len = ntohs(pdu->params_len);
 	uint16_t pos;
 	uint8_t nattr;
-	int size;
 	unsigned int i;
 
-	if (len < 8 || *identifier != 0 || !control->mp)
+	if (len < 8 || *identifier != 0 || !mp)
 		goto err;
 
-	len = 0;
-	pos = 1; /* Keep track of current position in reponse */
 	nattr = pdu->params[8];
 
+	DBG("nattr=%u", nattr);
 
-	if (!nattr) {
-		/*
-		 * Return all available information, at least
-		 * title must be returned.
-		 */
-		for (i = 1; i < MEDIA_INFO_LAST; i++) {
-			size = mp_get_media_attribute(control->mp, i,
-					&pdu->params[pos], AVRCP_PDU_MTU - pos);
-
+	/* Copy the requested attribute ids to our private vector */
+	if (nattr) {
+		uint32_t *attr = (uint32_t *) &pdu->params[9];
 
-			if (size > 0) {
-				len++;
-				pos += size;
-			}
+		if (nattr > MEDIA_INFO_LAST) {
+			error("Got number of attributes %u > %u",
+						nattr, MEDIA_INFO_LAST);
+			nattr = MEDIA_INFO_LAST;
 		}
+
+		for (i = 0; attr < (uint32_t *)&pdu->params[9] +
+						nattr * sizeof(uint32_t);
+						attr++, i++)
+			attr_ids[i] = ntohl(*attr);
 	} else {
-		uint32_t *attr_ids;
+		nattr = MEDIA_INFO_LAST - 1;
 
-		attr_ids = g_memdup(&pdu->params[9], sizeof(uint32_t) * nattr);
+		for (i = 1; i < MEDIA_INFO_LAST; i++)
+			attr_ids[i - 1] = i;
+	}
 
-		for (i = 0; i < nattr; i++) {
-			uint32_t attr = ntohl(attr_ids[i]);
+	for (i = 0, len = 0, pos = 1; i < nattr; i++) {
+		uint8_t *remainder;
+		int size, remainder_len;
 
-			size = mp_get_media_attribute(control->mp, attr,
-							&pdu->params[pos],
-							AVRCP_PDU_MTU - pos);
+		size = mp_get_media_attribute(control->mp, attr_ids[i],
+						&pdu->params[pos],
+						AVRCP_PDU_MTU - pos,
+						&remainder, &remainder_len);
 
-			if (size > 0) {
-				len++;
-				pos += size;
-			}
+		if (size < 0)
+			continue;
+
+		pos += size;
+		len++;
+
+		if (remainder) {
+			pdu = avrcp_split_pdu(control->mp, pdu, remainder,
+							remainder_len, &pos);
+			g_free(remainder);
 		}
+	}
 
-		g_free(attr_ids);
+	if (!len)
+		goto err;
+
+	/* Close last pdu - keep host endiannes */
+	pdu->params_len = pos;
 
-		if (!len)
-			goto err;
+	if (first_pdu != pdu) {
+		first_pdu->packet_type = AVCTP_PACKET_START;
+		pos = first_pdu->params_len;
 	}
 
-	pdu->params[0] = len;
-	pdu->params_len = htons(pos);
+	first_pdu->params[0] = len;
+	first_pdu->params_len = htons(first_pdu->params_len);
 
 	return pos;
 err:
-	pdu->params[0] = E_INVALID_PARAM;
+	first_pdu->params[0] = E_INVALID_PARAM;
 	return -EINVAL;
 }
 
@@ -1421,6 +1482,56 @@ err:
 	return -EINVAL;
 }
 
+static void cancel_pending_pdus(struct media_player *mp)
+{
+	struct avrcp_spec_avc_pdu *pdu;
+
+	if (!mp || !mp->pending_pdus)
+		return;
+
+	DBG("%u PDUs cancelled.", g_queue_get_length(mp->pending_pdus));
+
+	while ((pdu = g_queue_pop_head(mp->pending_pdus)))
+		g_free(pdu);
+
+	g_queue_free(mp->pending_pdus);
+	mp->pending_pdus = NULL;
+}
+
+static int avrcp_handle_request_continuing(struct control *control,
+						struct avrcp_spec_avc_pdu *pdu)
+{
+	struct media_player *mp = control->mp;
+	struct avrcp_spec_avc_pdu *saved_pdu;
+	int len;
+
+	if (!mp || !mp->pending_pdus) {
+		pdu->params[0] = E_INVALID_PARAM;
+		return -EINVAL;
+	}
+
+	DBG("");
+
+	saved_pdu = g_queue_pop_head(mp->pending_pdus);
+
+	len = saved_pdu->params_len;
+	memcpy(pdu, saved_pdu, sizeof(struct avrcp_spec_avc_pdu) + len);
+	pdu->params_len = htons(len);
+
+	g_free(saved_pdu);
+
+	/* Mark the pdu as the last one and destroy queue */
+	if (g_queue_is_empty(mp->pending_pdus)) {
+		pdu->packet_type = AVCTP_PACKET_END;
+		g_queue_free(mp->pending_pdus);
+		mp->pending_pdus = NULL;
+	} else {
+		pdu->packet_type = AVCTP_PACKET_CONTINUE;
+	}
+
+	return len;
+}
+
 /* handle vendordep pdu inside an avctp packet */
 static int handle_vendordep_pdu(struct control *control,
 					struct avctp_header *avctp,
@@ -1446,6 +1557,10 @@ static int handle_vendordep_pdu(struct control *control,
 		goto err_metadata;
 	}
 
+	/* We have to cancel pending pdus before processing new message */
+	if (pdu->pdu_id != AVRCP_REQUEST_CONTINUING)
+		cancel_pending_pdus(control->mp);
+
 	switch (pdu->pdu_id) {
 	case AVRCP_GET_CAPABILITIES:
 		if (avrcp->code != CTYPE_STATUS) {
@@ -1601,6 +1716,34 @@ static int handle_vendordep_pdu(struct control *control,
 		avrcp->code = CTYPE_INTERIM;
 
 		break;
+	case AVRCP_REQUEST_CONTINUING:
+		/*
+		 * This case is special as it's the only one that does not
+		 * cancel pending pdus, but instead it returns one of them.
+		 * All the others will break the switch, but this returns
+		 * directly.
+		 */
+
+		len = avrcp_handle_request_continuing(control, pdu);
+
+		if (len < 0)
+			goto err_metadata;
+
+		avrcp->code = CTYPE_STABLE;
+
+		return AVRCP_HEADER_LENGTH + AVRCP_SPECAVCPDU_HEADER_LENGTH
+								+ len;
+	case AVRCP_ABORT_CONTINUING:
+		if (avrcp->code != CTYPE_CONTROL) {
+			pdu->params[0] = E_INVALID_COMMAND;
+			goto err_metadata;
+		}
+
+		pdu->params_len = 0;
+		avrcp->code = CTYPE_STABLE;
+		len = 0;
+
+		break;
 	default:
 		/* Invalid pdu_id */
 		pdu->params[0] = E_INVALID_COMMAND;
@@ -2537,6 +2680,7 @@ static void mp_path_unregister(void *data)
 	DBG("Unregistered interface %s on path %s",
 		MEDIA_PLAYER_INTERFACE, dev->path);
 
+	cancel_pending_pdus(mp);
 	g_timer_destroy(mp->timer);
 	g_free(mp);
 	control->mp = NULL;
-- 
1.7.6.1

Re: [PATCH 3/5] avrcp: get/set three-byte company-id

[Johan Hedberg]

Hi,

On Thu, Sep 08, 2011, Lucas De Marchi wrote:
> +static void set_company_id(uint8_t cid[3], const uint32_t cid_in)
> +{
> +	cid[0] = cid_in >> 16;
> +	cid[1] = cid_in >> 8;
> +	cid[2] = cid_in;
> +}

You seem to have forgotten the "& 0xff" part here. Other than that I
didn't seen any obvious issues in the patch set.

Johan

Re: [PATCH 3/5] avrcp: get/set three-byte company-id

[David Stockwell]

A short cut: the output is a contiguous array of uint_8, so masking with 
0xFF should be unnecessary.  Would this be required for non-x86 (including 
x86_64) architectures?

DS

-----Original Message----- 
From: Johan Hedberg
Sent: Friday, September 09, 2011 5:41 AM
To: Lucas De Marchi
Cc: linux-bluetooth@vger.kernel.org ; David Stockwell
Subject: Re: [PATCH 3/5] avrcp: get/set three-byte company-id

Hi,

On Thu, Sep 08, 2011, Lucas De Marchi wrote:
> +static void set_company_id(uint8_t cid[3], const uint32_t cid_in)
> +{
> + cid[0] = cid_in >> 16;
> + cid[1] = cid_in >> 8;
> + cid[2] = cid_in;
> +}

You seem to have forgotten the "& 0xff" part here. Other than that I
didn't seen any obvious issues in the patch set.

Johan 

Re: [PATCH 3/5] avrcp: get/set three-byte company-id

[Johan Hedberg]

Hi David,

On Fri, Sep 09, 2011, David Stockwell wrote:
> A short cut: the output is a contiguous array of uint_8, so masking
> with 0xFF should be unnecessary.  Would this be required for non-x86
> (including x86_64) architectures?

Firstly, please don't top-post on this mailing list.

If you don't do "& 0xff" and the value you're trying to assign to cid[n]
is greater than 0xff isn't the result of the integer overflow that
follows essentially the same as if you had done "% (0xff + 1)" (which is
not what you want in this case).

Johan

Re: [PATCH 3/5] avrcp: get/set three-byte company-id

[Johan Hedberg]

Hi,

On Fri, Sep 09, 2011, Johan Hedberg wrote:
> On Fri, Sep 09, 2011, David Stockwell wrote:
> > A short cut: the output is a contiguous array of uint_8, so masking
> > with 0xFF should be unnecessary.  Would this be required for non-x86
> > (including x86_64) architectures?
> 
> Firstly, please don't top-post on this mailing list.
> 
> If you don't do "& 0xff" and the value you're trying to assign to cid[n]
> is greater than 0xff isn't the result of the integer overflow that
> follows essentially the same as if you had done "% (0xff + 1)" (which is
> not what you want in this case).

Actually those two operations result in the same value (I even did a
quick test program for this). So you're right :)

Johan

Re: [PATCH 3/5] avrcp: get/set three-byte company-id

[Lucas De Marchi]

Hi Johan,

On Fri, Sep 9, 2011 at 9:46 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi,
>
> On Fri, Sep 09, 2011, Johan Hedberg wrote:
>> On Fri, Sep 09, 2011, David Stockwell wrote:
>> > A short cut: the output is a contiguous array of uint_8, so masking
>> > with 0xFF should be unnecessary.  Would this be required for non-x86
>> > (including x86_64) architectures?
>>
>> Firstly, please don't top-post on this mailing list.
>>
>> If you don't do "& 0xff" and the value you're trying to assign to cid[n]
>> is greater than 0xff isn't the result of the integer overflow that
>> follows essentially the same as if you had done "% (0xff + 1)" (which is
>> not what you want in this case).
>
> Actually those two operations result in the same value (I even did a
> quick test program for this). So you're right :)

I thought the same as you when I saw this code, but after testing the
same code was being generated too.

If we had the following:

#define BAAA 0x204050
uint8_t c[3];

c[1] = BAAA >> 16;

Then it would give us "warning: large integer implicitly truncated to
unsigned type [-Woverflow]"


But with "uint32_t baaa = 0x204050" there's no such warning and the
compiler does the right thing.



Lucas De Marchi

Re: [PATCH 3/5] avrcp: get/set three-byte company-id

[David Stockwell]

Hi Johan

-----Original Message----- 
From: Johan Hedberg
Sent: Friday, September 09, 2011 7:35 AM
To: David Stockwell
Cc: Lucas De Marchi ; linux-bluetooth@vger.kernel.org
Subject: Re: [PATCH 3/5] avrcp: get/set three-byte company-id

Hi David,

On Fri, Sep 09, 2011, David Stockwell wrote:
> A short cut: the output is a contiguous array of uint_8, so masking
> with 0xFF should be unnecessary.  Would this be required for non-x86
> (including x86_64) architectures?

Firstly, please don't top-post on this mailing list.

+++++ Sorry...I am quite entrenched in the .Net world by day, so I forgot. 
Then did not send an apology afterwards.

If you don't do "& 0xff" and the value you're trying to assign to cid[n]
is greater than 0xff isn't the result of the integer overflow that
follows essentially the same as if you had done "% (0xff + 1)" (which is
not what you want in this case).

+++++ I saw the rest of the discussion...cheers.

Johan
--
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