[PATCH for v5.5 0/2] input/rmi4 regression fix

[PATCH for v5.5 0/2] input/rmi4 regression fix

[Hans Verkuil]

Hi Dmitry,

Timo Kaufmann reported that the "don't increment rmiaddr" patch broke
his Thinkpad T450 (the cursor started drifting after a while).

Some more digging revealed that that patch was bad, and that the
real solution for the original problem (function F54 reported bad
data) had to be done in rmi_f54.c.

So the first patch reverts the old fix (and so will break F54 again),
and this should be merged for v5.5 asap. The second patch that fixes
F54 can be postponed to v5.6: F54 is rarely used and I don't think it
is important enough for v5.5.

I have only tested the F54 fix for smbus, I don't know if it is OK
for i2c/spi since I have no hardware to test that. It seems sane :-)

Regards,

	Hans

Hans Verkuil (2):
  Revert "Input: synaptics-rmi4 - don't increment rmiaddr for SMBus
    transfers"
  Input: rmi_f54: read from FIFO in 32 byte blocks

 drivers/input/rmi4/rmi_f54.c   | 43 +++++++++++++++++++++-------------
 drivers/input/rmi4/rmi_smbus.c |  2 ++
 2 files changed, 29 insertions(+), 16 deletions(-)

-- 
2.24.0

[PATCH for v5.5 1/2] Revert "Input: synaptics-rmi4 - don't increment rmiaddr for SMBus transfers"

[Hans Verkuil]

This reverts commit a284e11c371e446371675668d8c8120a27227339.

This causes problems (drifting cursor) with at least the F11 function that
reads more than 32 bytes.

The real issue is in the F54 driver, and so this should be fixed there, and
not in rmi_smbus.c.

So first revert this bad commit, then fix the real problem in F54 in another
patch.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reported-by: Timo Kaufmann <timokau@zoho.com>
Fixes: a284e11c371e ("Input: synaptics-rmi4 - don't increment rmiaddr for SMBus transfers")
Cc: stable@vger.kernel.org
---
 drivers/input/rmi4/rmi_smbus.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
index b313c579914f..2407ea43de59 100644
--- a/drivers/input/rmi4/rmi_smbus.c
+++ b/drivers/input/rmi4/rmi_smbus.c
@@ -163,6 +163,7 @@ static int rmi_smb_write_block(struct rmi_transport_dev *xport, u16 rmiaddr,
 		/* prepare to write next block of bytes */
 		cur_len -= SMB_MAX_COUNT;
 		databuff += SMB_MAX_COUNT;
+		rmiaddr += SMB_MAX_COUNT;
 	}
 exit:
 	mutex_unlock(&rmi_smb->page_mutex);
@@ -214,6 +215,7 @@ static int rmi_smb_read_block(struct rmi_transport_dev *xport, u16 rmiaddr,
 		/* prepare to read next block of bytes */
 		cur_len -= SMB_MAX_COUNT;
 		databuff += SMB_MAX_COUNT;
+		rmiaddr += SMB_MAX_COUNT;
 	}
 
 	retval = 0;
-- 
2.24.0

Re: [PATCH for v5.5 1/2] Revert "Input: synaptics-rmi4 - don't increment rmiaddr for SMBus transfers"

[Dmitry Torokhov]

On Wed, Jan 15, 2020 at 01:48:18PM +0100, Hans Verkuil wrote:
> This reverts commit a284e11c371e446371675668d8c8120a27227339.
> 
> This causes problems (drifting cursor) with at least the F11 function that
> reads more than 32 bytes.
> 
> The real issue is in the F54 driver, and so this should be fixed there, and
> not in rmi_smbus.c.
> 
> So first revert this bad commit, then fix the real problem in F54 in another
> patch.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reported-by: Timo Kaufmann <timokau@zoho.com>
> Fixes: a284e11c371e ("Input: synaptics-rmi4 - don't increment rmiaddr for SMBus transfers")
> Cc: stable@vger.kernel.org

Applied, thank you.

> ---
>  drivers/input/rmi4/rmi_smbus.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
> index b313c579914f..2407ea43de59 100644
> --- a/drivers/input/rmi4/rmi_smbus.c
> +++ b/drivers/input/rmi4/rmi_smbus.c
> @@ -163,6 +163,7 @@ static int rmi_smb_write_block(struct rmi_transport_dev *xport, u16 rmiaddr,
>  		/* prepare to write next block of bytes */
>  		cur_len -= SMB_MAX_COUNT;
>  		databuff += SMB_MAX_COUNT;
> +		rmiaddr += SMB_MAX_COUNT;
>  	}
>  exit:
>  	mutex_unlock(&rmi_smb->page_mutex);
> @@ -214,6 +215,7 @@ static int rmi_smb_read_block(struct rmi_transport_dev *xport, u16 rmiaddr,
>  		/* prepare to read next block of bytes */
>  		cur_len -= SMB_MAX_COUNT;
>  		databuff += SMB_MAX_COUNT;
> +		rmiaddr += SMB_MAX_COUNT;
>  	}
>  
>  	retval = 0;
> -- 
> 2.24.0
> 

