[PATCHv1] Input: atmel_mxt_ts - fix the firmware update

[PATCHv1] Input: atmel_mxt_ts - fix the firmware update

[Sebastian Reichel]

From: Nandor Han <nandor.han@ge.com>

The automatic update mechanism will trigger an update if the
info block CRCs are different between maxtouch configuration
file (maxtouch.cfg) and chip.

The driver compared the CRCs without retrieving the chip CRC,
resulting always in a failure and firmware flashing action
triggered. The patch will fix this issue by retrieving the
chip info block CRC before the check.

Suggested-by: Todd Weyenberg <Todd.Weyenberg@med.ge.com>
Signed-off-by: Nandor Han <nandor.han@ge.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 7659bc48f1db..ab936e6b0286 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -1728,6 +1728,25 @@ static int mxt_get_object_table(struct mxt_data *data)
 	return error;
 }
 
+static int mxt_get_info_block_crc(struct mxt_data *data)
+{
+	size_t table_size;
+	int error;
+	u8 info_block_crc[MXT_INFO_CHECKSUM_SIZE];
+
+	table_size = data->info.object_num * sizeof(struct mxt_object);
+
+	/* Read the info block CRC */
+	error = __mxt_read_reg(data->client, MXT_OBJECT_START + table_size,
+					sizeof(info_block_crc), info_block_crc);
+	if (!error) {
+		data->info_crc = info_block_crc[0] | (info_block_crc[1] << 8) |
+				(info_block_crc[2] << 16);
+	}
+
+	return error;
+}
+
 static int mxt_read_t9_resolution(struct mxt_data *data)
 {
 	struct i2c_client *client = data->client;
@@ -2077,6 +2096,14 @@ static int mxt_initialize(struct mxt_data *data)
 		return error;
 	}
 
+	/* Get info block CRC */
+	error = mxt_get_info_block_crc(data);
+	if (error) {
+		dev_err(&client->dev, "Error %d reading info block CRC\n",
+			error);
+		return error;
+	}
+
 	error = mxt_acquire_irq(data);
 	if (error)
 		goto err_free_object_table;
-- 
2.16.2

Re: [PATCHv1] Input: atmel_mxt_ts - fix the firmware update

[Dmitry Torokhov]

Hi Sebastian, Nandor,

