[PATCH 00/10] pulse8-cec: improvements and many fixes

[PATCH 00/10] pulse8-cec: improvements and many fixes

[Hans Verkuil]

This series assumes this v5.5 patch has been applied first:

https://patchwork.linuxtv.org/patch/60641/

This series improves the code and fixes many issues that would
only appear during stress testing.

Part of it was trial-and-error based on observing how the USB device
acted in corner cases. But after this series was applied this driver
now passes the 'cec-compliance --test-adapter' tests.

The stress testing was done by connecting two Pulse-Eight devices
together and running this in one shell:

	cec-ctl --playback
	cec-ctl -d1 --tv -p0

	while true; do date; cec-ctl -s -p f.f.f.f; sleep 2; cec-ctl -s -p 1.0.0.0; sleep 2; done

This in another:

	while true ; do cec-ctl -s -t0 --image-view-on -v; done

And this in a third:

	while true; do date; cec-ctl -s -d1 --tv; sleep .3; done

This tests continuously unconfiguring and reconfiguring the logical
addresses for both Pulse-Eights, and continuously sending the Image View On
message.

Regards,

	Hans

Hans Verkuil (10):
  pulse8-cec: improve debugging
  pulse8-cec: reorganize function order
  pulse8-cec: locking improvements
  pulse8-cec: add 2nd debug level
  pulse8-cec: set tx_done_status for transmit_done status
  pulse8-cec: move the transmit to a workqueue
  pulse8-cec: queue received messages in an array
  pulse8-cec: use adap_free callback
  pulse8-cec: schedule next ping after current ping finished
  pulse8-cec: log when a CEC message is received

 drivers/media/usb/pulse8-cec/pulse8-cec.c | 769 +++++++++++++---------
 1 file changed, 457 insertions(+), 312 deletions(-)

-- 
2.23.0

[PATCH 01/10] pulse8-cec: improve debugging

[Hans Verkuil]

Add and use pulse8_msgname() to show the message codes as a
human readable text instead of a number.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/pulse8-cec/pulse8-cec.c | 65 +++++++++++++++++++++--
 1 file changed, 60 insertions(+), 5 deletions(-)

diff --git a/drivers/media/usb/pulse8-cec/pulse8-cec.c b/drivers/media/usb/pulse8-cec/pulse8-cec.c
index 59609556d969..ea9d42d19bfd 100644
--- a/drivers/media/usb/pulse8-cec/pulse8-cec.c
+++ b/drivers/media/usb/pulse8-cec/pulse8-cec.c
@@ -100,6 +100,61 @@ enum pulse8_msgcodes {
 	MSGCODE_FRAME_ACK = 0x40,
 };
 
+static const char * const pulse8_msgnames[] = {
+	"NOTHING",
+	"PING",
+	"TIMEOUT_ERROR",
+	"HIGH_ERROR",
+	"LOW_ERROR",
+	"FRAME_START",
+	"FRAME_DATA",
+	"RECEIVE_FAILED",
+	"COMMAND_ACCEPTED",
+	"COMMAND_REJECTED",
+	"SET_ACK_MASK",
+	"TRANSMIT",
+	"TRANSMIT_EOM",
+	"TRANSMIT_IDLETIME",
+	"TRANSMIT_ACK_POLARITY",
+	"TRANSMIT_LINE_TIMEOUT",
+	"TRANSMIT_SUCCEEDED",
+	"TRANSMIT_FAILED_LINE",
+	"TRANSMIT_FAILED_ACK",
+	"TRANSMIT_FAILED_TIMEOUT_DATA",
+	"TRANSMIT_FAILED_TIMEOUT_LINE",
+	"FIRMWARE_VERSION",
+	"START_BOOTLOADER",
+	"GET_BUILDDATE",
+	"SET_CONTROLLED",
+	"GET_AUTO_ENABLED",
+	"SET_AUTO_ENABLED",
+	"GET_DEFAULT_LOGICAL_ADDRESS",
+	"SET_DEFAULT_LOGICAL_ADDRESS",
+	"GET_LOGICAL_ADDRESS_MASK",
+	"SET_LOGICAL_ADDRESS_MASK",
+	"GET_PHYSICAL_ADDRESS",
+	"SET_PHYSICAL_ADDRESS",
+	"GET_DEVICE_TYPE",
+	"SET_DEVICE_TYPE",
+	"GET_HDMI_VERSION",
+	"SET_HDMI_VERSION",
+	"GET_OSD_NAME",
+	"SET_OSD_NAME",
+	"WRITE_EEPROM",
+	"GET_ADAPTER_TYPE",
+	"SET_ACTIVE_SOURCE",
+};
+
+static const char *pulse8_msgname(u8 cmd)
+{
+	static char unknown_msg[5];
+
+	if ((cmd & 0x3f) < ARRAY_SIZE(pulse8_msgnames))
+		return pulse8_msgnames[cmd & 0x3f];
+	snprintf(unknown_msg, sizeof(unknown_msg), "0x%02x", cmd);
+	return unknown_msg;
+}
+
 #define MSGSTART	0xff
 #define MSGEND		0xfe
 #define MSGESC		0xfd
