[PATCH 0/7] cec: various fixes

[PATCH 0/7] cec: various fixes

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

While writing more and better compliance tests for the CEC API I found
a number of corner cases that were not handled correctly.

For the most part it was fields that weren't initialized correctly, but
but more serious was the fact that there was no limit to the number of
pending transmits. The other important bug was that the result of a
non-blocking transmit was never returned to userspace, so applications
using this method would have no way of knowing that the transmit was
successful or not.

The last patch adds MONITOR_ALL mode to vivid.

Regards,

	Hans

Hans Verkuil (7):
  cec: CEC_RECEIVE overwrote the timeout field
  cec: clear all status fields before transmit and always fill in
    sequence
  cec: don't set fh to NULL in CEC_TRANSMIT
  cec: zero unused msg part after msg->len
  cec: limit the size of the transmit queue
  cec: fix test for unconfigured adapter in main message loop
  vivid: support monitor all mode

 drivers/media/platform/vivid/vivid-cec.c | 44 ++++++++---------------
 drivers/staging/media/cec/cec-adap.c     | 61 +++++++++++++++++++++-----------
 drivers/staging/media/cec/cec-api.c      | 12 +++----
 include/media/cec.h                      | 19 +++++++---
 4 files changed, 75 insertions(+), 61 deletions(-)

-- 
2.8.1

[PATCH 1/7] cec: CEC_RECEIVE overwrote the timeout field

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

