[PATCH 0/7] libps2 facelift

[PATCH 0/7] libps2 facelift

[Dmitry Torokhov]

Hi,

The main reason for the patch series is to have a bit more manageable
debug info for PS/2 init sequence: raw i8042 debug is way too noisy,
one has to decode sliced commands by hand, etc, etc. With proposed
changes you get a nice parsed command flow:

[14421.985416] __ps2_command: psmouse serio2: f5 [] - 0/00000000 []
[14421.993112] __ps2_command: psmouse serio2: f6 [] - 0/00000000 []
[14422.007738] __ps2_command: psmouse serio2: e1 [] - 0/00000000 [03 04]
[14422.042522] __ps2_command: psmouse serio2: e2 [81 4a 80] - 0/00000000 []
[14422.057091] __ps2_command: psmouse serio2: e2 [2c] - 0/00000000 [20]
[14422.057118] psmouse serio2: trackpoint: Elan TrackPoint firmware: 0x04, buttons: 3/3
[14422.072062] __ps2_command: psmouse serio2: f3 [64] - 0/00000000 []
[14422.086839] __ps2_command: psmouse serio2: e8 [03] - 0/00000000 []
[14422.094306] __ps2_command: psmouse serio2: e6 [] - 0/00000000 []
[14422.094504] input: TPPS/2 Elan TrackPoint as /devices/rmi4-00/rmi4-00.fn03/serio2/input/input33
[14422.128629] __ps2_command: psmouse serio2: f4 [] - 0/00000000 []

Additionally libps2 now can handle command and parameter byte
retransmissions and we relaxed rules for getting initial ACK to the
command byte, which hopefully will help properly detect devices that
are not silent when we start probing them.

Thanks.

Dmitry Torokhov (7):
  Input: libps2 - fix switch statement formatting
  Input: libps2 - use u8 for byte data
  Input: libps2 - use BIT() for bitmask constants
  Input: psmouse - move sliced command implementation to libps2
  Input: libps2 - add debugging statements
  Input: libps2 - support retransmission of command data
  Input: libps2 - relax command byte ACK handling

 drivers/input/mouse/elantech.c     |  12 +-
 drivers/input/mouse/logips2pp.c    |   2 +-
 drivers/input/mouse/psmouse-base.c |  26 ---
 drivers/input/mouse/psmouse.h      |   1 -
 drivers/input/mouse/synaptics.c    |   8 +-
 drivers/input/serio/libps2.c       | 322 +++++++++++++++++++++++++------------
 include/linux/libps2.h             |  34 ++--
 7 files changed, 249 insertions(+), 156 deletions(-)

-- 
Dmitry

[PATCH 1/7] Input: libps2 - fix switch statement formatting

[Dmitry Torokhov]

Individual labels of switch statements should have the same indentation
level as the switch statement itself.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/serio/libps2.c | 131 +++++++++++++++++++++----------------------
 1 file changed, 65 insertions(+), 66 deletions(-)