-- 
Dmitry

[PATCH for v5.5 2/2] Input: rmi_f54: read from FIFO in 32 byte blocks

[Hans Verkuil]

The F54 Report Data is apparently read through a fifo and for
the smbus protocol that means that between reading a block of 32
bytes the rmiaddr shouldn't be incremented. However, changing
that causes other non-fifo reads to fail and so that change was
reverted.

This patch changes just the F54 function and it now reads 32 bytes
at a time from the fifo, using the F54_FIFO_OFFSET to update the
start address that is used when reading from the fifo.

This has only been tested with smbus, not with i2c or spi. But I
suspect that the same is needed there since I think similar
problems will occur there when reading more than 256 bytes.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reported-by: Timo Kaufmann <timokau@zoho.com>
Fixes: a284e11c371e ("Input: synaptics-rmi4 - don't increment rmiaddr for SMBus transfers")
Cc: stable@vger.kernel.org
---
 drivers/input/rmi4/rmi_f54.c | 43 ++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
index 0bc01cfc2b51..6b23e679606e 100644
--- a/drivers/input/rmi4/rmi_f54.c
+++ b/drivers/input/rmi4/rmi_f54.c
@@ -24,6 +24,12 @@
 #define F54_NUM_TX_OFFSET       1
 #define F54_NUM_RX_OFFSET       0
 
+/*
+ * The smbus protocol can read only 32 bytes max at a time.
+ * But this should be fine for i2c/spi as well.
+ */
+#define F54_REPORT_DATA_SIZE	32
+
 /* F54 commands */
 #define F54_GET_REPORT          1
 #define F54_FORCE_CAL           2
@@ -526,6 +532,7 @@ static void rmi_f54_work(struct work_struct *work)
 	int report_size;
 	u8 command;
 	int error;
+	int i;
 
 	report_size = rmi_f54_get_report_size(f54);
 	if (report_size == 0) {
@@ -558,23 +565,27 @@ static void rmi_f54_work(struct work_struct *work)
 
 	rmi_dbg(RMI_DEBUG_FN, &fn->dev, "Get report command completed, reading data\n");
 
-	fifo[0] = 0;
-	fifo[1] = 0;
-	error = rmi_write_block(fn->rmi_dev,
-				fn->fd.data_base_addr + F54_FIFO_OFFSET,
-				fifo, sizeof(fifo));
-	if (error) {
-		dev_err(&fn->dev, "Failed to set fifo start offset\n");
-		goto abort;
-	}
+	for (i = 0; i < report_size; i += F54_REPORT_DATA_SIZE) {
+		int size = min(F54_REPORT_DATA_SIZE, report_size - i);
+
+		fifo[0] = i & 0xff;
+		fifo[1] = i >> 8;
+		error = rmi_write_block(fn->rmi_dev,
+					fn->fd.data_base_addr + F54_FIFO_OFFSET,
+					fifo, sizeof(fifo));
+		if (error) {
+			dev_err(&fn->dev, "Failed to set fifo start offset\n");
+			goto abort;
+		}
 
-	error = rmi_read_block(fn->rmi_dev, fn->fd.data_base_addr +
-			       F54_REPORT_DATA_OFFSET, f54->report_data,
-			       report_size);
-	if (error) {
-		dev_err(&fn->dev, "%s: read [%d bytes] returned %d\n",
-			__func__, report_size, error);
-		goto abort;
+		error = rmi_read_block(fn->rmi_dev, fn->fd.data_base_addr +
+				       F54_REPORT_DATA_OFFSET,
+				       f54->report_data + i, size);
+		if (error) {
+			dev_err(&fn->dev, "%s: read [%d bytes] returned %d\n",
+				__func__, size, error);
+			goto abort;
+		}
 	}
 
 abort:
-- 
2.24.0

Re: [PATCH for v5.5 2/2] Input: rmi_f54: read from FIFO in 32 byte blocks

[Dmitry Torokhov]

Hi Hans,

On Wed, Jan 15, 2020 at 01:48:19PM +0100, Hans Verkuil wrote:
> The F54 Report Data is apparently read through a fifo and for
> the smbus protocol that means that between reading a block of 32
> bytes the rmiaddr shouldn't be incremented. However, changing
> that causes other non-fifo reads to fail and so that change was
> reverted.
> 
> This patch changes just the F54 function and it now reads 32 bytes
> at a time from the fifo, using the F54_FIFO_OFFSET to update the
> start address that is used when reading from the fifo.
> 
> This has only been tested with smbus, not with i2c or spi. But I
> suspect that the same is needed there since I think similar
> problems will occur there when reading more than 256 bytes.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Reported-by: Timo Kaufmann <timokau@zoho.com>
> Fixes: a284e11c371e ("Input: synaptics-rmi4 - don't increment rmiaddr for SMBus transfers")
> Cc: stable@vger.kernel.org

As you mentioned this one is not urgent so I dropped the stable
designation (you may forward to stable once it cooks in 5.5 for a bit)
and also dropped fixes as it does not fixes this particular commit but
something that was done before.

Otherwise applied.

Thanks.

-- 
Dmitry