On Thu, Mar 22, 2018 at 05:43:30PM +0100, Sebastian Reichel wrote:
> From: Nandor Han <nandor.han@ge.com>
> 
> The automatic update mechanism will trigger an update if the
> info block CRCs are different between maxtouch configuration
> file (maxtouch.cfg) and chip.
> 
> The driver compared the CRCs without retrieving the chip CRC,
> resulting always in a failure and firmware flashing action
> triggered. The patch will fix this issue by retrieving the
> chip info block CRC before the check.
> 
> Suggested-by: Todd Weyenberg <Todd.Weyenberg@med.ge.com>
> Signed-off-by: Nandor Han <nandor.han@ge.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 7659bc48f1db..ab936e6b0286 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -1728,6 +1728,25 @@ static int mxt_get_object_table(struct mxt_data *data)
>  	return error;
>  }
>  
> +static int mxt_get_info_block_crc(struct mxt_data *data)
> +{
> +	size_t table_size;
> +	int error;
> +	u8 info_block_crc[MXT_INFO_CHECKSUM_SIZE];
> +
> +	table_size = data->info.object_num * sizeof(struct mxt_object);
> +
> +	/* Read the info block CRC */
> +	error = __mxt_read_reg(data->client, MXT_OBJECT_START + table_size,
> +					sizeof(info_block_crc), info_block_crc);
> +	if (!error) {
> +		data->info_crc = info_block_crc[0] | (info_block_crc[1] << 8) |
> +				(info_block_crc[2] << 16);
> +	}
> +
> +	return error;
> +}
> +
>  static int mxt_read_t9_resolution(struct mxt_data *data)
>  {
>  	struct i2c_client *client = data->client;
> @@ -2077,6 +2096,14 @@ static int mxt_initialize(struct mxt_data *data)
>  		return error;
>  	}
>  
> +	/* Get info block CRC */
> +	error = mxt_get_info_block_crc(data);
> +	if (error) {
> +		dev_err(&client->dev, "Error %d reading info block CRC\n",
> +			error);
> +		return error;

You are leaking object table memory here.

By the way, why do we do this here, and not when we actually have config
file and is about to apply it?

Thanks.

-- 
Dmitry

Re: [PATCHv1] Input: atmel_mxt_ts - fix the firmware update

[Nick Dyer]

On Thu, Mar 22, 2018 at 05:43:30PM +0100, Sebastian Reichel wrote:
> The automatic update mechanism will trigger an update if the
> info block CRCs are different between maxtouch configuration
> file (maxtouch.cfg) and chip.
> 
> The driver compared the CRCs without retrieving the chip CRC,
> resulting always in a failure and firmware flashing action
> triggered. The patch will fix this issue by retrieving the
> chip info block CRC before the check.

Thanks for raising this, I agree it's definitely something we want to
fix.

However, I'm not convinced you're solving the problem in the best way.
You've attached it to the read_t9_resolution() code path, whereas the
info block is common between T9 and T100 and works in the same way.

Would you mind trying the below patch? I've dusted it off from some
work that I did back in 2012 and it should solve your issue.

It also has the benefit that by reading the information block and the
object table into a contiguous region of memory, we can verify the
checksum at probe time. This means we make sure that we are indeed
talking to a chip that supports object protocol correctly.

Signed-off-by: Nick Dyer <nick.dyer@shmanahar.org>
Acked-by: Benson Leung <bleung@chromium.org>
---
 drivers/input/touchscreen/atmel_mxt_ts.c | 193 +++++++++++++++++++------------
 1 file changed, 117 insertions(+), 76 deletions(-)

diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
index 7659bc48f1db..8a60d91d49a6 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -275,7 +275,8 @@ struct mxt_data {
 	char phys[64];		/* device physical location */
 	const struct mxt_platform_data *pdata;
 	struct mxt_object *object_table;
-	struct mxt_info info;
+	struct mxt_info *info;
+	void *raw_info_block;
 	unsigned int irq;
 	unsigned int max_x;
 	unsigned int max_y;
@@ -450,12 +451,13 @@ static int mxt_lookup_bootloader_address(struct mxt_data *data, bool retry)
 {
 	u8 appmode = data->client->addr;
 	u8 bootloader;
+	u8 family_id = data->info ? data->info->family_id : 0;
 
 	switch (appmode) {
 	case 0x4a:
 	case 0x4b:
 		/* Chips after 1664S use different scheme */
-		if (retry || data->info.family_id >= 0xa2) {
+		if (retry || family_id >= 0xa2) {
 			bootloader = appmode - 0x24;
 			break;
 		}
@@ -682,7 +684,7 @@ mxt_get_object(struct mxt_data *data, u8 type)
 	struct mxt_object *object;
 	int i;
 
-	for (i = 0; i < data->info.object_num; i++) {
+	for (i = 0; i < data->info->object_num; i++) {
 		object = data->object_table + i;
 		if (object->type == type)
 			return object;
@@ -1453,12 +1455,12 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
 		data_pos += offset;
 	}
 
-	if (cfg_info.family_id != data->info.family_id) {
+	if (cfg_info.family_id != data->info->family_id) {
 		dev_err(dev, "Family ID mismatch!\n");
 		return -EINVAL;
 	}
 
-	if (cfg_info.variant_id != data->info.variant_id) {
+	if (cfg_info.variant_id != data->info->variant_id) {
 		dev_err(dev, "Variant ID mismatch!\n");
 		return -EINVAL;
 	}
@@ -1503,7 +1505,7 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
 
 	/* Malloc memory to store configuration */
 	cfg_start_ofs = MXT_OBJECT_START +
-			data->info.object_num * sizeof(struct mxt_object) +
+			data->info->object_num * sizeof(struct mxt_object) +
 			MXT_INFO_CHECKSUM_SIZE;
 	config_mem_size = data->mem_size - cfg_start_ofs;
 	config_mem = kzalloc(config_mem_size, GFP_KERNEL);
@@ -1554,20 +1556,6 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
 	return ret;
 }
 
-static int mxt_get_info(struct mxt_data *data)
-{
-	struct i2c_client *client = data->client;
-	struct mxt_info *info = &data->info;
-	int error;
-
-	/* Read 7-byte info block starting at address 0 */
-	error = __mxt_read_reg(client, 0, sizeof(*info), info);
-	if (error)
-		return error;
-
-	return 0;
-}
-
 static void mxt_free_input_device(struct mxt_data *data)
 {
 	if (data->input_dev) {
@@ -1582,9 +1570,10 @@ static void mxt_free_object_table(struct mxt_data *data)
 	video_unregister_device(&data->dbg.vdev);
 	v4l2_device_unregister(&data->dbg.v4l2);
 #endif
-
-	kfree(data->object_table);
 	data->object_table = NULL;
+	data->info = NULL;
+	kfree(data->raw_info_block);
+	data->raw_info_block = NULL;
 	kfree(data->msg_buf);
 	data->msg_buf = NULL;
 	data->T5_address = 0;
@@ -1600,34 +1589,18 @@ static void mxt_free_object_table(struct mxt_data *data)
 	data->max_reportid = 0;
 }
 
-static int mxt_get_object_table(struct mxt_data *data)
+static int mxt_parse_object_table(struct mxt_data *data,
+				  struct mxt_object *object_table)
 {
 	struct i2c_client *client = data->client;
-	size_t table_size;
-	struct mxt_object *object_table;
-	int error;
 	int i;
 	u8 reportid;
 	u16 end_address;
 
-	table_size = data->info.object_num * sizeof(struct mxt_object);
-	object_table = kzalloc(table_size, GFP_KERNEL);
-	if (!object_table) {
-		dev_err(&data->client->dev, "Failed to allocate memory\n");
-		return -ENOMEM;
-	}
-
-	error = __mxt_read_reg(client, MXT_OBJECT_START, table_size,
-			object_table);
-	if (error) {
-		kfree(object_table);
-		return error;
-	}
-
 	/* Valid Report IDs start counting from 1 */
 	reportid = 1;
 	data->mem_size = 0;
-	for (i = 0; i < data->info.object_num; i++) {
+	for (i = 0; i < data->info->object_num; i++) {
 		struct mxt_object *object = object_table + i;
 		u8 min_id, max_id;
 
@@ -1651,8 +1624,8 @@ static int mxt_get_object_table(struct mxt_data *data)
 
 		switch (object->type) {
 		case MXT_GEN_MESSAGE_T5:
-			if (data->info.family_id == 0x80 &&
-			    data->info.version < 0x20) {
+			if (data->info->family_id == 0x80 &&
+			    data->info->version < 0x20) {
 				/*
 				 * On mXT224 firmware versions prior to V2.0
 				 * read and discard unused CRC byte otherwise
@@ -1707,24 +1680,108 @@ static int mxt_get_object_table(struct mxt_data *data)
 	/* If T44 exists, T5 position has to be directly after */
 	if (data->T44_address && (data->T5_address != data->T44_address + 1)) {
 		dev_err(&client->dev, "Invalid T44 position\n");
-		error = -EINVAL;
-		goto free_object_table;
+		return -EINVAL;
 	}
 
 	data->msg_buf = kcalloc(data->max_reportid,
 				data->T5_msg_size, GFP_KERNEL);
 	if (!data->msg_buf) {
 		dev_err(&client->dev, "Failed to allocate message buffer\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int mxt_read_info_block(struct mxt_data *data)
+{
+	struct i2c_client *client = data->client;
+	int error;
+	size_t size;
+	void *id_buf, *buf;
+	uint8_t num_objects;
+	u32 calculated_crc;
+	u8 *crc_ptr;
+
+	/* If info block already allocated, free it */
+	if (data->raw_info_block != NULL)
+		mxt_free_object_table(data);
+
+	/* Read 7-byte ID information block starting at address 0 */
+	size = sizeof(struct mxt_info);
+	id_buf = kzalloc(size, GFP_KERNEL);
+	if (!id_buf) {
+		dev_err(&client->dev, "Failed to allocate memory\n");
+		return -ENOMEM;
+	}
+
+	error = __mxt_read_reg(client, 0, size, id_buf);
+	if (error) {
+		kfree(id_buf);
+		return error;
+	}
+
+	/* Resize buffer to give space for rest of info block */
+	num_objects = ((struct mxt_info *)id_buf)->object_num;
+	size += (num_objects * sizeof(struct mxt_object))
+		+ MXT_INFO_CHECKSUM_SIZE;
+
+	buf = krealloc(id_buf, size, GFP_KERNEL);
+	if (!buf) {
+		dev_err(&client->dev, "Failed to allocate memory\n");
 		error = -ENOMEM;
-		goto free_object_table;
+		goto err_free_mem;
+	}
+
+	/* Read rest of info block */
+	error = __mxt_read_reg(client, MXT_OBJECT_START,
+			       size - MXT_OBJECT_START,
+			       buf + MXT_OBJECT_START);
+	if (error)
+		goto err_free_mem;
+
+	/* Extract & calculate checksum */
+	crc_ptr = buf + size - MXT_INFO_CHECKSUM_SIZE;
+	data->info_crc = crc_ptr[0] | (crc_ptr[1] << 8) | (crc_ptr[2] << 16);
+
+	calculated_crc = mxt_calculate_crc(buf, 0,
+					   size - MXT_INFO_CHECKSUM_SIZE);
+
+	/*
+	 * CRC mismatch can be caused by data corruption due to I2C comms
+	 * issue or else device is not using Object Based Protocol (eg i2c-hid)
+	 */
+	if ((data->info_crc == 0) || (data->info_crc != calculated_crc)) {
+		dev_err(&client->dev,
+			"Info Block CRC error calculated=0x%06X read=0x%06X\n",
+			calculated_crc, data->info_crc);
+		error = -EIO;
+		goto err_free_mem;
 	}
 
-	data->object_table = object_table;
+	data->raw_info_block = buf;
+	data->info = (struct mxt_info *)buf;
+
+	dev_info(&client->dev,
+		 "Family: %u Variant: %u Firmware V%u.%u.%02X Objects: %u\n",
+		 data->info->family_id, data->info->variant_id,
+		 data->info->version >> 4, data->info->version & 0xf,
+		 data->info->build, data->info->object_num);
+
+	/* Parse object table information */
+	error = mxt_parse_object_table(data, buf + MXT_OBJECT_START);
+	if (error) {
+		dev_err(&client->dev, "Error %d parsing object table\n", error);
+		mxt_free_object_table(data);
+		return error;
+	}
+
+	data->object_table = (struct mxt_object *)(buf + MXT_OBJECT_START);
 
 	return 0;
 
-free_object_table:
-	mxt_free_object_table(data);
+err_free_mem:
+	kfree(buf);
 	return error;
 }
 
@@ -2039,7 +2096,7 @@ static int mxt_initialize(struct mxt_data *data)
 	int error;
 
 	while (1) {
-		error = mxt_get_info(data);
+		error = mxt_read_info_block(data);
 		if (!error)
 			break;
 
@@ -2070,13 +2127,6 @@ static int mxt_initialize(struct mxt_data *data)
 		msleep(MXT_FW_RESET_TIME);
 	}
 
-	/* Get object table information */
-	error = mxt_get_object_table(data);
-	if (error) {
-		dev_err(&client->dev, "Error %d reading object table\n", error);
-		return error;
-	}
-
 	error = mxt_acquire_irq(data);
 	if (error)
 		goto err_free_object_table;
@@ -2155,25 +2205,24 @@ static int mxt_init_t7_power_cfg(struct mxt_data *data)
 static u16 mxt_get_debug_value(struct mxt_data *data, unsigned int x,
 			       unsigned int y)
 {
-	struct mxt_info *info = &data->info;
 	struct mxt_dbg *dbg = &data->dbg;
 	unsigned int ofs, page;
 	unsigned int col = 0;
 	unsigned int col_width;
 
-	if (info->family_id == MXT_FAMILY_1386) {
-		col_width = info->matrix_ysize / MXT1386_COLUMNS;
+	if (data->info->family_id == MXT_FAMILY_1386) {
+		col_width = data->info->matrix_ysize / MXT1386_COLUMNS;
 		col = y / col_width;
 		y = y % col_width;
 	} else {
-		col_width = info->matrix_ysize;
+		col_width = data->info->matrix_ysize;
 	}
 
 	ofs = (y + (x * col_width)) * sizeof(u16);
 	page = ofs / MXT_DIAGNOSTIC_SIZE;
 	ofs %= MXT_DIAGNOSTIC_SIZE;
 
-	if (info->family_id == MXT_FAMILY_1386)
+	if (data->info->family_id == MXT_FAMILY_1386)
 		page += col * MXT1386_PAGES_PER_COLUMN;
 
 	return get_unaligned_le16(&dbg->t37_buf[page].data[ofs]);
@@ -2483,7 +2532,6 @@ static const struct video_device mxt_video_device = {
 
 static void mxt_debug_init(struct mxt_data *data)
 {
-	struct mxt_info *info = &data->info;
 	struct mxt_dbg *dbg = &data->dbg;
 	struct mxt_object *object;
 	int error;
@@ -2508,11 +2556,11 @@ static void mxt_debug_init(struct mxt_data *data)
 	/* Calculate size of data and allocate buffer */
 	dbg->t37_nodes = data->xsize * data->ysize;
 
-	if (info->family_id == MXT_FAMILY_1386)
+	if (data->info->family_id == MXT_FAMILY_1386)
 		dbg->t37_pages = MXT1386_COLUMNS * MXT1386_PAGES_PER_COLUMN;
 	else
 		dbg->t37_pages = DIV_ROUND_UP(data->xsize *
-					      info->matrix_ysize *
+					      data->info->matrix_ysize *
 					      sizeof(u16),
 					      sizeof(dbg->t37_buf->data));
 
@@ -2569,7 +2617,6 @@ static int mxt_configure_objects(struct mxt_data *data,
 				 const struct firmware *cfg)
 {
 	struct device *dev = &data->client->dev;
-	struct mxt_info *info = &data->info;
 	int error;
 
 	error = mxt_init_t7_power_cfg(data);
@@ -2594,11 +2641,6 @@ static int mxt_configure_objects(struct mxt_data *data,
 
 	mxt_debug_init(data);
 
-	dev_info(dev,
-		 "Family: %u Variant: %u Firmware V%u.%u.%02X Objects: %u\n",
-		 info->family_id, info->variant_id, info->version >> 4,
-		 info->version & 0xf, info->build, info->object_num);
-
 	return 0;
 }
 
@@ -2607,9 +2649,9 @@ static ssize_t mxt_fw_version_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
 	struct mxt_data *data = dev_get_drvdata(dev);
-	struct mxt_info *info = &data->info;
 	return scnprintf(buf, PAGE_SIZE, "%u.%u.%02X\n",
-			 info->version >> 4, info->version & 0xf, info->build);
+			 data->info->version >> 4, data->info->version & 0xf,
+			 data->info->build);
 }
 
 /* Hardware Version is returned as FamilyID.VariantID */
@@ -2617,9 +2659,8 @@ static ssize_t mxt_hw_version_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
 	struct mxt_data *data = dev_get_drvdata(dev);
-	struct mxt_info *info = &data->info;
 	return scnprintf(buf, PAGE_SIZE, "%u.%u\n",
-			 info->family_id, info->variant_id);
+			data->info->family_id, data->info->variant_id);
 }
 
 static ssize_t mxt_show_instance(char *buf, int count,
@@ -2656,7 +2697,7 @@ static ssize_t mxt_object_show(struct device *dev,
 		return -ENOMEM;
 
 	error = 0;
-	for (i = 0; i < data->info.object_num; i++) {
+	for (i = 0; i < data->info->object_num; i++) {
 		object = data->object_table + i;
 
 		if (!mxt_object_readable(object->type))
-- 
2.14.1

Re: [PATCHv1] Input: atmel_mxt_ts - fix the firmware update

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 14823 bytes --]

Hi Nick,

On Fri, Mar 23, 2018 at 07:47:36PM +0000, Nick Dyer wrote:
> On Thu, Mar 22, 2018 at 05:43:30PM +0100, Sebastian Reichel wrote:
> > The automatic update mechanism will trigger an update if the
> > info block CRCs are different between maxtouch configuration
> > file (maxtouch.cfg) and chip.
> > 
> > The driver compared the CRCs without retrieving the chip CRC,
> > resulting always in a failure and firmware flashing action
> > triggered. The patch will fix this issue by retrieving the
> > chip info block CRC before the check.
> 
> Thanks for raising this, I agree it's definitely something we want to
> fix.
> 
> However, I'm not convinced you're solving the problem in the best way.
> You've attached it to the read_t9_resolution() code path, whereas the
> info block is common between T9 and T100 and works in the same way.
> 
> Would you mind trying the below patch? I've dusted it off from some
> work that I did back in 2012 and it should solve your issue.
> 
> It also has the benefit that by reading the information block and the
> object table into a contiguous region of memory, we can verify the
> checksum at probe time. This means we make sure that we are indeed
> talking to a chip that supports object protocol correctly.
> 
> Signed-off-by: Nick Dyer <nick.dyer@shmanahar.org>
> Acked-by: Benson Leung <bleung@chromium.org>

I currently have an unrelated issue that breaks boot on that board,
so I can't test it, but the patch looks mostly good to me. I
noticed, two useless error messages for -ENOMEM. After fixing those
the patch is

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

> ---
>  drivers/input/touchscreen/atmel_mxt_ts.c | 193 +++++++++++++++++++------------
>  1 file changed, 117 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c
> index 7659bc48f1db..8a60d91d49a6 100644
> --- a/drivers/input/touchscreen/atmel_mxt_ts.c
> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c
> @@ -275,7 +275,8 @@ struct mxt_data {
>  	char phys[64];		/* device physical location */
>  	const struct mxt_platform_data *pdata;
>  	struct mxt_object *object_table;
> -	struct mxt_info info;
> +	struct mxt_info *info;
> +	void *raw_info_block;
>  	unsigned int irq;
>  	unsigned int max_x;
>  	unsigned int max_y;
> @@ -450,12 +451,13 @@ static int mxt_lookup_bootloader_address(struct mxt_data *data, bool retry)
>  {
>  	u8 appmode = data->client->addr;
>  	u8 bootloader;
> +	u8 family_id = data->info ? data->info->family_id : 0;
>  
>  	switch (appmode) {
>  	case 0x4a:
>  	case 0x4b:
>  		/* Chips after 1664S use different scheme */
> -		if (retry || data->info.family_id >= 0xa2) {
> +		if (retry || family_id >= 0xa2) {
>  			bootloader = appmode - 0x24;
>  			break;
>  		}
> @@ -682,7 +684,7 @@ mxt_get_object(struct mxt_data *data, u8 type)
>  	struct mxt_object *object;
>  	int i;
>  
> -	for (i = 0; i < data->info.object_num; i++) {
> +	for (i = 0; i < data->info->object_num; i++) {
>  		object = data->object_table + i;
>  		if (object->type == type)
>  			return object;
> @@ -1453,12 +1455,12 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
>  		data_pos += offset;
>  	}
>  
> -	if (cfg_info.family_id != data->info.family_id) {
> +	if (cfg_info.family_id != data->info->family_id) {
>  		dev_err(dev, "Family ID mismatch!\n");
>  		return -EINVAL;
>  	}
>  
> -	if (cfg_info.variant_id != data->info.variant_id) {
> +	if (cfg_info.variant_id != data->info->variant_id) {
>  		dev_err(dev, "Variant ID mismatch!\n");
>  		return -EINVAL;
>  	}
> @@ -1503,7 +1505,7 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
>  
>  	/* Malloc memory to store configuration */
>  	cfg_start_ofs = MXT_OBJECT_START +
> -			data->info.object_num * sizeof(struct mxt_object) +
> +			data->info->object_num * sizeof(struct mxt_object) +
>  			MXT_INFO_CHECKSUM_SIZE;
>  	config_mem_size = data->mem_size - cfg_start_ofs;
>  	config_mem = kzalloc(config_mem_size, GFP_KERNEL);
> @@ -1554,20 +1556,6 @@ static int mxt_update_cfg(struct mxt_data *data, const struct firmware *cfg)
>  	return ret;
>  }
>  
> -static int mxt_get_info(struct mxt_data *data)
> -{
> -	struct i2c_client *client = data->client;
> -	struct mxt_info *info = &data->info;
> -	int error;
> -
> -	/* Read 7-byte info block starting at address 0 */
> -	error = __mxt_read_reg(client, 0, sizeof(*info), info);
> -	if (error)
> -		return error;
> -
> -	return 0;
> -}
> -
>  static void mxt_free_input_device(struct mxt_data *data)
>  {
>  	if (data->input_dev) {
> @@ -1582,9 +1570,10 @@ static void mxt_free_object_table(struct mxt_data *data)
>  	video_unregister_device(&data->dbg.vdev);
>  	v4l2_device_unregister(&data->dbg.v4l2);
>  #endif
> -
> -	kfree(data->object_table);
>  	data->object_table = NULL;
> +	data->info = NULL;
> +	kfree(data->raw_info_block);
> +	data->raw_info_block = NULL;
>  	kfree(data->msg_buf);
>  	data->msg_buf = NULL;
>  	data->T5_address = 0;
> @@ -1600,34 +1589,18 @@ static void mxt_free_object_table(struct mxt_data *data)
>  	data->max_reportid = 0;
>  }
>  
> -static int mxt_get_object_table(struct mxt_data *data)
> +static int mxt_parse_object_table(struct mxt_data *data,
> +				  struct mxt_object *object_table)
>  {
>  	struct i2c_client *client = data->client;
> -	size_t table_size;
> -	struct mxt_object *object_table;
> -	int error;
>  	int i;
>  	u8 reportid;
>  	u16 end_address;
>  
> -	table_size = data->info.object_num * sizeof(struct mxt_object);
> -	object_table = kzalloc(table_size, GFP_KERNEL);
> -	if (!object_table) {
> -		dev_err(&data->client->dev, "Failed to allocate memory\n");
> -		return -ENOMEM;
> -	}
> -
> -	error = __mxt_read_reg(client, MXT_OBJECT_START, table_size,
> -			object_table);
> -	if (error) {
> -		kfree(object_table);
> -		return error;
> -	}
> -
>  	/* Valid Report IDs start counting from 1 */
>  	reportid = 1;
>  	data->mem_size = 0;
> -	for (i = 0; i < data->info.object_num; i++) {
> +	for (i = 0; i < data->info->object_num; i++) {
>  		struct mxt_object *object = object_table + i;
>  		u8 min_id, max_id;
>  
> @@ -1651,8 +1624,8 @@ static int mxt_get_object_table(struct mxt_data *data)
>  
>  		switch (object->type) {
>  		case MXT_GEN_MESSAGE_T5:
> -			if (data->info.family_id == 0x80 &&
> -			    data->info.version < 0x20) {
> +			if (data->info->family_id == 0x80 &&
> +			    data->info->version < 0x20) {
>  				/*
>  				 * On mXT224 firmware versions prior to V2.0
>  				 * read and discard unused CRC byte otherwise
> @@ -1707,24 +1680,108 @@ static int mxt_get_object_table(struct mxt_data *data)
>  	/* If T44 exists, T5 position has to be directly after */
>  	if (data->T44_address && (data->T5_address != data->T44_address + 1)) {
>  		dev_err(&client->dev, "Invalid T44 position\n");
> -		error = -EINVAL;
> -		goto free_object_table;
> +		return -EINVAL;
>  	}
>  
>  	data->msg_buf = kcalloc(data->max_reportid,
>  				data->T5_msg_size, GFP_KERNEL);
>  	if (!data->msg_buf) {
>  		dev_err(&client->dev, "Failed to allocate message buffer\n");
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mxt_read_info_block(struct mxt_data *data)
> +{
> +	struct i2c_client *client = data->client;
> +	int error;
> +	size_t size;
> +	void *id_buf, *buf;
> +	uint8_t num_objects;
> +	u32 calculated_crc;
> +	u8 *crc_ptr;
> +
> +	/* If info block already allocated, free it */
> +	if (data->raw_info_block != NULL)
> +		mxt_free_object_table(data);
> +
> +	/* Read 7-byte ID information block starting at address 0 */
> +	size = sizeof(struct mxt_info);
> +	id_buf = kzalloc(size, GFP_KERNEL);
> +	if (!id_buf) {
> +		dev_err(&client->dev, "Failed to allocate memory\n");
> +		return -ENOMEM;
> +	}
> +
> +	error = __mxt_read_reg(client, 0, size, id_buf);
> +	if (error) {
> +		kfree(id_buf);
> +		return error;
> +	}
> +
> +	/* Resize buffer to give space for rest of info block */
> +	num_objects = ((struct mxt_info *)id_buf)->object_num;
> +	size += (num_objects * sizeof(struct mxt_object))
> +		+ MXT_INFO_CHECKSUM_SIZE;
> +
> +	buf = krealloc(id_buf, size, GFP_KERNEL);
> +	if (!buf) {
> +		dev_err(&client->dev, "Failed to allocate memory\n");
>  		error = -ENOMEM;
> -		goto free_object_table;
> +		goto err_free_mem;
> +	}
> +
> +	/* Read rest of info block */
> +	error = __mxt_read_reg(client, MXT_OBJECT_START,
> +			       size - MXT_OBJECT_START,
> +			       buf + MXT_OBJECT_START);
> +	if (error)
> +		goto err_free_mem;
> +
> +	/* Extract & calculate checksum */
> +	crc_ptr = buf + size - MXT_INFO_CHECKSUM_SIZE;
> +	data->info_crc = crc_ptr[0] | (crc_ptr[1] << 8) | (crc_ptr[2] << 16);
> +
> +	calculated_crc = mxt_calculate_crc(buf, 0,
> +					   size - MXT_INFO_CHECKSUM_SIZE);
> +
> +	/*
> +	 * CRC mismatch can be caused by data corruption due to I2C comms
> +	 * issue or else device is not using Object Based Protocol (eg i2c-hid)
> +	 */
> +	if ((data->info_crc == 0) || (data->info_crc != calculated_crc)) {
> +		dev_err(&client->dev,
> +			"Info Block CRC error calculated=0x%06X read=0x%06X\n",
> +			calculated_crc, data->info_crc);
> +		error = -EIO;
> +		goto err_free_mem;
>  	}
>  
> -	data->object_table = object_table;
> +	data->raw_info_block = buf;
> +	data->info = (struct mxt_info *)buf;
> +
> +	dev_info(&client->dev,
> +		 "Family: %u Variant: %u Firmware V%u.%u.%02X Objects: %u\n",
> +		 data->info->family_id, data->info->variant_id,
> +		 data->info->version >> 4, data->info->version & 0xf,
> +		 data->info->build, data->info->object_num);
> +
> +	/* Parse object table information */
> +	error = mxt_parse_object_table(data, buf + MXT_OBJECT_START);
> +	if (error) {
> +		dev_err(&client->dev, "Error %d parsing object table\n", error);
> +		mxt_free_object_table(data);
> +		return error;
> +	}
> +
> +	data->object_table = (struct mxt_object *)(buf + MXT_OBJECT_START);
>  
>  	return 0;
>  
> -free_object_table:
> -	mxt_free_object_table(data);
> +err_free_mem:
> +	kfree(buf);
>  	return error;
>  }
>  
> @@ -2039,7 +2096,7 @@ static int mxt_initialize(struct mxt_data *data)
>  	int error;
>  
>  	while (1) {
> -		error = mxt_get_info(data);
> +		error = mxt_read_info_block(data);
>  		if (!error)
>  			break;
>  
> @@ -2070,13 +2127,6 @@ static int mxt_initialize(struct mxt_data *data)
>  		msleep(MXT_FW_RESET_TIME);
>  	}
>  
> -	/* Get object table information */
> -	error = mxt_get_object_table(data);
> -	if (error) {
> -		dev_err(&client->dev, "Error %d reading object table\n", error);
> -		return error;
> -	}
> -
>  	error = mxt_acquire_irq(data);
>  	if (error)
>  		goto err_free_object_table;
> @@ -2155,25 +2205,24 @@ static int mxt_init_t7_power_cfg(struct mxt_data *data)
>  static u16 mxt_get_debug_value(struct mxt_data *data, unsigned int x,
>  			       unsigned int y)
>  {
> -	struct mxt_info *info = &data->info;
>  	struct mxt_dbg *dbg = &data->dbg;
>  	unsigned int ofs, page;
>  	unsigned int col = 0;
>  	unsigned int col_width;
>  
> -	if (info->family_id == MXT_FAMILY_1386) {
> -		col_width = info->matrix_ysize / MXT1386_COLUMNS;
> +	if (data->info->family_id == MXT_FAMILY_1386) {
> +		col_width = data->info->matrix_ysize / MXT1386_COLUMNS;
>  		col = y / col_width;
>  		y = y % col_width;
>  	} else {
> -		col_width = info->matrix_ysize;
> +		col_width = data->info->matrix_ysize;
>  	}
>  
>  	ofs = (y + (x * col_width)) * sizeof(u16);
>  	page = ofs / MXT_DIAGNOSTIC_SIZE;
>  	ofs %= MXT_DIAGNOSTIC_SIZE;
>  
> -	if (info->family_id == MXT_FAMILY_1386)
> +	if (data->info->family_id == MXT_FAMILY_1386)
>  		page += col * MXT1386_PAGES_PER_COLUMN;
>  
>  	return get_unaligned_le16(&dbg->t37_buf[page].data[ofs]);
> @@ -2483,7 +2532,6 @@ static const struct video_device mxt_video_device = {
>  
>  static void mxt_debug_init(struct mxt_data *data)
>  {
> -	struct mxt_info *info = &data->info;
>  	struct mxt_dbg *dbg = &data->dbg;
>  	struct mxt_object *object;
>  	int error;
> @@ -2508,11 +2556,11 @@ static void mxt_debug_init(struct mxt_data *data)
>  	/* Calculate size of data and allocate buffer */
>  	dbg->t37_nodes = data->xsize * data->ysize;
>  
> -	if (info->family_id == MXT_FAMILY_1386)
> +	if (data->info->family_id == MXT_FAMILY_1386)
>  		dbg->t37_pages = MXT1386_COLUMNS * MXT1386_PAGES_PER_COLUMN;
>  	else
>  		dbg->t37_pages = DIV_ROUND_UP(data->xsize *
> -					      info->matrix_ysize *
> +					      data->info->matrix_ysize *
>  					      sizeof(u16),
>  					      sizeof(dbg->t37_buf->data));
>  
> @@ -2569,7 +2617,6 @@ static int mxt_configure_objects(struct mxt_data *data,
>  				 const struct firmware *cfg)
>  {
>  	struct device *dev = &data->client->dev;
> -	struct mxt_info *info = &data->info;
>  	int error;
>  
>  	error = mxt_init_t7_power_cfg(data);
> @@ -2594,11 +2641,6 @@ static int mxt_configure_objects(struct mxt_data *data,
>  
>  	mxt_debug_init(data);
>  
> -	dev_info(dev,
> -		 "Family: %u Variant: %u Firmware V%u.%u.%02X Objects: %u\n",
> -		 info->family_id, info->variant_id, info->version >> 4,
> -		 info->version & 0xf, info->build, info->object_num);
> -
>  	return 0;
>  }
>  
> @@ -2607,9 +2649,9 @@ static ssize_t mxt_fw_version_show(struct device *dev,
>  				   struct device_attribute *attr, char *buf)
>  {
>  	struct mxt_data *data = dev_get_drvdata(dev);
> -	struct mxt_info *info = &data->info;
>  	return scnprintf(buf, PAGE_SIZE, "%u.%u.%02X\n",
> -			 info->version >> 4, info->version & 0xf, info->build);
> +			 data->info->version >> 4, data->info->version & 0xf,
> +			 data->info->build);
>  }
>  
>  /* Hardware Version is returned as FamilyID.VariantID */
> @@ -2617,9 +2659,8 @@ static ssize_t mxt_hw_version_show(struct device *dev,
>  				   struct device_attribute *attr, char *buf)
>  {
>  	struct mxt_data *data = dev_get_drvdata(dev);
> -	struct mxt_info *info = &data->info;
>  	return scnprintf(buf, PAGE_SIZE, "%u.%u\n",
> -			 info->family_id, info->variant_id);
> +			data->info->family_id, data->info->variant_id);
>  }
>  
>  static ssize_t mxt_show_instance(char *buf, int count,
> @@ -2656,7 +2697,7 @@ static ssize_t mxt_object_show(struct device *dev,
>  		return -ENOMEM;
>  
>  	error = 0;
> -	for (i = 0; i < data->info.object_num; i++) {
> +	for (i = 0; i < data->info->object_num; i++) {
>  		object = data->object_table + i;
>  
>  		if (!mxt_object_readable(object->type))
> -- 
> 2.14.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: EXT: Re: [PATCHv1] Input: atmel_mxt_ts - fix the firmware update

[Nandor Han]

On 23/03/18 21:47, Nick Dyer wrote:
> On Thu, Mar 22, 2018 at 05:43:30PM +0100, Sebastian Reichel wrote:
>> The automatic update mechanism will trigger an update if the
>> info block CRCs are different between maxtouch configuration
>> file (maxtouch.cfg) and chip.
>>
>> The driver compared the CRCs without retrieving the chip CRC,
>> resulting always in a failure and firmware flashing action
>> triggered. The patch will fix this issue by retrieving the
>> chip info block CRC before the check.
> 
> Thanks for raising this, I agree it's definitely something we want to
> fix.
> 
> However, I'm not convinced you're solving the problem in the best way.
> You've attached it to the read_t9_resolution() code path, whereas the
> info block is common between T9 and T100 and works in the same way.
> 
> Would you mind trying the below patch? I've dusted it off from some
> work that I did back in 2012 and it should solve your issue.
> 
> It also has the benefit that by reading the information block and the
> object table into a contiguous region of memory, we can verify the
> checksum at probe time. This means we make sure that we are indeed
> talking to a chip that supports object protocol correctly.
> 

Thanks Nick. This looks like a better solution.
Sebastian will test this and we can see the results.


Nandor

Re: EXT: Re: [PATCHv1] Input: atmel_mxt_ts - fix the firmware update

[Nandor Han]

On 23/03/18 21:47, Nick Dyer wrote:
> On Thu, Mar 22, 2018 at 05:43:30PM +0100, Sebastian Reichel wrote:
>> The automatic update mechanism will trigger an update if the
>> info block CRCs are different between maxtouch configuration
>> file (maxtouch.cfg) and chip.
>>
>> The driver compared the CRCs without retrieving the chip CRC,
>> resulting always in a failure and firmware flashing action
>> triggered. The patch will fix this issue by retrieving the
>> chip info block CRC before the check.
> 
> Thanks for raising this, I agree it's definitely something we want to
> fix.
> 
> However, I'm not convinced you're solving the problem in the best way.
> You've attached it to the read_t9_resolution() code path, whereas the
> info block is common between T9 and T100 and works in the same way.
> 
> Would you mind trying the below patch? I've dusted it off from some
> work that I did back in 2012 and it should solve your issue.
> 
> It also has the benefit that by reading the information block and the
> object table into a contiguous region of memory, we can verify the
> checksum at probe time. This means we make sure that we are indeed
> talking to a chip that supports object protocol correctly.
> 

Thanks Nick. This looks like a better solution.
Sebastian will test this and we can see the results.


Nandor