[PATCH 1/2] input: cyapa: fix sparse warning issue of incorrect type in assiggment

[PATCH 1/2] input: cyapa: fix sparse warning issue of incorrect type in assiggment

[Dudley Du]

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(-)

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;
 	}
-- 
1.9.1

[PATCH 2/2] input: cyapa: fix variable dereferenced before check 'gen5_pip->resp_len' issue

[Dudley Du]

Fixes the warning issue of the variable dereferenced before check
'gen5_pip->resp_len' report by dan Carpenter.

Signed-off-by: Dudley Du <dudl@cypress.com>
---
 drivers/input/mouse/cyapa_gen5.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/input/mouse/cyapa_gen5.c b/drivers/input/mouse/cyapa_gen5.c
index 69d9059..ed1af74 100644
--- a/drivers/input/mouse/cyapa_gen5.c
+++ b/drivers/input/mouse/cyapa_gen5.c
@@ -2558,7 +2558,9 @@ static bool cyapa_gen5_irq_cmd_handler(struct cyapa *cyapa)
 			 * trackpad device when booting/rebooting
 			 * their chrome book.
 			 */
-			length = *gen5_pip->resp_len;
+			length = 0;
+			if (gen5_pip->resp_len)
+				length = *gen5_pip->resp_len;
 			cyapa_empty_pip_output_data(cyapa,
 					gen5_pip->resp_data,
 					&length,
-- 
1.9.1

Re: [PATCH 1/2] input: cyapa: fix sparse warning issue of incorrect type in assiggment

[Jeremiah Mahler]

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.

[...]

-- 
- Jeremiah Mahler

Re: [PATCH 1/2] input: cyapa: fix sparse warning issue of incorrect type in assiggment

[Dan Carpenter]

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

RE: [PATCH 1/2] input: cyapa: fix sparse warning issue of incorrect type in assiggment

[Dudley Du]

> -----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.

RE: [PATCH 1/2] input: cyapa: fix sparse warning issue of incorrect type in assiggment

[Dudley Du]

> -----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.

Re: [PATCH 2/2] input: cyapa: fix variable dereferenced before check 'gen5_pip->resp_len' issue

[Dmitry Torokhov]

On Thu, Jan 22, 2015 at 02:50:06PM +0800, Dudley Du wrote:
> Fixes the warning issue of the variable dereferenced before check
> 'gen5_pip->resp_len' report by dan Carpenter.
> 
> Signed-off-by: Dudley Du <dudl@cypress.com>

Applied, thank you.

> ---
>  drivers/input/mouse/cyapa_gen5.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/mouse/cyapa_gen5.c b/drivers/input/mouse/cyapa_gen5.c
> index 69d9059..ed1af74 100644
> --- a/drivers/input/mouse/cyapa_gen5.c
> +++ b/drivers/input/mouse/cyapa_gen5.c
> @@ -2558,7 +2558,9 @@ static bool cyapa_gen5_irq_cmd_handler(struct cyapa *cyapa)
>  			 * trackpad device when booting/rebooting
>  			 * their chrome book.
>  			 */
> -			length = *gen5_pip->resp_len;
> +			length = 0;
> +			if (gen5_pip->resp_len)
> +				length = *gen5_pip->resp_len;
>  			cyapa_empty_pip_output_data(cyapa,
>  					gen5_pip->resp_data,
>  					&length,
> -- 
> 1.9.1
> 

-- 
Dmitry

Re: [PATCH 1/2] input: cyapa: fix sparse warning issue of incorrect type in assiggment

[Jeremiah Mahler]

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 ...)

-- 
- Jeremiah Mahler

Re: [PATCH 1/2] input: cyapa: fix sparse warning issue of incorrect type in assiggment

[Jeremiah Mahler]

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.

-- 
- Jeremiah Mahler