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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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