Re: [PATCH 44/53] Input: atmel_mxt_ts - Verify Information Block checksum on probe
Yufeng Shen wrote: >> + dev_info(&client->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); >> + > > Use data->info->XXX here, info was pointing to buf which could have > changed after krealloc. Thank you, that's a really good spot. Caused when I merged the dev_info line into that function. It actually explains a report I had about getting junk in this output but the info_crc check succeeding, hadn't had time to investigate yet. I will fix. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 44/53] Input: atmel_mxt_ts - Verify Information Block checksum on probe
On Wed, Jun 5, 2013 at 1:37 PM, Nick Dyer wrote: > 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 verify that > we are indeed talking to a chip that supports object protocol correctly. We > also detect I2C comms problems much earlier, resulting in easier diagnosis. > > Signed-off-by: Nick Dyer > Acked-by: Benson Leung > --- > drivers/input/touchscreen/atmel_mxt_ts.c | 181 > ++ > 1 file changed, 111 insertions(+), 70 deletions(-) > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c > b/drivers/input/touchscreen/atmel_mxt_ts.c > index bbfea7a..e0ff017 100644 > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > @@ -194,7 +194,8 @@ struct mxt_data { > char phys[64]; /* device physical location */ > 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; > @@ -365,12 +366,16 @@ static int mxt_lookup_bootloader_address(struct > mxt_data *data, bool retry) > { > u8 appmode = data->client->addr; > u8 bootloader; > + u8 family_id = 0; > + > + if (data->info) > + family_id = data->info->family_id; > > 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; > } > @@ -610,7 +615,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; > @@ -1249,13 +1254,13 @@ static int mxt_check_reg_init(struct mxt_data *data) > 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"); > ret = -EINVAL; > goto release; > } > > - 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"); > ret = -EINVAL; > goto release; > @@ -1302,7 +1307,7 @@ static int mxt_check_reg_init(struct mxt_data *data) > > /* 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); > @@ -1510,24 +1515,12 @@ static int mxt_acquire_irq(struct mxt_data *data) > return 0; > } > > -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_object_table(struct mxt_data *data) > { > - kfree(data->object_table); > + kfree(data->raw_info_block); > data->object_table = NULL; > + data->info = NULL; > + data->raw_info_block = NULL; > kfree(data->msg_buf); > data->msg_buf = NULL; > data->enable_reporting = false; > @@ -1549,25 +1542,17 @@ 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 i2c_client *client = data->client; > - size_t table_size; > - int error; > int i; > u8 reportid; > u16 end_address; > > - table_size = data->info.object_num * sizeof(struct mxt_object); > - error = __mxt_read_reg(client, MXT_OBJECT_START, table_size, > - data->object_table); > - if (error) > - return error; > - > /* Valid Report IDs start counting from 1 */ > reportid = 1; > data->mem_size = 0; > - for (i = 0; i < data->info.object_num; i++) { >
[PATCH 44/53] Input: atmel_mxt_ts - Verify Information Block checksum on probe
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 verify that we are indeed talking to a chip that supports object protocol correctly. We also detect I2C comms problems much earlier, resulting in easier diagnosis. Signed-off-by: Nick Dyer Acked-by: Benson Leung --- drivers/input/touchscreen/atmel_mxt_ts.c | 181 ++ 1 file changed, 111 insertions(+), 70 deletions(-) diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index bbfea7a..e0ff017 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -194,7 +194,8 @@ struct mxt_data { char phys[64]; /* device physical location */ 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; @@ -365,12 +366,16 @@ static int mxt_lookup_bootloader_address(struct mxt_data *data, bool retry) { u8 appmode = data->client->addr; u8 bootloader; + u8 family_id = 0; + + if (data->info) + family_id = data->info->family_id; 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; } @@ -610,7 +615,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; @@ -1249,13 +1254,13 @@ static int mxt_check_reg_init(struct mxt_data *data) 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"); ret = -EINVAL; goto release; } - 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"); ret = -EINVAL; goto release; @@ -1302,7 +1307,7 @@ static int mxt_check_reg_init(struct mxt_data *data) /* 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); @@ -1510,24 +1515,12 @@ static int mxt_acquire_irq(struct mxt_data *data) return 0; } -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_object_table(struct mxt_data *data) { - kfree(data->object_table); + kfree(data->raw_info_block); data->object_table = NULL; + data->info = NULL; + data->raw_info_block = NULL; kfree(data->msg_buf); data->msg_buf = NULL; data->enable_reporting = false; @@ -1549,25 +1542,17 @@ 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 i2c_client *client = data->client; - size_t table_size; - int error; int i; u8 reportid; u16 end_address; - table_size = data->info.object_num * sizeof(struct mxt_object); - error = __mxt_read_reg(client, MXT_OBJECT_START, table_size, - data->object_table); - if (error) - 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 = data->object_table + i; u8 min_id, max_id; @@ -1591,7 +1576,7 @@ static int mxt_get_object_table(struct mxt_data *data) switch (object->type) { case M