diff --git a/drivers/input/serio/libps2.c b/drivers/input/serio/libps2.c
index 83e9c663aa672..21aea5169a99c 100644
--- a/drivers/input/serio/libps2.c
+++ b/drivers/input/serio/libps2.c
@@ -126,46 +126,46 @@ EXPORT_SYMBOL(ps2_is_keyboard_id);
 static int ps2_adjust_timeout(struct ps2dev *ps2dev, int command, int timeout)
 {
 	switch (command) {
-		case PS2_CMD_RESET_BAT:
-			/*
-			 * Device has sent the first response byte after
-			 * reset command, reset is thus done, so we can
-			 * shorten the timeout.
-			 * The next byte will come soon (keyboard) or not
-			 * at all (mouse).
-			 */
-			if (timeout > msecs_to_jiffies(100))
-				timeout = msecs_to_jiffies(100);
-			break;
+	case PS2_CMD_RESET_BAT:
+		/*
+		 * Device has sent the first response byte after
+		 * reset command, reset is thus done, so we can
+		 * shorten the timeout.
+		 * The next byte will come soon (keyboard) or not
+		 * at all (mouse).
+		 */
+		if (timeout > msecs_to_jiffies(100))
+			timeout = msecs_to_jiffies(100);
+		break;
 
-		case PS2_CMD_GETID:
-			/*
-			 * Microsoft Natural Elite keyboard responds to
-			 * the GET ID command as it were a mouse, with
-			 * a single byte. Fail the command so atkbd will
-			 * use alternative probe to detect it.
-			 */
-			if (ps2dev->cmdbuf[1] == 0xaa) {
-				serio_pause_rx(ps2dev->serio);
-				ps2dev->flags = 0;
-				serio_continue_rx(ps2dev->serio);
-				timeout = 0;
-			}
-
-			/*
-			 * If device behind the port is not a keyboard there
-			 * won't be 2nd byte of ID response.
-			 */
-			if (!ps2_is_keyboard_id(ps2dev->cmdbuf[1])) {
-				serio_pause_rx(ps2dev->serio);
-				ps2dev->flags = ps2dev->cmdcnt = 0;
-				serio_continue_rx(ps2dev->serio);
-				timeout = 0;
-			}
-			break;
+	case PS2_CMD_GETID:
+		/*
+		 * Microsoft Natural Elite keyboard responds to
+		 * the GET ID command as it were a mouse, with
+		 * a single byte. Fail the command so atkbd will
+		 * use alternative probe to detect it.
+		 */
+		if (ps2dev->cmdbuf[1] == 0xaa) {
+			serio_pause_rx(ps2dev->serio);
+			ps2dev->flags = 0;
+			serio_continue_rx(ps2dev->serio);
+			timeout = 0;
+		}
 
-		default:
-			break;
+		/*
+		 * If device behind the port is not a keyboard there
+		 * won't be 2nd byte of ID response.
+		 */
+		if (!ps2_is_keyboard_id(ps2dev->cmdbuf[1])) {
+			serio_pause_rx(ps2dev->serio);
+			ps2dev->flags = ps2dev->cmdcnt = 0;
+			serio_continue_rx(ps2dev->serio);
+			timeout = 0;
+		}
+		break;
+
+	default:
+		break;
 	}
 
 	return timeout;
@@ -289,39 +289,38 @@ EXPORT_SYMBOL(ps2_init);
 int ps2_handle_ack(struct ps2dev *ps2dev, unsigned char data)
 {
 	switch (data) {
-		case PS2_RET_ACK:
-			ps2dev->nak = 0;
+	case PS2_RET_ACK:
+		ps2dev->nak = 0;
+		break;
+
+	case PS2_RET_NAK:
+		ps2dev->flags |= PS2_FLAG_NAK;
+		ps2dev->nak = PS2_RET_NAK;
+		break;
+
+	case PS2_RET_ERR:
+		if (ps2dev->flags & PS2_FLAG_NAK) {
+			ps2dev->flags &= ~PS2_FLAG_NAK;
+			ps2dev->nak = PS2_RET_ERR;
 			break;
+		}
 
-		case PS2_RET_NAK:
-			ps2dev->flags |= PS2_FLAG_NAK;
-			ps2dev->nak = PS2_RET_NAK;
+	/*
+	 * Workaround for mice which don't ACK the Get ID command.
+	 * These are valid mouse IDs that we recognize.
+	 */
+	case 0x00:
+	case 0x03:
+	case 0x04:
+		if (ps2dev->flags & PS2_FLAG_WAITID) {
+			ps2dev->nak = 0;
 			break;
-
-		case PS2_RET_ERR:
-			if (ps2dev->flags & PS2_FLAG_NAK) {
-				ps2dev->flags &= ~PS2_FLAG_NAK;
-				ps2dev->nak = PS2_RET_ERR;
-				break;
-			}
-
-		/*
-		 * Workaround for mice which don't ACK the Get ID command.
-		 * These are valid mouse IDs that we recognize.
-		 */
-		case 0x00:
-		case 0x03:
-		case 0x04:
-			if (ps2dev->flags & PS2_FLAG_WAITID) {
-				ps2dev->nak = 0;
-				break;
-			}
-			/* Fall through */
-		default:
-			return 0;
+		}
+		/* Fall through */
+	default:
+		return 0;
 	}
 
-
 	if (!ps2dev->nak) {
 		ps2dev->flags &= ~PS2_FLAG_NAK;
 		if (ps2dev->cmdcnt)
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 2/7] Input: libps2 - use u8 for byte data

[Dmitry Torokhov]

Instead of using unsigned char for the byte data switch to using u8. Also
use unsigned int for the command codes and timeouts, and have
ps2_handle_ack() and ps2_handle_response() return bool instead of int, as
they do not return error codes but rather signal whether a byte was handled
or not handled. ps2_is_keyboard_id() now returns bool as well.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/serio/libps2.c | 31 ++++++++++++++++---------------
 include/linux/libps2.h       | 20 ++++++++++----------
 2 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/drivers/input/serio/libps2.c b/drivers/input/serio/libps2.c
index 21aea5169a99c..c3712f0a47b50 100644
--- a/drivers/input/serio/libps2.c
+++ b/drivers/input/serio/libps2.c
@@ -34,7 +34,7 @@ MODULE_LICENSE("GPL");
  * ps2_sendbyte() can only be called from a process context.
  */
 
-int ps2_sendbyte(struct ps2dev *ps2dev, unsigned char byte, int timeout)
+int ps2_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
 {
 	serio_pause_rx(ps2dev->serio);
 	ps2dev->nak = 1;
@@ -75,7 +75,7 @@ EXPORT_SYMBOL(ps2_end_command);
  * and discards them.
  */
 
-void ps2_drain(struct ps2dev *ps2dev, int maxbytes, int timeout)
+void ps2_drain(struct ps2dev *ps2dev, size_t maxbytes, unsigned int timeout)
 {
 	if (maxbytes > sizeof(ps2dev->cmdbuf)) {
 		WARN_ON(1);
@@ -102,9 +102,9 @@ EXPORT_SYMBOL(ps2_drain);
  * known keyboard IDs.
  */
 
-int ps2_is_keyboard_id(char id_byte)
+bool ps2_is_keyboard_id(u8 id_byte)
 {
-	static const char keyboard_ids[] = {
+	static const u8 keyboard_ids[] = {
 		0xab,	/* Regular keyboards		*/
 		0xac,	/* NCD Sun keyboard		*/
 		0x2b,	/* Trust keyboard, translated	*/
@@ -123,7 +123,8 @@ EXPORT_SYMBOL(ps2_is_keyboard_id);
  * completion.
  */
 
-static int ps2_adjust_timeout(struct ps2dev *ps2dev, int command, int timeout)
+static int ps2_adjust_timeout(struct ps2dev *ps2dev,
+			      unsigned int command, unsigned int timeout)
 {
 	switch (command) {
 	case PS2_CMD_RESET_BAT:
@@ -178,11 +179,11 @@ static int ps2_adjust_timeout(struct ps2dev *ps2dev, int command, int timeout)
  * ps2_command() can only be called from a process context
  */
 
-int __ps2_command(struct ps2dev *ps2dev, unsigned char *param, int command)
+int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
 {
-	int timeout;
-	int send = (command >> 12) & 0xf;
-	int receive = (command >> 8) & 0xf;
+	unsigned int timeout;
+	unsigned int send = (command >> 12) & 0xf;
+	unsigned int receive = (command >> 8) & 0xf;
 	int rc = -1;
 	int i;
 
@@ -256,7 +257,7 @@ int __ps2_command(struct ps2dev *ps2dev, unsigned char *param, int command)
 }
 EXPORT_SYMBOL(__ps2_command);
 
-int ps2_command(struct ps2dev *ps2dev, unsigned char *param, int command)
+int ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
 {
 	int rc;
 
@@ -286,7 +287,7 @@ EXPORT_SYMBOL(ps2_init);
  * to properly process ACK/NAK of a command from a PS/2 device.
  */
 
-int ps2_handle_ack(struct ps2dev *ps2dev, unsigned char data)
+bool ps2_handle_ack(struct ps2dev *ps2dev, u8 data)
 {
 	switch (data) {
 	case PS2_RET_ACK:
@@ -318,7 +319,7 @@ int ps2_handle_ack(struct ps2dev *ps2dev, unsigned char data)
 		}
 		/* Fall through */
 	default:
-		return 0;
+		return false;
 	}
 
 	if (!ps2dev->nak) {
@@ -333,7 +334,7 @@ int ps2_handle_ack(struct ps2dev *ps2dev, unsigned char data)
 	if (data != PS2_RET_ACK)
 		ps2_handle_response(ps2dev, data);
 
-	return 1;
+	return true;
 }
 EXPORT_SYMBOL(ps2_handle_ack);
 
@@ -343,7 +344,7 @@ EXPORT_SYMBOL(ps2_handle_ack);
  * waiting for completion of the command.
  */
 
-int ps2_handle_response(struct ps2dev *ps2dev, unsigned char data)
+bool ps2_handle_response(struct ps2dev *ps2dev, u8 data)
 {
 	if (ps2dev->cmdcnt)
 		ps2dev->cmdbuf[--ps2dev->cmdcnt] = data;
@@ -359,7 +360,7 @@ int ps2_handle_response(struct ps2dev *ps2dev, unsigned char data)
 		wake_up(&ps2dev->wait);
 	}
 
-	return 1;
+	return true;
 }
 EXPORT_SYMBOL(ps2_handle_response);
 
diff --git a/include/linux/libps2.h b/include/linux/libps2.h
index 4ad06e824f76f..365c50b097ded 100644
--- a/include/linux/libps2.h
+++ b/include/linux/libps2.h
@@ -36,21 +36,21 @@ struct ps2dev {
 	wait_queue_head_t wait;
 
 	unsigned long flags;
-	unsigned char cmdbuf[8];
-	unsigned char cmdcnt;
-	unsigned char nak;
+	u8 cmdbuf[8];
+	u8 cmdcnt;
+	u8 nak;
 };
 
 void ps2_init(struct ps2dev *ps2dev, struct serio *serio);
-int ps2_sendbyte(struct ps2dev *ps2dev, unsigned char byte, int timeout);
-void ps2_drain(struct ps2dev *ps2dev, int maxbytes, int timeout);
+int ps2_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout);
+void ps2_drain(struct ps2dev *ps2dev, size_t maxbytes, unsigned int timeout);
 void ps2_begin_command(struct ps2dev *ps2dev);
 void ps2_end_command(struct ps2dev *ps2dev);
-int __ps2_command(struct ps2dev *ps2dev, unsigned char *param, int command);
-int ps2_command(struct ps2dev *ps2dev, unsigned char *param, int command);
-int ps2_handle_ack(struct ps2dev *ps2dev, unsigned char data);
-int ps2_handle_response(struct ps2dev *ps2dev, unsigned char data);
+int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command);
+int ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command);
+bool ps2_handle_ack(struct ps2dev *ps2dev, u8 data);
+bool ps2_handle_response(struct ps2dev *ps2dev, u8 data);
 void ps2_cmd_aborted(struct ps2dev *ps2dev);
-int ps2_is_keyboard_id(char id);
+bool ps2_is_keyboard_id(u8 id);
 
 #endif /* _LIBPS2_H */
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 3/7] Input: libps2 - use BIT() for bitmask constants

[Dmitry Torokhov]

Let's explicitly document bit numbers with BIT() macro.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 include/linux/libps2.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/libps2.h b/include/linux/libps2.h
index 365c50b097ded..649295a5ff47d 100644
--- a/include/linux/libps2.h
+++ b/include/linux/libps2.h
@@ -20,11 +20,11 @@
 #define PS2_RET_NAK		0xfe
 #define PS2_RET_ERR		0xfc
 
-#define PS2_FLAG_ACK		1	/* Waiting for ACK/NAK */
-#define PS2_FLAG_CMD		2	/* Waiting for command to finish */
-#define PS2_FLAG_CMD1		4	/* Waiting for the first byte of command response */
-#define PS2_FLAG_WAITID		8	/* Command execiting is GET ID */
-#define PS2_FLAG_NAK		16	/* Last transmission was NAKed */
+#define PS2_FLAG_ACK		BIT(0)	/* Waiting for ACK/NAK */
+#define PS2_FLAG_CMD		BIT(1)	/* Waiting for a command to finish */
+#define PS2_FLAG_CMD1		BIT(2)	/* Waiting for the first byte of command response */
+#define PS2_FLAG_WAITID		BIT(3)	/* Command executing is GET ID */
+#define PS2_FLAG_NAK		BIT(4)	/* Last transmission was NAKed */
 
 struct ps2dev {
 	struct serio *serio;
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 4/7] Input: psmouse - move sliced command implementation to libps2

[Dmitry Torokhov]

In preparation to adding some debugging statements to PS/2 control
sequences let's move psmouse_sliced_command() into libps2 and rename it
to ps2_sliced_command().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/mouse/elantech.c     | 12 ++++++------
 drivers/input/mouse/logips2pp.c    |  2 +-
 drivers/input/mouse/psmouse-base.c | 26 --------------------------
 drivers/input/mouse/psmouse.h      |  1 -
 drivers/input/mouse/synaptics.c    |  8 ++++----
 drivers/input/serio/libps2.c       | 32 ++++++++++++++++++++++++++++++++
 include/linux/libps2.h             |  3 +++
 7 files changed, 46 insertions(+), 38 deletions(-)

diff --git a/drivers/input/mouse/elantech.c b/drivers/input/mouse/elantech.c
index af7fc17c14d96..db47a5e1d114d 100644
--- a/drivers/input/mouse/elantech.c
+++ b/drivers/input/mouse/elantech.c
@@ -35,7 +35,7 @@
 static int synaptics_send_cmd(struct psmouse *psmouse, unsigned char c,
 				unsigned char *param)
 {
-	if (psmouse_sliced_command(psmouse, c) ||
+	if (ps2_sliced_command(&psmouse->ps2dev, c) ||
 	    ps2_command(&psmouse->ps2dev, param, PSMOUSE_CMD_GETINFO)) {
 		psmouse_err(psmouse, "%s query 0x%02x failed.\n", __func__, c);
 		return -1;
@@ -107,8 +107,8 @@ static int elantech_read_reg(struct psmouse *psmouse, unsigned char reg,
 
 	switch (etd->hw_version) {
 	case 1:
-		if (psmouse_sliced_command(psmouse, ETP_REGISTER_READ) ||
-		    psmouse_sliced_command(psmouse, reg) ||
+		if (ps2_sliced_command(&psmouse->ps2dev, ETP_REGISTER_READ) ||
+		    ps2_sliced_command(&psmouse->ps2dev, reg) ||
 		    ps2_command(&psmouse->ps2dev, param, PSMOUSE_CMD_GETINFO)) {
 			rc = -1;
 		}
@@ -162,9 +162,9 @@ static int elantech_write_reg(struct psmouse *psmouse, unsigned char reg,
 
 	switch (etd->hw_version) {
 	case 1:
-		if (psmouse_sliced_command(psmouse, ETP_REGISTER_WRITE) ||
-		    psmouse_sliced_command(psmouse, reg) ||
-		    psmouse_sliced_command(psmouse, val) ||
+		if (ps2_sliced_command(&psmouse->ps2dev, ETP_REGISTER_WRITE) ||
+		    ps2_sliced_command(&psmouse->ps2dev, reg) ||
+		    ps2_sliced_command(&psmouse->ps2dev, val) ||
 		    ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_SETSCALE11)) {
 			rc = -1;
 		}
diff --git a/drivers/input/mouse/logips2pp.c b/drivers/input/mouse/logips2pp.c
index 7f4813fc3c6dd..51a156afa6c21 100644
--- a/drivers/input/mouse/logips2pp.c
+++ b/drivers/input/mouse/logips2pp.c
@@ -116,7 +116,7 @@ static int ps2pp_cmd(struct psmouse *psmouse, u8 *param, u8 command)
 {
 	int error;
 
-	error = psmouse_sliced_command(psmouse, command);
+	error = ps2_sliced_command(&psmouse->ps2dev, command);
 	if (error)
 		return error;
 
diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
index e3523143296ad..cbe12671f784c 100644
--- a/drivers/input/mouse/psmouse-base.c
+++ b/drivers/input/mouse/psmouse-base.c
@@ -429,32 +429,6 @@ static irqreturn_t psmouse_interrupt(struct serio *serio,
 	return IRQ_HANDLED;
 }
 
-/*
- * psmouse_sliced_command() sends an extended PS/2 command to the mouse
- * using sliced syntax, understood by advanced devices, such as Logitech
- * or Synaptics touchpads. The command is encoded as:
- * 0xE6 0xE8 rr 0xE8 ss 0xE8 tt 0xE8 uu where (rr*64)+(ss*16)+(tt*4)+uu
- * is the command.
- */
-int psmouse_sliced_command(struct psmouse *psmouse, u8 command)
-{
-	int i;
-	int error;
-
-	error = ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_SETSCALE11);
-	if (error)
-		return error;
-
-	for (i = 6; i >= 0; i -= 2) {
-		u8 d = (command >> i) & 3;
-		error = ps2_command(&psmouse->ps2dev, &d, PSMOUSE_CMD_SETRES);
-		if (error)
-			return error;
-	}
-
-	return 0;
-}
-
 /*
  * psmouse_reset() resets the mouse into power-on state.
  */
diff --git a/drivers/input/mouse/psmouse.h b/drivers/input/mouse/psmouse.h
index 8bc99691494e9..71ac50082c8b4 100644
--- a/drivers/input/mouse/psmouse.h
+++ b/drivers/input/mouse/psmouse.h
@@ -131,7 +131,6 @@ struct psmouse {
 
 void psmouse_queue_work(struct psmouse *psmouse, struct delayed_work *work,
 		unsigned long delay);
-int psmouse_sliced_command(struct psmouse *psmouse, unsigned char command);
 int psmouse_reset(struct psmouse *psmouse);
 void psmouse_set_state(struct psmouse *psmouse, enum psmouse_state new_state);
 void psmouse_set_resolution(struct psmouse *psmouse, unsigned int resolution);
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index 3d2e23a0ae39d..588408abc17e0 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -84,7 +84,7 @@ static int synaptics_mode_cmd(struct psmouse *psmouse, u8 mode)
 	u8 param[1];
 	int error;
 
-	error = psmouse_sliced_command(psmouse, mode);
+	error = ps2_sliced_command(&psmouse->ps2dev, mode);
 	if (error)
 		return error;
 
@@ -190,7 +190,7 @@ static int synaptics_send_cmd(struct psmouse *psmouse, u8 cmd, u8 *param)
 {
 	int error;
 
-	error = psmouse_sliced_command(psmouse, cmd);
+	error = ps2_sliced_command(&psmouse->ps2dev, cmd);
 	if (error)
 		return error;
 
@@ -547,7 +547,7 @@ static int synaptics_set_advanced_gesture_mode(struct psmouse *psmouse)
 	static u8 param = 0xc8;
 	int error;
 
-	error = psmouse_sliced_command(psmouse, SYN_QUE_MODEL);
+	error = ps2_sliced_command(&psmouse->ps2dev, SYN_QUE_MODEL);
 	if (error)
 		return error;
 
@@ -614,7 +614,7 @@ static int synaptics_pt_write(struct serio *serio, u8 c)
 	u8 rate_param = SYN_PS_CLIENT_CMD; /* indicates that we want pass-through port */
 	int error;
 
-	error = psmouse_sliced_command(parent, c);
+	error = ps2_sliced_command(&parent->ps2dev, c);
 	if (error)
 		return error;
 
diff --git a/drivers/input/serio/libps2.c b/drivers/input/serio/libps2.c
index c3712f0a47b50..e96ae477f0b56 100644
--- a/drivers/input/serio/libps2.c
+++ b/drivers/input/serio/libps2.c
@@ -269,6 +269,38 @@ int ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
 }
 EXPORT_SYMBOL(ps2_command);
 
+/*
+ * ps2_sliced_command() sends an extended PS/2 command to the mouse
+ * using sliced syntax, understood by advanced devices, such as Logitech
+ * or Synaptics touchpads. The command is encoded as:
+ * 0xE6 0xE8 rr 0xE8 ss 0xE8 tt 0xE8 uu where (rr*64)+(ss*16)+(tt*4)+uu
+ * is the command.
+ */
+
+int ps2_sliced_command(struct ps2dev *ps2dev, u8 command)
+{
+	int i;
+	int retval;
+
+	ps2_begin_command(ps2dev);
+
+	retval = __ps2_command(ps2dev, NULL, PS2_CMD_SETSCALE11);
+	if (retval)
+		goto out;
+
+	for (i = 6; i >= 0; i -= 2) {
+		u8 d = (command >> i) & 3;
+		retval = __ps2_command(ps2dev, &d, PS2_CMD_SETRES);
+		if (retval)
+			break;
+	}
+
+out:
+	ps2_end_command(ps2dev);
+	return retval;
+}
+EXPORT_SYMBOL(ps2_sliced_command);
+
 /*
  * ps2_init() initializes ps2dev structure
  */
diff --git a/include/linux/libps2.h b/include/linux/libps2.h
index 649295a5ff47d..cd4454502a042 100644
--- a/include/linux/libps2.h
+++ b/include/linux/libps2.h
@@ -11,6 +11,8 @@
  */
 
 
+#define PS2_CMD_SETSCALE11	0x00e6
+#define PS2_CMD_SETRES		0x10e8
 #define PS2_CMD_GETID		0x02f2
 #define PS2_CMD_RESET_BAT	0x02ff
 
@@ -48,6 +50,7 @@ void ps2_begin_command(struct ps2dev *ps2dev);
 void ps2_end_command(struct ps2dev *ps2dev);
 int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command);
 int ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command);
+int ps2_sliced_command(struct ps2dev *ps2dev, u8 command);
 bool ps2_handle_ack(struct ps2dev *ps2dev, u8 data);
 bool ps2_handle_response(struct ps2dev *ps2dev, u8 data);
 void ps2_cmd_aborted(struct ps2dev *ps2dev);
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 5/7] Input: libps2 - add debugging statements

[Dmitry Torokhov]

Debugging via i8042.debug and analyzing raw PS/2 data stream may be
cumbersome as you need to locate the boundaries of commands, decipher the
sliced commands, etc, etc. Let's add a bit more high level debug statements
for ps2_sendbyte(), ps2_command(), and ps2_sliced_command().

We do not introduce a new module parameter, but rater rely on the kernel
having dynamic debug facility enabled (which most everyone has nowadays).
Enable with:

echo "file libps2.c +pf" > /sys/kernel/debug/dynamic_debug/control

or add "libps2.dyndbg=+pf" to the kernel command line.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/serio/libps2.c | 52 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/input/serio/libps2.c b/drivers/input/serio/libps2.c
index e96ae477f0b56..82befae4dab04 100644
--- a/drivers/input/serio/libps2.c
+++ b/drivers/input/serio/libps2.c
@@ -26,22 +26,20 @@ MODULE_AUTHOR("Dmitry Torokhov <dtor@mail.ru>");
 MODULE_DESCRIPTION("PS/2 driver library");
 MODULE_LICENSE("GPL");
 
-/*
- * ps2_sendbyte() sends a byte to the device and waits for acknowledge.
- * It doesn't handle retransmission, though it could - because if there
- * is a need for retransmissions device has to be replaced anyway.
- *
- * ps2_sendbyte() can only be called from a process context.
- */
-
-int ps2_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
+static int ps2_do_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
 {
+	int error;
+
 	serio_pause_rx(ps2dev->serio);
 	ps2dev->nak = 1;
 	ps2dev->flags |= PS2_FLAG_ACK;
 	serio_continue_rx(ps2dev->serio);
 
-	if (serio_write(ps2dev->serio, byte) == 0)
+	error = serio_write(ps2dev->serio, byte);
+	if (error)
+		dev_dbg(&ps2dev->serio->dev,
+			"failed to write %#02x: %d\n", byte, error);
+	else
 		wait_event_timeout(ps2dev->wait,
 				   !(ps2dev->flags & PS2_FLAG_ACK),
 				   msecs_to_jiffies(timeout));
@@ -52,6 +50,24 @@ int ps2_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
 
 	return -ps2dev->nak;
 }
+
+/*
+ * ps2_sendbyte() sends a byte to the device and waits for acknowledge.
+ * It doesn't handle retransmission, though it could - because if there
+ * is a need for retransmissions device has to be replaced anyway.
+ *
+ * ps2_sendbyte() can only be called from a process context.
+ */
+
+int ps2_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
+{
+	int retval;
+
+	retval = ps2_do_sendbyte(ps2dev, byte, timeout);
+	dev_dbg(&ps2dev->serio->dev, "%02x - %x\n", byte, ps2dev->nak);
+
+	return retval;
+}
 EXPORT_SYMBOL(ps2_sendbyte);
 
 void ps2_begin_command(struct ps2dev *ps2dev)
@@ -186,6 +202,7 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
 	unsigned int receive = (command >> 8) & 0xf;
 	int rc = -1;
 	int i;
+	u8 send_param[16];
 
 	if (receive > sizeof(ps2dev->cmdbuf)) {
 		WARN_ON(1);
@@ -197,6 +214,8 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
 		return -1;
 	}
 
+	memcpy(send_param, param, send);
+
 	serio_pause_rx(ps2dev->serio);
 	ps2dev->flags = command == PS2_CMD_GETID ? PS2_FLAG_WAITID : 0;
 	ps2dev->cmdcnt = receive;
@@ -210,14 +229,14 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
 	 * ACKing the reset command, and so it can take a long
 	 * time before the ACK arrives.
 	 */
-	if (ps2_sendbyte(ps2dev, command & 0xff,
-			 command == PS2_CMD_RESET_BAT ? 1000 : 200)) {
+	if (ps2_do_sendbyte(ps2dev, command & 0xff,
+			    command == PS2_CMD_RESET_BAT ? 1000 : 200)) {
 		serio_pause_rx(ps2dev->serio);
 		goto out_reset_flags;
 	}
 
 	for (i = 0; i < send; i++) {
-		if (ps2_sendbyte(ps2dev, param[i], 200)) {
+		if (ps2_do_sendbyte(ps2dev, param[i], 200)) {
 			serio_pause_rx(ps2dev->serio);
 			goto out_reset_flags;
 		}
@@ -253,6 +272,12 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
 	ps2dev->flags = 0;
 	serio_continue_rx(ps2dev->serio);
 
+	dev_dbg(&ps2dev->serio->dev,
+		"%02x [%*ph] - %x/%08lx [%*ph]\n",
+		command & 0xff, send, send_param,
+		ps2dev->nak, ps2dev->flags,
+		receive, param ?: send_param);
+
 	return rc;
 }
 EXPORT_SYMBOL(__ps2_command);
@@ -296,6 +321,7 @@ int ps2_sliced_command(struct ps2dev *ps2dev, u8 command)
 	}
 
 out:
+	dev_dbg(&ps2dev->serio->dev, "%02x - %d\n", command, retval);
 	ps2_end_command(ps2dev);
 	return retval;
 }
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 6/7] Input: libps2 - support retransmission of command data

[Dmitry Torokhov]

The devices are allowed to respond to either command byte or command
parameter with a NAK (0xfe), and the host is supposed to resend the
"correct" byte. The device then will either respond with ACK or ERR (0xfc).
Let's teach libps2 to handle the NAK responses properly, so that individual
drivers do not need to handle them.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/serio/libps2.c | 103 +++++++++++++++++++++++++++++--------------
 1 file changed, 71 insertions(+), 32 deletions(-)

diff --git a/drivers/input/serio/libps2.c b/drivers/input/serio/libps2.c
index 82befae4dab04..f05c407b31f3d 100644
--- a/drivers/input/serio/libps2.c
+++ b/drivers/input/serio/libps2.c
@@ -26,35 +26,63 @@ MODULE_AUTHOR("Dmitry Torokhov <dtor@mail.ru>");
 MODULE_DESCRIPTION("PS/2 driver library");
 MODULE_LICENSE("GPL");
 
-static int ps2_do_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
+static int ps2_do_sendbyte(struct ps2dev *ps2dev, u8 byte,
+			   unsigned int timeout, unsigned int max_attempts)
+	__releases(&ps2dev->serio->lock) __acquires(&ps2dev->serio->lock)
 {
+	int attempt = 0;
 	int error;
 
-	serio_pause_rx(ps2dev->serio);
-	ps2dev->nak = 1;
-	ps2dev->flags |= PS2_FLAG_ACK;
-	serio_continue_rx(ps2dev->serio);
+	lockdep_assert_held(&ps2dev->serio->lock);
 
-	error = serio_write(ps2dev->serio, byte);
-	if (error)
-		dev_dbg(&ps2dev->serio->dev,
-			"failed to write %#02x: %d\n", byte, error);
-	else
-		wait_event_timeout(ps2dev->wait,
-				   !(ps2dev->flags & PS2_FLAG_ACK),
-				   msecs_to_jiffies(timeout));
+	do {
+		ps2dev->nak = 1;
+		ps2dev->flags |= PS2_FLAG_ACK;
+
+		serio_continue_rx(ps2dev->serio);
+
+		error = serio_write(ps2dev->serio, byte);
+		if (error)
+			dev_dbg(&ps2dev->serio->dev,
+				"failed to write %#02x: %d\n", byte, error);
+		else
+			wait_event_timeout(ps2dev->wait,
+					   !(ps2dev->flags & PS2_FLAG_ACK),
+					   msecs_to_jiffies(timeout));
+
+		serio_pause_rx(ps2dev->serio);
+	} while (ps2dev->nak == PS2_RET_NAK && ++attempt < max_attempts);
 
-	serio_pause_rx(ps2dev->serio);
 	ps2dev->flags &= ~PS2_FLAG_ACK;
-	serio_continue_rx(ps2dev->serio);
 
-	return -ps2dev->nak;
+	if (!error) {
+		switch (ps2dev->nak) {
+		case 0:
+			break;
+		case PS2_RET_NAK:
+			error = -EAGAIN;
+			break;
+		case PS2_RET_ERR:
+			error = -EPROTO;
+			break;
+		default:
+			error = -EIO;
+			break;
+		}
+	}
+
+	if (error || attempt > 1)
+		dev_dbg(&ps2dev->serio->dev,
+			"%02x - %d (%x), attempt %d\n",
+			byte, error, ps2dev->nak, attempt);
+
+	return error;
 }
 
 /*
  * ps2_sendbyte() sends a byte to the device and waits for acknowledge.
- * It doesn't handle retransmission, though it could - because if there
- * is a need for retransmissions device has to be replaced anyway.
+ * It doesn't handle retransmission, the caller is expected to handle
+ * it when needed.
  *
  * ps2_sendbyte() can only be called from a process context.
  */
@@ -63,9 +91,13 @@ int ps2_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
 {
 	int retval;
 
-	retval = ps2_do_sendbyte(ps2dev, byte, timeout);
+	serio_pause_rx(ps2dev->serio);
+
+	retval = ps2_do_sendbyte(ps2dev, byte, timeout, 1);
 	dev_dbg(&ps2dev->serio->dev, "%02x - %x\n", byte, ps2dev->nak);
 
+	serio_continue_rx(ps2dev->serio);
+
 	return retval;
 }
 EXPORT_SYMBOL(ps2_sendbyte);
@@ -200,48 +232,48 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
 	unsigned int timeout;
 	unsigned int send = (command >> 12) & 0xf;
 	unsigned int receive = (command >> 8) & 0xf;
-	int rc = -1;
+	int rc;
 	int i;
 	u8 send_param[16];
 
 	if (receive > sizeof(ps2dev->cmdbuf)) {
 		WARN_ON(1);
-		return -1;
+		return -EINVAL;
 	}
 
 	if (send && !param) {
 		WARN_ON(1);
-		return -1;
+		return -EINVAL;
 	}
 
 	memcpy(send_param, param, send);
 
 	serio_pause_rx(ps2dev->serio);
+
 	ps2dev->flags = command == PS2_CMD_GETID ? PS2_FLAG_WAITID : 0;
 	ps2dev->cmdcnt = receive;
 	if (receive && param)
 		for (i = 0; i < receive; i++)
 			ps2dev->cmdbuf[(receive - 1) - i] = param[i];
-	serio_continue_rx(ps2dev->serio);
 
 	/*
 	 * Some devices (Synaptics) peform the reset before
 	 * ACKing the reset command, and so it can take a long
 	 * time before the ACK arrives.
 	 */
-	if (ps2_do_sendbyte(ps2dev, command & 0xff,
-			    command == PS2_CMD_RESET_BAT ? 1000 : 200)) {
-		serio_pause_rx(ps2dev->serio);
+	rc = ps2_do_sendbyte(ps2dev, command & 0xff,
+			     command == PS2_CMD_RESET_BAT ? 1000 : 200, 2);
+	if (rc)
 		goto out_reset_flags;
-	}
 
 	for (i = 0; i < send; i++) {
-		if (ps2_do_sendbyte(ps2dev, param[i], 200)) {
-			serio_pause_rx(ps2dev->serio);
+		rc = ps2_do_sendbyte(ps2dev, param[i], 200, 2);
+		if (rc)
 			goto out_reset_flags;
-		}
 	}
 
+	serio_continue_rx(ps2dev->serio);
+
 	/*
 	 * The reset command takes a long time to execute.
 	 */
@@ -263,8 +295,11 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
 		for (i = 0; i < receive; i++)
 			param[i] = ps2dev->cmdbuf[(receive - 1) - i];
 
-	if (ps2dev->cmdcnt && (command != PS2_CMD_RESET_BAT || ps2dev->cmdcnt != 1))
+	if (ps2dev->cmdcnt &&
+	    (command != PS2_CMD_RESET_BAT || ps2dev->cmdcnt != 1)) {
+		rc = -EPROTO;
 		goto out_reset_flags;
+	}
 
 	rc = 0;
 
@@ -278,7 +313,11 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
 		ps2dev->nak, ps2dev->flags,
 		receive, param ?: send_param);
 
-	return rc;
+	/*
+	 * ps_command() handles resends itself, so do not leak -EAGAIN
+	 * to the callers.
+	 */
+	return rc != -EAGAIN ? rc : -EPROTO;
 }
 EXPORT_SYMBOL(__ps2_command);
 
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 7/7] Input: libps2 - relax command byte ACK handling

[Dmitry Torokhov]

When we probe PS/2 devices we first issue "Get ID" command and only if we
receive what we consider a valid keyboard or mouse ID we disable the device
and continue with protocol detection. That means that the device may be
transmitting motion or keystroke data, while we expect ACK response.

Instead of signaling failure if we see anything but ACK/NAK let's ignore
"garbage" response until we see ACK for the command byte (first byte). The
checks for subsequent ACKs of command parameters will continue be strict.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/serio/libps2.c | 25 ++++++++++++++++++++++---
 include/linux/libps2.h       |  1 +
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/input/serio/libps2.c b/drivers/input/serio/libps2.c
index f05c407b31f3d..e6a07e68d1ff6 100644
--- a/drivers/input/serio/libps2.c
+++ b/drivers/input/serio/libps2.c
@@ -256,16 +256,23 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
 		for (i = 0; i < receive; i++)
 			ps2dev->cmdbuf[(receive - 1) - i] = param[i];
 
+	/* Signal that we are sending the command byte */
+	ps2dev->flags |= PS2_FLAG_ACK_CMD;
+
 	/*
 	 * Some devices (Synaptics) peform the reset before
 	 * ACKing the reset command, and so it can take a long
 	 * time before the ACK arrives.
 	 */
-	rc = ps2_do_sendbyte(ps2dev, command & 0xff,
-			     command == PS2_CMD_RESET_BAT ? 1000 : 200, 2);
+	timeout = command == PS2_CMD_RESET_BAT ? 1000 : 200;
+
+	rc = ps2_do_sendbyte(ps2dev, command & 0xff, timeout, 2);
 	if (rc)
 		goto out_reset_flags;
 
+	/* Now we are sending command parameters, if any */
+	ps2dev->flags &= ~PS2_FLAG_ACK_CMD;
+
 	for (i = 0; i < send; i++) {
 		rc = ps2_do_sendbyte(ps2dev, param[i], 200, 2);
 		if (rc)
@@ -416,7 +423,19 @@ bool ps2_handle_ack(struct ps2dev *ps2dev, u8 data)
 		}
 		/* Fall through */
 	default:
-		return false;
+		/*
+		 * Do not signal errors if we get unexpected reply while
+		 * waiting for an ACK to the initial (first) command byte:
+		 * the device might not be quiesced yet and continue
+		 * delivering data.
+		 * Note that we reset PS2_FLAG_WAITID flag, so the workaround
+		 * for mice not acknowledging the Get ID command only triggers
+		 * on the 1st byte; if device spews data we really want to see
+		 * a real ACK from it.
+		 */
+		dev_dbg(&ps2dev->serio->dev, "unexpected %#02x\n", data);
+		ps2dev->flags &= ~PS2_FLAG_WAITID;
+		return ps2dev->flags & PS2_FLAG_ACK_CMD;
 	}
 
 	if (!ps2dev->nak) {
diff --git a/include/linux/libps2.h b/include/linux/libps2.h
index cd4454502a042..8b8139337e552 100644
--- a/include/linux/libps2.h
+++ b/include/linux/libps2.h
@@ -27,6 +27,7 @@
 #define PS2_FLAG_CMD1		BIT(2)	/* Waiting for the first byte of command response */
 #define PS2_FLAG_WAITID		BIT(3)	/* Command executing is GET ID */
 #define PS2_FLAG_NAK		BIT(4)	/* Last transmission was NAKed */
+#define PS2_FLAG_ACK_CMD	BIT(5)	/* Waiting to ACK the command (first) byte */
 
 struct ps2dev {
 	struct serio *serio;
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH 3/7] Input: libps2 - use BIT() for bitmask constants

[Randy Dunlap]

On 01/19/2018 11:41 AM, Dmitry Torokhov wrote:
> Let's explicitly document bit numbers with BIT() macro.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Makes sense, but something should
#include <linux/bitops.h>

per Documentation/process/submit-checklist.rst Rule #1:
1) If you use a facility then #include the file that defines/declares
   that facility.  Don't depend on other header files pulling in ones
   that you use.


> ---
>  include/linux/libps2.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/libps2.h b/include/linux/libps2.h
> index 365c50b097ded..649295a5ff47d 100644
> --- a/include/linux/libps2.h
> +++ b/include/linux/libps2.h
> @@ -20,11 +20,11 @@
>  #define PS2_RET_NAK		0xfe
>  #define PS2_RET_ERR		0xfc
>  
> -#define PS2_FLAG_ACK		1	/* Waiting for ACK/NAK */
> -#define PS2_FLAG_CMD		2	/* Waiting for command to finish */
> -#define PS2_FLAG_CMD1		4	/* Waiting for the first byte of command response */
> -#define PS2_FLAG_WAITID		8	/* Command execiting is GET ID */
> -#define PS2_FLAG_NAK		16	/* Last transmission was NAKed */
> +#define PS2_FLAG_ACK		BIT(0)	/* Waiting for ACK/NAK */
> +#define PS2_FLAG_CMD		BIT(1)	/* Waiting for a command to finish */
> +#define PS2_FLAG_CMD1		BIT(2)	/* Waiting for the first byte of command response */
> +#define PS2_FLAG_WAITID		BIT(3)	/* Command executing is GET ID */
> +#define PS2_FLAG_NAK		BIT(4)	/* Last transmission was NAKed */
>  
>  struct ps2dev {
>  	struct serio *serio;
> 


-- 
~Randy

Re: [PATCH 3/7] Input: libps2 - use BIT() for bitmask constants

[Dmitry Torokhov]

On Fri, Jan 19, 2018 at 02:26:20PM -0800, Randy Dunlap wrote:
> On 01/19/2018 11:41 AM, Dmitry Torokhov wrote:
> > Let's explicitly document bit numbers with BIT() macro.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Makes sense, but something should
> #include <linux/bitops.h>

Indeed, I'll add it, thanks.

> 
> per Documentation/process/submit-checklist.rst Rule #1:
> 1) If you use a facility then #include the file that defines/declares
>    that facility.  Don't depend on other header files pulling in ones
>    that you use.
> 
> 
> > ---
> >  include/linux/libps2.h | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/linux/libps2.h b/include/linux/libps2.h
> > index 365c50b097ded..649295a5ff47d 100644
> > --- a/include/linux/libps2.h
> > +++ b/include/linux/libps2.h
> > @@ -20,11 +20,11 @@
> >  #define PS2_RET_NAK		0xfe
> >  #define PS2_RET_ERR		0xfc
> >  
> > -#define PS2_FLAG_ACK		1	/* Waiting for ACK/NAK */
> > -#define PS2_FLAG_CMD		2	/* Waiting for command to finish */
> > -#define PS2_FLAG_CMD1		4	/* Waiting for the first byte of command response */
> > -#define PS2_FLAG_WAITID		8	/* Command execiting is GET ID */
> > -#define PS2_FLAG_NAK		16	/* Last transmission was NAKed */
> > +#define PS2_FLAG_ACK		BIT(0)	/* Waiting for ACK/NAK */
> > +#define PS2_FLAG_CMD		BIT(1)	/* Waiting for a command to finish */
> > +#define PS2_FLAG_CMD1		BIT(2)	/* Waiting for the first byte of command response */
> > +#define PS2_FLAG_WAITID		BIT(3)	/* Command executing is GET ID */
> > +#define PS2_FLAG_NAK		BIT(4)	/* Last transmission was NAKed */
> >  
> >  struct ps2dev {
> >  	struct serio *serio;
> > 
> 
> 
> -- 
> ~Randy

-- 
Dmitry

Re: [PATCH 5/7] Input: libps2 - add debugging statements

[ulrik.debie-os]

Hi,

the patch series looks nice, but best to also include the procedure
described here in Documentation/input somewhere. I would not expect
everyone to dig into git commit logs to find the howto :)

Best regards,
Ulrik


On Fri, Jan 19, 2018 at 11:41:09AM -0800, Dmitry Torokhov wrote:
> Date:   Fri, 19 Jan 2018 11:41:09 -0800
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> To: Benjamin Tissoires <benjamin.tissoires@redhat.com>, Hans de Goede
>  <hdegoede@redhat.com>, Lyude Paul <lyude@redhat.com>
> Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
> Subject: [PATCH 5/7] Input: libps2 - add debugging statements
> X-Mailer: git-send-email 2.16.0.rc1.238.g530d649a79-goog
> X-Mailing-List: linux-input@vger.kernel.org
> 
> Debugging via i8042.debug and analyzing raw PS/2 data stream may be
> cumbersome as you need to locate the boundaries of commands, decipher the
> sliced commands, etc, etc. Let's add a bit more high level debug statements
> for ps2_sendbyte(), ps2_command(), and ps2_sliced_command().
> 
> We do not introduce a new module parameter, but rater rely on the kernel
> having dynamic debug facility enabled (which most everyone has nowadays).
> Enable with:
> 
> echo "file libps2.c +pf" > /sys/kernel/debug/dynamic_debug/control
> 
> or add "libps2.dyndbg=+pf" to the kernel command line.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/serio/libps2.c | 52 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/input/serio/libps2.c b/drivers/input/serio/libps2.c
> index e96ae477f0b56..82befae4dab04 100644
> --- a/drivers/input/serio/libps2.c
> +++ b/drivers/input/serio/libps2.c
> @@ -26,22 +26,20 @@ MODULE_AUTHOR("Dmitry Torokhov <dtor@mail.ru>");
>  MODULE_DESCRIPTION("PS/2 driver library");
>  MODULE_LICENSE("GPL");
>  
> -/*
> - * ps2_sendbyte() sends a byte to the device and waits for acknowledge.
> - * It doesn't handle retransmission, though it could - because if there
> - * is a need for retransmissions device has to be replaced anyway.
> - *
> - * ps2_sendbyte() can only be called from a process context.
> - */
> -
> -int ps2_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
> +static int ps2_do_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
>  {
> +	int error;
> +
>  	serio_pause_rx(ps2dev->serio);
>  	ps2dev->nak = 1;
>  	ps2dev->flags |= PS2_FLAG_ACK;
>  	serio_continue_rx(ps2dev->serio);
>  
> -	if (serio_write(ps2dev->serio, byte) == 0)
> +	error = serio_write(ps2dev->serio, byte);
> +	if (error)
> +		dev_dbg(&ps2dev->serio->dev,
> +			"failed to write %#02x: %d\n", byte, error);
> +	else
>  		wait_event_timeout(ps2dev->wait,
>  				   !(ps2dev->flags & PS2_FLAG_ACK),
>  				   msecs_to_jiffies(timeout));
> @@ -52,6 +50,24 @@ int ps2_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
>  
>  	return -ps2dev->nak;
>  }
> +
> +/*
> + * ps2_sendbyte() sends a byte to the device and waits for acknowledge.
> + * It doesn't handle retransmission, though it could - because if there
> + * is a need for retransmissions device has to be replaced anyway.
> + *
> + * ps2_sendbyte() can only be called from a process context.
> + */
> +
> +int ps2_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
> +{
> +	int retval;
> +
> +	retval = ps2_do_sendbyte(ps2dev, byte, timeout);
> +	dev_dbg(&ps2dev->serio->dev, "%02x - %x\n", byte, ps2dev->nak);
> +
> +	return retval;
> +}
>  EXPORT_SYMBOL(ps2_sendbyte);
>  
>  void ps2_begin_command(struct ps2dev *ps2dev)
> @@ -186,6 +202,7 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
>  	unsigned int receive = (command >> 8) & 0xf;
>  	int rc = -1;
>  	int i;
> +	u8 send_param[16];
>  
>  	if (receive > sizeof(ps2dev->cmdbuf)) {
>  		WARN_ON(1);
> @@ -197,6 +214,8 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
>  		return -1;
>  	}
>  
> +	memcpy(send_param, param, send);
> +
>  	serio_pause_rx(ps2dev->serio);
>  	ps2dev->flags = command == PS2_CMD_GETID ? PS2_FLAG_WAITID : 0;
>  	ps2dev->cmdcnt = receive;
> @@ -210,14 +229,14 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
>  	 * ACKing the reset command, and so it can take a long
>  	 * time before the ACK arrives.
>  	 */
> -	if (ps2_sendbyte(ps2dev, command & 0xff,
> -			 command == PS2_CMD_RESET_BAT ? 1000 : 200)) {
> +	if (ps2_do_sendbyte(ps2dev, command & 0xff,
> +			    command == PS2_CMD_RESET_BAT ? 1000 : 200)) {
>  		serio_pause_rx(ps2dev->serio);
>  		goto out_reset_flags;
>  	}
>  
>  	for (i = 0; i < send; i++) {
> -		if (ps2_sendbyte(ps2dev, param[i], 200)) {
> +		if (ps2_do_sendbyte(ps2dev, param[i], 200)) {
>  			serio_pause_rx(ps2dev->serio);
>  			goto out_reset_flags;
>  		}
> @@ -253,6 +272,12 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
>  	ps2dev->flags = 0;
>  	serio_continue_rx(ps2dev->serio);
>  
> +	dev_dbg(&ps2dev->serio->dev,
> +		"%02x [%*ph] - %x/%08lx [%*ph]\n",
> +		command & 0xff, send, send_param,
> +		ps2dev->nak, ps2dev->flags,
> +		receive, param ?: send_param);
> +
>  	return rc;
>  }
>  EXPORT_SYMBOL(__ps2_command);
> @@ -296,6 +321,7 @@ int ps2_sliced_command(struct ps2dev *ps2dev, u8 command)
>  	}
>  
>  out:
> +	dev_dbg(&ps2dev->serio->dev, "%02x - %d\n", command, retval);
>  	ps2_end_command(ps2dev);
>  	return retval;
>  }
> -- 
> 2.16.0.rc1.238.g530d649a79-goog
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] Input: libps2 - add debugging statements

[Dmitry Torokhov]

H Ulrik,

On Sun, Jan 21, 2018 at 09:22:31PM +0100, ulrik.debie-os@e2big.org wrote:
> 
> Hi,
> 
> the patch series looks nice, but best to also include the procedure
> described here in Documentation/input somewhere. I would not expect
> everyone to dig into git commit logs to find the howto :)

Right now we do not have any notes in kernel on debugging PS/2 devices,
although we have a bunch of tools (ps2emu), etc. I wonder if I can get
Benjamin or Lyude or Hans or Peter write something up as they've been
doing a lot of touchpad bringups...

Thanks.

> 
> Best regards,
> Ulrik
> 
> 
> On Fri, Jan 19, 2018 at 11:41:09AM -0800, Dmitry Torokhov wrote:
> > Date:   Fri, 19 Jan 2018 11:41:09 -0800
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > To: Benjamin Tissoires <benjamin.tissoires@redhat.com>, Hans de Goede
> >  <hdegoede@redhat.com>, Lyude Paul <lyude@redhat.com>
> > Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
> > Subject: [PATCH 5/7] Input: libps2 - add debugging statements
> > X-Mailer: git-send-email 2.16.0.rc1.238.g530d649a79-goog
> > X-Mailing-List: linux-input@vger.kernel.org
> > 
> > Debugging via i8042.debug and analyzing raw PS/2 data stream may be
> > cumbersome as you need to locate the boundaries of commands, decipher the
> > sliced commands, etc, etc. Let's add a bit more high level debug statements
> > for ps2_sendbyte(), ps2_command(), and ps2_sliced_command().
> > 
> > We do not introduce a new module parameter, but rater rely on the kernel
> > having dynamic debug facility enabled (which most everyone has nowadays).
> > Enable with:
> > 
> > echo "file libps2.c +pf" > /sys/kernel/debug/dynamic_debug/control
> > 
> > or add "libps2.dyndbg=+pf" to the kernel command line.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/input/serio/libps2.c | 52 +++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 39 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/input/serio/libps2.c b/drivers/input/serio/libps2.c
> > index e96ae477f0b56..82befae4dab04 100644
> > --- a/drivers/input/serio/libps2.c
> > +++ b/drivers/input/serio/libps2.c
> > @@ -26,22 +26,20 @@ MODULE_AUTHOR("Dmitry Torokhov <dtor@mail.ru>");
> >  MODULE_DESCRIPTION("PS/2 driver library");
> >  MODULE_LICENSE("GPL");
> >  
> > -/*
> > - * ps2_sendbyte() sends a byte to the device and waits for acknowledge.
> > - * It doesn't handle retransmission, though it could - because if there
> > - * is a need for retransmissions device has to be replaced anyway.
> > - *
> > - * ps2_sendbyte() can only be called from a process context.
> > - */
> > -
> > -int ps2_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
> > +static int ps2_do_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
> >  {
> > +	int error;
> > +
> >  	serio_pause_rx(ps2dev->serio);
> >  	ps2dev->nak = 1;
> >  	ps2dev->flags |= PS2_FLAG_ACK;
> >  	serio_continue_rx(ps2dev->serio);
> >  
> > -	if (serio_write(ps2dev->serio, byte) == 0)
> > +	error = serio_write(ps2dev->serio, byte);
> > +	if (error)
> > +		dev_dbg(&ps2dev->serio->dev,
> > +			"failed to write %#02x: %d\n", byte, error);
> > +	else
> >  		wait_event_timeout(ps2dev->wait,
> >  				   !(ps2dev->flags & PS2_FLAG_ACK),
> >  				   msecs_to_jiffies(timeout));
> > @@ -52,6 +50,24 @@ int ps2_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
> >  
> >  	return -ps2dev->nak;
> >  }
> > +
> > +/*
> > + * ps2_sendbyte() sends a byte to the device and waits for acknowledge.
> > + * It doesn't handle retransmission, though it could - because if there
> > + * is a need for retransmissions device has to be replaced anyway.
> > + *
> > + * ps2_sendbyte() can only be called from a process context.
> > + */
> > +
> > +int ps2_sendbyte(struct ps2dev *ps2dev, u8 byte, unsigned int timeout)
> > +{
> > +	int retval;
> > +
> > +	retval = ps2_do_sendbyte(ps2dev, byte, timeout);
> > +	dev_dbg(&ps2dev->serio->dev, "%02x - %x\n", byte, ps2dev->nak);
> > +
> > +	return retval;
> > +}
> >  EXPORT_SYMBOL(ps2_sendbyte);
> >  
> >  void ps2_begin_command(struct ps2dev *ps2dev)
> > @@ -186,6 +202,7 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
> >  	unsigned int receive = (command >> 8) & 0xf;
> >  	int rc = -1;
> >  	int i;
> > +	u8 send_param[16];
> >  
> >  	if (receive > sizeof(ps2dev->cmdbuf)) {
> >  		WARN_ON(1);
> > @@ -197,6 +214,8 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
> >  		return -1;
> >  	}
> >  
> > +	memcpy(send_param, param, send);
> > +
> >  	serio_pause_rx(ps2dev->serio);
> >  	ps2dev->flags = command == PS2_CMD_GETID ? PS2_FLAG_WAITID : 0;
> >  	ps2dev->cmdcnt = receive;
> > @@ -210,14 +229,14 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
> >  	 * ACKing the reset command, and so it can take a long
> >  	 * time before the ACK arrives.
> >  	 */
> > -	if (ps2_sendbyte(ps2dev, command & 0xff,
> > -			 command == PS2_CMD_RESET_BAT ? 1000 : 200)) {
> > +	if (ps2_do_sendbyte(ps2dev, command & 0xff,
> > +			    command == PS2_CMD_RESET_BAT ? 1000 : 200)) {
> >  		serio_pause_rx(ps2dev->serio);
> >  		goto out_reset_flags;
> >  	}
> >  
> >  	for (i = 0; i < send; i++) {
> > -		if (ps2_sendbyte(ps2dev, param[i], 200)) {
> > +		if (ps2_do_sendbyte(ps2dev, param[i], 200)) {
> >  			serio_pause_rx(ps2dev->serio);
> >  			goto out_reset_flags;
> >  		}
> > @@ -253,6 +272,12 @@ int __ps2_command(struct ps2dev *ps2dev, u8 *param, unsigned int command)
> >  	ps2dev->flags = 0;
> >  	serio_continue_rx(ps2dev->serio);
> >  
> > +	dev_dbg(&ps2dev->serio->dev,
> > +		"%02x [%*ph] - %x/%08lx [%*ph]\n",
> > +		command & 0xff, send, send_param,
> > +		ps2dev->nak, ps2dev->flags,
> > +		receive, param ?: send_param);
> > +
> >  	return rc;
> >  }
> >  EXPORT_SYMBOL(__ps2_command);
> > @@ -296,6 +321,7 @@ int ps2_sliced_command(struct ps2dev *ps2dev, u8 command)
> >  	}
> >  
> >  out:
> > +	dev_dbg(&ps2dev->serio->dev, "%02x - %d\n", command, retval);
> >  	ps2_end_command(ps2dev);
> >  	return retval;
> >  }
> > -- 
> > 2.16.0.rc1.238.g530d649a79-goog
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-input" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Dmitry