@@ -178,8 +233,8 @@ static irqreturn_t pulse8_interrupt(struct serio *serio, unsigned char data,
 		u8 msgcode = pulse8->buf[0];
 
 		if (debug)
-			dev_info(pulse8->dev, "received: %*ph\n",
-				 pulse8->idx, pulse8->buf);
+			dev_info(pulse8->dev, "received %s: %*ph\n",
+				 pulse8_msgname(msgcode), pulse8->idx, pulse8->buf);
 		switch (msgcode & 0x3f) {
 		case MSGCODE_FRAME_START:
 			msg->len = 1;
@@ -278,7 +333,7 @@ static int pulse8_send_and_wait_once(struct pulse8 *pulse8,
 {
 	int err;
 
-	/*dev_info(pulse8->dev, "transmit: %*ph\n", cmd_len, cmd);*/
+	/* dev_info(pulse8->dev, "transmit %s: %*ph\n", pulse8_msgname(cmd[0]), cmd_len, cmd); */
 	init_completion(&pulse8->cmd_done);
 
 	err = pulse8_send(pulse8->serio, cmd, cmd_len);
@@ -294,8 +349,8 @@ static int pulse8_send_and_wait_once(struct pulse8 *pulse8,
 		return -ENOTTY;
 	if (response &&
 	    ((pulse8->data[0] & 0x3f) != response || pulse8->len < size + 1)) {
-		dev_info(pulse8->dev, "transmit: failed %02x\n",
-			 pulse8->data[0] & 0x3f);
+		dev_info(pulse8->dev, "transmit %s: failed %s\n",
+			 pulse8_msgname(cmd[0]), pulse8_msgname(pulse8->data[0]));
 		return -EIO;
 	}
 	return 0;
-- 
2.23.0

[PATCH 02/10] pulse8-cec: reorganize function order

[Hans Verkuil]

Reorganize the order of the functions in the source, going from
low-level to high-level.

No functional changes were made.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/pulse8-cec/pulse8-cec.c | 504 +++++++++++-----------
 1 file changed, 248 insertions(+), 256 deletions(-)

diff --git a/drivers/media/usb/pulse8-cec/pulse8-cec.c b/drivers/media/usb/pulse8-cec/pulse8-cec.c
index ea9d42d19bfd..1637141907d0 100644
--- a/drivers/media/usb/pulse8-cec/pulse8-cec.c
+++ b/drivers/media/usb/pulse8-cec/pulse8-cec.c
@@ -187,7 +187,81 @@ struct pulse8 {
 	bool autonomous;
 };
 
-static void pulse8_ping_eeprom_work_handler(struct work_struct *work);
+static int pulse8_send(struct serio *serio, const u8 *command, u8 cmd_len)
+{
+	int err = 0;
+
+	err = serio_write(serio, MSGSTART);
+	if (err)
+		return err;
+	for (; !err && cmd_len; command++, cmd_len--) {
+		if (*command >= MSGESC) {
+			err = serio_write(serio, MSGESC);
+			if (!err)
+				err = serio_write(serio, *command - MSGOFFSET);
+		} else {
+			err = serio_write(serio, *command);
+		}
+	}
+	if (!err)
+		err = serio_write(serio, MSGEND);
+
+	return err;
+}
+
+static int pulse8_send_and_wait_once(struct pulse8 *pulse8,
+				     const u8 *cmd, u8 cmd_len,
+				     u8 response, u8 size)
+{
+	int err;
+
+	/* dev_info(pulse8->dev, "transmit %s: %*ph\n", pulse8_msgname(cmd[0]), cmd_len, cmd); */
+	init_completion(&pulse8->cmd_done);
+
+	err = pulse8_send(pulse8->serio, cmd, cmd_len);
+	if (err)
+		return err;
+
+	if (!wait_for_completion_timeout(&pulse8->cmd_done, HZ))
+		return -ETIMEDOUT;
+	if ((pulse8->data[0] & 0x3f) == MSGCODE_COMMAND_REJECTED &&
+	    cmd[0] != MSGCODE_SET_CONTROLLED &&
+	    cmd[0] != MSGCODE_SET_AUTO_ENABLED &&
+	    cmd[0] != MSGCODE_GET_BUILDDATE)
+		return -ENOTTY;
+	if (response &&
+	    ((pulse8->data[0] & 0x3f) != response || pulse8->len < size + 1)) {
+		dev_info(pulse8->dev, "transmit %s: failed %s\n",
+			 pulse8_msgname(cmd[0]), pulse8_msgname(pulse8->data[0]));
+		return -EIO;
+	}
+	return 0;
+}
+
+static int pulse8_send_and_wait(struct pulse8 *pulse8,
+				const u8 *cmd, u8 cmd_len, u8 response, u8 size)
+{
+	u8 cmd_sc[2];
+	int err;
+
+	mutex_lock(&pulse8->write_lock);
+	err = pulse8_send_and_wait_once(pulse8, cmd, cmd_len, response, size);
+
+	if (err == -ENOTTY) {
+		cmd_sc[0] = MSGCODE_SET_CONTROLLED;
+		cmd_sc[1] = 1;
+		err = pulse8_send_and_wait_once(pulse8, cmd_sc, 2,
+						MSGCODE_COMMAND_ACCEPTED, 1);
+		if (err)
+			goto unlock;
+		err = pulse8_send_and_wait_once(pulse8, cmd, cmd_len,
+						response, size);
+	}
+
+unlock:
+	mutex_unlock(&pulse8->write_lock);
+	return err == -ENOTTY ? -EIO : err;
+}
 
 static void pulse8_irq_work_handler(struct work_struct *work)
 {
@@ -293,228 +367,6 @@ static irqreturn_t pulse8_interrupt(struct serio *serio, unsigned char data,
 	return IRQ_HANDLED;
 }
 
-static void pulse8_disconnect(struct serio *serio)
-{
-	struct pulse8 *pulse8 = serio_get_drvdata(serio);
-
-	cec_unregister_adapter(pulse8->adap);
-	cancel_delayed_work_sync(&pulse8->ping_eeprom_work);
-	dev_info(&serio->dev, "disconnected\n");
-	serio_close(serio);
-	serio_set_drvdata(serio, NULL);
-	kfree(pulse8);
-}
-
-static int pulse8_send(struct serio *serio, const u8 *command, u8 cmd_len)
-{
-	int err = 0;
-
-	err = serio_write(serio, MSGSTART);
-	if (err)
-		return err;
-	for (; !err && cmd_len; command++, cmd_len--) {
-		if (*command >= MSGESC) {
-			err = serio_write(serio, MSGESC);
-			if (!err)
-				err = serio_write(serio, *command - MSGOFFSET);
-		} else {
-			err = serio_write(serio, *command);
-		}
-	}
-	if (!err)
-		err = serio_write(serio, MSGEND);
-
-	return err;
-}
-
-static int pulse8_send_and_wait_once(struct pulse8 *pulse8,
-				     const u8 *cmd, u8 cmd_len,
-				     u8 response, u8 size)
-{
-	int err;
-
-	/* dev_info(pulse8->dev, "transmit %s: %*ph\n", pulse8_msgname(cmd[0]), cmd_len, cmd); */
-	init_completion(&pulse8->cmd_done);
-
-	err = pulse8_send(pulse8->serio, cmd, cmd_len);
-	if (err)
-		return err;
-
-	if (!wait_for_completion_timeout(&pulse8->cmd_done, HZ))
-		return -ETIMEDOUT;
-	if ((pulse8->data[0] & 0x3f) == MSGCODE_COMMAND_REJECTED &&
-	    cmd[0] != MSGCODE_SET_CONTROLLED &&
-	    cmd[0] != MSGCODE_SET_AUTO_ENABLED &&
-	    cmd[0] != MSGCODE_GET_BUILDDATE)
-		return -ENOTTY;
-	if (response &&
-	    ((pulse8->data[0] & 0x3f) != response || pulse8->len < size + 1)) {
-		dev_info(pulse8->dev, "transmit %s: failed %s\n",
-			 pulse8_msgname(cmd[0]), pulse8_msgname(pulse8->data[0]));
-		return -EIO;
-	}
-	return 0;
-}
-
-static int pulse8_send_and_wait(struct pulse8 *pulse8,
-				const u8 *cmd, u8 cmd_len, u8 response, u8 size)
-{
-	u8 cmd_sc[2];
-	int err;
-
-	mutex_lock(&pulse8->write_lock);
-	err = pulse8_send_and_wait_once(pulse8, cmd, cmd_len, response, size);
-
-	if (err == -ENOTTY) {
-		cmd_sc[0] = MSGCODE_SET_CONTROLLED;
-		cmd_sc[1] = 1;
-		err = pulse8_send_and_wait_once(pulse8, cmd_sc, 2,
-						MSGCODE_COMMAND_ACCEPTED, 1);
-		if (err)
-			goto unlock;
-		err = pulse8_send_and_wait_once(pulse8, cmd, cmd_len,
-						response, size);
-	}
-
-unlock:
-	mutex_unlock(&pulse8->write_lock);
-	return err == -ENOTTY ? -EIO : err;
-}
-
-static int pulse8_setup(struct pulse8 *pulse8, struct serio *serio,
-			struct cec_log_addrs *log_addrs, u16 *pa)
-{
-	u8 *data = pulse8->data + 1;
-	u8 cmd[2];
-	int err;
-	struct tm tm;
-	time64_t date;
-
-	pulse8->vers = 0;
-
-	cmd[0] = MSGCODE_FIRMWARE_VERSION;
-	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 2);
-	if (err)
-		return err;
-	pulse8->vers = (data[0] << 8) | data[1];
-	dev_info(pulse8->dev, "Firmware version %04x\n", pulse8->vers);
-	if (pulse8->vers < 2) {
-		*pa = CEC_PHYS_ADDR_INVALID;
-		return 0;
-	}
-
-	cmd[0] = MSGCODE_GET_BUILDDATE;
-	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 4);
-	if (err)
-		return err;
-	date = (data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3];
-	time64_to_tm(date, 0, &tm);
-	dev_info(pulse8->dev, "Firmware build date %04ld.%02d.%02d %02d:%02d:%02d\n",
-		 tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
-		 tm.tm_hour, tm.tm_min, tm.tm_sec);
-
-	dev_dbg(pulse8->dev, "Persistent config:\n");
-	cmd[0] = MSGCODE_GET_AUTO_ENABLED;
-	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 1);
-	if (err)
-		return err;
-	pulse8->autonomous = data[0];
-	dev_dbg(pulse8->dev, "Autonomous mode: %s",
-		data[0] ? "on" : "off");
-
-	cmd[0] = MSGCODE_GET_DEVICE_TYPE;
-	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 1);
-	if (err)
-		return err;
-	log_addrs->primary_device_type[0] = data[0];
-	dev_dbg(pulse8->dev, "Primary device type: %d\n", data[0]);
-	switch (log_addrs->primary_device_type[0]) {
-	case CEC_OP_PRIM_DEVTYPE_TV:
-		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_TV;
-		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_TV;
-		break;
-	case CEC_OP_PRIM_DEVTYPE_RECORD:
-		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_RECORD;
-		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_RECORD;
-		break;
-	case CEC_OP_PRIM_DEVTYPE_TUNER:
-		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_TUNER;
-		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_TUNER;
-		break;
-	case CEC_OP_PRIM_DEVTYPE_PLAYBACK:
-		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_PLAYBACK;
-		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_PLAYBACK;
-		break;
-	case CEC_OP_PRIM_DEVTYPE_AUDIOSYSTEM:
-		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_PLAYBACK;
-		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_AUDIOSYSTEM;
-		break;
-	case CEC_OP_PRIM_DEVTYPE_SWITCH:
-		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_UNREGISTERED;
-		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_SWITCH;
-		break;
-	case CEC_OP_PRIM_DEVTYPE_PROCESSOR:
-		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_SPECIFIC;
-		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_SWITCH;
-		break;
-	default:
-		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_UNREGISTERED;
-		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_SWITCH;
-		dev_info(pulse8->dev, "Unknown Primary Device Type: %d\n",
-			 log_addrs->primary_device_type[0]);
-		break;
-	}
-
-	cmd[0] = MSGCODE_GET_LOGICAL_ADDRESS_MASK;
-	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 2);
-	if (err)
-		return err;
-	log_addrs->log_addr_mask = (data[0] << 8) | data[1];
-	dev_dbg(pulse8->dev, "Logical address ACK mask: %x\n",
-		log_addrs->log_addr_mask);
-	if (log_addrs->log_addr_mask)
-		log_addrs->num_log_addrs = 1;
-
-	cmd[0] = MSGCODE_GET_PHYSICAL_ADDRESS;
-	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 1);
-	if (err)
-		return err;
-	*pa = (data[0] << 8) | data[1];
-	dev_dbg(pulse8->dev, "Physical address: %x.%x.%x.%x\n",
-		cec_phys_addr_exp(*pa));
-
-	cmd[0] = MSGCODE_GET_HDMI_VERSION;
-	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 1);
-	if (err)
-		return err;
-	log_addrs->cec_version = data[0];
-	dev_dbg(pulse8->dev, "CEC version: %d\n", log_addrs->cec_version);
-
-	cmd[0] = MSGCODE_GET_OSD_NAME;
-	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 0);
-	if (err)
-		return err;
-	strscpy(log_addrs->osd_name, data, sizeof(log_addrs->osd_name));
-	dev_dbg(pulse8->dev, "OSD name: %s\n", log_addrs->osd_name);
-
-	return 0;
-}
-
-static int pulse8_apply_persistent_config(struct pulse8 *pulse8,
-					  struct cec_log_addrs *log_addrs,
-					  u16 pa)
-{
-	int err;
-
-	err = cec_s_log_addrs(pulse8->adap, log_addrs, false);
-	if (err)
-		return err;
-
-	cec_s_phys_addr(pulse8->adap, pa, false);
-
-	return 0;
-}
-
 static int pulse8_cec_adap_enable(struct cec_adapter *adap, bool enable)
 {
 	struct pulse8 *pulse8 = cec_get_drvdata(adap);
@@ -688,18 +540,185 @@ static int pulse8_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 	return err;
 }
 
