| Message ID | 1421909406-1240-1-git-send-email-dudl@cypress.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dudley, Mis-spelled 'assiggment' in Subject line. On Thu, Jan 22, 2015 at 02:50:05PM +0800, Dudley Du wrote: > Fixes the sparse warning issue of the incorrect type in assignment which > found by the kbuild test robot. > I wasn't able to reproduce the sparse warning. Was it for a specific architecture? Perhaps include a link to the email sent by the kbuild test robot. [...] > struct gen5_bl_metadata_row_params { > __le16 size; > - __le16 maximun_size; > + __le16 maximum_size; I don't think sparse cares about variable names. Be sure to note these changes in your log. > __le32 app_start; > __le16 app_len; > __le16 app_crc; > @@ -1192,67 +1192,69 @@ static int cyapa_gen5_bl_enter(struct cyapa *cyapa) > static int cyapa_gen5_check_fw(struct cyapa *cyapa, const struct firmware *fw) > { > struct device *dev = &cyapa->client->dev; > - struct gen5_bl_metadata_row_params metadata; > - struct cyapa_tsg_bin_image *image; > - int flash_records_count; > - u16 app_crc = 0; > - u16 app_integrity_crc = 0; > - u16 row_num; > - u8 *data; > + const struct cyapa_tsg_bin_image *image = (const void *)fw->data; > + const struct cyapa_tsg_bin_image_data_record *app_integrity; > + const struct gen5_bl_metadata_row_params *metadata; > + size_t flash_records_count; > + u32 fw_app_start, fw_upgrade_start; > + u16 fw_app_len, fw_upgrade_len; > + u16 app_crc; > + u16 app_integrity_crc; > int record_index; > int i; > > - image = (struct cyapa_tsg_bin_image *)fw->data; > flash_records_count = (fw->size - > sizeof(struct cyapa_tsg_bin_image_head)) / > sizeof(struct cyapa_tsg_bin_image_data_record); > > - /* APP_INTEGRITY row is always the last row block, > - * and the row id must be 0x01ff */ > - row_num = get_unaligned_be16( > - &image->records[flash_records_count - 1].row_number); > - if (image->records[flash_records_count - 1].flash_array_id != 0x00 && > - row_num != 0x01ff) { > + /* > + * APP_INTEGRITY row is always the last row block, > + * and the row id must be 0x01ff. > + */ > + app_integrity = &image->records[flash_records_count - 1]; > + > + if (app_integrity->flash_array_id != 0x00 || > + get_unaligned_be16(&app_integrity->row_number) != 0x01ff) { I could be wrong but it looks there are a lot of changes un-related to the sparse warning. I am not opposed to this per se but they should be noted in the log message. [...]
On Thu, Jan 22, 2015 at 01:31:29AM -0800, Jeremiah Mahler wrote: > I wasn't able to reproduce the sparse warning. Was it for a specific > architecture? Perhaps include a link to the email sent by the kbuild > test robot. You need to add a "CF=-D__CHECK_ENDIAN__" to turn these on. make C=2 CF=-D__CHECK_ENDIAN__ http://lwn.net/Articles/205624/ regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Jeremiah Mahler [mailto:jmmahler@gmail.com] > Sent: 2015?1?22? 17:31 > To: Dudley Du > Cc: dmitry.torokhov@gmail.com; rydberg@euromail.se; bleung@google.com; > dan.carpenter@oracle.com; linux-input@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/2] input: cyapa: fix sparse warning issue of incorrect type in > assiggment > > Dudley, > > Mis-spelled 'assiggment' in Subject line. Thanks for spotting it. > > On Thu, Jan 22, 2015 at 02:50:05PM +0800, Dudley Du wrote: > > Fixes the sparse warning issue of the incorrect type in assignment which > > found by the kbuild test robot. > > > I wasn't able to reproduce the sparse warning. Was it for a specific > architecture? Perhaps include a link to the email sent by the kbuild > test robot. Thanks. The kbuild test robot sends automat email about the warning. Based on Dimtry's modification and help, I generated the patch. and tested locally with the command 'make C=1 DF="-D__CHECK_ENDIAN__"', not issue found currently. > > [...] > > struct gen5_bl_metadata_row_params { > > __le16 size; > > -__le16 maximun_size; > > +__le16 maximum_size; > I don't think sparse cares about variable names. > Be sure to note these changes in your log. This is a spell mistake issue, not sparse warning issue. Thanks. > > > __le32 app_start; > > __le16 app_len; > > __le16 app_crc; > > @@ -1192,67 +1192,69 @@ static int cyapa_gen5_bl_enter(struct cyapa > *cyapa) > > static int cyapa_gen5_check_fw(struct cyapa *cyapa, const struct firmware > *fw) > > { > > struct device *dev = &cyapa->client->dev; > > -struct gen5_bl_metadata_row_params metadata; > > -struct cyapa_tsg_bin_image *image; > > -int flash_records_count; > > -u16 app_crc = 0; > > -u16 app_integrity_crc = 0; > > -u16 row_num; > > -u8 *data; > > +const struct cyapa_tsg_bin_image *image = (const void *)fw->data; > > +const struct cyapa_tsg_bin_image_data_record *app_integrity; > > +const struct gen5_bl_metadata_row_params *metadata; > > +size_t flash_records_count; > > +u32 fw_app_start, fw_upgrade_start; > > +u16 fw_app_len, fw_upgrade_len; > > +u16 app_crc; > > +u16 app_integrity_crc; > > int record_index; > > int i; > > > > -image = (struct cyapa_tsg_bin_image *)fw->data; > > flash_records_count = (fw->size - > > sizeof(struct cyapa_tsg_bin_image_head)) / > > sizeof(struct cyapa_tsg_bin_image_data_record); > > > > -/* APP_INTEGRITY row is always the last row block, > > - * and the row id must be 0x01ff */ > > -row_num = get_unaligned_be16( > > -&image->records[flash_records_count - 1].row_number); > > -if (image->records[flash_records_count - 1].flash_array_id != 0x00 && > > -row_num != 0x01ff) { > > +/* > > + * APP_INTEGRITY row is always the last row block, > > + * and the row id must be 0x01ff. > > + */ > > +app_integrity = &image->records[flash_records_count - 1]; > > + > > +if (app_integrity->flash_array_id != 0x00 || > > + get_unaligned_be16(&app_integrity->row_number) != 0x01ff) { > I could be wrong but it looks there are a lot of changes un-related to > the sparse warning. I am not opposed to this per se but they should be > noted in the log message. Understood. Indeed, excepted the spell error, the purpose of the changes are aimed to fix the warning issue. Do I need to add the log message and re-submit the patch or in other method ot change this? Thanks. > > [...] > > -- > - Jeremiah Mahler This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@oracle.com] > Sent: 2015?1?22? 17:55 > To: Jeremiah Mahler; Dudley Du; dmitry.torokhov@gmail.com; > rydberg@euromail.se; bleung@google.com; linux-input@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/2] input: cyapa: fix sparse warning issue of incorrect type in > assiggment > > On Thu, Jan 22, 2015 at 01:31:29AM -0800, Jeremiah Mahler wrote: > > I wasn't able to reproduce the sparse warning. Was it for a specific > > architecture? Perhaps include a link to the email sent by the kbuild > > test robot. > > You need to add a "CF=-D__CHECK_ENDIAN__" to turn these on. > > make C=2 CF=-D__CHECK_ENDIAN__ Yes, thanks. I verified it locally, no issue found currently. > > http://lwn.net/Articles/205624/ > > regards, > dan carpenter This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 22, 2015 at 12:54:53PM +0300, Dan Carpenter wrote: > On Thu, Jan 22, 2015 at 01:31:29AM -0800, Jeremiah Mahler wrote: > > I wasn't able to reproduce the sparse warning. Was it for a specific > > architecture? Perhaps include a link to the email sent by the kbuild > > test robot. > > You need to add a "CF=-D__CHECK_ENDIAN__" to turn these on. > > make C=2 CF=-D__CHECK_ENDIAN__ > > http://lwn.net/Articles/205624/ > > regards, > dan carpenter > Even with __CHECK_ENDIAN__ I wasn't able to reproduce these errors straight away because this fix is already in next. But once reverted they appeared and the patch does fix all of them. jeri@hudson:~/linux-next$ git checkout 2be7256fe7f jeri@hudson:~/linux-next$ git reset --hard HEAD~1 jeri@hudson:~/linux-next$ make C=1 "CF=-D__CHECK_ENDIAN__" (errors ...)
Dudley, On Thu, Jan 22, 2015 at 09:57:42AM +0000, Dudley Du wrote: > > -----Original Message----- > > From: Jeremiah Mahler [mailto:jmmahler@gmail.com] > > Sent: 2015?1?22? 17:31 > > To: Dudley Du [...] > > > struct gen5_bl_metadata_row_params { > > > __le16 size; > > > -__le16 maximun_size; > > > +__le16 maximum_size; > > I don't think sparse cares about variable names. > > Be sure to note these changes in your log. > > This is a spell mistake issue, not sparse warning issue. > Thanks. > That is fine but just be sure to note that in the log. I get suspicious when there are changes that aren't mentioned in the log. [...] > > > + > > > +if (app_integrity->flash_array_id != 0x00 || > > > + get_unaligned_be16(&app_integrity->row_number) != 0x01ff) { > > I could be wrong but it looks there are a lot of changes un-related to > > the sparse warning. I am not opposed to this per se but they should be > > noted in the log message. > > Understood. > Indeed, excepted the spell error, the purpose of the changes are aimed to fix the warning issue. > Do I need to add the log message and re-submit the patch or in other method ot change this? > Thanks. > Once I was able to reproduce the error there were quite a few. So this number of changes seems reasonable. The fix is already in -next so I don't think it is a big enough problem to worry about.
diff --git a/drivers/input/mouse/cyapa_gen5.c b/drivers/input/mouse/cyapa_gen5.c index ced2a2c..69d9059 100644 --- a/drivers/input/mouse/cyapa_gen5.c +++ b/drivers/input/mouse/cyapa_gen5.c @@ -319,7 +319,7 @@ struct gen5_bl_initiate_cmd_data { struct gen5_bl_metadata_row_params { __le16 size; - __le16 maximun_size; + __le16 maximum_size; __le32 app_start; __le16 app_len; __le16 app_crc; @@ -1192,67 +1192,69 @@ static int cyapa_gen5_bl_enter(struct cyapa *cyapa) static int cyapa_gen5_check_fw(struct cyapa *cyapa, const struct firmware *fw) { struct device *dev = &cyapa->client->dev; - struct gen5_bl_metadata_row_params metadata; - struct cyapa_tsg_bin_image *image; - int flash_records_count; - u16 app_crc = 0; - u16 app_integrity_crc = 0; - u16 row_num; - u8 *data; + const struct cyapa_tsg_bin_image *image = (const void *)fw->data; + const struct cyapa_tsg_bin_image_data_record *app_integrity; + const struct gen5_bl_metadata_row_params *metadata; + size_t flash_records_count; + u32 fw_app_start, fw_upgrade_start; + u16 fw_app_len, fw_upgrade_len; + u16 app_crc; + u16 app_integrity_crc; int record_index; int i; - image = (struct cyapa_tsg_bin_image *)fw->data; flash_records_count = (fw->size - sizeof(struct cyapa_tsg_bin_image_head)) / sizeof(struct cyapa_tsg_bin_image_data_record); - /* APP_INTEGRITY row is always the last row block, - * and the row id must be 0x01ff */ - row_num = get_unaligned_be16( - &image->records[flash_records_count - 1].row_number); - if (image->records[flash_records_count - 1].flash_array_id != 0x00 && - row_num != 0x01ff) { + /* + * APP_INTEGRITY row is always the last row block, + * and the row id must be 0x01ff. + */ + app_integrity = &image->records[flash_records_count - 1]; + + if (app_integrity->flash_array_id != 0x00 || + get_unaligned_be16(&app_integrity->row_number) != 0x01ff) { dev_err(dev, "%s: invalid app_integrity data.\n", __func__); return -EINVAL; } - data = image->records[flash_records_count - 1].record_data; - - metadata.app_start = get_unaligned_le32(&data[4]); - metadata.app_len = get_unaligned_le16(&data[8]); - metadata.app_crc = get_unaligned_le16(&data[10]); - metadata.upgrade_start = get_unaligned_le32(&data[16]); - metadata.upgrade_len = get_unaligned_le16(&data[20]); - metadata.metadata_crc = get_unaligned_le16(&data[60]); - - if ((metadata.app_start + metadata.app_len + - metadata.upgrade_start + metadata.upgrade_len) % - CYAPA_TSG_FW_ROW_SIZE) { - dev_err(dev, "%s: invalid image alignment.\n", __func__); - return -EINVAL; - } + + metadata = (const void *)app_integrity->record_data; /* Verify app_integrity crc */ - app_integrity_crc = crc_itu_t(0xffff, data, - CYAPA_TSG_APP_INTEGRITY_SIZE); - if (app_integrity_crc != metadata.metadata_crc) { + app_integrity_crc = crc_itu_t(0xffff, app_integrity->record_data, + CYAPA_TSG_APP_INTEGRITY_SIZE); + if (app_integrity_crc != get_unaligned_le16(&metadata->metadata_crc)) { dev_err(dev, "%s: invalid app_integrity crc.\n", __func__); return -EINVAL; } + fw_app_start = get_unaligned_le32(&metadata->app_start); + fw_app_len = get_unaligned_le16(&metadata->app_len); + fw_upgrade_start = get_unaligned_le32(&metadata->upgrade_start); + fw_upgrade_len = get_unaligned_le16(&metadata->upgrade_len); + + if (fw_app_start % CYAPA_TSG_FW_ROW_SIZE || + fw_app_len % CYAPA_TSG_FW_ROW_SIZE || + fw_upgrade_start % CYAPA_TSG_FW_ROW_SIZE || + fw_upgrade_len % CYAPA_TSG_FW_ROW_SIZE) { + dev_err(dev, "%s: invalid image alignment.\n", __func__); + return -EINVAL; + } + /* * Verify application image CRC */ - record_index = metadata.app_start / CYAPA_TSG_FW_ROW_SIZE - + record_index = fw_app_start / CYAPA_TSG_FW_ROW_SIZE - CYAPA_TSG_IMG_START_ROW_NUM; - data = (u8 *)&image->records[record_index].record_data; - app_crc = crc_itu_t(0xffff, data, CYAPA_TSG_FW_ROW_SIZE); - for (i = 1; i < (metadata.app_len / CYAPA_TSG_FW_ROW_SIZE); i++) { - data = (u8 *)&image->records[++record_index].record_data; + app_crc = 0xffffU; + for (i = 0; i < fw_app_len / CYAPA_TSG_FW_ROW_SIZE; i++) { + const u8 *data = image->records[record_index + i].record_data; + app_crc = crc_itu_t(app_crc, data, CYAPA_TSG_FW_ROW_SIZE); } - if (app_crc != metadata.app_crc) { + if (app_crc != get_unaligned_le16(&metadata->app_crc)) { dev_err(dev, "%s: invalid firmware app crc check.\n", __func__); return -EINVAL; }
Fixes the sparse warning issue of the incorrect type in assignment which found by the kbuild test robot. Signed-off-by: Dudley Du <dudl@cypress.com> --- drivers/input/mouse/cyapa_gen5.c | 80 ++++++++++++++++++++-------------------- 1 file changed, 41 insertions(+), 39 deletions(-)