[PATCH] Input: atmel_mxt_ts: Avoid excess read length on limited controllers

[PATCH] Input: atmel_mxt_ts: Avoid excess read length on limited controllers

[Marek Vasut]

Some I2C controllers have a hard limit on the number of data they can
transfer in one transfer (e.g. Xilinx XIIC has 255 bytes). The Atmel
MXT touchscreen driver mxt_process_messages_until_invalid() function
can trigger a read much longer than that (e.g. 690 bytes in my case).
This transfer can however be easily split into multiple shorter ones,
esp. since the single T5 message is 10 bytes or so.

This patch adds a check for the quirk presence and if it is present,
limits the number of messages read out of the controller such that
they are below the quirk limit. This makes it possible for the MXT
driver to work even on such limited controllers.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Nick Dyer <nick@shmanahar.org>
Cc: Evan Green <evgreen@chromium.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Sasha Levin <sashal@kernel.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 30 ++++++++++++++++++------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index a2189739e30f5..faa3f3f987d46 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -985,21 +985,37 @@ static int mxt_proc_message(struct mxt_data *data, u8 *message)
 
 static int mxt_read_and_process_messages(struct mxt_data *data, u8 count)
 {
+	const struct i2c_adapter_quirks *q = data->client->adapter->quirks;
 	struct device *dev = &data->client->dev;
-	int ret;
-	int i;
+	int i, ret, offset = 0;
+	u16 rem, chunk = count, total = count;
 	u8 num_valid = 0;
 
 	/* Safety check for msg_buf */
 	if (count > data->max_reportid)
 		return -EINVAL;
 
+	/* Handle controller read-length limitations */
+	if (q && q->max_read_len) {
+		chunk = min((u16)(q->max_read_len / data->T5_msg_size),
+			    (u16)count);
+	}
+
 	/* Process remaining messages if necessary */
-	ret = __mxt_read_reg(data->client, data->T5_address,
-				data->T5_msg_size * count, data->msg_buf);
-	if (ret) {
-		dev_err(dev, "Failed to read %u messages (%d)\n", count, ret);
-		return ret;
+	while (total) {
+		rem = min(total, chunk);
+		ret = __mxt_read_reg(data->client, data->T5_address,
+				     data->T5_msg_size * rem,
+				     data->msg_buf +
+					(offset * data->T5_msg_size));
+		if (ret) {
+			dev_err(dev,
+				"Failed to read %u messages (offset %u of total %u) (%d)\n",
+				rem, offset, count, ret);
+			return ret;
+		}
+		total -= rem;
+		offset += rem;
 	}
 
 	for (i = 0;  i < count; i++) {
-- 
2.26.2

Re: [PATCH] Input: atmel_mxt_ts: Avoid excess read length on limited controllers

[Marek Vasut]

On 6/13/20 4:56 PM, Marek Vasut wrote:
> Some I2C controllers have a hard limit on the number of data they can
> transfer in one transfer (e.g. Xilinx XIIC has 255 bytes). The Atmel
> MXT touchscreen driver mxt_process_messages_until_invalid() function
> can trigger a read much longer than that (e.g. 690 bytes in my case).
> This transfer can however be easily split into multiple shorter ones,
> esp. since the single T5 message is 10 bytes or so.
> 
> This patch adds a check for the quirk presence and if it is present,
> limits the number of messages read out of the controller such that
> they are below the quirk limit. This makes it possible for the MXT
> driver to work even on such limited controllers.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Nick Dyer <nick@shmanahar.org>
> Cc: Evan Green <evgreen@chromium.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Sasha Levin <sashal@kernel.org>
> ---
>   drivers/input/touchscreen/atmel_mxt_ts.c | 30 ++++++++++++++++++------
>   1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index a2189739e30f5..faa3f3f987d46 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -985,21 +985,37 @@ static int mxt_proc_message(struct mxt_data *data, u8 *message)
>   
>   static int mxt_read_and_process_messages(struct mxt_data *data, u8 count)
>   {
> +	const struct i2c_adapter_quirks *q = data->client->adapter->quirks;
>   	struct device *dev = &data->client->dev;
> -	int ret;
> -	int i;
> +	int i, ret, offset = 0;
> +	u16 rem, chunk = count, total = count;
>   	u8 num_valid = 0;
>   
>   	/* Safety check for msg_buf */
>   	if (count > data->max_reportid)
>   		return -EINVAL;
>   
> +	/* Handle controller read-length limitations */
> +	if (q && q->max_read_len) {
> +		chunk = min((u16)(q->max_read_len / data->T5_msg_size),
> +			    (u16)count);
> +	}
> +
>   	/* Process remaining messages if necessary */
> -	ret = __mxt_read_reg(data->client, data->T5_address,
> -				data->T5_msg_size * count, data->msg_buf);
> -	if (ret) {
> -		dev_err(dev, "Failed to read %u messages (%d)\n", count, ret);
> -		return ret;
> +	while (total) {
> +		rem = min(total, chunk);
> +		ret = __mxt_read_reg(data->client, data->T5_address,
> +				     data->T5_msg_size * rem,
> +				     data->msg_buf +
> +					(offset * data->T5_msg_size));
> +		if (ret) {
> +			dev_err(dev,
> +				"Failed to read %u messages (offset %u of total %u) (%d)\n",
> +				rem, offset, count, ret);
> +			return ret;
> +		}
> +		total -= rem;
> +		offset += rem;
>   	}
>   
>   	for (i = 0;  i < count; i++) {

I just found this patch still in my upstreaming queue. Any input on 
this? It seems to still apply and it is still needed on Xilinx XIIC I2C.

Re: [PATCH] Input: atmel_mxt_ts: Avoid excess read length on limited controllers

[Dmitry Torokhov]

Hi Marek,

On Sat, Jun 13, 2020 at 04:56:32PM +0200, Marek Vasut wrote:
> Some I2C controllers have a hard limit on the number of data they can
> transfer in one transfer (e.g. Xilinx XIIC has 255 bytes). The Atmel
> MXT touchscreen driver mxt_process_messages_until_invalid() function
> can trigger a read much longer than that (e.g. 690 bytes in my case).
> This transfer can however be easily split into multiple shorter ones,
> esp. since the single T5 message is 10 bytes or so.
> 
> This patch adds a check for the quirk presence and if it is present,
> limits the number of messages read out of the controller such that
> they are below the quirk limit. This makes it possible for the MXT
> driver to work even on such limited controllers.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Nick Dyer <nick@shmanahar.org>
> Cc: Evan Green <evgreen@chromium.org>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Sasha Levin <sashal@kernel.org>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 30 ++++++++++++++++++------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index a2189739e30f5..faa3f3f987d46 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -985,21 +985,37 @@ static int mxt_proc_message(struct mxt_data *data, u8 *message)
>  
>  static int mxt_read_and_process_messages(struct mxt_data *data, u8 count)
>  {
> +	const struct i2c_adapter_quirks *q = data->client->adapter->quirks;
>  	struct device *dev = &data->client->dev;
> -	int ret;
> -	int i;
> +	int i, ret, offset = 0;
> +	u16 rem, chunk = count, total = count;
>  	u8 num_valid = 0;
>  
>  	/* Safety check for msg_buf */
>  	if (count > data->max_reportid)
>  		return -EINVAL;
>  
> +	/* Handle controller read-length limitations */
> +	if (q && q->max_read_len) {
> +		chunk = min((u16)(q->max_read_len / data->T5_msg_size),
> +			    (u16)count);

I do not think you need this min() here. The "rem = min(total, chunk);"
below will take care of cases where chunk is bigger than total count.

> +	}
> +
>  	/* Process remaining messages if necessary */
> -	ret = __mxt_read_reg(data->client, data->T5_address,
> -				data->T5_msg_size * count, data->msg_buf);
> -	if (ret) {
> -		dev_err(dev, "Failed to read %u messages (%d)\n", count, ret);
> -		return ret;
> +	while (total) {
> +		rem = min(total, chunk);
> +		ret = __mxt_read_reg(data->client, data->T5_address,
> +				     data->T5_msg_size * rem,
> +				     data->msg_buf +
> +					(offset * data->T5_msg_size));
> +		if (ret) {
> +			dev_err(dev,
> +				"Failed to read %u messages (offset %u of total %u) (%d)\n",
> +				rem, offset, count, ret);
> +			return ret;
> +		}
> +		total -= rem;
> +		offset += rem;
>  	}
>  
>  	for (i = 0;  i < count; i++) {
> -- 
> 2.26.2
> 

Thanks.

-- 
Dmitry

Re: [PATCH] Input: atmel_mxt_ts: Avoid excess read length on limited controllers

[Marek Vasut]

On 11/8/24 7:08 AM, Dmitry Torokhov wrote:
> Hi Marek,
> 
> On Sat, Jun 13, 2020 at 04:56:32PM +0200, Marek Vasut wrote:
>> Some I2C controllers have a hard limit on the number of data they can
>> transfer in one transfer (e.g. Xilinx XIIC has 255 bytes). The Atmel
>> MXT touchscreen driver mxt_process_messages_until_invalid() function
>> can trigger a read much longer than that (e.g. 690 bytes in my case).
>> This transfer can however be easily split into multiple shorter ones,
>> esp. since the single T5 message is 10 bytes or so.
>>
>> This patch adds a check for the quirk presence and if it is present,
>> limits the number of messages read out of the controller such that
>> they are below the quirk limit. This makes it possible for the MXT
>> driver to work even on such limited controllers.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Nick Dyer <nick@shmanahar.org>
>> Cc: Evan Green <evgreen@chromium.org>
>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> Cc: Sasha Levin <sashal@kernel.org>
>> ---
>>   drivers/input/touchscreen/atmel_mxt_ts.c | 30 ++++++++++++++++++------
>>   1 file changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
>> index a2189739e30f5..faa3f3f987d46 100644
>> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
>> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
>> @@ -985,21 +985,37 @@ static int mxt_proc_message(struct mxt_data *data, u8 *message)
>>   
>>   static int mxt_read_and_process_messages(struct mxt_data *data, u8 count)
>>   {
>> +	const struct i2c_adapter_quirks *q = data->client->adapter->quirks;
>>   	struct device *dev = &data->client->dev;
>> -	int ret;
>> -	int i;
>> +	int i, ret, offset = 0;
>> +	u16 rem, chunk = count, total = count;
>>   	u8 num_valid = 0;
>>   
>>   	/* Safety check for msg_buf */
>>   	if (count > data->max_reportid)
>>   		return -EINVAL;
>>   
>> +	/* Handle controller read-length limitations */
>> +	if (q && q->max_read_len) {
>> +		chunk = min((u16)(q->max_read_len / data->T5_msg_size),
>> +			    (u16)count);
> 
> I do not think you need this min() here. The "rem = min(total, chunk);"
> below will take care of cases where chunk is bigger than total count.
'chunk' here has to be limited to at most as many T5 messages as fit 
into q->max_read_len limit of the controller, that is the purpose of 
this 'min' here.