-static int pulse8_received(struct cec_adapter *adap, struct cec_msg *msg)
-{
-	return -ENOMSG;
-}
-
 static const struct cec_adap_ops pulse8_cec_adap_ops = {
 	.adap_enable = pulse8_cec_adap_enable,
 	.adap_log_addr = pulse8_cec_adap_log_addr,
 	.adap_transmit = pulse8_cec_adap_transmit,
-	.received = pulse8_received,
 };
 
+static void pulse8_disconnect(struct serio *serio)
+{
+	struct pulse8 *pulse8 = serio_get_drvdata(serio);
+
+	cec_unregister_adapter(pulse8->adap);
+	cancel_delayed_work_sync(&pulse8->ping_eeprom_work);
+	dev_info(&serio->dev, "disconnected\n");
+	serio_close(serio);
+	serio_set_drvdata(serio, NULL);
+	kfree(pulse8);
+}
+
+static int pulse8_setup(struct pulse8 *pulse8, struct serio *serio,
+			struct cec_log_addrs *log_addrs, u16 *pa)
+{
+	u8 *data = pulse8->data + 1;
+	u8 cmd[2];
+	int err;
+	struct tm tm;
+	time64_t date;
+
+	pulse8->vers = 0;
+
+	cmd[0] = MSGCODE_FIRMWARE_VERSION;
+	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 2);
+	if (err)
+		return err;
+	pulse8->vers = (data[0] << 8) | data[1];
+	dev_info(pulse8->dev, "Firmware version %04x\n", pulse8->vers);
+	if (pulse8->vers < 2) {
+		*pa = CEC_PHYS_ADDR_INVALID;
+		return 0;
+	}
+
+	cmd[0] = MSGCODE_GET_BUILDDATE;
+	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 4);
+	if (err)
+		return err;
+	date = (data[0] << 24) | (data[1] << 16) | (data[2] << 8) | data[3];
+	time64_to_tm(date, 0, &tm);
+	dev_info(pulse8->dev, "Firmware build date %04ld.%02d.%02d %02d:%02d:%02d\n",
+		 tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,
+		 tm.tm_hour, tm.tm_min, tm.tm_sec);
+
+	dev_dbg(pulse8->dev, "Persistent config:\n");
+	cmd[0] = MSGCODE_GET_AUTO_ENABLED;
+	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 1);
+	if (err)
+		return err;
+	pulse8->autonomous = data[0];
+	dev_dbg(pulse8->dev, "Autonomous mode: %s",
+		data[0] ? "on" : "off");
+
+	cmd[0] = MSGCODE_GET_DEVICE_TYPE;
+	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 1);
+	if (err)
+		return err;
+	log_addrs->primary_device_type[0] = data[0];
+	dev_dbg(pulse8->dev, "Primary device type: %d\n", data[0]);
+	switch (log_addrs->primary_device_type[0]) {
+	case CEC_OP_PRIM_DEVTYPE_TV:
+		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_TV;
+		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_TV;
+		break;
+	case CEC_OP_PRIM_DEVTYPE_RECORD:
+		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_RECORD;
+		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_RECORD;
+		break;
+	case CEC_OP_PRIM_DEVTYPE_TUNER:
+		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_TUNER;
+		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_TUNER;
+		break;
+	case CEC_OP_PRIM_DEVTYPE_PLAYBACK:
+		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_PLAYBACK;
+		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_PLAYBACK;
+		break;
+	case CEC_OP_PRIM_DEVTYPE_AUDIOSYSTEM:
+		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_PLAYBACK;
+		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_AUDIOSYSTEM;
+		break;
+	case CEC_OP_PRIM_DEVTYPE_SWITCH:
+		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_UNREGISTERED;
+		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_SWITCH;
+		break;
+	case CEC_OP_PRIM_DEVTYPE_PROCESSOR:
+		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_SPECIFIC;
+		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_SWITCH;
+		break;
+	default:
+		log_addrs->log_addr_type[0] = CEC_LOG_ADDR_TYPE_UNREGISTERED;
+		log_addrs->all_device_types[0] = CEC_OP_ALL_DEVTYPE_SWITCH;
+		dev_info(pulse8->dev, "Unknown Primary Device Type: %d\n",
+			 log_addrs->primary_device_type[0]);
+		break;
+	}
+
+	cmd[0] = MSGCODE_GET_LOGICAL_ADDRESS_MASK;
+	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 2);
+	if (err)
+		return err;
+	log_addrs->log_addr_mask = (data[0] << 8) | data[1];
+	dev_dbg(pulse8->dev, "Logical address ACK mask: %x\n",
+		log_addrs->log_addr_mask);
+	if (log_addrs->log_addr_mask)
+		log_addrs->num_log_addrs = 1;
+
+	cmd[0] = MSGCODE_GET_PHYSICAL_ADDRESS;
+	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 1);
+	if (err)
+		return err;
+	*pa = (data[0] << 8) | data[1];
+	dev_dbg(pulse8->dev, "Physical address: %x.%x.%x.%x\n",
+		cec_phys_addr_exp(*pa));
+
+	cmd[0] = MSGCODE_GET_HDMI_VERSION;
+	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 1);
+	if (err)
+		return err;
+	log_addrs->cec_version = data[0];
+	dev_dbg(pulse8->dev, "CEC version: %d\n", log_addrs->cec_version);
+
+	cmd[0] = MSGCODE_GET_OSD_NAME;
+	err = pulse8_send_and_wait(pulse8, cmd, 1, cmd[0], 0);
+	if (err)
+		return err;
+	strscpy(log_addrs->osd_name, data, sizeof(log_addrs->osd_name));
+	dev_dbg(pulse8->dev, "OSD name: %s\n", log_addrs->osd_name);
+
+	return 0;
+}
+
+static int pulse8_apply_persistent_config(struct pulse8 *pulse8,
+					  struct cec_log_addrs *log_addrs,
+					  u16 pa)
+{
+	int err;
+
+	err = cec_s_log_addrs(pulse8->adap, log_addrs, false);
+	if (err)
+		return err;
+
+	cec_s_phys_addr(pulse8->adap, pa, false);
+
+	return 0;
+}
+
+static void pulse8_ping_eeprom_work_handler(struct work_struct *work)
+{
+	struct pulse8 *pulse8 =
+		container_of(work, struct pulse8, ping_eeprom_work.work);
+	u8 cmd;
+
+	schedule_delayed_work(&pulse8->ping_eeprom_work, PING_PERIOD);
+	cmd = MSGCODE_PING;
+	pulse8_send_and_wait(pulse8, &cmd, 1,
+			     MSGCODE_COMMAND_ACCEPTED, 0);
+
+	if (pulse8->vers < 2)
+		return;
+
+	mutex_lock(&pulse8->config_lock);
+	if (pulse8->config_pending && persistent_config) {
+		dev_dbg(pulse8->dev, "writing pending config to EEPROM\n");
+		cmd = MSGCODE_WRITE_EEPROM;
+		if (pulse8_send_and_wait(pulse8, &cmd, 1,
+					 MSGCODE_COMMAND_ACCEPTED, 0))
+			dev_info(pulse8->dev, "failed to write pending config to EEPROM\n");
+		else
+			pulse8->config_pending = false;
+	}
+	mutex_unlock(&pulse8->config_lock);
+}
+
 static int pulse8_connect(struct serio *serio, struct serio_driver *drv)
 {
 	u32 caps = CEC_CAP_DEFAULTS | CEC_CAP_PHYS_ADDR | CEC_CAP_MONITOR_ALL;
@@ -764,33 +783,6 @@ static int pulse8_connect(struct serio *serio, struct serio_driver *drv)
 	return err;
 }
 