When CEC_RECEIVE returns a message the original timeout field
was overwritten. Restore the timeout field.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/staging/media/cec/cec-api.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/media/cec/cec-api.c b/drivers/staging/media/cec/cec-api.c
index 9151b1f..879f7d9 100644
--- a/drivers/staging/media/cec/cec-api.c
+++ b/drivers/staging/media/cec/cec-api.c
@@ -209,6 +209,7 @@ static long cec_transmit(struct cec_adapter *adap, struct cec_fh *fh,
 /* Called by CEC_RECEIVE: wait for a message to arrive */
 static int cec_receive_msg(struct cec_fh *fh, struct cec_msg *msg, bool block)
 {
+	u32 timeout = msg->timeout;
 	int res;
 
 	do {
@@ -225,6 +226,8 @@ static int cec_receive_msg(struct cec_fh *fh, struct cec_msg *msg, bool block)
 			kfree(entry);
 			fh->queued_msgs--;
 			mutex_unlock(&fh->lock);
+			/* restore original timeout value */
+			msg->timeout = timeout;
 			return 0;
 		}
 
-- 
2.8.1

[PATCH 2/7] cec: clear all status fields before transmit and always fill in sequence

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Before transmitting a message clear all status fields and always fill
in the sequence number. Make sure the sequence number is never 0.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/staging/media/cec/cec-adap.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/cec/cec-adap.c b/drivers/staging/media/cec/cec-adap.c
index 4d86a6c..2b34c0f 100644
--- a/drivers/staging/media/cec/cec-adap.c
+++ b/drivers/staging/media/cec/cec-adap.c
@@ -574,6 +574,19 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
 	unsigned int timeout;
 	int res = 0;
 
+	msg->rx_ts = 0;
+	msg->tx_ts = 0;
+	msg->rx_status = 0;
+	msg->tx_status = 0;
+	msg->tx_arb_lost_cnt = 0;
+	msg->tx_nack_cnt = 0;
+	msg->tx_low_drive_cnt = 0;
+	msg->tx_error_cnt = 0;
+	msg->flags = 0;
+	msg->sequence = ++adap->sequence;
+	if (!msg->sequence)
+		msg->sequence = ++adap->sequence;
+
 	if (msg->reply && msg->timeout == 0) {
 		/* Make sure the timeout isn't 0. */
 		msg->timeout = 1000;
@@ -640,14 +653,6 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
 		dprintk(2, "cec_transmit_msg: %*ph%s\n",
 			msg->len, msg->msg, !block ? " (nb)" : "");
 
-	msg->rx_ts = 0;
-	msg->tx_ts = 0;
-	msg->rx_status = 0;
-	msg->tx_status = 0;
-	msg->tx_arb_lost_cnt = 0;
-	msg->tx_nack_cnt = 0;
-	msg->tx_low_drive_cnt = 0;
-	msg->tx_error_cnt = 0;
 	data->msg = *msg;
 	data->fh = fh;
 	data->adap = adap;
@@ -673,7 +678,6 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
 	init_completion(&data->c);
 	INIT_DELAYED_WORK(&data->work, cec_wait_timeout);
 
-	data->msg.sequence = adap->sequence++;
 	if (fh)
 		list_add_tail(&data->xfer_list, &fh->xfer_list);
 	list_add_tail(&data->list, &adap->transmit_queue);
-- 
2.8.1

[PATCH 3/7] cec: don't set fh to NULL in CEC_TRANSMIT

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The filehandle was set to NULL when in non-blocking mode or when
no reply is needed.

This is wrong: the filehandle is needed in non-blocking mode to ensure
that the result of the transmit can be obtained through CEC_RECEIVE.

And the 'reply' check was also incorrect since it should have checked the
timeout field (the reply can be 0). In any case, when in blocking mode
there is no need to set the fh to NULL either.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/staging/media/cec/cec-api.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/cec/cec-api.c b/drivers/staging/media/cec/cec-api.c
index 879f7d9..559f650 100644
--- a/drivers/staging/media/cec/cec-api.c
+++ b/drivers/staging/media/cec/cec-api.c
@@ -189,15 +189,12 @@ static long cec_transmit(struct cec_adapter *adap, struct cec_fh *fh,
 	if (copy_from_user(&msg, parg, sizeof(msg)))
 		return -EFAULT;
 	mutex_lock(&adap->lock);
-	if (!adap->is_configured) {
+	if (!adap->is_configured)
 		err = -ENONET;
-	} else if (cec_is_busy(adap, fh)) {
+	else if (cec_is_busy(adap, fh))
 		err = -EBUSY;
-	} else {
-		if (!block || !msg.reply)
-			fh = NULL;
+	else
 		err = cec_transmit_msg_fh(adap, &msg, fh, block);
-	}
 	mutex_unlock(&adap->lock);
 	if (err)
 		return err;
-- 
2.8.1

[PATCH 4/7] cec: zero unused msg part after msg->len

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Ensure that the unused part of the msg array is zeroed. This is
required by CEC 2.0 when receiving shorter messages than expected.
In that case the remaining bytes are assumed to be 0.

There are no such CEC messages yet, but it is required to be future proof.

And since we're doing it for received messages, do it for transmitted
messages as well. It's unambiguous this way.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/staging/media/cec/cec-adap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/media/cec/cec-adap.c b/drivers/staging/media/cec/cec-adap.c
index 2b34c0f..cd39a9a 100644
--- a/drivers/staging/media/cec/cec-adap.c
+++ b/drivers/staging/media/cec/cec-adap.c
@@ -601,6 +601,7 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
 		dprintk(1, "cec_transmit_msg: can't reply for poll msg\n");
 		return -EINVAL;
 	}
+	memset(msg->msg + msg->len, 0, sizeof(msg->msg) - msg->len);
 	if (msg->len == 1) {
 		if (cec_msg_initiator(msg) != 0xf ||
 		    cec_msg_destination(msg) == 0xf) {
@@ -771,6 +772,7 @@ void cec_received_msg(struct cec_adapter *adap, struct cec_msg *msg)
 	msg->tx_status = 0;
 	msg->tx_ts = 0;
 	msg->flags = 0;
+	memset(msg->msg + msg->len, 0, sizeof(msg->msg) - msg->len);
 
 	mutex_lock(&adap->lock);
 	dprintk(2, "cec_received_msg: %*ph\n", msg->len, msg->msg);
-- 
2.8.1

[PATCH 5/7] cec: limit the size of the transmit queue

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The size of the transmit queue was unlimited, which meant that
in non-blocking mode you could flood the CEC adapter with messages
to be transmitted.

Limit this to 18 messages.

Also print the number of pending transmits and the timeout value
in the status debugfs file.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/staging/media/cec/cec-adap.c | 33 +++++++++++++++++++++++----------
 include/media/cec.h                  | 19 ++++++++++++++-----
 2 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/media/cec/cec-adap.c b/drivers/staging/media/cec/cec-adap.c
index cd39a9a..159a893 100644
--- a/drivers/staging/media/cec/cec-adap.c
+++ b/drivers/staging/media/cec/cec-adap.c
@@ -156,10 +156,10 @@ static void cec_queue_msg_fh(struct cec_fh *fh, const struct cec_msg *msg)
 	list_add_tail(&entry->list, &fh->msgs);
 
 	/*
-	 * if the queue now has more than CEC_MAX_MSG_QUEUE_SZ
+	 * if the queue now has more than CEC_MAX_MSG_RX_QUEUE_SZ
 	 * messages, drop the oldest one and send a lost message event.
 	 */
-	if (fh->queued_msgs == CEC_MAX_MSG_QUEUE_SZ) {
+	if (fh->queued_msgs == CEC_MAX_MSG_RX_QUEUE_SZ) {
 		list_del(&entry->list);
 		goto lost_msgs;
 	}
@@ -278,10 +278,13 @@ static void cec_data_cancel(struct cec_data *data)
 	 * It's either the current transmit, or it is a pending
 	 * transmit. Take the appropriate action to clear it.
 	 */
-	if (data->adap->transmitting == data)
+	if (data->adap->transmitting == data) {
 		data->adap->transmitting = NULL;
-	else
+	} else {
 		list_del_init(&data->list);
+		if (!(data->msg.tx_status & CEC_TX_STATUS_OK))
+			data->adap->transmit_queue_sz--;
+	}
 
 	/* Mark it as an error */
 	data->msg.tx_ts = ktime_get_ns();
@@ -405,6 +408,7 @@ int cec_thread_func(void *_adap)
 		data = list_first_entry(&adap->transmit_queue,
 					struct cec_data, list);
 		list_del_init(&data->list);
+		adap->transmit_queue_sz--;
 		/* Make this the current transmitting message */
 		adap->transmitting = data;
 
@@ -498,6 +502,7 @@ void cec_transmit_done(struct cec_adapter *adap, u8 status, u8 arb_lost_cnt,
 		data->attempts--;
 		/* Add the message in front of the transmit queue */
 		list_add(&data->list, &adap->transmit_queue);
+		adap->transmit_queue_sz++;
 		goto wake_thread;
 	}
 
@@ -638,6 +643,9 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
 	if (!adap->is_configured && !adap->is_configuring)
 		return -ENONET;
 
+	if (adap->transmit_queue_sz >= CEC_MAX_MSG_TX_QUEUE_SZ)
+		return -EBUSY;
+
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
@@ -682,6 +690,7 @@ int cec_transmit_msg_fh(struct cec_adapter *adap, struct cec_msg *msg,
 	if (fh)
 		list_add_tail(&data->xfer_list, &fh->xfer_list);
 	list_add_tail(&data->list, &adap->transmit_queue);
+	adap->transmit_queue_sz++;
 	if (!adap->transmitting)
 		wake_up_interruptible(&adap->kthread_waitq);
 
@@ -1621,15 +1630,19 @@ int cec_adap_status(struct seq_file *file, void *priv)
 			   adap->monitor_all_cnt);
 	data = adap->transmitting;
 	if (data)
-		seq_printf(file, "transmitting message: %*ph (reply: %02x)\n",
-			   data->msg.len, data->msg.msg, data->msg.reply);
+		seq_printf(file, "transmitting message: %*ph (reply: %02x, timeout: %ums)\n",
+			   data->msg.len, data->msg.msg, data->msg.reply,
+			   data->msg.timeout);
+	seq_printf(file, "pending transmits: %u\n", adap->transmit_queue_sz);
 	list_for_each_entry(data, &adap->transmit_queue, list) {
-		seq_printf(file, "queued tx message: %*ph (reply: %02x)\n",
-			   data->msg.len, data->msg.msg, data->msg.reply);
+		seq_printf(file, "queued tx message: %*ph (reply: %02x, timeout: %ums)\n",
+			   data->msg.len, data->msg.msg, data->msg.reply,
+			   data->msg.timeout);
 	}
 	list_for_each_entry(data, &adap->wait_queue, list) {
-		seq_printf(file, "message waiting for reply: %*ph (reply: %02x)\n",
-			   data->msg.len, data->msg.msg, data->msg.reply);
+		seq_printf(file, "message waiting for reply: %*ph (reply: %02x, timeout: %ums)\n",
+			   data->msg.len, data->msg.msg, data->msg.reply,
+			   data->msg.timeout);
 	}
 
 	call_void_op(adap, adap_status, file);
diff --git a/include/media/cec.h b/include/media/cec.h
index 9a791c0..dc7854b 100644
--- a/include/media/cec.h
+++ b/include/media/cec.h
@@ -126,12 +126,20 @@ struct cec_adap_ops {
  * With a transfer rate of at most 36 bytes per second this makes 18 messages
  * per second worst case.
  *
- * We queue at most 3 seconds worth of messages. The CEC specification requires
- * that messages are replied to within a second, so 3 seconds should give more
- * than enough margin. Since most messages are actually more than 2 bytes, this
- * is in practice a lot more than 3 seconds.
+ * We queue at most 3 seconds worth of received messages. The CEC specification
+ * requires that messages are replied to within a second, so 3 seconds should
+ * give more than enough margin. Since most messages are actually more than 2
+ * bytes, this is in practice a lot more than 3 seconds.
  */
-#define CEC_MAX_MSG_QUEUE_SZ		(18 * 3)
+#define CEC_MAX_MSG_RX_QUEUE_SZ		(18 * 3)
+
+/*
+ * The transmit queue is limited to 1 second worth of messages (worst case).
+ * Messages can be transmitted by userspace and kernel space. But for both it
+ * makes no sense to have a lot of messages queued up. One second seems
+ * reasonable.
+ */
+#define CEC_MAX_MSG_TX_QUEUE_SZ		(18 * 1)
 
 struct cec_adapter {
 	struct module *owner;
@@ -141,6 +149,7 @@ struct cec_adapter {
 	struct rc_dev *rc;
 
 	struct list_head transmit_queue;
+	unsigned int transmit_queue_sz;
 	struct list_head wait_queue;
 	struct cec_data *transmitting;
 
-- 
2.8.1

[PATCH 6/7] cec: fix test for unconfigured adapter in main message loop

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The main message loop checks if the physical address was valid, and if
not it is assumed that the adapter had been unconfigured.

However, this check is no longer correct, instead it should check
that both adap->is_configured and adap->is_configuring are false.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/staging/media/cec/cec-adap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/cec/cec-adap.c b/drivers/staging/media/cec/cec-adap.c
index 159a893..6fa3db5 100644
--- a/drivers/staging/media/cec/cec-adap.c
+++ b/drivers/staging/media/cec/cec-adap.c
@@ -332,7 +332,7 @@ int cec_thread_func(void *_adap)
 			 */
 			err = wait_event_interruptible_timeout(adap->kthread_waitq,
 				kthread_should_stop() ||
-				adap->phys_addr == CEC_PHYS_ADDR_INVALID ||
+				(!adap->is_configured && !adap->is_configuring) ||
 				(!adap->transmitting &&
 				 !list_empty(&adap->transmit_queue)),
 				msecs_to_jiffies(CEC_XFER_TIMEOUT_MS));
@@ -347,7 +347,7 @@ int cec_thread_func(void *_adap)
 
 		mutex_lock(&adap->lock);
 
-		if (adap->phys_addr == CEC_PHYS_ADDR_INVALID ||
+		if ((!adap->is_configured && !adap->is_configuring) ||
 		    kthread_should_stop()) {
 			/*
 			 * If the adapter is disabled, or we're asked to stop,
-- 
2.8.1

[PATCH 7/7] vivid: support monitor all mode

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Just pass the transmitted CEC message to all CEC adapters.
This implements the Monitor All mode for vivid.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/vivid/vivid-cec.c | 44 +++++++++++---------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-cec.c b/drivers/media/platform/vivid/vivid-cec.c
index b5714fa..66aa729 100644
--- a/drivers/media/platform/vivid/vivid-cec.c
+++ b/drivers/media/platform/vivid/vivid-cec.c
@@ -40,19 +40,18 @@ void vivid_cec_bus_free_work(struct vivid_dev *dev)
 	spin_unlock(&dev->cec_slock);
 }
 
-static struct cec_adapter *vivid_cec_find_dest_adap(struct vivid_dev *dev,
-						    struct cec_adapter *adap,
-						    u8 dest)
+static bool vivid_cec_find_dest_adap(struct vivid_dev *dev,
+				     struct cec_adapter *adap, u8 dest)
 {
 	unsigned int i;
 
 	if (dest >= 0xf)
-		return NULL;
+		return false;
 
 	if (adap != dev->cec_rx_adap && dev->cec_rx_adap &&
 	    dev->cec_rx_adap->is_configured &&
 	    cec_has_log_addr(dev->cec_rx_adap, dest))
-		return dev->cec_rx_adap;
+		return true;
 
 	for (i = 0; i < MAX_OUTPUTS && dev->cec_tx_adap[i]; i++) {
 		if (adap == dev->cec_tx_adap[i])
@@ -60,9 +59,9 @@ static struct cec_adapter *vivid_cec_find_dest_adap(struct vivid_dev *dev,
 		if (!dev->cec_tx_adap[i]->is_configured)
 			continue;
 		if (cec_has_log_addr(dev->cec_tx_adap[i], dest))
-			return dev->cec_tx_adap[i];
+			return true;
 	}
-	return NULL;
+	return false;
 }
 
 static void vivid_cec_xfer_done_worker(struct work_struct *work)
@@ -71,18 +70,14 @@ static void vivid_cec_xfer_done_worker(struct work_struct *work)
 		container_of(work, struct vivid_cec_work, work.work);
 	struct vivid_dev *dev = cw->dev;
 	struct cec_adapter *adap = cw->adap;
-	bool is_poll = cw->msg.len == 1;
 	u8 dest = cec_msg_destination(&cw->msg);
-	struct cec_adapter *dest_adap = NULL;
 	bool valid_dest;
 	unsigned int i;
 
 	valid_dest = cec_msg_is_broadcast(&cw->msg);
-	if (!valid_dest) {
-		dest_adap = vivid_cec_find_dest_adap(dev, adap, dest);
-		if (dest_adap)
-			valid_dest = true;
-	}
+	if (!valid_dest)
+		valid_dest = vivid_cec_find_dest_adap(dev, adap, dest);
+
 	cw->tx_status = valid_dest ? CEC_TX_STATUS_OK : CEC_TX_STATUS_NACK;
 	spin_lock(&dev->cec_slock);
 	dev->cec_xfer_time_jiffies = 0;
@@ -91,21 +86,12 @@ static void vivid_cec_xfer_done_worker(struct work_struct *work)
 	spin_unlock(&dev->cec_slock);
 	cec_transmit_done(cw->adap, cw->tx_status, 0, valid_dest ? 0 : 1, 0, 0);
 
-	if (!is_poll && dest_adap) {
-		/* Directed message */
-		cec_received_msg(dest_adap, &cw->msg);
-	} else if (!is_poll && valid_dest) {
-		/* Broadcast message */
-		if (adap != dev->cec_rx_adap &&
-		    dev->cec_rx_adap->log_addrs.log_addr_mask)
-			cec_received_msg(dev->cec_rx_adap, &cw->msg);
-		for (i = 0; i < MAX_OUTPUTS && dev->cec_tx_adap[i]; i++) {
-			if (adap == dev->cec_tx_adap[i] ||
-			    !dev->cec_tx_adap[i]->log_addrs.log_addr_mask)
-				continue;
+	/* Broadcast message */
+	if (adap != dev->cec_rx_adap)
+		cec_received_msg(dev->cec_rx_adap, &cw->msg);
+	for (i = 0; i < MAX_OUTPUTS && dev->cec_tx_adap[i]; i++)
+		if (adap != dev->cec_tx_adap[i])
 			cec_received_msg(dev->cec_tx_adap[i], &cw->msg);
-		}
-	}
 	kfree(cw);
 }
 
@@ -245,7 +231,7 @@ struct cec_adapter *vivid_cec_alloc_adap(struct vivid_dev *dev,
 {
 	char name[sizeof(dev->vid_out_dev.name) + 2];
 	u32 caps = CEC_CAP_TRANSMIT | CEC_CAP_LOG_ADDRS |
-		CEC_CAP_PASSTHROUGH | CEC_CAP_RC;
+		CEC_CAP_PASSTHROUGH | CEC_CAP_RC | CEC_CAP_MONITOR_ALL;
 
 	snprintf(name, sizeof(name), "%s%d",
 		 is_source ? dev->vid_out_dev.name : dev->vid_cap_dev.name,
-- 
2.8.1

[PATCH 8/7] cec: poll should check if there is room in the tx queue

[Hans Verkuil]

For POLLOUT poll only checked if the adapter was configured, not
if there was room in the transmit queue. Add that check.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/staging/media/cec/cec-api.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/cec/cec-api.c b/drivers/staging/media/cec/cec-api.c
index 559f650..7be7615 100644
--- a/drivers/staging/media/cec/cec-api.c
+++ b/drivers/staging/media/cec/cec-api.c
@@ -52,7 +52,8 @@ static unsigned int cec_poll(struct file *filp,
 	if (!devnode->registered)
 		return POLLERR | POLLHUP;
 	mutex_lock(&adap->lock);
-	if (adap->is_configured)
+	if (adap->is_configured &&
+	    adap->transmit_queue_sz < CEC_MAX_MSG_TX_QUEUE_SZ)
 		res |= POLLOUT | POLLWRNORM;
 	if (fh->queued_msgs)
 		res |= POLLIN | POLLRDNORM;
-- 
2.8.1