-static void pulse8_ping_eeprom_work_handler(struct work_struct *work)
-{
-	struct pulse8 *pulse8 =
-		container_of(work, struct pulse8, ping_eeprom_work.work);
-	u8 cmd;
-
-	schedule_delayed_work(&pulse8->ping_eeprom_work, PING_PERIOD);
-	cmd = MSGCODE_PING;
-	pulse8_send_and_wait(pulse8, &cmd, 1,
-			     MSGCODE_COMMAND_ACCEPTED, 0);
-
-	if (pulse8->vers < 2)
-		return;
-
-	mutex_lock(&pulse8->config_lock);
-	if (pulse8->config_pending && persistent_config) {
-		dev_dbg(pulse8->dev, "writing pending config to EEPROM\n");
-		cmd = MSGCODE_WRITE_EEPROM;
-		if (pulse8_send_and_wait(pulse8, &cmd, 1,
-					 MSGCODE_COMMAND_ACCEPTED, 0))
-			dev_info(pulse8->dev, "failed to write pending config to EEPROM\n");
-		else
-			pulse8->config_pending = false;
-	}
-	mutex_unlock(&pulse8->config_lock);
-}
-
 static const struct serio_device_id pulse8_serio_ids[] = {
 	{
 		.type	= SERIO_RS232,
-- 
2.23.0

[PATCH 03/10] pulse8-cec: locking improvements

[Hans Verkuil]

Drop the write_lock, rename config_lock to plain lock since this
now locks access to the adapter. Use 'lock' when transmitting
a message, ensuring that nothing interferes with the transmit.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/pulse8-cec/pulse8-cec.c | 47 +++++++++++++----------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/media/usb/pulse8-cec/pulse8-cec.c b/drivers/media/usb/pulse8-cec/pulse8-cec.c
index 1637141907d0..40880cb79f9a 100644
--- a/drivers/media/usb/pulse8-cec/pulse8-cec.c
+++ b/drivers/media/usb/pulse8-cec/pulse8-cec.c
@@ -180,8 +180,8 @@ struct pulse8 {
 	unsigned int idx;
 	bool escape;
 	bool started;
-	struct mutex config_lock;
-	struct mutex write_lock;
+	/* locks access to the adapter */
+	struct mutex lock;
 	bool config_pending;
 	bool restoring_config;
 	bool autonomous;
@@ -244,22 +244,17 @@ static int pulse8_send_and_wait(struct pulse8 *pulse8,
 	u8 cmd_sc[2];
 	int err;
 
-	mutex_lock(&pulse8->write_lock);
 	err = pulse8_send_and_wait_once(pulse8, cmd, cmd_len, response, size);
+	if (err != -ENOTTY)
+		return err;
 
-	if (err == -ENOTTY) {
-		cmd_sc[0] = MSGCODE_SET_CONTROLLED;
-		cmd_sc[1] = 1;
-		err = pulse8_send_and_wait_once(pulse8, cmd_sc, 2,
-						MSGCODE_COMMAND_ACCEPTED, 1);
-		if (err)
-			goto unlock;
+	cmd_sc[0] = MSGCODE_SET_CONTROLLED;
+	cmd_sc[1] = 1;
+	err = pulse8_send_and_wait_once(pulse8, cmd_sc, 2,
+					MSGCODE_COMMAND_ACCEPTED, 1);
+	if (!err)
 		err = pulse8_send_and_wait_once(pulse8, cmd, cmd_len,
 						response, size);
-	}
-
-unlock:
-	mutex_unlock(&pulse8->write_lock);
 	return err == -ENOTTY ? -EIO : err;
 }
 
@@ -275,15 +270,21 @@ static void pulse8_irq_work_handler(struct work_struct *work)
 		cec_received_msg(pulse8->adap, &pulse8->rx_msg);
 		break;
 	case MSGCODE_TRANSMIT_SUCCEEDED:
+		mutex_lock(&pulse8->lock);
 		cec_transmit_attempt_done(pulse8->adap, CEC_TX_STATUS_OK);
+		mutex_unlock(&pulse8->lock);
 		break;
 	case MSGCODE_TRANSMIT_FAILED_ACK:
+		mutex_lock(&pulse8->lock);
 		cec_transmit_attempt_done(pulse8->adap, CEC_TX_STATUS_NACK);
+		mutex_unlock(&pulse8->lock);
 		break;
 	case MSGCODE_TRANSMIT_FAILED_LINE:
 	case MSGCODE_TRANSMIT_FAILED_TIMEOUT_DATA:
 	case MSGCODE_TRANSMIT_FAILED_TIMEOUT_LINE:
+		mutex_lock(&pulse8->lock);
 		cec_transmit_attempt_done(pulse8->adap, CEC_TX_STATUS_ERROR);
+		mutex_unlock(&pulse8->lock);
 		break;
 	}
 }
@@ -373,10 +374,12 @@ static int pulse8_cec_adap_enable(struct cec_adapter *adap, bool enable)
 	u8 cmd[16];
 	int err;
 
+	mutex_lock(&pulse8->lock);
 	cmd[0] = MSGCODE_SET_CONTROLLED;
 	cmd[1] = enable;
 	err = pulse8_send_and_wait(pulse8, cmd, 2,
 				   MSGCODE_COMMAND_ACCEPTED, 1);
+	mutex_unlock(&pulse8->lock);
 	return enable ? err : 0;
 }
 
@@ -388,7 +391,7 @@ static int pulse8_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
 	u8 cmd[16];
 	int err = 0;
 
-	mutex_lock(&pulse8->config_lock);
+	mutex_lock(&pulse8->lock);
 	if (log_addr != CEC_LOG_ADDR_INVALID)
 		mask = 1 << log_addr;
 	cmd[0] = MSGCODE_SET_ACK_MASK;
@@ -496,7 +499,7 @@ static int pulse8_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
 		pulse8->restoring_config = false;
 	else
 		pulse8->config_pending = true;
-	mutex_unlock(&pulse8->config_lock);
+	mutex_unlock(&pulse8->lock);
 	return log_addr == CEC_LOG_ADDR_INVALID ? 0 : err;
 }
 
@@ -508,6 +511,7 @@ static int pulse8_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 	unsigned int i;
 	int err;
 
+	mutex_lock(&pulse8->lock);
 	cmd[0] = MSGCODE_TRANSMIT_IDLETIME;
 	cmd[1] = signal_free_time;
 	err = pulse8_send_and_wait(pulse8, cmd, 2,
@@ -537,6 +541,7 @@ static int pulse8_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 		}
 	}
 
+	mutex_unlock(&pulse8->lock);
 	return err;
 }
 
@@ -699,14 +704,14 @@ static void pulse8_ping_eeprom_work_handler(struct work_struct *work)
 	u8 cmd;
 
 	schedule_delayed_work(&pulse8->ping_eeprom_work, PING_PERIOD);
+	mutex_lock(&pulse8->lock);
 	cmd = MSGCODE_PING;
 	pulse8_send_and_wait(pulse8, &cmd, 1,
 			     MSGCODE_COMMAND_ACCEPTED, 0);
 
 	if (pulse8->vers < 2)
-		return;
+		goto unlock;
 
-	mutex_lock(&pulse8->config_lock);
 	if (pulse8->config_pending && persistent_config) {
 		dev_dbg(pulse8->dev, "writing pending config to EEPROM\n");
 		cmd = MSGCODE_WRITE_EEPROM;
@@ -716,7 +721,8 @@ static void pulse8_ping_eeprom_work_handler(struct work_struct *work)
 		else
 			pulse8->config_pending = false;
 	}
-	mutex_unlock(&pulse8->config_lock);
+unlock:
+	mutex_unlock(&pulse8->lock);
 }
 
 static int pulse8_connect(struct serio *serio, struct serio_driver *drv)
@@ -742,8 +748,7 @@ static int pulse8_connect(struct serio *serio, struct serio_driver *drv)
 	pulse8->dev = &serio->dev;
 	serio_set_drvdata(serio, pulse8);
 	INIT_WORK(&pulse8->work, pulse8_irq_work_handler);
-	mutex_init(&pulse8->write_lock);
-	mutex_init(&pulse8->config_lock);
+	mutex_init(&pulse8->lock);
 	pulse8->config_pending = false;
 
 	err = serio_open(serio, drv);
-- 
2.23.0

[PATCH 04/10] pulse8-cec: add 2nd debug level

[Hans Verkuil]

Use debug level 2 to show the low-level Pulse-Eight commands.

Also show the message to transmit on debug level 1 and add a
debug log to show where the transmit failed.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/pulse8-cec/pulse8-cec.c | 24 ++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/pulse8-cec/pulse8-cec.c b/drivers/media/usb/pulse8-cec/pulse8-cec.c
index 40880cb79f9a..5ac257d01243 100644
--- a/drivers/media/usb/pulse8-cec/pulse8-cec.c
+++ b/drivers/media/usb/pulse8-cec/pulse8-cec.c
@@ -49,7 +49,7 @@ static int debug;
 static int persistent_config;
 module_param(debug, int, 0644);
 module_param(persistent_config, int, 0644);
-MODULE_PARM_DESC(debug, "debug level (0-1)");
+MODULE_PARM_DESC(debug, "debug level (0-2)");
 MODULE_PARM_DESC(persistent_config, "read config from persistent memory (0-1)");
 
 enum pulse8_msgcodes {
@@ -215,7 +215,9 @@ static int pulse8_send_and_wait_once(struct pulse8 *pulse8,
 {
 	int err;
 
-	/* dev_info(pulse8->dev, "transmit %s: %*ph\n", pulse8_msgname(cmd[0]), cmd_len, cmd); */
+	if (debug > 1)
+		dev_info(pulse8->dev, "transmit %s: %*ph\n",
+			 pulse8_msgname(cmd[0]), cmd_len, cmd);
 	init_completion(&pulse8->cmd_done);
 
 	err = pulse8_send(pulse8->serio, cmd, cmd_len);
@@ -231,8 +233,9 @@ static int pulse8_send_and_wait_once(struct pulse8 *pulse8,
 		return -ENOTTY;
 	if (response &&
 	    ((pulse8->data[0] & 0x3f) != response || pulse8->len < size + 1)) {
-		dev_info(pulse8->dev, "transmit %s: failed %s\n",
-			 pulse8_msgname(cmd[0]), pulse8_msgname(pulse8->data[0]));
+		dev_info(pulse8->dev, "transmit %s failed with %s\n",
+			 pulse8_msgname(cmd[0]),
+			 pulse8_msgname(pulse8->data[0]));
 		return -EIO;
 	}
 	return 0;
@@ -307,9 +310,10 @@ static irqreturn_t pulse8_interrupt(struct serio *serio, unsigned char data,
 		struct cec_msg *msg = &pulse8->rx_msg;
 		u8 msgcode = pulse8->buf[0];
 
-		if (debug)
+		if (debug > 1)
 			dev_info(pulse8->dev, "received %s: %*ph\n",
-				 pulse8_msgname(msgcode), pulse8->idx, pulse8->buf);
+				 pulse8_msgname(msgcode),
+				 pulse8->idx, pulse8->buf);
 		switch (msgcode & 0x3f) {
 		case MSGCODE_FRAME_START:
 			msg->len = 1;
@@ -511,6 +515,9 @@ static int pulse8_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 	unsigned int i;
 	int err;
 
+	if (debug)
+		dev_info(pulse8->dev, "adap transmit %*ph\n",
+			 msg->len, msg->msg);
 	mutex_lock(&pulse8->lock);
 	cmd[0] = MSGCODE_TRANSMIT_IDLETIME;
 	cmd[1] = signal_free_time;
@@ -540,7 +547,10 @@ static int pulse8_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 						   MSGCODE_COMMAND_ACCEPTED, 1);
 		}
 	}
-
+	if (err && debug)
+		dev_info(pulse8->dev, "%s(0x%02x) failed with error %d for msg %*ph\n",
+			 pulse8_msgname(cmd[0]), cmd[1],
+			 err, msg->len, msg->msg);
 	mutex_unlock(&pulse8->lock);
 	return err;
 }
-- 
2.23.0

[PATCH 05/10] pulse8-cec: set tx_done_status for transmit_done status

[Hans Verkuil]

Instead of translating work_result to a transmit_done status
in pulse8_irq_work_handler(), pass the CEC_TX_STATUS via a
new tx_done_status field.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/pulse8-cec/pulse8-cec.c | 41 ++++++++++++-----------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/media/usb/pulse8-cec/pulse8-cec.c b/drivers/media/usb/pulse8-cec/pulse8-cec.c
index 5ac257d01243..68bc2462c829 100644
--- a/drivers/media/usb/pulse8-cec/pulse8-cec.c
+++ b/drivers/media/usb/pulse8-cec/pulse8-cec.c
@@ -174,6 +174,7 @@ struct pulse8 {
 	u8 work_result;
 	struct delayed_work ping_eeprom_work;
 	struct cec_msg rx_msg;
+	u32 tx_done_status;
 	u8 data[DATA_SIZE];
 	unsigned int len;
 	u8 buf[DATA_SIZE];
@@ -266,30 +267,20 @@ static void pulse8_irq_work_handler(struct work_struct *work)
 	struct pulse8 *pulse8 =
 		container_of(work, struct pulse8, work);
 	u8 result = pulse8->work_result;
+	u32 status;
 
 	pulse8->work_result = 0;
 	switch (result & 0x3f) {
 	case MSGCODE_FRAME_DATA:
 		cec_received_msg(pulse8->adap, &pulse8->rx_msg);
 		break;
-	case MSGCODE_TRANSMIT_SUCCEEDED:
-		mutex_lock(&pulse8->lock);
-		cec_transmit_attempt_done(pulse8->adap, CEC_TX_STATUS_OK);
-		mutex_unlock(&pulse8->lock);
-		break;
-	case MSGCODE_TRANSMIT_FAILED_ACK:
-		mutex_lock(&pulse8->lock);
-		cec_transmit_attempt_done(pulse8->adap, CEC_TX_STATUS_NACK);
-		mutex_unlock(&pulse8->lock);
-		break;
-	case MSGCODE_TRANSMIT_FAILED_LINE:
-	case MSGCODE_TRANSMIT_FAILED_TIMEOUT_DATA:
-	case MSGCODE_TRANSMIT_FAILED_TIMEOUT_LINE:
-		mutex_lock(&pulse8->lock);
-		cec_transmit_attempt_done(pulse8->adap, CEC_TX_STATUS_ERROR);
-		mutex_unlock(&pulse8->lock);
-		break;
 	}
+	mutex_lock(&pulse8->lock);
+	status = pulse8->tx_done_status;
+	pulse8->tx_done_status = 0;
+	mutex_unlock(&pulse8->lock);
+	if (status)
+		cec_transmit_attempt_done(pulse8->adap, status);
 }
 
 static irqreturn_t pulse8_interrupt(struct serio *serio, unsigned char data,
@@ -331,12 +322,20 @@ static irqreturn_t pulse8_interrupt(struct serio *serio, unsigned char data,
 			}
 			break;
 		case MSGCODE_TRANSMIT_SUCCEEDED:
-		case MSGCODE_TRANSMIT_FAILED_LINE:
+			WARN_ON(pulse8->tx_done_status);
+			pulse8->tx_done_status = CEC_TX_STATUS_OK;
+			schedule_work(&pulse8->work);
+			break;
 		case MSGCODE_TRANSMIT_FAILED_ACK:
+			WARN_ON(pulse8->tx_done_status);
+			pulse8->tx_done_status = CEC_TX_STATUS_NACK;
+			schedule_work(&pulse8->work);
+			break;
+		case MSGCODE_TRANSMIT_FAILED_LINE:
 		case MSGCODE_TRANSMIT_FAILED_TIMEOUT_DATA:
 		case MSGCODE_TRANSMIT_FAILED_TIMEOUT_LINE:
-			WARN_ON(pulse8->work_result);
-			pulse8->work_result = msgcode;
+			WARN_ON(pulse8->tx_done_status);
+			pulse8->tx_done_status = CEC_TX_STATUS_ERROR;
 			schedule_work(&pulse8->work);
 			break;
 		case MSGCODE_HIGH_ERROR:
@@ -383,6 +382,8 @@ static int pulse8_cec_adap_enable(struct cec_adapter *adap, bool enable)
 	cmd[1] = enable;
 	err = pulse8_send_and_wait(pulse8, cmd, 2,
 				   MSGCODE_COMMAND_ACCEPTED, 1);
+	if (!enable)
+		pulse8->tx_done_status = 0;
 	mutex_unlock(&pulse8->lock);
 	return enable ? err : 0;
 }
-- 
2.23.0

[PATCH 06/10] pulse8-cec: move the transmit to a workqueue

[Hans Verkuil]

Instead of adap_transmit waiting until the full message
is transmitted (and thus hoarding the adap->lock mutex), have it
kick off a transmit workqueue. This prevents adap->lock from
being locked for a very long time.

Also skip FAILED_ACK reports for broadcast messages: this makes
no sense, and it seems a spurious message coming from the
Pulse-Eight, since some time later I see the SUCCEEDED message.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/pulse8-cec/pulse8-cec.c | 132 +++++++++++++---------
 1 file changed, 81 insertions(+), 51 deletions(-)

diff --git a/drivers/media/usb/pulse8-cec/pulse8-cec.c b/drivers/media/usb/pulse8-cec/pulse8-cec.c
index 68bc2462c829..34dbc567dbe0 100644
--- a/drivers/media/usb/pulse8-cec/pulse8-cec.c
+++ b/drivers/media/usb/pulse8-cec/pulse8-cec.c
@@ -169,18 +169,27 @@ struct pulse8 {
 	struct serio *serio;
 	struct cec_adapter *adap;
 	unsigned int vers;
-	struct completion cmd_done;
-	struct work_struct work;
-	u8 work_result;
+
 	struct delayed_work ping_eeprom_work;
+
+	struct work_struct irq_work;
+	u8 irq_work_result;
 	struct cec_msg rx_msg;
+
+	struct work_struct tx_work;
 	u32 tx_done_status;
+	u32 tx_signal_free_time;
+	struct cec_msg tx_msg;
+	bool tx_msg_is_bcast;
+
+	struct completion cmd_done;
 	u8 data[DATA_SIZE];
 	unsigned int len;
 	u8 buf[DATA_SIZE];
 	unsigned int idx;
 	bool escape;
 	bool started;
+
 	/* locks access to the adapter */
 	struct mutex lock;
 	bool config_pending;
@@ -262,14 +271,60 @@ static int pulse8_send_and_wait(struct pulse8 *pulse8,
 	return err == -ENOTTY ? -EIO : err;
 }
 
+static void pulse8_tx_work_handler(struct work_struct *work)
+{
+	struct pulse8 *pulse8 = container_of(work, struct pulse8, tx_work);
+	struct cec_msg *msg = &pulse8->tx_msg;
+	unsigned int i;
+	u8 cmd[2];
+	int err;
+
+	if (msg->len == 0)
+		return;
+
+	mutex_lock(&pulse8->lock);
+	cmd[0] = MSGCODE_TRANSMIT_IDLETIME;
+	cmd[1] = pulse8->tx_signal_free_time;
+	err = pulse8_send_and_wait(pulse8, cmd, 2,
+				   MSGCODE_COMMAND_ACCEPTED, 1);
+	cmd[0] = MSGCODE_TRANSMIT_ACK_POLARITY;
+	cmd[1] = cec_msg_is_broadcast(msg);
+	pulse8->tx_msg_is_bcast = cec_msg_is_broadcast(msg);
+	if (!err)
+		err = pulse8_send_and_wait(pulse8, cmd, 2,
+					   MSGCODE_COMMAND_ACCEPTED, 1);
+	cmd[0] = msg->len == 1 ? MSGCODE_TRANSMIT_EOM : MSGCODE_TRANSMIT;
+	cmd[1] = msg->msg[0];
+	if (!err)
+		err = pulse8_send_and_wait(pulse8, cmd, 2,
+					   MSGCODE_COMMAND_ACCEPTED, 1);
+	if (!err && msg->len > 1) {
+		for (i = 1; !err && i < msg->len; i++) {
+			cmd[0] = ((i == msg->len - 1)) ?
+				MSGCODE_TRANSMIT_EOM : MSGCODE_TRANSMIT;
+			cmd[1] = msg->msg[i];
+			err = pulse8_send_and_wait(pulse8, cmd, 2,
+						   MSGCODE_COMMAND_ACCEPTED, 1);
+		}
+	}
+	if (err && debug)
+		dev_info(pulse8->dev, "%s(0x%02x) failed with error %d for msg %*ph\n",
+			 pulse8_msgname(cmd[0]), cmd[1],
+			 err, msg->len, msg->msg);
+	msg->len = 0;
+	mutex_unlock(&pulse8->lock);
+	if (err)
+		cec_transmit_attempt_done(pulse8->adap, CEC_TX_STATUS_ERROR);
+}
+
 static void pulse8_irq_work_handler(struct work_struct *work)
 {
 	struct pulse8 *pulse8 =
-		container_of(work, struct pulse8, work);
-	u8 result = pulse8->work_result;
+		container_of(work, struct pulse8, irq_work);
+	u8 result = pulse8->irq_work_result;
 	u32 status;
 
-	pulse8->work_result = 0;
+	pulse8->irq_work_result = 0;
 	switch (result & 0x3f) {
 	case MSGCODE_FRAME_DATA:
 		cec_received_msg(pulse8->adap, &pulse8->rx_msg);
@@ -315,28 +370,34 @@ static irqreturn_t pulse8_interrupt(struct serio *serio, unsigned char data,
 				break;
 			msg->msg[msg->len++] = pulse8->buf[1];
 			if (msgcode & MSGCODE_FRAME_EOM) {
-				WARN_ON(pulse8->work_result);
-				pulse8->work_result = msgcode;
-				schedule_work(&pulse8->work);
+				WARN_ON(pulse8->irq_work_result);
+				pulse8->irq_work_result = msgcode;
+				schedule_work(&pulse8->irq_work);
 				break;
 			}
 			break;
 		case MSGCODE_TRANSMIT_SUCCEEDED:
 			WARN_ON(pulse8->tx_done_status);
 			pulse8->tx_done_status = CEC_TX_STATUS_OK;
-			schedule_work(&pulse8->work);
+			schedule_work(&pulse8->irq_work);
 			break;
 		case MSGCODE_TRANSMIT_FAILED_ACK:
+			/*
+			 * A NACK for a broadcast message makes no sense, these
+			 * seem to be spurious messages and are skipped.
+			 */
+			if (pulse8->tx_msg_is_bcast)
+				break;
 			WARN_ON(pulse8->tx_done_status);
 			pulse8->tx_done_status = CEC_TX_STATUS_NACK;
-			schedule_work(&pulse8->work);
+			schedule_work(&pulse8->irq_work);
 			break;
 		case MSGCODE_TRANSMIT_FAILED_LINE:
 		case MSGCODE_TRANSMIT_FAILED_TIMEOUT_DATA:
 		case MSGCODE_TRANSMIT_FAILED_TIMEOUT_LINE:
 			WARN_ON(pulse8->tx_done_status);
 			pulse8->tx_done_status = CEC_TX_STATUS_ERROR;
-			schedule_work(&pulse8->work);
+			schedule_work(&pulse8->irq_work);
 			break;
 		case MSGCODE_HIGH_ERROR:
 		case MSGCODE_LOW_ERROR:
@@ -512,48 +573,14 @@ static int pulse8_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 				    u32 signal_free_time, struct cec_msg *msg)
 {
 	struct pulse8 *pulse8 = cec_get_drvdata(adap);
-	u8 cmd[2];
-	unsigned int i;
-	int err;
 
+	pulse8->tx_msg = *msg;
 	if (debug)
 		dev_info(pulse8->dev, "adap transmit %*ph\n",
 			 msg->len, msg->msg);
-	mutex_lock(&pulse8->lock);
-	cmd[0] = MSGCODE_TRANSMIT_IDLETIME;
-	cmd[1] = signal_free_time;
-	err = pulse8_send_and_wait(pulse8, cmd, 2,
-				   MSGCODE_COMMAND_ACCEPTED, 1);
-	cmd[0] = MSGCODE_TRANSMIT_ACK_POLARITY;
-	cmd[1] = cec_msg_is_broadcast(msg);
-	if (!err)
-		err = pulse8_send_and_wait(pulse8, cmd, 2,
-					   MSGCODE_COMMAND_ACCEPTED, 1);
-	cmd[0] = msg->len == 1 ? MSGCODE_TRANSMIT_EOM : MSGCODE_TRANSMIT;
-	cmd[1] = msg->msg[0];
-	if (!err)
-		err = pulse8_send_and_wait(pulse8, cmd, 2,
-					   MSGCODE_COMMAND_ACCEPTED, 1);
-	if (!err && msg->len > 1) {
-		cmd[0] = msg->len == 2 ? MSGCODE_TRANSMIT_EOM :
-					 MSGCODE_TRANSMIT;
-		cmd[1] = msg->msg[1];
-		err = pulse8_send_and_wait(pulse8, cmd, 2,
-					   MSGCODE_COMMAND_ACCEPTED, 1);
-		for (i = 0; !err && i + 2 < msg->len; i++) {
-			cmd[0] = (i + 2 == msg->len - 1) ?
-				MSGCODE_TRANSMIT_EOM : MSGCODE_TRANSMIT;
-			cmd[1] = msg->msg[i + 2];
-			err = pulse8_send_and_wait(pulse8, cmd, 2,
-						   MSGCODE_COMMAND_ACCEPTED, 1);
-		}
-	}
-	if (err && debug)
-		dev_info(pulse8->dev, "%s(0x%02x) failed with error %d for msg %*ph\n",
-			 pulse8_msgname(cmd[0]), cmd[1],
-			 err, msg->len, msg->msg);
-	mutex_unlock(&pulse8->lock);
-	return err;
+	pulse8->tx_signal_free_time = signal_free_time;
+	schedule_work(&pulse8->tx_work);
+	return 0;
 }
 
 static const struct cec_adap_ops pulse8_cec_adap_ops = {
@@ -568,6 +595,8 @@ static void pulse8_disconnect(struct serio *serio)
 
 	cec_unregister_adapter(pulse8->adap);
 	cancel_delayed_work_sync(&pulse8->ping_eeprom_work);
+	cancel_work_sync(&pulse8->irq_work);
+	cancel_work_sync(&pulse8->tx_work);
 	dev_info(&serio->dev, "disconnected\n");
 	serio_close(serio);
 	serio_set_drvdata(serio, NULL);
@@ -758,7 +787,8 @@ static int pulse8_connect(struct serio *serio, struct serio_driver *drv)
 
 	pulse8->dev = &serio->dev;
 	serio_set_drvdata(serio, pulse8);
-	INIT_WORK(&pulse8->work, pulse8_irq_work_handler);
+	INIT_WORK(&pulse8->irq_work, pulse8_irq_work_handler);
+	INIT_WORK(&pulse8->tx_work, pulse8_tx_work_handler);
 	mutex_init(&pulse8->lock);
 	pulse8->config_pending = false;
 
-- 
2.23.0

[PATCH 07/10] pulse8-cec: queue received messages in an array

[Hans Verkuil]

It turns out that received CEC messages can arrive faster than
can be processed by the CEC framework, resulting in lost messages.

Instead of storing only one CEC message, store up to 8.

Also fix a bug where the EOM bit wasn't checked for a received
message of length 1, so POLL messages weren't properly reported.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/pulse8-cec/pulse8-cec.c | 80 +++++++++++++++++------
 1 file changed, 61 insertions(+), 19 deletions(-)

diff --git a/drivers/media/usb/pulse8-cec/pulse8-cec.c b/drivers/media/usb/pulse8-cec/pulse8-cec.c
index 34dbc567dbe0..961b34dfaf6d 100644
--- a/drivers/media/usb/pulse8-cec/pulse8-cec.c
+++ b/drivers/media/usb/pulse8-cec/pulse8-cec.c
@@ -164,6 +164,8 @@ static const char *pulse8_msgname(u8 cmd)
 
 #define PING_PERIOD	(15 * HZ)
 
+#define NUM_MSGS 8
+
 struct pulse8 {
 	struct device *dev;
 	struct serio *serio;
@@ -173,8 +175,12 @@ struct pulse8 {
 	struct delayed_work ping_eeprom_work;
 
 	struct work_struct irq_work;
-	u8 irq_work_result;
-	struct cec_msg rx_msg;
+	struct cec_msg rx_msg[NUM_MSGS];
+	unsigned int rx_msg_cur_idx, rx_msg_num;
+	/* protect rx_msg_cur_idx and rx_msg_num */
+	spinlock_t msg_lock;
+	u8 new_rx_msg[CEC_MAX_MSG_SIZE];
+	u8 new_rx_msg_len;
 
 	struct work_struct tx_work;
 	u32 tx_done_status;
@@ -321,15 +327,22 @@ static void pulse8_irq_work_handler(struct work_struct *work)
 {
 	struct pulse8 *pulse8 =
 		container_of(work, struct pulse8, irq_work);
-	u8 result = pulse8->irq_work_result;
+	unsigned long flags;
 	u32 status;
 
-	pulse8->irq_work_result = 0;
-	switch (result & 0x3f) {
-	case MSGCODE_FRAME_DATA:
-		cec_received_msg(pulse8->adap, &pulse8->rx_msg);
-		break;
+	spin_lock_irqsave(&pulse8->msg_lock, flags);
+	while (pulse8->rx_msg_num) {
+		spin_unlock_irqrestore(&pulse8->msg_lock, flags);
+		cec_received_msg(pulse8->adap,
+				 &pulse8->rx_msg[pulse8->rx_msg_cur_idx]);
+		spin_lock_irqsave(&pulse8->msg_lock, flags);
+		if (pulse8->rx_msg_num)
+			pulse8->rx_msg_num--;
+		pulse8->rx_msg_cur_idx =
+			(pulse8->rx_msg_cur_idx + 1) % NUM_MSGS;
 	}
+	spin_unlock_irqrestore(&pulse8->msg_lock, flags);
+
 	mutex_lock(&pulse8->lock);
 	status = pulse8->tx_done_status;
 	pulse8->tx_done_status = 0;
@@ -342,6 +355,8 @@ static irqreturn_t pulse8_interrupt(struct serio *serio, unsigned char data,
 				    unsigned int flags)
 {
 	struct pulse8 *pulse8 = serio_get_drvdata(serio);
+	unsigned long irq_flags;
+	unsigned int idx;
 
 	if (!pulse8->started && data != MSGSTART)
 		return IRQ_HANDLED;
@@ -353,7 +368,6 @@ static irqreturn_t pulse8_interrupt(struct serio *serio, unsigned char data,
 		data += MSGOFFSET;
 		pulse8->escape = false;
 	} else if (data == MSGEND) {
-		struct cec_msg *msg = &pulse8->rx_msg;
 		u8 msgcode = pulse8->buf[0];
 
 		if (debug > 1)
@@ -362,19 +376,43 @@ static irqreturn_t pulse8_interrupt(struct serio *serio, unsigned char data,
 				 pulse8->idx, pulse8->buf);
 		switch (msgcode & 0x3f) {
 		case MSGCODE_FRAME_START:
-			msg->len = 1;
-			msg->msg[0] = pulse8->buf[1];
-			break;
+			/*
+			 * Test if we are receiving a new msg when a previous
+			 * message is still pending.
+			 */
+			if (!(msgcode & MSGCODE_FRAME_EOM)) {
+				pulse8->new_rx_msg_len = 1;
+				pulse8->new_rx_msg[0] = pulse8->buf[1];
+				break;
+			}
+			/* fall through */
 		case MSGCODE_FRAME_DATA:
-			if (msg->len == CEC_MAX_MSG_SIZE)
+			if (pulse8->new_rx_msg_len < CEC_MAX_MSG_SIZE)
+				pulse8->new_rx_msg[pulse8->new_rx_msg_len++] =
+					pulse8->buf[1];
+			if (!(msgcode & MSGCODE_FRAME_EOM))
 				break;
-			msg->msg[msg->len++] = pulse8->buf[1];
-			if (msgcode & MSGCODE_FRAME_EOM) {
-				WARN_ON(pulse8->irq_work_result);
-				pulse8->irq_work_result = msgcode;
-				schedule_work(&pulse8->irq_work);
+
+			spin_lock_irqsave(&pulse8->msg_lock, irq_flags);
+			idx = (pulse8->rx_msg_cur_idx + pulse8->rx_msg_num) %
+				NUM_MSGS;
+			if (pulse8->rx_msg_num == NUM_MSGS) {
+				dev_warn(pulse8->dev,
+					 "message queue is full, dropping %*ph\n",
+					 pulse8->new_rx_msg_len,
+					 pulse8->new_rx_msg);
+				spin_unlock_irqrestore(&pulse8->msg_lock,
+						       irq_flags);
+				pulse8->new_rx_msg_len = 0;
 				break;
 			}
+			pulse8->rx_msg_num++;
+			memcpy(pulse8->rx_msg[idx].msg, pulse8->new_rx_msg,
+			       pulse8->new_rx_msg_len);
+			pulse8->rx_msg[idx].len = pulse8->new_rx_msg_len;
+			spin_unlock_irqrestore(&pulse8->msg_lock, irq_flags);
+			schedule_work(&pulse8->irq_work);
+			pulse8->new_rx_msg_len = 0;
 			break;
 		case MSGCODE_TRANSMIT_SUCCEEDED:
 			WARN_ON(pulse8->tx_done_status);
@@ -403,6 +441,7 @@ static irqreturn_t pulse8_interrupt(struct serio *serio, unsigned char data,
 		case MSGCODE_LOW_ERROR:
 		case MSGCODE_RECEIVE_FAILED:
 		case MSGCODE_TIMEOUT_ERROR:
+			pulse8->new_rx_msg_len = 0;
 			break;
 		case MSGCODE_COMMAND_ACCEPTED:
 		case MSGCODE_COMMAND_REJECTED:
@@ -443,8 +482,10 @@ static int pulse8_cec_adap_enable(struct cec_adapter *adap, bool enable)
 	cmd[1] = enable;
 	err = pulse8_send_and_wait(pulse8, cmd, 2,
 				   MSGCODE_COMMAND_ACCEPTED, 1);
-	if (!enable)
+	if (!enable) {
+		pulse8->rx_msg_num = 0;
 		pulse8->tx_done_status = 0;
+	}
 	mutex_unlock(&pulse8->lock);
 	return enable ? err : 0;
 }
@@ -790,6 +831,7 @@ static int pulse8_connect(struct serio *serio, struct serio_driver *drv)
 	INIT_WORK(&pulse8->irq_work, pulse8_irq_work_handler);
 	INIT_WORK(&pulse8->tx_work, pulse8_tx_work_handler);
 	mutex_init(&pulse8->lock);
+	spin_lock_init(&pulse8->msg_lock);
 	pulse8->config_pending = false;
 
 	err = serio_open(serio, drv);
-- 
2.23.0

[PATCH 08/10] pulse8-cec: use adap_free callback

[Hans Verkuil]

Don't free everything in the disconnect callback, instead use
the adap_free callback, which is called when the last open
filehandle is closed.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/pulse8-cec/pulse8-cec.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/media/usb/pulse8-cec/pulse8-cec.c b/drivers/media/usb/pulse8-cec/pulse8-cec.c
index 961b34dfaf6d..d18d1f456576 100644
--- a/drivers/media/usb/pulse8-cec/pulse8-cec.c
+++ b/drivers/media/usb/pulse8-cec/pulse8-cec.c
@@ -624,10 +624,23 @@ static int pulse8_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
 	return 0;
 }
 
+static void pulse8_cec_adap_free(struct cec_adapter *adap)
+{
+	struct pulse8 *pulse8 = cec_get_drvdata(adap);
+
+	cancel_delayed_work_sync(&pulse8->ping_eeprom_work);
+	cancel_work_sync(&pulse8->irq_work);
+	cancel_work_sync(&pulse8->tx_work);
+	serio_close(pulse8->serio);
+	serio_set_drvdata(pulse8->serio, NULL);
+	kfree(pulse8);
+}
+
 static const struct cec_adap_ops pulse8_cec_adap_ops = {
 	.adap_enable = pulse8_cec_adap_enable,
 	.adap_log_addr = pulse8_cec_adap_log_addr,
 	.adap_transmit = pulse8_cec_adap_transmit,
+	.adap_free = pulse8_cec_adap_free,
 };
 
 static void pulse8_disconnect(struct serio *serio)
@@ -635,13 +648,6 @@ static void pulse8_disconnect(struct serio *serio)
 	struct pulse8 *pulse8 = serio_get_drvdata(serio);
 
 	cec_unregister_adapter(pulse8->adap);
-	cancel_delayed_work_sync(&pulse8->ping_eeprom_work);
-	cancel_work_sync(&pulse8->irq_work);
-	cancel_work_sync(&pulse8->tx_work);
-	dev_info(&serio->dev, "disconnected\n");
-	serio_close(serio);
-	serio_set_drvdata(serio, NULL);
-	kfree(pulse8);
 }
 
 static int pulse8_setup(struct pulse8 *pulse8, struct serio *serio,
-- 
2.23.0

[PATCH 09/10] pulse8-cec: schedule next ping after current ping finished

[Hans Verkuil]

Don't schedule the next ping before the current ping is
sent, schedule it after.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/pulse8-cec/pulse8-cec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/pulse8-cec/pulse8-cec.c b/drivers/media/usb/pulse8-cec/pulse8-cec.c
index d18d1f456576..17fcaf7e558a 100644
--- a/drivers/media/usb/pulse8-cec/pulse8-cec.c
+++ b/drivers/media/usb/pulse8-cec/pulse8-cec.c
@@ -790,7 +790,6 @@ static void pulse8_ping_eeprom_work_handler(struct work_struct *work)
 		container_of(work, struct pulse8, ping_eeprom_work.work);
 	u8 cmd;
 
-	schedule_delayed_work(&pulse8->ping_eeprom_work, PING_PERIOD);
 	mutex_lock(&pulse8->lock);
 	cmd = MSGCODE_PING;
 	pulse8_send_and_wait(pulse8, &cmd, 1,
@@ -809,6 +808,7 @@ static void pulse8_ping_eeprom_work_handler(struct work_struct *work)
 			pulse8->config_pending = false;
 	}
 unlock:
+	schedule_delayed_work(&pulse8->ping_eeprom_work, PING_PERIOD);
 	mutex_unlock(&pulse8->lock);
 }
 
-- 
2.23.0

[PATCH 10/10] pulse8-cec: log when a CEC message is received

[Hans Verkuil]

Log (if debug > 0) when a CEC message is received.

This is done for transmits already, so it makes sense to do the
same for receives.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/usb/pulse8-cec/pulse8-cec.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/usb/pulse8-cec/pulse8-cec.c b/drivers/media/usb/pulse8-cec/pulse8-cec.c
index 17fcaf7e558a..afda438d4e0a 100644
--- a/drivers/media/usb/pulse8-cec/pulse8-cec.c
+++ b/drivers/media/usb/pulse8-cec/pulse8-cec.c
@@ -333,6 +333,10 @@ static void pulse8_irq_work_handler(struct work_struct *work)
 	spin_lock_irqsave(&pulse8->msg_lock, flags);
 	while (pulse8->rx_msg_num) {
 		spin_unlock_irqrestore(&pulse8->msg_lock, flags);
+		if (debug)
+			dev_info(pulse8->dev, "adap received %*ph\n",
+				 pulse8->rx_msg[pulse8->rx_msg_cur_idx].len,
+				 pulse8->rx_msg[pulse8->rx_msg_cur_idx].msg);
 		cec_received_msg(pulse8->adap,
 				 &pulse8->rx_msg[pulse8->rx_msg_cur_idx]);
 		spin_lock_irqsave(&pulse8->msg_lock, flags);
-- 
2.23.0